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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

> Alexandre Derumier via pve-devel <pve-devel@lists.proxmox.com> 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 <alexandre.derumier@groupe-cyllene.com>
> ---
>  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