[reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone (#5238)

This greatly simplifies the handling of the expunged disposition in
#5211. It also is more understandable, and less prone to making
mistakes.

This PR only changes the in-memory representation of what is currently
`zones_in_service`. I've skipped doing the DB migration in this PR
because it'll be easier to just do it wholesale in #5211.

There are some small differences in diff output that I think make sense.
But I took a bigger swing at it in #5270, which depends on this.

Author: sunshowers

clients/sled-agent-client/src/lib.rs

@@ -114,6 +114,24 @@ impl types::OmicronZoneType {
         }
     }
 
+    /// Identifies whether this a Crucible (not Crucible pantry) zone
+    pub fn is_crucible(&self) -> bool {
+        match self {
+            types::OmicronZoneType::Crucible { .. } => true,
+
+            types::OmicronZoneType::BoundaryNtp { .. }
+            | types::OmicronZoneType::InternalNtp { .. }
+            | types::OmicronZoneType::Clickhouse { .. }
+            | types::OmicronZoneType::ClickhouseKeeper { .. }
+            | types::OmicronZoneType::CockroachDb { .. }
+            | types::OmicronZoneType::CruciblePantry { .. }
+            | types::OmicronZoneType::ExternalDns { .. }
+            | types::OmicronZoneType::InternalDns { .. }
+            | types::OmicronZoneType::Nexus { .. }
+            | types::OmicronZoneType::Oximeter { .. } => false,
+        }
+    }
+
     /// This zone's external IP
     pub fn external_ip(&self) -> anyhow::Result<Option<IpAddr>> {
         match self {

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

@@ -547,8 +547,7 @@ fn cmd_blueprint_diff_inventory(
         .get(&blueprint_id)
         .ok_or_else(|| anyhow!("no such blueprint: {}", blueprint_id))?;
 
-    let zones = collection.all_omicron_zones().map(|z| z.id).collect();
-    let diff = blueprint.diff_sleds_from_collection(&collection, &zones);
+    let diff = blueprint.diff_sleds_from_collection(&collection);
     Ok(Some(diff.display().to_string()))
 }
 

dev-tools/reconfigurator-cli/tests/output/cmd-complex-stdout

@@ -15,6 +15,9 @@ use crate::{ipv6, Generation, MacAddr, Name, SqlU16, SqlU32, SqlU8};
 use chrono::{DateTime, Utc};
 use ipnetwork::IpNetwork;
 use nexus_types::deployment::BlueprintTarget;
+use nexus_types::deployment::BlueprintZoneConfig;
+use nexus_types::deployment::BlueprintZoneDisposition;
+use nexus_types::deployment::BlueprintZonesConfig;
 use omicron_common::api::internal::shared::NetworkInterface;
 use uuid::Uuid;
 
@@ -103,7 +106,7 @@ impl BpSledOmicronZones {
     pub fn new(
         blueprint_id: Uuid,
         sled_id: Uuid,
-        zones_config: &nexus_types::deployment::OmicronZonesConfig,
+        zones_config: &BlueprintZonesConfig,
     ) -> Self {
         Self {
             blueprint_id,
@@ -144,9 +147,9 @@ impl BpOmicronZone {
     pub fn new(
         blueprint_id: Uuid,
         sled_id: Uuid,
-        zone: &nexus_types::inventory::OmicronZoneConfig,
+        zone: &BlueprintZoneConfig,
     ) -> Result<Self, anyhow::Error> {
-        let zone = OmicronZone::new(sled_id, zone)?;
+        let zone = OmicronZone::new(sled_id, &zone.config)?;
         Ok(Self {
             blueprint_id,
             sled_id: zone.sled_id,
@@ -172,10 +175,11 @@ impl BpOmicronZone {
         })
     }
 
-    pub fn into_omicron_zone_config(
+    pub fn into_blueprint_zone_config(
         self,
         nic_row: Option<BpOmicronZoneNic>,
-    ) -> Result<nexus_types::inventory::OmicronZoneConfig, anyhow::Error> {
+        disposition: BlueprintZoneDisposition,
+    ) -> Result<BlueprintZoneConfig, anyhow::Error> {
         let zone = OmicronZone {
             sled_id: self.sled_id,
             id: self.id,
@@ -198,7 +202,9 @@ impl BpOmicronZone {
             snat_first_port: self.snat_first_port,
             snat_last_port: self.snat_last_port,
         };
-        zone.into_omicron_zone_config(nic_row.map(OmicronZoneNic::from))
+        let config =
+            zone.into_omicron_zone_config(nic_row.map(OmicronZoneNic::from))?;
+        Ok(BlueprintZoneConfig { config, disposition })
     }
 }
 
@@ -234,9 +240,9 @@ impl From<BpOmicronZoneNic> for OmicronZoneNic {
 impl BpOmicronZoneNic {
     pub fn new(
         blueprint_id: Uuid,
-        zone: &nexus_types::inventory::OmicronZoneConfig,
+        zone: &BlueprintZoneConfig,
     ) -> Result<Option<BpOmicronZoneNic>, anyhow::Error> {
-        let zone_nic = OmicronZoneNic::new(zone)?;
+        let zone_nic = OmicronZoneNic::new(&zone.config)?;
         Ok(zone_nic.map(|nic| Self {
             blueprint_id,
             id: nic.id,

nexus/db-model/src/deployment.rs

@@ -43,7 +43,9 @@ use nexus_db_model::BpTarget;
 use nexus_types::deployment::Blueprint;
 use nexus_types::deployment::BlueprintMetadata;
 use nexus_types::deployment::BlueprintTarget;
-use nexus_types::deployment::OmicronZonesConfig;
+use nexus_types::deployment::BlueprintZoneDisposition;
+use nexus_types::deployment::BlueprintZoneFilter;
+use nexus_types::deployment::BlueprintZonesConfig;
 use omicron_common::api::external::DataPageParams;
 use omicron_common::api::external::Error;
 use omicron_common::api::external::ListResultVec;
@@ -105,52 +107,67 @@ impl DataStore {
         // so that we can produce the `Error` type that we want here.
         let row_blueprint = DbBlueprint::from(blueprint);
         let blueprint_id = row_blueprint.id;
+
+        // `Blueprint` stores the policy for each zone next to the zone itself.
+        // This would ideally be represented as a simple column in
+        // bp_omicron_zone.
+        //
+        // But historically, `Blueprint` used to store the set of zones in
+        // service in a BTreeSet. Since most zones are expected to be in
+        // service, we store the set of zones NOT in service (which we expect
+        // to be much smaller, often empty). Build that inverted set here.
+        //
+        // This will soon be replaced with an extra column in the
+        // `bp_omicron_zone` table, coupled with other data migrations.
+        let omicron_zones_not_in_service = blueprint
+            .all_blueprint_zones(BlueprintZoneFilter::All)
+            .filter_map(|(_, zone)| {
+                // This is going to go away soon when we change the database
+                // representation to store the zone disposition enum next to
+                // each zone. For now, do an exhaustive match so that this
+                // fails if we add a new variant.
+                match zone.disposition {
+                    BlueprintZoneDisposition::InService => None,
+                    BlueprintZoneDisposition::Quiesced => {
+                        Some(BpOmicronZoneNotInService {
+                            blueprint_id,
+                            bp_omicron_zone_id: zone.config.id,
+                        })
+                    }
+                }
+            })
+            .collect::<Vec<_>>();
+
         let sled_omicron_zones = blueprint
-            .omicron_zones
+            .blueprint_zones
             .iter()
             .map(|(sled_id, zones_config)| {
                 BpSledOmicronZones::new(blueprint_id, *sled_id, zones_config)
             })
             .collect::<Vec<_>>();
         let omicron_zones = blueprint
-            .omicron_zones
+            .blueprint_zones
             .iter()
             .flat_map(|(sled_id, zones_config)| {
-                zones_config.zones.iter().map(|zone| {
+                zones_config.zones.iter().map(move |zone| {
                     BpOmicronZone::new(blueprint_id, *sled_id, zone)
                         .map_err(|e| Error::internal_error(&format!("{:#}", e)))
                 })
             })
             .collect::<Result<Vec<_>, Error>>()?;
         let omicron_zone_nics = blueprint
-            .omicron_zones
+            .blueprint_zones
             .values()
             .flat_map(|zones_config| {
                 zones_config.zones.iter().filter_map(|zone| {
                     BpOmicronZoneNic::new(blueprint_id, zone)
-                        .with_context(|| format!("zone {:?}", zone.id))
+                        .with_context(|| format!("zone {:?}", zone.config.id))
                         .map_err(|e| Error::internal_error(&format!("{:#}", e)))
                         .transpose()
                 })
             })
             .collect::<Result<Vec<BpOmicronZoneNic>, _>>()?;
 
-        // `Blueprint` stores a set of zones in service, but in the database we
-        // store the set of zones NOT in service (which we expect to be much
-        // smaller, often empty). Build that inverted set here.
-        let omicron_zones_not_in_service = {
-            let mut zones_not_in_service = Vec::new();
-            for zone in &omicron_zones {
-                if !blueprint.zones_in_service.contains(&zone.id) {
-                    zones_not_in_service.push(BpOmicronZoneNotInService {
-                        blueprint_id,
-                        bp_omicron_zone_id: zone.id,
-                    });
-                }
-            }
-            zones_not_in_service
-        };
-
         // This implementation inserts all records associated with the
         // blueprint in one transaction.  This is required: we don't want
         // any planner or executor to see a half-inserted blueprint, nor do we
@@ -273,10 +290,10 @@ impl DataStore {
         // the `OmicronZonesConfig` generation number for each sled that is a
         // part of this blueprint. Construct the BTreeMap we ultimately need,
         // but all the `zones` vecs will be empty until our next query below.
-        let mut omicron_zones: BTreeMap<Uuid, OmicronZonesConfig> = {
+        let mut blueprint_zones: BTreeMap<Uuid, BlueprintZonesConfig> = {
             use db::schema::bp_sled_omicron_zones::dsl;
 
-            let mut omicron_zones = BTreeMap::new();
+            let mut blueprint_zones = BTreeMap::new();
             let mut paginator = Paginator::new(SQL_BATCH_SIZE);
             while let Some(p) = paginator.next() {
                 let batch = paginated(
@@ -295,9 +312,9 @@ impl DataStore {
                 paginator = p.found_batch(&batch, &|s| s.sled_id);
 
                 for s in batch {
-                    let old = omicron_zones.insert(
+                    let old = blueprint_zones.insert(
                         s.sled_id,
-                        OmicronZonesConfig {
+                        BlueprintZonesConfig {
                             generation: *s.generation,
                             zones: Vec::new(),
                         },
@@ -310,7 +327,7 @@ impl DataStore {
                 }
             }
 
-            omicron_zones
+            blueprint_zones
         };
 
         // Assemble a mutable map of all the NICs found, by NIC id.  As we
@@ -391,11 +408,6 @@ impl DataStore {
             omicron_zones_not_in_service
         };
 
-        // Create the in-memory list of zones _in_ service, which we'll
-        // calculate below as we load zones. (Any zone that isn't present in
-        // `omicron_zones_not_in_service` is considered in service.)
-        let mut zones_in_service = BTreeSet::new();
-
         // Load all the zones for each sled.
         {
             use db::schema::bp_omicron_zone::dsl;
@@ -438,8 +450,9 @@ impl DataStore {
                             })
                         })
                         .transpose()?;
-                    let sled_zones =
-                        omicron_zones.get_mut(&z.sled_id).ok_or_else(|| {
+                    let sled_zones = blueprint_zones
+                        .get_mut(&z.sled_id)
+                        .ok_or_else(|| {
                             // This error means that we found a row in
                             // bp_omicron_zone with no associated record in
                             // bp_sled_omicron_zones.  This should be
@@ -451,8 +464,14 @@ impl DataStore {
                             ))
                         })?;
                     let zone_id = z.id;
+                    let disposition =
+                        if omicron_zones_not_in_service.remove(&zone_id) {
+                            BlueprintZoneDisposition::Quiesced
+                        } else {
+                            BlueprintZoneDisposition::InService
+                        };
                     let zone = z
-                        .into_omicron_zone_config(nic_row)
+                        .into_blueprint_zone_config(nic_row, disposition)
                         .with_context(|| {
                             format!("zone {:?}: parse from database", zone_id)
                         })
@@ -463,14 +482,6 @@ impl DataStore {
                             ))
                         })?;
                     sled_zones.zones.push(zone);
-
-                    // If we can remove `zone_id` from
-                    // `omicron_zones_not_in_service`, then the zone is not in
-                    // service. Otherwise, add it to the list of in-service
-                    // zones.
-                    if !omicron_zones_not_in_service.remove(&zone_id) {
-                        zones_in_service.insert(zone_id);
-                    }
                 }
             }
         }
@@ -488,8 +499,7 @@ impl DataStore {
 
         Ok(Blueprint {
             id: blueprint_id,
-            omicron_zones,
-            zones_in_service,
+            blueprint_zones,
             parent_blueprint_id,
             internal_dns_version,
             external_dns_version,
@@ -1359,10 +1369,8 @@ mod tests {
             [blueprint1.id]
         );
 
-        // There ought to be no sleds or zones in service, and no parent
-        // blueprint.
-        assert_eq!(blueprint1.omicron_zones.len(), 0);
-        assert_eq!(blueprint1.zones_in_service.len(), 0);
+        // There ought to be no sleds or zones, and no parent blueprint.
+        assert_eq!(blueprint1.blueprint_zones.len(), 0);
         assert_eq!(blueprint1.parent_blueprint_id, None);
 
         // Trying to insert the same blueprint again should fail.
@@ -1407,20 +1415,17 @@ mod tests {
         );
 
         // Check the number of blueprint elements against our collection.
-        assert_eq!(blueprint1.omicron_zones.len(), policy.sleds.len());
+        assert_eq!(blueprint1.blueprint_zones.len(), policy.sleds.len());
         assert_eq!(
-            blueprint1.omicron_zones.len(),
+            blueprint1.blueprint_zones.len(),
             collection.omicron_zones.len()
         );
         assert_eq!(
             blueprint1.all_omicron_zones().count(),
             collection.all_omicron_zones().count()
         );
         // All zones should be in service.
-        assert_eq!(
-            blueprint1.zones_in_service.len(),
-            blueprint1.all_omicron_zones().count()
-        );
+        assert_all_zones_in_service(&blueprint1);
         assert_eq!(blueprint1.parent_blueprint_id, None);
 
         // Set blueprint1 as the current target, and ensure that we cannot
@@ -1489,19 +1494,16 @@ mod tests {
 
         // Check that we added the new sled and its zones.
         assert_eq!(
-            blueprint1.omicron_zones.len() + 1,
-            blueprint2.omicron_zones.len()
+            blueprint1.blueprint_zones.len() + 1,
+            blueprint2.blueprint_zones.len()
         );
         assert_eq!(
             blueprint1.all_omicron_zones().count() + num_new_sled_zones,
             blueprint2.all_omicron_zones().count()
         );
 
         // All zones should be in service.
-        assert_eq!(
-            blueprint2.zones_in_service.len(),
-            blueprint2.all_omicron_zones().count()
-        );
+        assert_all_zones_in_service(&blueprint2);
         assert_eq!(blueprint2.parent_blueprint_id, Some(blueprint1.id));
 
         // Check that we can write it to the DB and read it back.
@@ -1869,4 +1871,18 @@ mod tests {
         db.cleanup().await.unwrap();
         logctx.cleanup_successful();
     }
+
+    fn assert_all_zones_in_service(blueprint: &Blueprint) {
+        let not_in_service = blueprint
+            .all_blueprint_zones(BlueprintZoneFilter::All)
+            .filter(|(_, z)| {
+                z.disposition != BlueprintZoneDisposition::InService
+            })
+            .collect::<Vec<_>>();
+        assert!(
+            not_in_service.is_empty(),
+            "expected all zones to be in service, \
+             found these zones not in service: {not_in_service:?}"
+        );
+    }
 }

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

@@ -952,7 +952,7 @@ mod test {
     };
     use omicron_common::api::internal::shared::SourceNatConfig;
     use omicron_test_utils::dev;
-    use std::collections::{BTreeMap, BTreeSet, HashMap};
+    use std::collections::{BTreeMap, HashMap};
     use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddrV6};
     use std::num::NonZeroU32;
 
@@ -965,8 +965,7 @@ mod test {
                 rack_subnet: nexus_test_utils::RACK_SUBNET.parse().unwrap(),
                 blueprint: Blueprint {
                     id: Uuid::new_v4(),
-                    omicron_zones: BTreeMap::new(),
-                    zones_in_service: BTreeSet::new(),
+                    blueprint_zones: BTreeMap::new(),
                     parent_blueprint_id: None,
                     internal_dns_version: *Generation::new(),
                     external_dns_version: *Generation::new(),