From 8d8786b904da626cb6ed02e1f54b19f7884529bb Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 10 Jan 2025 10:02:20 -0500 Subject: [PATCH 1/3] add is_current_target_blueprint() extension for specific blueprint_id columns --- nexus/db-model/src/deployment.rs | 71 +++++++++++++++++++++++- nexus/db-queries/src/db/datastore/vpc.rs | 30 +--------- 2 files changed, 72 insertions(+), 29 deletions(-) diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index a44b81e943..7ea52be80c 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -1087,7 +1087,7 @@ impl BpClickhouseServerZoneIdToNodeId { } } -mod diesel_util { +mod diesel_blueprint_zone_filter { use crate::{ schema::bp_omicron_zone::disposition, to_db_bp_zone_disposition, DbBpZoneDisposition, @@ -1146,4 +1146,71 @@ mod diesel_util { EqAny>; } -pub use diesel_util::ApplyBlueprintZoneFilterExt; +pub use diesel_blueprint_zone_filter::ApplyBlueprintZoneFilterExt; + +mod diesel_is_current_target_blueprint { + use crate::schema; + use crate::schema::bp_target; + use diesel::dsl::Desc; + use diesel::dsl::EqAny; + use diesel::dsl::Limit; + use diesel::dsl::OrderBy; + use diesel::dsl::Select; + use diesel::prelude::*; + use diesel::sql_types; + + // Writing the return type of diesel queries is not awesome. Build up the + // actual return type via a sequence of type aliases that correspond to each + // of the steps of the query implementation below. + type SelectBlueprintId = Select; + type OrderByVersion = OrderBy>; + type IsCurrentTargetBlueprintExpr = EqAny>; + + /// An extension trait to check that a `Uuid` expression is equal + /// to the current target blueprint. + /// + /// The typical use of this will be to search one of the various blueprint + /// tables, restricted to only rows from the current target blueprint. + /// + /// `diesel` does not know about our strongly-typed UUID wrappers, so + /// technically this trait could apply to any expression that evaluates to a + /// `sql_types::Uuid`. To restrict this to only blueprint IDs, we also + /// restrict this via a `Sealed` trait to only specific columns known to + /// hold blueprint IDs. This list may need to be expanded if we write new + /// blueprint ID expression sources or need to query against an expression + /// more complex than a column. + pub trait ExprIsCurrentTargetBlueprintExt: + Expression + Sized + Sealed + { + fn is_current_target_blueprint( + self, + ) -> IsCurrentTargetBlueprintExpr; + } + + impl ExprIsCurrentTargetBlueprintExt for E + where + E: Expression + Sized + Sealed, + { + fn is_current_target_blueprint( + self, + ) -> IsCurrentTargetBlueprintExpr { + use bp_target::dsl; + + self.eq_any( + dsl::bp_target + .select(dsl::blueprint_id) + .order_by(dsl::version.desc()) + .limit(1), + ) + } + } + + // The specific list of columns for which `ExprIsCurrentTargetBlueprintExt` + // exists. + pub trait Sealed {} + impl Sealed for schema::bp_omicron_dataset::blueprint_id {} + impl Sealed for schema::bp_omicron_physical_disk::blueprint_id {} + impl Sealed for schema::bp_omicron_zone::blueprint_id {} +} + +pub use diesel_is_current_target_blueprint::ExprIsCurrentTargetBlueprintExt; diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 4691bede83..d2321bb283 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -18,6 +18,7 @@ use crate::db::error::ErrorHandler; use crate::db::identity::Resource; use crate::db::model::ApplyBlueprintZoneFilterExt; use crate::db::model::ApplySledFilterExt; +use crate::db::model::ExprIsCurrentTargetBlueprintExt; use crate::db::model::IncompleteVpc; use crate::db::model::InstanceNetworkInterface; use crate::db::model::Name; @@ -745,15 +746,9 @@ impl DataStore { // Resolve each VNIC in the VPC to the Sled it's on, so we know which // Sleds to notify when firewall rules change. use db::schema::{ - bp_omicron_zone, bp_target, instance, instance_network_interface, + bp_omicron_zone, instance, instance_network_interface, service_network_interface, sled, vmm, }; - // Diesel requires us to use aliases in order to refer to the - // `bp_target` table twice in the same query. - let (bp_target1, bp_target2) = diesel::alias!( - db::schema::bp_target as bp_target1, - db::schema::bp_target as bp_target2 - ); let instance_query = instance_network_interface::table .inner_join(instance::table) @@ -772,27 +767,8 @@ impl DataStore { .inner_join(bp_omicron_zone::table.on( bp_omicron_zone::id.eq(service_network_interface::service_id), )) - .inner_join( - bp_target1.on(bp_omicron_zone::blueprint_id - .eq(bp_target1.field(bp_target::blueprint_id))), - ) .inner_join(sled::table.on(sled::id.eq(bp_omicron_zone::sled_id))) - .filter( - // This filters us down to the one current target blueprint (if - // it exists); i.e., the target with the maximal version. We - // could also check that the current target is `enabled`, but - // that could very easily be incorrect: if the current target - // or any of its blueprint ancestors were _ever_ enabled, it's - // possible the current target blueprint describes running - // services that were added after RSS and therefore wouldn't be - // seen in `rss_service_query`. - bp_target1.field(bp_target::version).eq_any( - bp_target2 - .select(bp_target2.field(bp_target::version)) - .order_by(bp_target2.field(bp_target::version).desc()) - .limit(1), - ), - ) + .filter(bp_omicron_zone::blueprint_id.is_current_target_blueprint()) // Filter out services that are expunged and shouldn't be resolved // here. .blueprint_zone_filter( From 885e64f1ac124669d9f5cca53a87dfd29b91e417 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 10 Jan 2025 10:06:31 -0500 Subject: [PATCH 2/3] add blueprint_dataset_filter() diesel extension trait --- nexus/db-model/src/deployment.rs | 62 ++++++++++++++++++++++++++++++++ nexus/types/src/deployment.rs | 7 ++++ 2 files changed, 69 insertions(+) diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index 7ea52be80c..76ae272796 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -1148,6 +1148,68 @@ mod diesel_blueprint_zone_filter { pub use diesel_blueprint_zone_filter::ApplyBlueprintZoneFilterExt; +mod diesel_blueprint_dataset_filter { + use crate::{ + schema::bp_omicron_dataset::disposition, to_db_bp_dataset_disposition, + DbBpDatasetDisposition, + }; + use diesel::{ + helper_types::EqAny, prelude::*, query_dsl::methods::FilterDsl, + }; + use nexus_types::deployment::{ + BlueprintDatasetDisposition, BlueprintDatasetFilter, + }; + + /// An extension trait to apply a [`BlueprintDatasetFilter`] to a Diesel + /// expression. + /// + /// This is applicable to any Diesel expression which includes the + /// `bp_omicron_dataset` table. + /// + /// This needs to live here, rather than in `nexus-db-queries`, because it + /// names the `DbBpDatasetDisposition` type which is private to this crate. + pub trait ApplyBlueprintDatasetFilterExt { + type Output; + + /// Applies a [`BlueprintDatasetFilter`] to a Diesel expression. + fn blueprint_dataset_filter( + self, + filter: BlueprintDatasetFilter, + ) -> Self::Output; + } + + impl ApplyBlueprintDatasetFilterExt for E + where + E: FilterDsl, + { + type Output = E::Output; + + fn blueprint_dataset_filter( + self, + filter: BlueprintDatasetFilter, + ) -> Self::Output { + // This is only boxed for ease of reference above. + let all_matching_dispositions: BoxedIterator< + DbBpDatasetDisposition, + > = Box::new( + BlueprintDatasetDisposition::all_matching(filter) + .map(to_db_bp_dataset_disposition), + ); + + FilterDsl::filter( + self, + disposition.eq_any(all_matching_dispositions), + ) + } + } + + type BoxedIterator = Box>; + type BlueprintDatasetFilterQuery = + EqAny>; +} + +pub use diesel_blueprint_dataset_filter::ApplyBlueprintDatasetFilterExt; + mod diesel_is_current_target_blueprint { use crate::schema; use crate::schema::bp_target; diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index 33b8e102c5..8fd03e6702 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -1012,6 +1012,13 @@ impl BlueprintDatasetDisposition { }, } } + + /// Returns all dataset dispositions that match the given filter. + pub fn all_matching( + filter: BlueprintDatasetFilter, + ) -> impl Iterator { + BlueprintDatasetDisposition::iter().filter(move |&d| d.matches(filter)) + } } /// Information about a dataset as recorded in a blueprint From 40ead1d401c54e6da7c1edfe5f19ca968926af8c Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 10 Jan 2025 11:28:28 -0500 Subject: [PATCH 3/3] support bundles: choose dataset from blueprint --- nexus/db-model/src/schema.rs | 1 + .../src/db/datastore/support_bundle.rs | 136 ++++++++++-------- 2 files changed, 77 insertions(+), 60 deletions(-) diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index a8e1141db6..147b84d158 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -2021,6 +2021,7 @@ allow_tables_to_appear_in_same_query!( allow_tables_to_appear_in_same_query!(hw_baseboard_id, inv_sled_agent,); allow_tables_to_appear_in_same_query!( + bp_omicron_dataset, bp_omicron_zone, bp_target, dataset, diff --git a/nexus/db-queries/src/db/datastore/support_bundle.rs b/nexus/db-queries/src/db/datastore/support_bundle.rs index 52f120d49a..0b39179a4a 100644 --- a/nexus/db-queries/src/db/datastore/support_bundle.rs +++ b/nexus/db-queries/src/db/datastore/support_bundle.rs @@ -10,7 +10,6 @@ use crate::context::OpContext; use crate::db; use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; -use crate::db::model::Dataset; use crate::db::model::DatasetKind; use crate::db::model::SupportBundle; use crate::db::model::SupportBundleState; @@ -20,7 +19,10 @@ use crate::transaction_retry::OptionalError; use async_bb8_diesel::AsyncRunQueryDsl; use diesel::prelude::*; use futures::FutureExt; -use nexus_types::identity::Asset; +use nexus_db_model::ApplyBlueprintDatasetFilterExt; +use nexus_db_model::BpOmicronDataset; +use nexus_db_model::ExprIsCurrentTargetBlueprintExt; +use nexus_types::deployment::BlueprintDatasetFilter; use omicron_common::api::external; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; @@ -31,7 +33,6 @@ use omicron_uuid_kinds::BlueprintUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::OmicronZoneUuid; use omicron_uuid_kinds::SupportBundleUuid; -use omicron_uuid_kinds::ZpoolUuid; use uuid::Uuid; const CANNOT_ALLOCATE_ERR_MSG: &'static str = @@ -93,21 +94,28 @@ impl DataStore { let err = err.clone(); async move { - use db::schema::dataset::dsl as dataset_dsl; + use db::schema::bp_omicron_dataset::dsl as dataset_dsl; use db::schema::support_bundle::dsl as support_bundle_dsl; - // Observe all "non-deleted, debug datasets". + // Observe all in-service `Debug` dataset in the current + // target blueprint. // // Return the first one we find that doesn't already // have a support bundle allocated to it. - let free_dataset = dataset_dsl::dataset - .filter(dataset_dsl::time_deleted.is_null()) + let free_dataset = dataset_dsl::bp_omicron_dataset + .filter( + db::schema::bp_omicron_dataset::blueprint_id + .is_current_target_blueprint(), + ) + .blueprint_dataset_filter( + BlueprintDatasetFilter::InService, + ) .filter(dataset_dsl::kind.eq(DatasetKind::Debug)) .left_join(support_bundle_dsl::support_bundle.on( dataset_dsl::id.eq(support_bundle_dsl::dataset_id), )) .filter(support_bundle_dsl::dataset_id.is_null()) - .select(Dataset::as_select()) + .select(BpOmicronDataset::as_select()) .first_async(&conn) .await .optional()?; @@ -129,8 +137,8 @@ impl DataStore { let bundle = SupportBundle::new( reason_for_creation, - ZpoolUuid::from_untyped_uuid(dataset.pool_id), - dataset.id(), + dataset.pool_id.into(), + dataset.id.into(), this_nexus_id, ); @@ -259,9 +267,7 @@ impl DataStore { // For this blueprint: The set of expunged debug datasets let invalid_datasets = blueprint - .all_omicron_datasets( - nexus_types::deployment::BlueprintDatasetFilter::Expunged, - ) + .all_omicron_datasets(BlueprintDatasetFilter::Expunged) .filter_map(|(_sled_id, dataset_config)| { if matches!( dataset_config.kind, @@ -486,6 +492,7 @@ mod test { use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::SledUuid; + use omicron_uuid_kinds::ZpoolUuid; use rand::Rng; // Pool/Dataset pairs, for debug datasets only. @@ -501,18 +508,6 @@ mod test { } impl TestSled { - fn new_with_pool_count(pool_count: usize) -> Self { - Self { - sled: SledUuid::new_v4(), - pools: (0..pool_count) - .map(|_| TestPool { - pool: ZpoolUuid::new_v4(), - dataset: DatasetUuid::new_v4(), - }) - .collect(), - } - } - fn new_from_blueprint(blueprint: &Blueprint) -> Vec { let mut sleds = vec![]; for (sled, datasets) in &blueprint.blueprint_datasets { @@ -579,17 +574,6 @@ mod test { .zpool_insert(opctx, zpool) .await .expect("failed to upsert zpool"); - - let dataset = Dataset::new( - pool.dataset, - pool.pool.into_untyped_uuid(), - None, - DebugDatasetKind, - ); - datastore - .dataset_upsert(dataset) - .await - .expect("failed to upsert dataset"); } } } @@ -601,8 +585,42 @@ mod test { opctx: &OpContext, pool_count: usize, ) -> TestSled { - let sled = TestSled::new_with_pool_count(pool_count); - sled.create_database_records(&datastore, &opctx).await; + let mut rng = SimRngState::from_seed(&format!( + "create_sled_and_zpools-{pool_count}" + )); + let (example, bp1) = ExampleSystemBuilder::new_with_rng( + &opctx.log, + rng.next_system_rng(), + ) + .nsleds(1) + .create_zones(false) + .ndisks_per_sled( + pool_count.try_into().expect("reasonable number of pools"), + ) + .build(); + + bp_insert_and_make_target(opctx, datastore, &example.initial_blueprint) + .await; + bp_insert_and_make_target(opctx, datastore, &bp1).await; + + // Flatten the blueprint down to just a `TestSled` with its Debug + // datasets. + assert_eq!(bp1.blueprint_datasets.len(), 1); + let (&sled, datasets_config) = bp1 + .blueprint_datasets + .first_key_value() + .expect("blueprint has 1 sled"); + + let pools = datasets_config + .datasets + .values() + .filter(|d| d.kind == DebugDatasetKind) + .map(|d| TestPool { pool: d.pool.id(), dataset: d.id }) + .collect::>(); + assert_eq!(pools.len(), pool_count); + + let sled = TestSled { sled, pools }; + sled.create_database_records(datastore, opctx).await; sled } @@ -973,22 +991,21 @@ mod test { let (opctx, datastore) = (db.opctx(), db.datastore()); let mut rng = SimRngState::from_seed(TEST_NAME); - let (_example, mut bp1) = ExampleSystemBuilder::new_with_rng( + let (example, bp1) = ExampleSystemBuilder::new_with_rng( &logctx.log, rng.next_system_rng(), ) .build(); - // Weirdly, the "ExampleSystemBuilder" blueprint has a parent blueprint, - // but which isn't exposed through the API. Since we're only able to see - // the blueprint it emits, that means we can't actually make it the - // target because "the parent blueprint is not the current target". - // - // Instead of dealing with that, we lie: claim this is the primordial - // blueprint, with no parent. - // - // Regardless, make this starter blueprint our target. - bp1.parent_blueprint_id = None; + // Insert both the primordial blueprint created by + // `ExampleSystemBuilder` and the useful blueprint (its child), making + // the latter the target for the rest of the test. + bp_insert_and_make_target( + &opctx, + &datastore, + &example.initial_blueprint, + ) + .await; bp_insert_and_make_target(&opctx, &datastore, &bp1).await; // Manually perform the equivalent of blueprint execution to populate @@ -1080,22 +1097,21 @@ mod test { let (opctx, datastore) = (db.opctx(), db.datastore()); let mut rng = SimRngState::from_seed(TEST_NAME); - let (_example, mut bp1) = ExampleSystemBuilder::new_with_rng( + let (example, bp1) = ExampleSystemBuilder::new_with_rng( &logctx.log, rng.next_system_rng(), ) .build(); - // Weirdly, the "ExampleSystemBuilder" blueprint has a parent blueprint, - // but which isn't exposed through the API. Since we're only able to see - // the blueprint it emits, that means we can't actually make it the - // target because "the parent blueprint is not the current target". - // - // Instead of dealing with that, we lie: claim this is the primordial - // blueprint, with no parent. - // - // Regardless, make this starter blueprint our target. - bp1.parent_blueprint_id = None; + // Insert both the primordial blueprint created by + // `ExampleSystemBuilder` and the useful blueprint (its child), making + // the latter the target for the rest of the test. + bp_insert_and_make_target( + &opctx, + &datastore, + &example.initial_blueprint, + ) + .await; bp_insert_and_make_target(&opctx, &datastore, &bp1).await; // Manually perform the equivalent of blueprint execution to populate