[pve-devel] [PATCH v6 storage 1/1] add disk reassign feature
Fabian Ebner
f.ebner at proxmox.com
Thu Apr 15 13:07:49 CEST 2021
We often have a generic implementation in Plugin.pm for filesystem-based
storages (maybe erroring out if there is no $scfg->{path}). Is there a
reason not to use such an approach, i.e. making file_reassign_volume the
default reassign_volume implementation in Plugin.pm? This would avoid
some fragmentation/duplication. Of course, the plugins that cannot use
the generic implementation need to provide their own or signal that they
do not have the feature.
More comments inline:
Am 02.04.21 um 12:19 schrieb Aaron Lauterer:
> Functionality has been added for the following storage types:
>
> * dir based ones
> * directory
> * NFS
> * CIFS
> * gluster
What about CephFS? Can it use the generic implementation or not? If it
can and you choose the approach I suggested at the beginning, you don't
even need to add code to it ;)
> * ZFS
> * (thin) LVM
> * Ceph
>
> A new feature `reassign` 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 at proxmox.com>
> ---
> v5 -> v6:
> * refactor dir based feature check to reduce code repetition by
> introducing the file_can_reassign_volume sub that does the actual check
>
> v4 -> v5:
> * rebased on master
> * bumped api ver and api age
> * rephrased "not implemented" message as suggested [0].
>
> v3 -> v4:
> * revert intermediate storage plugin for directory based plugins
> * add a `die "not supported"` method in Plugin.pm
> * dir based plugins now call the file_reassign_volume method in
> Plugin.pm as the generic file/directory based method
> * restored old `volume_has_feature` method in Plugin.pm and override it
> in directory based plugins to check against the new `reassign` feature
> (not too happy about the repetition for each plugin)
>
> 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`.
> * 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
>
> [0] https://lists.proxmox.com/pipermail/pve-devel/2020-November/046031.html
>
>
> PVE/Storage.pm | 15 +++++++--
> PVE/Storage/CIFSPlugin.pm | 13 ++++++++
> PVE/Storage/DirPlugin.pm | 13 ++++++++
> PVE/Storage/GlusterfsPlugin.pm | 13 ++++++++
> PVE/Storage/LVMPlugin.pm | 24 ++++++++++++++
> PVE/Storage/LvmThinPlugin.pm | 1 +
> PVE/Storage/NFSPlugin.pm | 13 ++++++++
> PVE/Storage/Plugin.pm | 60 ++++++++++++++++++++++++++++++++++
> PVE/Storage/RBDPlugin.pm | 31 ++++++++++++++++++
> PVE/Storage/ZFSPoolPlugin.pm | 26 +++++++++++++++
> 10 files changed, 207 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 122c3e9..b950c82 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -41,11 +41,11 @@ use PVE::Storage::DRBDPlugin;
> 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();
> @@ -349,6 +349,7 @@ sub volume_snapshot_needs_fsfreeze {
> # snapshot - taking a snapshot is possible
> # sparseinit - volume is sparsely initialized
> # template - conversion to base image is possible
> +# reassign - reassigning disks to other guest 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:
> @@ -1843,6 +1844,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/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
> index be06cc7..3affa5d 100644
> --- a/PVE/Storage/CIFSPlugin.pm
> +++ b/PVE/Storage/CIFSPlugin.pm
> @@ -293,4 +293,17 @@ sub update_volume_notes {
> PVE::Storage::DirPlugin::update_volume_notes($class, @_);
> }
>
> +sub reassign_volume {
> + my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
> + return $class->file_reassign_volume($scfg, $storeid, $volname, $target_vmid);
> +}
> +
> +sub volume_has_feature {
> + my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
> +
> + shift @_;
> + return $class->file_can_reassign_volume(@_) if $feature eq 'reassign';
> + return $class->SUPER::volume_has_feature(@_);
> +}
> +
> 1;
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index 2267f11..cfdb575 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -156,4 +156,17 @@ sub check_config {
> return $opts;
> }
>
> +sub reassign_volume {
> + my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
> + return $class->file_reassign_volume($scfg, $storeid, $volname, $target_vmid);
> +}
> +
> +sub volume_has_feature {
> + my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
> +
> + shift @_;
> + return $class->file_can_reassign_volume(@_) if $feature eq 'reassign';
> + return $class->SUPER::volume_has_feature(@_);
> +}
> +
> 1;
> diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
> index 2dd414d..8132a3e 100644
> --- a/PVE/Storage/GlusterfsPlugin.pm
> +++ b/PVE/Storage/GlusterfsPlugin.pm
> @@ -348,4 +348,17 @@ sub check_connection {
> return defined($server) ? 1 : 0;
> }
>
> +sub reassign_volume {
> + my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
> + return $class->file_reassign_volume($scfg, $storeid, $volname, $target_vmid);
> +}
> +
> +sub volume_has_feature {
> + my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
> +
> + shift @_;
> + return $class->file_can_reassign_volume(@_) if $feature eq 'reassign';
> + return $class->SUPER::volume_has_feature(@_);
> +}
> +
> 1;
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index df49b76..0d9a691 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -339,6 +339,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) = @_;
>
> @@ -584,6 +591,7 @@ sub volume_has_feature {
>
> my $features = {
> copy => { base => 1, current => 1},
> + reassign => {current => 1},
> };
>
> my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
> @@ -684,4 +692,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) = @_;
Nit: The signature is different than the corresponding one in Plugin.pm.
I know you're not using $base here, but maybe add it for consistency.
That said, why not assemble the new volume ID in the reassign function
itself and have the rename function only be concerned with renaming?
Then you could drop the $base form the signature altogether.
> +
> + 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..895af8b 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/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
> index 39bf15a..7217ca2 100644
> --- a/PVE/Storage/NFSPlugin.pm
> +++ b/PVE/Storage/NFSPlugin.pm
> @@ -197,4 +197,17 @@ sub update_volume_notes {
> PVE::Storage::DirPlugin::update_volume_notes($class, @_);
> }
>
> +sub reassign_volume {
> + my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
> + return $class->file_reassign_volume($scfg, $storeid, $volname, $target_vmid);
> +}
> +
> +sub volume_has_feature {
> + my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
> +
> + shift @_;
> + return $class->file_can_reassign_volume(@_) if $feature eq 'reassign';
> + return $class->SUPER::volume_has_feature(@_);
> +}
> +
> 1;
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index d2d8184..c1ea41d 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -927,6 +927,7 @@ sub storage_can_replicate {
> return 0;
> }
>
> +
Nit: accidental newline
> sub volume_has_feature {
> my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
>
> @@ -1456,4 +1457,63 @@ sub volume_import_formats {
> return ();
> }
>
> +sub reassign_volume {
> + my ($class, $scfg, $storeid, $volname, $target_vmid) = @_;
> + die "not implemented in storage plugin '$class'\n";
> +}
> +
> +# general reassignment method for file/directory based storages
> +sub file_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}";
> +}
> +
> +sub file_can_reassign_volume {
> + my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
> +
> + my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
> + $class->parse_volname($volname);
> +
> + return 1 if !$isBase && $format =~ /^raw$|^qcow2$|^vmdk$/;
> + return undef;
> +}
> +
> 1;
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index fab6d57..86a9a0d 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -712,6 +712,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) =
> @@ -728,4 +729,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 fe65ae4..c63a94b 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -686,6 +686,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) =
> @@ -788,4 +789,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;
>
More information about the pve-devel
mailing list