[reconfigurator] Remove spicy blueprint -> sled-agent From impls (#7505)

This is followup from #7308 and
#7500 (comment):

* Remove `From<BlueprintZonesConfig> for OmicronZonesConfig`. This
included expunged zones, but thankfully had no callers.
* Change `BlueprintZonesConfig::to_omicron_zones_config(filter)` to
`BlueprintZonesConfig::into_running_omicron_zones_config()` (no `filter`
argument). All the callers of this method were passing
`BlueprintZoneFilter::ShouldBeRunning`, and I don't think there's a
reason to use any other filter?
* Remove `From<BlueprintPhysicalDisksConfig> for
OmicronPhysicalDisksConfig` (which included expunged disks), and replace
it with `BlueprintPhysicalDisksConfig::into_in_service_disks()`. This
one _did_ have callers, including the blueprint executor, but I think
we've avoided problems because the planner current [drops disks if the
incoming planning input says they're not in
service](https://github.com/oxidecomputer/omicron/blob/3ae42bf76cb9b55993705b817157e4f60935b6dd/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs#L1090-L1097).
I'm not sure that planner behavior is right, and might change with
#7286, so it seemed safer to go ahead and fix this now.

Author: jgallagher

nexus/reconfigurator/execution/src/clickhouse.rs

@@ -22,7 +22,7 @@ use futures::future::Either;
 use futures::stream::FuturesUnordered;
 use futures::stream::StreamExt;
 use nexus_db_queries::context::OpContext;
-use nexus_sled_agent_shared::inventory::OmicronZoneType;
+use nexus_types::deployment::Blueprint;
 use nexus_types::deployment::BlueprintZoneFilter;
 use nexus_types::deployment::BlueprintZonesConfig;
 use nexus_types::deployment::ClickhouseClusterConfig;
@@ -182,18 +182,9 @@ pub(crate) async fn deploy_single_node(
     opctx: &OpContext,
     zones: &BTreeMap<SledUuid, BlueprintZonesConfig>,
 ) -> Result<(), anyhow::Error> {
-    if let Some(zone) = zones
-        .values()
-        .flat_map(|zones| {
-            zones
-                .to_omicron_zones_config(BlueprintZoneFilter::ShouldBeRunning)
-                .zones
-                .into_iter()
-                .find(|zone| {
-                    matches!(zone.zone_type, OmicronZoneType::Clickhouse { .. })
-                })
-        })
-        .next()
+    if let Some((_, zone)) =
+        Blueprint::filtered_zones(zones, BlueprintZoneFilter::ShouldBeRunning)
+            .find(|(_, zone)| zone.zone_type.is_clickhouse())
     {
         let admin_addr = SocketAddr::V6(SocketAddrV6::new(
             zone.underlay_ip(),

nexus/reconfigurator/execution/src/omicron_physical_disks.rs

@@ -48,7 +48,9 @@ pub(crate) async fn deploy_disks(
                 &log,
             );
             let result = client
-                .omicron_physical_disks_put(&config.clone().into())
+                .omicron_physical_disks_put(
+                    &config.clone().into_in_service_disks(),
+                )
                 .await
                 .with_context(|| {
                     format!("Failed to put {config:#?} to sled {sled_id}")

nexus/reconfigurator/execution/src/omicron_zones.rs

@@ -19,7 +19,6 @@ use nexus_db_queries::db::datastore::CollectorReassignment;
 use nexus_db_queries::db::DataStore;
 use nexus_types::deployment::BlueprintZoneConfig;
 use nexus_types::deployment::BlueprintZoneDisposition;
-use nexus_types::deployment::BlueprintZoneFilter;
 use nexus_types::deployment::BlueprintZoneType;
 use nexus_types::deployment::BlueprintZonesConfig;
 use omicron_common::address::COCKROACH_ADMIN_PORT;
@@ -65,8 +64,8 @@ pub(crate) async fn deploy_zones(
                 db_sled.sled_agent_address(),
                 &opctx.log,
             );
-            let omicron_zones = config
-                .to_omicron_zones_config(BlueprintZoneFilter::ShouldBeRunning);
+            let omicron_zones =
+                config.clone().into_running_omicron_zones_config();
             let result = client
                 .omicron_zones_put(&omicron_zones)
                 .await

nexus/reconfigurator/planning/src/example.rs

@@ -15,7 +15,6 @@ use crate::system::SledBuilder;
 use crate::system::SystemDescription;
 use nexus_inventory::CollectionBuilderRng;
 use nexus_types::deployment::Blueprint;
-use nexus_types::deployment::BlueprintZoneFilter;
 use nexus_types::deployment::OmicronZoneNic;
 use nexus_types::deployment::PlanningInput;
 use nexus_types::deployment::SledFilter;
@@ -490,9 +489,7 @@ impl ExampleSystemBuilder {
             system
                 .sled_set_omicron_zones(
                     *sled_id,
-                    zones.to_omicron_zones_config(
-                        BlueprintZoneFilter::ShouldBeRunning,
-                    ),
+                    zones.clone().into_running_omicron_zones_config(),
                 )
                 .unwrap();
         }
@@ -546,7 +543,7 @@ impl ZoneCount {
 mod tests {
     use chrono::{NaiveDateTime, TimeZone, Utc};
     use nexus_sled_agent_shared::inventory::{OmicronZoneConfig, ZoneKind};
-    use nexus_types::deployment::BlueprintZoneConfig;
+    use nexus_types::deployment::{BlueprintZoneConfig, BlueprintZoneFilter};
     use omicron_test_utils::dev::test_setup_log;
 
     use super::*;

nexus/reconfigurator/planning/src/planner.rs

@@ -976,9 +976,8 @@ mod test {
                     .blueprint_zones
                     .get(&new_sled_id)
                     .expect("blueprint should contain zones for new sled")
-                    .to_omicron_zones_config(
-                        BlueprintZoneFilter::ShouldBeRunning,
-                    ),
+                    .clone()
+                    .into_running_omicron_zones_config(),
             )
             .unwrap();
         let collection =

nexus/types/src/deployment.rs

@@ -547,33 +547,30 @@ pub struct BlueprintZonesConfig {
     pub zones: IdMap<BlueprintZoneConfig>,
 }
 
-impl From<BlueprintZonesConfig> for OmicronZonesConfig {
-    fn from(config: BlueprintZonesConfig) -> Self {
-        Self {
-            generation: config.generation,
-            zones: config.zones.into_iter().map(From::from).collect(),
-        }
-    }
-}
-
 impl BlueprintZonesConfig {
-    /// Converts self to an [`OmicronZonesConfig`], applying the provided
-    /// [`BlueprintZoneFilter`].
+    /// Converts self into [`OmicronZonesConfig`].
     ///
-    /// The filter controls which zones should be exported into the resulting
-    /// [`OmicronZonesConfig`].
-    pub fn to_omicron_zones_config(
-        &self,
-        filter: BlueprintZoneFilter,
-    ) -> OmicronZonesConfig {
+    /// [`OmicronZonesConfig`] is a format of the zones configuration that can
+    /// be passed to Sled Agents for deployment.
+    ///
+    /// This function is effectively a `From` implementation, but
+    /// is named slightly more explicitly, as it filters the blueprint
+    /// configuration to only consider zones that should be running.
+    pub fn into_running_omicron_zones_config(self) -> OmicronZonesConfig {
         OmicronZonesConfig {
             generation: self.generation,
             zones: self
                 .zones
-                .iter()
-                .filter(|z| z.disposition.matches(filter))
-                .cloned()
-                .map(OmicronZoneConfig::from)
+                .into_iter()
+                .filter_map(|z| {
+                    if z.disposition
+                        .matches(BlueprintZoneFilter::ShouldBeRunning)
+                    {
+                        Some(z.into())
+                    } else {
+                        None
+                    }
+                })
                 .collect(),
         }
     }
@@ -899,6 +896,33 @@ pub struct BlueprintPhysicalDisksConfig {
     pub disks: IdMap<BlueprintPhysicalDiskConfig>,
 }
 
+impl BlueprintPhysicalDisksConfig {
+    /// Converts self into [`OmicronPhysicalDisksConfig`].
+    ///
+    /// [`OmicronPhysicalDisksConfig`] is a format of the disks configuration
+    /// that can be passed to Sled Agents for deployment.
+    ///
+    /// This function is effectively a `From` implementation, but
+    /// is named slightly more explicitly, as it filters the blueprint
+    /// configuration to only consider in-service disks.
+    pub fn into_in_service_disks(self) -> OmicronPhysicalDisksConfig {
+        OmicronPhysicalDisksConfig {
+            generation: self.generation,
+            disks: self
+                .disks
+                .into_iter()
+                .filter_map(|d| {
+                    if d.disposition.matches(DiskFilter::InService) {
+                        Some(d.into())
+                    } else {
+                        None
+                    }
+                })
+                .collect(),
+        }
+    }
+}
+
 impl IdMappable for BlueprintPhysicalDiskConfig {
     type Id = PhysicalDiskUuid;
 
@@ -927,19 +951,6 @@ impl From<BlueprintPhysicalDiskConfig> for OmicronPhysicalDiskConfig {
     }
 }
 
-impl From<BlueprintPhysicalDisksConfig> for OmicronPhysicalDisksConfig {
-    fn from(value: BlueprintPhysicalDisksConfig) -> Self {
-        OmicronPhysicalDisksConfig {
-            generation: value.generation,
-            disks: value
-                .disks
-                .into_iter()
-                .map(OmicronPhysicalDiskConfig::from)
-                .collect(),
-        }
-    }
-}
-
 /// Information about Omicron datasets as recorded in a blueprint.
 #[derive(
     Debug, Clone, Eq, PartialEq, JsonSchema, Deserialize, Serialize, Diffable,
@@ -950,7 +961,7 @@ pub struct BlueprintDatasetsConfig {
 }
 
 impl BlueprintDatasetsConfig {
-    /// Converts [Self] into [DatasetsConfig].
+    /// Converts self into [DatasetsConfig].
     ///
     /// [DatasetsConfig] is a format of the dataset configuration that can be
     /// passed to Sled Agents for deployment.

sled-agent/src/rack_setup/service.rs

@@ -344,7 +344,7 @@ impl ServiceInner {
                 // Ensure all the physical disks are initialized
                 self.initialize_disks_on_sled(
                     *sled_address,
-                    config.disks.clone().into(),
+                    config.disks.clone().into_in_service_disks(),
                 )
                 .await?;