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 0BD4163D56 for ; Thu, 27 Jan 2022 15:02:34 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C9F992393E for ; Thu, 27 Jan 2022 15:02:03 +0100 (CET) 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 0E8AF237F4 for ; Thu, 27 Jan 2022 15:02:00 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id DB22F46CC7 for ; Thu, 27 Jan 2022 15:01:59 +0100 (CET) From: Fabian Ebner To: pve-devel@lists.proxmox.com Date: Thu, 27 Jan 2022 15:01:54 +0100 Message-Id: <20220127140155.66141-4-f.ebner@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20220127140155.66141-1-f.ebner@proxmox.com> References: <20220127140155.66141-1-f.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.133 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pve-devel] [PATCH qemu-server 3/4] api: move disk: fork before locking 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: Thu, 27 Jan 2022 14:02:34 -0000 using the familiar early+repeated checks pattern from other API calls. Only intended functional changes are with regard to locking/forking. Signed-off-by: Fabian Ebner --- Best viewed with -w PVE/API2/Qemu.pm | 162 +++++++++++++++++++++++++---------------------- 1 file changed, 86 insertions(+), 76 deletions(-) diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index 38e08c8..59e083e 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -3396,7 +3396,7 @@ __PACKAGE__->register_method({ my $storecfg = PVE::Storage::config(); - my $move_updatefn = sub { + my $load_and_check_move = sub { my $conf = PVE::QemuConfig->load_config($vmid); PVE::QemuConfig->check_lock($conf); @@ -3416,8 +3416,8 @@ __PACKAGE__->register_method({ $oldfmt = $1; } - die "you can't move to the same storage with same format\n" if $oldstoreid eq $storeid && - (!$format || !$oldfmt || $oldfmt eq $format); + die "you can't move to the same storage with same format\n" + if $oldstoreid eq $storeid && (!$format || !$oldfmt || $oldfmt eq $format); # this only checks snapshots because $disk is passed! my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use( @@ -3429,6 +3429,13 @@ __PACKAGE__->register_method({ die "you can't move a disk with snapshots and delete the source\n" if $snapshotted && $param->{delete}; + return ($conf, $drive, $oldstoreid, $snapshotted); + }; + + my $move_updatefn = sub { + my ($conf, $drive, $oldstoreid, $snapshotted) = $load_and_check_move->(); + my $old_volid = $drive->{file}; + PVE::Cluster::log_msg( 'info', $authuser, @@ -3439,83 +3446,79 @@ __PACKAGE__->register_method({ PVE::Storage::activate_volumes($storecfg, [ $drive->{file} ]); - my $realcmd = sub { - my $newvollist = []; + my $newvollist = []; + + eval { + local $SIG{INT} = + local $SIG{TERM} = + local $SIG{QUIT} = + local $SIG{HUP} = sub { die "interrupted by signal\n"; }; + + warn "moving disk with snapshots, snapshots will not be moved!\n" + if $snapshotted; + + my $bwlimit = extract_param($param, 'bwlimit'); + my $movelimit = PVE::Storage::get_bandwidth_limit( + 'move', + [$oldstoreid, $storeid], + $bwlimit + ); + + my $newdrive = PVE::QemuServer::clone_disk( + $storecfg, + $vmid, + $running, + $disk, + $drive, + undef, + $vmid, + $storeid, + $format, + 1, + $newvollist, + undef, + undef, + undef, + $movelimit, + $conf, + ); + $conf->{$disk} = PVE::QemuServer::print_drive($newdrive); + + PVE::QemuConfig->add_unused_volume($conf, $old_volid) if !$param->{delete}; + + # convert moved disk to base if part of template + PVE::QemuServer::template_create($vmid, $conf, $disk) + if PVE::QemuConfig->is_template($conf); + + PVE::QemuConfig->write_config($vmid, $conf); + + my $do_trim = PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks'); + if ($running && $do_trim && PVE::QemuServer::qga_check_running($vmid)) { + eval { mon_cmd($vmid, "guest-fstrim") }; + } eval { - local $SIG{INT} = - local $SIG{TERM} = - local $SIG{QUIT} = - local $SIG{HUP} = sub { die "interrupted by signal\n"; }; - - warn "moving disk with snapshots, snapshots will not be moved!\n" - if $snapshotted; - - my $bwlimit = extract_param($param, 'bwlimit'); - my $movelimit = PVE::Storage::get_bandwidth_limit( - 'move', - [$oldstoreid, $storeid], - $bwlimit - ); - - my $newdrive = PVE::QemuServer::clone_disk( - $storecfg, - $vmid, - $running, - $disk, - $drive, - undef, - $vmid, - $storeid, - $format, - 1, - $newvollist, - undef, - undef, - undef, - $movelimit, - $conf, - ); - $conf->{$disk} = PVE::QemuServer::print_drive($newdrive); - - PVE::QemuConfig->add_unused_volume($conf, $old_volid) if !$param->{delete}; - - # convert moved disk to base if part of template - PVE::QemuServer::template_create($vmid, $conf, $disk) - if PVE::QemuConfig->is_template($conf); - - PVE::QemuConfig->write_config($vmid, $conf); - - my $do_trim = PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks'); - if ($running && $do_trim && PVE::QemuServer::qga_check_running($vmid)) { - eval { mon_cmd($vmid, "guest-fstrim") }; - } - - eval { - # try to deactivate volumes - avoid lvm LVs to be active on several nodes - PVE::Storage::deactivate_volumes($storecfg, [ $newdrive->{file} ]) - if !$running; - }; - warn $@ if $@; + # try to deactivate volumes - avoid lvm LVs to be active on several nodes + PVE::Storage::deactivate_volumes($storecfg, [ $newdrive->{file} ]) + if !$running; }; - if (my $err = $@) { - foreach my $volid (@$newvollist) { - eval { PVE::Storage::vdisk_free($storecfg, $volid) }; - warn $@ if $@; - } - die "storage migration failed: $err"; - } - - if ($param->{delete}) { - eval { - PVE::Storage::deactivate_volumes($storecfg, [$old_volid]); - PVE::Storage::vdisk_free($storecfg, $old_volid); - }; - warn $@ if $@; - } + warn $@ if $@; }; + if (my $err = $@) { + foreach my $volid (@$newvollist) { + eval { PVE::Storage::vdisk_free($storecfg, $volid) }; + warn $@ if $@; + } + die "storage migration failed: $err"; + } - return $rpcenv->fork_worker('qmmove', $vmid, $authuser, $realcmd); + if ($param->{delete}) { + eval { + PVE::Storage::deactivate_volumes($storecfg, [$old_volid]); + PVE::Storage::vdisk_free($storecfg, $old_volid); + }; + warn $@ if $@; + } }; my $load_and_check_reassign_configs = sub { @@ -3695,7 +3698,14 @@ __PACKAGE__->register_method({ 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); + + $load_and_check_move->(); # early checks before forking/locking + + my $realcmd = sub { + PVE::QemuConfig->lock_config($vmid, $move_updatefn); + }; + + return $rpcenv->fork_worker('qmmove', $vmid, $authuser, $realcmd); } else { my $msg = "both 'storage' and 'target-vmid' missing, either needs to be set"; raise_param_exc({ 'target-vmid' => $msg, 'storage' => $msg }); -- 2.30.2