[reconfigurator] Rework planner/builder expungement logic (#7495)

The high-level summary of this change is that it moves the logic for
handling expunged sleds out of
`BlueprintBuilder::expunge_zones_for_sled()` (which is now gone
entirely) and into a new method on the planner
(`do_plan_expunge_for_commissioned_sled()`, naming suggestions welcome).

This fixes two problems; one stylistic, and one semantic:

1. `BlueprintBuilder::expunge_zones_for_sled()` was maybe the worst
offender for "is this a builder API or a planner API", because a big
chunk of its logic was actually delegated back to
`zone_needs_expungement()`, a helper function defined in the planner.
The builder now has more explicit and direct methods to expunge specific
things, and it's the planner that decides based on the planning input
which things to expunge.
2. `BlueprintBuilder::expunge_zones_for_sled()` only handled expunging
_zones_. When a sled was expunged, at no point did the planner expunge
the disks or datasets on that sled. We haven't been bitten by this yet
because the builder currently omits expunged sleds from
`blueprint_disks` and `blueprint_datasets` entirely, but that's a major
roadblock in the path to joining the maps together (which is a prereq
for fixing #7309 by merging sled-agent endpoints together).
`do_plan_expunge_for_commissioned_sled()` expunges disks, datasets, and
zones. For now we still omit the expunged sleds from `blueprint_disks`
and `blueprint_datasets`, but when we stop doing that we'll at least
have set the dispositions correctly.

Author: jgallagher

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

@@ -25,5 +25,6 @@ pub use sled_editor::ZonesEditError;
 pub(crate) use allocators::BlueprintResourceAllocator;
 pub(crate) use allocators::ExternalNetworkingChoice;
 pub(crate) use allocators::ExternalSnatNetworkingChoice;
+pub(crate) use sled_editor::DiskExpungeDetails;
 pub(crate) use sled_editor::EditedSled;
 pub(crate) use sled_editor::SledEditor;

nexus/reconfigurator/planning/src/blueprint_editor.rs

@@ -10,6 +10,8 @@ use illumos_utils::zpool::ZpoolName;
 use itertools::Either;
 use nexus_sled_agent_shared::inventory::ZoneKind;
 use nexus_types::deployment::blueprint_zone_type;
+use nexus_types::deployment::BlueprintDatasetConfig;
+use nexus_types::deployment::BlueprintDatasetFilter;
 use nexus_types::deployment::BlueprintDatasetsConfig;
 use nexus_types::deployment::BlueprintPhysicalDiskConfig;
 use nexus_types::deployment::BlueprintPhysicalDisksConfig;
@@ -68,6 +70,14 @@ pub enum SledInputError {
     MultipleDatasetsOfKind(#[from] MultipleDatasetsOfKind),
 }
 
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct DiskExpungeDetails {
+    pub disk_id: PhysicalDiskUuid,
+    pub did_expunge_disk: bool,
+    pub num_datasets_expunged: usize,
+    pub num_zones_expunged: usize,
+}
+
 #[derive(Debug, thiserror::Error)]
 pub enum SledEditError {
     #[error("editing a decommissioned sled is not allowed")]
@@ -236,6 +246,24 @@ impl SledEditor {
         }
     }
 
+    pub fn datasets(
+        &self,
+        filter: BlueprintDatasetFilter,
+    ) -> impl Iterator<Item = &BlueprintDatasetConfig> {
+        match &self.0 {
+            InnerSledEditor::Active(editor) => {
+                Either::Left(editor.datasets(filter))
+            }
+            InnerSledEditor::Decommissioned(edited) => Either::Right(
+                edited
+                    .datasets
+                    .datasets
+                    .iter()
+                    .filter(move |disk| disk.disposition.matches(filter)),
+            ),
+        }
+    }
+
     pub fn zones(
         &self,
         filter: BlueprintZoneFilter,
@@ -277,7 +305,7 @@ impl SledEditor {
     pub fn expunge_disk(
         &mut self,
         disk_id: &PhysicalDiskUuid,
-    ) -> Result<(), SledEditError> {
+    ) -> Result<DiskExpungeDetails, SledEditError> {
         self.as_active_mut()?.expunge_disk(disk_id)
     }
 
@@ -292,7 +320,7 @@ impl SledEditor {
     pub fn expunge_zone(
         &mut self,
         zone_id: &OmicronZoneUuid,
-    ) -> Result<(), SledEditError> {
+    ) -> Result<bool, SledEditError> {
         self.as_active_mut()?.expunge_zone(zone_id)
     }
 
@@ -437,6 +465,13 @@ impl ActiveSledEditor {
         self.disks.disks(filter)
     }
 
+    pub fn datasets(
+        &self,
+        filter: BlueprintDatasetFilter,
+    ) -> impl Iterator<Item = &BlueprintDatasetConfig> {
+        self.datasets.datasets(filter)
+    }
+
     pub fn zones(
         &self,
         filter: BlueprintZoneFilter,
@@ -465,15 +500,28 @@ impl ActiveSledEditor {
     pub fn expunge_disk(
         &mut self,
         disk_id: &PhysicalDiskUuid,
-    ) -> Result<(), SledEditError> {
-        let zpool_id = self.disks.expunge(disk_id)?;
+    ) -> Result<DiskExpungeDetails, SledEditError> {
+        let (did_expunge_disk, zpool_id) = self.disks.expunge(disk_id)?;
 
         // When we expunge a disk, we must also expunge any datasets on it, and
         // any zones that relied on those datasets.
-        self.datasets.expunge_all_on_zpool(&zpool_id);
-        self.zones.expunge_all_on_zpool(&zpool_id);
+        let num_datasets_expunged =
+            self.datasets.expunge_all_on_zpool(&zpool_id);
+        let num_zones_expunged = self.zones.expunge_all_on_zpool(&zpool_id);
+
+        if !did_expunge_disk {
+            // If we didn't expunge the disk, it was already expunged, so there
+            // shouldn't have been any datasets or zones to expunge.
+            debug_assert_eq!(num_datasets_expunged, 0);
+            debug_assert_eq!(num_zones_expunged, 0);
+        }
 
-        Ok(())
+        Ok(DiskExpungeDetails {
+            disk_id: *disk_id,
+            did_expunge_disk,
+            num_datasets_expunged,
+            num_zones_expunged,
+        })
     }
 
     pub fn add_zone(
@@ -499,7 +547,7 @@ impl ActiveSledEditor {
     pub fn expunge_zone(
         &mut self,
         zone_id: &OmicronZoneUuid,
-    ) -> Result<(), SledEditError> {
+    ) -> Result<bool, SledEditError> {
         let (did_expunge, config) = self.zones.expunge(zone_id)?;
 
         // If we didn't actually expunge the zone in this edit, we don't
@@ -513,7 +561,7 @@ impl ActiveSledEditor {
         // explicitly instead of only recording its zpool; once we fix that we
         // should be able to remove this check.
         if !did_expunge {
-            return Ok(());
+            return Ok(did_expunge);
         }
 
         if let Some(dataset) = config.filesystem_dataset() {
@@ -524,7 +572,7 @@ impl ActiveSledEditor {
                 .expunge(&dataset.dataset.pool_name.id(), &dataset.kind)?;
         }
 
-        Ok(())
+        Ok(did_expunge)
     }
 
     /// Backwards compatibility / test helper: If we're given a blueprint that

nexus/reconfigurator/planning/src/blueprint_editor/sled_editor.rs

@@ -8,6 +8,7 @@ use illumos_utils::zpool::ZpoolName;
 use nexus_types::deployment::id_map::{self, IdMap};
 use nexus_types::deployment::BlueprintDatasetConfig;
 use nexus_types::deployment::BlueprintDatasetDisposition;
+use nexus_types::deployment::BlueprintDatasetFilter;
 use nexus_types::deployment::BlueprintDatasetsConfig;
 use omicron_common::api::external::ByteCount;
 use omicron_common::api::external::Generation;
@@ -19,12 +20,8 @@ use omicron_uuid_kinds::DatasetUuid;
 use omicron_uuid_kinds::ZpoolUuid;
 use std::collections::btree_map::Entry;
 use std::collections::BTreeMap;
-use std::collections::BTreeSet;
 use std::net::SocketAddrV6;
 
-#[cfg(test)]
-use nexus_types::deployment::BlueprintDatasetFilter;
-
 #[derive(Debug, thiserror::Error)]
 #[error(
     "invalid blueprint input: multiple datasets with kind {kind:?} \
@@ -125,11 +122,6 @@ pub(super) struct DatasetsEditor {
     // Cache of _in service only_ datasets, identified by (zpool, kind).
     in_service_by_zpool_and_kind:
         BTreeMap<ZpoolUuid, BTreeMap<DatasetKind, DatasetUuid>>,
-    // Cache of _expunged_ dataset IDs. This serves as a list of IDs from
-    // `preexisting_dataset_ids` to ignore, as we shouldn't reuse old IDs if
-    // they belong to expunged datasets. We should be able to remove this when
-    // we remove `preexisting_dataset_ids`.
-    expunged_datasets: BTreeSet<DatasetUuid>,
     counts: EditCounts,
 }
 
@@ -138,7 +130,6 @@ impl DatasetsEditor {
         config: BlueprintDatasetsConfig,
     ) -> Result<Self, MultipleDatasetsOfKind> {
         let mut in_service_by_zpool_and_kind = BTreeMap::new();
-        let mut expunged_datasets = BTreeSet::new();
         for dataset in config.datasets.iter() {
             match dataset.disposition {
                 BlueprintDatasetDisposition::InService => {
@@ -160,15 +151,12 @@ impl DatasetsEditor {
                         }
                     }
                 }
-                BlueprintDatasetDisposition::Expunged => {
-                    expunged_datasets.insert(dataset.id);
-                }
+                BlueprintDatasetDisposition::Expunged => (),
             }
         }
         Ok(Self {
             config,
             in_service_by_zpool_and_kind,
-            expunged_datasets,
             counts: EditCounts::zeroes(),
         })
     }
@@ -180,7 +168,6 @@ impl DatasetsEditor {
                 datasets: IdMap::new(),
             },
             in_service_by_zpool_and_kind: BTreeMap::new(),
-            expunged_datasets: BTreeSet::new(),
             counts: EditCounts::zeroes(),
         }
     }
@@ -197,7 +184,6 @@ impl DatasetsEditor {
         self.counts
     }
 
-    #[cfg(test)]
     pub fn datasets(
         &self,
         filter: BlueprintDatasetFilter,
@@ -210,7 +196,7 @@ impl DatasetsEditor {
 
     // Private method; panics if given an ID that isn't present in
     // `self.config.datasets`. Callers must ensure the ID is valid.
-    fn expunge_by_known_valid_id(&mut self, id: DatasetUuid) {
+    fn expunge_by_known_valid_id(&mut self, id: DatasetUuid) -> bool {
         let mut dataset = self
             .config
             .datasets
@@ -220,12 +206,13 @@ impl DatasetsEditor {
             BlueprintDatasetDisposition::InService => {
                 dataset.disposition = BlueprintDatasetDisposition::Expunged;
                 self.counts.expunged += 1;
+                true
             }
             BlueprintDatasetDisposition::Expunged => {
                 // already expunged; nothing to do
+                false
             }
         }
-        self.expunged_datasets.insert(dataset.id);
     }
 
     /// Expunge a dataset identified by its zpool + kind combo.
@@ -252,15 +239,20 @@ impl DatasetsEditor {
         Ok(())
     }
 
-    pub fn expunge_all_on_zpool(&mut self, zpool: &ZpoolUuid) {
+    pub fn expunge_all_on_zpool(&mut self, zpool: &ZpoolUuid) -> usize {
         let Some(by_kind) = self.in_service_by_zpool_and_kind.remove(zpool)
         else {
-            return;
+            return 0;
         };
 
+        let mut nexpunged = 0;
         for id in by_kind.into_values() {
-            self.expunge_by_known_valid_id(id);
+            if self.expunge_by_known_valid_id(id) {
+                nexpunged += 1;
+            }
         }
+
+        nexpunged
     }
 
     pub fn ensure_in_service(
@@ -340,7 +332,6 @@ impl DatasetsEditor {
 mod tests {
     use super::*;
     use crate::planner::PlannerRng;
-    use nexus_types::deployment::BlueprintDatasetFilter;
     use omicron_uuid_kinds::GenericUuid;
     use omicron_uuid_kinds::SledUuid;
     use proptest::prelude::*;

nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/datasets.rs

@@ -96,22 +96,25 @@ impl DisksEditor {
     pub fn expunge(
         &mut self,
         disk_id: &PhysicalDiskUuid,
-    ) -> Result<ZpoolUuid, DisksEditError> {
+    ) -> Result<(bool, ZpoolUuid), DisksEditError> {
         let config = self.disks.get_mut(disk_id).ok_or_else(|| {
             DisksEditError::ExpungeNonexistentDisk { id: *disk_id }
         })?;
 
+        let did_expunge: bool;
         match config.disposition {
             BlueprintPhysicalDiskDisposition::InService => {
                 config.disposition = BlueprintPhysicalDiskDisposition::Expunged;
                 self.counts.expunged += 1;
+                did_expunge = true;
             }
             BlueprintPhysicalDiskDisposition::Expunged => {
                 // expunge is idempotent; do nothing
+                did_expunge = false;
             }
         }
 
-        Ok(config.pool_id)
+        Ok((did_expunge, config.pool_id))
     }
 }
 

nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/disks.rs

@@ -155,7 +155,8 @@ impl ZonesEditor {
         }
     }
 
-    pub fn expunge_all_on_zpool(&mut self, zpool: &ZpoolUuid) {
+    pub fn expunge_all_on_zpool(&mut self, zpool: &ZpoolUuid) -> usize {
+        let mut nexpunged = 0;
         for mut config in self.zones.iter_mut() {
             // Expunge this zone if its filesystem or durable dataset are on
             // this zpool. (If it has both, they should be on the _same_ zpool,
@@ -170,9 +171,12 @@ impl ZonesEditor {
                 .durable_zpool()
                 .map_or(false, |pool| pool.id() == *zpool);
             if fs_is_on_zpool || dd_is_on_zpool {
-                Self::expunge_impl(&mut config, &mut self.counts);
+                if Self::expunge_impl(&mut config, &mut self.counts) {
+                    nexpunged += 1;
+                }
             }
         }
+        nexpunged
     }
 }