[27/n] [reconfigurator-cli] allow specifying "latest" for collection ID (#8536)

Also clean up code to diff blueprint versus inventory, an operation that
makes less and less sense as time goes on.

Author: sunshowers

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

@@ -22,7 +22,7 @@ use nexus_reconfigurator_planning::planner::Planner;
 use nexus_reconfigurator_planning::system::{
     SledBuilder, SledInventoryVisibility, SystemDescription,
 };
-use nexus_reconfigurator_simulation::{BlueprintId, SimState};
+use nexus_reconfigurator_simulation::{BlueprintId, CollectionId, SimState};
 use nexus_reconfigurator_simulation::{SimStateBuilder, SimTufRepoSource};
 use nexus_reconfigurator_simulation::{SimTufRepoDescription, Simulator};
 use nexus_sled_agent_shared::inventory::ZoneKind;
@@ -239,9 +239,6 @@ fn process_command(
         Commands::BlueprintShow(args) => cmd_blueprint_show(sim, args),
         Commands::BlueprintDiff(args) => cmd_blueprint_diff(sim, args),
         Commands::BlueprintDiffDns(args) => cmd_blueprint_diff_dns(sim, args),
-        Commands::BlueprintDiffInventory(args) => {
-            cmd_blueprint_diff_inventory(sim, args)
-        }
         Commands::BlueprintSave(args) => cmd_blueprint_save(sim, args),
         Commands::Show => cmd_show(sim),
         Commands::Set(args) => cmd_set(sim, args),
@@ -312,8 +309,6 @@ enum Commands {
     BlueprintDiff(BlueprintDiffArgs),
     /// show differences between a blueprint and a particular DNS version
     BlueprintDiffDns(BlueprintDiffDnsArgs),
-    /// show differences between a blueprint and an inventory collection
-    BlueprintDiffInventory(BlueprintDiffInventoryArgs),
     /// write one blueprint to a file
     BlueprintSave(BlueprintSaveArgs),
 
@@ -522,11 +517,11 @@ struct BlueprintPlanArgs {
     /// id of the blueprint on which this one will be based, "latest", or
     /// "target"
     parent_blueprint_id: BlueprintIdOpt,
-    /// id of the inventory collection to use in planning
+    /// id of the inventory collection to use in planning or "latest"
     ///
     /// Must be provided unless there is only one collection in the loaded
     /// state.
-    collection_id: Option<CollectionUuid>,
+    collection_id: Option<CollectionIdOpt>,
 }
 
 #[derive(Debug, Args)]
@@ -729,6 +724,34 @@ impl From<BlueprintIdOpt> for BlueprintId {
     }
 }
 
+#[derive(Clone, Debug)]
+enum CollectionIdOpt {
+    /// use the latest collection sorted by time created
+    Latest,
+    /// use a specific collection
+    Id(CollectionUuid),
+}
+
+impl FromStr for CollectionIdOpt {
+    type Err = anyhow::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {
+            "latest" => Ok(CollectionIdOpt::Latest),
+            _ => Ok(CollectionIdOpt::Id(s.parse()?)),
+        }
+    }
+}
+
+impl From<CollectionIdOpt> for CollectionId {
+    fn from(value: CollectionIdOpt) -> Self {
+        match value {
+            CollectionIdOpt::Latest => CollectionId::Latest,
+            CollectionIdOpt::Id(id) => CollectionId::Id(id),
+        }
+    }
+}
+
 /// Clap field for an optional mupdate override UUID.
 ///
 /// This structure is similar to `Option`, but is specified separately to:
@@ -1467,10 +1490,13 @@ fn cmd_blueprint_plan(
 
     let parent_blueprint_id =
         system.resolve_blueprint_id(args.parent_blueprint_id.into())?;
-    let collection_id = args.collection_id;
     let parent_blueprint = system.get_blueprint(&parent_blueprint_id)?;
-    let collection = match collection_id {
-        Some(collection_id) => system.get_collection(collection_id)?,
+    let collection = match args.collection_id {
+        Some(collection_id) => {
+            let resolved =
+                system.resolve_collection_id(collection_id.into())?;
+            system.get_collection(&resolved)?
+        }
         None => {
             let mut all_collections_iter = system.all_collections();
             match all_collections_iter.len() {
@@ -1888,23 +1914,6 @@ fn cmd_blueprint_diff_dns(
     Ok(Some(dns_diff.to_string()))
 }
 
-fn cmd_blueprint_diff_inventory(
-    sim: &mut ReconfiguratorSim,
-    args: BlueprintDiffInventoryArgs,
-) -> anyhow::Result<Option<String>> {
-    let collection_id = args.collection_id;
-    let blueprint_id = args.blueprint_id;
-
-    let state = sim.current_state();
-    let _collection = state.system().get_collection(collection_id)?;
-    let _blueprint =
-        state.system().resolve_and_get_blueprint(blueprint_id.into())?;
-    // See https://github.com/oxidecomputer/omicron/issues/7242
-    // let diff = blueprint.diff_since_collection(&collection);
-    // Ok(Some(diff.display().to_string()))
-    bail!("Not Implemented")
-}
-
 fn cmd_blueprint_save(
     sim: &mut ReconfiguratorSim,
     args: BlueprintSaveArgs,

dev-tools/reconfigurator-cli/tests/input/cmds-noop-image-source.txt

@@ -50,17 +50,17 @@ sled-set serial6 inventory-hidden
 
 # Generate an inventory and run a blueprint planning step.
 inventory-generate
-blueprint-plan latest eb0796d5-ab8a-4f7b-a884-b4aeacb8ab51
+blueprint-plan latest latest
 
 # This diff should show expected changes to the blueprint.
-blueprint-diff 8da82a8e-bf97-4fbd-8ddd-9f6462732cf1 latest
+blueprint-diff latest
 
 # Bring the hidden sled back.
 sled-set serial6 inventory-visible
 
 # Run another inventory and blueprint planning step.
 inventory-generate
-blueprint-plan latest 61f451b3-2121-4ed6-91c7-a550054f6c21
+blueprint-plan latest latest
 
 # This diff should show changes to the sled that's back in inventory.
-blueprint-diff 58d5e830-0884-47d8-a7cd-b2b3751adeb4 latest
+blueprint-diff latest

dev-tools/reconfigurator-cli/tests/output/cmds-noop-image-source-stdout

@@ -154,7 +154,7 @@ set sled e96e226f-4ed9-4c01-91b9-69a9cd076c9e inventory visibility: visible -> h
 > inventory-generate
 generated inventory collection eb0796d5-ab8a-4f7b-a884-b4aeacb8ab51 from configured sleds
 
-> blueprint-plan latest eb0796d5-ab8a-4f7b-a884-b4aeacb8ab51
+> blueprint-plan latest latest
 WARN skipping zones eligible for cleanup check (sled not present in latest inventory collection), sled_id: e96e226f-4ed9-4c01-91b9-69a9cd076c9e
 WARN skipping noop image source check since sled-agent encountered error retrieving zone manifest (this is abnormal), sled_id: 2b8f0cb3-0295-4b3c-bc58-4fe88b57112c, error: reconfigurator-sim simulated error: simulated error obtaining zone manifest
 INFO install dataset artifact hash matches TUF repo, switching out the zone image source to Artifact, sled_id: 98e6b7c2-2efa-41ca-b20a-0a4d61102fe6, tuf_artifact_id: nexus v1.0.0 (zone)
@@ -192,7 +192,7 @@ generated blueprint 58d5e830-0884-47d8-a7cd-b2b3751adeb4 based on parent bluepri
 
 
 > # This diff should show expected changes to the blueprint.
-> blueprint-diff 8da82a8e-bf97-4fbd-8ddd-9f6462732cf1 latest
+> blueprint-diff latest
 from: blueprint 8da82a8e-bf97-4fbd-8ddd-9f6462732cf1
 to:   blueprint 58d5e830-0884-47d8-a7cd-b2b3751adeb4
 
@@ -510,7 +510,7 @@ set sled e96e226f-4ed9-4c01-91b9-69a9cd076c9e inventory visibility: hidden -> vi
 > inventory-generate
 generated inventory collection 61f451b3-2121-4ed6-91c7-a550054f6c21 from configured sleds
 
-> blueprint-plan latest 61f451b3-2121-4ed6-91c7-a550054f6c21
+> blueprint-plan latest latest
 WARN skipping noop image source check since sled-agent encountered error retrieving zone manifest (this is abnormal), sled_id: 2b8f0cb3-0295-4b3c-bc58-4fe88b57112c, error: reconfigurator-sim simulated error: simulated error obtaining zone manifest
 INFO noop converting 0/0 install-dataset zones to artifact store, sled_id: 98e6b7c2-2efa-41ca-b20a-0a4d61102fe6
 WARN zone manifest inventory indicated install dataset artifact is invalid, not using artifact (this is abnormal), sled_id: aff6c093-197d-42c5-ad80-9f10ba051a34, zone_id: e8fe709c-725f-4bb2-b714-ffcda13a9e54, kind: internal_ntp, file_name: ntp.tar.gz, error: reconfigurator-sim: simulated error validating zone image
@@ -537,7 +537,7 @@ generated blueprint af934083-59b5-4bf6-8966-6fb5292c29e1 based on parent bluepri
 
 
 > # This diff should show changes to the sled that's back in inventory.
-> blueprint-diff 58d5e830-0884-47d8-a7cd-b2b3751adeb4 latest
+> blueprint-diff latest
 from: blueprint 58d5e830-0884-47d8-a7cd-b2b3751adeb4
 to:   blueprint af934083-59b5-4bf6-8966-6fb5292c29e1
 

nexus/reconfigurator/simulation/src/errors.rs

@@ -6,10 +6,11 @@ use std::collections::BTreeSet;
 
 use itertools::Itertools;
 use omicron_common::api::external::{Generation, Name};
-use omicron_uuid_kinds::CollectionUuid;
 use thiserror::Error;
 
-use crate::{BlueprintId, ResolvedBlueprintId};
+use crate::{
+    BlueprintId, CollectionId, ResolvedBlueprintId, ResolvedCollectionId,
+};
 
 /// The caller attempted to insert a duplicate key.
 #[derive(Clone, Debug, Error)]
@@ -23,7 +24,7 @@ impl DuplicateError {
         &self.id
     }
 
-    pub(crate) fn collection(id: CollectionUuid) -> Self {
+    pub(crate) fn collection(id: CollectionId) -> Self {
         Self { id: ObjectId::Collection(id) }
     }
 
@@ -46,8 +47,9 @@ impl DuplicateError {
 
 #[derive(Clone, Debug)]
 pub enum ObjectId {
-    Collection(CollectionUuid),
+    Collection(CollectionId),
     Blueprint(BlueprintId),
+    ResolvedCollection(ResolvedCollectionId),
     ResolvedBlueprint(ResolvedBlueprintId),
     InternalDns(Generation),
     ExternalDns(Generation),
@@ -57,7 +59,10 @@ pub enum ObjectId {
 impl ObjectId {
     fn to_error_string(&self) -> String {
         match self {
-            ObjectId::Collection(id) => {
+            ObjectId::Collection(CollectionId::Latest) => {
+                "no latest collection found".to_string()
+            }
+            ObjectId::Collection(CollectionId::Id(id)) => {
                 format!("collection ID {id}")
             }
             ObjectId::Blueprint(BlueprintId::Latest) => {
@@ -69,6 +74,7 @@ impl ObjectId {
             ObjectId::Blueprint(BlueprintId::Id(id)) => {
                 format!("blueprint ID {id}")
             }
+            ObjectId::ResolvedCollection(id) => id.to_string(),
             ObjectId::ResolvedBlueprint(id) => id.to_string(),
             ObjectId::InternalDns(generation) => {
                 format!("internal DNS at generation {generation}")
@@ -95,14 +101,18 @@ impl KeyError {
         &self.id
     }
 
-    pub(crate) fn collection(id: CollectionUuid) -> Self {
+    pub(crate) fn collection(id: CollectionId) -> Self {
         Self { id: ObjectId::Collection(id) }
     }
 
     pub(crate) fn blueprint(id: BlueprintId) -> Self {
         Self { id: ObjectId::Blueprint(id) }
     }
 
+    pub(crate) fn resolved_collection(id: ResolvedCollectionId) -> Self {
+        Self { id: ObjectId::ResolvedCollection(id) }
+    }
+
     pub(crate) fn resolved_blueprint(id: ResolvedBlueprintId) -> Self {
         Self { id: ObjectId::ResolvedBlueprint(id) }
     }

nexus/reconfigurator/simulation/src/system.rs

@@ -121,13 +121,32 @@ impl SimSystem {
         &self.description
     }
 
+    pub fn resolve_collection_id(
+        &self,
+        original: CollectionId,
+    ) -> Result<ResolvedCollectionId, KeyError> {
+        let resolved = match original {
+            CollectionId::Latest => {
+                // The invariant of self.collections is that the last element is
+                // the latest.
+                let (id, _) = self
+                    .collections
+                    .last()
+                    .ok_or(KeyError::collection(original))?;
+                *id
+            }
+            CollectionId::Id(id) => id,
+        };
+        Ok(ResolvedCollectionId { original, resolved })
+    }
+
     pub fn get_collection(
         &self,
-        id: CollectionUuid,
+        id: &ResolvedCollectionId,
     ) -> Result<&Collection, KeyError> {
-        match self.collections.get(&id) {
+        match self.collections.get(&id.resolved()) {
             Some(c) => Ok(&**c),
-            None => Err(KeyError::collection(id)),
+            None => Err(KeyError::resolved_collection(id.clone())),
         }
     }
 
@@ -261,10 +280,18 @@ impl SimSystemBuilder {
         &self.inner.system.description()
     }
 
+    #[inline]
+    pub fn resolve_collection_id(
+        &self,
+        original: CollectionId,
+    ) -> Result<ResolvedCollectionId, KeyError> {
+        self.inner.system.resolve_collection_id(original)
+    }
+
     #[inline]
     pub fn get_collection(
         &self,
-        id: CollectionUuid,
+        id: &ResolvedCollectionId,
     ) -> Result<&Collection, KeyError> {
         self.inner.system.get_collection(id)
     }
@@ -489,6 +516,49 @@ impl fmt::Display for ResolvedBlueprintId {
     }
 }
 
+/// An identifier for an inventory collection.
+#[derive(Clone, Copy, Debug)]
+pub enum CollectionId {
+    /// The latest inventory collection added to the system.
+    Latest,
+
+    /// The specified inventory collection.
+    Id(CollectionUuid),
+}
+
+/// An identifier for a blueprint after it has been resolved.
+#[derive(Clone, Debug)]
+pub struct ResolvedCollectionId {
+    /// The original collection ID.
+    original: CollectionId,
+
+    /// The resolved collection ID.
+    resolved: CollectionUuid,
+}
+
+impl ResolvedCollectionId {
+    /// Return the original collection ID.
+    pub fn original(&self) -> CollectionId {
+        self.original
+    }
+
+    /// Return the resolved collection ID.
+    pub fn resolved(&self) -> CollectionUuid {
+        self.resolved
+    }
+}
+
+impl fmt::Display for ResolvedCollectionId {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self.original {
+            CollectionId::Latest => {
+                write!(f, "latest collection ({})", self.resolved)
+            }
+            CollectionId::Id(id) => write!(f, "collection {id}"),
+        }
+    }
+}
+
 /// A log entry corresponding to an individual operation on a
 /// [`SimSystemBuilder`].
 #[derive(Clone, Debug)]
@@ -808,7 +878,9 @@ impl SimSystemBuilderInner {
             |_, other| other.time_started <= time_started,
         ) {
             Ok(()) => Ok(()),
-            Err(_) => Err(DuplicateError::collection(collection_id)),
+            Err(_) => {
+                Err(DuplicateError::collection(CollectionId::Id(collection_id)))
+            }
         }
     }