[pve-devel] [PATCH cluster 2/2] pvecm: add/delete: local lock & avoid problems on first node addition

[Thomas Lamprecht]

cfs_lock is per node, thus we had a possibility for a node addition
race if the process was started on the same node (e.g. by a
script/ansible/...).

So always request a local lock first, if that is acquired check how
many members currently reside in the cluster and then decide if we
can directly execute the code (single node cluster = no contenders)
or must hold the lock.

One may think that there remains a race when adding a node to single
node cluster, i.e., once the node is added it may itself be a target
for another joining node. But this cannot happen as we only tell the
joining node that it could be added once we already *have* added it
locally.

Besides the defense against a race if two user execute a node
addition to the same node at the same time, this also addresses a
issue where the cluster lock could not be removed after writing the
corosync conf, as pmxcfs and corosync triggered an config reload and
added the new node, which itself did not yet know that it was
accepted in the cluster. Thus, the former single node cluster expects
two nodes but has only one for now, until the other node pulled the
config and authkey and started up its cluster stack.

That resulted in a failing removal of the corosync lock, thus adding
another node did not work until this lock timed out (~2 minutes).

While often node additions are separated by more than 2 minutes time
intervall, deployment helpers (or fast admins, for that matter) may
trigger this easily.

Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
---
 data/PVE/CLI/pvecm.pm | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/data/PVE/CLI/pvecm.pm b/data/PVE/CLI/pvecm.pm
index ba72bfe..cdba7b2 100755
--- a/data/PVE/CLI/pvecm.pm
+++ b/data/PVE/CLI/pvecm.pm
@@ -58,6 +58,24 @@ sub backup_database {
     }
 }
 
+# lock method to ensure local and cluster wide atomicity
+# if we're a single node cluster just lock locally, we have no other cluster
+# node which we could contend with, else also acquire a cluster wide lock
+my $config_change_lock = sub {
+    my ($code) = @_;
+
+    my $local_lock_fn = "/var/lock/pvecm.lock";
+    PVE::Tools::lock_file($local_lock_fn, 10, sub {
+	PVE::Cluster::cfs_update(1);
+	my $members = PVE::Cluster::get_members();
+	if (scalar(keys %$members) > 1) {
+	    return PVE::Cluster::cfs_lock_file('corosync.conf', 10, $code);
+	} else {
+	    return $code->();
+	}
+    });
+};
+
 __PACKAGE__->register_method ({
     name => 'keygen',
     path => 'keygen',
@@ -386,7 +404,7 @@ __PACKAGE__->register_method ({
 	    PVE::Corosync::update_nodelist($conf, $nodelist);
 	};
 
-	PVE::Cluster::cfs_lock_file('corosync.conf', 10, $code);
+	$config_change_lock->($code);
 	die $@ if $@;
 
 	exit (0);
@@ -447,7 +465,7 @@ __PACKAGE__->register_method ({
 	    run_command(['corosync-cfgtool','-k', $nodeid]) if defined($nodeid);
 	};
 
-	PVE::Cluster::cfs_lock_file('corosync.conf', 10, $code);
+	$config_change_lock->($code);
 	die $@ if $@;
 
 	return undef;
-- 
2.11.0