[PATCH manager v2 3/3] migrations: cephx: rotate daemons whose key on disk differs

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

A daemon whose key on disk differs from the monitors' is now only
reported, and fixing it is left to the admin, although the helper
already knows how to bring every copy of a daemon key in line.

Have needs_rotation() count such a daemon, so the plan takes it like
one still on the old cipher and migrating it writes a key the monitors
accept to every copy. A daemon Ceph does not list stays stopped as
before, and as it may have stopped because of that very key, the plan
and the run now name the commands that start it, with or without
'--restart-daemons'. They clear the unit's start limit first, since
failed starts can use it up, and 'systemctl start' alone is then
refused.

A daemon an earlier run rotated can now reach the branch that keeps its
current key after that key was changed by hand. Its message claimed the
earlier run produced the key, so fold it into the branch that keeps any
key already on the new cipher, which says only that.

Signed-off-by: Kefu Chai <k.chai@proxmox.com>
---
 PVE/Ceph/KeyMigration.pm            |  3 ++
 bin/pve-cephx-rotate-service-keys   | 41 +++++++++++++---------
 test/CephKeyMigrationScript_test.pl | 53 ++++++++++++++++++++++++-----
 test/CephKeyMigration_test.pl       | 16 +++++++++
 4 files changed, 88 insertions(+), 25 deletions(-)

diff --git a/PVE/Ceph/KeyMigration.pm b/PVE/Ceph/KeyMigration.pm
index 0cf2fc0e..45d64dc6 100644
--- a/PVE/Ceph/KeyMigration.pm
+++ b/PVE/Ceph/KeyMigration.pm
@@ -376,6 +376,9 @@ sub disk_key_verdict($info, $daemon, $probe) {
 }
 
 sub needs_rotation($info, $entity) {
+    # a key changed by hand reads as migrated in the auth database
+    return 1 if ($info->{stale_keys} // {})->{$entity};
+
     my $entry = $info->{exported}->{$entity};
 
     # 'ceph-mon --mkfs' moves 'mon.' out of the auth database into the monitor keyrings, so judge
diff --git a/bin/pve-cephx-rotate-service-keys b/bin/pve-cephx-rotate-service-keys
index 7b6bd536..f3af6cfa 100755
--- a/bin/pve-cephx-rotate-service-keys
+++ b/bin/pve-cephx-rotate-service-keys
@@ -1766,8 +1766,7 @@ my sub check_disk_keys($info, $state, $opts) {
             push @doubts, [$entity, $text];
         }
     }
-    # a stale copy is not planned for repair yet, so it also keeps the verdict from passing
-    $disk_key_doubts = [(map { $_->[0] } @doubts), sort keys $info->{stale_keys}->%*];
+    $disk_key_doubts = [map { $_->[0] } @doubts];
     if (@doubts) {
         log_warn("Could not compare the key on disk with the monitors' for these daemons, so a"
             . " key changed outside this script would go unnoticed there:");
@@ -2560,6 +2559,7 @@ my sub preflight_cluster(
         && !restrict_wanted($info, $opts)
         && !$recovered_count
         && !@unfinished
+        && !%{ $info->{stale_keys} // {} }
     ) {
         log_verdict("Every service key uses '$CIPHER', and so do the service tickets.");
         mon_key_hint($info, $opts);
@@ -2964,6 +2964,12 @@ my sub recover_left_behind($info, $state) {
     return \@recovered;
 }
 
+# failed starts may have used up the unit's start limit, and a plain start is refused then
+my sub start_command($type, $id) {
+    my $unit = "ceph-$type\@$id";
+    return "systemctl reset-failed $unit; systemctl start $unit";
+}
+
 my sub print_plan($info, $plan, $state, $opts, $storage_entities) {
     $storage_entities //= {};
     my $records = staged_records($info, $state);
@@ -3136,16 +3142,17 @@ my sub print_plan($info, $plan, $state, $opts, $storage_entities) {
                 . " with the same checks.");
             log_step("A standby manager cannot take a key while running and is restarted.")
                 if grep { $_->{type} eq 'mgr' } $plan->{daemons}->@*;
-            my @down = map { $_->{entity} } grep { $_->{down} } $plan->{daemons}->@*;
-            if (@down) {
-                log_text("  Not running right now, so the key is written to disk and the daemon"
-                    . " left stopped: "
-                    . join(', ', @down)
-                    . ".");
-            }
             log_step("'noout' is set on this run's OSDs, as that stop can happen at any point.")
                 if grep { $_->{type} eq 'osd' } $plan->{daemons}->@*;
         }
+        my @down = grep { $_->{down} } $plan->{daemons}->@*;
+        if (@down) {
+            log_text("  Not running right now, so the key is written to disk and the daemon"
+                . " left stopped: "
+                . join(', ', map { $_->{entity} } @down)
+                . ". Start one that should run afterwards, on its node:");
+            log_steps([map { start_command($_->{type}, $_->{id}) . " (node '$_->{node}')" } @down]);
+        }
         if ($opts->{verbose}) {
             log_step("in this order:");
             log_steps([map { "$_->{entity} on $_->{node}" } $plan->{daemons}->@*]);
@@ -3383,9 +3390,9 @@ my sub health_gate($rados, $type, $what) {
 my sub rotate_entity($rados, $state, $entity) {
     my $before = auth_entry($rados, $entity);
 
-    # trust the marker only as far as the key backs it: one changed since would read as migrated
-    if ($state->{rotated}->{$entity} && (key_cipher($before->{key}) // -1) == $CIPHER_ID) {
-        log_info("the key of '$entity' was already rotated by an earlier run, reusing it");
+    # the key may come from an earlier run or from someone else, so judge it, not the marker
+    if ((key_cipher($before->{key}) // -1) == $CIPHER_ID) {
+        log_info("the key of '$entity' already uses the '$CIPHER' cipher, keeping it");
         return $before;
     }
     if ($state->{rotated}->{$entity}) {
@@ -3393,10 +3400,6 @@ my sub rotate_entity($rados, $state, $entity) {
             . ($CIPHER_NAMES->{ key_cipher($before->{key}) // -1 } // 'unreadable')
             . "' cipher now, so it is rotated again");
     }
-    if ((key_cipher($before->{key}) // -1) == $CIPHER_ID) {
-        log_info("the key of '$entity' already uses the '$CIPHER' cipher, leaving it alone");
-        return $before;
-    }
 
     # markers of an older rotation must not hide this one on resume
     delete $state->{done}->{$entity};
@@ -4948,7 +4951,9 @@ my sub migrate_daemon($rados, $state, $daemon, $opts) {
     # left as found: whoever stopped it did not ask for it back
     if ($daemon->{down} && !$up) {
         log_pass("'$entity' uses the '$CIPHER' cipher and stays stopped, as it was before this"
-            . " run");
+            . " run. If it should run, start it on node '$node' with '"
+            . start_command($type, $id)
+            . "'.");
         $state->{done}->{$entity} = time();
         delete $state->{live_swap}->{$entity};
         save_state($state);
@@ -5987,6 +5992,8 @@ my sub collect_run_info($rados, $opts, $state, $previous = undef) {
     # read on its own, as the monitor map names an unsupported value 'unknown'
     $info->{preferred_cipher} = current_preferred_cipher($rados);
 
+    # what the daemons hold on disk does not change with the auth database
+    $info->{stale_keys} = $previous->{stale_keys} if $previous;
     for my $type (keys %{ ($previous // {})->{daemons} // {} }) {
         my $listed = { map { $_->{entity} => 1 } @{ $info->{daemons}->{$type} // [] } };
         push $info->{daemons}->{$type}->@*,
diff --git a/test/CephKeyMigrationScript_test.pl b/test/CephKeyMigrationScript_test.pl
index 1d473dd6..aadeb702 100755
--- a/test/CephKeyMigrationScript_test.pl
+++ b/test/CephKeyMigrationScript_test.pl
@@ -4448,6 +4448,27 @@ sub run_aggregate_confirmation {
         kernel => 0,
     };
 
+    # a daemon Ceph does not list is left stopped, and the plan says how to start it
+    {
+        my $plan = {
+            mon_key => 0,
+            daemons =>
+                [{ type => 'mds', id => 'a', entity => 'mds.a', node => 'pve1', down => 1 }],
+            client_keys => [],
+            lockbox_keys => [],
+            service_cipher => 0,
+        };
+        my $output = '';
+        open(my $stdout, '>', \$output) or die $!;
+        local *STDOUT = $stdout;
+        $HOOKS->{print_plan}->($info, $plan, {}, {}, $stores);
+        like(
+            $output,
+            qr/left stopped: mds\.a\. Start one that should run afterwards, on its node:\n  systemctl reset-failed ceph-mds\@a; systemctl start ceph-mds\@a \(node 'pve1'\)$/m,
+            'a daemon left stopped comes with the command to start it',
+        );
+    }
+
     my $mixed = $render->(
         [$tool, $resume], { 'rotate-all-storage-keys' => 1, 'rotate-client-keys' => 1 },
     );
@@ -8614,20 +8635,36 @@ for my $case ([0, 0], [1, 0], [0, 1], [1, 1]) {
     is(scalar(@probes), 1, 'after inspecting the keyring of the metadata server once');
 
     ($result, $out) = $run->($OLD, {});
-    is($result, 0, 'a daemon holding another key than the monitors does not stop the run');
+    is($result, 0, 'a daemon holding another key than the monitors is planned');
     like(
         $out,
-        qr/^WARN: These keep a key on disk that the monitors do not accept, as after a 'ceph auth rotate', and would fail to authenticate once they read it: mds\.a \(keyring\)\.$/m,
-        'but is reported, naming the copy that differs',
-    );
-    like(
-        $out,
-        qr/^WARN: Every service key uses 'aes256k', and so do the service tickets\. But the key on disk could not be confirmed for mds\.a/m,
-        'and the verdict names it',
+        qr/^WARN: These keep a key on disk that the monitors do not accept, as after a 'ceph auth rotate', and would fail to authenticate once they read it: mds\.a \(keyring\)\.\n(?s:.*)^Step 1: rotate 1 metadata server key/m,
+        'with the warning saying why, and which copy differs',
     );
     unlike($out, qr/^PASS: Every service key/m, 'without reporting the cluster as migrated');
     is_deeply([@written, @told, @units, @auth], [], 'and a dry run changes nothing');
 
+    ($result, $out) = $run->($OLD, { apply => 1 });
+    is($result, 0, 'the apply run fixes the daemon');
+    is_deeply(
+        \@auth,
+        ['get-or-create-pending mds.a', 'commit-pending mds.a'],
+        'with the live swap of any rotation',
+    );
+    is_deeply(\@written, [$fresh], 'which writes the new key to its keyring');
+    is_deeply(\@told, ['tell mds.a rotate-key -i -'], 'and hands it to the running daemon');
+    is_deeply(\@units, [], 'without a restart');
+
+    ($result, $out) = $run->($OLD, { apply => 1, 'restart-daemons' => 1 });
+    is($result, 0, "'--restart-daemons' fixes it too");
+    is_deeply(\@auth, [], 'without rotating the key, which already uses the new cipher');
+    is_deeply(\@written, [$NEW], 'by writing the key the monitors accept');
+    is_deeply(
+        \@units,
+        ['stop ceph-mds@a', 'reset-failed ceph-mds@a', 'start ceph-mds@a'],
+        'and restarting the daemon',
+    );
+
     ($result, $out) = $run->($NEW, {}, 1);
     is($result, 0, 'a cluster whose daemon could not be checked still reads as migrated');
     like(
diff --git a/test/CephKeyMigration_test.pl b/test/CephKeyMigration_test.pl
index dfa3ec91..6cbf51af 100755
--- a/test/CephKeyMigration_test.pl
+++ b/test/CephKeyMigration_test.pl
@@ -512,6 +512,9 @@ isnt(key_fingerprint($NEW), key_fingerprint($OLD), 'different keys get different
         0,
         'an entity with no auth entry is not this step\'s business',
     );
+    # a key changed by hand reads as migrated in the auth database
+    $info->{stale_keys} = { 'osd.2' => ['label'] };
+    is(needs_rotation($info, 'osd.2'), 1, 'unless a copy on disk holds another key');
 }
 {
     # 'ceph-mon --mkfs' moves 'mon.' out of the auth database and 'ceph-authtool --gen-key' has
@@ -1652,6 +1655,19 @@ my sub cluster {
         'a scope can name a single daemon, which is how one left behind is repaired',
     );
 
+    # a key changed in the auth database alone reads as migrated
+    my $stale = cluster();
+    $stale->{exported}->{'osd.1'} = { key => $NEW };
+    my $state = { done => { 'mgr.a' => 1, 'osd.1' => 1 } };
+    is_deeply(build_plan($stale, $state, {}, {})->{daemons}, [],
+        'a migrated cluster plans nothing');
+    $stale->{stale_keys} = { 'mgr.a' => ['keyring'] };
+    is_deeply(
+        [map { $_->{entity} } build_plan($stale, $state, {}, {})->{daemons}->@*],
+        ['mgr.a'],
+        'unless a daemon keeps a key on disk that the monitors do not accept',
+    );
+
     my $switched = cluster();
     $switched->{service_cipher} = $CIPHER;
     is(
-- 
2.47.3