[PATCH manager v2 1/4] migrations: cephx: ask a monitor about itself through one route

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

The helper queries each monitor over its admin socket, which needs a
shell on the monitor's host. For a monitor on a host outside this
cluster, such as a stretch cluster's tiebreaker, these queries fail. The
helper then treats the monitor as unreachable, refuses to stage client
keys, and reports the session list as incomplete.

Record whether a daemon runs on one of our nodes. For a monitor that
does not, use 'ceph tell' as 'mon.' instead, with a short timeout.

'ceph tell' shows up in the monitor's own session list as a client named
'mon.'. Ignore 'mon.' sessions, since 'mon.' is the monitors' own key
and not a client key to refresh. Otherwise, if 'mon.' is not in the auth
database, the run would stop on a client without an auth entry.

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

diff --git a/PVE/Ceph/KeyMigration.pm b/PVE/Ceph/KeyMigration.pm
index d0613e8b..c24c1c3b 100644
--- a/PVE/Ceph/KeyMigration.pm
+++ b/PVE/Ceph/KeyMigration.pm
@@ -561,6 +561,8 @@ sub summarize_sessions($per_mon, $expected_mons = undef) {
                 next;
             }
             next if $session->{con_type} ne 'client';
+            # skip the monitors' own key, which a 'ceph tell' sent as 'mon.' also shows up as
+            next if ($session->{entity_name} // '') eq 'mon.';
 
             my $gid = $session->{global_id};
             my $entity = $session->{entity_name};
@@ -1101,15 +1103,16 @@ sub merge_configured_daemons($daemons, $type, $configured, $existing = undef) {
             push @$ghosts, { type => $type, id => "$id", node => $configured->{$id} };
             next;
         }
-        push @$daemons,
-            {
-                type => $type,
-                id => "$id",
-                entity => $type eq 'mon' ? 'mon.' : "$type.$id",
-                node => $configured->{$id},
-                down => 1,
-                $type eq 'osd' && $existing ? ('osd-uuid' => $existing->{$id}) : (),
-            };
+        push @$daemons, {
+            type => $type,
+            id => "$id",
+            entity => $type eq 'mon' ? 'mon.' : "$type.$id",
+            node => $configured->{$id},
+            # pvestatd only reports daemons on nodes of this cluster
+            managed => 1,
+            down => 1,
+            $type eq 'osd' && $existing ? ('osd-uuid' => $existing->{$id}) : (),
+        };
     }
 
     return wantarray ? ($daemons, $ghosts) : $daemons;
diff --git a/bin/pve-cephx-rotate-service-keys b/bin/pve-cephx-rotate-service-keys
index ad425f96..84daff6d 100755
--- a/bin/pve-cephx-rotate-service-keys
+++ b/bin/pve-cephx-rotate-service-keys
@@ -175,11 +175,18 @@ my sub cluster_nodes() {
     return { map { $_ => 1 } PVE::Cluster::get_nodelist()->@* };
 }
 
+# 'managed': the host is a node of this cluster, so we can reach its files, units and packages.
+# Other daemons, like the tiebreaker monitor of a stretch cluster, are reachable only via Ceph.
 my sub daemon_location($type, $id, $host) {
-    return { node => undef } if !defined($host) || (!ref($host) && !length($host));
+    return { node => undef, managed => 0 } if !defined($host) || (!ref($host) && !length($host));
     die "Cannot locate '$type.$id': no valid host name was reported\n" if ref($host);
-    my $node = PVE::Ceph::Services::metadata_host_node($host, cluster_nodes());
-    return { node => $node, $node ne $host ? ('metadata-host' => $host) : () };
+    my $nodes = cluster_nodes();
+    my $node = PVE::Ceph::Services::metadata_host_node($host, $nodes);
+    return {
+        node => $node,
+        managed => $nodes->{$node} ? 1 : 0,
+        $node ne $host ? ('metadata-host' => $host) : (),
+    };
 }
 
 my sub assert_member($node, $entity) {
@@ -559,13 +566,14 @@ my sub auth_entry($rados, $entity) {
 }
 
 # the monitor identity keeps working while a client.admin rotation invalidates the default keyring
-my sub monitor_command($args) {
+my sub monitor_command($args, $timeout = undef) {
     return node_run(
         $nodename,
         [
             'ceph', '--cluster', $ccname, '--name', 'mon.', '--keyring', $pve_mon_keyring,
             @$args,
         ],
+        defined($timeout) ? (timeout => $timeout) : (),
     );
 }
 
@@ -579,19 +587,26 @@ my sub monitor_auth_entry($entity) {
     return $res->[0];
 }
 
+# Ask a monitor about itself over its admin socket, which works even when cephx does not. For a
+# monitor outside this cluster, use 'ceph tell' to reach the same socket. That needs cephx and a
+# quorum. It runs as 'mon.', so it still works while 'client.admin' is rotated.
+my sub mon_admin_run($node, $id, $words, $run_node = undef) {
+    if (defined($node) && cluster_nodes()->{$node}) {
+        my $cmd = ['ceph', 'daemon', "mon.$id", @$words];
+        return $run_node ? $run_node->($node, $cmd) : node_run($node, $cmd, entity => "mon.$id");
+    }
+
+    # do not let an unreachable monitor hold the run for the 300 second default
+    return monitor_command(['--connect-timeout', '10', 'tell', "mon.$id", @$words], 20);
+}
+
 my sub poll_monitor_authentication($monitors, $nodes, $run_node = undef) {
-    $run_node //= sub($node, $command) { node_run($node, $command, entity => $command->[2]) };
     my ($reports, $errors) = ({}, {});
     for my $mon (@$monitors) {
         my $option = 'auth_service_required';
         my $reply = eval {
-            assert_member($nodes->{$mon}, "mon.$mon");
-            decode_json(
-                $run_node->(
-                    $nodes->{$mon},
-                    ['ceph', 'daemon', "mon.$mon", 'config', 'get', $option],
-                ),
-            );
+            decode_json(mon_admin_run(
+                $nodes->{$mon}, $mon, ['config', 'get', $option], $run_node));
         };
         my $error = $@;
         $reports->{$mon}->{$option} = ref($reply) eq 'HASH' ? $reply->{$option} : undef;
@@ -789,20 +804,12 @@ for my $fsid (@fsids) {
 }
 PERL
 
-# Every monitor is asked over its admin socket, which needs no cephx and answers even while the
-# authentication layer itself is in trouble. One that does not answer marks the result incomplete,
-# so the checks built on it stay honest.
+# A monitor that does not answer marks the result incomplete.
 my sub poll_client_sessions($info) {
     my $per_mon = [];
     for my $mon ($info->{daemons}->{mon}->@*) {
         next if $mon->{down};
-        my $sessions = eval {
-            decode_json(node_run(
-                $mon->{node},
-                ['ceph', 'daemon', "mon.$mon->{id}", 'sessions'],
-                entity => "mon.$mon->{id}",
-            ));
-        };
+        my $sessions = eval { decode_json(mon_admin_run($mon->{node}, $mon->{id}, ['sessions'])) };
         push @$per_mon, { mon => $mon->{id}, sessions => $sessions };
     }
     return summarize_sessions($per_mon, $info->{monmap_mons});
@@ -811,15 +818,14 @@ my sub poll_client_sessions($info) {
 # Whether a monitor can keep two client keys valid is told by its admin socket, which needs no
 # cephx. One that answers but does not report the option is old; one that does not answer at all
 # cannot be told apart from a broken connection, and rules staging out just the same.
-my sub probe_manual_promotion($node, $id, $run_node) {
-    my $out =
-        eval { $run_node->($node, ['ceph', 'daemon', "mon.$id", 'config', 'get', $GRACE_OPTION]) };
+my sub probe_manual_promotion($node, $id, $run_node = undef) {
+    my $out = eval { mon_admin_run($node, $id, ['config', 'get', $GRACE_OPTION], $run_node) };
     if (!$@) {
         my $res = eval { decode_json($out) };
         my $value = ref($res) eq 'HASH' ? $res->{$GRACE_OPTION} : undef;
         return { reached => 1, value => defined($value) && !ref($value) ? "$value" : undef };
     }
-    my $reached = eval { $run_node->($node, ['ceph', 'daemon', "mon.$id", 'version']); 1 } ? 1 : 0;
+    my $reached = eval { mon_admin_run($node, $id, ['version'], $run_node); 1 } ? 1 : 0;
     return { reached => $reached, value => undef };
 }
 
@@ -827,11 +833,7 @@ my sub poll_manual_promotion($info) {
     my $reports = {};
     for my $mon ($info->{daemons}->{mon}->@*) {
         next if $mon->{down};
-        $reports->{ $mon->{id} } = probe_manual_promotion(
-            $mon->{node},
-            $mon->{id},
-            sub($node, $cmd) { node_run($node, $cmd, entity => "mon.$mon->{id}") },
-        );
+        $reports->{ $mon->{id} } = probe_manual_promotion($mon->{node}, $mon->{id});
     }
     return manual_promotion_support($reports, $info->{monmap_mons});
 }
@@ -882,8 +884,6 @@ my sub session_snapshot_with_retries($collect, $attempts = 10) {
 # metadata, missing quorum membership, and a failed admin-socket query therefore make the summary
 # incomplete rather than making that monitor disappear from it.
 my sub collect_current_monitor_state($rados, $run_node = undef) {
-    $run_node //= sub($node, $command) { node_run($node, $command) };
-
     my $mon_dump = $rados->mon_command({ prefix => 'mon dump', format => 'json' });
     die "could not refresh the monitor map\n"
         if ref($mon_dump) ne 'HASH'
@@ -936,13 +936,9 @@ my sub collect_current_monitor_state($rados, $run_node = undef) {
             $errors->{$id} = 'not in quorum';
         } elsif (!$nodes->{$id}) {
             $errors->{$id} = 'no hostname in monitor metadata';
-        } elsif (!eval { assert_member($nodes->{$id}, "mon.$id"); 1 }) {
-            $errors->{$id} = $@;
         } else {
-            $sessions = eval {
-                decode_json($run_node->(
-                    $nodes->{$id}, ['ceph', 'daemon', "mon.$id", 'sessions']));
-            };
+            $sessions =
+                eval { decode_json(mon_admin_run($nodes->{$id}, $id, ['sessions'], $run_node)) };
             my $error = $@;
             if ($error) {
                 $error =~ s/\s+/ /g;
@@ -6258,6 +6254,7 @@ sub key_migration_test_hooks {
         client_key_files => \&client_key_files,
         check_client_kernels => \&check_client_kernels,
         manual_promotion_with_retries => \&manual_promotion_with_retries,
+        mon_admin_run => \&mon_admin_run,
         describe_live => \&describe_live,
     };
 }
diff --git a/test/CephKeyMigrationScript_test.pl b/test/CephKeyMigrationScript_test.pl
index ef13d8dd..fc9811d7 100755
--- a/test/CephKeyMigrationScript_test.pl
+++ b/test/CephKeyMigrationScript_test.pl
@@ -6600,7 +6600,16 @@ for my $case ([0, 0], [1, 0], [0, 1], [1, 1]) {
     $rados->{host} = 'foreign.invalid';
     @commands = ();
     $info = $HOOKS->{collect_cluster_info}->($rados, { 'rotate-lockbox-keys' => 1 }, {});
-    is_deeply(\@commands, [], 'non-member gets no commands during pre-preflight collection');
+    is(
+        scalar(grep { /foreign\.invalid/ } map { join(' ', @$_) } @commands),
+        0,
+        'pre-preflight collection sends nothing to the non-member host',
+    );
+    is(
+        scalar(grep { /tell mon\.a config get/ } map { join(' ', @$_) } @commands),
+        1,
+        'the monitor outside the cluster is asked with tell instead of its socket',
+    );
     like(
         $info->{lockbox}->{'client.osd-lockbox.osd-uuid'}->{missing},
         qr/osd\.7.*foreign\.invalid.*not a node/,
@@ -6609,10 +6618,14 @@ for my $case ([0, 0], [1, 0], [0, 1], [1, 1]) {
     @targets = ();
     $monitor = $HOOKS->{collect_monitor_state}->($rados, sub { push @targets, $_[0]; return '[]' });
     is_deeply(\@targets, [], 'fresh monitor collection never queries a non-member');
-    like(
+    is(
         $monitor->{sessions}->{errors}->{a},
-        qr/mon\.a.*not a node/,
-        'monitor diagnostic names membership',
+        undef,
+        'and its sessions come over the network, so nothing is left unanswered',
+    );
+    ok(
+        $monitor->{manual_promotion}->{supported},
+        'so a client key can still be staged with a monitor outside the cluster',
     );
 
     for my $version (1, 2) {
@@ -7842,6 +7855,46 @@ for my $case ([0, 0], [1, 0], [0, 1], [1, 1]) {
     );
 }
 
+# A monitor on one of our nodes is asked over its admin socket, any other one with 'ceph tell'.
+{
+    no warnings qw(once redefine);
+    local *PVE::SSHInfo::get_ssh_info = sub { return { node => $_[0] } };
+    local *PVE::SSHInfo::ssh_info_to_command = sub { return ['ssh', $_[0]->{node}, '--'] };
+
+    my @commands;
+    local *main::run_command = sub {
+        my ($cmd, %args) = @_;
+        push @commands, join(' ', @$cmd);
+        $args{outfunc}->('answer');
+    };
+
+    my $run = sub {
+        @commands = ();
+        return $HOOKS->{mon_admin_run}->(@_);
+    };
+
+    is($run->('node-a', 'a', ['sessions']), "answer\n", 'a member answers over its admin socket');
+    is($commands[0], 'ssh node-a -- ceph daemon mon.a sessions', 'which is asked through ssh');
+
+    is($run->('witness.invalid', 'tiebreaker', ['sessions']), "answer\n", 'so does an outsider');
+    like(
+        $commands[0],
+        qr/^ceph --cluster \S+ --name mon\. --keyring \S+ --connect-timeout 10 tell mon\.tiebreaker sessions$/,
+        'asked locally with tell, as mon., and with a connect timeout instead of a long default',
+    );
+
+    $run->(undef, 'nameless', ['version']);
+    like($commands[0], qr/tell mon\.nameless version/, 'a monitor without a known host too');
+
+    my @routed;
+    $run->('node-a', 'a', ['config', 'get', 'opt'], sub { push @routed, $_[1]; return '{}' });
+    is_deeply(
+        \@routed,
+        [['ceph', 'daemon', 'mon.a', 'config', 'get', 'opt']],
+        'a caller-supplied runner keeps the member route, which the tests rely on',
+    );
+}
+
 {
     my $out = '';
     {
diff --git a/test/CephKeyMigration_test.pl b/test/CephKeyMigration_test.pl
index 79fccedf..d22ac24b 100755
--- a/test/CephKeyMigration_test.pl
+++ b/test/CephKeyMigration_test.pl
@@ -771,6 +771,18 @@ is(
         sessions => [{ con_type => 'osd', entity_name => '', global_id => [] }],
     }]);
     ok($non_client->{complete}, 'a valid non-client session remains irrelevant');
+
+    my $own_tell = summarize_sessions([{
+        mon => 'a',
+        sessions => [{
+            con_type => 'client',
+            entity_name => 'mon.',
+            global_id => 140,
+            socket_addr => { addr => '10.0.0.3:0' },
+        }],
+    }]);
+    ok($own_tell->{complete}, "a 'mon.' session leaves the picture complete");
+    is_deeply($own_tell->{clients}, {}, "a 'mon.' session is no client of a key to refresh");
 }
 
 # --- the final step is offered once nothing else is open ---------------------------------------
-- 
2.47.3