[reconfigurator] remove the add-zones-with-mupdate-override planner config (#10462)

While attempting to resolve
#10420, we realized that
the add-zones-with-mupdate-override config simply cannot result in a
blueprint with coherent semantics in some cases.

Concretely, consider the situation where:

* a target release has been set at least twice (so both the old and new
image sources are `Artifact`)
* a mupdate override is present on at least one sled
* there is a missing Nexus zone (which is a discretionary zone)

If this planner config is set, we would attempt to place a Nexus zone on
one of the sleds. What version should it have?

* Currently, this is either the old `Artifact` image source or the new
one.
* If we decide to place the zone on the sled with a mupdate override,
then that produces a blueprint that both blippy and Sled Agent (for that
sled) reject.
* But it would be very strange for the image source of the zone to be
_different_ based on the sled it is put on.

This planner config was not used in practice (documentation for our
support team has no mention of it). I've left the infrastructure to add
new planner configs in place, though -- happy to remove that as well if
y'all feel like that makes sense. (Previously daft would not handle
empty structs properly -- as part of this PR I've fixed that.)

This has no impact on racks that have never have a target release set --
for those racks, the image source is always `InstallDataset` so this is
moot (and pre-existing code handles the "target release generation is
one" case already).

Fixes #10420.

Author: sunshowers

Cargo.lock

@@ -484,7 +484,7 @@ crucible-common = { git = "https://github.com/oxidecomputer/crucible", rev = "71
 # NOTE: See above!
 csv = "1.3.1"
 curve25519-dalek = "4"
-daft = { version = "0.1.4", features = ["derive", "newtype-uuid1", "oxnet01", "uuid1"] }
+daft = { version = "0.1.7", features = ["derive", "newtype-uuid1", "oxnet01", "uuid1"] }
 display-error-chain = "0.2.2"
 omicron-ddm-admin-client = { path = "clients/ddm-admin-client" }
 datatest-stable = "0.3.2"

Cargo.toml

@@ -50,9 +50,6 @@ pub struct ReconfiguratorConfigOpts {
     #[clap(long, action = ArgAction::Set)]
     planner_enabled: Option<bool>,
 
-    #[clap(long, action = ArgAction::Set)]
-    add_zones_with_mupdate_override: Option<bool>,
-
     #[clap(long, action = ArgAction::Set)]
     tuf_repo_pruner_enabled: Option<bool>,
 }
@@ -65,13 +62,7 @@ impl ReconfiguratorConfigOpts {
             planner_enabled: self
                 .planner_enabled
                 .unwrap_or(current.planner_enabled),
-            planner_config: PlannerConfig {
-                add_zones_with_mupdate_override: self
-                    .add_zones_with_mupdate_override
-                    .unwrap_or(
-                        current.planner_config.add_zones_with_mupdate_override,
-                    ),
-            },
+            planner_config: PlannerConfig::default(),
             tuf_repo_pruner_enabled: self
                 .tuf_repo_pruner_enabled
                 .unwrap_or(current.tuf_repo_pruner_enabled),

dev-tools/omdb/src/bin/omdb/nexus/reconfigurator_config.rs

@@ -420,7 +420,6 @@ async fn cmd_reconfigurator_config_history(
     struct SwitchesRow {
         version: String,
         planner_enabled: String,
-        add_zones_with_mupdate_override: String,
         tuf_repo_pruner_enabled: String,
         time_modified: String,
     }
@@ -433,17 +432,14 @@ async fn cmd_reconfigurator_config_history(
                 config:
                     ReconfiguratorConfig {
                         planner_enabled,
-                        planner_config:
-                            PlannerConfig { add_zones_with_mupdate_override },
+                        planner_config: PlannerConfig {},
                         tuf_repo_pruner_enabled,
                     },
                 time_modified,
             } = s;
             SwitchesRow {
                 version: version.to_string(),
                 planner_enabled: planner_enabled.to_string(),
-                add_zones_with_mupdate_override:
-                    add_zones_with_mupdate_override.to_string(),
                 tuf_repo_pruner_enabled: tuf_repo_pruner_enabled.to_string(),
                 time_modified: time_modified.to_string(),
             }

dev-tools/omdb/src/bin/omdb/reconfigurator.rs

@@ -2164,8 +2164,6 @@ Reconfigurator config:
     modified time: <REDACTED_TIMESTAMP>
     tuf repo pruner enabled: true
     planner enabled: false
-    planner config:
-        add zones with mupdate override:   false
 ---------------------------------------------
 stderr:
 note: using Nexus URL http://127.0.0.1:REDACTED_PORT/
@@ -2245,21 +2243,6 @@ stdout:
 reconfigurator config updated to version 2:
     tuf repo pruner enabled:   true (unchanged)
 *   planner enabled:::::::::   false -> true
-    planner config:
-        add zones with mupdate override:   false (unchanged)
----------------------------------------------
-stderr:
-note: using Nexus URL http://127.0.0.1:REDACTED_PORT/
-=============================================
-EXECUTING COMMAND: omdb ["-w", "nexus", "reconfigurator-config", "set", "--add-zones-with-mupdate-override", "true"]
-termination: Exited(0)
----------------------------------------------
-stdout:
-reconfigurator config updated to version 3:
-    tuf repo pruner enabled:   true (unchanged)
-    planner enabled:::::::::   true (unchanged)
-    planner config:
-    *   add zones with mupdate override:   false -> true
 ---------------------------------------------
 stderr:
 note: using Nexus URL http://127.0.0.1:REDACTED_PORT/
@@ -2269,12 +2252,10 @@ termination: Exited(0)
 ---------------------------------------------
 stdout:
 Reconfigurator config:
-    version: 3
+    version: 2
     modified time: <REDACTED_TIMESTAMP>
     tuf repo pruner enabled: true
     planner enabled: true
-    planner config:
-        add zones with mupdate override:   true
 ---------------------------------------------
 stderr:
 note: using Nexus URL http://127.0.0.1:REDACTED_PORT/

dev-tools/omdb/tests/successes.out

@@ -290,14 +290,6 @@ async fn test_omdb_success_cases() {
             "--planner-enabled",
             "true",
         ],
-        &[
-            "-w",
-            "nexus",
-            "reconfigurator-config",
-            "set",
-            "--add-zones-with-mupdate-override",
-            "true",
-        ],
         &["nexus", "reconfigurator-config", "show", "current"],
         &["reconfigurator", "export", tmppath.as_str()],
         // We can't easily test the sled agent output because that's only

dev-tools/omdb/tests/test_all_output.rs

@@ -7,7 +7,7 @@
 use anyhow::{Context, anyhow, bail, ensure};
 use camino::Utf8PathBuf;
 use chrono::{DateTime, Utc};
-use clap::{ArgAction, ValueEnum};
+use clap::ValueEnum;
 use clap::{Args, Parser, Subcommand};
 use daft::Diffable;
 use gateway_types::rot::RotSlot;
@@ -34,6 +34,7 @@ use nexus_reconfigurator_simulation::{
 };
 use nexus_reconfigurator_simulation::{SimStateBuilder, SimTufRepoSource};
 use nexus_reconfigurator_simulation::{SimTufRepoDescription, Simulator};
+use nexus_types::deployment::BlueprintHostPhase2DesiredContents;
 use nexus_types::deployment::BlueprintMeasurements;
 use nexus_types::deployment::CockroachDbSettings;
 use nexus_types::deployment::ExpectedVersion;
@@ -42,9 +43,6 @@ use nexus_types::deployment::execution::blueprint_internal_dns_config;
 use nexus_types::deployment::{Blueprint, UnstableReconfiguratorState};
 use nexus_types::deployment::{BlueprintArtifactVersion, PendingMgsUpdate};
 use nexus_types::deployment::{BlueprintExpungedZoneAccessReason, execution};
-use nexus_types::deployment::{
-    BlueprintHostPhase2DesiredContents, PlannerConfig,
-};
 use nexus_types::deployment::{BlueprintSource, SledFilter};
 use nexus_types::deployment::{
     BlueprintZoneImageSource, PendingMgsUpdateDetails,
@@ -1554,8 +1552,6 @@ enum SetArgs {
         /// TUF repo containing release artifacts
         filename: Utf8PathBuf,
     },
-    /// planner config
-    PlannerConfig(SetPlannerConfigArgs),
     /// timestamp for ignoring impossible MGS updates
     IgnoreImpossibleMgsUpdatesSince {
         since: SetIgnoreImpossibleMgsUpdatesSinceArgs,
@@ -1581,35 +1577,6 @@ impl FromStr for SetIgnoreImpossibleMgsUpdatesSinceArgs {
     }
 }
 
-#[derive(Debug, Args)]
-struct SetPlannerConfigArgs {
-    #[clap(flatten)]
-    planner_config: PlannerConfigOpts,
-}
-
-// Define the config fields separately so we can use `group(required = true,
-// multiple = true).`
-#[derive(Debug, Clone, Args)]
-#[group(required = true, multiple = true)]
-pub struct PlannerConfigOpts {
-    #[clap(long, action = ArgAction::Set)]
-    add_zones_with_mupdate_override: Option<bool>,
-}
-
-impl PlannerConfigOpts {
-    fn update_if_modified(
-        &self,
-        current: &PlannerConfig,
-    ) -> Option<PlannerConfig> {
-        let new = PlannerConfig {
-            add_zones_with_mupdate_override: self
-                .add_zones_with_mupdate_override
-                .unwrap_or(current.add_zones_with_mupdate_override),
-        };
-        (new != *current).then_some(new)
-    }
-}
-
 #[derive(Debug, Args)]
 struct SetCockroachdbSettingsArgs {
     #[clap(flatten)]
@@ -3542,17 +3509,6 @@ fn cmd_set(
             );
             format!("set target release based on {}", filename)
         }
-        SetArgs::PlannerConfig(args) => {
-            let current = state.system_mut().description().get_planner_config();
-            if let Some(new) = args.planner_config.update_if_modified(&current)
-            {
-                state.system_mut().description_mut().set_planner_config(new);
-                let diff = current.diff(&new);
-                format!("planner config updated:\n{}", diff.display())
-            } else {
-                format!("no changes to planner config:\n{}", current.display())
-            }
-        }
         SetArgs::IgnoreImpossibleMgsUpdatesSince { since } => {
             state
                 .system_mut()

dev-tools/reconfigurator-cli/src/lib.rs

@@ -59,11 +59,6 @@ inventory-show latest all
 blueprint-plan latest
 blueprint-diff ade5749d-bdf3-4fab-a8ae-00bea01b3a5a latest
 
-# Set planner config.
-set planner-config --add-zones-with-mupdate-override false
-set planner-config --add-zones-with-mupdate-override true
-set planner-config --add-zones-with-mupdate-override true
-
 # Sled index out of bounds, will error out.
 wipe all
 load-example --seed test-basic --nsleds 3 --sled-policy 3:non-provisionable
@@ -72,15 +67,15 @@ log
 log --verbose
 
 # Switch states with a full UUID.
-switch 4d8d6725-1ae3-4f88-b9cb-a34db82d7439
+switch 4a3b153b-fc8a-485a-baee-badaad53d2fc
 log
 
 # Switch states with a unique prefix.
-switch 860f
+switch 0a3
 log
 
 # Switch states with an ambiguous prefix (should fail).
-switch 4
+switch 2
 
 # Switch states with a non-existent prefix (should fail).
 switch zzzz
@@ -90,11 +85,11 @@ sled-add
 sled-add
 
 # Make an additional branch based on the previous branch.
-switch 4d8d6725
+switch 860f
 sled-add
 
 # Go back to the previous branch.
-switch 5c53cf4b
+switch 4d8d6725
 
 log
 log --verbose

dev-tools/reconfigurator-cli/tests/input/cmds-add-zones-with-mupdate-override.txt

@@ -10,10 +10,6 @@ blueprint-diff 3f00b694-1b16-4aaa-8f78-e6b3a527b434 366b0b68-d80e-4bc1-abd3-dc69
 
 blueprint-show 366b0b68-d80e-4bc1-abd3-dc69837847e0
 
-# Set the add_zones_with_mupdate_override planner config to ensure that zone
-# adds happen despite zone image sources not being Artifact.
-set planner-config --add-zones-with-mupdate-override true
-
 # blueprint-plan will place a new external DNS zone, diff DNS to see the new zone has `ns<N>` and NS records.
 blueprint-plan 366b0b68-d80e-4bc1-abd3-dc69837847e0
 blueprint-diff 366b0b68-d80e-4bc1-abd3-dc69837847e0 9c998c1d-1a7b-440a-ae0c-40f781dea6e2