[PATCH manager 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.

Plan such a daemon like one still on the old cipher, so migrating it
writes a key the monitors accept to every copy. A stopped daemon stays
stopped as before, and as it has often stopped because of that very
key, the plan and the run now name the command that starts it, with or
without '--restart-daemons'.

A daemon whose key an earlier run rotated can now reach the branch that
reuses that key after the key was changed by hand, so its message no
longer claims that the earlier run produced the current key.

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

diff --git a/PVE/Ceph/KeyMigration.pm b/PVE/Ceph/KeyMigration.pm
index 1abf1eee..ced9da94 100644
--- a/PVE/Ceph/KeyMigration.pm
+++ b/PVE/Ceph/KeyMigration.pm
@@ -1810,6 +1810,11 @@ sub build_plan($info, $state, $opts, $files) {
 
         for my $daemon (@$daemons) {
             next if !daemon_in_scope($only, $type, $daemon->{entity});
+            # a key changed by hand reads as migrated in the auth database
+            if (($info->{stale_keys} // {})->{ $daemon->{entity} }) {
+                push $plan->{daemons}->@*, $daemon;
+                next;
+            }
             my $unfinished = migration_unfinished($state, $daemon->{entity});
             # Not the marker alone: a reused OSD id gets a fresh key that needs migrating like any
             # other.
diff --git a/bin/pve-cephx-rotate-service-keys b/bin/pve-cephx-rotate-service-keys
index 647251cf..d9078819 100755
--- a/bin/pve-cephx-rotate-service-keys
+++ b/bin/pve-cephx-rotate-service-keys
@@ -1703,7 +1703,6 @@ my sub check_disk_keys($info, $state, $opts) {
         my $verdict = disk_key_verdict($info, $daemon, $probe);
         if ($verdict->{verdict} eq 'stale') {
             $info->{stale_keys}->{ $daemon->{entity} } = $verdict->{copies};
-            push @$disk_key_doubts, $daemon->{entity};
         } elsif ($verdict->{verdict} eq 'unknown') {
             chomp(my $text = describe_disk_key_doubt({ %$daemon, %$probe }, $verdict->{why}));
             push @doubts, $text;
@@ -2498,6 +2497,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);
@@ -3025,16 +3025,18 @@ 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 { "systemctl start ceph-$_->{type}\@$_->{id} (node '$_->{node}')" } @down]);
+        }
         if ($opts->{verbose}) {
             log_step("in this order:");
             log_steps([map { "$_->{entity} on $_->{node}" } $plan->{daemons}->@*]);
@@ -3273,7 +3275,8 @@ my sub rotate_entity($rados, $state, $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");
+        log_info("an earlier run rotated '$entity' and its key uses the '$CIPHER' cipher now,"
+            . " reusing that key");
         return $before;
     }
     if ($state->{rotated}->{$entity}) {
@@ -4778,7 +4781,7 @@ 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 'systemctl start $unit'.");
         $state->{done}->{$entity} = time();
         delete $state->{live_swap}->{$entity};
         save_state($state);
@@ -5817,6 +5820,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 2cfc75f0..adb9b77b 100755
--- a/test/CephKeyMigrationScript_test.pl
+++ b/test/CephKeyMigrationScript_test.pl
@@ -4399,6 +4399,29 @@ 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,
+        };
+        for my $restart (0, 1) {
+            my $output = '';
+            open(my $stdout, '>', \$output) or die $!;
+            local *STDOUT = $stdout;
+            $HOOKS->{print_plan}->($info, $plan, {}, { 'restart-daemons' => $restart }, $stores);
+            like(
+                $output,
+                qr/left stopped: mds\.a\. Start one that should run afterwards, on its node:\n  systemctl start ceph-mds\@a \(node 'pve1'\)$/m,
+                "a daemon left stopped comes with the command to start it (restart: $restart)",
+            );
+        }
+    }
+
     my $mixed = $render->(
         [$tool, $resume], { 'rotate-all-storage-keys' => 1, 'rotate-client-keys' => 1 },
     );
@@ -8156,7 +8179,13 @@ for my $case ([0, 0], [1, 0], [0, 1], [1, 1]) {
 }
 
 # the real orchestration runs on a migrated-looking cluster whose daemon kept an old key on disk
-for my $case (['dry run', $OLD], ['dry run', $NEW], ['unreachable', $NEW]) {
+for my $case (
+    ['dry run', $OLD],
+    ['dry run', $NEW],
+    ['apply', $OLD],
+    ['apply restart', $OLD],
+    ['unreachable', $NEW],
+) {
     my ($mode, $on_disk) = @$case;
     no warnings qw(once redefine);
     my $dir = tempdir(CLEANUP => 1);
@@ -8280,7 +8309,12 @@ for my $case (['dry run', $OLD], ['dry run', $NEW], ['unreachable', $NEW]) {
         $error = $@;
     }
     is($error, '', "the $mode itself does not die");
-    is(scalar(@probes), 1, 'the keyring of the metadata server is inspected once');
+    # once for the check, and again for the node preflight of a plan
+    is(
+        scalar(@probes),
+        $case->[1] eq $OLD ? 2 : 1,
+        'the keyring of the metadata server is inspected for each',
+    );
     if ($mode eq 'unreachable') {
         is($result, 0, 'a cluster whose daemon could not be checked still reads as migrated');
         like(
@@ -8290,22 +8324,36 @@ for my $case (['dry run', $OLD], ['dry run', $NEW], ['unreachable', $NEW]) {
         );
         unlike($out, qr/^PASS:/m, 'and passes nothing');
     } elsif ($mode eq 'dry run' && $on_disk eq $OLD) {
-        is($result, 0, 'a daemon holding another key than the monitors does not stop the run');
-        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',
-        );
+        is($result, 0, 'a daemon holding another key than the monitors is planned');
         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');
     } elsif ($mode eq 'dry run') {
         is($result, 0, 'with the key on disk matching, the migrated cluster passes');
         like($out, qr/^PASS: Every service key uses 'aes256k'/m, 'as before');
+    } elsif ($mode eq 'apply') {
+        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');
+    } else {
+        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',
+        );
     }
 }
 
diff --git a/test/CephKeyMigration_test.pl b/test/CephKeyMigration_test.pl
index c0d9384a..a9ba6d71 100755
--- a/test/CephKeyMigration_test.pl
+++ b/test/CephKeyMigration_test.pl
@@ -1666,6 +1666,27 @@ 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',
+    );
+    is_deeply(
+        [
+            map { $_->{entity} }
+                build_plan($stale, $state, { only => { osd => 1 } }, {})->{daemons}->@*
+        ],
+        [],
+        "but not outside the '--only' scope",
+    );
+
     my $switched = cluster();
     $switched->{service_cipher} = $CIPHER;
     is(
-- 
2.47.3