From 05a9f38a8569f0742ba24807a342a45e00f79428 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 3 Jul 2024 15:59:37 -0700 Subject: [PATCH] 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 | 92 ++++++++++++++++++- dev-tools/omdb/src/bin/omdb/nexus.rs | 28 +++--- dev-tools/omdb/tests/usage_errors.out | 8 +- nexus/db-model/src/physical_disk.rs | 72 +++++++++++++++ .../src/db/datastore/physical_disk.rs | 5 +- nexus/src/app/sled.rs | 5 +- nexus/types/src/deployment/planning_input.rs | 52 ++++++++++- 7 files changed, 241 insertions(+), 21 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index 7df457b247..aa0bcf3860 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/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,7 +441,7 @@ struct CollectionsShowArgs { } #[derive(Debug, Args, Clone, Copy)] -struct PhysicalDisksArgs { +struct InvPhysicalDisksArgs { #[clap(long)] collection_id: Option, @@ -443,6 +449,13 @@ struct PhysicalDisksArgs { sled_id: Option, } +#[derive(Debug, Args)] +struct PhysicalDisksArgs { + /// Show disks that match the given filter + #[clap(short = 'F', long, value_enum)] + filter: Option, +} + #[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 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")] diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index ac1a83e75c..d3754cf7f1 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/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"); } } diff --git a/dev-tools/omdb/tests/usage_errors.out b/dev-tools/omdb/tests/usage_errors.out index ebf6c25531..1340b508ca 100644 --- a/dev-tools/omdb/tests/usage_errors.out +++ b/dev-tools/omdb/tests/usage_errors.out @@ -103,9 +103,10 @@ Usage: omdb db [OPTIONS] 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] 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] diff --git a/nexus/db-model/src/physical_disk.rs b/nexus/db-model/src/physical_disk.rs index c6ef97ee1f..d4a1dcd33c 100644 --- a/nexus/db-model/src/physical_disk.rs +++ b/nexus/db-model/src/physical_disk.rs @@ -85,3 +85,75 @@ impl DatastoreCollectionConfig 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 ApplyPhysicalDiskFilterExt for E + where + E: FilterDsl, + { + 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 = + 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 = Box>; + type PhysicalDiskFilterQuery = And< + EqAny< + crate::schema::physical_disk::disk_policy, + BoxedIterator, + >, + EqAny< + crate::schema::physical_disk::disk_state, + BoxedIterator, + >, + >; +} + +pub use diesel_util::ApplyPhysicalDiskFilterExt; diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs index e51d59075e..11e056d19b 100644 --- a/nexus/db-queries/src/db/datastore/physical_disk.rs +++ b/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 { 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 diff --git a/nexus/src/app/sled.rs b/nexus/src/app/sled.rs index 24d5ff22a3..0165b2d261 100644 --- a/nexus/src/app/sled.rs +++ b/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 { - 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. diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index 3e8bc9aa2c..c6ece3b08a 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -340,7 +340,7 @@ impl SledDisk { } /// Filters that apply to disks. -#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, ValueEnum)] pub enum DiskFilter { /// All disks All, @@ -367,6 +367,56 @@ impl DiskFilter { } } +impl PhysicalDiskPolicy { + /// Returns true if self matches the filter + pub fn matches(self, filter: DiskFilter) -> bool { + match self { + PhysicalDiskPolicy::InService => match filter { + DiskFilter::All => true, + DiskFilter::InService => true, + }, + PhysicalDiskPolicy::Expunged => match filter { + DiskFilter::All => true, + DiskFilter::InService => false, + }, + } + } + + /// Returns all policies matching the given filter. + /// + /// This is meant for database access, and is generally paired with + /// [`PhysicalDiskState::all_matching`]. See `ApplyPhysicalDiskFilterExt` in + /// nexus-db-model. + pub fn all_matching(filter: DiskFilter) -> impl Iterator { + Self::iter().filter(move |state| state.matches(filter)) + } +} + +impl PhysicalDiskState { + /// Returns true if self matches the filter + pub fn matches(self, filter: DiskFilter) -> bool { + match self { + PhysicalDiskState::Active => match filter { + DiskFilter::All => true, + DiskFilter::InService => true, + }, + PhysicalDiskState::Decommissioned => match filter { + DiskFilter::All => true, + DiskFilter::InService => false, + }, + } + } + + /// Returns all state matching the given filter. + /// + /// This is meant for database access, and is generally paired with + /// [`PhysicalDiskPolicy::all_matching`]. See `ApplyPhysicalDiskFilterExt` in + /// nexus-db-model. + pub fn all_matching(filter: DiskFilter) -> impl Iterator { + Self::iter().filter(move |state| state.matches(filter)) + } +} + /// Filters that apply to zpools. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub enum ZpoolFilter {