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

[Aaron Lauterer <a.lauterer@proxmox.com>]

On 8/3/21 9:34 AM, Fabian Ebner wrote:
> 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?

I do like the latter idea, to use the same drive key if there is no other provided.

> 
>> +
>> +
>>       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?

If the target config already has the disk key present (undef or not) we bail in order to not overwrite anything that is already there (even if it's just the config key).
Of course we could argue that if only the config key is present, we could just overwrite it, but I wanted to err on the safe side.

> 
>> +
>> +        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.

With the other changes to decide in the "find_free_volname" if we should attach the fmt suffix, this is now possible :)
> 
>> +
>> +            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 } ],
>>