cockroach-admin: Allow partial success parsing cockroach node status --all rows (#10072)

The `cockroach-admin` server runs `cockroach node status --all` both to
find its local node ID and to service the `/node/status` endpoint. We
have no production callers of the latter, but inventory relies on the
former. We've previously seen that we can't get reasonable status from
decommissioned nodes (in particular, we rely on socket addresses being
present, and decommissioned nodes have `NULL` socket addresses) and so
already skip rows from decommissioned nodes. However, in #10068 we saw
similar invalid rows from not-yet-decommissioned nodes, which broke
inventory collection.

This change relaxes our parsing: instead of bailing on any failure
parsing a single row, we accumulate separate vecs of successfully parsed
rows and failures parsing rows and return both. `cockroach-admin` logs
the errors but otherwise ignores them. I don't love this but I _think_
it should be okay; a failure mode here is "we fail to parse any rows at
all", but in practice the effect of having an empty vec of
successfully-parsed rows is the same as failing the call entirely: we
won't be able to find our local node ID or report any useful node
status.

I think we'd be better off jettisoning this parsing entirely, if there's
a way we could get at all this information from another source (either
the prometheus endpoint we're already scraping, or maaaaaybe poking at
files on disk, e.g., if there's a file that contains the local node
ID?). But either of those would be larger changes, and this should
unblock testing that bumped into the parsing issue.

Closes #10068.

Author: jgallagher

Cargo.lock

@@ -7,6 +7,8 @@ use cockroach_admin_types::node::{NodeDecommission, NodeStatus, ParseError};
 use dropshot::HttpError;
 use illumos_utils::ExecutionError;
 use illumos_utils::output_to_exec_error;
+use slog::Logger;
+use slog::warn;
 use slog_error_chain::InlineErrorChain;
 use slog_error_chain::SlogInlineError;
 use std::io;
@@ -159,20 +161,40 @@ impl CockroachCli {
 
     pub async fn node_status(
         &self,
+        log: &Logger,
     ) -> Result<Vec<NodeStatus>, CockroachCliError> {
-        self.invoke_cli_with_format_csv(
-            ["node", "status", "--all"],
-            NodeStatus::parse_from_csv,
-            "node status",
-        )
-        .await
+        let (statuses, errs) = self
+            .invoke_cli_with_format_csv(
+                ["node", "status", "--all"],
+                NodeStatus::parse_from_csv,
+                "node status",
+            )
+            .await?;
+        if !errs.is_empty() {
+            warn!(
+                log,
+                "partial failure parsing `node status --all` output: \
+                 successfully parsed {} of {} rows",
+                statuses.len(),
+                statuses.len() + errs.len(),
+            );
+            for err in errs {
+                warn!(
+                    log,
+                    "skipped row from `node status --all` due to parse error";
+                    InlineErrorChain::new(&err),
+                );
+            }
+        }
+        Ok(statuses)
     }
 
     pub async fn node_decommission(
         &self,
         node_id: &str,
+        log: &Logger,
     ) -> Result<NodeDecommission, CockroachCliError> {
-        let statuses = self.node_status().await?;
+        let statuses = self.node_status(log).await?;
         self.validate_node_decommissionable(node_id, statuses)?;
         self.invoke_node_decommission(node_id).await
     }
@@ -423,7 +445,8 @@ mod tests {
     #[tokio::test]
     async fn test_node_status_compatibility() {
         let logctx = dev::test_setup_log("test_node_status_compatibility");
-        let db = TestDatabase::new_populate_nothing(&logctx.log).await;
+        let log = &logctx.log;
+        let db = TestDatabase::new_populate_nothing(log).await;
         let db_url = db.crdb().listen_url().to_string();
 
         let expected_headers = "id,address,sql_address,build,started_at,updated_at,locality,is_available,is_live,replicas_leaders,replicas_leaseholders,ranges,ranges_unavailable,ranges_underreplicated,live_bytes,key_bytes,value_bytes,intent_bytes,system_bytes,gossiped_replicas,is_decommissioning,membership,is_draining";
@@ -463,7 +486,7 @@ mod tests {
             cockroach_address,
             SocketAddr::V6(cockroach_address),
         );
-        let status = cli.node_status().await.expect("got node status");
+        let status = cli.node_status(log).await.expect("got node status");
 
         // We can't check all the fields exactly, but some we know based on the
         // fact that our test database is a single node.

cockroach-admin/src/cockroach_cli.rs

@@ -113,7 +113,7 @@ impl ServerContext {
         // crdb instance.
         let node_statuses = self
             .cockroach_cli()
-            .node_status()
+            .node_status(&self.log)
             .await
             .context("failed to get node status")?;
 

cockroach-admin/src/context.rs

@@ -48,8 +48,11 @@ impl CockroachAdminApi for CockroachAdminImpl {
         rqctx: RequestContext<Self::Context>,
     ) -> Result<HttpResponseOk<ClusterNodeStatus>, HttpError> {
         let ctx = rqctx.context();
-        let all_nodes =
-            ctx.cockroach_cli().node_status().await.map_err(HttpError::from)?;
+        let all_nodes = ctx
+            .cockroach_cli()
+            .node_status(ctx.log())
+            .await
+            .map_err(HttpError::from)?;
         Ok(HttpResponseOk(ClusterNodeStatus { all_nodes }))
     }
 

cockroach-admin/src/http_entrypoints.rs

@@ -18,4 +18,5 @@ thiserror.workspace = true
 
 [dev-dependencies]
 proptest.workspace = true
+slog-error-chain.workspace = true
 test-strategy.workspace = true