[PATCH manager 2/4] migrations: cephx: judge cipher support by the version a daemon runs

[Kefu Chai <k.chai@proxmox.com>]

A daemon's running version and its installed version answer different
questions: the running version decides whether it can hold a key in the
new cipher now, and Ceph reports that for every daemon; the installed
version decides whether it still could after a restart, and only
pvestatd, on a node of this cluster, reports that.

The check demanded both, so a daemon on a host this cluster does not
manage had no installed version, and the run stopped unconditionally
with "Could not verify 'aes256k' support for every service daemon".
That hit every daemon whenever a run still had to switch the service
cipher, which is every run before the switch unless '--only' narrows it,
so a stretch cluster with its tiebreaker outside this cluster could not
migrate anything.

Judge such a daemon by the version it runs instead, and note that only
the running version was checked, so an operator knows to keep Ceph there
at that version or newer. The run still stops if the daemon is down,
since nothing then reports a version, or if the running version lacks
cipher support.

Signed-off-by: Kefu Chai <k.chai@proxmox.com>
---
 bin/pve-cephx-rotate-service-keys   | 44 +++++++++++++++----
 test/CephKeyMigrationScript_test.pl | 68 +++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 9 deletions(-)

diff --git a/bin/pve-cephx-rotate-service-keys b/bin/pve-cephx-rotate-service-keys
index 95d10266..28161306 100755
--- a/bin/pve-cephx-rotate-service-keys
+++ b/bin/pve-cephx-rotate-service-keys
@@ -2554,31 +2554,50 @@ my sub preflight_nodes($info, $plan, $opts) {
             (@touched, grep { defined($_->{node}) && $lockbox_nodes->{ $_->{node} } } @all);
     }
 
-    my (@outdated, @unknown_versions);
+    # Two versions answer two questions. What a daemon runs decides whether it can hold a key in
+    # the new cipher now, and Ceph reports that for every daemon of the cluster. What is installed
+    # on its node decides whether it still could after a restart, and only pvestatd on a node of
+    # this cluster publishes that. A daemon this cluster does not manage is judged by the first
+    # alone, which is sound while it runs and nothing at all once it stops.
+    my (@outdated, @unknown_versions, @running_only);
     for my $daemon (@judged) {
-        if (!eval { assert_member($daemon->{node}, "$daemon->{type}.$daemon->{id}"); 1 }) {
-            push @unknown_versions, $@;
+        my $label = "$daemon->{type}.$daemon->{id}";
+        my $stopped = ($daemon->{recovered} || $daemon->{down}) ? 1 : 0;
+
+        if (!defined($daemon->{node})) {
+            push @unknown_versions, "Cannot manage '$label': no valid host name was reported";
+            next;
+        }
+        if (!$daemon->{managed} && $stopped) {
+            push @unknown_versions,
+                "cannot verify the Ceph version of $label: it does not run, and its host"
+                . " '$daemon->{node}' is not a node of this Proxmox VE cluster";
             next;
         }
-        if (!$daemon->{recovered} && !$daemon->{down}) {
+
+        if (!$stopped) {
             if (!defined($daemon->{version}) || $daemon->{version} eq '') {
                 push @unknown_versions,
-                    "could not verify the running Ceph version of $daemon->{entity} on node"
+                    "could not verify the running Ceph version of $label on node"
                     . " '$daemon->{node}'";
             } elsif (!version_has_cipher($daemon->{version})) {
                 push @outdated,
-                    "$daemon->{entity} on node '$daemon->{node}' runs "
-                    . short_version($daemon->{version});
+                    "$label on node '$daemon->{node}' runs " . short_version($daemon->{version});
+            } elsif (!$daemon->{managed}) {
+                push @running_only,
+                    "$label on '$daemon->{node}' runs " . short_version($daemon->{version});
             }
         }
 
+        next if !$daemon->{managed};
+
         if (!defined($daemon->{binary}) || $daemon->{binary} eq '') {
             push @unknown_versions,
-                "could not verify the installed Ceph version of $daemon->{entity} on node"
+                "could not verify the installed Ceph version of $label on node"
                 . " '$daemon->{node}'; check that 'pvestatd' runs there";
         } elsif (!version_has_cipher($daemon->{binary})) {
             push @outdated,
-                "$daemon->{entity} on node '$daemon->{node}' would restart into "
+                "$label on node '$daemon->{node}' would restart into "
                 . short_version($daemon->{binary});
         }
     }
@@ -2645,6 +2664,13 @@ my sub preflight_nodes($info, $plan, $opts) {
         }
     }
 
+    if (@running_only) {
+        log_warn("These daemons run on hosts this cluster does not manage, so only the version they"
+            . " run was checked, not the one installed there. Keep Ceph on those hosts at this"
+            . " version or newer, or they lose the new cipher on their next restart:");
+        log_steps(\@running_only);
+    }
+
     if (@unknown_versions) {
         log_fail("Could not verify '$CIPHER' support for every service daemon:");
         log_steps(\@unknown_versions);
diff --git a/test/CephKeyMigrationScript_test.pl b/test/CephKeyMigrationScript_test.pl
index fa1cee4e..a0b2fe95 100755
--- a/test/CephKeyMigrationScript_test.pl
+++ b/test/CephKeyMigrationScript_test.pl
@@ -6166,6 +6166,7 @@ for my $case ([0, 0], [1, 0], [0, 1], [1, 1]) {
                 id => "$_",
                 entity => "mon.$_",
                 node => "node$_",
+                managed => 1,
                 store => 'file',
                 version => '20.2.4-pve4',
                 binary => '20.2.4-pve4',
@@ -8026,4 +8027,71 @@ for my $case ([0, 0], [1, 0], [0, 1], [1, 1]) {
     );
 }
 
+# only cluster nodes publish their installed Ceph version, so a daemon on a host outside the
+# cluster can only be judged by the version it runs
+{
+    my $info = sub {
+        my (%tiebreaker) = @_;
+        my $mons = [
+            (
+                map { {
+                    type => 'mon',
+                    id => "$_",
+                    entity => "mon.$_",
+                    node => "node$_",
+                    managed => 1,
+                    store => 'file',
+                    version => '20.2.4-pve4',
+                    binary => '20.2.4-pve4',
+                } } 1 .. 2
+            ),
+            {
+                type => 'mon',
+                id => 'tiebreaker',
+                # every monitor shares the one 'mon.' key, and that is the entity the inventory
+                # records for each of them
+                entity => 'mon.',
+                node => 'tiebreaker.invalid',
+                managed => 0,
+                store => 'file',
+                version => '20.2.4-pve4',
+                %tiebreaker,
+            },
+        ];
+        return {
+            daemons => { mon => $mons, mgr => [], mds => [], osd => [] },
+            monmap_mons => [map { $_->{id} } @$mons],
+            rados => MonCountRados->new(),
+        };
+    };
+    my $preflight = sub {
+        my ($info) = @_;
+        my ($output, $verdict) = ('');
+        {
+            local *STDOUT;
+            open(STDOUT, '>', \$output) or die $!;
+            $verdict = $HOOKS->{preflight_nodes}
+                ->($info, { daemons => [], lockbox_keys => [], service_cipher => 1 }, {});
+        }
+        return ($verdict, $output // '');
+    };
+
+    my ($verdict, $output) = $preflight->($info->());
+    is($verdict, 1, 'a capable monitor outside the cluster does not block the cipher switch');
+    like(
+        $output,
+        qr/does not manage.*mon\.tiebreaker on 'tiebreaker\.invalid' runs 20\.2\.4/s,
+        'naming it with the version it was judged by',
+    );
+
+    ($verdict, $output) = $preflight->($info->(version => '19.2.5'));
+    is($verdict, -1, 'an outdated monitor outside the cluster still blocks');
+    like($output, qr/mon\.tiebreaker on node 'tiebreaker\.invalid' runs 19\.2\.5/, 'as outdated');
+
+    ($verdict, $output) = $preflight->($info->(down => 1));
+    is($verdict, -1, 'a stopped monitor outside the cluster cannot be judged');
+    like($output, qr/mon\.tiebreaker.*not a node/, 'which names the membership');
+
+}
+
 done_testing();
-- 
2.47.3