[pve-devel] [PATCH v6 storage 1/1] add disk reassign feature

[Fabian Ebner]

Am 15.04.21 um 13:07 schrieb Fabian Ebner:
> 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 

I mean together with adapting the default implementation for 
volume_has_feature too.

One more thing I noticed below:

> 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});
>> +

Check if the storage is enabled and activate it?
And is it necessary to activate the volume for certain storages, e.g. 
LVM (might not be, but did you check)?

>> +    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;
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>