From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 5B38C1FF0B2 for ; Sun, 25 Oct 2026 10:57:59 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 146D821610; Sun, 25 Oct 2026 10:57:44 +0100 (CET) From: Kefu Chai To: pve-devel@lists.proxmox.com Subject: [PATCH manager v2 3/3] migrations: cephx: rotate daemons whose key on disk differs Date: Sun, 25 Oct 2026 17:57:30 +0800 Message-ID: <20261025095730.4027539-4-k.chai@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261025095730.4027539-1-k.chai@proxmox.com> References: <20261025095730.4027539-1-k.chai@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792922258872 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.727 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_MAILER 2 Automated Mailer Tag Left in Email RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: CXVCTZ2EKP4GCUVMWKKGTHDHZOD466CD X-Message-ID-Hash: CXVCTZ2EKP4GCUVMWKKGTHDHZOD466CD X-MailFrom: k.chai@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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