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

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

The cipher check needs both the running and the installed version of a
daemon. Only pvestatd reports the installed version, and only on our
nodes. So a daemon on another host always failed the check, and a
stretch cluster with an external tiebreaker could not migrate.

For such a daemon, check only the running version and warn that Ceph on
that host must not be downgraded. A stopped daemon still fails the
check, as nothing reports its version.

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

diff --git a/bin/pve-cephx-rotate-service-keys b/bin/pve-cephx-rotate-service-keys
index 84daff6d..8048944b 100755
--- a/bin/pve-cephx-rotate-service-keys
+++ b/bin/pve-cephx-rotate-service-keys
@@ -189,9 +189,13 @@ my sub daemon_location($type, $id, $host) {
     };
 }
 
-my sub assert_member($node, $entity) {
+my sub assert_located($node, $entity) {
     die "Cannot manage '$entity': no valid host name was reported\n"
         if !defined($node) || ref($node) || !length($node);
+}
+
+my sub assert_member($node, $entity) {
+    assert_located($node, $entity);
     die "Cannot manage '$entity' on '$node', which is not a node of this Proxmox VE cluster\n"
         if !cluster_nodes()->{$node};
 }
@@ -2545,31 +2549,49 @@ my sub preflight_nodes($info, $plan, $opts) {
             (@touched, grep { defined($_->{node}) && $lockbox_nodes->{ $_->{node} } } @all);
     }
 
-    my (@outdated, @unknown_versions);
+    # The running version tells whether a daemon can use the new cipher now, and Ceph reports it for
+    # every daemon. The installed version tells whether it still can after a restart, and only
+    # pvestatd reports it, for our nodes. A daemon elsewhere is checked by its running version only,
+    # so it has to be running.
+    my (@outdated, @unknown_versions, @running_only);
     for my $daemon (@judged) {
-        if (!eval { assert_member($daemon->{node}, "$daemon->{type}.$daemon->{id}"); 1 }) {
+        my $label = "$daemon->{type}.$daemon->{id}";
+        my $stopped = ($daemon->{recovered} || $daemon->{down}) ? 1 : 0;
+
+        if (!eval { assert_located($daemon->{node}, $label); 1 }) {
             push @unknown_versions, $@;
             next;
         }
-        if (!$daemon->{recovered} && !$daemon->{down}) {
+        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 (!$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});
         }
     }
@@ -2636,6 +2658,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 fc9811d7..6ccadbc4 100755
--- a/test/CephKeyMigrationScript_test.pl
+++ b/test/CephKeyMigrationScript_test.pl
@@ -6171,6 +6171,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',
@@ -8030,4 +8031,69 @@ for my $case ([0, 0], [1, 0], [0, 1], [1, 1]) {
     );
 }
 
+# a daemon outside the cluster has no installed version, so only its running version counts
+{
+    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',
+                # all monitors share the 'mon.' key, as the inventory records
+                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