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 7F8B272F7B for ; Thu, 17 Jun 2021 12:52:42 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 72DDA1A7E6 for ; Thu, 17 Jun 2021 12:52:12 +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 58EC41A7C7 for ; Thu, 17 Jun 2021 12:52:11 +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 330CB441B9 for ; Thu, 17 Jun 2021 12:52:11 +0200 (CEST) From: Oguz Bektas To: pve-devel@lists.proxmox.com Date: Thu, 17 Jun 2021 12:51:59 +0200 Message-Id: <20210617105201.804786-3-o.bektas@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20210617105201.804786-1-o.bektas@proxmox.com> References: <20210617105201.804786-1-o.bektas@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.705 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 v5 container 2/4] clone_vm: improve config 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, 17 Jun 2021 10:52:42 -0000 cleaned up the locking situation with config files as Fabian G. suggested in the review. use the 'create_and_lock_config' helper in the beginning to ensure that the target CTID is available, and that the target config is locked from the beginning. in case any error happens during the initial checks, we unlink this config in error handling. firewall config is also now cloned inside the worker instead of before the worker, in case the clone fails. also lock the config file when renaming the conf (for moving to a target node when the option is passed). Signed-off-by: Oguz Bektas --- v4->v5: * move create_and_lock_config outside the eval * also lock moving config to target node src/PVE/API2/LXC.pm | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm index 936debb..ade109b 100644 --- a/src/PVE/API2/LXC.pm +++ b/src/PVE/API2/LXC.pm @@ -1394,6 +1394,9 @@ __PACKAGE__->register_method({ my $vollist = []; my $running; + PVE::LXC::Config->create_and_lock_config($newid, 0); + $conffile = PVE::LXC::Config->config_file($newid); + PVE::LXC::Config->lock_config($vmid, sub { my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk'); @@ -1411,10 +1414,6 @@ __PACKAGE__->register_method({ my $src_conf = $snapname ? $src_conf->{snapshots}->{$snapname} : $src_conf; - $conffile = PVE::LXC::Config->config_file($newid); - die "unable to create CT $newid: config file already exists\n" - if -f $conffile; - my $sharedvm = 1; for my $opt (sort keys %$src_conf) { next if $opt =~ m/^unused\d+$/; @@ -1482,11 +1481,23 @@ __PACKAGE__->register_method({ $newconf->{description} = $param->{description}; } - # create empty/temp config - this fails if CT already exists on other node - PVE::LXC::Config->write_config($newid, $newconf); + PVE::LXC::Config->lock_config($newid, sub { + # read empty config, lock needs to be still here + my $conf = PVE::LXC::Config->load_config($newid); + die "Lost 'create' config lock, aborting.\n" + if !PVE::LXC::Config->has_lock($conf, 'create'); + # write the actual new config now to disk + PVE::LXC::Config->write_config($newid, $newconf); + }); }; if (my $err = $@) { eval { PVE::LXC::Config->remove_lock($vmid, 'disk') }; + PVE::LXC::Config->lock_config($newid, sub { + my $conf = PVE::LXC::Config->load_config($newid); + die "Lost 'create' config lock, aborting.\n" + if !PVE::LXC::Config->has_lock($conf, 'create'); + unlink($conffile); + }); warn $@ if $@; die $err; } @@ -1545,18 +1556,19 @@ __PACKAGE__->register_method({ PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool; PVE::LXC::Config->remove_lock($newid, 'create'); - if ($target) { - # always deactivate volumes - avoid lvm LVs to be active on several nodes - PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running; - PVE::Storage::deactivate_volumes($storecfg, $newvollist); + PVE::LXC::Config->lock_config($newid, sub { + if ($target) { + # always deactivate volumes - avoid lvm LVs to be active on several nodes + PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running; + PVE::Storage::deactivate_volumes($storecfg, $newvollist); - my $newconffile = PVE::LXC::Config->config_file($newid, $target); - die "Failed to move config to node '$target' - rename failed: $!\n" - if !rename($conffile, $newconffile); - } + my $newconffile = PVE::LXC::Config->config_file($newid, $target); + die "Failed to move config to node '$target' - rename failed: $!\n" + if !rename($conffile, $newconffile); + } + }); }; my $err = $@; - # Unlock the source config in any case: eval { PVE::LXC::Config->remove_lock($vmid, 'disk') }; warn $@ if $@; @@ -1574,10 +1586,10 @@ __PACKAGE__->register_method({ die "clone failed: $err"; } + PVE::Firewall::clone_vmfw_conf($vmid, $newid); return; }; - PVE::Firewall::clone_vmfw_conf($vmid, $newid); return $rpcenv->fork_worker('vzclone', $vmid, $authuser, $realcmd); }}); -- 2.20.1