Re: [pve-devel] [PATCH v1 qemu-server 5/9] api: move-disk: add move to other VM

[Fabian Ebner <f.ebner@proxmox.com>]

Am 19.07.21 um 16:52 schrieb Aaron Lauterer:
> The goal of this is to expand the move-disk API endpoint to make it
> possible to move a disk to another VM. Previously this was only possible
> with manual intervertion either by renaming the VM disk or by manually
> adding the disks volid to the config of the other VM.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> rfc -> v1:
> * add check if target guest is replicated and fail if the moved volume
>    does not support it
> * check if source volume has a format suffix and pass it to
>    'find_free_disk'
> * fixed some style nits
> 
> old dedicated api endpoint -> rfc:
> There are some big changes here. The old [0] dedicated API endpoint is
> gone and most of its code is now part of move_disk. Error messages have
> been changed accordingly and sometimes enahnced by adding disk keys and
> VMIDs where appropriate.
> 
> Since a move to other guests should be possible for unused disks, we
> need to check before doing a move to storage to make sure to not
> handle unused disks.
> 
> [0] https://lists.proxmox.com/pipermail/pve-devel/2021-April/047738.html
> 
>   PVE/API2/Qemu.pm | 229 +++++++++++++++++++++++++++++++++++++++++++++--
>   PVE/CLI/qm.pm    |   2 +-
>   2 files changed, 225 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index f2557e3..ed1179b 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -35,6 +35,7 @@ use PVE::API2::Qemu::Agent;
>   use PVE::VZDump::Plugin;
>   use PVE::DataCenterConfig;
>   use PVE::SSHInfo;
> +use PVE::Replication;
>   
>   BEGIN {
>       if (!$ENV{PVE_GENERATING_DOCS}) {
> @@ -3263,9 +3264,11 @@ __PACKAGE__->register_method({
>       method => 'POST',
>       protected => 1,
>       proxyto => 'node',
> -    description => "Move volume to different storage.",
> +    description => "Move volume to different storage or to a different VM.",
>       permissions => {
> -	description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, and 'Datastore.AllocateSpace' permissions on the storage.",
> +	description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, " .
> +	    "and 'Datastore.AllocateSpace' permissions on the storage. To move ".
> +	    "a disk to another VM, you need the permissions on the target VM as well.",
>   	check => [ 'and',
>   		   ['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
>   		   ['perm', '/storage/{storage}', [ 'Datastore.AllocateSpace' ]],
> @@ -3276,14 +3279,19 @@ __PACKAGE__->register_method({
>   	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,
> +		optional => 1,
> +	    }),
>   	    disk => {
>   	        type => 'string',
>   		description => "The disk you want to move.",
> -		enum => [PVE::QemuServer::Drive::valid_drive_names()],
> +		enum => [PVE::QemuServer::Drive::valid_drive_names_with_unused()],
>   	    },
>               storage => get_standard_option('pve-storage-id', {
>   		description => "Target storage.",
>   		completion => \&PVE::QemuServer::complete_storage,
> +		optional => 1,
>               }),
>               'format' => {
>                   type => 'string',
> @@ -3310,6 +3318,20 @@ __PACKAGE__->register_method({
>   		minimum => '0',
>   		default => 'move limit from datacenter or storage config',
>   	    },
> +	    'target-disk' => {
> +	        type => 'string',
> +		description => "The config key the disk will be moved to on the target VM " .
> +		    "(for example, ide0 or scsi1).",
> +		enum => [PVE::QemuServer::Drive::valid_drive_names_with_unused()],
> +		optional => 1,
> +	    },
> +	    'target-digest' => {
> +		type => 'string',
> +		description => 'Prevent changes if current configuration file of the target VM has " .
> +		    "a different SHA1 digest. This can be used to prevent concurrent modifications.',
> +		maxLength => 40,
> +		optional => 1,
> +	    },
>   	},
>       },
>       returns => {
> @@ -3324,14 +3346,22 @@ __PACKAGE__->register_method({
>   
>   	my $node = extract_param($param, 'node');
>   	my $vmid = extract_param($param, 'vmid');
> +	my $target_vmid = extract_param($param, 'target-vmid');
>   	my $digest = extract_param($param, 'digest');
> +	my $target_digest = extract_param($param, 'target-digest');
>   	my $disk = extract_param($param, 'disk');
> +	my $target_disk = extract_param($param, 'target-disk');
>   	my $storeid = extract_param($param, 'storage');
>   	my $format = extract_param($param, 'format');
>   
> +	die "either set storage or target-vmid, but not both\n"
> +	    if $storeid && $target_vmid;

If the VM -> VM variant is used, shouldn't we also test for $target_disk 
being set? When not passing --target-disk, it fails further below, but 
here we could die clean and early. Or make $target_disk default to $disk 
in that case?

> +
> +
>   	my $storecfg = PVE::Storage::config();
> +	my $source_volid;
>   
> -	my $updatefn =  sub {
> +	my $move_updatefn =  sub {
>   	    my $conf = PVE::QemuConfig->load_config($vmid);
>   	    PVE::QemuConfig->check_lock($conf);
>   
> @@ -3441,7 +3471,196 @@ __PACKAGE__->register_method({
>               return $rpcenv->fork_worker('qmmove', $vmid, $authuser, $realcmd);
>   	};
>   
> -	return PVE::QemuConfig->lock_config($vmid, $updatefn);
> +	my $load_and_check_reassign_configs = sub {
> +	    my $vmlist = PVE::Cluster::get_vmlist()->{ids};
> +	    die "Both VMs need to be on the same node ($vmlist->{$vmid}->{node}) ".
> +		"but target VM is on $vmlist->{$target_vmid}->{node}.\n"
> +		if $vmlist->{$vmid}->{node} ne $vmlist->{$target_vmid}->{node};

Style nit: multiline expression with post-if

> +
> +	    my $source_conf = PVE::QemuConfig->load_config($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 move disks from or to template VMs\n"
> +		if ($source_conf->{template} || $target_conf->{template});
> +
> +	    if ($digest) {
> +		eval { PVE::Tools::assert_if_modified($digest, $source_conf->{digest}) };
> +		if (my $err = $@) {
> +		    die "VM ${vmid}: ${err}";
> +		}
> +	    }
> +
> +	    if ($target_digest) {
> +		eval { PVE::Tools::assert_if_modified($target_digest, $target_conf->{digest}) };
> +		if (my $err = $@) {
> +		    die "VM ${target_vmid}: ${err}";
> +		}
> +	    }
> +
> +	    die "Disk '${disk}' for VM '$vmid' does not exist\n"
> +		if !defined($source_conf->{$disk});
> +
> +	    die "Target disk key '${target_disk}' is already in use for VM '$target_vmid'\n"
> +		if exists $target_conf->{$target_disk};

Any special reason to test for defined above and exists here? Is there a 
problem if the target config has an explicit undef at that key?

> +
> +	    my $drive = PVE::QemuServer::parse_drive(
> +		$disk,
> +		$source_conf->{$disk},
> +	    );
> +	    $source_volid = $drive->{file};
> +
> +	    die "disk '${disk}' has no associated volume\n"
> +		if !$source_volid;
> +	    die "CD drive contents can't be moved to another VM\n"
> +		if PVE::QemuServer::drive_is_cdrom($drive, 1);
> +	    die "Can't move  physical disk to another VM\n" if $drive->{file} =~ m|^/dev/|;

Could re-use $source_volid instead of $drive->{file}

> +	    die "Can't move disk used by a snapshot to another VM\n"
> +		if PVE::QemuServer::Drive::is_volume_in_use(
> +		    $storecfg,
> +		    $source_conf,
> +		    $disk,
> +		    $source_volid,
> +		);
> +
> +	    die "Storage does not support moving of this disk to another VM\n"
> +		if !PVE::Storage::volume_has_feature(
> +		    $storecfg,
> +		    'rename',
> +		    $source_volid,
> +		);

Style nit: two multiline post-if expressions

> +
> +	    die "Cannot move disk to another VM while the source VM is running\n"
> +		if PVE::QemuServer::check_running($vmid) && $disk !~ m/^unused\d+$/;
> +
> +	    if ($target_disk !~ m/^unused\d+$/ && $target_disk =~ m/^([^\d]+)\d+$/) {
> +		my $interface = $1;
> +		my $desc = PVE::JSONSchema::get_standard_option("pve-qm-${interface}");
> +		eval {
> +		    PVE::JSONSchema::parse_property_string(
> +			$desc->{format},
> +			$source_conf->{$disk},
> +		    )
> +		};
> +		if (my $err = $@) {
> +		    die "Cannot move disk to another VM: ${err}";
> +		}
> +	    }
> +
> +	    my $repl_conf = PVE::ReplicationConfig->new();
> +	    my $is_replicated = $repl_conf->check_for_existing_jobs($target_vmid, 1);
> +	    my ($storeid, undef) = PVE::Storage::parse_volume_id($source_volid);
> +	    my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
> +	    if ($is_replicated && !PVE::Storage::storage_can_replicate($storecfg, $storeid, $format)) {
> +		die "Cannot move disk to a replicated VM. Storage does not support replication!\n";
> +	    }
> +
> +	    return ($source_conf, $target_conf);
> +	};
> +
> +	my $logfunc = sub {
> +	    my ($msg) = @_;
> +	    print STDERR "$msg\n";
> +	};
> +
> +	my $disk_reassignfn = sub {
> +	    return PVE::QemuConfig->lock_config($vmid, sub {
> +		return PVE::QemuConfig->lock_config($target_vmid, sub {
> +		    my ($source_conf, $target_conf) = &$load_and_check_reassign_configs();
> +
> +		    my $drive_param = PVE::QemuServer::parse_drive(
> +			$target_disk,
> +			$source_conf->{$disk},
> +		    );
> +
> +		    print "moving disk '$disk' from VM '$vmid' to '$target_vmid'\n";
> +		    my ($storeid, $source_volname) = PVE::Storage::parse_volume_id($source_volid);
> +
> +		    my $fmt = undef;
> +		    if ($source_volname =~ m/vm-\d+-\S+\.(raw|qcow2|vmdk)$/) {
> +			$fmt = $1;
> +		    }

Using parse_volname to get the format should be more complete/robust.

> +
> +		    my $target_volname = PVE::Storage::find_free_diskname(
> +			$storecfg,
> +			$storeid,
> +			$target_vmid,
> +			$fmt
> +		    );
> +
> +		    my $new_volid = PVE::Storage::rename_volume(
> +			$storecfg,
> +			$source_volid,
> +			$target_volname,
> +		    );
> +
> +		    $drive_param->{file} = $new_volid;
> +
> +		    delete $source_conf->{$disk};
> +		    print "removing disk '${disk}' from VM '${vmid}' config\n";
> +		    PVE::QemuConfig->write_config($vmid, $source_conf);
> +
> +		    my $drive_string = PVE::QemuServer::print_drive($drive_param);
> +		    &$update_vm_api(
> +			{
> +			    node => $node,
> +			    vmid => $target_vmid,
> +			    digest => $target_digest,
> +			    $target_disk => $drive_string,
> +			},
> +			1,
> +		    );
> +
> +		    # remove possible replication snapshots
> +		    if (PVE::Storage::volume_has_feature(
> +			    $storecfg,
> +			    'replicate',
> +			    $source_volid),
> +		    ) {
> +			eval {
> +			    PVE::Replication::prepare(
> +				$storecfg,
> +				[$new_volid],
> +				undef,
> +				1,
> +				undef,
> +				$logfunc,
> +			    )
> +			};
> +			if (my $err = $@) {
> +			    print "Failed to remove replication snapshots on moved disk " .
> +				"'$target_disk'. Manual cleanup could be necessary.\n";
> +			}
> +		    }
> +		});
> +	    });
> +	};
> +
> +	if ($target_vmid) {
> +	    $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk'])
> +		if $authuser ne 'root@pam';
> +
> +	    die "Moving a disk to the same VM is not possible. Did you mean to ".
> +		"move the disk to a different storage?\n"
> +		if $vmid eq $target_vmid;
> +
> +	    &$load_and_check_reassign_configs();
> +	    return $rpcenv->fork_worker(
> +		'qmmove',
> +		"${vmid}-${disk}>${target_vmid}-${target_disk}",
> +		$authuser,
> +		$disk_reassignfn
> +	    );
> +	} elsif ($storeid) {
> +	    die "cannot move disk '$disk', only configured disks can be moved to another storage\n"
> +		if $disk =~ m/^unused\d+$/;
> +	    return PVE::QemuConfig->lock_config($vmid, $move_updatefn);
> +	} else {
> +	    die "Provide either a 'storage' to move the disk to a different " .
> +		"storage or 'target-vmid' and 'target-disk' to move the disk " .
> +		"to another VM\n";
> +	}
>       }});
>   
>   my $check_vm_disks_local = sub {
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index ef99b6d..a92d301 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -910,7 +910,7 @@ our $cmddef = {
>   
>       resize => [ "PVE::API2::Qemu", 'resize_vm', ['vmid', 'disk', 'size'], { node => $nodename } ],
>   
> -    'move-disk' => [ "PVE::API2::Qemu", 'move_vm_disk', ['vmid', 'disk', 'storage'], { node => $nodename }, $upid_exit ],
> +    'move-disk' => [ "PVE::API2::Qemu", 'move_vm_disk', ['vmid', 'disk', 'storage', 'target-vmid', 'target-disk'], { node => $nodename }, $upid_exit ],
>       move_disk => { alias => 'move-disk' },
>   
>       unlink => [ "PVE::API2::Qemu", 'unlink', ['vmid'], { node => $nodename } ],
>