nits from self-review

oxidecomputer · May 24, 2024 · 7df2d80 · 7df2d80
1 parent cbe475c
commit 7df2d80
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 15 deletions.
diff --git a/nexus/db-queries/src/db/datastore/cockroachdb_settings.rs b/nexus/db-queries/src/db/datastore/cockroachdb_settings.rs
@@ -114,13 +114,19 @@ impl DataStore {
             // below in `test_state_fingerprint`).
             .sql(" ELSE NULL END")
             .bind::<sql_types::Text, _>(state_fingerprint)
-            .bind::<sql_types::Text, _>(value)
+            .bind::<sql_types::Text, _>(value.clone())
             .query::<()>()
             .execute_async(&*conn)
             .await
             .map_err(|err| {
                 public_error_from_diesel(err, ErrorHandler::Server)
             })?;
+        info!(
+            opctx.log,
+            "set cockroachdb setting";
+            "setting" => setting,
+            "value" => &value,
+        );
         Ok(())
     }
 }

diff --git a/nexus/reconfigurator/execution/src/cockroachdb.rs b/nexus/reconfigurator/execution/src/cockroachdb.rs
@@ -8,7 +8,6 @@ use anyhow::Context;
 use nexus_db_queries::context::OpContext;
 use nexus_db_queries::db::DataStore;
 use nexus_types::deployment::Blueprint;
-use slog::info;
 
 pub(crate) async fn ensure_settings(
     opctx: &OpContext,
@@ -23,16 +22,10 @@ pub(crate) async fn ensure_settings(
                 opctx,
                 blueprint.cockroachdb_fingerprint.clone(),
                 "cluster.preserve_downgrade_option",
-                value.clone(),
+                value,
             )
             .await
             .context("failed to set cluster.preserve_downgrade_option")?;
-        info!(
-            opctx.log,
-            "set cockroachdb setting";
-            "setting" => "cluster.preserve_downgrade_option",
-            "value" => &value,
-        );
     }
     Ok(())
 }

diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs
@@ -535,8 +535,8 @@ impl<'a> Planner<'a> {
                 }
             }
             // The current version is unknown to us; we are likely in the middle
-            // of an upgrade and shouldn't change _any_ settings.
-            Err(_) => return,
+            // of an cluster upgrade.
+            Err(_) => CockroachDbPreserveDowngrade::DoNotModify,
         };
         self.blueprint.cockroachdb_preserve_downgrade(value);
         info!(
@@ -545,6 +545,14 @@ impl<'a> Planner<'a> {
             "setting" => "cluster.preserve_downgrade_option",
             "value" => ?value,
         );
+
+        // Hey! Listen!
+        //
+        // If we need to manage more CockroachDB settings, we should ensure
+        // that no settings will be modified if we don't recognize the current
+        // cluster version -- we're likely in the middle of an upgrade!
+        //
+        // https://www.cockroachlabs.com/docs/stable/cluster-settings#change-a-cluster-setting
     }
 }
 

diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs
@@ -54,6 +54,7 @@ use omicron_common::api::external::BgpPeer;
 use omicron_common::api::external::DataPageParams;
 use omicron_common::api::external::Error;
 use omicron_common::api::external::IdentityMetadataCreateParams;
+use omicron_common::api::external::InternalContext;
 use omicron_common::api::external::ListResultVec;
 use omicron_common::api::external::LookupResult;
 use omicron_common::api::external::Name;
@@ -232,16 +233,25 @@ impl super::Nexus {
         // Fill in the CockroachDB metadata for the initial blueprint, and set
         // the `cluster.preserve_downgrade_option` setting ahead of blueprint
         // execution.
-        let cockroachdb_settings =
-            self.datastore().cockroachdb_settings(opctx).await?;
+        let cockroachdb_settings = self
+            .datastore()
+            .cockroachdb_settings(opctx)
+            .await
+            .internal_context(
+                "fetching cockroachdb settings for rack initialization",
+            )?;
         self.datastore()
             .cockroachdb_setting_set_string(
                 opctx,
                 cockroachdb_settings.state_fingerprint.clone(),
                 "cluster.preserve_downgrade_option",
                 CockroachDbClusterVersion::NEWLY_INITIALIZED.to_string(),
             )
-            .await?;
+            .await
+            .internal_context(
+                "setting `cluster.preserve_downgrade_option` \
+                for rack initialization",
+            )?;
         blueprint.cockroachdb_fingerprint =
             cockroachdb_settings.state_fingerprint;
         blueprint.cockroachdb_setting_preserve_downgrade =

diff --git a/sled-agent/src/rack_setup/service.rs b/sled-agent/src/rack_setup/service.rs
@@ -1436,7 +1436,7 @@ pub(crate) fn build_initial_blueprint_from_sled_configs(
         // generation of 1. Nexus will bump this up when it updates external DNS
         // (including creating the recovery silo).
         external_dns_version: Generation::new(),
-        // Avoid making decisions about CockroachDB settings before Nexus is up.
+        // Nexus will fill in the CockroachDB values during initialization.
         cockroachdb_fingerprint: String::new(),
         cockroachdb_setting_preserve_downgrade:
             CockroachDbPreserveDowngrade::DoNotModify,