Expunge and Decommission disks in planner

nexus/db-model/src/deployment.rs

@@ -167,30 +167,66 @@ impl_enum_type!(
     Expunged => b"expunged"
 );
 
-/// Converts a [`BlueprintPhysicalDiskDisposition`] to a version that can be inserted
-/// into a database.
-pub fn to_db_bp_physical_disk_disposition(
-    disposition: BlueprintPhysicalDiskDisposition,
-) -> DbBpPhysicalDiskDisposition {
-    match disposition {
-        BlueprintPhysicalDiskDisposition::InService => {
-            DbBpPhysicalDiskDisposition::InService
-        }
-        BlueprintPhysicalDiskDisposition::Expunged => {
-            DbBpPhysicalDiskDisposition::Expunged
+struct DbBpPhysicalDiskDispositionColumns {
+    disposition: DbBpPhysicalDiskDisposition,
+    expunged_as_of_generation: Option<Generation>,
+    expunged_ready_for_cleanup: bool,
+}
+
+impl From<BlueprintPhysicalDiskDisposition>
+    for DbBpPhysicalDiskDispositionColumns
+{
+    fn from(value: BlueprintPhysicalDiskDisposition) -> Self {
+        let (
+            disposition,
+            disposition_expunged_as_of_generation,
+            disposition_expunged_ready_for_cleanup,
+        ) = match value {
+            BlueprintPhysicalDiskDisposition::InService => {
+                (DbBpPhysicalDiskDisposition::InService, None, false)
+            }
+            BlueprintPhysicalDiskDisposition::Expunged {
+                as_of_generation,
+                ready_for_cleanup,
+            } => (
+                DbBpPhysicalDiskDisposition::Expunged,
+                Some(Generation(as_of_generation)),
+                ready_for_cleanup,
+            ),
+        };
+        Self {
+            disposition,
+            expunged_as_of_generation: disposition_expunged_as_of_generation,
+            expunged_ready_for_cleanup: disposition_expunged_ready_for_cleanup,
         }
     }
 }
 
-impl From<DbBpPhysicalDiskDisposition> for BlueprintPhysicalDiskDisposition {
-    fn from(disposition: DbBpPhysicalDiskDisposition) -> Self {
-        match disposition {
-            DbBpPhysicalDiskDisposition::InService => {
-                BlueprintPhysicalDiskDisposition::InService
+impl TryFrom<DbBpPhysicalDiskDispositionColumns>
+    for BlueprintPhysicalDiskDisposition
+{
+    type Error = anyhow::Error;
+
+    fn try_from(
+        value: DbBpPhysicalDiskDispositionColumns,
+    ) -> Result<Self, Self::Error> {
+        match (value.disposition, value.expunged_as_of_generation) {
+            (DbBpPhysicalDiskDisposition::InService, None) => {
+                Ok(Self::InService)
             }
-            DbBpPhysicalDiskDisposition::Expunged => {
-                BlueprintPhysicalDiskDisposition::Expunged
+            (DbBpPhysicalDiskDisposition::Expunged, Some(as_of_generation)) => {
+                Ok(Self::Expunged {
+                    as_of_generation: *as_of_generation,
+                    ready_for_cleanup: value.expunged_ready_for_cleanup,
+                })
             }
+            (DbBpPhysicalDiskDisposition::InService, Some(_))
+            | (DbBpPhysicalDiskDisposition::Expunged, None) => Err(anyhow!(
+                "illegal database state (CHECK constraint broken?!): \
+                 disposition {:?}, disposition_expunged_as_of_generation {:?}",
+                value.disposition,
+                value.expunged_as_of_generation,
+            )),
         }
     }
 }
@@ -232,7 +268,9 @@ pub struct BpOmicronPhysicalDisk {
     pub id: DbTypedUuid<PhysicalDiskKind>,
     pub pool_id: Uuid,
 
-    pub disposition: DbBpPhysicalDiskDisposition,
+    disposition: DbBpPhysicalDiskDisposition,
+    disposition_expunged_as_of_generation: Option<Generation>,
+    disposition_expunged_ready_for_cleanup: bool,
 }
 
 impl BpOmicronPhysicalDisk {
@@ -241,6 +279,11 @@ impl BpOmicronPhysicalDisk {
         sled_id: SledUuid,
         disk_config: &BlueprintPhysicalDiskConfig,
     ) -> Self {
+        let DbBpPhysicalDiskDispositionColumns {
+            disposition,
+            expunged_as_of_generation: disposition_expunged_as_of_generation,
+            expunged_ready_for_cleanup: disposition_expunged_ready_for_cleanup,
+        } = disk_config.disposition.into();
         Self {
             blueprint_id: blueprint_id.into(),
             sled_id: sled_id.into(),
@@ -249,25 +292,35 @@ impl BpOmicronPhysicalDisk {
             model: disk_config.identity.model.clone(),
             id: disk_config.id.into(),
             pool_id: disk_config.pool_id.into_untyped_uuid(),
-            disposition: to_db_bp_physical_disk_disposition(
-                disk_config.disposition,
-            ),
+            disposition,
+            disposition_expunged_as_of_generation,
+            disposition_expunged_ready_for_cleanup,
         }
     }
 }
 
-impl From<BpOmicronPhysicalDisk> for BlueprintPhysicalDiskConfig {
-    fn from(disk: BpOmicronPhysicalDisk) -> Self {
-        Self {
-            disposition: disk.disposition.into(),
+impl TryFrom<BpOmicronPhysicalDisk> for BlueprintPhysicalDiskConfig {
+    type Error = anyhow::Error;
+
+    fn try_from(disk: BpOmicronPhysicalDisk) -> Result<Self, Self::Error> {
+        let disposition_cols = DbBpPhysicalDiskDispositionColumns {
+            disposition: disk.disposition,
+            expunged_as_of_generation: disk
+                .disposition_expunged_as_of_generation,
+            expunged_ready_for_cleanup: disk
+                .disposition_expunged_ready_for_cleanup,
+        };
+
+        Ok(Self {
+            disposition: disposition_cols.try_into()?,
             identity: DiskIdentity {
                 vendor: disk.vendor,
                 serial: disk.serial,
                 model: disk.model,
             },
             id: disk.id.into(),
             pool_id: ZpoolUuid::from_untyped_uuid(disk.pool_id),
-        }
+        })
     }
 }
 

nexus/db-model/src/schema.rs

@@ -1686,6 +1686,8 @@ table! {
         pool_id -> Uuid,
 
         disposition -> crate::DbBpPhysicalDiskDispositionEnum,
+        disposition_expunged_as_of_generation -> Nullable<Int8>,
+        disposition_expunged_ready_for_cleanup -> Bool,
     }
 }
 

nexus/db-model/src/schema_versions.rs

@@ -17,7 +17,7 @@ use std::collections::BTreeMap;
 ///
 /// This must be updated when you change the database schema.  Refer to
 /// schema/crdb/README.adoc in the root of this repository for details.
-pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(125, 0, 0);
+pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(126, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///
@@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy<Vec<KnownVersion>> = Lazy::new(|| {
         // |  leaving the first copy as an example for the next person.
         // v
         // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
+        KnownVersion::new(126, "bp-disk-disposition-expunged-cleanup"),
         KnownVersion::new(125, "blueprint-disposition-expunged-cleanup"),
         KnownVersion::new(124, "support-read-only-region-replacement"),
         KnownVersion::new(123, "vpc-subnet-contention"),

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

@@ -838,7 +838,13 @@ impl DataStore {
                                 d.id, d.sled_id
                             ))
                         })?;
-                    sled_disks.disks.insert(d.into());
+                    let disk_id = d.id;
+                    sled_disks.disks.insert(d.try_into().map_err(|e| {
+                        Error::internal_error(&format!(
+                            "Cannot convert BpOmicronPhysicalDisk {}: {e}",
+                            disk_id
+                        ))
+                    })?);
                 }
             }
         }
@@ -2344,12 +2350,12 @@ mod tests {
         assert_eq!(
             EnsureMultiple::from(
                 builder
-                    .sled_ensure_disks(
+                    .sled_add_disks(
                         new_sled_id,
                         &planning_input
                             .sled_lookup(SledFilter::Commissioned, new_sled_id)
                             .unwrap()
-                            .resources,
+                            .resources
                     )
                     .unwrap()
                     .disks

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

@@ -154,12 +154,13 @@ impl DataStore {
     ) -> Result<(), Error> {
         opctx.authorize(authz::Action::Read, &authz::FLEET).await?;
         use db::schema::physical_disk::dsl;
+        let now = Utc::now();
 
         diesel::update(
             dsl::physical_disk.filter(dsl::id.eq(to_db_typed_uuid(id))),
         )
         .filter(dsl::time_deleted.is_null())
-        .set(dsl::disk_policy.eq(policy))
+        .set((dsl::disk_policy.eq(policy), dsl::time_modified.eq(now)))
         .execute_async(&*self.pool_connection_authorized(&opctx).await?)
         .await
         .map_err(|err| public_error_from_diesel(err, ErrorHandler::Server))?;
@@ -174,12 +175,13 @@ impl DataStore {
     ) -> Result<(), Error> {
         opctx.authorize(authz::Action::Read, &authz::FLEET).await?;
         use db::schema::physical_disk::dsl;
+        let now = Utc::now();
 
         diesel::update(
             dsl::physical_disk.filter(dsl::id.eq(to_db_typed_uuid(id))),
         )
         .filter(dsl::time_deleted.is_null())
-        .set(dsl::disk_state.eq(state))
+        .set((dsl::disk_state.eq(state), dsl::time_modified.eq(now)))
         .execute_async(&*self.pool_connection_authorized(&opctx).await?)
         .await
         .map_err(|err| public_error_from_diesel(err, ErrorHandler::Server))?;
@@ -281,19 +283,27 @@ impl DataStore {
             .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
     }
 
-    /// Decommissions all expunged disks.
-    pub async fn physical_disk_decommission_all_expunged(
+    /// Decommissions a single expunged disk.
+    ///
+    /// This is a no-op if the disk is already decommissioned.
+    pub async fn physical_disk_decommission(
         &self,
         opctx: &OpContext,
+        id: PhysicalDiskUuid,
     ) -> Result<(), Error> {
         opctx.authorize(authz::Action::Modify, &authz::FLEET).await?;
         use db::schema::physical_disk::dsl;
-
+        let now = Utc::now();
         let conn = &*self.pool_connection_authorized(&opctx).await?;
         diesel::update(dsl::physical_disk)
+            .filter(dsl::id.eq(to_db_typed_uuid(id)))
             .filter(dsl::time_deleted.is_null())
-            .physical_disk_filter(DiskFilter::ExpungedButActive)
-            .set(dsl::disk_state.eq(PhysicalDiskState::Decommissioned))
+            .filter(dsl::disk_policy.eq(PhysicalDiskPolicy::Expunged))
+            .filter(dsl::disk_state.ne(PhysicalDiskState::Decommissioned))
+            .set((
+                dsl::disk_state.eq(PhysicalDiskState::Decommissioned),
+                dsl::time_modified.eq(now),
+            ))
             .execute_async(conn)
             .await
             .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;

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

@@ -3234,7 +3234,7 @@ mod tests {
             .expect("created blueprint builder");
             for &sled_id in &sled_ids {
                 builder
-                    .sled_ensure_disks(
+                    .sled_add_disks(
                         sled_id,
                         &planning_input
                             .sled_lookup(SledFilter::InService, sled_id)

nexus/reconfigurator/blippy/src/checks.rs

@@ -10,6 +10,7 @@ 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::BlueprintPhysicalDiskDisposition;
 use nexus_types::deployment::BlueprintZoneConfig;
 use nexus_types::deployment::BlueprintZoneDisposition;
 use nexus_types::deployment::BlueprintZoneType;
@@ -461,11 +462,13 @@ fn check_datasets(blippy: &mut Blippy<'_>) {
     let mut expected_datasets = BTreeSet::new();
 
     // All disks should have debug and zone root datasets.
-    //
-    // TODO-correctness We currently only include in-service disks in the
-    // blueprint; once we include expunged or decommissioned disks too, we
-    // should filter here to only in-service.
-    for (&sled_id, disk_config) in &blippy.blueprint().blueprint_disks {
+    for (sled_id, disk) in blippy
+        .blueprint()
+        .all_omicron_disks(BlueprintPhysicalDiskDisposition::is_in_service)
+    {
+        // Note: This may be called multiple times per `sled_id`,
+        // which is somewhat inefficient. However it will still only report
+        // one error note per `sled_id`.
         let Some(sled_datasets) = datasets.get_sled_or_note_missing(
             blippy,
             sled_id,
@@ -474,41 +477,37 @@ fn check_datasets(blippy: &mut Blippy<'_>) {
             continue;
         };
 
-        for disk in &disk_config.disks {
-            let sled_datasets = sled_datasets.get(&disk.pool_id);
+        let sled_datasets = sled_datasets.get(&disk.pool_id);
 
-            match sled_datasets
-                .and_then(|by_zpool| by_zpool.get(&DatasetKind::Debug))
-            {
-                Some(dataset) => {
-                    expected_datasets.insert(dataset.id);
-                }
-                None => {
-                    blippy.push_sled_note(
-                        sled_id,
-                        Severity::Fatal,
-                        SledKind::ZpoolMissingDebugDataset {
-                            zpool: disk.pool_id,
-                        },
-                    );
-                }
+        match sled_datasets
+            .and_then(|by_zpool| by_zpool.get(&DatasetKind::Debug))
+        {
+            Some(dataset) => {
+                expected_datasets.insert(dataset.id);
+            }
+            None => {
+                blippy.push_sled_note(
+                    sled_id,
+                    Severity::Fatal,
+                    SledKind::ZpoolMissingDebugDataset { zpool: disk.pool_id },
+                );
             }
+        }
 
-            match sled_datasets.and_then(|by_zpool| {
-                by_zpool.get(&DatasetKind::TransientZoneRoot)
-            }) {
-                Some(dataset) => {
-                    expected_datasets.insert(dataset.id);
-                }
-                None => {
-                    blippy.push_sled_note(
-                        sled_id,
-                        Severity::Fatal,
-                        SledKind::ZpoolMissingZoneRootDataset {
-                            zpool: disk.pool_id,
-                        },
-                    );
-                }
+        match sled_datasets
+            .and_then(|by_zpool| by_zpool.get(&DatasetKind::TransientZoneRoot))
+        {
+            Some(dataset) => {
+                expected_datasets.insert(dataset.id);
+            }
+            None => {
+                blippy.push_sled_note(
+                    sled_id,
+                    Severity::Fatal,
+                    SledKind::ZpoolMissingZoneRootDataset {
+                        zpool: disk.pool_id,
+                    },
+                );
             }
         }
     }