Re: [pve-devel] [PATCH pve-storage 04/10] rename_volume: add source && target snap - DERUMIER, Alexandre via pve-devel

From: "DERUMIER, Alexandre via pve-devel" <pve-devel@lists.proxmox.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>,
	"f.gruenbichler@proxmox.com" <f.gruenbichler@proxmox.com>
Cc: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>,
	"t.lamprecht@proxmox.com" <t.lamprecht@proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-storage 04/10] rename_volume: add source && target snap
Date: Mon, 7 Jul 2025 10:34:21 +0000	[thread overview]
Message-ID: <mailman.1066.1751884473.395.pve-devel@lists.proxmox.com> (raw)
In-Reply-To: <1812738791.2234.1751629948403@webmail.proxmox.com>

[-- Attachment #1: Type: message/rfc822, Size: 14547 bytes --]

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "f.gruenbichler@proxmox.com" <f.gruenbichler@proxmox.com>
Cc: "t.lamprecht@proxmox.com" <t.lamprecht@proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-storage 04/10] rename_volume: add source && target snap
Date: Mon, 7 Jul 2025 10:34:21 +0000
Message-ID: <fc6e2ce2fd3aa270f21224a870416235e064ddb6.camel@groupe-cyllene.com>

>>we could consider adding a new API method `rename_snapshot` instead:
>>
>>my ($class, $scfg, $storeid, $volname, $source_snap, $target_snap) =
>>@_;
>>
>>for the two plugins here it could easily share most of the
>>implementation
>>with rename_volume, without blowing up the interface for a fairly
>>limited
>>use case?
>>
>>rename_volume right now is used for re-assigning a volume from one
>>owner/vmid to another only, AFAICT, with $target_volname never being
>>actually provided by callers. the new call would then never provide
>>$target_vmid and never provide $target_volname, while existing ones
>>never provide the snapshot parameters. OTOH, just like the existing
>>rename_volume, such a rename_snapshot method would only have a
>>single use case/call site, unless we plan to also add generic
>>snapshot renaming as a feature down the line..

ok, no problem, I'll add it

>>another missing piece here is that for external snapshots, we need
>>to actually rename all the volumes when reassigning, else only the
>>current volume is renamed, and the rest still have the old
owner/name..

ah yes, indeed, I'll fix it

> 
>  
>      my (
>          undef, $source_image, $source_vmid, $base_name, $base_vmid,
> undef, $format,
>      ) = $class->parse_volname($source_volname);
> +
> +    $source_image = $class->get_snap_volname($source_volname,
> $source_snap) if $source_snap;

>>if you flip these two around, this could just overwrite
>>$source_volname, because otherwise
>>you rely on $source_image and $source_volname being identical always
>>which might not be
>>the case in the future?
>> or is this more correct in general, in case we ever add base image
>>support to non-thin LVM?


it was mostly because source_volname is used just after to find
target_volname. 
I'll cleanup this in the dedicated rename_snasphot sub


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel