Skip to content

Commit 7be692d

Browse files
andrewjstoneandrewjstone
committed
Use expunged disposition fields rather than state
I still need to add the schema migration code and do some testing.
1 parent 1822a2b commit 7be692d

File tree

17 files changed

+269
-161
lines changed

17 files changed

+269
-161
lines changed

‎nexus/db-model/src/deployment.rs‎

Lines changed: 82 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ use crate::schema::{
1717
};
1818
use crate::typed_uuid::DbTypedUuid;
1919
use crate::{
20-
impl_enum_type, ipv6, ByteCount, Generation, MacAddr, Name,
21-
PhysicalDiskState, SledState, SqlU16, SqlU32, SqlU8,
20+
impl_enum_type, ipv6, ByteCount, Generation, MacAddr, Name, SledState,
21+
SqlU16, SqlU32, SqlU8,
2222
};
2323
use anyhow::{anyhow, bail, Context, Result};
2424
use chrono::{DateTime, Utc};
@@ -167,30 +167,66 @@ impl_enum_type!(
167167
Expunged => b"expunged"
168168
);
169169

170-
/// Converts a [`BlueprintPhysicalDiskDisposition`] to a version that can be inserted
171-
/// into a database.
172-
pub fn to_db_bp_physical_disk_disposition(
173-
disposition: BlueprintPhysicalDiskDisposition,
174-
) -> DbBpPhysicalDiskDisposition {
175-
match disposition {
176-
BlueprintPhysicalDiskDisposition::InService => {
177-
DbBpPhysicalDiskDisposition::InService
178-
}
179-
BlueprintPhysicalDiskDisposition::Expunged => {
180-
DbBpPhysicalDiskDisposition::Expunged
170+
struct DbBpPhysicalDiskDispositionColumns {
171+
disposition: DbBpPhysicalDiskDisposition,
172+
expunged_as_of_generation: Option<Generation>,
173+
expunged_ready_for_cleanup: bool,
174+
}
175+
176+
impl From<BlueprintPhysicalDiskDisposition>
177+
for DbBpPhysicalDiskDispositionColumns
178+
{
179+
fn from(value: BlueprintPhysicalDiskDisposition) -> Self {
180+
let (
181+
disposition,
182+
disposition_expunged_as_of_generation,
183+
disposition_expunged_ready_for_cleanup,
184+
) = match value {
185+
BlueprintPhysicalDiskDisposition::InService => {
186+
(DbBpPhysicalDiskDisposition::InService, None, false)
187+
}
188+
BlueprintPhysicalDiskDisposition::Expunged {
189+
as_of_generation,
190+
ready_for_cleanup,
191+
} => (
192+
DbBpPhysicalDiskDisposition::Expunged,
193+
Some(Generation(as_of_generation)),
194+
ready_for_cleanup,
195+
),
196+
};
197+
Self {
198+
disposition,
199+
expunged_as_of_generation: disposition_expunged_as_of_generation,
200+
expunged_ready_for_cleanup: disposition_expunged_ready_for_cleanup,
181201
}
182202
}
183203
}
184204

185-
impl From<DbBpPhysicalDiskDisposition> for BlueprintPhysicalDiskDisposition {
186-
fn from(disposition: DbBpPhysicalDiskDisposition) -> Self {
187-
match disposition {
188-
DbBpPhysicalDiskDisposition::InService => {
189-
BlueprintPhysicalDiskDisposition::InService
205+
impl TryFrom<DbBpPhysicalDiskDispositionColumns>
206+
for BlueprintPhysicalDiskDisposition
207+
{
208+
type Error = anyhow::Error;
209+
210+
fn try_from(
211+
value: DbBpPhysicalDiskDispositionColumns,
212+
) -> Result<Self, Self::Error> {
213+
match (value.disposition, value.expunged_as_of_generation) {
214+
(DbBpPhysicalDiskDisposition::InService, None) => {
215+
Ok(Self::InService)
190216
}
191-
DbBpPhysicalDiskDisposition::Expunged => {
192-
BlueprintPhysicalDiskDisposition::Expunged
217+
(DbBpPhysicalDiskDisposition::Expunged, Some(as_of_generation)) => {
218+
Ok(Self::Expunged {
219+
as_of_generation: *as_of_generation,
220+
ready_for_cleanup: value.expunged_ready_for_cleanup,
221+
})
193222
}
223+
(DbBpPhysicalDiskDisposition::InService, Some(_))
224+
| (DbBpPhysicalDiskDisposition::Expunged, None) => Err(anyhow!(
225+
"illegal database state (CHECK constraint broken?!): \
226+
disposition {:?}, disposition_expunged_as_of_generation {:?}",
227+
value.disposition,
228+
value.expunged_as_of_generation,
229+
)),
194230
}
195231
}
196232
}
@@ -232,8 +268,9 @@ pub struct BpOmicronPhysicalDisk {
232268
pub id: DbTypedUuid<PhysicalDiskKind>,
233269
pub pool_id: Uuid,
234270

235-
pub disposition: DbBpPhysicalDiskDisposition,
236-
pub state: PhysicalDiskState,
271+
disposition: DbBpPhysicalDiskDisposition,
272+
disposition_expunged_as_of_generation: Option<Generation>,
273+
disposition_expunged_ready_for_cleanup: bool,
237274
}
238275

239276
impl BpOmicronPhysicalDisk {
@@ -242,6 +279,11 @@ impl BpOmicronPhysicalDisk {
242279
sled_id: SledUuid,
243280
disk_config: &BlueprintPhysicalDiskConfig,
244281
) -> Self {
282+
let DbBpPhysicalDiskDispositionColumns {
283+
disposition,
284+
expunged_as_of_generation: disposition_expunged_as_of_generation,
285+
expunged_ready_for_cleanup: disposition_expunged_ready_for_cleanup,
286+
} = disk_config.disposition.into();
245287
Self {
246288
blueprint_id: blueprint_id.into(),
247289
sled_id: sled_id.into(),
@@ -250,27 +292,35 @@ impl BpOmicronPhysicalDisk {
250292
model: disk_config.identity.model.clone(),
251293
id: disk_config.id.into(),
252294
pool_id: disk_config.pool_id.into_untyped_uuid(),
253-
disposition: to_db_bp_physical_disk_disposition(
254-
disk_config.disposition,
255-
),
256-
state: disk_config.state.into(),
295+
disposition,
296+
disposition_expunged_as_of_generation,
297+
disposition_expunged_ready_for_cleanup,
257298
}
258299
}
259300
}
260301

261-
impl From<BpOmicronPhysicalDisk> for BlueprintPhysicalDiskConfig {
262-
fn from(disk: BpOmicronPhysicalDisk) -> Self {
263-
Self {
264-
disposition: disk.disposition.into(),
302+
impl TryFrom<BpOmicronPhysicalDisk> for BlueprintPhysicalDiskConfig {
303+
type Error = anyhow::Error;
304+
305+
fn try_from(disk: BpOmicronPhysicalDisk) -> Result<Self, Self::Error> {
306+
let disposition_cols = DbBpPhysicalDiskDispositionColumns {
307+
disposition: disk.disposition,
308+
expunged_as_of_generation: disk
309+
.disposition_expunged_as_of_generation,
310+
expunged_ready_for_cleanup: disk
311+
.disposition_expunged_ready_for_cleanup,
312+
};
313+
314+
Ok(Self {
315+
disposition: disposition_cols.try_into()?,
265316
identity: DiskIdentity {
266317
vendor: disk.vendor,
267318
serial: disk.serial,
268319
model: disk.model,
269320
},
270321
id: disk.id.into(),
271322
pool_id: ZpoolUuid::from_untyped_uuid(disk.pool_id),
272-
state: disk.state.into(),
273-
}
323+
})
274324
}
275325
}
276326

‎nexus/db-model/src/schema.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1686,7 +1686,8 @@ table! {
16861686
pool_id -> Uuid,
16871687

16881688
disposition -> crate::DbBpPhysicalDiskDispositionEnum,
1689-
state -> crate::PhysicalDiskStateEnum,
1689+
disposition_expunged_as_of_generation -> Nullable<Int8>,
1690+
disposition_expunged_ready_for_cleanup -> Bool,
16901691
}
16911692
}
16921693

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -839,7 +839,13 @@ impl DataStore {
839839
d.id, d.sled_id
840840
))
841841
})?;
842-
sled_disks.disks.insert(d.into());
842+
let disk_id = d.id;
843+
sled_disks.disks.insert(d.try_into().map_err(|e| {
844+
Error::internal_error(&format!(
845+
"Cannot convert BpOmicronPhysicalDisk {}: {e}",
846+
disk_id
847+
))
848+
})?);
843849
}
844850
}
845851
}

‎nexus/reconfigurator/blippy/src/checks.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,10 @@ use nexus_sled_agent_shared::inventory::ZoneKind;
1010
use nexus_types::deployment::blueprint_zone_type;
1111
use nexus_types::deployment::BlueprintDatasetConfig;
1212
use nexus_types::deployment::BlueprintDatasetFilter;
13+
use nexus_types::deployment::BlueprintPhysicalDiskDisposition;
1314
use nexus_types::deployment::BlueprintZoneConfig;
1415
use nexus_types::deployment::BlueprintZoneFilter;
1516
use nexus_types::deployment::BlueprintZoneType;
16-
use nexus_types::deployment::DiskFilter;
1717
use nexus_types::deployment::OmicronZoneExternalIp;
1818
use nexus_types::external_api::views::SledState;
1919
use omicron_common::address::DnsSubnet;
@@ -462,8 +462,9 @@ fn check_datasets(blippy: &mut Blippy<'_>) {
462462
let mut expected_datasets = BTreeSet::new();
463463

464464
// All disks should have debug and zone root datasets.
465-
for (sled_id, disk) in
466-
blippy.blueprint().all_omicron_disks(DiskFilter::InService)
465+
for (sled_id, disk) in blippy
466+
.blueprint()
467+
.all_omicron_disks(BlueprintPhysicalDiskDisposition::is_in_service)
467468
{
468469
// Note: This may be called multiple times per `sled_id`,
469470
// which is somewhat inefficient. However it will still only report

‎nexus/reconfigurator/execution/src/lib.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use nexus_db_queries::context::OpContext;
1212
use nexus_db_queries::db::DataStore;
1313
use nexus_types::deployment::execution::*;
1414
use nexus_types::deployment::Blueprint;
15+
use nexus_types::deployment::BlueprintPhysicalDiskDisposition;
1516
use nexus_types::deployment::BlueprintZoneFilter;
1617
use nexus_types::deployment::SledFilter;
1718
use nexus_types::external_api::views::SledState;
@@ -535,7 +536,7 @@ fn register_decommission_disks_step<'a>(
535536
&opctx,
536537
datastore,
537538
blueprint
538-
.all_decommisioned_disks()
539+
.all_omicron_disks(BlueprintPhysicalDiskDisposition::is_ready_for_cleanup)
539540
.map(|(sled_id, config)| (sled_id, config.id)),
540541
)
541542
.await

‎nexus/reconfigurator/execution/src/omicron_physical_disks.rs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,6 @@ mod test {
268268
generation: Generation::new().next(),
269269
disks: [BlueprintPhysicalDiskConfig {
270270
disposition: BlueprintPhysicalDiskDisposition::InService,
271-
state: nexus_types::external_api::views::PhysicalDiskState::Active,
272271
identity: DiskIdentity {
273272
vendor: "test-vendor".to_string(),
274273
serial: "test-serial".to_string(),

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

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@ use nexus_types::deployment::SledFilter;
4949
use nexus_types::deployment::SledResources;
5050
use nexus_types::deployment::ZpoolFilter;
5151
use nexus_types::deployment::ZpoolName;
52-
use nexus_types::external_api::views::PhysicalDiskState;
5352
use nexus_types::external_api::views::SledPolicy;
5453
use nexus_types::external_api::views::SledState;
5554
use nexus_types::inventory::Collection;
@@ -904,12 +903,12 @@ impl<'a> BlueprintBuilder<'a> {
904903
let mut num_zones_expunged = 0;
905904

906905
let mut disks_to_expunge = Vec::new();
907-
for disk in editor.disks(DiskFilter::All) {
906+
for disk in editor.disks(BlueprintPhysicalDiskDisposition::any) {
908907
match disk.disposition {
909908
BlueprintPhysicalDiskDisposition::InService => {
910909
disks_to_expunge.push(disk.id);
911910
}
912-
BlueprintPhysicalDiskDisposition::Expunged => (),
911+
BlueprintPhysicalDiskDisposition::Expunged { .. } => (),
913912
}
914913
}
915914
for disk_id in disks_to_expunge {
@@ -1081,7 +1080,6 @@ impl<'a> BlueprintBuilder<'a> {
10811080
BlueprintPhysicalDiskConfig {
10821081
disposition:
10831082
BlueprintPhysicalDiskDisposition::InService,
1084-
state: PhysicalDiskState::Active,
10851083
identity: disk.disk_identity.clone(),
10861084
id: disk_id,
10871085
pool_id: *zpool,
@@ -1161,21 +1159,16 @@ impl<'a> BlueprintBuilder<'a> {
11611159
.get(&sled_id)
11621160
.map(|sa| sa.omicron_physical_disks_generation);
11631161

1164-
// We can only check if the current generation has been seen.
1165-
// If a new generation has been seen then this blueprint is already
1166-
// out of date. If an old generation has been seen, the parent_blueprint
1167-
// has not been reflected in inventory.
1168-
//
1169-
// This means that many frequent expungements or additions will
1170-
// delay decommissioning, but we are limited by the total number of
1171-
// disks per sled that this will reasonably happen to in multiple
1172-
// passes.
1173-
if seen_generation == Some(parent_bp_config.generation) {
1174-
// Do we have any expunged disks in the parent blueprint?
1175-
for disk in &parent_bp_config.disks {
1176-
if disk.disposition
1177-
== BlueprintPhysicalDiskDisposition::Expunged
1178-
{
1162+
// Do we have any expunged disks in the parent blueprint?
1163+
for disk in &parent_bp_config.disks {
1164+
if let BlueprintPhysicalDiskDisposition::Expunged {
1165+
as_of_generation,
1166+
..
1167+
} = disk.disposition
1168+
{
1169+
// Has the sled_agent seen this disks expungement yet as
1170+
// reflected in inventory?
1171+
if seen_generation >= Some(as_of_generation) {
11791172
editor.decommission_disk(&disk.id).map_err(|err| {
11801173
Error::SledEditError { sled_id, err }
11811174
})?;
@@ -1899,7 +1892,7 @@ impl<'a> BlueprintBuilder<'a> {
18991892
// blueprint and the list of all in-service zpools on this sled per our
19001893
// planning input, and only pick zpools that are available in both.
19011894
let current_sled_disks = editor
1902-
.disks(DiskFilter::InService)
1895+
.disks(BlueprintPhysicalDiskDisposition::is_in_service)
19031896
.map(|disk_config| disk_config.pool_id)
19041897
.collect::<BTreeSet<_>>();
19051898

@@ -2485,7 +2478,7 @@ pub mod test {
24852478
.sled_editors
24862479
.get(&sled_id)
24872480
.unwrap()
2488-
.disks(DiskFilter::All)
2481+
.disks(BlueprintPhysicalDiskDisposition::any)
24892482
.collect::<Vec<_>>();
24902483
assert!(
24912484
disks.is_empty(),

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

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,13 @@ use nexus_types::deployment::BlueprintDatasetConfig;
1414
use nexus_types::deployment::BlueprintDatasetFilter;
1515
use nexus_types::deployment::BlueprintDatasetsConfig;
1616
use nexus_types::deployment::BlueprintPhysicalDiskConfig;
17+
use nexus_types::deployment::BlueprintPhysicalDiskDisposition;
1718
use nexus_types::deployment::BlueprintPhysicalDisksConfig;
1819
use nexus_types::deployment::BlueprintZoneConfig;
1920
use nexus_types::deployment::BlueprintZoneDisposition;
2021
use nexus_types::deployment::BlueprintZoneFilter;
2122
use nexus_types::deployment::BlueprintZoneType;
2223
use nexus_types::deployment::BlueprintZonesConfig;
23-
use nexus_types::deployment::DiskFilter;
2424
use nexus_types::external_api::views::SledState;
2525
use nexus_types::inventory::Dataset;
2626
use nexus_types::inventory::Zpool;
@@ -228,10 +228,13 @@ impl SledEditor {
228228
.ok_or(SledEditError::OutOfUnderlayIps)
229229
}
230230

231-
pub fn disks(
231+
pub fn disks<F>(
232232
&self,
233-
filter: DiskFilter,
234-
) -> impl Iterator<Item = &BlueprintPhysicalDiskConfig> {
233+
mut filter: F,
234+
) -> impl Iterator<Item = &BlueprintPhysicalDiskConfig>
235+
where
236+
F: FnMut(BlueprintPhysicalDiskDisposition) -> bool,
237+
{
235238
match &self.0 {
236239
InnerSledEditor::Active(editor) => {
237240
Either::Left(editor.disks(filter))
@@ -241,7 +244,7 @@ impl SledEditor {
241244
.disks
242245
.disks
243246
.iter()
244-
.filter(move |disk| disk.disposition.matches(filter)),
247+
.filter(move |disk| filter(disk.disposition)),
245248
),
246249
}
247250
}
@@ -465,10 +468,13 @@ impl ActiveSledEditor {
465468
self.underlay_ip_allocator.alloc()
466469
}
467470

468-
pub fn disks(
471+
pub fn disks<F>(
469472
&self,
470-
filter: DiskFilter,
471-
) -> impl Iterator<Item = &BlueprintPhysicalDiskConfig> {
473+
filter: F,
474+
) -> impl Iterator<Item = &BlueprintPhysicalDiskConfig>
475+
where
476+
F: FnMut(BlueprintPhysicalDiskDisposition) -> bool,
477+
{
472478
self.disks.disks(filter)
473479
}
474480

0 commit comments

Comments
 (0)