From a102d1cdc51227dc5e364a7dff970b12813cd8a8 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 1 Sep 2026 11:29:12 -0700 Subject: [PATCH 1/3] [spr] initial version Created using spr 1.3.6-beta.1 --- .../src/rendezvous_sled_bp_availability.rs | 73 +++- nexus/db-queries/src/db/datastore/mod.rs | 4 + .../rendezvous_sled_bp_availability.rs | 315 +++++++++++++++--- nexus/reconfigurator/rendezvous/src/lib.rs | 12 +- .../src/sled_blueprint_availability.rs | 215 +++++------- 5 files changed, 438 insertions(+), 181 deletions(-) diff --git a/nexus/db-model/src/rendezvous_sled_bp_availability.rs b/nexus/db-model/src/rendezvous_sled_bp_availability.rs index 50e90ef56d1..f4cc12ad85f 100644 --- a/nexus/db-model/src/rendezvous_sled_bp_availability.rs +++ b/nexus/db-model/src/rendezvous_sled_bp_availability.rs @@ -7,8 +7,11 @@ use crate::typed_generation::DbTypedGeneration; use crate::typed_uuid::DbTypedUuid; use anyhow::{Context, bail}; use chrono::{DateTime, Utc}; -use iddqd::{IdOrdItem, id_upcast}; +use iddqd::{IdOrdItem, IdOrdMap, id_upcast}; use nexus_db_schema::schema::rendezvous_sled_bp_availability; +use nexus_types::deployment::Blueprint; +use nexus_types::deployment::BlueprintSledConfig; +use nexus_types::external_api::sled::SledState; use omicron_generation_kinds::{ UpdateDispositionGeneration, UpdateDispositionGenerationKind, }; @@ -93,6 +96,74 @@ pub enum SledBpAvailabilityState { Decommissioned, } +impl SledBpAvailabilityState { + /// Create a `SledBpAvailabilityState` from a sled config. + pub fn from_blueprint_sled_config(config: &BlueprintSledConfig) -> Self { + match config.state { + SledState::Decommissioned => { + SledBpAvailabilityState::Decommissioned + } + SledState::Active => { + let disposition = config.update_disposition; + let availability = + if disposition.kind.is_available_for_provisioning() { + ActiveSledBpAvailability::Available + } else { + ActiveSledBpAvailability::Unavailable + }; + SledBpAvailabilityState::Active { + availability, + update_disposition_generation: disposition.generation, + } + } + } + } +} + +/// Data prepared for a single sled to write to the `rendezvous_sled_bp_availability` table. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct SledBlueprintAvailabilityInput { + /// The sled ID. + pub sled_id: SledUuid, + + /// The current availability state of the sled. + pub state: SledBpAvailabilityState, +} + +impl IdOrdItem for SledBlueprintAvailabilityInput { + type Key<'a> = SledUuid; + + fn key(&self) -> Self::Key<'_> { + self.sled_id + } + + id_upcast!(); +} + +impl SledBlueprintAvailabilityInput { + /// Derive a sled's reconciliation input from its blueprint config. + pub fn from_blueprint( + sled_id: SledUuid, + config: &BlueprintSledConfig, + ) -> Self { + Self { + sled_id, + state: SledBpAvailabilityState::from_blueprint_sled_config(config), + } + } + + /// Generate a [`SledBlueprintAvailabilityInput`] for every sled in the + /// blueprint. + pub fn all_from_blueprint(blueprint: &Blueprint) -> IdOrdMap { + IdOrdMap::from_iter_unique( + blueprint.sleds.iter().map(|(&sled_id, config)| { + Self::from_blueprint(sled_id, config) + }), + ) + .expect("blueprint.sleds is keyed by sled ID, so inputs are unique") + } +} + /// Database representation of a sled tracked by the `rendezvous_sled_bp_availability` /// table. /// diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 3443ac5c2e8..1cc06cc8058 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -187,6 +187,10 @@ pub use region::RegionAllocationFor; pub use region::RegionAllocationParameters; pub use region_snapshot_replacement::NewRegionVolumeId; pub use region_snapshot_replacement::OldSnapshotVolumeId; +pub use rendezvous_sled_bp_availability::SledBpAvailabilityDecommissionOutcome; +pub use rendezvous_sled_bp_availability::SledBpAvailabilityUpsertOutcome; +pub use rendezvous_sled_bp_availability::SledBpAvailabilityWrite; +pub use rendezvous_sled_bp_availability::SledBpAvailabilityWriteOutcome; pub use saga::NewSagaState; pub use saga::SagaStateDbFields; pub use scim_provider_store::CrdbScimProviderStore; diff --git a/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs b/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs index 2c6c72739c8..cb9761418ea 100644 --- a/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs +++ b/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs @@ -11,19 +11,115 @@ use crate::db::pagination::paginated; use async_bb8_diesel::AsyncRunQueryDsl; use diesel::prelude::*; use diesel::upsert::excluded; +use iddqd::IdOrdItem; use iddqd::IdOrdMap; +use iddqd::id_upcast; use nexus_db_errors::ErrorHandler; use nexus_db_errors::public_error_from_diesel; +use nexus_db_lookup::DbConnection; +use nexus_db_model::ActiveSledBpAvailability; use nexus_db_model::DbSledBpAvailability; use nexus_db_model::RendezvousSledBpAvailability; use nexus_db_model::RendezvousSledBpAvailabilityDecommission; use nexus_db_model::RendezvousSledBpAvailabilityUpdate; +use nexus_db_model::SledBlueprintAvailabilityInput; +use nexus_db_model::SledBpAvailabilityState; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; +use omicron_generation_kinds::UpdateDispositionGeneration; +use omicron_uuid_kinds::BlueprintUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::SledUuid; +/// The result of a generation-guarded availability upsert. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SledBpAvailabilityUpsertOutcome { + /// The row was inserted or updated. + Written, + /// The guard rejected the write. + /// + /// This can happen because: + /// + /// - the stored row is at an equal or newer `update_disposition` generation; + /// - or, the sled is decommissioned. + Rejected, +} + +/// The result of recording a sled as decommissioned. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SledBpAvailabilityDecommissionOutcome { + /// The sled transitioned to decommissioned (fresh tombstone or an active + /// row overwritten). + Decommissioned, + /// The sled was already decommissioned; the row was left alone. + AlreadyDecommissioned, +} + +/// The result of writing one blueprint sled's availability to the +/// `rendezvous_sled_bp_availability` table. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct SledBpAvailabilityWrite { + pub sled_id: SledUuid, + pub outcome: SledBpAvailabilityWriteOutcome, +} + +impl IdOrdItem for SledBpAvailabilityWrite { + type Key<'a> = SledUuid; + + fn key(&self) -> Self::Key<'_> { + self.sled_id + } + + id_upcast!(); +} + +/// The result of writing one blueprint sled's availability to the +/// `rendezvous_sled_bp_availability` table. +/// +/// Part of [`SledBpAvailabilityWrite`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SledBpAvailabilityWriteOutcome { + /// The sled is active in the blueprint. + Active { + /// The availability written to the database. + availability: ActiveSledBpAvailability, + + /// The generation of the update disposition that was written. + update_disposition_generation: UpdateDispositionGeneration, + + /// Whether the upsert operation was successful. + outcome: SledBpAvailabilityUpsertOutcome, + }, + + /// The sled is decommissioned in the blueprint. + Decommission(SledBpAvailabilityDecommissionOutcome), +} + +impl SledBpAvailabilityWriteOutcome { + /// Return true if the write changed the database. + pub fn applied(&self) -> bool { + match self { + SledBpAvailabilityWriteOutcome::Active { outcome, .. } => { + match outcome { + SledBpAvailabilityUpsertOutcome::Written => true, + SledBpAvailabilityUpsertOutcome::Rejected => false, + } + } + SledBpAvailabilityWriteOutcome::Decommission(outcome) => { + match outcome { + SledBpAvailabilityDecommissionOutcome::Decommissioned => { + true + } + SledBpAvailabilityDecommissionOutcome::AlreadyDecommissioned => { + false + } + } + } + } + } +} + impl DataStore { /// List one page of sleds in the rendezvous table. async fn rendezvous_sled_bp_availability_list_all_page( @@ -88,6 +184,88 @@ impl DataStore { Ok(all_sleds) } + /// Write the availability of each sled in `sleds` to the database. + pub async fn rendezvous_sled_bp_availability_write( + &self, + opctx: &OpContext, + blueprint_id: BlueprintUuid, + sleds: IdOrdMap, + ) -> Result, Error> { + opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; + let conn = self.pool_connection_authorized(opctx).await?; + Self::rendezvous_sled_bp_availability_write_on_connection( + &conn, + blueprint_id, + sleds, + ) + .await + } + + /// on_connection variant of `rendezvous_sled_bp_availability_write`. + /// + /// The caller is responsible for authorizing the operation. + pub(crate) async fn rendezvous_sled_bp_availability_write_on_connection( + conn: &async_bb8_diesel::Connection, + blueprint_id: BlueprintUuid, + sleds: IdOrdMap, + ) -> Result, Error> { + let mut writes = IdOrdMap::new(); + for SledBlueprintAvailabilityInput { sled_id, state } in sleds { + let outcome = match state { + SledBpAvailabilityState::Active { + availability, + update_disposition_generation, + } => { + let outcome = + Self::rendezvous_sled_bp_availability_upsert_on_connection( + conn, + RendezvousSledBpAvailabilityUpdate::new( + sled_id, + availability, + update_disposition_generation, + blueprint_id, + ), + ) + .await + .map_err(|e| { + e.internal_context(format!( + "failed to upsert availability for sled \ + {sled_id}" + )) + })?; + SledBpAvailabilityWriteOutcome::Active { + availability, + update_disposition_generation, + outcome, + } + } + SledBpAvailabilityState::Decommissioned => { + let outcome = + Self::rendezvous_sled_bp_availability_decommission_on_connection( + conn, + RendezvousSledBpAvailabilityDecommission::new( + sled_id, + blueprint_id, + ), + ) + .await + .map_err(|e| { + e.internal_context(format!( + "failed to decommission sled {sled_id}" + )) + })?; + SledBpAvailabilityWriteOutcome::Decommission(outcome) + } + }; + writes + .insert_unique(SledBpAvailabilityWrite { sled_id, outcome }) + .expect( + "input map is keyed by sled ID, so each sled appears once", + ); + } + Ok(writes) + } + /// Record a sled's availability in the `rendezvous_sled_bp_availability` /// table, guarded by its `update_disposition` generation. /// @@ -99,22 +277,32 @@ impl DataStore { /// /// Together, these prevent duelling Nexuses from trampling over each /// other's state. - /// - /// Returns `true` if a row was written, and `false` if the write was - /// rejected (stale generation, or the sled is already decommissioned). pub async fn rendezvous_sled_bp_availability_upsert( &self, opctx: &OpContext, update: RendezvousSledBpAvailabilityUpdate, - ) -> Result { + ) -> Result { + opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; + let conn = self.pool_connection_authorized(opctx).await?; + Self::rendezvous_sled_bp_availability_upsert_on_connection( + &conn, update, + ) + .await + } + + /// on_connection variant of `rendezvous_sled_bp_availability_upsert`. + /// + /// The caller is responsible for authorizing the operation. + pub(crate) async fn rendezvous_sled_bp_availability_upsert_on_connection( + conn: &async_bb8_diesel::Connection, + update: RendezvousSledBpAvailabilityUpdate, + ) -> Result { // `FilterDsl` brings `.filter()` (the `WHERE` on the `ON CONFLICT DO // UPDATE` below) into scope; the prelude's `QueryDsl` only offers it for // table-like queries, not upsert statements. use diesel::query_dsl::methods::FilterDsl; use nexus_db_schema::schema::rendezvous_sled_bp_availability::dsl; - opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; - // Compared against the stored generation by the staleness guard. let incoming_generation = nexus_db_model::to_db_typed_generation( update.update_disposition_generation(), @@ -138,13 +326,13 @@ impl DataStore { ) // Overwrite only if our generation is newer than what's stored. .filter(dsl::update_disposition_generation.lt(incoming_generation)) - .execute_async(&*self.pool_connection_authorized(opctx).await?) + .execute_async(conn) .await // The conflict target is the primary key, so at most one row is // inserted or updated. .map(|rows_modified| match rows_modified { - 0 => false, - 1 => true, + 0 => SledBpAvailabilityUpsertOutcome::Rejected, + 1 => SledBpAvailabilityUpsertOutcome::Written, n => unreachable!( "upsert by primary key sled_id modified {n} rows" ), @@ -156,22 +344,33 @@ impl DataStore { /// `rendezvous_sled_bp_availability` table. /// /// This is a terminal state. - /// - /// Returns `true` if the sled transitioned to decommissioned, and `false` - /// if it was already decommissioned. pub async fn rendezvous_sled_bp_availability_decommission( &self, opctx: &OpContext, decommission: RendezvousSledBpAvailabilityDecommission, - ) -> Result { + ) -> Result { + opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; + let conn = self.pool_connection_authorized(opctx).await?; + Self::rendezvous_sled_bp_availability_decommission_on_connection( + &conn, + decommission, + ) + .await + } + + /// on_connection variant of `rendezvous_sled_bp_availability_decommission`. + /// + /// The caller is responsible for authorizing the operation. + pub(crate) async fn rendezvous_sled_bp_availability_decommission_on_connection( + conn: &async_bb8_diesel::Connection, + decommission: RendezvousSledBpAvailabilityDecommission, + ) -> Result { // `FilterDsl` brings `.filter()` (the `WHERE` on the `ON CONFLICT DO // UPDATE` below) into scope; the prelude's `QueryDsl` only offers it for // table-like queries, not upsert statements. use diesel::query_dsl::methods::FilterDsl; use nexus_db_schema::schema::rendezvous_sled_bp_availability::dsl; - opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; - let tombstone = decommission.into_insertable(); diesel::insert_into(dsl::rendezvous_sled_bp_availability) @@ -194,13 +393,15 @@ impl DataStore { .filter( dsl::bp_availability.ne(DbSledBpAvailability::Decommissioned), ) - .execute_async(&*self.pool_connection_authorized(opctx).await?) + .execute_async(conn) .await // The conflict target is the primary key, so at most one row is // inserted or updated. .map(|rows_modified| match rows_modified { - 0 => false, - 1 => true, + 0 => { + SledBpAvailabilityDecommissionOutcome::AlreadyDecommissioned + } + 1 => SledBpAvailabilityDecommissionOutcome::Decommissioned, n => unreachable!( "upsert by primary key sled_id modified {n} rows" ), @@ -260,14 +461,18 @@ mod tests { let bp2 = BlueprintUuid::new_v4(); // Initial insert at generation 1, available. - let wrote = datastore + let outcome = datastore .rendezvous_sled_bp_availability_upsert( opctx, row(sled_id, ActiveSledBpAvailability::Available, 1, bp1), ) .await .expect("query succeeded"); - assert!(wrote, "first insert should write"); + assert_eq!( + outcome, + SledBpAvailabilityUpsertOutcome::Written, + "first insert should write" + ); let got = get(opctx, datastore, sled_id).await.expect("row present"); assert_eq!( got.state().expect("reassembled row state"), @@ -279,14 +484,18 @@ mod tests { ); // A newer generation (2) flipping the sled to unavailable should win. - let wrote = datastore + let outcome = datastore .rendezvous_sled_bp_availability_upsert( opctx, row(sled_id, ActiveSledBpAvailability::Unavailable, 2, bp2), ) .await .expect("query succeeded"); - assert!(wrote, "newer generation should write"); + assert_eq!( + outcome, + SledBpAvailabilityUpsertOutcome::Written, + "newer generation should write" + ); let got = get(opctx, datastore, sled_id).await.expect("row present"); assert_eq!( got.state().expect("reassembled row state"), @@ -300,14 +509,18 @@ mod tests { // A stale write at the same generation (2) trying to flip back to // available must be rejected. - let wrote = datastore + let outcome = datastore .rendezvous_sled_bp_availability_upsert( opctx, row(sled_id, ActiveSledBpAvailability::Available, 2, bp1), ) .await .expect("query succeeded"); - assert!(!wrote, "equal-generation write should be rejected as stale"); + assert_eq!( + outcome, + SledBpAvailabilityUpsertOutcome::Rejected, + "equal-generation write should be rejected as stale" + ); let got = get(opctx, datastore, sled_id).await.expect("row present"); assert_eq!( got.state().expect("reassembled row state"), @@ -320,14 +533,18 @@ mod tests { ); // A stale write at an older generation (1) must also be rejected. - let wrote = datastore + let outcome = datastore .rendezvous_sled_bp_availability_upsert( opctx, row(sled_id, ActiveSledBpAvailability::Available, 1, bp1), ) .await .expect("query succeeded"); - assert!(!wrote, "older-generation write should be rejected as stale"); + assert_eq!( + outcome, + SledBpAvailabilityUpsertOutcome::Rejected, + "older-generation write should be rejected as stale" + ); let got = get(opctx, datastore, sled_id).await.expect("row present"); assert_eq!( got.state().expect("reassembled row state"), @@ -340,14 +557,18 @@ mod tests { ); // A newer generation (3) flipping back to available wins. - let wrote = datastore + let outcome = datastore .rendezvous_sled_bp_availability_upsert( opctx, row(sled_id, ActiveSledBpAvailability::Available, 3, bp2), ) .await .expect("query succeeded"); - assert!(wrote, "newest generation should write"); + assert_eq!( + outcome, + SledBpAvailabilityUpsertOutcome::Written, + "newest generation should write" + ); let got = get(opctx, datastore, sled_id).await.expect("row present"); assert_eq!( got.state().expect("reassembled row state"), @@ -394,14 +615,17 @@ mod tests { // Decommissioning it should change the sled's state to // `decommissioned`. - let decommissioned = datastore + let outcome = datastore .rendezvous_sled_bp_availability_decommission( opctx, RendezvousSledBpAvailabilityDecommission::new(sled_id, bp), ) .await .expect("query succeeded"); - assert!(decommissioned); + assert_eq!( + outcome, + SledBpAvailabilityDecommissionOutcome::Decommissioned + ); let got = get(opctx, datastore, sled_id).await.expect("row remains"); assert_eq!( got.state().expect("reassembled row state"), @@ -409,25 +633,32 @@ mod tests { ); // A second decommission is a no-op. - let decommissioned = datastore + let outcome = datastore .rendezvous_sled_bp_availability_decommission( opctx, RendezvousSledBpAvailabilityDecommission::new(sled_id, bp), ) .await .expect("query succeeded"); - assert!(!decommissioned); + assert_eq!( + outcome, + SledBpAvailabilityDecommissionOutcome::AlreadyDecommissioned + ); // A stale Nexus must not be able to resurrect a decommissioned sled, // even at a newer generation. - let wrote = datastore + let outcome = datastore .rendezvous_sled_bp_availability_upsert( opctx, row(sled_id, ActiveSledBpAvailability::Available, 5, bp), ) .await .expect("query succeeded"); - assert!(!wrote, "decommissioned sled must not be resurrected"); + assert_eq!( + outcome, + SledBpAvailabilityUpsertOutcome::Rejected, + "decommissioned sled must not be resurrected" + ); assert_eq!( get(opctx, datastore, sled_id) .await @@ -452,14 +683,18 @@ mod tests { let bp = BlueprintUuid::new_v4(); let bp_stale = BlueprintUuid::new_v4(); - let decommissioned = datastore + let outcome = datastore .rendezvous_sled_bp_availability_decommission( opctx, RendezvousSledBpAvailabilityDecommission::new(sled_id, bp), ) .await .expect("query succeeded"); - assert!(decommissioned, "fresh tombstone insert should report true"); + assert_eq!( + outcome, + SledBpAvailabilityDecommissionOutcome::Decommissioned, + "fresh tombstone insert should report true" + ); let got = get(opctx, datastore, sled_id).await.expect("row present"); assert_eq!( got.state().expect("reassembled row state"), @@ -467,14 +702,18 @@ mod tests { ); assert_eq!(got.blueprint_id(), bp); - let wrote = datastore + let outcome = datastore .rendezvous_sled_bp_availability_upsert( opctx, row(sled_id, ActiveSledBpAvailability::Available, 5, bp_stale), ) .await .expect("query succeeded"); - assert!(!wrote, "decommissioned sled must not be resurrected"); + assert_eq!( + outcome, + SledBpAvailabilityUpsertOutcome::Rejected, + "decommissioned sled must not be resurrected" + ); assert_eq!( get(opctx, datastore, sled_id).await.unwrap().blueprint_id(), bp, diff --git a/nexus/reconfigurator/rendezvous/src/lib.rs b/nexus/reconfigurator/rendezvous/src/lib.rs index f6086c1a48b..07eca42bf4d 100644 --- a/nexus/reconfigurator/rendezvous/src/lib.rs +++ b/nexus/reconfigurator/rendezvous/src/lib.rs @@ -8,16 +8,14 @@ //! Rendezvous tables reflect resources that are in service and available for //! other parts of Nexus to use. See RFD 541 for more background. -use iddqd::IdOrdMap; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; +use nexus_db_queries::db::model::SledBlueprintAvailabilityInput; use nexus_types::deployment::Blueprint; use nexus_types::deployment::BlueprintDatasetDisposition; use nexus_types::internal_api::background::BlueprintRendezvousStats; use nexus_types::inventory::Collection; -use crate::sled_blueprint_availability::SledBlueprintAvailabilityInput; - mod crucible_dataset; mod debug_dataset; mod local_storage_dataset; @@ -81,12 +79,8 @@ pub async fn reconcile_blueprint_rendezvous_tables( ) .await?; - let sled_inputs = IdOrdMap::from_iter_unique(blueprint.sleds.iter().map( - |(&sled_id, config)| { - SledBlueprintAvailabilityInput::from_blueprint(sled_id, config) - }, - )) - .expect("blueprint.sleds is keyed by sled ID, so inputs are unique"); + let sled_inputs = + SledBlueprintAvailabilityInput::all_from_blueprint(blueprint); let sled_blueprint_availability = sled_blueprint_availability::reconcile_sled_blueprint_availability( opctx, diff --git a/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs b/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs index e9b3694bca5..ae8662587b0 100644 --- a/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs +++ b/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs @@ -7,72 +7,21 @@ //! See the table comment in `dbinit.sql`. use anyhow::Context; -use iddqd::IdOrdItem; use iddqd::IdOrdMap; -use iddqd::id_upcast; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; +use nexus_db_queries::db::datastore::SledBpAvailabilityDecommissionOutcome; +use nexus_db_queries::db::datastore::SledBpAvailabilityUpsertOutcome; +use nexus_db_queries::db::datastore::SledBpAvailabilityWriteOutcome; use nexus_db_queries::db::model::ActiveSledBpAvailability; use nexus_db_queries::db::model::DbSledBpAvailability; -use nexus_db_queries::db::model::RendezvousSledBpAvailabilityDecommission; -use nexus_db_queries::db::model::RendezvousSledBpAvailabilityUpdate; +use nexus_db_queries::db::model::SledBlueprintAvailabilityInput; use nexus_db_queries::db::model::SledBpAvailabilityState; -use nexus_types::deployment::BlueprintSledConfig; -use nexus_types::external_api::sled::SledState; use nexus_types::internal_api::background::SledBlueprintAvailabilityRendezvousStats; use omicron_uuid_kinds::BlueprintUuid; -use omicron_uuid_kinds::SledUuid; use slog::error; use slog::info; -/// One blueprint sled's input to reconciliation. -#[derive(Debug)] -pub(crate) struct SledBlueprintAvailabilityInput { - /// The sled ID. - pub(crate) sled_id: SledUuid, - - /// The current availability state of the sled. - pub(crate) state: SledBpAvailabilityState, -} - -impl IdOrdItem for SledBlueprintAvailabilityInput { - type Key<'a> = SledUuid; - - fn key(&self) -> Self::Key<'_> { - self.sled_id - } - - id_upcast!(); -} - -impl SledBlueprintAvailabilityInput { - /// Derives a sled's reconciliation input from its blueprint config. - pub(crate) fn from_blueprint( - sled_id: SledUuid, - config: &BlueprintSledConfig, - ) -> Self { - let state = match config.state { - SledState::Decommissioned => { - SledBpAvailabilityState::Decommissioned - } - SledState::Active => { - let disposition = config.update_disposition; - let availability = - if disposition.kind.is_available_for_provisioning() { - ActiveSledBpAvailability::Available - } else { - ActiveSledBpAvailability::Unavailable - }; - SledBpAvailabilityState::Active { - availability, - update_disposition_generation: disposition.generation, - } - } - }; - Self { sled_id, state } - } -} - /// Reconcile the `rendezvous_sled_bp_availability` table against the target /// blueprint. /// @@ -96,6 +45,9 @@ pub(crate) async fn reconcile_sled_blueprint_availability( let mut stats = SledBlueprintAvailabilityRendezvousStats::default(); + // Use the snapshot to decide which sleds need a write at all. (This will + // always identify a superset of writes that will be successful.) + let mut to_write = IdOrdMap::new(); for input in blueprint_sleds { let SledBlueprintAvailabilityInput { sled_id, state } = input; // Account for this sled; rows left in `existing_db_sleds` after the loop @@ -166,55 +118,6 @@ pub(crate) async fn reconcile_sled_blueprint_availability( stats.num_unchanged += 1; continue; } - - let wrote = datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - RendezvousSledBpAvailabilityUpdate::new( - sled_id, - availability, - update_disposition_generation, - blueprint_id, - ), - ) - .await - .with_context(|| { - format!( - "failed to upsert availability for sled {sled_id}" - ) - })?; - - if wrote { - match availability { - ActiveSledBpAvailability::Available => { - stats.num_marked_available += 1; - info!( - opctx.log, - "marked sled available for provisioning"; - "sled_id" => %sled_id, - "update_disposition_generation" => - %update_disposition_generation, - ); - } - ActiveSledBpAvailability::Unavailable => { - stats.num_marked_unavailable += 1; - info!( - opctx.log, - "marked sled unavailable for provisioning"; - "sled_id" => %sled_id, - "update_disposition_generation" => - %update_disposition_generation, - ); - } - } - } else { - // We decided to perform a write, but a duelling Nexus - // recorded an equal-or-newer generation (or decommissioned - // the sled) first, so the row already reflects - // current-or-newer state. From our perspective, this is an - // unchanged sled. - stats.num_unchanged += 1; - } } SledBpAvailabilityState::Decommissioned => { // This is monotonic, so unlike the available/unavailable flip @@ -222,6 +125,7 @@ pub(crate) async fn reconcile_sled_blueprint_availability( match existing_state { Some(SledBpAvailabilityState::Decommissioned) => { stats.num_already_decommissioned += 1; + continue; } // The None case here means that: // @@ -238,44 +142,86 @@ pub(crate) async fn reconcile_sled_blueprint_availability( // the sled was still active, inserted a row after the // snapshot was taken. // - // So in the None case we still call - // rendezvous_sled_bp_availability_decommission, which does - // an upsert just like - // rendezvous_sled_bp_availability_upsert. + // So in the None case we still write the decommission, + // which does an upsert just like the active-sled write. // // * With case 1 we'll insert a fresh row. // * With case 2 we'll tombstone the racing row. // // Either way, the sled ends up with a durable // decommissioned tombstone. - None | Some(SledBpAvailabilityState::Active { .. }) => { - let decommissioned = datastore - .rendezvous_sled_bp_availability_decommission( - opctx, - RendezvousSledBpAvailabilityDecommission::new( - sled_id, - blueprint_id, - ), - ) - .await - .with_context(|| { - format!("failed to decommission sled {sled_id}") - })?; - if decommissioned { - stats.num_decommissioned += 1; - info!( - opctx.log, - "decommissioned sled in rendezvous table"; - "sled_id" => %sled_id, - ); - } else { - // Another Nexus decommissioned it first. - stats.num_already_decommissioned += 1; - } - } + None | Some(SledBpAvailabilityState::Active { .. }) => {} } } } + + to_write.insert_unique(input).expect( + "blueprint_sleds is keyed by sled ID, so each sled appears once", + ); + } + + let writes = datastore + .rendezvous_sled_bp_availability_write(opctx, blueprint_id, to_write) + .await + .context("failed to write sled availability")?; + + for write in writes { + let sled_id = write.sled_id; + match write.outcome { + SledBpAvailabilityWriteOutcome::Active { + availability, + update_disposition_generation, + outcome: SledBpAvailabilityUpsertOutcome::Written, + } => match availability { + ActiveSledBpAvailability::Available => { + stats.num_marked_available += 1; + info!( + opctx.log, + "marked sled available for provisioning"; + "sled_id" => %sled_id, + "update_disposition_generation" => + %update_disposition_generation, + ); + } + ActiveSledBpAvailability::Unavailable => { + stats.num_marked_unavailable += 1; + info!( + opctx.log, + "marked sled unavailable for provisioning"; + "sled_id" => %sled_id, + "update_disposition_generation" => + %update_disposition_generation, + ); + } + }, + SledBpAvailabilityWriteOutcome::Active { + outcome: SledBpAvailabilityUpsertOutcome::Rejected, + .. + } => { + // We decided to perform a write, but a duelling Nexus + // recorded an equal-or-newer generation (or decommissioned + // the sled) first, so the row already reflects + // current-or-newer state. From our perspective, this is an + // unchanged sled. + stats.num_unchanged += 1; + } + SledBpAvailabilityWriteOutcome::Decommission( + SledBpAvailabilityDecommissionOutcome::Decommissioned, + ) => { + stats.num_decommissioned += 1; + info!( + opctx.log, + "decommissioned sled in rendezvous table"; + "sled_id" => %sled_id, + ); + } + SledBpAvailabilityWriteOutcome::Decommission( + SledBpAvailabilityDecommissionOutcome::AlreadyDecommissioned, + ) => { + // Another Nexus decommissioned it first. + stats.num_already_decommissioned += 1; + } + } } // Rows still here are for sleds the blueprint doesn't mention. We leave @@ -317,10 +263,13 @@ mod tests { use crate::tests::usize_to_id; use async_bb8_diesel::AsyncRunQueryDsl; use async_bb8_diesel::AsyncSimpleConnection; + use nexus_db_queries::db::model::RendezvousSledBpAvailabilityDecommission; + use nexus_db_queries::db::model::RendezvousSledBpAvailabilityUpdate; use nexus_db_queries::db::pub_test_utils::TestDatabase; use nexus_db_queries::db::queries::ALLOW_FULL_TABLE_SCAN_SQL; use omicron_generation_kinds::UpdateDispositionGeneration; use omicron_test_utils::dev; + use omicron_uuid_kinds::SledUuid; use proptest::prelude::*; use proptest::proptest; use test_strategy::Arbitrary; From b665f4dd6d4b3c90c29d44a0940e20b1b17df50a Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 1 Sep 2026 13:18:01 -0700 Subject: [PATCH 2/3] remove the old APIs as well Created using spr 1.3.6-beta.1 --- .../rendezvous_sled_bp_availability.rs | 256 ++++++++++-------- .../src/sled_blueprint_availability.rs | 91 +++---- 2 files changed, 184 insertions(+), 163 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs b/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs index cb9761418ea..7d8b2812669 100644 --- a/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs +++ b/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs @@ -277,20 +277,6 @@ impl DataStore { /// /// Together, these prevent duelling Nexuses from trampling over each /// other's state. - pub async fn rendezvous_sled_bp_availability_upsert( - &self, - opctx: &OpContext, - update: RendezvousSledBpAvailabilityUpdate, - ) -> Result { - opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; - let conn = self.pool_connection_authorized(opctx).await?; - Self::rendezvous_sled_bp_availability_upsert_on_connection( - &conn, update, - ) - .await - } - - /// on_connection variant of `rendezvous_sled_bp_availability_upsert`. /// /// The caller is responsible for authorizing the operation. pub(crate) async fn rendezvous_sled_bp_availability_upsert_on_connection( @@ -344,21 +330,6 @@ impl DataStore { /// `rendezvous_sled_bp_availability` table. /// /// This is a terminal state. - pub async fn rendezvous_sled_bp_availability_decommission( - &self, - opctx: &OpContext, - decommission: RendezvousSledBpAvailabilityDecommission, - ) -> Result { - opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; - let conn = self.pool_connection_authorized(opctx).await?; - Self::rendezvous_sled_bp_availability_decommission_on_connection( - &conn, - decommission, - ) - .await - } - - /// on_connection variant of `rendezvous_sled_bp_availability_decommission`. /// /// The caller is responsible for authorizing the operation. pub(crate) async fn rendezvous_sled_bp_availability_decommission_on_connection( @@ -414,26 +385,81 @@ impl DataStore { mod tests { use super::*; use crate::db::pub_test_utils::TestDatabase; + use iddqd::id_ord_map; use nexus_db_model::ActiveSledBpAvailability; use nexus_db_model::SledBpAvailabilityState; use omicron_generation_kinds::UpdateDispositionGeneration; use omicron_test_utils::dev; use omicron_uuid_kinds::BlueprintUuid; - // Convenience for building an availability update at a given - // availability/generation. - fn row( + async fn upsert_one( + opctx: &OpContext, + datastore: &DataStore, sled_id: SledUuid, availability: ActiveSledBpAvailability, generation: u32, blueprint_id: BlueprintUuid, - ) -> RendezvousSledBpAvailabilityUpdate { - RendezvousSledBpAvailabilityUpdate::new( - sled_id, - availability, - UpdateDispositionGeneration::from(generation), - blueprint_id, - ) + ) -> SledBpAvailabilityUpsertOutcome { + let update_disposition_generation = + UpdateDispositionGeneration::from(generation); + let writes = datastore + .rendezvous_sled_bp_availability_write( + opctx, + blueprint_id, + id_ord_map! { + SledBlueprintAvailabilityInput { + sled_id, + state: SledBpAvailabilityState::Active { + availability, + update_disposition_generation, + }, + }, + }, + ) + .await + .expect("query succeeded"); + match writes.get(&sled_id).expect("write for the input sled").outcome { + SledBpAvailabilityWriteOutcome::Active { + availability: written_availability, + update_disposition_generation: written_generation, + outcome, + } => { + assert_eq!(written_availability, availability); + assert_eq!(written_generation, update_disposition_generation); + outcome + } + SledBpAvailabilityWriteOutcome::Decommission(outcome) => panic!( + "an active input must report an active outcome, got \ + {outcome:?}" + ), + } + } + + async fn decommission_one( + opctx: &OpContext, + datastore: &DataStore, + sled_id: SledUuid, + blueprint_id: BlueprintUuid, + ) -> SledBpAvailabilityDecommissionOutcome { + let writes = datastore + .rendezvous_sled_bp_availability_write( + opctx, + blueprint_id, + id_ord_map! { + SledBlueprintAvailabilityInput { + sled_id, + state: SledBpAvailabilityState::Decommissioned, + }, + }, + ) + .await + .expect("query succeeded"); + match writes.get(&sled_id).expect("write for the input sled").outcome { + SledBpAvailabilityWriteOutcome::Decommission(outcome) => outcome, + SledBpAvailabilityWriteOutcome::Active { .. } => panic!( + "a decommissioned input must report a decommission outcome" + ), + } } // Fetch the single row for a sled, if present. @@ -461,13 +487,15 @@ mod tests { let bp2 = BlueprintUuid::new_v4(); // Initial insert at generation 1, available. - let outcome = datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - row(sled_id, ActiveSledBpAvailability::Available, 1, bp1), - ) - .await - .expect("query succeeded"); + let outcome = upsert_one( + opctx, + datastore, + sled_id, + ActiveSledBpAvailability::Available, + 1, + bp1, + ) + .await; assert_eq!( outcome, SledBpAvailabilityUpsertOutcome::Written, @@ -484,13 +512,15 @@ mod tests { ); // A newer generation (2) flipping the sled to unavailable should win. - let outcome = datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - row(sled_id, ActiveSledBpAvailability::Unavailable, 2, bp2), - ) - .await - .expect("query succeeded"); + let outcome = upsert_one( + opctx, + datastore, + sled_id, + ActiveSledBpAvailability::Unavailable, + 2, + bp2, + ) + .await; assert_eq!( outcome, SledBpAvailabilityUpsertOutcome::Written, @@ -509,13 +539,15 @@ mod tests { // A stale write at the same generation (2) trying to flip back to // available must be rejected. - let outcome = datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - row(sled_id, ActiveSledBpAvailability::Available, 2, bp1), - ) - .await - .expect("query succeeded"); + let outcome = upsert_one( + opctx, + datastore, + sled_id, + ActiveSledBpAvailability::Available, + 2, + bp1, + ) + .await; assert_eq!( outcome, SledBpAvailabilityUpsertOutcome::Rejected, @@ -533,13 +565,15 @@ mod tests { ); // A stale write at an older generation (1) must also be rejected. - let outcome = datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - row(sled_id, ActiveSledBpAvailability::Available, 1, bp1), - ) - .await - .expect("query succeeded"); + let outcome = upsert_one( + opctx, + datastore, + sled_id, + ActiveSledBpAvailability::Available, + 1, + bp1, + ) + .await; assert_eq!( outcome, SledBpAvailabilityUpsertOutcome::Rejected, @@ -557,13 +591,15 @@ mod tests { ); // A newer generation (3) flipping back to available wins. - let outcome = datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - row(sled_id, ActiveSledBpAvailability::Available, 3, bp2), - ) - .await - .expect("query succeeded"); + let outcome = upsert_one( + opctx, + datastore, + sled_id, + ActiveSledBpAvailability::Available, + 3, + bp2, + ) + .await; assert_eq!( outcome, SledBpAvailabilityUpsertOutcome::Written, @@ -593,13 +629,15 @@ mod tests { let bp = BlueprintUuid::new_v4(); // Insert the sled (available, generation 1) and confirm we see it. - datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - row(sled_id, ActiveSledBpAvailability::Available, 1, bp), - ) - .await - .expect("query succeeded"); + upsert_one( + opctx, + datastore, + sled_id, + ActiveSledBpAvailability::Available, + 1, + bp, + ) + .await; assert_eq!( get(opctx, datastore, sled_id) .await @@ -615,13 +653,7 @@ mod tests { // Decommissioning it should change the sled's state to // `decommissioned`. - let outcome = datastore - .rendezvous_sled_bp_availability_decommission( - opctx, - RendezvousSledBpAvailabilityDecommission::new(sled_id, bp), - ) - .await - .expect("query succeeded"); + let outcome = decommission_one(opctx, datastore, sled_id, bp).await; assert_eq!( outcome, SledBpAvailabilityDecommissionOutcome::Decommissioned @@ -633,13 +665,7 @@ mod tests { ); // A second decommission is a no-op. - let outcome = datastore - .rendezvous_sled_bp_availability_decommission( - opctx, - RendezvousSledBpAvailabilityDecommission::new(sled_id, bp), - ) - .await - .expect("query succeeded"); + let outcome = decommission_one(opctx, datastore, sled_id, bp).await; assert_eq!( outcome, SledBpAvailabilityDecommissionOutcome::AlreadyDecommissioned @@ -647,13 +673,15 @@ mod tests { // A stale Nexus must not be able to resurrect a decommissioned sled, // even at a newer generation. - let outcome = datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - row(sled_id, ActiveSledBpAvailability::Available, 5, bp), - ) - .await - .expect("query succeeded"); + let outcome = upsert_one( + opctx, + datastore, + sled_id, + ActiveSledBpAvailability::Available, + 5, + bp, + ) + .await; assert_eq!( outcome, SledBpAvailabilityUpsertOutcome::Rejected, @@ -683,13 +711,7 @@ mod tests { let bp = BlueprintUuid::new_v4(); let bp_stale = BlueprintUuid::new_v4(); - let outcome = datastore - .rendezvous_sled_bp_availability_decommission( - opctx, - RendezvousSledBpAvailabilityDecommission::new(sled_id, bp), - ) - .await - .expect("query succeeded"); + let outcome = decommission_one(opctx, datastore, sled_id, bp).await; assert_eq!( outcome, SledBpAvailabilityDecommissionOutcome::Decommissioned, @@ -702,13 +724,15 @@ mod tests { ); assert_eq!(got.blueprint_id(), bp); - let outcome = datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - row(sled_id, ActiveSledBpAvailability::Available, 5, bp_stale), - ) - .await - .expect("query succeeded"); + let outcome = upsert_one( + opctx, + datastore, + sled_id, + ActiveSledBpAvailability::Available, + 5, + bp_stale, + ) + .await; assert_eq!( outcome, SledBpAvailabilityUpsertOutcome::Rejected, diff --git a/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs b/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs index ae8662587b0..abc86b644f6 100644 --- a/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs +++ b/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs @@ -263,8 +263,7 @@ mod tests { use crate::tests::usize_to_id; use async_bb8_diesel::AsyncRunQueryDsl; use async_bb8_diesel::AsyncSimpleConnection; - use nexus_db_queries::db::model::RendezvousSledBpAvailabilityDecommission; - use nexus_db_queries::db::model::RendezvousSledBpAvailabilityUpdate; + use iddqd::id_ord_map; use nexus_db_queries::db::pub_test_utils::TestDatabase; use nexus_db_queries::db::queries::ALLOW_FULL_TABLE_SCAN_SQL; use omicron_generation_kinds::UpdateDispositionGeneration; @@ -305,6 +304,15 @@ mod tests { } impl DbPrep { + fn state(self) -> SledBpAvailabilityState { + match self { + DbPrep::Active(ps) => ps.state(), + DbPrep::Decommissioned => { + SledBpAvailabilityState::Decommissioned + } + } + } + async fn insert( self, opctx: &OpContext, @@ -312,36 +320,19 @@ mod tests { sled_id: SledUuid, blueprint_id: BlueprintUuid, ) { - match self { - DbPrep::Active(ps) => { - datastore - .rendezvous_sled_bp_availability_upsert( - opctx, - RendezvousSledBpAvailabilityUpdate::new( - sled_id, - ps.availability, - UpdateDispositionGeneration::from( - ps.generation, - ), - blueprint_id, - ), - ) - .await - .expect("query succeeded"); - } - DbPrep::Decommissioned => { - datastore - .rendezvous_sled_bp_availability_decommission( - opctx, - RendezvousSledBpAvailabilityDecommission::new( - sled_id, - blueprint_id, - ), - ) - .await - .expect("query succeeded"); - } - } + datastore + .rendezvous_sled_bp_availability_write( + opctx, + blueprint_id, + id_ord_map! { + SledBlueprintAvailabilityInput { + sled_id, + state: self.state(), + }, + }, + ) + .await + .expect("query succeeded"); } } @@ -615,14 +606,19 @@ mod tests { let bp_reconciled = BlueprintUuid::new_v4(); datastore - .rendezvous_sled_bp_availability_upsert( + .rendezvous_sled_bp_availability_write( opctx, - RendezvousSledBpAvailabilityUpdate::new( - sled_id, - ActiveSledBpAvailability::Available, - UpdateDispositionGeneration::from(2u32), - bp_stored, - ), + bp_stored, + id_ord_map! { + SledBlueprintAvailabilityInput { + sled_id, + state: SledBpAvailabilityState::Active { + availability: ActiveSledBpAvailability::Available, + update_disposition_generation: + UpdateDispositionGeneration::from(2u32), + }, + }, + }, ) .await .expect("seeded the stored row"); @@ -631,15 +627,16 @@ mod tests { opctx, datastore, bp_reconciled, - IdOrdMap::from_iter_unique([SledBlueprintAvailabilityInput { - sled_id, - state: SledBpAvailabilityState::Active { - availability: ActiveSledBpAvailability::Unavailable, - update_disposition_generation: - UpdateDispositionGeneration::from(2u32), + id_ord_map! { + SledBlueprintAvailabilityInput { + sled_id, + state: SledBpAvailabilityState::Active { + availability: ActiveSledBpAvailability::Unavailable, + update_disposition_generation: + UpdateDispositionGeneration::from(2u32), + }, }, - }]) - .expect("a single input is trivially unique"), + }, ) .await .expect("reconciled sled availability"); From b09ff355480dbb244e16177eaa0131f40df39b34 Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 3 Sep 2026 19:30:17 -0700 Subject: [PATCH 3/3] cleanup Created using spr 1.3.6-beta.1 --- Cargo.lock | 1 + .../rendezvous_sled_bp_availability.rs | 37 ++++---- nexus/reconfigurator/rendezvous/Cargo.toml | 1 + .../src/sled_blueprint_availability.rs | 86 +++++++++++-------- 4 files changed, 65 insertions(+), 60 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8e905375f73..d65c05264e3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8032,6 +8032,7 @@ dependencies = [ "serde", "sled-agent-types", "slog", + "slog-error-chain 0.1.0 (git+https://github.com/oxidecomputer/slog-error-chain?branch=main)", "test-strategy", "tokio", "uuid", diff --git a/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs b/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs index a23e5e57f02..7a58a6125e1 100644 --- a/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs +++ b/nexus/db-queries/src/db/datastore/rendezvous_sled_bp_availability.rs @@ -168,13 +168,14 @@ impl SledBpAvailabilityWrite { pub enum SledBpAvailabilityWriteOutcome { /// The sled is active in the blueprint. Active { - /// The availability written to the database. + /// The availability requested to be written to the database. availability: ActiveSledBpAvailability, - /// The generation of the update disposition that was written. + /// The generation of the update disposition that was requested to be + /// written. update_disposition_generation: UpdateDispositionGeneration, - /// Whether the upsert operation was successful. + /// Whether the write was successful. outcome: SledBpAvailabilityUpsertOutcome, }, @@ -338,9 +339,7 @@ impl DataStore { outcome, }) .map_err(|e| { - e.internal_context(format!( - "failed to upsert availability for sled {sled_id}" - )) + e.internal_context("failed to upsert availability") }) } SledBpAvailabilityState::Decommissioned => { @@ -353,11 +352,7 @@ impl DataStore { ) .await .map(SledBpAvailabilityWriteOutcome::Decommission) - .map_err(|e| { - e.internal_context(format!( - "failed to decommission sled {sled_id}" - )) - }) + .map_err(|e| e.internal_context("failed to decommission sled")) } }; let outcome = match result { @@ -1066,7 +1061,7 @@ mod tests { expected_completed: IdOrdMap, expected_failed_sled_id: SledUuid, expected_num_not_attempted: usize, - expected_context: String, + expected_context: &'static str, expected_absent: Vec, } @@ -1107,9 +1102,7 @@ mod tests { }, expected_failed_sled_id: rejected_upsert, expected_num_not_attempted: 2, - expected_context: format!( - "failed to upsert availability for sled {rejected_upsert}" - ), + expected_context: "failed to upsert availability", expected_absent: vec![rejected_upsert, sled(4), sled(5)], }, Case { @@ -1144,9 +1137,7 @@ mod tests { }, expected_failed_sled_id: rejected_decommission, expected_num_not_attempted: 1, - expected_context: format!( - "failed to decommission sled {rejected_decommission}" - ), + expected_context: "failed to decommission sled", expected_absent: vec![rejected_decommission, sled(14)], }, ]; @@ -1175,12 +1166,14 @@ mod tests { ); match error { Error::InternalError { internal_message } => { + let expected_prefix = format!( + "{}: unexpected database error: ", + case.expected_context + ); assert!( - internal_message - .contains(&case.expected_context), + internal_message.starts_with(&expected_prefix), "{name}: internal message {internal_message:?} \ - must contain {:?}", - case.expected_context, + must start with {expected_prefix:?}", ); assert!( internal_message.contains("CHECK constraint"), diff --git a/nexus/reconfigurator/rendezvous/Cargo.toml b/nexus/reconfigurator/rendezvous/Cargo.toml index bb089105db1..5188a2fa68e 100644 --- a/nexus/reconfigurator/rendezvous/Cargo.toml +++ b/nexus/reconfigurator/rendezvous/Cargo.toml @@ -19,6 +19,7 @@ omicron-uuid-kinds.workspace = true serde.workspace = true sled-agent-types.workspace = true slog.workspace = true +slog-error-chain.workspace = true # See omicron-rpaths for more about the "pq-sys" dependency. This is needed # because we use the database in the test suite, though it doesn't appear to diff --git a/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs b/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs index 89fb27d0947..6b4af2505e7 100644 --- a/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs +++ b/nexus/reconfigurator/rendezvous/src/sled_blueprint_availability.rs @@ -18,6 +18,7 @@ use nexus_types::internal_api::background::SledBlueprintAvailabilityRendezvousSt use omicron_uuid_kinds::BlueprintUuid; use slog::error; use slog::info; +use slog_error_chain::InlineErrorChain; /// Reconcile the `rendezvous_sled_bp_availability` table against the target /// blueprint. @@ -157,38 +158,50 @@ pub(crate) async fn reconcile_sled_blueprint_availability( ); } + let num_to_write = to_write.len(); let writes = match datastore .rendezvous_sled_bp_availability_write(opctx, blueprint_id, to_write) .await { Ok(writes) => writes, - Err(SledBpAvailabilityWriteError::Failed { - completed, - failed_sled_id, - num_not_attempted, - error, - }) => { - for write in &completed { - write.log_to_and_count(&opctx.log, &mut stats); + Err(err) => { + match &err { + SledBpAvailabilityWriteError::Failed { + completed, + failed_sled_id, + num_not_attempted, + error, + } => { + for write in completed { + write.log_to_and_count(&opctx.log, &mut stats); + } + error!( + opctx.log, + "sled availability write failed partway; writes \ + completed before the failure are counted here"; + "blueprint_id" => %blueprint_id, + "failed_sled_id" => %failed_sled_id, + "num_not_attempted" => num_not_attempted, + InlineErrorChain::new(error), + &stats, + ); + } + SledBpAvailabilityWriteError::NotStarted(error) => { + error!( + opctx.log, + "sled availability write did not start; no rows \ + were touched"; + "blueprint_id" => %blueprint_id, + "num_to_write" => num_to_write, + InlineErrorChain::new(error), + &stats, + ); + } } - error!( - opctx.log, - "sled availability write failed partway; writes completed \ - before the failure are counted here"; - "blueprint_id" => %blueprint_id, - "failed_sled_id" => %failed_sled_id, - "num_not_attempted" => num_not_attempted, - "error" => %error, - &stats, - ); - return Err(anyhow::Error::from(error).context(format!( - "failed to write availability for sled {failed_sled_id} \ - ({num_not_attempted} sled(s) not attempted)", - ))); - } - Err(err @ SledBpAvailabilityWriteError::NotStarted(_)) => { - return Err(anyhow::Error::from(err) - .context("failed to write sled availability")); + // Do not add `.context()` on this error, since + // SledBpAvailabilityWriteError's Display already produces a + // complete message. + return Err(anyhow::Error::from(err)); } }; for write in &writes { @@ -612,18 +625,15 @@ mod tests { .await .expect_err("the injected constraint fails the pass"); let message = format!("{err:#}"); - for needle in [ - format!( - "failed to write availability for sled {rejected} \ - (2 sled(s) not attempted)" - ), - format!("failed to upsert availability for sled {rejected}"), - ] { - assert!( - message.contains(&needle), - "error {message:?} must contain {needle:?}" - ); - } + let expected_prefix = format!( + "failed to write availability for sled {rejected} after 2 \ + write(s) completed (2 not attempted): Internal Error: failed to \ + upsert availability: unexpected database error: " + ); + assert!( + message.starts_with(&expected_prefix), + "error {message:?} must start with {expected_prefix:?}" + ); let rows = datastore .rendezvous_sled_bp_availability_list_all_batched(opctx)