[pve-devel] [PATCH v3 series 0/4] disk reassign: add new feature

[pve-devel] [PATCH v3 series 0/4] disk reassign: add new feature

[Aaron Lauterer]

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

[pve-devel] [PATCH v3 qemu-server 1/4] disk reassign: add API endpoint

[Aaron Lauterer]

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

[pve-devel] [PATCH v3 qemu-server 2/4] cli: disk reassign: add reassign_disk to qm command

[Aaron Lauterer]

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

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

[Aaron Lauterer]

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

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

[Thomas Lamprecht]

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;
> 

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

[Aaron Lauterer]

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 :)

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

[Thomas Lamprecht]

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.

[pve-devel] [PATCH v3 widget-toolkit 4/4] utils: task_desc_table: add qmreassign

[Aaron Lauterer]

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

Re: [pve-devel] [PATCH v3 widget-toolkit 4/4] utils: task_desc_table: add qmreassign

[Thomas Lamprecht]

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