More omdb commands, physical disk filtering, safer expunge

- Adds an omdb command to see physical disks that are in-service,
not just in the inventory.
- Expands physical disk filtering to match parity with sled filtering
- Hard error on some disk expungement cases

dev-tools/omdb/src/bin/omdb/db.rs

@@ -62,6 +62,7 @@ use nexus_db_model::IpAttachState;
 use nexus_db_model::IpKind;
 use nexus_db_model::NetworkInterface;
 use nexus_db_model::NetworkInterfaceKind;
+use nexus_db_model::PhysicalDisk;
 use nexus_db_model::Probe;
 use nexus_db_model::Project;
 use nexus_db_model::Region;
@@ -96,7 +97,10 @@ use nexus_types::deployment::Blueprint;
 use nexus_types::deployment::BlueprintZoneDisposition;
 use nexus_types::deployment::BlueprintZoneFilter;
 use nexus_types::deployment::BlueprintZoneType;
+use nexus_types::deployment::DiskFilter;
 use nexus_types::deployment::SledFilter;
+use nexus_types::external_api::views::PhysicalDiskPolicy;
+use nexus_types::external_api::views::PhysicalDiskState;
 use nexus_types::external_api::views::SledPolicy;
 use nexus_types::external_api::views::SledState;
 use nexus_types::identity::Resource;
@@ -281,12 +285,14 @@ pub struct DbFetchOptions {
 enum DbCommands {
     /// Print information about the rack
     Rack(RackArgs),
-    /// Print information about disks
+    /// Print information about virtual disks
     Disks(DiskArgs),
     /// Print information about internal and external DNS
     Dns(DnsArgs),
     /// Print information about collected hardware/software inventory
     Inventory(InventoryArgs),
+    /// Print information about physical disks
+    PhysicalDisks(PhysicalDisksArgs),
     /// Save the current Reconfigurator inputs to a file
     ReconfiguratorSave(ReconfiguratorSaveArgs),
     /// Query for information about region replacements, optionally manually
@@ -406,7 +412,7 @@ enum InventoryCommands {
     /// list and show details from particular collections
     Collections(CollectionsArgs),
     /// show all physical disks every found
-    PhysicalDisks(PhysicalDisksArgs),
+    PhysicalDisks(InvPhysicalDisksArgs),
     /// list all root of trust pages ever found
     RotPages,
 }
@@ -435,14 +441,21 @@ struct CollectionsShowArgs {
 }
 
 #[derive(Debug, Args, Clone, Copy)]
-struct PhysicalDisksArgs {
+struct InvPhysicalDisksArgs {
     #[clap(long)]
     collection_id: Option<CollectionUuid>,
 
     #[clap(long, requires("collection_id"))]
     sled_id: Option<SledUuid>,
 }
 
+#[derive(Debug, Args)]
+struct PhysicalDisksArgs {
+    /// Show disks that match the given filter
+    #[clap(short = 'F', long, value_enum)]
+    filter: Option<DiskFilter>,
+}
+
 #[derive(Debug, Args)]
 struct ReconfiguratorSaveArgs {
     /// where to save the output
@@ -597,6 +610,15 @@ impl DbArgs {
                 )
                 .await
             }
+            DbCommands::PhysicalDisks(args) => {
+                cmd_db_physical_disks(
+                    &opctx,
+                    &datastore,
+                    &self.fetch_opts,
+                    args,
+                )
+                .await
+            }
             DbCommands::ReconfiguratorSave(reconfig_save_args) => {
                 cmd_db_reconfigurator_save(
                     &opctx,
@@ -1368,6 +1390,68 @@ async fn cmd_db_disk_physical(
     Ok(())
 }
 
+#[derive(Tabled)]
+#[tabled(rename_all = "SCREAMING_SNAKE_CASE")]
+struct PhysicalDiskRow {
+    id: Uuid,
+    serial: String,
+    vendor: String,
+    model: String,
+    sled_id: Uuid,
+    policy: PhysicalDiskPolicy,
+    state: PhysicalDiskState,
+}
+
+impl From<PhysicalDisk> for PhysicalDiskRow {
+    fn from(d: PhysicalDisk) -> Self {
+        PhysicalDiskRow {
+            id: d.id(),
+            serial: d.serial.clone(),
+            vendor: d.vendor.clone(),
+            model: d.model.clone(),
+            sled_id: d.sled_id,
+            policy: d.disk_policy.into(),
+            state: d.disk_state.into(),
+        }
+    }
+}
+
+/// Run `omdb db physical-disks`.
+async fn cmd_db_physical_disks(
+    opctx: &OpContext,
+    datastore: &DataStore,
+    fetch_opts: &DbFetchOptions,
+    args: &PhysicalDisksArgs,
+) -> Result<(), anyhow::Error> {
+    let limit = fetch_opts.fetch_limit;
+    let filter = match args.filter {
+        Some(filter) => filter,
+        None => {
+            eprintln!(
+                "note: listing all in-service disks \
+                 (use -F to filter, e.g. -F in-service)"
+            );
+            DiskFilter::InService
+        }
+    };
+
+    let sleds = datastore
+        .physical_disk_list(&opctx, &first_page(limit), filter)
+        .await
+        .context("listing sleds")?;
+    check_limit(&sleds, limit, || String::from("listing sleds"));
+
+    let rows = sleds.into_iter().map(|s| PhysicalDiskRow::from(s));
+    let table = tabled::Table::new(rows)
+        .with(tabled::settings::Style::empty())
+        .with(tabled::settings::Padding::new(1, 1, 0, 0))
+        .to_string();
+
+    println!("{}", table);
+
+    Ok(())
+}
+
 // SERVICES
 
 // Snapshots
@@ -3156,7 +3240,7 @@ async fn cmd_db_inventory_cabooses(
 async fn cmd_db_inventory_physical_disks(
     conn: &DataStoreConnection<'_>,
     limit: NonZeroU32,
-    args: PhysicalDisksArgs,
+    args: InvPhysicalDisksArgs,
 ) -> Result<(), anyhow::Error> {
     #[derive(Tabled)]
     #[tabled(rename_all = "SCREAMING_SNAKE_CASE")]

dev-tools/omdb/src/bin/omdb/nexus.rs

@@ -1656,27 +1656,33 @@ async fn cmd_nexus_sled_expunge_disk(
                     }
                 }
                 _ => {
+                    // This should be impossible due to a unique database index,
+                    // "vendor_serial_model_unique".
+                    //
+                    // Even if someone tried moving a disk, it would need to be
+                    // decommissioned before being re-commissioned elsewhere.
+                    //
+                    // However, we still print out an error message here in the
+                    // (unlikely) even that it happens anyway.
                     eprintln!(
-                        "WARNING: physical disk {} is PRESENT MULTIPLE TIMES in \
-                        the most recent inventory collection (spotted at {}). \
-                        It is dangerous to expunge a disk that is still \
-                        running. Are you sure you want to proceed anyway?",
+                        "ERROR: physical disk {} is PRESENT MULTIPLE TIMES in \
+                        the most recent inventory collection (spotted at {}).
+                        This should not be possible, and is an indication of a \
+                        database issue.",
                         args.physical_disk_id, collection.time_done,
                     );
-                    let confirm = read_with_prompt("y/N")?;
-                    if confirm != "y" {
-                        eprintln!("expungement not confirmed: aborting");
-                        return Ok(());
-                    }
+                    bail!("Physical Disk appeared on multiple sleds");
                 }
             }
         }
         None => {
             eprintln!(
-                "WARNING: cannot verify that the physical disk is physically gone \
+                "ERROR: cannot verify that the physical disk inventory status \
                  because there are no inventory collections present. Please \
-                 make sure that the physical disk has been physically removed."
+                 make sure that the physical disk has been physically removed, \
+                 or ensure that inventory may be collected."
             );
+            bail!("No inventory");
         }
     }
 

dev-tools/omdb/tests/usage_errors.out

@@ -103,9 +103,10 @@ Usage: omdb db [OPTIONS] <COMMAND>
 
 Commands:
   rack                 Print information about the rack
-  disks                Print information about disks
+  disks                Print information about virtual disks
   dns                  Print information about internal and external DNS
   inventory            Print information about collected hardware/software inventory
+  physical-disks       Print information about physical disks
   reconfigurator-save  Save the current Reconfigurator inputs to a file
   region-replacement   Query for information about region replacements, optionally manually
                            triggering one
@@ -143,9 +144,10 @@ Usage: omdb db [OPTIONS] <COMMAND>
 
 Commands:
   rack                 Print information about the rack
-  disks                Print information about disks
+  disks                Print information about virtual disks
   dns                  Print information about internal and external DNS
   inventory            Print information about collected hardware/software inventory
+  physical-disks       Print information about physical disks
   reconfigurator-save  Save the current Reconfigurator inputs to a file
   region-replacement   Query for information about region replacements, optionally manually
                            triggering one
@@ -181,7 +183,7 @@ termination: Exited(2)
 stdout:
 ---------------------------------------------
 stderr:
-Print information about disks
+Print information about virtual disks
 
 Usage: omdb db disks [OPTIONS] <COMMAND>
 

nexus/db-model/src/physical_disk.rs

@@ -85,3 +85,75 @@ impl DatastoreCollectionConfig<super::Zpool> for PhysicalDisk {
     type CollectionTimeDeletedColumn = physical_disk::dsl::time_deleted;
     type CollectionIdColumn = zpool::dsl::sled_id;
 }
+
+mod diesel_util {
+    use diesel::{
+        helper_types::{And, EqAny},
+        prelude::*,
+        query_dsl::methods::FilterDsl,
+    };
+    use nexus_types::{
+        deployment::DiskFilter,
+        external_api::views::{PhysicalDiskPolicy, PhysicalDiskState},
+    };
+
+    /// An extension trait to apply a [`DiskFilter`] to a Diesel expression.
+    ///
+    /// This is applicable to any Diesel expression which includes the `physical_disk`
+    /// table.
+    ///
+    /// This needs to live here, rather than in `nexus-db-queries`, because it
+    /// names the `DbPhysicalDiskPolicy` type which is private to this crate.
+    pub trait ApplyPhysicalDiskFilterExt {
+        type Output;
+
+        /// Applies a [`DiskFilter`] to a Diesel expression.
+        fn physical_disk_filter(self, filter: DiskFilter) -> Self::Output;
+    }
+
+    impl<E> ApplyPhysicalDiskFilterExt for E
+    where
+        E: FilterDsl<PhysicalDiskFilterQuery>,
+    {
+        type Output = E::Output;
+
+        fn physical_disk_filter(self, filter: DiskFilter) -> Self::Output {
+            use crate::schema::physical_disk::dsl as physical_disk_dsl;
+
+            // These are only boxed for ease of reference above.
+            let all_matching_policies: BoxedIterator<
+                crate::PhysicalDiskPolicy,
+            > = Box::new(
+                PhysicalDiskPolicy::all_matching(filter).map(Into::into),
+            );
+            let all_matching_states: BoxedIterator<crate::PhysicalDiskState> =
+                Box::new(
+                    PhysicalDiskState::all_matching(filter).map(Into::into),
+                );
+
+            FilterDsl::filter(
+                self,
+                physical_disk_dsl::disk_policy
+                    .eq_any(all_matching_policies)
+                    .and(
+                        physical_disk_dsl::disk_state
+                            .eq_any(all_matching_states),
+                    ),
+            )
+        }
+    }
+
+    type BoxedIterator<T> = Box<dyn Iterator<Item = T>>;
+    type PhysicalDiskFilterQuery = And<
+        EqAny<
+            crate::schema::physical_disk::disk_policy,
+            BoxedIterator<crate::PhysicalDiskPolicy>,
+        >,
+        EqAny<
+            crate::schema::physical_disk::disk_state,
+            BoxedIterator<crate::PhysicalDiskState>,
+        >,
+    >;
+}
+
+pub use diesel_util::ApplyPhysicalDiskFilterExt;

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

@@ -26,7 +26,8 @@ use crate::transaction_retry::OptionalError;
 use async_bb8_diesel::AsyncRunQueryDsl;
 use chrono::Utc;
 use diesel::prelude::*;
-use nexus_types::deployment::SledFilter;
+use nexus_db_model::ApplyPhysicalDiskFilterExt;
+use nexus_types::deployment::{DiskFilter, SledFilter};
 use omicron_common::api::external::CreateResult;
 use omicron_common::api::external::DataPageParams;
 use omicron_common::api::external::DeleteResult;
@@ -247,11 +248,13 @@ impl DataStore {
         &self,
         opctx: &OpContext,
         pagparams: &DataPageParams<'_, Uuid>,
+        disk_filter: DiskFilter,
     ) -> ListResultVec<PhysicalDisk> {
         opctx.authorize(authz::Action::Read, &authz::FLEET).await?;
         use db::schema::physical_disk::dsl;
         paginated(dsl::physical_disk, dsl::id, pagparams)
             .filter(dsl::time_deleted.is_null())
+            .physical_disk_filter(disk_filter)
             .select(PhysicalDisk::as_select())
             .load_async(&*self.pool_connection_authorized(opctx).await?)
             .await

nexus/src/app/sled.rs

@@ -13,6 +13,7 @@ use nexus_db_queries::context::OpContext;
 use nexus_db_queries::db;
 use nexus_db_queries::db::lookup;
 use nexus_db_queries::db::model::DatasetKind;
+use nexus_types::deployment::DiskFilter;
 use nexus_types::deployment::SledFilter;
 use nexus_types::external_api::views::PhysicalDiskPolicy;
 use nexus_types::external_api::views::SledPolicy;
@@ -212,7 +213,9 @@ impl super::Nexus {
         opctx: &OpContext,
         pagparams: &DataPageParams<'_, Uuid>,
     ) -> ListResultVec<db::model::PhysicalDisk> {
-        self.db_datastore.physical_disk_list(&opctx, pagparams).await
+        self.db_datastore
+            .physical_disk_list(&opctx, pagparams, DiskFilter::InService)
+            .await
     }
 
     /// Upserts a physical disk into the database, updating it if it already exists.