[reconfigurator-cli] disallow loading state into non-empty systems (#6891)

sunshowers · web-flow · commit 1e12c0e39b84 · 2024-10-19T00:56:24.000Z
Currently, the `cmd_load` command doesn't require an empty system and
lets you effectively merge two states. However, one of the things it
does is overwrite the internal and external DNS maps, which means that
existing blueprints can end up pointing to DNS generations that don't
have any correspondence.

The easiest way to address this issue is to change the `load` function
to require an empty system, similar to `load-example`. Make that change
here, and add some tests for it.

The load function also becomes somewhat simpler, since a bunch of checks
that currently exist can go away.
diff --git a/dev-tools/reconfigurator-cli/src/main.rs b/dev-tools/reconfigurator-cli/src/main.rs
@@ -29,7 +29,6 @@ use nexus_types::deployment::BlueprintZoneFilter;
 use nexus_types::deployment::OmicronZoneNic;
 use nexus_types::deployment::PlanningInput;
 use nexus_types::deployment::SledFilter;
-use nexus_types::deployment::SledLookupErrorKind;
 use nexus_types::deployment::{Blueprint, UnstableReconfiguratorState};
 use nexus_types::internal_api::params::DnsConfigParams;
 use nexus_types::inventory::Collection;
@@ -152,18 +151,6 @@ impl ReconfiguratorSim {
         assert!(previous.is_none());
     }
 
-    fn blueprint_insert_loaded(
-        &mut self,
-        blueprint: Blueprint,
-    ) -> Result<(), anyhow::Error> {
-        let entry = self.blueprints.entry(blueprint.id);
-        if let indexmap::map::Entry::Occupied(_) = &entry {
-            return Err(anyhow!("blueprint already exists: {}", blueprint.id));
-        }
-        let _ = entry.or_insert(blueprint);
-        Ok(())
-    }
-
     fn planning_input(
         &self,
         parent_blueprint: &Blueprint,
@@ -1162,6 +1149,10 @@ fn cmd_load(
     sim: &mut ReconfiguratorSim,
     args: LoadArgs,
 ) -> anyhow::Result<Option<String>> {
+    if sim.user_made_system_changes() {
+        bail!("changes made to simulated system: run `wipe` before loading");
+    }
+
     let input_path = args.filename;
     let collection_id = args.collection_id;
     let loaded = read_file(&input_path)?;
@@ -1205,44 +1196,9 @@ fn cmd_load(
             },
         )?;
 
-    let current_planning_input = sim
-        .system
-        .to_planning_input_builder()
-        .context("generating planning input")?
-        .build();
     for (sled_id, sled_details) in
         loaded.planning_input.all_sleds(SledFilter::Commissioned)
     {
-        match current_planning_input
-            .sled_lookup(SledFilter::Commissioned, sled_id)
-        {
-            Ok(_) => {
-                swriteln!(
-                    s,
-                    "sled {}: skipped (one with \
-                     the same id is already loaded)",
-                    sled_id
-                );
-                continue;
-            }
-            Err(error) => match error.kind() {
-                SledLookupErrorKind::Filtered { .. } => {
-                    swriteln!(
-                        s,
-                        "error: load sled {}: turning a decommissioned sled \
-                         into a commissioned one is not supported",
-                        sled_id
-                    );
-                    continue;
-                }
-                SledLookupErrorKind::Missing => {
-                    // A sled being missing from the input is the only case in
-                    // which we decide to load new sleds. The logic to do that
-                    // is below.
-                }
-            },
-        }
-
         let Some(inventory_sled_agent) =
             primary_collection.sled_agents.get(&sled_id)
         else {
@@ -1290,32 +1246,38 @@ fn cmd_load(
     }
 
     for collection in loaded.collections {
-        if sim.collections.contains_key(&collection.id) {
-            swriteln!(
-                s,
-                "collection {}: skipped (one with the \
-                same id is already loaded)",
-                collection.id
-            );
-        } else {
-            swriteln!(s, "collection {} loaded", collection.id);
-            sim.collections.insert(collection.id, collection);
+        match sim.collections.entry(collection.id) {
+            indexmap::map::Entry::Occupied(_) => {
+                // We started with an empty system, so the only way we can hit
+                // this is if the serialized state contains a duplicate
+                // collection ID.
+                swriteln!(
+                    s,
+                    "error: collection {} skipped (duplicate found)",
+                    collection.id
+                )
+            }
+            indexmap::map::Entry::Vacant(entry) => {
+                swriteln!(s, "collection {} loaded", collection.id);
+                entry.insert(collection);
+            }
         }
     }
 
     for blueprint in loaded.blueprints {
-        let blueprint_id = blueprint.id;
-        match sim.blueprint_insert_loaded(blueprint) {
-            Ok(_) => {
-                swriteln!(s, "blueprint {} loaded", blueprint_id);
-            }
-            Err(error) => {
+        match sim.blueprints.entry(blueprint.id) {
+            // We started with an empty system, so the only way we can hit this
+            // is if the serialized state contains a duplicate blueprint ID.
+            indexmap::map::Entry::Occupied(_) => {
                 swriteln!(
                     s,
-                    "blueprint {}: skipped ({:#})",
-                    blueprint_id,
-                    error
-                );
+                    "error: blueprint {} skipped (duplicate found)",
+                    blueprint.id
+                )
+            }
+            indexmap::map::Entry::Vacant(entry) => {
+                swriteln!(s, "blueprint {} loaded", blueprint.id);
+                entry.insert(blueprint);
             }
         }
     }
@@ -1356,10 +1318,7 @@ fn cmd_load_example(
     args: LoadExampleArgs,
 ) -> anyhow::Result<Option<String>> {
     if sim.user_made_system_changes() {
-        bail!(
-            "changes made to simulated system: run `wipe system` before \
-             loading an example system"
-        );
+        bail!("changes made to simulated system: run `wipe` before loading");
     }
 
     // Generate the example system.
diff --git a/dev-tools/reconfigurator-cli/tests/input/cmds.txt b/dev-tools/reconfigurator-cli/tests/input/cmds.txt
@@ -12,3 +12,10 @@ sled-list
 
 inventory-generate
 inventory-list
+
+save state.json
+load state.json
+
+wipe
+load state.json
+sled-show dde1c0e2-b10d-4621-b420-f179f7a7a00a
diff --git a/dev-tools/reconfigurator-cli/tests/output/cmd-example-stdout b/dev-tools/reconfigurator-cli/tests/output/cmd-example-stdout
@@ -4,7 +4,7 @@ loaded example system with:
 - blueprint: ade5749d-bdf3-4fab-a8ae-00bea01b3a5a
 
 > load-example --seed test-basic
-error: changes made to simulated system: run `wipe system` before loading an example system
+error: changes made to simulated system: run `wipe` before loading
 
 > 
 
diff --git a/dev-tools/reconfigurator-cli/tests/output/cmd-stdout b/dev-tools/reconfigurator-cli/tests/output/cmd-stdout
@@ -66,3 +66,56 @@ generated inventory collection ..........<REDACTED_UUID>........... from configu
 ID                                   NERRORS TIME_DONE                
 ..........<REDACTED_UUID>........... 0       <REDACTED_TIMESTAMP> 
 
+> 
+
+> save state.json
+saved planning input, collections, and blueprints to "state.json"
+
+> load state.json
+error: changes made to simulated system: run `wipe` before loading
+
+> 
+
+> wipe
+wiped reconfigurator-sim state
+
+> load state.json
+using collection ..........<REDACTED_UUID>........... as source of sled inventory data
+sled ..........<REDACTED_UUID>........... loaded
+sled ..........<REDACTED_UUID>........... loaded
+sled ..........<REDACTED_UUID>........... loaded
+collection ..........<REDACTED_UUID>........... loaded
+loaded service IP pool ranges: [V4(Ipv4Range { first: 192.0.2.2, last: 192.0.2.20 })]
+configured external DNS zone name: oxide.example
+configured silo names: example-silo
+internal DNS generations: 
+external DNS generations: 
+loaded data from "state.json"
+
+
+> sled-show ..........<REDACTED_UUID>...........
+sled ..........<REDACTED_UUID>...........
+subnet fd00:1122:3344:101::/64
+zpools (10):
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+    ..........<REDACTED_UUID>........... (zpool)
+    ↳ SledDisk { disk_identity: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-..........<REDACTED_UUID>..........." }, disk_id: ..........<REDACTED_UUID>........... (physical_disk), policy: InService, state: Active }
+
+
diff --git a/dev-tools/reconfigurator-cli/tests/test_basic.rs b/dev-tools/reconfigurator-cli/tests/test_basic.rs
@@ -30,18 +30,34 @@ use std::path::PathBuf;
 use std::sync::Arc;
 use std::time::Duration;
 use subprocess::Exec;
+use subprocess::ExitStatus;
 use swrite::swriteln;
 use swrite::SWrite;
 
 fn path_to_cli() -> PathBuf {
     path_to_executable(env!("CARGO_BIN_EXE_reconfigurator-cli"))
 }
 
+fn run_cli(file: impl AsRef<Utf8Path>) -> (ExitStatus, String, String) {
+    let file = file.as_ref();
+
+    // Turn the path into an absolute one, because we're going to set a custom
+    // cwd for the subprocess.
+    let file = file.canonicalize_utf8().expect("file canonicalized");
+    eprintln!("using file: {file}");
+
+    // Create a temporary directory for the CLI to use -- that will let it
+    // read and write files in its own sandbox.
+    let tmpdir = camino_tempfile::tempdir().expect("failed to create tmpdir");
+    let exec = Exec::cmd(path_to_cli()).arg(file).cwd(tmpdir.path());
+    run_command(exec)
+}
+
 // Run a battery of simple commands and make sure things basically seem to work.
 #[test]
 fn test_basic() {
-    let exec = Exec::cmd(path_to_cli()).arg("tests/input/cmds.txt");
-    let (exit_status, stdout_text, stderr_text) = run_command(exec);
+    let (exit_status, stdout_text, stderr_text) =
+        run_cli("tests/input/cmds.txt");
     assert_exit_code(exit_status, EXIT_SUCCESS, &stderr_text);
     let stdout_text = Redactor::default().do_redact(&stdout_text);
     assert_contents("tests/output/cmd-stdout", &stdout_text);
@@ -51,8 +67,8 @@ fn test_basic() {
 // Run tests against a loaded example system.
 #[test]
 fn test_example() {
-    let exec = Exec::cmd(path_to_cli()).arg("tests/input/cmds-example.txt");
-    let (exit_status, stdout_text, stderr_text) = run_command(exec);
+    let (exit_status, stdout_text, stderr_text) =
+        run_cli("tests/input/cmds-example.txt");
     assert_exit_code(exit_status, EXIT_SUCCESS, &stderr_text);
 
     // The example system uses a fixed seed, which means that UUIDs are

Original file line number	Diff line number	Diff line change
`@@ -4,7 +4,7 @@ loaded example system with:`
`4`	`4`	`- blueprint: ade5749d-bdf3-4fab-a8ae-00bea01b3a5a`
`5`	`5`
`6`	`6`	`> load-example --seed test-basic`
`7`		-error: changes made to simulated system: run `wipe system` before loading an example system
	`7`	+error: changes made to simulated system: run `wipe` before loading
`8`	`8`
`9`	`9`	`>`
`10`	`10`