Reconfigurator: Disable decommissioning cockroach nodes (#8446)

See #8445 for more
context.

This PR disables decommissioning from both sides:

* cockroach-admin will return a BAD_REQUEST for any decommissioning
attempts.
* The blueprint_executor RPW will not attempt to decommission expunged
cockroach zones.

Author: jgallagher

cockroach-admin/src/cockroach_cli.rs

@@ -179,6 +179,11 @@ impl CockroachCli {
         self.invoke_node_decommission(node_id).await
     }
 
+    // TODO-correctness These validation checks are NOT sufficient; see
+    // https://github.com/oxidecomputer/omicron/issues/8445. Our request handler
+    // for node decommissioning rejects requests, but we keep this method around
+    // for documentation and because we hope to extend it to have correct checks
+    // in the near future.
     fn validate_node_decommissionable(
         &self,
         node_id: &str,
@@ -191,7 +196,9 @@ impl CockroachCli {
         // 2. The node must not have any `gossiped_replicas` (the cockroach
         //    cluster must not think this node is an active member of any ranges
         //    - in practice, we see this go to 0 about 60 seconds after a node
-        //    goes offline)
+        //    goes offline). This attempts to guard against a window of time
+        //    where a node has gone offline but cockroach has not yet reflected
+        //    that fact in the counts of underreplicated ranges.
         // 3. The range descriptor for all ranges must not include the node we
         //    want to decommission. This gate shouldn't be necessary, but exists
         //    as a safeguard to avoid triggering what appears to be a CRDB race

cockroach-admin/src/http_entrypoints.rs

@@ -60,19 +60,16 @@ impl CockroachAdminApi for CockroachAdminImpl {
     }
 
     async fn node_decommission(
-        rqctx: RequestContext<Self::Context>,
-        body: TypedBody<NodeId>,
+        _rqctx: RequestContext<Self::Context>,
+        _body: TypedBody<NodeId>,
     ) -> Result<HttpResponseOk<NodeDecommission>, HttpError> {
-        let ctx = rqctx.context();
-        let NodeId { node_id } = body.into_inner();
-        let decommission_status =
-            ctx.cockroach_cli().node_decommission(&node_id).await?;
-        info!(
-            ctx.log(), "successfully decommissioned node";
-            "node_id" => node_id,
-            "status" => ?decommission_status,
-        );
-        Ok(HttpResponseOk(decommission_status))
+        // We should call ctx.cockroach_cli().node_decommission(), but can't at
+        // the moment due to timing concerns.
+        Err(HttpError::for_bad_request(
+            None,
+            "decommissioning cockroach nodes not supported (see omicron#8445)"
+                .to_string(),
+        ))
     }
 
     async fn status_vars(

nexus/reconfigurator/execution/src/omicron_zones.rs

@@ -37,10 +37,19 @@ pub(crate) async fn clean_up_expunged_zones<R: CleanupResolver>(
     resolver: &R,
     blueprint: &Blueprint,
 ) -> Result<(), Vec<anyhow::Error>> {
+    // TODO-correctness Decommissioning cockroach nodes is currently disabled
+    // while we work through issues with timing; see
+    // https://github.com/oxidecomputer/omicron/issues/8445. We keep the
+    // implementation around and gate it via an argument because we already have
+    // unit tests for it and because we'd like to restore it once
+    // cockroach-admin knows how to gate decommissioning correctly.
+    let decommission_cockroach = false;
+
     clean_up_expunged_zones_impl(
         opctx,
         datastore,
         resolver,
+        decommission_cockroach,
         blueprint
             .all_omicron_zones(BlueprintZoneDisposition::is_ready_for_cleanup),
     )
@@ -51,6 +60,7 @@ async fn clean_up_expunged_zones_impl<R: CleanupResolver>(
     opctx: &OpContext,
     datastore: &DataStore,
     resolver: &R,
+    decommission_cockroach: bool,
     zones_to_clean_up: impl Iterator<Item = (SledUuid, &BlueprintZoneConfig)>,
 ) -> Result<(), Vec<anyhow::Error>> {
     let errors: Vec<anyhow::Error> = stream::iter(zones_to_clean_up)
@@ -66,12 +76,18 @@ async fn clean_up_expunged_zones_impl<R: CleanupResolver>(
                 BlueprintZoneType::Nexus(_) => None,
 
                 // Zones which need cleanup after expungement.
-                BlueprintZoneType::CockroachDb(_) => Some(
-                    decommission_cockroachdb_node(
-                        opctx, datastore, resolver, config.id, &log,
-                    )
-                    .await,
-                ),
+                BlueprintZoneType::CockroachDb(_) => {
+                    if decommission_cockroach {
+                        Some(
+                            decommission_cockroachdb_node(
+                                opctx, datastore, resolver, config.id, &log,
+                            )
+                            .await,
+                        )
+                    } else {
+                        None
+                    }
+                }
                 BlueprintZoneType::Oximeter(_) => Some(
                     oximeter_cleanup(opctx, datastore, config.id, &log).await,
                 ),
@@ -303,6 +319,11 @@ mod test {
     async fn test_clean_up_cockroach_zones(
         cptestctx: &ControlPlaneTestContext,
     ) {
+        // The whole point of this test is to check that we send decommissioning
+        // requests; enable that. (See the real `clean_up_expunged_zones()` for
+        // more context.)
+        let decommission_cockroach = true;
+
         // Test setup boilerplate.
         let nexus = &cptestctx.server.server_context().nexus;
         let datastore = nexus.datastore();
@@ -356,6 +377,7 @@ mod test {
             &opctx,
             datastore,
             &fake_resolver,
+            decommission_cockroach,
             iter::once((any_sled_id, &crdb_zone)),
         )
         .await
@@ -402,6 +424,7 @@ mod test {
             &opctx,
             datastore,
             &fake_resolver,
+            decommission_cockroach,
             iter::once((any_sled_id, &crdb_zone)),
         )
         .await
@@ -433,6 +456,7 @@ mod test {
             &opctx,
             datastore,
             &fake_resolver,
+            decommission_cockroach,
             iter::once((any_sled_id, &crdb_zone)),
         )
         .await
@@ -461,6 +485,7 @@ mod test {
             &opctx,
             datastore,
             &fake_resolver,
+            decommission_cockroach,
             iter::once((any_sled_id, &crdb_zone)),
         )
         .await