Skip to content

[reconfigurator] Rework planner/builder expungement logic #7495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 6, 2025
Merged

[reconfigurator] Rework planner/builder expungement logic #7495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ef6c5e
[reconfigurator] Rework planner/builder expungement logic
jgallagher Feb 6, 2025
7f4dc1f
cargo fmt
jgallagher Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
330 changes: 190 additions & 140 deletions nexus/reconfigurator/planning/src/blueprint_builder/builder.rs
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions nexus/reconfigurator/planning/src/blueprint_editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
68 changes: 58 additions & 10 deletions nexus/reconfigurator/planning/src/blueprint_editor/sled_editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")]
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
}

Expand All @@ -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)
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand All @@ -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() {
Expand All @@ -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
Expand Down
35 changes: 13 additions & 22 deletions nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/datasets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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:?} \
Expand Down Expand Up @@ -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,
}

Expand All @@ -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 => {
Expand All @@ -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(),
})
}
Expand All @@ -180,7 +168,6 @@ impl DatasetsEditor {
datasets: IdMap::new(),
},
in_service_by_zpool_and_kind: BTreeMap::new(),
expunged_datasets: BTreeSet::new(),
counts: EditCounts::zeroes(),
}
}
Expand All @@ -197,7 +184,6 @@ impl DatasetsEditor {
self.counts
}

#[cfg(test)]
pub fn datasets(
&self,
filter: BlueprintDatasetFilter,
Expand All @@ -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
Expand All @@ -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.
Expand All @@ -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(
Expand Down Expand Up @@ -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::*;
Expand Down
7 changes: 5 additions & 2 deletions nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/disks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}

Expand Down
8 changes: 6 additions & 2 deletions nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/zones.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
}
}

Expand Down
Loading
Loading