[pve-devel] [PATCH v6 storage 1/1] add disk reassign feature
Aaron Lauterer
a.lauterer at proxmox.com
Thu Apr 15 13:53:28 CEST 2021
On 4/15/21 1:31 PM, Fabian Ebner wrote:
> 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
Just adding the functionality on the top level Plugin.pm could have some potential ugly side effects for 3rd party plugins that do not yet handle that call themselves. So to be on the safe side, by default we rather fail right there (was discussed a versions ago).
IMHO it would be nice though to change the structure of the storage plugins a bit. E.g. instead of assuming dir/file storages for Plugin.pm, having a basic abstraction specifically for any directory/file based storage which handles all the common tasks and further down the hierarchy the specific implementations regarding mounting and such. But that would mean a hard break of the current approach, especially for 3rd party plugins.
>
> 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 ;)
We do not store any guest disks on CephFS. If people do that (not a good idea as it can be unresponsive for some time if an MDS takes over and needs to do some replay), they usually configure a directory storage on top of /mnt/pve/cephfs/.
>>
>>> * 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)?
Good point about activating the storage. I don't think I did check on that. I'll do some tests.
>
>>> + 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.
Yeah, Dominic also encountered a possible change/improvement on to how handle the base. I'll take a closer look :)
>>
>>> +
>>> + 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
>>
>>
More information about the pve-devel
mailing list