[pve-devel] [PATCH v1 qemu-server 5/9] api: move-disk: add move to other VM
Fabian Ebner
f.ebner at proxmox.com
Tue Aug 3 09:34:31 CEST 2021
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 at 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 at 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 } ],
>
More information about the pve-devel
mailing list