From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B054B1FF164 for ; Fri, 4 Jul 2025 13:51:52 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0A36136CB4; Fri, 4 Jul 2025 13:52:32 +0200 (CEST) Date: Fri, 4 Jul 2025 13:52:28 +0200 (CEST) From: =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= To: Proxmox VE development discussion Message-ID: <1812738791.2234.1751629948403@webmail.proxmox.com> In-Reply-To: References: <20250704064507.511884-1-alexandre.derumier@groupe-cyllene.com> MIME-Version: 1.0 X-Priority: 3 Importance: Normal X-Mailer: Open-Xchange Mailer v7.10.6-Rev79 X-Originating-Client: open-xchange-appsuite X-SPAM-LEVEL: Spam detection results: 0 AWL 0.044 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH pve-storage 04/10] rename_volume: add source && target snap X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: Thomas Lamprecht Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" > Alexandre Derumier via pve-devel hat am 04.07.2025 08:45 CEST geschrieben: > allow to rename from|to external snapshot volname 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.. 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.. > > Signed-off-by: Alexandre Derumier > --- > src/PVE/Storage.pm | 10 ++++++++-- > src/PVE/Storage/LVMPlugin.pm | 17 +++++++++++++++-- > src/PVE/Storage/Plugin.pm | 14 +++++++++++++- > 3 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index 7861bf6..fe6eaf7 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -2319,7 +2319,7 @@ sub complete_volume { > } > > sub rename_volume { > - my ($cfg, $source_volid, $target_vmid, $target_volname) = @_; > + my ($cfg, $source_volid, $target_vmid, $target_volname, $source_snap, $target_snap) = @_; > > die "no source volid provided\n" if !$source_volid; > die "no target VMID or target volname provided\n" if !$target_vmid && !$target_volname; > @@ -2339,7 +2339,13 @@ sub rename_volume { > undef, > sub { > return $plugin->rename_volume( > - $scfg, $storeid, $source_volname, $target_vmid, $target_volname, > + $scfg, > + $storeid, > + $source_volname, > + $target_vmid, > + $target_volname, > + $source_snap, > + $target_snap, > ); > }, > ); > diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm > index 1a992e8..0b506c7 100644 > --- a/src/PVE/Storage/LVMPlugin.pm > +++ b/src/PVE/Storage/LVMPlugin.pm > @@ -838,11 +838,24 @@ sub volume_import_write { > } > > sub rename_volume { > - my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_; > + my ( > + $class, > + $scfg, > + $storeid, > + $source_volname, > + $target_vmid, > + $target_volname, > + $source_snap, > + $target_snap, > + ) = @_; > > 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? > + $target_volname = $class->get_snap_volname($source_volname, $target_snap) if $target_snap; if we keep this, this should assert that $target_volname wasn't provided by the caller.. > + > $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format) > if !$target_volname; > > @@ -851,7 +864,7 @@ sub rename_volume { > die "target volume '${target_volname}' already exists\n" > if ($lvs->{$vg}->{$target_volname}); > > - lvrename($vg, $source_volname, $target_volname); > + lvrename($vg, $source_image, $target_volname); this could then be dropped.. > return "${storeid}:${target_volname}"; > } > > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm > index c35d5e5..5afe29b 100644 > --- a/src/PVE/Storage/Plugin.pm > +++ b/src/PVE/Storage/Plugin.pm > @@ -1877,7 +1877,16 @@ sub volume_import_formats { > } > > sub rename_volume { > - my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_; > + my ( > + $class, > + $scfg, > + $storeid, > + $source_volname, > + $target_vmid, > + $target_volname, > + $source_snap, > + $target_snap, > + ) = @_; > die "not implemented in storage plugin '$class'\n" if $class->can('api') && $class->api() < 10; > die "no path found\n" if !$scfg->{path}; > > @@ -1885,6 +1894,9 @@ sub rename_volume { > undef, $source_image, $source_vmid, $base_name, $base_vmid, undef, $format, > ) = $class->parse_volname($source_volname); > > + $source_image = $class->get_snap_name($source_volname, $source_snap) if $source_snap; > + $target_volname = $class->get_snap_name($source_volname, $target_snap) if $target_snap; same here - this should assert that $target_volname wasn't provided, if we don't split it out into a new sub.. > + > $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1) > if !$target_volname; > > -- > 2.39.5 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel