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 BF58861AA0 for ; Wed, 19 Aug 2020 10:35:49 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5541419E48 for ; Wed, 19 Aug 2020 10:35:49 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id BCA3F19DE9 for ; Wed, 19 Aug 2020 10:35:46 +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 83E29446EC for ; Wed, 19 Aug 2020 10:35:46 +0200 (CEST) From: Aaron Lauterer To: pve-devel@lists.proxmox.com Date: Wed, 19 Aug 2020 10:35:41 +0200 Message-Id: <20200819083545.27992-2-a.lauterer@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200819083545.27992-1-a.lauterer@proxmox.com> References: <20200819083545.27992-1-a.lauterer@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.039 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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. [drive.pm, qemu.pm] Subject: [pve-devel] [PATCH qemu-server v1 1/5] disk reassign: add API endpoint 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, 19 Aug 2020 08:35:49 -0000 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 --- 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. Feedback, especially on the checks is welcome. PVE/API2/Qemu.pm | 106 ++++++++++++++++++++++++++++++++++++++++ PVE/QemuServer/Drive.pm | 4 ++ 2 files changed, 110 insertions(+) diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index 8da616a..533f01c 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -4265,4 +4265,110 @@ __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 }), + disk => { + type => 'string', + description => "The config key of the disk to move (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 $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 $storecfg = PVE::Storage::config(); + + die "You cannot reassign a disk to the same VM\n" + if $vmid eq $target_vmid; + + my $vmlist = PVE::QemuServer::vzlist(); + die "Both VMs need to be on the same node\n" + if !$vmlist->{$vmid}->{exists} || !$vmlist->{$target_vmid}->{exists}; + + return PVE::QemuConfig->lock_config($vmid, sub { + my $conf = PVE::QemuConfig->load_config($vmid); + PVE::QemuConfig->check_lock($conf); + + die "Source VM config checksum missmatch (file change by other user?)\n" + if $digest && $digest ne $conf->{digest}; + + die "Disk '${disk}' does not exist\n" + if !exists($conf->{$disk}); + + my $drive = PVE::QemuServer::parse_drive($disk, $conf->{$disk}); + die "disk '$disk' has no associated volume\n" if !$drive->{file}; + die "Cannot reassign a cdrom drive\n" if PVE::QemuServer::drive_is_cdrom($drive, 1); + + die "Cannot reassign disk while the source VM is running\n" + if PVE::QemuServer::check_running($vmid) && $disk !~ m/unused[0-9]/; + + return PVE::QemuConfig->lock_config($target_vmid, sub { + my $target_conf = PVE::QemuConfig->load_config($target_vmid); + PVE::QemuConfig->check_lock($target_conf); + + die "Target VM config checksum missmatch (file change by other user?)\n" + if $target_digest && $target_digest ne $target_conf->{digest}; + + PVE::Cluster::log_msg('info', $authuser, "reassign disk VM $vmid: reassign --disk $disk --target_vmid $target_vmid"); + + my $realcmd = sub { + my $new_volid = PVE::Storage::reassign_volume($storecfg, $drive->{file}, $target_vmid); + + delete $conf->{$disk}; + PVE::QemuConfig->write_config($vmid, $conf); + + PVE::QemuConfig->add_unused_volume($target_conf, $new_volid); + PVE::QemuConfig->write_config($target_vmid, $target_conf); + }; + + return $rpcenv->fork_worker('qmreassign', $vmid, $authuser, $realcmd); + }); + }); + }}); + 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