From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <o.bektas@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Thu, 17 Jun 2021 12:52:11 +0200 (CEST)
From: Oguz Bektas <o.bektas@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=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 <o.bektas@proxmox.com>
---
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