diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 5c5481d49dd..759245082c2 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -310,9 +310,6 @@ impl DataStore { /// 3. Save the new allocation, if there isn't one for the given /// `hw_baseboard_id` /// 4. Return the new allocation - /// - // TODO: This could all actually be done in SQL using a `next_item` query. - // See https://github.com/oxidecomputer/omicron/issues/4544 pub async fn allocate_sled_underlay_subnet_octets( &self, opctx: &OpContext, diff --git a/nexus/db-queries/src/db/datastore/trust_quorum.rs b/nexus/db-queries/src/db/datastore/trust_quorum.rs index 96a5f151e57..357fad7342f 100644 --- a/nexus/db-queries/src/db/datastore/trust_quorum.rs +++ b/nexus/db-queries/src/db/datastore/trust_quorum.rs @@ -10,6 +10,7 @@ use super::DataStore; use crate::authz; use crate::context::OpContext; +use crate::db::model::to_db_sled_policy; use crate::db::pagination::paginated; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; @@ -25,6 +26,7 @@ use nexus_db_model::DbTypedUuid; use nexus_db_model::HwBaseboardId; use nexus_db_model::TrustQuorumConfiguration as DbTrustQuorumConfiguration; use nexus_db_model::TrustQuorumMember as DbTrustQuorumMember; +use nexus_types::external_api::sled::SledPolicy; use nexus_types::trust_quorum::IsLrtqUpgrade; use nexus_types::trust_quorum::ProposedTrustQuorumConfig; use nexus_types::trust_quorum::{ @@ -36,6 +38,7 @@ use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::OptionalLookupResult; use omicron_common::bail_unless; +use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::RackKind; use omicron_uuid_kinds::RackUuid; use rand::rng; @@ -208,6 +211,25 @@ impl DataStore { .map_err(|err| err.into_public_ignore_retries()) } + /// Return the `BaseboardId` of every member that is `Committed` in the + /// latest configuration for this rack but has no corresponding sled on this + /// rack that is both present and not expunged. + /// + /// Returns an empty set if the rack has no trust quorum configuration. + pub async fn tq_get_committed_members_without_active_sled( + &self, + opctx: &OpContext, + authz_tq: authz::TrustQuorumConfig, + ) -> Result, Error> { + opctx.authorize(authz::Action::Read, &authz_tq).await?; + let conn = &*self.pool_connection_authorized(opctx).await?; + let rack_id = authz_tq.rack().id(); + + Self::tq_get_committed_members_without_active_sled_conn(conn, rack_id) + .await + .map_err(|err| err.into_public_ignore_retries()) + } + async fn tq_get_latest_config_with_members_conn( conn: &async_bb8_diesel::Connection, rack_id: RackUuid, @@ -1449,18 +1471,72 @@ impl DataStore { Ok(members) } + + async fn tq_get_committed_members_without_active_sled_conn( + conn: &async_bb8_diesel::Connection, + rack_id: RackUuid, + ) -> Result, TransactionError> { + use nexus_db_schema::schema::hw_baseboard_id::dsl as hw_dsl; + use nexus_db_schema::schema::sled::dsl as sled_dsl; + use nexus_db_schema::schema::trust_quorum_member::dsl; + + let Some(latest) = + Self::tq_get_latest_config_conn(conn, rack_id).await? + else { + return Ok(BTreeSet::new()); + }; + + let rows: Vec<(String, String)> = dsl::trust_quorum_member + .filter(dsl::rack_id.eq(DbTypedUuid::::from(rack_id))) + .filter(dsl::epoch.eq(latest.epoch)) + .filter(dsl::state.eq(DbTrustQuorumMemberState::Committed)) + .inner_join( + hw_dsl::hw_baseboard_id.on(hw_dsl::id.eq(dsl::hw_baseboard_id)), + ) + .filter(diesel::dsl::not(diesel::dsl::exists( + sled_dsl::sled + .filter(sled_dsl::part_number.eq(hw_dsl::part_number)) + .filter(sled_dsl::serial_number.eq(hw_dsl::serial_number)) + .filter(sled_dsl::rack_id.eq(rack_id.into_untyped_uuid())) + .filter(sled_dsl::time_deleted.is_null()) + .filter( + sled_dsl::sled_policy + .ne(to_db_sled_policy(SledPolicy::Expunged)), + ), + ))) + .select((hw_dsl::part_number, hw_dsl::serial_number)) + .load_async(conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + Ok(rows + .into_iter() + .map(|(part_number, serial_number)| BaseboardId { + part_number, + serial_number, + }) + .collect()) + } } #[cfg(test)] mod tests { use super::*; use crate::db::pub_test_utils::TestDatabase; + use crate::db::pub_test_utils::helpers::SledSystemHardwareBuilder; + use nexus_db_model::Generation; use nexus_db_model::HwBaseboardId; + use nexus_db_model::SledBaseboard; + use nexus_db_model::SledState; + use nexus_db_model::SledUpdate; + use nexus_types::external_api::sled::SledProvisionPolicy; use nexus_types::trust_quorum::{ IsLrtqUpgrade, TrustQuorumConfigState, TrustQuorumMemberState, }; use omicron_test_utils::dev::test_setup_log; use omicron_uuid_kinds::RackUuid; + use omicron_uuid_kinds::SledUuid; + use std::net::{Ipv6Addr, SocketAddrV6}; use uuid::Uuid; fn make_authz_tq(rack_id: RackUuid) -> authz::TrustQuorumConfig { @@ -1496,6 +1572,33 @@ mod tests { hw_baseboard_ids } + /// Insert a sled whose baseboard matches `hw`. + /// + /// `SledUpdateBuilder` always uses `sled_baseboard_for_test()`, so build the + /// `SledUpdate` directly to control the part number and serial. + async fn insert_sled_for_baseboard( + datastore: &DataStore, + rack_id: RackUuid, + hw: &HwBaseboardId, + ) -> SledUuid { + let sled_id = SledUuid::new_v4(); + let update = SledUpdate::new( + sled_id, + SocketAddrV6::new(Ipv6Addr::LOCALHOST, 0, 0, 0), + 0, + SledBaseboard { + serial_number: hw.serial_number.clone(), + part_number: hw.part_number.clone(), + revision: 1, + }, + SledSystemHardwareBuilder::new().build(), + rack_id, + Generation::new(), + ); + datastore.sled_upsert(update).await.unwrap(); + sled_id + } + #[tokio::test] async fn test_tq_insert_latest_errors() { let logctx = test_setup_log("test_tq_insert_latest_errors"); @@ -2348,4 +2451,132 @@ mod tests { db.terminate().await; logctx.cleanup_successful(); } + + #[tokio::test] + async fn test_tq_get_committed_members_without_active_sled() { + let logctx = + test_setup_log("test_tq_get_committed_members_without_active_sled"); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + let conn = datastore.pool_connection_for_tests().await.unwrap(); + + use nexus_db_schema::schema::sled::dsl as sled_dsl; + + let hw_ids = insert_hw_baseboard_ids(&db).await; + let rack_id = RackUuid::new_v4(); + let members: BTreeSet<_> = + hw_ids.iter().cloned().map(BaseboardId::from).collect(); + let coordinator = members.first().unwrap().clone(); + + let missing_members = || async { + datastore + .tq_get_committed_members_without_active_sled( + opctx, + make_authz_tq(rack_id), + ) + .await + .unwrap() + }; + + // A rack with no configuration at all reports nothing. + assert!(missing_members().await.is_empty()); + + // This inserts a configuration in which every member is `Committed`. + DataStore::tq_insert_rss_config_after_handoff( + opctx, + &conn, + make_authz_tq(rack_id), + members.clone(), + coordinator, + ) + .await + .unwrap(); + + // No sleds exist yet, so every committed member is missing one. + assert_eq!(missing_members().await, members); + + // Give the first 8 baseboards a sled. + let mut sled_ids = Vec::new(); + for hw in &hw_ids[..8] { + sled_ids + .push(insert_sled_for_baseboard(datastore, rack_id, hw).await); + } + + let mut expected: BTreeSet<_> = + hw_ids[8..].iter().cloned().map(BaseboardId::from).collect(); + assert_eq!(missing_members().await, expected); + + // Expunging a sled makes its member missing again. + diesel::update(sled_dsl::sled) + .filter(sled_dsl::id.eq(sled_ids[0].into_untyped_uuid())) + .set( + sled_dsl::sled_policy + .eq(to_db_sled_policy(SledPolicy::Expunged)), + ) + .execute_async(&*conn) + .await + .unwrap(); + expected.insert(hw_ids[0].clone().into()); + assert_eq!(missing_members().await, expected); + + // So does soft-deleting one. + diesel::update(sled_dsl::sled) + .filter(sled_dsl::id.eq(sled_ids[1].into_untyped_uuid())) + .set(sled_dsl::time_deleted.eq(Some(Utc::now()))) + .execute_async(&*conn) + .await + .unwrap(); + expected.insert(hw_ids[1].clone().into()); + assert_eq!(missing_members().await, expected); + + // A sled for the right baseboard but on a different rack does not + // count as present. + let sled9 = insert_sled_for_baseboard( + datastore, + RackUuid::new_v4(), + &hw_ids[9], + ) + .await; + assert_eq!(missing_members().await, expected); + + // Prepare for next test: reset sleds 0 and 1 to their prior states + diesel::update(sled_dsl::sled) + .filter(sled_dsl::id.eq(sled_ids[0].into_untyped_uuid())) + .set(sled_dsl::sled_policy.eq(to_db_sled_policy( + SledPolicy::InService { + provision_policy: SledProvisionPolicy::Provisionable, + }, + ))) + .execute_async(&*conn) + .await + .unwrap(); + diesel::update(sled_dsl::sled) + .filter(sled_dsl::id.eq(sled_ids[1].into_untyped_uuid())) + .set( + sled_dsl::time_deleted + .eq(Option::>::None), + ) + .execute_async(&*conn) + .await + .unwrap(); + + // Prepare for next test: decommission sled 9 + diesel::update(sled_dsl::sled) + .filter(sled_dsl::id.eq(sled9.into_untyped_uuid())) + .set(sled_dsl::sled_state.eq(SledState::Decommissioned)) + .execute_async(&*conn) + .await + .unwrap(); + + // All sleds having sled rows means missing_members() is empty + for hw in &hw_ids[8..] { + sled_ids + .push(insert_sled_for_baseboard(datastore, rack_id, hw).await); + } + let expected = BTreeSet::new(); + assert_eq!(missing_members().await, expected); + + db.terminate().await; + logctx.cleanup_successful(); + } } diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 234a3e77883..336908bdf75 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -3510,3 +3510,8 @@ table! { allow_tables_to_appear_in_same_query!(trust_quorum_member, hw_baseboard_id); joinable!(trust_quorum_member -> hw_baseboard_id(hw_baseboard_id)); + +// Declared as separate pairs rather than one three-table invocation, which +// would re-emit the `trust_quorum_member`/`hw_baseboard_id` impls above. +allow_tables_to_appear_in_same_query!(sled, hw_baseboard_id); +allow_tables_to_appear_in_same_query!(sled, trust_quorum_member); diff --git a/nexus/src/app/background/tasks/trust_quorum.rs b/nexus/src/app/background/tasks/trust_quorum.rs index 78d1bdb4346..ec823341222 100644 --- a/nexus/src/app/background/tasks/trust_quorum.rs +++ b/nexus/src/app/background/tasks/trust_quorum.rs @@ -19,8 +19,7 @@ use nexus_networking::{ }; use nexus_types::internal_api::background::TrustQuorumManagerStatus; use nexus_types::trust_quorum::{ - TrustQuorumConfig as NexusTrustQuorumConfig, TrustQuorumConfigState, - TrustQuorumMemberState, + TrustQuorumConfig as NexusTrustQuorumConfig, TrustQuorumMemberState, }; use omicron_common::address::{Ipv6Subnet, RACK_PREFIX_LENGTH, get_64_subnet}; use omicron_uuid_kinds::{RackUuid, SledUuid}; @@ -165,7 +164,7 @@ enum Status { CoordinatorNoLongerCommissioned(BaseboardId), Preparing, Commit(Vec), - Committed(Vec, Vec), + CommitAndStartSledAgents(Vec, Vec), } impl Status { @@ -197,7 +196,7 @@ impl Status { r.write_to_string(&mut s); } } - Status::Committed(op_results, start_results) => { + Status::CommitAndStartSledAgents(op_results, start_results) => { for r in op_results { r.write_to_string(&mut s); } @@ -228,41 +227,87 @@ async fn drive_reconfiguration( // If we are preparing, then collect from coordinator, otherwise // attempt to commit at unacked nodes. - if config.state.is_active() { + let status = if config.state.is_active() { info!( log, - "Loaded active trust quorum config from database"; + "Loaded active trust quorum config from database."; "rack_id" => %rack_id, "epoch" => %epoch, "state" => ?config.state ); + if config.state.is_preparing() { + prepare(&log, &opctx, &datastore, config.clone()).await? + } else { + commit(&log, &opctx, &datastore, config.clone()).await? + } } else { info!( log, - "Loaded inactive trust quorum config from database. Skipping"; + "Loaded inactive trust quorum config from database."; "rack_id" => %rack_id, "epoch" => %epoch, "state" => ?config.state ); - return Ok(Status::ConfigInactive); - } + Status::ConfigInactive + }; - // Poll the coordinator for commit acks - if config.state.is_preparing() { - return prepare(log, opctx, datastore, config).await; + // We may have committed all members, but some of them may not have started + // sled-agents yet. + // + // We continue to retry sending them a `StartSledAgentRequest` until it + // starts. + let added_sleds = datastore + .tq_get_committed_members_without_active_sled(&opctx, authz_tq.clone()) + .await?; + + if added_sleds.is_empty() { + return Ok(status); } - // For each unacked node, need to send a `Commit` or `PrepareAndCommit' - // - // At this point we know that the configuration is active and we are not - // preparing. Therefore we must be committing. - commit(log, opctx, datastore, config).await + // At this point, We know we've committed at least some members in this or + // prior attempts and need to start their sled-agents. Get the results for + // this attempt, if they exist. + let commit_results = match status { + Status::Commit(commit_results) => commit_results, + _ => vec![], + }; + + // Allocate a subnet and start a sled agent for each sled that needs it + let rack_id = config.rack_id; + let epoch = config.epoch; + + let existing_sleds: BTreeSet<_> = config + .members + .iter() + .filter_map(|m| { + if added_sleds.contains(&m.0) { None } else { Some(m.0.clone()) } + }) + .collect(); + + let started_sleds = allocate_subnets_and_start_sled_agents( + log, + opctx, + datastore, + rack_id, + epoch, + added_sleds, + existing_sleds, + ) + .await + .with_context(|| { + format!( + "Failed to start sled agents for added sleds for \ + rack {rack_id}, epoch {epoch}" + ) + })?; + + return Ok(Status::CommitAndStartSledAgents(commit_results, started_sleds)); } async fn prepare( - log: Logger, - opctx: OpContext, - datastore: Arc, + log: &Logger, + opctx: &OpContext, + datastore: &DataStore, config: NexusTrustQuorumConfig, ) -> Result { // Get a sled agent for the coordinator @@ -327,9 +372,9 @@ enum CommitOp { // // Return a unique status for each client that issued operations. async fn commit( - log: Logger, - opctx: OpContext, - datastore: Arc, + log: &Logger, + opctx: &OpContext, + datastore: &DataStore, nexus_config: NexusTrustQuorumConfig, ) -> Result { // All `Commit` or `PrepareAndCommit` requests sent to sled-agents @@ -407,7 +452,7 @@ async fn commit( // Write state back to DB let authz_tq = authz::TrustQuorumConfig::for_rack_id(nexus_config.rack_id); - let state = datastore + let _ = datastore .tq_update_commit_status( &opctx, authz_tq, @@ -423,45 +468,7 @@ async fn commit( ) })?; - // All sleds have acked commit. Let's see if any are newly added and - // therefore require their sled agents to be started. - // - // TODO: Currently we can only attempt to start sled agents from the - // Nexus that committed the configuration. This ensures a single Nexus - // issues the attempts, which is necessary because the call to start - // sled agents is neither idempotent nor concurrency safe. - // - // Unfortunately, this means that if the request fails here, it will - // never be retried. We'll likely have to debug this by either catching - // the error in the current call to the bg task status from omdb - // (unlikely), or reading logs. - // - // We are forced to do this because of urgency. Fixing how - // sled-agents get started has been a long standing issue: - // https://github.com/oxidecomputer/omicron/issues/4494. We'd like - // a reconciler pattern so that we could continuously retry from - // nexus in a background task similar to what is described here: - // https://github.com/oxidecomputer/omicron/issues/5132. However, this - // is a substantial project and will have to come after the initial - // trust quorum release. - if state == TrustQuorumConfigState::Committed { - let rack_id = nexus_config.rack_id; - let epoch = nexus_config.epoch; - let started_sleds = allocate_subnets_and_start_sled_agents( - log, - opctx, - datastore, - nexus_config, - ) - .await - .with_context(|| { - format!( - "Failed to start sled agents for added sleds for \ - rack {rack_id}, epoch {epoch}" - ) - })?; - return Ok(Status::Committed(client_results, started_sleds)); - } + return Ok(Status::Commit(client_results)); } Ok(Status::Commit(client_results)) @@ -471,37 +478,11 @@ async fn allocate_subnets_and_start_sled_agents( log: Logger, opctx: OpContext, datastore: Arc, - committed_config: NexusTrustQuorumConfig, + rack_id: RackUuid, + epoch: Epoch, + added_sleds: BTreeSet, + existing_sleds: BTreeSet, ) -> Result, Error> { - // No sleds could have been added to the trust quorum if there is no prior - // committed configuration. - let Some(last_committed_epoch) = committed_config.last_committed_epoch - else { - return Ok(vec![]); - }; - - let rack_id = committed_config.rack_id; - let epoch = committed_config.epoch; - - // Retrieve the last committed configuration so we can diff members and see - // who was added. - let authz_tq = authz::TrustQuorumConfig::for_rack_id(rack_id); - let Some(last_committed_config) = - datastore.tq_get_config(&opctx, authz_tq, last_committed_epoch).await? - else { - bail!( - "Failed to retrieve config from DB for rack {rack_id}, \ - last_committed_epoch {epoch}", - ); - }; - - let added_sleds: BTreeSet<_> = committed_config - .members - .keys() - .filter(|&id| !last_committed_config.members.contains_key(id)) - .cloned() - .collect(); - info!( log, "Looking up hw_baseboard_id for newly added sleds: {added_sleds:?}" @@ -547,7 +528,7 @@ async fn allocate_subnets_and_start_sled_agents( datastore, rack_id, epoch, - last_committed_config.members.keys().cloned().collect(), + existing_sleds, allocations_by_baseboard_id, ) .await @@ -559,7 +540,7 @@ async fn start_sled_agents( datastore: Arc, rack_id: RackUuid, epoch: Epoch, - all_members: BTreeSet, + existing_sleds: BTreeSet, allocations_by_baseboard_id: BTreeMap< BaseboardId, SledUnderlaySubnetAllocation, @@ -582,7 +563,7 @@ async fn start_sled_agents( &datastore, rack_id, epoch, - &all_members, + &existing_sleds, num_clients, Duration::from_mins(5), ) @@ -688,12 +669,12 @@ async fn get_sled_agent_clients( datastore: &DataStore, rack_id: RackUuid, epoch: Epoch, - all_members: &BTreeSet, + existing_sleds: &BTreeSet, num_clients: usize, timeout: Duration, ) -> Result, Error> { // First shuffle the possible members - let mut randomized: Vec<_> = all_members.iter().cloned().collect(); + let mut randomized: Vec<_> = existing_sleds.iter().cloned().collect(); randomized.shuffle(&mut rand::rng()); let mut clients = Vec::with_capacity(num_clients); diff --git a/nexus/types/src/trust_quorum.rs b/nexus/types/src/trust_quorum.rs index 6c3ed714b94..20437e88272 100644 --- a/nexus/types/src/trust_quorum.rs +++ b/nexus/types/src/trust_quorum.rs @@ -233,9 +233,9 @@ impl TrustQuorumConfig { pub fn commit_crash_tolerance(num_members: u8) -> u8 { match num_members { - 0..=3 => 0, - 4..=7 => 1, - 8..=15 => 2, + 0..=4 => 0, + 5..=8 => 1, + 9..=15 => 2, 16..=23 => 3, _ => 4, }