Re: [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 9/10/20 4:32 PM, Aaron Lauterer wrote:
> Functionality has been added for the following storage types:
> 
> * dir based ones
>     * directory
>     * NFS
>     * CIFS
>     * gluster
> * ZFS
> * (thin) LVM
> * Ceph
> 
> A new feature `reassign` has been introduced to mark which storage
> plugin supports the feature.
> 
> A new intermediate class for directory based storages has been
> introduced. This was necessary to maintain compatibility with third
> party storage plugins and to avoid duplicate code in the dir based
> plugins.
> 
> The new `BaseDirPlugin.pm` adds the `reassign` feature flag and
> containes the implementation for the reassign functionlity.
> 
> In the future all the directory specific code in Plugin.pm should be
> moved to the BaseDirPlugin.pm. But this will most likely break
> compatibility with third party plugins and should thus be done with
> care.

how so? why don't you just add it in plugin.pm with either:
* a general directory based implementation, and a no-op/die for the other
  ones
* a dummy "die implement me in subclass" method if above is not possible

Then increase the ABI version AND age to allow external plugins to tell
if they can or should implement this themself.

As is you do not allow any 3rd party plugin to provide their own method 
for this (they cannot know when it's even used) and those without it
get an error as the Storage::reassign_volume one plainly calls into
$class->reassign_volume which, as no default module is there, may fail
much uglier than a `die "not implemented in class $class\n"` excetpion.

> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> v2 -> v3:
> * added feature flags instead of dummy "not implemented" methods in
>   plugins which do not support it as that would break compatibility with
>   3rd party plugins.
>   Had to make $features available outside the `has_features` method in
>   Plugins.pm in order to be able to individually add features in the
>   `BaseDirPlugin.pm`.

no, this is not good and has bad side effects too, FWICT, as it changes
a variable for all api calls in a worker, which then is changed for all
plugins... NAK.

rather overwrite the has feature method, catch the specific case you need
(reassign) and pass the other stuff back to the parent (SUPER) module

> * added the BaseDirPlugin.pm to maintain compat with 3rd party plugins,
>   this is explained in the commit message
> * moved the actual renaming from the `reassign_volume` to a dedicated
>   `rename_volume` method to make this functionality available to other
>   possible uses in the future.
> * added support for linked clones ($base)
> 
> 
> rfc -> v1 -> v2: nothing changed
> 
> 
>  PVE/Storage.pm                 | 10 ++++++
>  PVE/Storage/BaseDirPlugin.pm   | 61 ++++++++++++++++++++++++++++++++++
>  PVE/Storage/CIFSPlugin.pm      |  2 +-
>  PVE/Storage/DirPlugin.pm       |  2 +-
>  PVE/Storage/GlusterfsPlugin.pm |  2 +-
>  PVE/Storage/LVMPlugin.pm       | 24 +++++++++++++
>  PVE/Storage/LvmThinPlugin.pm   |  1 +
>  PVE/Storage/Makefile           |  1 +
>  PVE/Storage/NFSPlugin.pm       |  2 +-
>  PVE/Storage/Plugin.pm          | 22 ++++++------
>  PVE/Storage/RBDPlugin.pm       | 31 +++++++++++++++++
>  PVE/Storage/ZFSPoolPlugin.pm   | 28 ++++++++++++++++
>  12 files changed, 172 insertions(+), 14 deletions(-)
>  create mode 100644 PVE/Storage/BaseDirPlugin.pm
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 4a60615..919c606 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1759,6 +1759,16 @@ sub complete_volume {
>      return $res;
>  }
>  
> +sub reassign_volume {
> +    my ($cfg, $volid, $target_vmid) = @_;
> +
> +    my ($storeid, $volname) = parse_volume_id($volid);
> +    my $scfg = storage_config($cfg, $storeid);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> +    return $plugin->reassign_volume($scfg, $storeid, $volname, $target_vmid);
> +}
> +
>  # 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/BaseDirPlugin.pm b/PVE/Storage/BaseDirPlugin.pm
> new file mode 100644
> index 0000000..b7f4ae4
> --- /dev/null
> +++ b/PVE/Storage/BaseDirPlugin.pm
> @@ -0,0 +1,61 @@
> +package PVE::Storage::BaseDirPlugin;
> +
> +use strict;
> +use warnings;
> +
> +use File::Path;
> +use Data::Dumper;
> +
> +use base qw(PVE::Storage::Plugin);
> +
> +$PVE::Storage::Plugin::features->{reassign} = { current => {
> +	    'raw' => 1,
> +	    'qcow2'=> 1,
> +	    'vmdk' => 1,
> +	}
> +    };
> +
> +sub reassign_volume {
> +    my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
> +
> +    my $base;
> +    my $base_vmid;
> +    my $format;
> +    my $source_vmid;
> +
> +    (undef, $volname, $source_vmid, $base, $base_vmid, undef, $format) = $class->parse_volname($volname);
> +
> +    # parse_volname strips the directory from volname
> +    my $source_volname = "${source_vmid}/${volname}";
> +
> +    if ($base) {
> +	$base = "${base_vmid}/${base}/";
> +    } else {
> +	$base = '';
> +    }
> +
> +    $class->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
> +	my $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1);
> +
> +	return $class->rename_volume($scfg, $storeid, $source_volname, $target_volname, $target_vmid, $base);
> +    });
> +}
> +
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $target_volname, $vmid, $base) = @_;
> +
> +    my $basedir = $class->get_subdir($scfg, 'images');
> +    my $imagedir = "${basedir}/${vmid}";
> +
> +    mkpath $imagedir;
> +
> +    my $old_path = "${basedir}/${source_volname}";
> +    my $new_path = "${imagedir}/${target_volname}";
> +
> +    rename($old_path, $new_path) ||
> +	die "rename '$old_path' to '$new_path' failed - $!\n";
> +
> +    return "${storeid}:${base}${vmid}/${target_volname}";
> +}
> +
> +1;
> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
> index 6edbc9d..d9714dd 100644
> --- a/PVE/Storage/CIFSPlugin.pm
> +++ b/PVE/Storage/CIFSPlugin.pm
> @@ -9,7 +9,7 @@ use File::Path;
>  use PVE::Storage::Plugin;
>  use PVE::JSONSchema qw(get_standard_option);
>  
> -use base qw(PVE::Storage::Plugin);
> +use base qw(PVE::Storage::BaseDirPlugin);
>  
>  # CIFS helper functions
>  
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index 3c81d24..66b590e 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -7,7 +7,7 @@ use File::Path;
>  use PVE::Storage::Plugin;
>  use PVE::JSONSchema qw(get_standard_option);
>  
> -use base qw(PVE::Storage::Plugin);
> +use base qw(PVE::Storage::BaseDirPlugin);
>  
>  # Configuration
>  
> diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
> index 2dd414d..14111e6 100644
> --- a/PVE/Storage/GlusterfsPlugin.pm
> +++ b/PVE/Storage/GlusterfsPlugin.pm
> @@ -10,7 +10,7 @@ use PVE::Network;
>  use PVE::Storage::Plugin;
>  use PVE::JSONSchema qw(get_standard_option);
>  
> -use base qw(PVE::Storage::Plugin);
> +use base qw(PVE::Storage::BaseDirPlugin);
>  
>  # Glusterfs helper functions
>  
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index c0740d4..4203eda 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -337,6 +337,13 @@ 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($cmd, errmsg => "lvrename '${vg}/${oldname}' to '${newname}' error");
> +}
> +
>  sub alloc_image {
>      my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
>  
> @@ -581,6 +588,7 @@ sub volume_has_feature {
>  
>      my $features = {
>  	copy => { base => 1, current => 1},
> +	reassign => {current => 1},
>      };
>  
>      my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> @@ -681,4 +689,20 @@ sub volume_import_write {
>  	input => '<&'.fileno($input_fh));
>  }
>  
> +sub reassign_volume {
> +    my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
> +
> +    $class->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
> +	my $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid);
> +	return $class->rename_volume($scfg, $storeid, $volname, $target_volname, $target_vmid);
> +    });
> +}
> +
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $target_volname, $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 d1c5b1f..adc4ae2 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},
> +	reassign => {current => 1},
>      };
>  
>      my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> diff --git a/PVE/Storage/Makefile b/PVE/Storage/Makefile
> index 5b8f6e8..936fbf2 100644
> --- a/PVE/Storage/Makefile
> +++ b/PVE/Storage/Makefile
> @@ -1,5 +1,6 @@
>  SOURCES= \
>  	Plugin.pm \
> +	BaseDirPlugin.pm \
>  	DirPlugin.pm \
>  	LVMPlugin.pm \
>  	NFSPlugin.pm \
> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
> index 6abb24b..f91decc 100644
> --- a/PVE/Storage/NFSPlugin.pm
> +++ b/PVE/Storage/NFSPlugin.pm
> @@ -10,7 +10,7 @@ use PVE::ProcFSTools;
>  use PVE::Storage::Plugin;
>  use PVE::JSONSchema qw(get_standard_option);
>  
> -use base qw(PVE::Storage::Plugin);
> +use base qw(PVE::Storage::BaseDirPlugin);
>  
>  # NFS helper functions
>  
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index e6cd99c..581c581 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -888,19 +888,20 @@ sub storage_can_replicate {
>      return 0;
>  }
>  
> +our $features = {
> +    snapshot => { current => { qcow2 => 1}, snap => { qcow2 => 1} },
> +    clone => { base => {qcow2 => 1, raw => 1, vmdk => 1} },
> +    template => { current => {qcow2 => 1, raw => 1, vmdk => 1, subvol => 1} },
> +    copy => { base => {qcow2 => 1, raw => 1, vmdk => 1},
> +	      current => {qcow2 => 1, raw => 1, vmdk => 1},
> +	      snap => {qcow2 => 1} },
> +    sparseinit => { base => {qcow2 => 1, raw => 1, vmdk => 1},
> +		    current => {qcow2 => 1, raw => 1, vmdk => 1} },
> +};
> +
>  sub volume_has_feature {
>      my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
>  
> -    my $features = {
> -	snapshot => { current => { qcow2 => 1}, snap => { qcow2 => 1} },
> -	clone => { base => {qcow2 => 1, raw => 1, vmdk => 1} },
> -	template => { current => {qcow2 => 1, raw => 1, vmdk => 1, subvol => 1} },
> -	copy => { base => {qcow2 => 1, raw => 1, vmdk => 1},
> -		  current => {qcow2 => 1, raw => 1, vmdk => 1},
> -		  snap => {qcow2 => 1} },
> -	sparseinit => { base => {qcow2 => 1, raw => 1, vmdk => 1},
> -			current => {qcow2 => 1, raw => 1, vmdk => 1} },
> -    };
>  
>      # clone_image creates a qcow2 volume
>      return 0 if $feature eq 'clone' &&
> @@ -1411,4 +1412,5 @@ sub volume_import_formats {
>      return ();
>  }
>  
> +
>  1;
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 38f2b46..325937a 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -703,6 +703,7 @@ sub volume_has_feature {
>  	template => { current => 1},
>  	copy => { base => 1, current => 1, snap => 1},
>  	sparseinit => { base => 1, current => 1},
> +	reassign => {current => 1},
>      };
>  
>      my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> @@ -719,4 +720,34 @@ sub volume_has_feature {
>      return undef;
>  }
>  
> +sub reassign_volume {
> +    my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
> +
> +    my $base;;
> +    (undef, $volname, undef, $base) = $class->parse_volname($volname);
> +
> +    if ($base) {
> +	$base = $base . '/';
> +    } else {
> +	$base = '';
> +    }
> +
> +    $class->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
> +	my $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid);
> +	return $class->rename_volume($scfg, $storeid, $volname, $target_volname, $target_vmid, $base);
> +    });
> +}
> +
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $target_volname, $vmid, $base) = @_;
> +
> +    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}'",
> +    );
> +    return "${storeid}:${base}${target_volname}";
> +}
> +
>  1;
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 10354b3..9f5f055 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -9,6 +9,8 @@ use PVE::Storage::Plugin;
>  use PVE::RPCEnvironment;
>  use Net::IP;
>  
> +use Data::Dumper;
> +
>  use base qw(PVE::Storage::Plugin);
>  
>  sub type {
> @@ -647,6 +649,7 @@ sub volume_has_feature {
>  	copy => { base => 1, current => 1},
>  	sparseinit => { base => 1, current => 1},
>  	replicate => { base => 1, current => 1},
> +	reassign => {current => 1},
>      };
>  
>      my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> @@ -749,4 +752,29 @@ sub volume_import_formats {
>      return $class->volume_export_formats($scfg, $storeid, $volname, undef, $base_snapshot, $with_snapshots);
>  }
>  
> +sub reassign_volume {
> +    my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
> +
> +    my $base;;
> +    (undef, $volname, undef, $base) = $class->parse_volname($volname);
> +
> +    if ($base) {
> +	$base = $base . '/';
> +    } else {
> +	$base = '';
> +    }
> +
> +    $class->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
> +	my $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid);
> +	return $class->rename_volume($scfg, $storeid, $volname, $target_volname, $target_vmid, $base);
> +    });
> +}
> +
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $target_volname, $vmid, $base) = @_;
> +
> +    $class->zfs_request($scfg, 5, 'rename', "$scfg->{pool}/$source_volname", "$scfg->{pool}/$target_volname");
> +    return "${storeid}:${base}${target_volname}";
> +}
> +
>  1;
>