* [pve-devel] [PATCH v3 series 0/4] disk reassign: add new feature @ 2020-09-10 14:32 Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 qemu-server 1/4] disk reassign: add API endpoint Aaron Lauterer ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Aaron Lauterer @ 2020-09-10 14:32 UTC (permalink / raw) To: pve-devel This series implements a new feature which allows users to easily reassign disks between VMs. Currently this is only possible with one of the following manual steps: * rename the disk image/file and do a `qm rescan` * configure the disk manually and use the old image name, having an image for VM A assigned to VM B The latter can cause unexpected behavior because PVE expects that the VMID in a disk name always corresponds to the VM it is assigned to. Thus when a disk, original from VM A was manually configured as disk for VM B it happens that, when deleting VM A, the disk in question will be deleted as well because it still had the VMID of VM A in it's name. To issue a reassign from the CLI run: qm reassign_disk <source VMID> <target VMID> <drive key> where <drive key> is the config key of the disk, e.g. ide0, scsi1 and so on. The following storage types are implemented at the moment: * dir based ones * directory * NFS * CIFS * gluster * ZFS * (thin) LVM * Ceph RBD v2 -> v3: * change locking approach * add more checks * add intermedia storage plugin for directory based plugins * use feature flags * split up the reassign method to have a dedicated method for the renaming itself * handle linked clones * clean up if disk used to be replicated I hope I didn't forget anything major. v1 -> v2: print info about the new disk volid and key at the end of the job so it shows up in the CLI output and task log Changes from RFC -> V1: * support to reassign unused disks * digest for target vm config * reorder the checks a bit * adding another one to check if the given key for the disk even exists in the config. qemu-server: Aaron Lauterer (2): disk reassign: add API endpoint cli: disk reassign: add reassign_disk to qm command PVE/API2/Qemu.pm | 156 ++++++++++++++++++++++++++++++++++++++++ PVE/CLI/qm.pm | 2 + PVE/QemuServer/Drive.pm | 4 ++ 3 files changed, 162 insertions(+) storage: Aaron Lauterer (1): add disk reassign feature 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 widget-toolkit: Aaron Lauterer (1): utils: task_desc_table: add qmreassign src/Utils.js | 1 + 1 file changed, 1 insertion(+) -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH v3 qemu-server 1/4] disk reassign: add API endpoint 2020-09-10 14:32 [pve-devel] [PATCH v3 series 0/4] disk reassign: add new feature Aaron Lauterer @ 2020-09-10 14:32 ` Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 qemu-server 2/4] cli: disk reassign: add reassign_disk to qm command Aaron Lauterer ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Aaron Lauterer @ 2020-09-10 14:32 UTC (permalink / raw) To: pve-devel The goal of this new API endpoint is to provide an easy way to move a disk between VMs as this was only possible with manual intervention until now. Either by renaming the VM disk or by manually adding the disks volid to the config of the other VM. The latter can easily cause unexpected behavior such as disks attached to VM B would be deleted if it used to be a disk of VM A. This happens because PVE assumes that the VMID in the volname always matches the VM the disk is attached to and thus, would remove any disk with VMID A when VM A was deleted. The term `reassign` was chosen as it is not yet used for disk VMs. Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com> --- v2 -> v3: * reordered the locking as discussed with fabian [0] to run checks fork worker lock source config lock target config run checks ... * added more checks * will not reassign to or from templates * will not reassign if VM has snapshots present * cleanup if disk used to be replicated * made task log slightly more verbose * integrated general recommendations regarding code * renamed `disk` to `drive_key` * prepended some vars with `source_` for easier distinction v1 -> v2: print config key and volid info at the end of the job so it shows up on the CLI and task log rfc -> v1: * add support to reassign unused disks * add support to provide a config digest for the target vm * add additional check if disk key is present in config * reorder checks a bit In order to support unused disk I had to extend PVE::QemuServer::Drive::valid_drive_names for the API parameter validation. Checks are ordered so that cheap tests are run at the first chance to fail early. The check if both VMs are present on the node is a bit redundant because locking the config files will fail if the VM is not present. But with the additional check we can provide a useful error message to the user instead of a "Configuration file xyz does not exist" error. [0] https://lists.proxmox.com/pipermail/pve-devel/2020-September/044930.html diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index 8da616a..613b257 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -4265,4 +4265,160 @@ __PACKAGE__->register_method({ return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type}); }}); +__PACKAGE__->register_method({ + name => 'reassign_vm_disk', + path => '{vmid}/reassign_disk', + method => 'POST', + protected => 1, + proxyto => 'node', + description => "Reassign a disk to another VM", + permissions => { + description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, and 'Datastore.Allocate' permissions on the storage.", + check => [ 'and', + ['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]], + ['perm', '/storage/{storage}', [ 'Datastore.Allocate' ]], + ], + }, + parameters => { + additionalProperties => 0, + properties => { + node => get_standard_option('pve-node'), + vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }), + target_vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }), + drive_key => { + type => 'string', + description => "The config key of the disk to reassign (for example, ide0 or scsi1).", + enum => [PVE::QemuServer::Drive::valid_drive_names_with_unused()], + }, + digest => { + type => 'string', + description => 'Prevent changes if current the configuration file of the source VM has a different SHA1 digest. This can be used to prevent concurrent modifications.', + maxLength => 40, + optional => 1, + }, + target_digest => { + type => 'string', + description => 'Prevent changes if current the configuration file of the target VM has a different SHA1 digest. This can be used to prevent concurrent modifications.', + maxLength => 40, + optional => 1, + }, + }, + }, + returns => { + type => 'string', + description => "the task ID.", + }, + code => sub { + my ($param) = @_; + + my $rpcenv = PVE::RPCEnvironment::get(); + my $authuser = $rpcenv->get_user(); + + my $node = extract_param($param, 'node'); + my $source_vmid = extract_param($param, 'vmid'); + my $target_vmid = extract_param($param, 'target_vmid'); + my $source_digest = extract_param($param, 'digest'); + my $target_digest = extract_param($param, 'target_digest'); + my $drive_key = extract_param($param, 'drive_key'); + + my $storecfg = PVE::Storage::config(); + my $vmlist; + my $drive; + my $source_volid; + + die "You cannot reassign a disk to the same VM\n" + if $source_vmid eq $target_vmid; + + my $load_and_check_configs = sub { + $vmlist = PVE::QemuServer::vzlist(); + die "Both VMs need to be on the same node\n" + if !$vmlist->{$source_vmid}->{exists} || !$vmlist->{$target_vmid}->{exists}; + + my $source_conf = PVE::QemuConfig->load_config($source_vmid); + PVE::QemuConfig->check_lock($source_conf); + my $target_conf = PVE::QemuConfig->load_config($target_vmid); + PVE::QemuConfig->check_lock($target_conf); + + die "Can't reassign disks with templates\n" + if ($source_conf->{template} || $target_conf->{template}); + + if ($source_digest) { + eval { PVE::Tools::assert_if_modified($source_digest, $source_conf->{digest}) }; + if (my $err = $@) { + die "Verification of source VM digest failed: ${err}"; + } + } + + if ($target_digest) { + eval { PVE::Tools::assert_if_modified($target_digest, $target_conf->{digest}) }; + if (my $err = $@) { + die "Verification of target VM digest failed: ${err}"; + } + } + + die "Disk '${drive_key}' does not exist\n" + if !defined($source_conf->{$drive_key}); + + $drive = PVE::QemuServer::parse_drive($drive_key, $source_conf->{$drive_key}); + $source_volid = $drive->{file}; + die "disk '${drive_key}' has no associated volume\n" if !$source_volid; + die "CD drive contents can't be reassigned\n" if PVE::QemuServer::drive_is_cdrom($drive, 1); + + die "Can't reassign disk used by a snapshot\n" + if PVE::QemuServer::Drive::is_volume_in_use($storecfg, $source_conf, $drive_key, $source_volid); + + my $hasfeature = PVE::Storage::volume_has_feature($storecfg, 'reassign', $source_volid); + die "Storage does not support the reassignment of this disk\n" if !$hasfeature; + + die "Cannot reassign disk while the source VM is running\n" + if PVE::QemuServer::check_running($source_vmid) && $drive_key !~ m/unused[0-9]/; + + return ($source_conf, $target_conf); + }; + + my $reassign_func = sub { + return PVE::QemuConfig->lock_config($source_vmid, sub { + return PVE::QemuConfig->lock_config($target_vmid, sub { + my ($source_conf, $target_conf) = &$load_and_check_configs(); + + PVE::Cluster::log_msg('info', $authuser, "reassign disk VM $source_vmid: reassign --disk ${drive_key} --target_vmid $target_vmid"); + + my $new_volid = PVE::Storage::reassign_volume($storecfg, $source_volid, $target_vmid); + + delete $source_conf->{$drive_key}; + PVE::QemuConfig->write_config($source_vmid, $source_conf); + print "removing disk '${drive_key}' from VM '${source_vmid}'\n"; + + # remove possible replication snapshots + my $had_snapshots = 0; + if (PVE::Storage::volume_has_feature($storecfg, 'replicate', $new_volid)) { + my $snapshots = PVE::Storage::volume_snapshot_list($storecfg, $new_volid); + for my $snap (@$snapshots) { + next if (substr($snap, 0, 12) ne '__replicate_'); + + $had_snapshots = 1; + PVE::Storage::volume_snapshot_delete($storecfg, $new_volid, $snap); + } + print "Disk '${drive_key}:${source_volid}' was replicated. On the next replication run it will be cleaned up on the replication target.\n" + if $had_snapshots;; + } + + my $key; + eval { $key = PVE::QemuConfig->add_unused_volume($target_conf, $new_volid) }; + if (my $err = $@) { + print "adding moved disk '${new_volid}' to VM '${target_vmid}' config failed.\n"; + return 0; + } + + PVE::QemuConfig->write_config($target_vmid, $target_conf); + print "adding disk to VM '${target_vmid}' as '${key}: ${new_volid}'\n"; + }); + }); + }; + + &$load_and_check_configs(); + + return $rpcenv->fork_worker('qmreassign', $source_vmid, $authuser, $reassign_func); + }}); + 1; diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm index 91c33f8..d2f59cd 100644 --- a/PVE/QemuServer/Drive.pm +++ b/PVE/QemuServer/Drive.pm @@ -383,6 +383,10 @@ sub valid_drive_names { 'efidisk0'); } +sub valid_drive_names_with_unused { + return (valid_drive_names(), map {"unused$_"} (0 .. ($MAX_UNUSED_DISKS -1))); +} + sub is_valid_drivename { my $dev = shift; -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH v3 qemu-server 2/4] cli: disk reassign: add reassign_disk to qm command 2020-09-10 14:32 [pve-devel] [PATCH v3 series 0/4] disk reassign: add new feature Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 qemu-server 1/4] disk reassign: add API endpoint Aaron Lauterer @ 2020-09-10 14:32 ` Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 widget-toolkit 4/4] utils: task_desc_table: add qmreassign Aaron Lauterer 3 siblings, 0 replies; 9+ messages in thread From: Aaron Lauterer @ 2020-09-10 14:32 UTC (permalink / raw) To: pve-devel Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com> --- v2 -> v3: renamed parameter `disk` to `drive_key` rfc -> v1 -> v2: nothing changed PVE/CLI/qm.pm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm index 282fa86..7fe25c6 100755 --- a/PVE/CLI/qm.pm +++ b/PVE/CLI/qm.pm @@ -913,6 +913,8 @@ our $cmddef = { move_disk => [ "PVE::API2::Qemu", 'move_vm_disk', ['vmid', 'disk', 'storage'], { node => $nodename }, $upid_exit ], + reassign_disk => [ "PVE::API2::Qemu", 'reassign_vm_disk', ['vmid', 'target_vmid', 'drive_key'], { node => $nodename } ], + unlink => [ "PVE::API2::Qemu", 'unlink', ['vmid'], { node => $nodename } ], config => [ "PVE::API2::Qemu", 'vm_config', ['vmid'], -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature 2020-09-10 14:32 [pve-devel] [PATCH v3 series 0/4] disk reassign: add new feature Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 qemu-server 1/4] disk reassign: add API endpoint Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 qemu-server 2/4] cli: disk reassign: add reassign_disk to qm command Aaron Lauterer @ 2020-09-10 14:32 ` Aaron Lauterer 2020-09-18 14:24 ` Thomas Lamprecht 2020-09-10 14:32 ` [pve-devel] [PATCH v3 widget-toolkit 4/4] utils: task_desc_table: add qmreassign Aaron Lauterer 3 siblings, 1 reply; 9+ messages in thread From: Aaron Lauterer @ 2020-09-10 14:32 UTC (permalink / raw) To: pve-devel 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. 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`. * 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; -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature 2020-09-10 14:32 ` [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature Aaron Lauterer @ 2020-09-18 14:24 ` Thomas Lamprecht 2020-09-18 15:07 ` Aaron Lauterer 0 siblings, 1 reply; 9+ messages in thread From: Thomas Lamprecht @ 2020-09-18 14:24 UTC (permalink / raw) To: Proxmox VE development discussion, Aaron Lauterer 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; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature 2020-09-18 14:24 ` Thomas Lamprecht @ 2020-09-18 15:07 ` Aaron Lauterer 2020-09-21 11:11 ` Thomas Lamprecht 0 siblings, 1 reply; 9+ messages in thread From: Aaron Lauterer @ 2020-09-18 15:07 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion On 9/18/20 4:24 PM, Thomas Lamprecht wrote: > 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 That was the first approach, but this would mean a die for all other plugins and if 3rd party plugins would not implement it, the directory based code will be used. But we cannot know if if the directory approach is the right one. > * a dummy "die implement me in subclass" method if above is not possible If I do it this way, I cannot put the actual code for dir based storage in the Plugin.pm, thus having an intermediate class for dir based storages would still be beneficial IMHO to avoid code duplication. IIRC we have 4 dir based storages (dir, CIFS, NFS, Gluster). > > Then increase the ABI version AND age to allow external plugins to tell > if they can or should implement this themself. Thanks for the hint. > > 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 Okay, thanks for the feedback :) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature 2020-09-18 15:07 ` Aaron Lauterer @ 2020-09-21 11:11 ` Thomas Lamprecht 0 siblings, 0 replies; 9+ messages in thread From: Thomas Lamprecht @ 2020-09-21 11:11 UTC (permalink / raw) To: Proxmox VE development discussion, Aaron Lauterer On 18.09.20 17:07, Aaron Lauterer wrote: > > > On 9/18/20 4:24 PM, Thomas Lamprecht wrote: >> 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 > > That was the first approach, but this would mean a die for all other plugins and if 3rd party plugins would not implement it, the directory based code will be used. But we cannot know if if the directory approach is the right one. Note that you also can check the APIVERSION of external plugins through the "api" method, which is required for them and checked on loading. > >> * a dummy "die implement me in subclass" method if above is not possible > > If I do it this way, I cannot put the actual code for dir based storage in the Plugin.pm, thus having an intermediate class for dir based storages would still be beneficial IMHO to avoid code duplication. IIRC we have 4 dir based storages (dir, CIFS, NFS, Gluster). code duplication can also be avoided by just having a general method somewhere which the single plugins just wrap around (call). That just "duplicates" the calls, which is no actual duplication as it provides concrete information about what which plugin uses they all share the implementation, which is the actual reason for avoiding code duplication. IMO a saner and more understandable than this approach, plus external plugins can just call into this if it works for them. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH v3 widget-toolkit 4/4] utils: task_desc_table: add qmreassign 2020-09-10 14:32 [pve-devel] [PATCH v3 series 0/4] disk reassign: add new feature Aaron Lauterer ` (2 preceding siblings ...) 2020-09-10 14:32 ` [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature Aaron Lauterer @ 2020-09-10 14:32 ` Aaron Lauterer 2020-09-21 12:09 ` Thomas Lamprecht 3 siblings, 1 reply; 9+ messages in thread From: Aaron Lauterer @ 2020-09-10 14:32 UTC (permalink / raw) To: pve-devel Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com> --- rfc -> v1 -> v2 -> v3: nothing changed src/Utils.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Utils.js b/src/Utils.js index 8595cce..af41f33 100644 --- a/src/Utils.js +++ b/src/Utils.js @@ -587,6 +587,7 @@ utilities: { qmigrate: ['VM', gettext('Migrate')], qmclone: ['VM', gettext('Clone')], qmmove: ['VM', gettext('Move disk')], + qmreassign: ['VM', gettext('Reassign disk')], qmtemplate: ['VM', gettext('Convert to template')], qmstart: ['VM', gettext('Start')], qmstop: ['VM', gettext('Stop')], -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH v3 widget-toolkit 4/4] utils: task_desc_table: add qmreassign 2020-09-10 14:32 ` [pve-devel] [PATCH v3 widget-toolkit 4/4] utils: task_desc_table: add qmreassign Aaron Lauterer @ 2020-09-21 12:09 ` Thomas Lamprecht 0 siblings, 0 replies; 9+ messages in thread From: Thomas Lamprecht @ 2020-09-21 12:09 UTC (permalink / raw) To: Proxmox VE development discussion, Aaron Lauterer On 10.09.20 16:32, Aaron Lauterer wrote: > Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com> > --- > rfc -> v1 -> v2 -> v3: nothing changed > > src/Utils.js | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/Utils.js b/src/Utils.js > index 8595cce..af41f33 100644 > --- a/src/Utils.js > +++ b/src/Utils.js > @@ -587,6 +587,7 @@ utilities: { > qmigrate: ['VM', gettext('Migrate')], > qmclone: ['VM', gettext('Clone')], > qmmove: ['VM', gettext('Move disk')], > + qmreassign: ['VM', gettext('Reassign disk')], > qmtemplate: ['VM', gettext('Convert to template')], > qmstart: ['VM', gettext('Start')], > qmstop: ['VM', gettext('Stop')], > I'd like to avoid adding new project specific task descriptions in widget toolkit. Rather use the Proxmox.Utils.override_task_descriptions [0] in pve-manager to add such project specific task descriptions. See Proxmox Backup Server UI for an usage example [1]. [0]: https://git.proxmox.com/?p=proxmox-widget-toolkit.git;a=blob;f=src/Utils.js;h=8595ccec41e00aed5790af1a6ad5cfcd0e35674c;hb=HEAD#l653 [1]: https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=www/Utils.js;h=7bcf1ba613fb6d0c52d7d862ba0d0264f1b738c0;hb=HEAD#l93 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-21 12:09 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-10 14:32 [pve-devel] [PATCH v3 series 0/4] disk reassign: add new feature Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 qemu-server 1/4] disk reassign: add API endpoint Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 qemu-server 2/4] cli: disk reassign: add reassign_disk to qm command Aaron Lauterer 2020-09-10 14:32 ` [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature Aaron Lauterer 2020-09-18 14:24 ` Thomas Lamprecht 2020-09-18 15:07 ` Aaron Lauterer 2020-09-21 11:11 ` Thomas Lamprecht 2020-09-10 14:32 ` [pve-devel] [PATCH v3 widget-toolkit 4/4] utils: task_desc_table: add qmreassign Aaron Lauterer 2020-09-21 12:09 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox