Re: [pve-devel] [RFC storage 2/7] add disk rename feature

[Fabian Ebner <f.ebner@proxmox.com>]

Am 01.06.21 um 18:10 schrieb Aaron Lauterer:
> Functionality has been added for the following storage types:
> 
> * dir based ones
>      * directory
>      * NFS
>      * CIFS
>      * gluster
> * ZFS
> * (thin) LVM
> * Ceph
> 
> A new feature `rename` has been introduced to mark which storage
> plugin supports the feature.
> 
> Version API and AGE have been bumped.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> changes:
> only do rename now but the rename function handles templates and returns
> the new volid as this can be differently handled on some storages.
> 
>   PVE/Storage.pm               | 19 +++++++++++++++++--
>   PVE/Storage/LVMPlugin.pm     | 18 ++++++++++++++++++
>   PVE/Storage/LvmThinPlugin.pm |  1 +
>   PVE/Storage/Plugin.pm        | 29 +++++++++++++++++++++++++++++
>   PVE/Storage/RBDPlugin.pm     | 16 ++++++++++++++++
>   PVE/Storage/ZFSPoolPlugin.pm | 12 ++++++++++++
>   6 files changed, 93 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 93d09ce..6936abd 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -40,11 +40,11 @@ use PVE::Storage::ZFSPlugin;
>   use PVE::Storage::PBSPlugin;
>   
>   # Storage API version. Increment it on changes in storage API interface.
> -use constant APIVER => 8;
> +use constant APIVER => 9;
>   # Age is the number of versions we're backward compatible with.
>   # This is like having 'current=APIVER' and age='APIAGE' in libtool,
>   # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> -use constant APIAGE => 7;
> +use constant APIAGE => 8;
>   
>   # load standard plugins
>   PVE::Storage::DirPlugin->register();
> @@ -355,6 +355,7 @@ sub volume_snapshot_needs_fsfreeze {
>   #            snapshot - taking a snapshot is possible
>   #            sparseinit - volume is sparsely initialized
>   #            template - conversion to base image is possible
> +#            rename - renaming volumes is possible
>   # $snap - check if the feature is supported for a given snapshot
>   # $running - if the guest owning the volume is running
>   # $opts - hash with further options:
> @@ -1849,6 +1850,20 @@ sub complete_volume {
>       return $res;
>   }
>   
> +sub rename_volume {
> +    my ($cfg, $source_volid, $source_vmid, $target_volname, $target_vmid) = @_;

Can't the vmid arguments be dropped and extracted directly from the 
volid/volname instead? Would prevent potential mismatch for careless 
callers.

> +
> +    my ($storeid) = parse_volume_id($source_volid);
> +    my (undef, $source_volname, undef, $base_name, $base_vmid, $isBase, $format) = parse_volname($cfg, $source_volid);
> +
> +    activate_storage($cfg, $storeid);
> +
> +    my $scfg = storage_config($cfg, $storeid);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +

Check that target_volname is valid (by parsing) either here or within 
each plugin's implementation?

> +    return $plugin->rename_volume($scfg, $storeid, $source_volname, $source_vmid, $target_volname, $target_vmid, $base_name, $base_vmid);

Similar here, not all plugins need all of those parameters. Isn't taking 
$scfg, $storeid, $source_volid, $target_volname enough and let the 
plugin itself extract additional information if needed?

> +}
> +
>   # Various io-heavy operations require io/bandwidth limits which can be
>   # configured on multiple levels: The global defaults in datacenter.cfg, and
>   # per-storage overrides. When we want to do a restore from storage A to storage
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index df49b76..977e6a4 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -339,6 +339,16 @@ sub lvcreate {
>       run_command($cmd, errmsg => "lvcreate '$vg/$name' error");
>   }
>   
> +sub lvrename {
> +    my ($vg, $oldname, $newname) = @_;
> +
> +    my $cmd = ['/sbin/lvrename', $vg, $oldname, $newname];
> +    run_command(
> +	['/sbin/lvrename', $vg, $oldname, $newname],
> +	errmsg => "lvrename '${vg}/${oldname}' to '${newname}' error"
> +    );
> +}
> +
>   sub alloc_image {
>       my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
>   
> @@ -584,6 +594,7 @@ sub volume_has_feature {
>   
>       my $features = {
>   	copy => { base => 1, current => 1},
> +	rename => {current => 1},
>       };
>   
>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> @@ -684,4 +695,11 @@ sub volume_import_write {
>   	input => '<&'.fileno($input_fh));
>   }
>   
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $source_vmid, $target_volname, $target_vmid, $base_name, $base_vmid) = @_;
> +
> +    lvrename($scfg->{vgname}, $source_volname, $target_volname);
> +    return "${storeid}:${target_volname}";
> +}
> +
>   1;
> diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
> index c9e127c..45ad0ad 100644
> --- a/PVE/Storage/LvmThinPlugin.pm
> +++ b/PVE/Storage/LvmThinPlugin.pm
> @@ -355,6 +355,7 @@ sub volume_has_feature {
>   	template => { current => 1},
>   	copy => { base => 1, current => 1, snap => 1},
>   	sparseinit => { base => 1, current => 1},
> +	rename => {current => 1},
>       };
>   
>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 4a10a1f..4e6e288 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -939,6 +939,7 @@ sub volume_has_feature {
>   		  snap => {qcow2 => 1} },
>   	sparseinit => { base => {qcow2 => 1, raw => 1, vmdk => 1},
>   			current => {qcow2 => 1, raw => 1, vmdk => 1} },
> +	rename => { current =>{qcow2 => 1, raw => 1, vmdk => 1} },
>       };
>   
>       # clone_image creates a qcow2 volume
> @@ -946,6 +947,10 @@ sub volume_has_feature {
>   		defined($opts->{valid_target_formats}) &&
>   		!(grep { $_ eq 'qcow2' } @{$opts->{valid_target_formats}});
>   
> +    return 0 if $feature eq 'rename'
> +	&& $class->can('api')
> +	&& $class->api() < 9;

Style-nit: multiline post-if

> +
>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
>   	$class->parse_volname($volname);
>   
> @@ -1463,4 +1468,28 @@ sub volume_import_formats {
>       return ();
>   }
>   
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $source_vmid, $target_volname, $target_vmid, $base_name, $base_vmid) = @_;
> +    die "not implemented in storage plugin '$class'\n" if $class->can('api') && $class->api() < 9;
> +
> +    my $basedir = $class->get_subdir($scfg, 'images');
> +    my $sourcedir = "${basedir}/${source_vmid}";
> +    my $targetdir = "${basedir}/${target_vmid}";
> +
> +    mkpath $targetdir;
> +
> +    my (undef, $format, undef) = parse_name_dir($source_volname);
> +    $target_volname = "${target_volname}.${format}";

Ideally, the $target_volname already contains the format suffix at this 
point. Otherwise, passing a $target_volname with a format suffix results 
in a second suffix.

> +
> +    my $old_path = "${sourcedir}/${source_volname}";
> +    my $new_path = "${targetdir}/${target_volname}";
> +
> +    my $base = $base_name ? "${base_vmid}/${base_name}/" : '';
> +
> +    rename($old_path, $new_path) ||
> +	die "rename '$old_path' to '$new_path' failed - $!\n";
> +
> +    return "${storeid}:${base}${target_vmid}/${target_volname}";
> +}
> +
>   1;
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index a8d1243..6e9bf8f 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -728,6 +728,7 @@ sub volume_has_feature {
>   	template => { current => 1},
>   	copy => { base => 1, current => 1, snap => 1},
>   	sparseinit => { base => 1, current => 1},
> +	rename => {current => 1},
>       };
>   
>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) = $class->parse_volname($volname);
> @@ -743,4 +744,19 @@ sub volume_has_feature {
>       return undef;
>   }
>   
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $source_vmid, $target_volname, $target_vmid, $base_name, $base_vmid) = @_;
> +
> +    my $cmd = $rbd_cmd->($scfg, $storeid, 'rename', $source_volname, $target_volname);
> +
> +    run_rbd_command(
> +	$cmd,
> +	errmsg => "could not rename image '${source_volname}' to '${target_volname}'",
> +    );
> +
> +    $base_name = $base_name ? "${base_name}/" : '';
> +
> +    return "${storeid}:${base_name}${target_volname}";
> +}
> +
>   1;
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 2e2abe3..f04f443 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -687,6 +687,7 @@ sub volume_has_feature {
>   	copy => { base => 1, current => 1},
>   	sparseinit => { base => 1, current => 1},
>   	replicate => { base => 1, current => 1},
> +	rename => {current => 1},
>       };
>   
>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> @@ -789,4 +790,15 @@ sub volume_import_formats {
>       return $class->volume_export_formats($scfg, $storeid, $volname, undef, $base_snapshot, $with_snapshots);
>   }
>   
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $source_vmid, $target_volname, $target_vmid, $base_name, $base_vmid) = @_;
> +
> +    my $pool = $scfg->{pool};
> +    $class->zfs_request($scfg, 5, 'rename', "${pool}/$source_volname", "${pool}/$target_volname");
> +
> +    $base_name = $base_name ? "${base_name}/" : '';
> +
> +    return "${storeid}:${base_name}${target_volname}";
> +}
> +
>   1;
>