[pve-devel] [PATCH v3 cluster 1/4] setup: split generation of local (i.e. non-pmxcfs) files out into helper

[pve-devel] [PATCH v3 cluster 1/4] setup: split generation of local (i.e. non-pmxcfs) files out into helper

[Fiona Ebner]

In preparation to wait for quorum in the updatecerts command. The
generation of files that do not depend on quorum should still be done
beforehand.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

No changes in v3.
New in v2.

 src/PVE/CLI/pvecm.pm     |  1 +
 src/PVE/Cluster/Setup.pm | 10 ++++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/PVE/CLI/pvecm.pm b/src/PVE/CLI/pvecm.pm
index 564dc99..ebc15bd 100755
--- a/src/PVE/CLI/pvecm.pm
+++ b/src/PVE/CLI/pvecm.pm
@@ -576,6 +576,7 @@ __PACKAGE__->register_method ({
 	# IO (on /etc/pve) which can hang (uninterruptedly D state). That'd be
 	# no-good for ExecStartPre as it fails the whole service in this case
 	PVE::Tools::run_fork_with_timeout(30, sub {
+	    PVE::Cluster::Setup::generate_local_files();
 	    PVE::Cluster::Setup::updatecerts_and_ssh($param->@{qw(force silent)});
 	    PVE::Cluster::prepare_observed_file_basedirs();
 	});
diff --git a/src/PVE/Cluster/Setup.pm b/src/PVE/Cluster/Setup.pm
index 108817e..f6b491c 100644
--- a/src/PVE/Cluster/Setup.pm
+++ b/src/PVE/Cluster/Setup.pm
@@ -786,6 +786,7 @@ sub finish_join {
     }
     print "OK\n" if !$printqmsg;
 
+    generate_local_files();
     updatecerts_and_ssh(1);
 
     print "generated new node certificate, restart pveproxy and pvedaemon services\n";
@@ -794,15 +795,16 @@ sub finish_join {
     print "successfully added node '$nodename' to cluster.\n";
 }
 
+sub generate_local_files {
+    setup_rootsshconfig();
+    gen_pve_vzdump_symlink();
+}
+
 sub updatecerts_and_ssh {
     my ($force_new_cert, $silent) = @_;
 
     my $p = sub { print "$_[0]\n" if !$silent };
 
-    setup_rootsshconfig();
-
-    gen_pve_vzdump_symlink();
-
     if (!PVE::Cluster::check_cfs_quorum(1)) {
 	return undef if $silent;
 	die "no quorum - unable to update files\n";
-- 
2.39.2

[pve-devel] [PATCH v3 cluster 2/4] pvecm: updatecerts: wait for quorum

[Fiona Ebner]

Mostly useful for the updatecerts call triggered via the ExecStartPre
hook for pveproxy.service.

When starting a node that's part of a cluster, there is a time window
between the start of pve-cluster.service and when quorum is reached
(from the node's perspective). pveproxy.service is ordered after
pve-cluster.service, but that does not prevent the ExecStartPre hook
from being executed before the node is part of the quorate partition.

The pvecm updatecerts command won't do much without quorum. Generating
local (non-pmxcfs) files is still done before waiting on quorum.

In particular, it might happen that the base directories for observed
files will not get created during/after the upgrade from Proxmox VE 7
to 8 (reported in the community forum [0] and reproduced right away in
a virtual test cluster).

Waiting on quorum should highly increase the chances for successful
execution of the ExecStartPre hook.

[0]: https://forum.proxmox.com/threads/129644/

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v3:
    * Fix commit title.
Changes in v2:
    * Different approach: always wait for quorum until timeout.

 src/PVE/CLI/pvecm.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/PVE/CLI/pvecm.pm b/src/PVE/CLI/pvecm.pm
index ebc15bd..a0550c2 100755
--- a/src/PVE/CLI/pvecm.pm
+++ b/src/PVE/CLI/pvecm.pm
@@ -6,6 +6,8 @@ use warnings;
 use Cwd qw(getcwd);
 use File::Path;
 use File::Basename;
+use Time::HiRes qw(usleep);
+
 use PVE::Tools qw(run_command);
 use PVE::Cluster;
 use PVE::INotify;
@@ -577,6 +579,12 @@ __PACKAGE__->register_method ({
 	# no-good for ExecStartPre as it fails the whole service in this case
 	PVE::Tools::run_fork_with_timeout(30, sub {
 	    PVE::Cluster::Setup::generate_local_files();
+
+	    for (my $i = 0; !PVE::Cluster::check_cfs_quorum(1); $i++) {
+		print "waiting for pmxcfs mount to appear and get quorate...\n" if $i % 50 == 0;
+		usleep(100 * 1000);
+	    }
+
 	    PVE::Cluster::Setup::updatecerts_and_ssh($param->@{qw(force silent)});
 	    PVE::Cluster::prepare_observed_file_basedirs();
 	});
-- 
2.39.2

[pve-devel] [PATCH v3 cluster 3/4] pvecm: updatecerts: silence warning that potentially has no context

[Fiona Ebner]

If timeout is reached, run_fork_with_timeout will warn "got timeout".
When processing triggers for pve-manager (because of ExecStartPre of
pveproxy.service invoking pvecm updatecerts) that warning can appear
in the apt output without any context (output of the forked sub
doesn't appear there). So make sure to silence it, if the silent param
is set.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

No changes in v3.
New in v2.

Can't use a normal if here, because of the 'local'.

 src/PVE/CLI/pvecm.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/PVE/CLI/pvecm.pm b/src/PVE/CLI/pvecm.pm
index a0550c2..2cdc2cd 100755
--- a/src/PVE/CLI/pvecm.pm
+++ b/src/PVE/CLI/pvecm.pm
@@ -574,6 +574,17 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
+	# If timeout is reached, run_fork_with_timeout will warn "got timeout". When processing
+	# triggers for pve-manager (because of ExecStartPre of pveproxy.service invoking this
+	# command here) that warning can appear in the apt output without any context (output of the
+	# forked sub doesn't appear there). So make sure to silence it, if the silent param is set.
+	my $filter_timeout_warning = sub {
+	    my ($msg) = @_;
+	    return if $msg =~ m/^got timeout/;
+	    print STDERR $msg;
+	};
+	local $SIG{__WARN__} = $filter_timeout_warning if $param->{silent};
+
 	# we get called by the pveproxy.service ExecStartPre and as we do
 	# IO (on /etc/pve) which can hang (uninterruptedly D state). That'd be
 	# no-good for ExecStartPre as it fails the whole service in this case
-- 
2.39.2

[pve-devel] [PATCH v3 cluster 4/4] fix typo

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

No changes in v3.
No changes in v2.

 src/PVE/Cluster.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Cluster.pm b/src/PVE/Cluster.pm
index ff777ba..c310a67 100644
--- a/src/PVE/Cluster.pm
+++ b/src/PVE/Cluster.pm
@@ -93,7 +93,7 @@ sub prepare_observed_file_basedirs {
 	next if $f !~ m!^(.*)/[^/]+$!;
 	my $dir = "$basedir/$1";
 	next if -e $dir; # can also be a link, so just use -e xist check
-	print "creating directory '$dir' for observerd files\n";
+	print "creating directory '$dir' for observed files\n";
 	make_path($dir);
     }
 }
-- 
2.39.2

Re: [pve-devel] [PATCH v3 cluster 1/4] setup: split generation of local (i.e. non-pmxcfs) files out into helper

[Fiona Ebner]

Ping for the series

Am 30.06.23 um 13:59 schrieb Fiona Ebner:
> In preparation to wait for quorum in the updatecerts command. The
> generation of files that do not depend on quorum should still be done
> beforehand.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Re: [pve-devel] [PATCH v3 cluster 1/4] setup: split generation of local (i.e. non-pmxcfs) files out into helper

[Thomas Lamprecht]

Am 30/08/2023 um 11:14 schrieb Fiona Ebner:
> Ping for the series

I already applied all but 3/4 "pvecm: updatecerts: silence warning that
potentially has no contex" one, which was replaced by


https://git.proxmox.com/?p=pve-common.git;a=commitdiff;h=a6aa0ae9453190e029dad43dfe3d19e830c1543a
and 
https://git.proxmox.com/?p=pve-cluster.git;a=commitdiff;h=66b0e690fec93c63952645213cc3110df3d56162

I seemingly forgot to reply here, sorry. Can you please recheck if nothing
was missed?

Re: [pve-devel] [PATCH v3 cluster 1/4] setup: split generation of local (i.e. non-pmxcfs) files out into helper

[Fiona Ebner]

Am 30.08.23 um 11:37 schrieb Thomas Lamprecht:
> Am 30/08/2023 um 11:14 schrieb Fiona Ebner:
>> Ping for the series
> 
> I already applied all but 3/4 "pvecm: updatecerts: silence warning that
> potentially has no contex" one, which was replaced by
> 
> 
> https://git.proxmox.com/?p=pve-common.git;a=commitdiff;h=a6aa0ae9453190e029dad43dfe3d19e830c1543a

Sent fixes for this one, because I think it ignores a bit too much now.

> and 
> https://git.proxmox.com/?p=pve-cluster.git;a=commitdiff;h=66b0e690fec93c63952645213cc3110df3d56162
> 
> I seemingly forgot to reply here, sorry. Can you please recheck if nothing
> was missed?
> 

Thanks! This is certainly better then the manual filter I proposed. The
rest looks fine to me :)