Store comments as strongly-typed operations (#5834)

Attempts to resolve the discussion in
#5599 (comment)
by using a strongly-typed `Operation` enum instead of arbitrary String
comments.

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

@@ -748,7 +748,10 @@ fn cmd_blueprint_edit(
             let added = builder
                 .sled_ensure_zone_multiple_nexus(sled_id, current + 1)
                 .context("failed to add Nexus zone")?;
-            assert_matches::assert_matches!(added, EnsureMultiple::Added(1));
+            assert_matches::assert_matches!(
+                added,
+                EnsureMultiple::Changed { added: 1, removed: 0 }
+            );
             format!("added Nexus zone to sled {}", sled_id)
         }
     };

nexus/db-queries/src/db/datastore/deployment.rs

@@ -1343,6 +1343,7 @@ mod tests {
     use nexus_inventory::now_db_precision;
     use nexus_reconfigurator_planning::blueprint_builder::BlueprintBuilder;
     use nexus_reconfigurator_planning::blueprint_builder::Ensure;
+    use nexus_reconfigurator_planning::blueprint_builder::EnsureMultiple;
     use nexus_reconfigurator_planning::example::example;
     use nexus_test_utils::db::test_setup_database;
     use nexus_types::deployment::BlueprintZoneDisposition;
@@ -1651,7 +1652,7 @@ mod tests {
                         .clone(),
                 )
                 .unwrap(),
-            Ensure::Added
+            EnsureMultiple::Changed { added: 4, removed: 0 }
         );
 
         // Add zones to our new sled.

nexus/reconfigurator/execution/src/dns.rs

@@ -1264,7 +1264,7 @@ mod test {
         let rv = builder
             .sled_ensure_zone_multiple_nexus(sled_id, nalready + 1)
             .unwrap();
-        assert_eq!(rv, EnsureMultiple::Added(1));
+        assert_eq!(rv, EnsureMultiple::Changed { added: 1, removed: 0 });
         let blueprint2 = builder.build();
         eprintln!("blueprint2: {}", blueprint2.display());
         // Figure out the id of the new zone.

nexus/reconfigurator/planning/src/blueprint_builder/builder.rs

@@ -58,6 +58,7 @@ use slog::Logger;
 use std::collections::BTreeMap;
 use std::collections::BTreeSet;
 use std::collections::HashSet;
+use std::fmt;
 use std::hash::Hash;
 use std::net::IpAddr;
 use std::net::Ipv6Addr;
@@ -112,11 +113,54 @@ pub enum Ensure {
 #[derive(Debug, Clone, Copy, Eq, PartialEq)]
 pub enum EnsureMultiple {
     /// action was taken, and multiple items were added
-    Added(usize),
+    Changed { added: usize, removed: usize },
+
     /// no action was necessary
     NotNeeded,
 }
 
+/// Describes operations which the BlueprintBuilder has performed to arrive
+/// at its state.
+///
+/// This information is meant to act as a more strongly-typed flavor of
+/// "comment", identifying which operations have occurred on the blueprint.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub(crate) enum Operation {
+    AddZone { sled_id: SledUuid, kind: sled_agent_client::ZoneKind },
+    UpdateDisks { sled_id: SledUuid, added: usize, removed: usize },
+    ZoneExpunged { sled_id: SledUuid, reason: ZoneExpungeReason, count: usize },
+}
+
+impl fmt::Display for Operation {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::AddZone { sled_id, kind } => {
+                write!(f, "sled {sled_id}: added zone: {kind}")
+            }
+            Self::UpdateDisks { sled_id, added, removed } => {
+                write!(f, "sled {sled_id}: added {added} disks, removed {removed} disks")
+            }
+            Self::ZoneExpunged { sled_id, reason, count } => {
+                let reason = match reason {
+                    ZoneExpungeReason::DiskExpunged => {
+                        "zone using expunged disk"
+                    }
+                    ZoneExpungeReason::SledDecommissioned => {
+                        "sled state is decomissioned"
+                    }
+                    ZoneExpungeReason::SledExpunged => {
+                        "sled policy is expunged"
+                    }
+                };
+                write!(
+                    f,
+                    "sled {sled_id}: expunged {count} zones because: {reason}"
+                )
+            }
+        }
+    }
+}
+
 /// Helper for assembling a blueprint
 ///
 /// There are two basic ways to assemble a new blueprint:
@@ -152,6 +196,7 @@ pub struct BlueprintBuilder<'a> {
     cockroachdb_setting_preserve_downgrade: CockroachDbPreserveDowngrade,
 
     creator: String,
+    operations: Vec<Operation>,
     comments: Vec<String>,
 
     // Random number generator for new UUIDs
@@ -274,6 +319,7 @@ impl<'a> BlueprintBuilder<'a> {
             cockroachdb_setting_preserve_downgrade: parent_blueprint
                 .cockroachdb_setting_preserve_downgrade,
             creator: creator.to_owned(),
+            operations: Vec::new(),
             comments: Vec::new(),
             rng: BlueprintBuilderRng::new(),
         })
@@ -320,7 +366,12 @@ impl<'a> BlueprintBuilder<'a> {
                 .cockroachdb_setting_preserve_downgrade,
             time_created: now_db_precision(),
             creator: self.creator,
-            comment: self.comments.join(", "),
+            comment: self
+                .comments
+                .into_iter()
+                .chain(self.operations.iter().map(|op| op.to_string()))
+                .collect::<Vec<String>>()
+                .join(", "),
         }
     }
 
@@ -342,17 +393,23 @@ impl<'a> BlueprintBuilder<'a> {
         self
     }
 
-    /// Sets the blueprints "comment"
-    ///
     /// This is a short human-readable string summarizing the changes reflected
-    /// in the blueprint.  This is only intended for debugging.
+    /// in the blueprint. This is only intended for debugging.
     pub fn comment<S>(&mut self, comment: S)
     where
         String: From<S>,
     {
         self.comments.push(String::from(comment));
     }
 
+    /// Records an operation to the blueprint, identifying what changes have
+    /// occurred.
+    ///
+    /// This is currently intended only for debugging.
+    pub(crate) fn record_operation(&mut self, operation: Operation) {
+        self.operations.push(operation);
+    }
+
     /// Expunges all zones requiring expungement from a sled.
     ///
     /// Returns a list of zone IDs expunged (excluding zones that were already
@@ -456,15 +513,17 @@ impl<'a> BlueprintBuilder<'a> {
             };
         }
         let count_and_reason = [
-            (count_disk_expunged, "zone was using expunged disk"),
-            (count_sled_decommissioned, "sled state is decommissioned"),
-            (count_sled_expunged, "sled policy is expunged"),
+            (count_disk_expunged, ZoneExpungeReason::DiskExpunged),
+            (count_sled_decommissioned, ZoneExpungeReason::SledDecommissioned),
+            (count_sled_expunged, ZoneExpungeReason::SledExpunged),
         ];
         for (count, reason) in count_and_reason {
             if count > 0 {
-                self.comment(format!(
-                    "sled {sled_id} ({reason}): {count} zones expunged",
-                ));
+                self.record_operation(Operation::ZoneExpunged {
+                    sled_id,
+                    reason,
+                    count,
+                });
             }
         }
 
@@ -483,7 +542,7 @@ impl<'a> BlueprintBuilder<'a> {
         &mut self,
         sled_id: SledUuid,
         resources: &SledResources,
-    ) -> Result<Ensure, Error> {
+    ) -> Result<EnsureMultiple, Error> {
         let (mut additions, removals) = {
             // These are the disks known to our (last?) blueprint
             let blueprint_disks: BTreeMap<_, _> = self
@@ -533,8 +592,10 @@ impl<'a> BlueprintBuilder<'a> {
         };
 
         if additions.is_empty() && removals.is_empty() {
-            return Ok(Ensure::NotNeeded);
+            return Ok(EnsureMultiple::NotNeeded);
         }
+        let added = additions.len();
+        let removed = removals.len();
 
         let disks = &mut self.disks.change_sled_disks(sled_id).disks;
 
@@ -543,7 +604,7 @@ impl<'a> BlueprintBuilder<'a> {
             !removals.contains(&PhysicalDiskUuid::from_untyped_uuid(config.id))
         });
 
-        Ok(Ensure::Added)
+        Ok(EnsureMultiple::Changed { added, removed })
     }
 
     pub fn sled_ensure_zone_ntp(
@@ -779,7 +840,7 @@ impl<'a> BlueprintBuilder<'a> {
             self.sled_add_zone(sled_id, zone)?;
         }
 
-        Ok(EnsureMultiple::Added(num_nexus_to_add))
+        Ok(EnsureMultiple::Changed { added: num_nexus_to_add, removed: 0 })
     }
 
     pub fn cockroachdb_preserve_downgrade(
@@ -1449,7 +1510,7 @@ pub mod test {
                     builder
                         .sled_ensure_disks(sled_id, &sled_resources)
                         .unwrap(),
-                    Ensure::Added,
+                    EnsureMultiple::Changed { added: 10, removed: 0 },
                 );
             }
 
@@ -1597,7 +1658,7 @@ pub mod test {
                 .sled_ensure_zone_multiple_nexus(sled_id, 1)
                 .expect("failed to ensure nexus zone");
 
-            assert_eq!(added, EnsureMultiple::Added(1));
+            assert_eq!(added, EnsureMultiple::Changed { added: 1, removed: 0 });
         }
 
         {
@@ -1615,7 +1676,7 @@ pub mod test {
                 .sled_ensure_zone_multiple_nexus(sled_id, 3)
                 .expect("failed to ensure nexus zone");
 
-            assert_eq!(added, EnsureMultiple::Added(3));
+            assert_eq!(added, EnsureMultiple::Changed { added: 3, removed: 0 });
         }
 
         {

nexus/reconfigurator/planning/src/planner.rs

@@ -10,6 +10,7 @@ use crate::blueprint_builder::BlueprintBuilder;
 use crate::blueprint_builder::Ensure;
 use crate::blueprint_builder::EnsureMultiple;
 use crate::blueprint_builder::Error;
+use crate::blueprint_builder::Operation;
 use crate::planner::omicron_zone_placement::PlacementError;
 use nexus_types::deployment::Blueprint;
 use nexus_types::deployment::BlueprintZoneConfig;
@@ -25,6 +26,7 @@ use nexus_types::external_api::views::SledPolicy;
 use nexus_types::external_api::views::SledState;
 use nexus_types::inventory::Collection;
 use omicron_uuid_kinds::SledUuid;
+use sled_agent_client::ZoneKind;
 use slog::error;
 use slog::{info, warn, Logger};
 use std::collections::BTreeMap;
@@ -228,16 +230,19 @@ impl<'a> Planner<'a> {
         {
             // First, we need to ensure that sleds are using their expected
             // disks. This is necessary before we can allocate any zones.
-            if self.blueprint.sled_ensure_disks(sled_id, &sled_resources)?
-                == Ensure::Added
+            if let EnsureMultiple::Changed { added, removed } =
+                self.blueprint.sled_ensure_disks(sled_id, &sled_resources)?
             {
                 info!(
                     &self.log,
                     "altered physical disks";
                     "sled_id" => %sled_id
                 );
-                self.blueprint
-                    .comment(&format!("sled {}: altered disks", sled_id));
+                self.blueprint.record_operation(Operation::UpdateDisks {
+                    sled_id,
+                    added,
+                    removed,
+                });
 
                 // Note that this doesn't actually need to short-circuit the
                 // rest of the blueprint planning, as long as during execution
@@ -254,8 +259,10 @@ impl<'a> Planner<'a> {
                     "found sled missing NTP zone (will add one)";
                     "sled_id" => %sled_id
                 );
-                self.blueprint
-                    .comment(&format!("sled {}: add NTP zone", sled_id));
+                self.blueprint.record_operation(Operation::AddZone {
+                    sled_id,
+                    kind: ZoneKind::BoundaryNtp,
+                });
                 // Don't make any other changes to this sled.  However, this
                 // change is compatible with any other changes to other sleds,
                 // so we can "continue" here rather than "break".
@@ -323,7 +330,10 @@ impl<'a> Planner<'a> {
                 // (Yes, it's currently the last thing in the loop, but being
                 // explicit here means we won't forget to do this when more code
                 // is added below.)
-                self.blueprint.comment(&format!("sled {}: add zones", sled_id));
+                self.blueprint.record_operation(Operation::AddZone {
+                    sled_id,
+                    kind: ZoneKind::Crucible,
+                });
                 continue;
             }
         }
@@ -429,12 +439,12 @@ impl<'a> Planner<'a> {
                 .blueprint
                 .sled_ensure_zone_multiple_nexus(sled_id, new_nexus_count)?
             {
-                EnsureMultiple::Added(n) => {
+                EnsureMultiple::Changed { added, removed: _ } => {
                     info!(
-                        self.log, "will add {n} Nexus zone(s) to sled";
+                        self.log, "will add {added} Nexus zone(s) to sled";
                         "sled_id" => %sled_id,
                     );
-                    total_added += n;
+                    total_added += added;
                 }
                 // This is only possible if we asked the sled to ensure the same
                 // number of zones it already has, but that's impossible based

nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_bp2.txt

@@ -104,7 +104,7 @@ WARNING: Zones exist without physical disks!
  METADATA:
     created by:::::::::::   test_blueprint2
     created at:::::::::::   1970-01-01T00:00:00.000Z
-    comment::::::::::::::   sled a1b477db-b629-48eb-911d-1ccdafca75b9 (sled policy is expunged): 12 zones expunged
+    comment::::::::::::::   sled a1b477db-b629-48eb-911d-1ccdafca75b9: expunged 12 zones because: sled policy is expunged
     internal DNS version:   1
     external DNS version:   1
 

nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt

@@ -168,7 +168,7 @@ WARNING: Zones exist without physical disks!
  METADATA:
     created by:::::::::::   test_blueprint2
     created at:::::::::::   1970-01-01T00:00:00.000Z
-    comment::::::::::::::   sled 48d95fef-bc9f-4f50-9a53-1e075836291d (sled policy is expunged): 12 zones expunged
+    comment::::::::::::::   sled 48d95fef-bc9f-4f50-9a53-1e075836291d: expunged 12 zones because: sled policy is expunged
     internal DNS version:   1
     external DNS version:   1