From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A2E84747AB for ; Wed, 2 Jun 2021 10:53:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8CD3F8DA1 for ; Wed, 2 Jun 2021 10:52:57 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id BE48B8D92 for ; Wed, 2 Jun 2021 10:52:52 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5793F466E4 for ; Wed, 2 Jun 2021 10:52:52 +0200 (CEST) To: pve-devel@lists.proxmox.com, Aaron Lauterer References: <20210601161025.32024-1-a.lauterer@proxmox.com> <20210601161025.32024-6-a.lauterer@proxmox.com> From: Fabian Ebner Message-ID: <5d511f57-5304-f62a-a82f-319798d0fb04@proxmox.com> Date: Wed, 2 Jun 2021 10:52:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <20210601161025.32024-6-a.lauterer@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.289 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.613 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemu.pm, proxmox.com, qm.pm] Subject: Re: [pve-devel] [RFC qemu-server 5/7] api: move-disk: add move to other VM X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Jun 2021 08:53:27 -0000 Am 01.06.21 um 18:10 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 > --- > > 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 | 204 +++++++++++++++++++++++++++++++++++++++++++++-- > PVE/CLI/qm.pm | 2 +- > 2 files changed, 200 insertions(+), 6 deletions(-) > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index 24dba86..f1aee8d 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}) { > @@ -3258,9 +3259,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' ]], > @@ -3271,14 +3274,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', > @@ -3305,6 +3313,18 @@ __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 => { > @@ -3319,14 +3339,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; > + > + > 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); > > @@ -3436,7 +3464,173 @@ __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: long line > + 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}; > + > + 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/|; > + 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, > + ); > + > + die "Cannot move disk to another VM while the source VM is running\n" > + if PVE::QemuServer::check_running($vmid) > + && $disk !~ m/^unused\d+$/; > + Style nit: multiline post-ifs > + 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}"; > + } > + } > + > + 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, undef) = PVE::Storage::parse_volume_id($source_volid); > + > + my $target_volname = PVE::Storage::find_free_diskname($storecfg, $storeid, $target_vmid, $format); The disk name from here might not be free anymore by the time the rename happens. Possible fix: Have a storage lock within PVE::Storage::rename_volume and have each plugin's implementation check that the name is actually free to use. Similar to volume_import, but there no lock is needed as we can check that the allocate name matches the expected one (not possible here, so a lock is needed AFAICT). > + > + my $new_volid = PVE::Storage::rename_volume( > + $storecfg, > + $source_volid, > + $vmid, > + $target_volname, > + $target_vmid, > + ); > + > + $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 60360c8..4b475f8 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 } ], >