From adec83777954f3b48f5034e2cbe4a48b04a0fde0 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 29 Apr 2024 14:38:56 -0400 Subject: [PATCH 01/17] Add sled_state map to blueprints (not yet in DB serialization) --- .../db-queries/src/db/datastore/deployment.rs | 1 + nexus/db-queries/src/db/datastore/rack.rs | 12 +++++++++ nexus/db-queries/src/db/datastore/vpc.rs | 12 +++++++++ nexus/reconfigurator/execution/src/dns.rs | 7 ++++++ .../execution/src/omicron_physical_disks.rs | 1 + .../execution/src/omicron_zones.rs | 1 + .../planning/src/blueprint_builder/builder.rs | 25 +++++++++++++++++++ .../src/app/background/blueprint_execution.rs | 8 ++++++ nexus/src/app/background/blueprint_load.rs | 1 + nexus/test-utils/src/lib.rs | 7 +++++- nexus/types/src/deployment.rs | 4 +++ sled-agent/src/rack_setup/service.rs | 4 +++ 12 files changed, 82 insertions(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 97f7296368..366feb6780 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -550,6 +550,7 @@ impl DataStore { id: blueprint_id, blueprint_zones, blueprint_disks, + sled_state: BTreeMap::new(), // TODO-john FIXME parent_blueprint_id, internal_dns_version, external_dns_version, diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 1e1b896d3a..218c05c005 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -964,6 +964,7 @@ mod test { BlueprintZoneDisposition, OmicronZoneExternalSnatIp, }; use nexus_types::external_api::shared::SiloIdentityMode; + use nexus_types::external_api::views::SledState; use nexus_types::identity::Asset; use nexus_types::internal_api::params::DnsRecord; use nexus_types::inventory::NetworkInterface; @@ -996,6 +997,7 @@ mod test { id: Uuid::new_v4(), blueprint_zones: BTreeMap::new(), blueprint_disks: BTreeMap::new(), + sled_state: BTreeMap::new(), parent_blueprint_id: None, internal_dns_version: *Generation::new(), external_dns_version: *Generation::new(), @@ -1234,6 +1236,12 @@ mod test { } } + fn sled_states_active( + sled_ids: impl Iterator, + ) -> BTreeMap { + sled_ids.map(|sled_id| (sled_id, SledState::Active)).collect() + } + #[tokio::test] async fn rack_set_initialized_with_services() { let test_name = "rack_set_initialized_with_services"; @@ -1466,6 +1474,7 @@ mod test { } let blueprint = Blueprint { id: Uuid::new_v4(), + sled_state: sled_states_active(blueprint_zones.keys().copied()), blueprint_zones, blueprint_disks: BTreeMap::new(), parent_blueprint_id: None, @@ -1721,6 +1730,7 @@ mod test { } let blueprint = Blueprint { id: Uuid::new_v4(), + sled_state: sled_states_active(blueprint_zones.keys().copied()), blueprint_zones, blueprint_disks: BTreeMap::new(), parent_blueprint_id: None, @@ -1932,6 +1942,7 @@ mod test { } let blueprint = Blueprint { id: Uuid::new_v4(), + sled_state: sled_states_active(blueprint_zones.keys().copied()), blueprint_zones, blueprint_disks: BTreeMap::new(), parent_blueprint_id: None, @@ -2070,6 +2081,7 @@ mod test { } let blueprint = Blueprint { id: Uuid::new_v4(), + sled_state: sled_states_active(blueprint_zones.keys().copied()), blueprint_zones, blueprint_disks: BTreeMap::new(), parent_blueprint_id: None, diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 632ea501e3..23d811fd43 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -1245,6 +1245,7 @@ mod tests { use nexus_types::deployment::BlueprintZonesConfig; use nexus_types::deployment::OmicronZoneExternalFloatingIp; use nexus_types::external_api::params; + use nexus_types::external_api::views::SledState; use nexus_types::identity::Asset; use omicron_common::address::NEXUS_OPTE_IPV4_SUBNET; use omicron_common::api::external; @@ -1603,6 +1604,12 @@ mod tests { } } + fn sled_states_active( + sled_ids: impl Iterator, + ) -> BTreeMap { + sled_ids.map(|sled_id| (sled_id, SledState::Active)).collect() + } + async fn assert_service_sled_ids( datastore: &DataStore, expected_sled_ids: &[SledUuid], @@ -1659,6 +1666,7 @@ mod tests { let bp1_id = Uuid::new_v4(); let bp1 = Blueprint { id: bp1_id, + sled_state: sled_states_active(bp1_zones.keys().copied()), blueprint_zones: bp1_zones, blueprint_disks: BTreeMap::new(), parent_blueprint_id: None, @@ -1711,6 +1719,7 @@ mod tests { let bp2_id = Uuid::new_v4(); let bp2 = Blueprint { id: bp2_id, + sled_state: BTreeMap::new(), blueprint_zones: BTreeMap::new(), blueprint_disks: BTreeMap::new(), parent_blueprint_id: Some(bp1_id), @@ -1771,6 +1780,7 @@ mod tests { let bp3_id = Uuid::new_v4(); let bp3 = Blueprint { id: bp3_id, + sled_state: sled_states_active(bp3_zones.keys().copied()), blueprint_zones: bp3_zones, blueprint_disks: BTreeMap::new(), parent_blueprint_id: Some(bp2_id), @@ -1831,6 +1841,7 @@ mod tests { let bp4_id = Uuid::new_v4(); let bp4 = Blueprint { id: bp4_id, + sled_state: sled_states_active(bp4_zones.keys().copied()), blueprint_zones: bp4_zones, blueprint_disks: BTreeMap::new(), parent_blueprint_id: Some(bp3_id), @@ -1939,6 +1950,7 @@ mod tests { let bp5_id = Uuid::new_v4(); let bp5 = Blueprint { id: bp5_id, + sled_state: sled_states_active(bp5_zones.keys().copied()), blueprint_zones: bp5_zones, blueprint_disks: BTreeMap::new(), parent_blueprint_id: Some(bp4_id), diff --git a/nexus/reconfigurator/execution/src/dns.rs b/nexus/reconfigurator/execution/src/dns.rs index e0105f1f64..6a3c1755cf 100644 --- a/nexus/reconfigurator/execution/src/dns.rs +++ b/nexus/reconfigurator/execution/src/dns.rs @@ -480,6 +480,7 @@ mod test { use nexus_types::deployment::SledFilter; use nexus_types::external_api::params; use nexus_types::external_api::shared; + use nexus_types::external_api::views::SledState; use nexus_types::identity::Resource; use nexus_types::internal_api::params::DnsConfigParams; use nexus_types::internal_api::params::DnsConfigZone; @@ -557,6 +558,10 @@ mod test { // `BlueprintZonesConfig`. This is going to get more painful over time // as we add to blueprints, but for now we can make this work. let mut blueprint_zones = BTreeMap::new(); + + // Also assume any sled in the collection is active. + let mut sled_state = BTreeMap::new(); + for (sled_id, zones_config) in collection.omicron_zones { blueprint_zones.insert( sled_id, @@ -580,6 +585,7 @@ mod test { .collect(), }, ); + sled_state.insert(sled_id, SledState::Active); } let dns_empty = dns_config_empty(); @@ -589,6 +595,7 @@ mod test { id: Uuid::new_v4(), blueprint_zones, blueprint_disks: BTreeMap::new(), + sled_state, parent_blueprint_id: None, internal_dns_version: initial_dns_generation, external_dns_version: Generation::new(), diff --git a/nexus/reconfigurator/execution/src/omicron_physical_disks.rs b/nexus/reconfigurator/execution/src/omicron_physical_disks.rs index a90b3c8e59..89287713c2 100644 --- a/nexus/reconfigurator/execution/src/omicron_physical_disks.rs +++ b/nexus/reconfigurator/execution/src/omicron_physical_disks.rs @@ -136,6 +136,7 @@ mod test { id, blueprint_zones: BTreeMap::new(), blueprint_disks, + sled_state: BTreeMap::new(), parent_blueprint_id: None, internal_dns_version: Generation::new(), external_dns_version: Generation::new(), diff --git a/nexus/reconfigurator/execution/src/omicron_zones.rs b/nexus/reconfigurator/execution/src/omicron_zones.rs index da74023841..3248269175 100644 --- a/nexus/reconfigurator/execution/src/omicron_zones.rs +++ b/nexus/reconfigurator/execution/src/omicron_zones.rs @@ -126,6 +126,7 @@ mod test { id, blueprint_zones, blueprint_disks: BTreeMap::new(), + sled_state: BTreeMap::new(), parent_blueprint_id: None, internal_dns_version: Generation::new(), external_dns_version: Generation::new(), diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index dc8b1452a7..0d187af50c 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -30,6 +30,7 @@ use nexus_types::deployment::PlanningInput; use nexus_types::deployment::SledFilter; use nexus_types::deployment::SledResources; use nexus_types::deployment::ZpoolName; +use nexus_types::external_api::views::SledState; use omicron_common::address::get_internal_dns_server_addresses; use omicron_common::address::get_sled_address; use omicron_common::address::get_switch_zone_address; @@ -141,6 +142,7 @@ pub struct BlueprintBuilder<'a> { // corresponding fields in `Blueprint`. pub(super) zones: BlueprintZonesBuilder<'a>, disks: BlueprintDisksBuilder<'a>, + sled_state: BTreeMap, creator: String, comments: Vec, @@ -200,10 +202,16 @@ impl<'a> BlueprintBuilder<'a> { }) .collect::>(); let num_sleds = blueprint_zones.len(); + let sled_state = blueprint_zones + .keys() + .copied() + .map(|sled_id| (sled_id, SledState::Active)) + .collect(); Blueprint { id: rng.blueprint_rng.next(), blueprint_zones, blueprint_disks: BTreeMap::new(), + sled_state, parent_blueprint_id: None, internal_dns_version: Generation::new(), external_dns_version: Generation::new(), @@ -332,6 +340,21 @@ impl<'a> BlueprintBuilder<'a> { let available_system_macs = AvailableIterator::new(MacAddr::iter_system(), used_macs); + let sled_state = input + .all_sleds(SledFilter::All) + .map(|(sled_id, details)| { + // Prefer the sled state from our parent blueprint for sleds + // that were in it; there may be new sleds in `input`, in which + // case we'll use their current state as our starting point. + let state = parent_blueprint + .sled_state + .get(&sled_id) + .copied() + .unwrap_or(details.state); + (sled_id, state) + }) + .collect(); + Ok(BlueprintBuilder { log, parent_blueprint, @@ -339,6 +362,7 @@ impl<'a> BlueprintBuilder<'a> { sled_ip_allocators: BTreeMap::new(), zones: BlueprintZonesBuilder::new(parent_blueprint), disks: BlueprintDisksBuilder::new(parent_blueprint), + sled_state, creator: creator.to_owned(), comments: Vec::new(), nexus_v4_ips, @@ -362,6 +386,7 @@ impl<'a> BlueprintBuilder<'a> { id: self.rng.blueprint_rng.next(), blueprint_zones, blueprint_disks, + sled_state: self.sled_state, parent_blueprint_id: Some(self.parent_blueprint.id), internal_dns_version: self.input.internal_dns_version(), external_dns_version: self.input.external_dns_version(), diff --git a/nexus/src/app/background/blueprint_execution.rs b/nexus/src/app/background/blueprint_execution.rs index 5a92d36895..1291e72a9b 100644 --- a/nexus/src/app/background/blueprint_execution.rs +++ b/nexus/src/app/background/blueprint_execution.rs @@ -125,6 +125,7 @@ mod test { BlueprintTarget, BlueprintZoneConfig, BlueprintZoneDisposition, BlueprintZoneType, BlueprintZonesConfig, }; + use nexus_types::external_api::views::SledState; use nexus_types::inventory::OmicronZoneDataset; use omicron_common::api::external::Generation; use omicron_uuid_kinds::GenericUuid; @@ -147,6 +148,12 @@ mod test { dns_version: Generation, ) -> (BlueprintTarget, Blueprint) { let id = Uuid::new_v4(); + // Assume all sleds are active. + let sled_state = blueprint_zones + .keys() + .copied() + .map(|sled_id| (sled_id, SledState::Active)) + .collect::>(); ( BlueprintTarget { target_id: id, @@ -157,6 +164,7 @@ mod test { id, blueprint_zones, blueprint_disks, + sled_state, parent_blueprint_id: None, internal_dns_version: dns_version, external_dns_version: dns_version, diff --git a/nexus/src/app/background/blueprint_load.rs b/nexus/src/app/background/blueprint_load.rs index 9c13d8e70a..8334abecb5 100644 --- a/nexus/src/app/background/blueprint_load.rs +++ b/nexus/src/app/background/blueprint_load.rs @@ -211,6 +211,7 @@ mod test { id, blueprint_zones: BTreeMap::new(), blueprint_disks: BTreeMap::new(), + sled_state: BTreeMap::new(), parent_blueprint_id: Some(parent_blueprint_id), internal_dns_version: Generation::new(), external_dns_version: Generation::new(), diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs index d7ebddd330..6f41aee860 100644 --- a/nexus/test-utils/src/lib.rs +++ b/nexus/test-utils/src/lib.rs @@ -33,6 +33,7 @@ use nexus_types::deployment::BlueprintZonesConfig; use nexus_types::deployment::OmicronZoneExternalFloatingAddr; use nexus_types::deployment::OmicronZoneExternalFloatingIp; use nexus_types::external_api::params::UserId; +use nexus_types::external_api::views::SledState; use nexus_types::internal_api::params::Certificate; use nexus_types::internal_api::params::DatasetCreateRequest; use nexus_types::internal_api::params::DatasetKind; @@ -751,18 +752,21 @@ impl<'a, N: NexusServer> ControlPlaneTestContextBuilder<'a, N> { let blueprint = { let mut blueprint_zones = BTreeMap::new(); + let mut sled_state = BTreeMap::new(); for (maybe_sled_agent, zones) in [ (self.sled_agent.as_ref(), &self.blueprint_zones), (self.sled_agent2.as_ref(), &self.blueprint_zones2), ] { if let Some(sa) = maybe_sled_agent { + let sled_id = SledUuid::from_untyped_uuid(sa.sled_agent.id); blueprint_zones.insert( - SledUuid::from_untyped_uuid(sa.sled_agent.id), + sled_id, BlueprintZonesConfig { generation: Generation::new().next(), zones: zones.clone(), }, ); + sled_state.insert(sled_id, SledState::Active); } } Blueprint { @@ -773,6 +777,7 @@ impl<'a, N: NexusServer> ControlPlaneTestContextBuilder<'a, N> { // // However, for now, this isn't necessary. blueprint_disks: BTreeMap::new(), + sled_state, parent_blueprint_id: None, internal_dns_version: dns_config .generation diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index b818b99794..f7d89ec414 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -12,6 +12,7 @@ //! nexus/db-model, but nexus/reconfigurator/planning does not currently know //! about nexus/db-model and it's convenient to separate these concerns.) +use crate::external_api::views::SledState; use crate::internal_api::params::DnsConfigParams; use crate::inventory::Collection; pub use crate::inventory::OmicronZoneConfig; @@ -115,6 +116,9 @@ pub struct Blueprint { /// A map of sled id -> disks in use on each sled. pub blueprint_disks: BTreeMap, + /// A map of sled id -> state of the sled. + pub sled_state: BTreeMap, + /// which blueprint this blueprint is based on pub parent_blueprint_id: Option, diff --git a/sled-agent/src/rack_setup/service.rs b/sled-agent/src/rack_setup/service.rs index f590428244..1abdac6284 100644 --- a/sled-agent/src/rack_setup/service.rs +++ b/sled-agent/src/rack_setup/service.rs @@ -95,6 +95,7 @@ use nexus_types::deployment::{ Blueprint, BlueprintPhysicalDisksConfig, BlueprintZoneConfig, BlueprintZoneDisposition, BlueprintZonesConfig, InvalidOmicronZoneType, }; +use nexus_types::external_api::views::SledState; use omicron_common::address::get_sled_address; use omicron_common::api::external::Generation; use omicron_common::api::internal::shared::ExternalPortDiscovery; @@ -1375,6 +1376,7 @@ pub(crate) fn build_initial_blueprint_from_sled_configs( } let mut blueprint_zones = BTreeMap::new(); + let mut sled_state = BTreeMap::new(); for (sled_id, sled_config) in sled_configs_by_id { let zones_config = BlueprintZonesConfig { // This is a bit of a hack. We only construct a blueprint after @@ -1396,12 +1398,14 @@ pub(crate) fn build_initial_blueprint_from_sled_configs( }; blueprint_zones.insert(*sled_id, zones_config); + sled_state.insert(*sled_id, SledState::Active); } Ok(Blueprint { id: Uuid::new_v4(), blueprint_zones, blueprint_disks, + sled_state, parent_blueprint_id: None, internal_dns_version, // We don't configure external DNS during RSS, so set it to an initial From ab67000bfc125f656b9dcb94a51442f418510637 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 29 Apr 2024 16:45:36 -0400 Subject: [PATCH 02/17] update blueprint db serialization --- nexus/db-model/src/deployment.rs | 15 +++- nexus/db-model/src/schema.rs | 9 +++ nexus/db-model/src/schema_versions.rs | 3 +- .../db-queries/src/db/datastore/deployment.rs | 74 ++++++++++++++++++- schema/crdb/blueprint-add-sled-state/up.sql | 6 ++ schema/crdb/dbinit.sql | 12 ++- 6 files changed, 113 insertions(+), 6 deletions(-) create mode 100644 schema/crdb/blueprint-add-sled-state/up.sql diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index 5a9d6af810..0b766f9e9b 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -9,11 +9,13 @@ use crate::inventory::ZoneType; use crate::omicron_zone_config::{OmicronZone, OmicronZoneNic}; use crate::schema::{ blueprint, bp_omicron_physical_disk, bp_omicron_zone, bp_omicron_zone_nic, - bp_sled_omicron_physical_disks, bp_sled_omicron_zones, bp_target, + bp_sled_omicron_physical_disks, bp_sled_omicron_zones, bp_sled_state, + bp_target, }; use crate::typed_uuid::DbTypedUuid; use crate::{ - impl_enum_type, ipv6, Generation, MacAddr, Name, SqlU16, SqlU32, SqlU8, + impl_enum_type, ipv6, Generation, MacAddr, Name, SledState, SqlU16, SqlU32, + SqlU8, }; use chrono::{DateTime, Utc}; use ipnetwork::IpNetwork; @@ -103,6 +105,15 @@ impl From for nexus_types::deployment::BlueprintTarget { } } +/// See [`nexus_types::deployment::Blueprint::sled_state`]. +#[derive(Queryable, Clone, Debug, Selectable, Insertable)] +#[diesel(table_name = bp_sled_state)] +pub struct BpSledState { + pub blueprint_id: Uuid, + pub sled_id: DbTypedUuid, + pub sled_state: SledState, +} + /// See [`nexus_types::deployment::BlueprintPhysicalDisksConfig`]. #[derive(Queryable, Clone, Debug, Selectable, Insertable)] #[diesel(table_name = bp_sled_omicron_physical_disks)] diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index fa03aca4fb..ef0db97277 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1459,6 +1459,15 @@ table! { } } +table! { + bp_sled_state (blueprint_id, sled_id) { + blueprint_id -> Uuid, + sled_id -> Uuid, + + sled_state -> crate::SledStateEnum, + } +} + table! { bp_sled_omicron_physical_disks (blueprint_id, sled_id) { blueprint_id -> Uuid, diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 753d2a8479..796f81314c 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(55, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(56, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy> = Lazy::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(56, "blueprint-add-sled-state"), KnownVersion::new(55, "add-lookup-sled-by-policy-and-state-index"), KnownVersion::new(54, "blueprint-add-external-ip-id"), KnownVersion::new(53, "drop-service-table"), diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 366feb6780..6fdeed9ee5 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -40,12 +40,14 @@ use nexus_db_model::BpOmicronZone; use nexus_db_model::BpOmicronZoneNic; use nexus_db_model::BpSledOmicronPhysicalDisks; use nexus_db_model::BpSledOmicronZones; +use nexus_db_model::BpSledState; use nexus_db_model::BpTarget; use nexus_types::deployment::Blueprint; use nexus_types::deployment::BlueprintMetadata; use nexus_types::deployment::BlueprintPhysicalDisksConfig; use nexus_types::deployment::BlueprintTarget; use nexus_types::deployment::BlueprintZonesConfig; +use nexus_types::external_api::views::SledState; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; @@ -109,6 +111,16 @@ impl DataStore { let row_blueprint = DbBlueprint::from(blueprint); let blueprint_id = row_blueprint.id; + let sled_states = blueprint + .sled_state + .iter() + .map(|(&sled_id, &state)| BpSledState { + blueprint_id, + sled_id: sled_id.into(), + sled_state: state.into(), + }) + .collect::>(); + let sled_omicron_physical_disks = blueprint .blueprint_disks .iter() @@ -187,6 +199,16 @@ impl DataStore { .await?; } + // Insert all the sled states for this blueprint. + { + use db::schema::bp_sled_state::dsl as sled_state; + + let _ = diesel::insert_into(sled_state::bp_sled_state) + .values(sled_states) + .execute_async(&conn) + .await?; + } + // Insert all physical disks for this blueprint. { @@ -290,6 +312,41 @@ impl DataStore { ) }; + // Load the sled states for this blueprint. + let sled_state: BTreeMap = { + use db::schema::bp_sled_state::dsl; + + let mut sled_state = BTreeMap::new(); + let mut paginator = Paginator::new(SQL_BATCH_SIZE); + while let Some(p) = paginator.next() { + let batch = paginated( + dsl::bp_sled_state, + dsl::sled_id, + &p.current_pagparams(), + ) + .filter(dsl::blueprint_id.eq(blueprint_id)) + .select(BpSledState::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + paginator = p.found_batch(&batch, &|s| s.sled_id); + + for s in batch { + let old = sled_state + .insert(s.sled_id.into(), s.sled_state.into()); + bail_unless!( + old.is_none(), + "found duplicate sled ID in bp_sled_state: {}", + s.sled_id + ); + } + } + sled_state + }; + // Read this blueprint's `bp_sled_omicron_zones` rows, which describes // the `OmicronZonesConfig` generation number for each sled that is a // part of this blueprint. Construct the BTreeMap we ultimately need, @@ -550,7 +607,7 @@ impl DataStore { id: blueprint_id, blueprint_zones, blueprint_disks, - sled_state: BTreeMap::new(), // TODO-john FIXME + sled_state, parent_blueprint_id, internal_dns_version, external_dns_version, @@ -579,6 +636,7 @@ impl DataStore { let ( nblueprints, + nsled_states, nsled_physical_disks, nphysical_disks, nsled_agent_zones, @@ -618,6 +676,17 @@ impl DataStore { )); } + // Remove rows associated with sled states. + let nsled_states = { + use db::schema::bp_sled_state::dsl; + diesel::delete( + dsl::bp_sled_state + .filter(dsl::blueprint_id.eq(blueprint_id)), + ) + .execute_async(&conn) + .await? + }; + // Remove rows associated with Omicron physical disks let nsled_physical_disks = { use db::schema::bp_sled_omicron_physical_disks::dsl; @@ -671,6 +740,7 @@ impl DataStore { Ok(( nblueprints, + nsled_states, nsled_physical_disks, nphysical_disks, nsled_agent_zones, @@ -689,6 +759,7 @@ impl DataStore { info!(&opctx.log, "removed blueprint"; "blueprint_id" => blueprint_id.to_string(), "nblueprints" => nblueprints, + "nsled_states" => nsled_states, "nsled_physical_disks" => nsled_physical_disks, "nphysical_disks" => nphysical_disks, "nsled_agent_zones" => nsled_agent_zones, @@ -1268,7 +1339,6 @@ mod tests { 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::inventory::Collection; use omicron_common::address::Ipv6Subnet; use omicron_common::disk::DiskIdentity; diff --git a/schema/crdb/blueprint-add-sled-state/up.sql b/schema/crdb/blueprint-add-sled-state/up.sql new file mode 100644 index 0000000000..855bc95ab4 --- /dev/null +++ b/schema/crdb/blueprint-add-sled-state/up.sql @@ -0,0 +1,6 @@ +CREATE TABLE IF NOT EXISTS omicron.public.bp_sled_state ( + blueprint_id UUID NOT NULL, + sled_id UUID NOT NULL, + sled_state omicron.public.sled_state NOT NULL, + PRIMARY KEY (blueprint_id, sled_id) +); diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index b42b63decc..ce711c4052 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3224,6 +3224,16 @@ CREATE TABLE IF NOT EXISTS omicron.public.bp_target ( time_made_target TIMESTAMPTZ NOT NULL ); +-- state of a sled in a blueprint +CREATE TABLE IF NOT EXISTS omicron.public.bp_sled_state ( + -- foreign key into `blueprint` table + blueprint_id UUID NOT NULL, + + sled_id UUID NOT NULL, + sled_state omicron.public.sled_state NOT NULL, + PRIMARY KEY (blueprint_id, sled_id) +); + -- description of a collection of omicron physical disks stored in a blueprint. CREATE TABLE IF NOT EXISTS omicron.public.bp_sled_omicron_physical_disks ( -- foreign key into `blueprint` table @@ -3771,7 +3781,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - ( TRUE, NOW(), NOW(), '55.0.0', NULL) + ( TRUE, NOW(), NOW(), '56.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From 6faa77e46c0e6f28855578d595e3ba4f5dc04d29 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 29 Apr 2024 16:47:23 -0400 Subject: [PATCH 03/17] expectorate openapi --- openapi/nexus-internal.json | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index ac0426eb2b..bfc67b1d62 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -1641,6 +1641,13 @@ "type": "string", "format": "uuid" }, + "sled_state": { + "description": "A map of sled id -> state of the sled.", + "type": "object", + "additionalProperties": { + "$ref": "#/components/schemas/SledState" + } + }, "time_created": { "description": "when this blueprint was generated (for debugging)", "type": "string", @@ -1655,6 +1662,7 @@ "external_dns_version", "id", "internal_dns_version", + "sled_state", "time_created" ] }, @@ -4479,6 +4487,25 @@ "sled" ] }, + "SledState": { + "description": "The current state of the sled, as determined by Nexus.", + "oneOf": [ + { + "description": "The sled is currently active, and has resources allocated on it.", + "type": "string", + "enum": [ + "active" + ] + }, + { + "description": "The sled has been permanently removed from service.\n\nThis is a terminal state: once a particular sled ID is decommissioned, it will never return to service. (The actual hardware may be reused, but it will be treated as a brand-new sled.)", + "type": "string", + "enum": [ + "decommissioned" + ] + } + ] + }, "SourceNatConfig": { "description": "An IP address and port range used for source NAT, i.e., making outbound network connections from guests or services.", "type": "object", From e985d91ceebb9e55423cd179c41f69d7de2242e2 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 29 Apr 2024 17:08:08 -0400 Subject: [PATCH 04/17] schema migration: backfill sled states for past blueprints --- .../blueprint-add-sled-state/{up.sql => up1.sql} | 0 schema/crdb/blueprint-add-sled-state/up2.sql | 13 +++++++++++++ 2 files changed, 13 insertions(+) rename schema/crdb/blueprint-add-sled-state/{up.sql => up1.sql} (100%) create mode 100644 schema/crdb/blueprint-add-sled-state/up2.sql diff --git a/schema/crdb/blueprint-add-sled-state/up.sql b/schema/crdb/blueprint-add-sled-state/up1.sql similarity index 100% rename from schema/crdb/blueprint-add-sled-state/up.sql rename to schema/crdb/blueprint-add-sled-state/up1.sql diff --git a/schema/crdb/blueprint-add-sled-state/up2.sql b/schema/crdb/blueprint-add-sled-state/up2.sql new file mode 100644 index 0000000000..e9c46fd063 --- /dev/null +++ b/schema/crdb/blueprint-add-sled-state/up2.sql @@ -0,0 +1,13 @@ +set local disallow_full_table_scans = off; + +-- All sleds are considered active (this migration is in support of +-- transitioning active-but-expunged sleds to 'decommissioned'). We'll fill in +-- this table for all historical blueprints by inserting rows for every sled +-- for which a given blueprint had a zone config with the state set to 'active'. +INSERT INTO bp_sled_state ( + SELECT DISTINCT + blueprint_id, + sled_id, + 'active'::sled_state + FROM bp_sled_omicron_zones +); From 0330f88d8f4c19cb716f5f007d6eccf1629578d1 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 1 May 2024 11:46:16 -0400 Subject: [PATCH 05/17] comment cleanup --- nexus/types/src/deployment.rs | 2 +- schema/crdb/blueprint-add-sled-state/up2.sql | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index f7d89ec414..fe013b8ca4 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -116,7 +116,7 @@ pub struct Blueprint { /// A map of sled id -> disks in use on each sled. pub blueprint_disks: BTreeMap, - /// A map of sled id -> state of the sled. + /// A map of sled id -> desired state of the sled. pub sled_state: BTreeMap, /// which blueprint this blueprint is based on diff --git a/schema/crdb/blueprint-add-sled-state/up2.sql b/schema/crdb/blueprint-add-sled-state/up2.sql index e9c46fd063..238870021f 100644 --- a/schema/crdb/blueprint-add-sled-state/up2.sql +++ b/schema/crdb/blueprint-add-sled-state/up2.sql @@ -1,9 +1,10 @@ set local disallow_full_table_scans = off; --- All sleds are considered active (this migration is in support of --- transitioning active-but-expunged sleds to 'decommissioned'). We'll fill in --- this table for all historical blueprints by inserting rows for every sled --- for which a given blueprint had a zone config with the state set to 'active'. +-- At this point in history, all sleds are considered active and this table only +-- exists to support transitioning active-but-expunged sleds to +-- 'decommissioned'. We'll fill in this table for all historical blueprints by +-- inserting rows for every sled for which a given blueprint had a zone config +-- with the state set to 'active'. INSERT INTO bp_sled_state ( SELECT DISTINCT blueprint_id, From ae4f77196658b62f37ea43d4f06d9de3e5212178 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 1 May 2024 12:46:16 -0400 Subject: [PATCH 06/17] expectorate comment update --- openapi/nexus-internal.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index bfc67b1d62..eec41b167f 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -1642,7 +1642,7 @@ "format": "uuid" }, "sled_state": { - "description": "A map of sled id -> state of the sled.", + "description": "A map of sled id -> desired state of the sled.", "type": "object", "additionalProperties": { "$ref": "#/components/schemas/SledState" From 4949eac70c7ccfda81d5fd8dea8704f19e4a0f65 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 1 May 2024 13:56:20 -0400 Subject: [PATCH 07/17] rework VPC/blueprint resolution test to use BlueprintBuilder --- nexus/db-queries/src/db/datastore/vpc.rs | 617 +++++++---------------- 1 file changed, 192 insertions(+), 425 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 23d811fd43..91843abf2e 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -1233,36 +1233,24 @@ mod tests { use crate::db::fixed_data::vpc_subnet::NEXUS_VPC_SUBNET; use crate::db::model::Project; use crate::db::queries::vpc::MAX_VNI_SEARCH_RANGE_SIZE; - use nexus_config::NUM_INITIAL_RESERVED_IP_ADDRESSES; + use nexus_db_model::IncompleteNetworkInterface; use nexus_db_model::SledUpdate; + use nexus_reconfigurator_planning::blueprint_builder::BlueprintBuilder; + use nexus_reconfigurator_planning::system::SledBuilder; + use nexus_reconfigurator_planning::system::SystemDescription; use nexus_test_utils::db::test_setup_database; - use nexus_types::deployment::blueprint_zone_type; use nexus_types::deployment::Blueprint; use nexus_types::deployment::BlueprintTarget; use nexus_types::deployment::BlueprintZoneConfig; use nexus_types::deployment::BlueprintZoneDisposition; - use nexus_types::deployment::BlueprintZoneType; - use nexus_types::deployment::BlueprintZonesConfig; - use nexus_types::deployment::OmicronZoneExternalFloatingIp; use nexus_types::external_api::params; - use nexus_types::external_api::views::SledState; use nexus_types::identity::Asset; - use omicron_common::address::NEXUS_OPTE_IPV4_SUBNET; use omicron_common::api::external; use omicron_common::api::external::Generation; - use omicron_common::api::external::IpNet; - use omicron_common::api::external::MacAddr; - use omicron_common::api::external::Vni; - use omicron_common::api::internal::shared::NetworkInterface; - use omicron_common::api::internal::shared::NetworkInterfaceKind; use omicron_test_utils::dev; - use omicron_uuid_kinds::ExternalIpUuid; use omicron_uuid_kinds::GenericUuid; - use omicron_uuid_kinds::OmicronZoneUuid; use omicron_uuid_kinds::SledUuid; use slog::info; - use std::collections::BTreeMap; - use std::net::IpAddr; // Test that we detect the right error condition and return None when we // fail to insert a VPC due to VNI exhaustion. @@ -1482,134 +1470,6 @@ mod tests { logctx.cleanup_successful(); } - #[derive(Debug)] - struct Harness { - rack_id: Uuid, - sled_ids: Vec, - nexuses: Vec, - } - - #[derive(Debug)] - struct HarnessNexus { - sled_id: SledUuid, - id: OmicronZoneUuid, - external_ip: OmicronZoneExternalFloatingIp, - mac: MacAddr, - nic_id: Uuid, - nic_ip: IpAddr, - } - - impl Harness { - fn new(num_sleds: usize) -> Self { - let mut sled_ids = - (0..num_sleds).map(|_| SledUuid::new_v4()).collect::>(); - sled_ids.sort(); - - // RFC 5737 TEST-NET-1 - let mut nexus_external_ips = - "192.0.2.0/24".parse::().unwrap().iter(); - let mut nexus_nic_ips = NEXUS_OPTE_IPV4_SUBNET - .iter() - .skip(NUM_INITIAL_RESERVED_IP_ADDRESSES) - .map(IpAddr::from); - let mut nexus_macs = MacAddr::iter_system(); - let nexuses = sled_ids - .iter() - .copied() - .map(|sled_id| HarnessNexus { - sled_id, - id: OmicronZoneUuid::new_v4(), - external_ip: OmicronZoneExternalFloatingIp { - id: ExternalIpUuid::new_v4(), - ip: nexus_external_ips.next().unwrap(), - }, - mac: nexus_macs.next().unwrap(), - nic_id: Uuid::new_v4(), - nic_ip: nexus_nic_ips.next().unwrap(), - }) - .collect::>(); - Self { rack_id: Uuid::new_v4(), sled_ids, nexuses } - } - - fn db_sleds(&self) -> impl Iterator + '_ { - self.sled_ids.iter().copied().map(|sled_id| { - SledUpdate::new( - sled_id.into_untyped_uuid(), - "[::1]:0".parse().unwrap(), - sled_baseboard_for_test(), - sled_system_hardware_for_test(), - self.rack_id, - Generation::new().into(), - ) - }) - } - - fn db_nics( - &self, - ) -> impl Iterator + '_ - { - self.nexuses.iter().map(|nexus| { - let name = format!("test-nexus-{}", nexus.id); - db::model::IncompleteNetworkInterface::new_service( - nexus.nic_id, - nexus.id.into_untyped_uuid(), - NEXUS_VPC_SUBNET.clone(), - IdentityMetadataCreateParams { - name: name.parse().unwrap(), - description: name, - }, - nexus.nic_ip, - nexus.mac, - 0, - ) - .expect("failed to create incomplete Nexus NIC") - }) - } - - fn blueprint_zone_configs( - &self, - ) -> impl Iterator + '_ - { - self.nexuses.iter().zip(self.db_nics()).map(|(nexus, nic)| { - let config = BlueprintZoneConfig { - disposition: BlueprintZoneDisposition::InService, - id: nexus.id, - underlay_address: "::1".parse().unwrap(), - zone_type: BlueprintZoneType::Nexus( - blueprint_zone_type::Nexus { - internal_address: "[::1]:0".parse().unwrap(), - external_ip: nexus.external_ip, - nic: NetworkInterface { - id: nic.identity.id, - kind: NetworkInterfaceKind::Service { - id: nexus.id.into_untyped_uuid(), - }, - name: format!("test-nic-{}", nic.identity.id) - .parse() - .unwrap(), - ip: nic.ip.unwrap(), - mac: nic.mac.unwrap(), - subnet: IpNet::from(*NEXUS_OPTE_IPV4_SUBNET), - vni: Vni::SERVICES_VNI, - primary: true, - slot: nic.slot.unwrap(), - }, - external_tls: false, - external_dns_servers: Vec::new(), - }, - ), - }; - (nexus.sled_id, config) - }) - } - } - - fn sled_states_active( - sled_ids: impl Iterator, - ) -> BTreeMap { - sled_ids.map(|sled_id| (sled_id, SledState::Active)).collect() - } - async fn assert_service_sled_ids( datastore: &DataStore, expected_sled_ids: &[SledUuid], @@ -1625,6 +1485,28 @@ mod tests { assert_eq!(expected_sled_ids, service_sled_ids); } + async fn bp_insert_and_make_target( + opctx: &OpContext, + datastore: &DataStore, + bp: &Blueprint, + ) { + datastore + .blueprint_insert(opctx, bp) + .await + .expect("inserted blueprint"); + datastore + .blueprint_target_set_current( + opctx, + BlueprintTarget { + target_id: bp.id, + enabled: true, + time_made_target: Utc::now(), + }, + ) + .await + .expect("made blueprint the target"); + } + #[tokio::test] async fn test_vpc_resolve_to_sleds_uses_current_target_blueprint() { // Test setup. @@ -1635,68 +1517,84 @@ mod tests { let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; - // Create five sleds. - let harness = Harness::new(5); - for sled in harness.db_sleds() { - datastore.sled_upsert(sled).await.expect("failed to upsert sled"); + // Set up our fake system with 5 sleds. + let rack_id = Uuid::new_v4(); + let mut system = SystemDescription::new(); + let mut sled_ids = Vec::new(); + for _ in 0..5 { + let sled_id = SledUuid::new_v4(); + sled_ids.push(sled_id); + system.sled(SledBuilder::new().id(sled_id)).expect("adding sled"); + datastore + .sled_upsert(SledUpdate::new( + sled_id.into_untyped_uuid(), + "[::1]:0".parse().unwrap(), + sled_baseboard_for_test(), + sled_system_hardware_for_test(), + rack_id, + Generation::new().into(), + )) + .await + .expect("upserting sled"); } - - // We don't have a blueprint yet, so we shouldn't find any services on - // sleds. - assert_service_sled_ids(&datastore, &[]).await; - - // Create a blueprint that has a Nexus on our third sled. (This - // blueprint is completely invalid in many ways, but all we care about - // here is inserting relevant records in `bp_omicron_zone`.) - let bp1_zones = { - let (sled_id, zone_config) = harness - .blueprint_zone_configs() - .nth(2) - .expect("fewer than 3 services in test harness"); - let mut zones = BTreeMap::new(); - zones.insert( - sled_id, - BlueprintZonesConfig { - generation: Generation::new(), - zones: vec![zone_config], + sled_ids.sort_unstable(); + let planning_input = system + .to_planning_input_builder() + .expect("creating planning builder") + .build(); + + // Helper to convert a zone's nic into an insertable nic. + let db_nic_from_zone = |zone_config: &BlueprintZoneConfig| { + let (_, nic) = zone_config + .zone_type + .external_networking() + .expect("external networking for zone type"); + IncompleteNetworkInterface::new_service( + nic.id, + zone_config.id.into_untyped_uuid(), + NEXUS_VPC_SUBNET.clone(), + IdentityMetadataCreateParams { + name: nic.name.clone(), + description: nic.name.to_string(), }, - ); - zones - }; - let bp1_id = Uuid::new_v4(); - let bp1 = Blueprint { - id: bp1_id, - sled_state: sled_states_active(bp1_zones.keys().copied()), - blueprint_zones: bp1_zones, - blueprint_disks: BTreeMap::new(), - parent_blueprint_id: None, - internal_dns_version: Generation::new(), - external_dns_version: Generation::new(), - time_created: Utc::now(), - creator: "test".to_string(), - comment: "test".to_string(), + nic.ip, + nic.mac, + nic.slot, + ) + .expect("creating service nic") }; - datastore - .blueprint_insert(&opctx, &bp1) - .await - .expect("failed to insert blueprint"); - // We haven't set a blueprint target yet, so we should still fail to see - // any services on sleds. + // Create an initial, empty blueprint, and make it the target. + let bp0 = BlueprintBuilder::build_empty_with_sleds( + sled_ids.iter().copied(), + "test", + ); + bp_insert_and_make_target(&opctx, &datastore, &bp0).await; + + // Our blueprint doesn't describe any services, so we shouldn't find any + // sled IDs running services. assert_service_sled_ids(&datastore, &[]).await; - // Make bp1 the current target. - datastore - .blueprint_target_set_current( - &opctx, - BlueprintTarget { - target_id: bp1_id, - enabled: true, - time_made_target: Utc::now(), - }, + // Create a blueprint that has a Nexus on our third sled. + let bp1 = { + let mut builder = BlueprintBuilder::new_based_on( + &logctx.log, + &bp0, + &planning_input, + "test", ) - .await - .expect("failed to set blueprint target"); + .expect("created blueprint builder"); + builder + .sled_ensure_zone_multiple_nexus_with_config( + sled_ids[2], + 1, + false, + Vec::new(), + ) + .expect("added nexus to third sled"); + builder.build() + }; + bp_insert_and_make_target(&opctx, &datastore, &bp1).await; // bp1 is the target, but we haven't yet inserted a vNIC record, so // we still won't see any services on sleds. @@ -1704,185 +1602,105 @@ mod tests { // Insert the relevant service NIC record (normally performed by the // reconfigurator's executor). - datastore + let bp1_nic = datastore .service_create_network_interface_raw( &opctx, - harness.db_nics().nth(2).unwrap(), + db_nic_from_zone(&bp1.blueprint_zones[&sled_ids[2]].zones[0]), ) .await .expect("failed to insert service VNIC"); - // We should now see our third sled running a service. - assert_service_sled_ids(&datastore, &[harness.sled_ids[2]]).await; - - // Create another blueprint with no services and make it the target. - let bp2_id = Uuid::new_v4(); - let bp2 = Blueprint { - id: bp2_id, - sled_state: BTreeMap::new(), - blueprint_zones: BTreeMap::new(), - blueprint_disks: BTreeMap::new(), - parent_blueprint_id: Some(bp1_id), - internal_dns_version: Generation::new(), - external_dns_version: Generation::new(), - time_created: Utc::now(), - creator: "test".to_string(), - comment: "test".to_string(), + assert_service_sled_ids(&datastore, &[sled_ids[2]]).await; + + // Create another blueprint, remove the one nexus we added, and make it + // the target. + let bp2 = { + let mut bp2 = bp1.clone(); + bp2.id = Uuid::new_v4(); + bp2.parent_blueprint_id = Some(bp1.id); + let sled2_zones = bp2 + .blueprint_zones + .get_mut(&sled_ids[2]) + .expect("zones for third sled"); + sled2_zones.zones.clear(); + sled2_zones.generation = sled2_zones.generation.next(); + bp2 }; - datastore - .blueprint_insert(&opctx, &bp2) - .await - .expect("failed to insert blueprint"); - datastore - .blueprint_target_set_current( - &opctx, - BlueprintTarget { - target_id: bp2_id, - enabled: true, - time_made_target: Utc::now(), - }, - ) - .await - .expect("failed to set blueprint target"); + bp_insert_and_make_target(&opctx, &datastore, &bp2).await; // We haven't removed the service NIC record, but we should no longer // see the third sled here. We should be back to no sleds with services. assert_service_sled_ids(&datastore, &[]).await; - // Insert a service NIC record for our fourth sled's Nexus. This - // shouldn't change our VPC resolution. + // Delete the service NIC record so we can reuse this IP later. datastore - .service_create_network_interface_raw( + .service_delete_network_interface( &opctx, - harness.db_nics().nth(3).unwrap(), + bp1.blueprint_zones[&sled_ids[2]].zones[0] + .id + .into_untyped_uuid(), + bp1_nic.id(), ) .await - .expect("failed to insert service VNIC"); - assert_service_sled_ids(&datastore, &[]).await; - - // Create a blueprint that has a Nexus on our fourth sled. This - // shouldn't change our VPC resolution. - let bp3_zones = { - let (sled_id, zone_config) = harness - .blueprint_zone_configs() - .nth(3) - .expect("fewer than 3 services in test harness"); - let mut zones = BTreeMap::new(); - zones.insert( - sled_id, - BlueprintZonesConfig { - generation: Generation::new(), - zones: vec![zone_config], - }, - ); - zones - }; - let bp3_id = Uuid::new_v4(); - let bp3 = Blueprint { - id: bp3_id, - sled_state: sled_states_active(bp3_zones.keys().copied()), - blueprint_zones: bp3_zones, - blueprint_disks: BTreeMap::new(), - parent_blueprint_id: Some(bp2_id), - internal_dns_version: Generation::new(), - external_dns_version: Generation::new(), - time_created: Utc::now(), - creator: "test".to_string(), - comment: "test".to_string(), + .expect("deleted bp1 nic"); + + // Create a blueprint with Nexus on all our sleds. + let bp3 = { + let mut builder = BlueprintBuilder::new_based_on( + &logctx.log, + &bp2, + &planning_input, + "test", + ) + .expect("created blueprint builder"); + for &sled_id in &sled_ids { + builder + .sled_ensure_zone_multiple_nexus_with_config( + sled_id, + 1, + false, + Vec::new(), + ) + .expect("added nexus to third sled"); + } + builder.build() }; - datastore - .blueprint_insert(&opctx, &bp3) - .await - .expect("failed to insert blueprint"); - assert_service_sled_ids(&datastore, &[]).await; - // Make this blueprint the target. We've already created the service - // VNIC, so we should immediately see our fourth sled in VPC resolution. - datastore - .blueprint_target_set_current( - &opctx, - BlueprintTarget { - target_id: bp3_id, - enabled: true, - time_made_target: Utc::now(), - }, - ) - .await - .expect("failed to set blueprint target"); - assert_service_sled_ids(&datastore, &[harness.sled_ids[3]]).await; + // Insert the service NIC records for all the Nexuses. + for &sled_id in &sled_ids { + datastore + .service_create_network_interface_raw( + &opctx, + db_nic_from_zone(&bp3.blueprint_zones[&sled_id].zones[0]), + ) + .await + .expect("failed to insert service VNIC"); + } - // --- + // We haven't made bp3 the target yet, so our resolution is still based + // on bp2; more service vNICs shouldn't matter. + assert_service_sled_ids(&datastore, &[]).await; - // Add a vNIC record for our fifth sled's Nexus, then create a blueprint - // that includes sleds with indexes 2, 3, and 4. Make it the target, - // and ensure we resolve to all five sleds. - datastore - .service_create_network_interface_raw( - &opctx, - harness.db_nics().nth(4).unwrap(), - ) - .await - .expect("failed to insert service VNIC"); - let bp4_zones = { - let mut zones = BTreeMap::new(); - for (sled_id, zone_config) in - harness.blueprint_zone_configs().skip(2) - { - zones.insert( - sled_id, - BlueprintZonesConfig { - generation: Generation::new(), - zones: vec![zone_config], - }, - ); - } - zones - }; - let bp4_id = Uuid::new_v4(); - let bp4 = Blueprint { - id: bp4_id, - sled_state: sled_states_active(bp4_zones.keys().copied()), - blueprint_zones: bp4_zones, - blueprint_disks: BTreeMap::new(), - parent_blueprint_id: Some(bp3_id), - internal_dns_version: Generation::new(), - external_dns_version: Generation::new(), - time_created: Utc::now(), - creator: "test".to_string(), - comment: "test".to_string(), - }; - datastore - .blueprint_insert(&opctx, &bp4) - .await - .expect("failed to insert blueprint"); - datastore - .blueprint_target_set_current( - &opctx, - BlueprintTarget { - target_id: bp4_id, - enabled: true, - time_made_target: Utc::now(), - }, - ) - .await - .expect("failed to set blueprint target"); - assert_service_sled_ids(&datastore, &harness.sled_ids[2..]).await; + // Make bp3 the target; we should immediately resolve that there are + // services on the sleds we set up in bp3. + bp_insert_and_make_target(&opctx, &datastore, &bp3).await; + assert_service_sled_ids(&datastore, &sled_ids).await; // --- // Mark some sleds as ineligible. Only the non-provisionable and // in-service sleds should be returned. let ineligible = IneligibleSleds { - expunged: harness.sled_ids[0], - decommissioned: harness.sled_ids[1], - illegal_decommissioned: harness.sled_ids[2], - non_provisionable: harness.sled_ids[3], + expunged: sled_ids[0], + decommissioned: sled_ids[1], + illegal_decommissioned: sled_ids[2], + non_provisionable: sled_ids[3], }; ineligible .setup(&opctx, &datastore) .await .expect("failed to set up ineligible sleds"); - assert_service_sled_ids(&datastore, &harness.sled_ids[3..=4]).await; + assert_service_sled_ids(&datastore, &sled_ids[3..=4]).await; // --- @@ -1891,92 +1709,41 @@ mod tests { .undo(&opctx, &datastore) .await .expect("failed to undo ineligible sleds"); + assert_service_sled_ids(&datastore, &sled_ids).await; // Make a new blueprint marking one of the zones as quiesced and one as // expunged. Ensure that the sled with *quiesced* zone is returned by // vpc_resolve_to_sleds, but the sled with the *expunged* zone is not. // (But other services are still running.) - let bp5_zones = { - let mut zones = BTreeMap::new(); - // Skip over sled index 0 (should be excluded). - let mut iter = harness.blueprint_zone_configs().skip(1); - - // Sled index 1's zone is active (should be included). - let (sled_id, zone_config) = iter.next().unwrap(); - zones.insert( - sled_id, - BlueprintZonesConfig { - generation: Generation::new(), - zones: vec![zone_config], - }, - ); - - // We never created a vNIC record for sled 1; do so now. - datastore - .service_create_network_interface_raw( - &opctx, - harness.db_nics().nth(1).unwrap(), - ) - .await - .expect("failed to insert service VNIC"); - - // Sled index 2's zone is quiesced (should be included). - let (sled_id, mut zone_config) = iter.next().unwrap(); - zone_config.disposition = BlueprintZoneDisposition::Quiesced; - zones.insert( - sled_id, - BlueprintZonesConfig { - generation: Generation::new(), - zones: vec![zone_config], - }, - ); + let bp4 = { + let mut bp4 = bp3.clone(); + bp4.id = Uuid::new_v4(); + bp4.parent_blueprint_id = Some(bp3.id); + + // Sled index 2's Nexus is quiesced (should be included). + let sled2 = bp4 + .blueprint_zones + .get_mut(&sled_ids[2]) + .expect("zones for sled"); + sled2.zones[0].disposition = BlueprintZoneDisposition::Quiesced; + sled2.generation = sled2.generation.next(); // Sled index 3's zone is expunged (should be excluded). - let (sled_id, mut zone_config) = iter.next().unwrap(); - zone_config.disposition = BlueprintZoneDisposition::Expunged; - zones.insert( - sled_id, - BlueprintZonesConfig { - generation: Generation::new(), - zones: vec![zone_config], - }, - ); - - // Sled index 4's zone is not in the blueprint (should be excluded). - - zones + let sled3 = bp4 + .blueprint_zones + .get_mut(&sled_ids[3]) + .expect("zones for sled"); + sled3.zones[0].disposition = BlueprintZoneDisposition::Expunged; + sled3.generation = sled3.generation.next(); + + bp4 }; - - let bp5_id = Uuid::new_v4(); - let bp5 = Blueprint { - id: bp5_id, - sled_state: sled_states_active(bp5_zones.keys().copied()), - blueprint_zones: bp5_zones, - blueprint_disks: BTreeMap::new(), - parent_blueprint_id: Some(bp4_id), - internal_dns_version: Generation::new(), - external_dns_version: Generation::new(), - time_created: Utc::now(), - creator: "test".to_string(), - comment: "test".to_string(), - }; - - datastore - .blueprint_insert(&opctx, &bp5) - .await - .expect("failed to insert blueprint"); - datastore - .blueprint_target_set_current( - &opctx, - BlueprintTarget { - target_id: bp5_id, - enabled: true, - time_made_target: Utc::now(), - }, - ) - .await - .expect("failed to set blueprint target"); - assert_service_sled_ids(&datastore, &harness.sled_ids[1..=2]).await; + bp_insert_and_make_target(&opctx, &datastore, &bp4).await; + assert_service_sled_ids( + &datastore, + &[sled_ids[0], sled_ids[1], sled_ids[2], sled_ids[4]], + ) + .await; db.cleanup().await.unwrap(); logctx.cleanup_successful(); From 36fde78b37bc4434408c34fac4b066f89729e928 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 2 May 2024 15:13:15 -0400 Subject: [PATCH 08/17] rename SledFilter::All -> ::Commissioned, omitting decommissioned sleds --- dev-tools/omdb/src/bin/omdb/db.rs | 2 +- dev-tools/omdb/tests/test_all_output.rs | 2 +- dev-tools/reconfigurator-cli/src/main.rs | 8 +- .../db-queries/src/db/datastore/deployment.rs | 2 +- nexus/db-queries/src/db/datastore/sled.rs | 2 +- nexus/reconfigurator/execution/src/dns.rs | 2 +- .../planning/src/blueprint_builder/builder.rs | 109 ++++++++++-------- .../planning/src/blueprint_builder/zones.rs | 4 +- nexus/reconfigurator/planning/src/example.rs | 4 +- nexus/reconfigurator/planning/src/planner.rs | 29 ++++- nexus/reconfigurator/preparation/src/lib.rs | 2 +- nexus/src/app/deployment.rs | 5 +- nexus/types/src/deployment.rs | 15 ++- nexus/types/src/deployment/planning_input.rs | 17 +-- 14 files changed, 123 insertions(+), 80 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index 4d36b4522e..ae94442437 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -1456,7 +1456,7 @@ async fn cmd_db_sleds( Some(filter) => filter, None => { eprintln!("note: listing all sleds (use -F to filter, e.g. -F in-service)"); - SledFilter::All + SledFilter::Commissioned } }; diff --git a/dev-tools/omdb/tests/test_all_output.rs b/dev-tools/omdb/tests/test_all_output.rs index a480683f04..19be33631d 100644 --- a/dev-tools/omdb/tests/test_all_output.rs +++ b/dev-tools/omdb/tests/test_all_output.rs @@ -165,7 +165,7 @@ async fn test_omdb_success_cases(cptestctx: &ControlPlaneTestContext) { // collection? assert!(parsed .planning_input - .all_sled_ids(SledFilter::All) + .all_sled_ids(SledFilter::Commissioned) .next() .is_some()); assert!(!parsed.collections.is_empty()); diff --git a/dev-tools/reconfigurator-cli/src/main.rs b/dev-tools/reconfigurator-cli/src/main.rs index ccc35fd74c..72add6ce8c 100644 --- a/dev-tools/reconfigurator-cli/src/main.rs +++ b/dev-tools/reconfigurator-cli/src/main.rs @@ -559,7 +559,7 @@ fn cmd_sled_list( .to_planning_input_builder() .context("failed to generate planning input")? .build(); - let rows = planning_input.all_sled_resources(SledFilter::All).map( + let rows = planning_input.all_sled_resources(SledFilter::Commissioned).map( |(sled_id, sled_resources)| Sled { id: sled_id, subnet: sled_resources.subnet.net().to_string(), @@ -648,7 +648,7 @@ fn cmd_inventory_generate( // has no zones on it. let planning_input = sim.system.to_planning_input_builder().unwrap().build(); - for sled_id in planning_input.all_sled_ids(SledFilter::All) { + for sled_id in planning_input.all_sled_ids(SledFilter::Commissioned) { builder .found_sled_omicron_zones( "fake sled agent", @@ -1077,7 +1077,7 @@ fn cmd_load( .context("generating planning input")? .build(); for (sled_id, sled_details) in - loaded.planning_input.all_sleds(SledFilter::All) + loaded.planning_input.all_sleds(SledFilter::Commissioned) { if current_planning_input.sled_resources(&sled_id).is_some() { swriteln!( @@ -1202,7 +1202,7 @@ fn cmd_file_contents(args: FileContentsArgs) -> anyhow::Result> { let mut s = String::new(); for (sled_id, sled_resources) in - loaded.planning_input.all_sled_resources(SledFilter::All) + loaded.planning_input.all_sled_resources(SledFilter::Commissioned) { swriteln!( s, diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 6fdeed9ee5..7359f1725b 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -1553,7 +1553,7 @@ mod tests { // Check the number of blueprint elements against our collection. assert_eq!( blueprint1.blueprint_zones.len(), - planning_input.all_sled_ids(SledFilter::All).count(), + planning_input.all_sled_ids(SledFilter::Commissioned).count(), ); assert_eq!( blueprint1.blueprint_zones.len(), diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index 76b42d0bf0..8706c7377e 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -1452,7 +1452,7 @@ mod test { assert_eq!(ninserted, size); let sleds = datastore - .sled_list_all_batched(&opctx, SledFilter::All) + .sled_list_all_batched(&opctx, SledFilter::Commissioned) .await .expect("failed to list all sleds"); diff --git a/nexus/reconfigurator/execution/src/dns.rs b/nexus/reconfigurator/execution/src/dns.rs index 6a3c1755cf..1760421dee 100644 --- a/nexus/reconfigurator/execution/src/dns.rs +++ b/nexus/reconfigurator/execution/src/dns.rs @@ -1194,7 +1194,7 @@ mod test { // We do this directly with BlueprintBuilder to avoid the planner // deciding to make other unrelated changes. let sled_rows = datastore - .sled_list_all_batched(&opctx, SledFilter::All) + .sled_list_all_batched(&opctx, SledFilter::Commissioned) .await .unwrap(); let zpool_rows = diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 0d187af50c..0c72002607 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -340,20 +340,15 @@ impl<'a> BlueprintBuilder<'a> { let available_system_macs = AvailableIterator::new(MacAddr::iter_system(), used_macs); - let sled_state = input - .all_sleds(SledFilter::All) - .map(|(sled_id, details)| { - // Prefer the sled state from our parent blueprint for sleds - // that were in it; there may be new sleds in `input`, in which - // case we'll use their current state as our starting point. - let state = parent_blueprint - .sled_state - .get(&sled_id) - .copied() - .unwrap_or(details.state); - (sled_id, state) - }) - .collect(); + // Prefer the sled state from our parent blueprint for sleds + // that were in it; there may be new sleds in `input`, in which + // case we'll use their current state as our starting point. + // + // TODO-correctness When can we drop decommissioned sleds from this map? + let mut sled_state = parent_blueprint.sled_state.clone(); + for (sled_id, details) in input.all_sleds(SledFilter::Commissioned) { + sled_state.entry(sled_id).or_insert(details.state); + } Ok(BlueprintBuilder { log, @@ -373,12 +368,20 @@ impl<'a> BlueprintBuilder<'a> { }) } + /// Iterates over the list of sled IDs for which we have zones. + /// + /// This may include decommissioned sleds. + pub fn sled_ids_with_zones(&self) -> impl Iterator { + self.zones.sled_ids_with_zones() + } + /// Assemble a final [`Blueprint`] based on the contents of the builder pub fn build(mut self) -> Blueprint { // Collect the Omicron zones config for all sleds, including sleds that // are no longer in service and need expungement work. - let blueprint_zones = - self.zones.into_zones_map(self.input.all_sled_ids(SledFilter::All)); + let blueprint_zones = self + .zones + .into_zones_map(self.input.all_sled_ids(SledFilter::Commissioned)); let blueprint_disks = self .disks .into_disks_map(self.input.all_sled_ids(SledFilter::InService)); @@ -465,7 +468,7 @@ impl<'a> BlueprintBuilder<'a> { &log, "sled has state Decommissioned, yet has zones \ allocated to it; will expunge them \ - (sled policy is \"{policy}\")" + (sled policy is \"{policy:?}\")" ); } ZoneExpungeReason::SledExpunged => { @@ -1012,6 +1015,18 @@ impl<'a> BlueprintZonesBuilder<'a> { }) } + /// Iterates over the list of sled IDs for which we have zones. + /// + /// This may include decommissioned sleds. + pub fn sled_ids_with_zones(&self) -> impl Iterator { + let mut sled_ids = + self.changed_zones.keys().copied().collect::>(); + for &sled_id in self.parent_zones.keys() { + sled_ids.insert(sled_id); + } + sled_ids.into_iter() + } + /// Iterates over the list of Omicron zones currently configured for this /// sled in the blueprint that's being built, along with each zone's state /// in the builder. @@ -1036,36 +1051,32 @@ impl<'a> BlueprintZonesBuilder<'a> { /// Produces an owned map of zones for the requested sleds pub fn into_zones_map( - mut self, - sled_ids: impl Iterator, + self, + commissioned_sled_ids: impl Iterator, ) -> BTreeMap { - sled_ids - .map(|sled_id| { - // Start with self.changed_zones, which contains entries for any - // sled whose zones config is changing in this blueprint. - if let Some(zones) = self.changed_zones.remove(&sled_id) { - (sled_id, zones.build()) - } - // Next, check self.parent_zones, to represent an unchanged - // sled. - else if let Some(parent_zones) = - self.parent_zones.get(&sled_id) - { - (sled_id, parent_zones.clone()) - } else { - // If the sled is not in self.parent_zones, then it must be - // a new sled and we haven't added any zones to it yet. Use - // the standard initial config. - ( - sled_id, - BlueprintZonesConfig { - generation: Generation::new(), - zones: vec![], - }, - ) - } - }) - .collect() + // Start with self.changed_zones, which contains entries for any + // sled whose zones config is changing in this blueprint. + let mut zones = self + .changed_zones + .into_iter() + .map(|(sled_id, zones)| (sled_id, zones.build())) + .collect::>(); + + // Carry forward any zones from our parent blueprint. This may include + // zones for decommissioned sleds. + for (sled_id, parent_zones) in self.parent_zones { + zones.entry(*sled_id).or_insert_with(|| parent_zones.clone()); + } + + // Finally, insert any newly-added sleds. + for sled_id in commissioned_sled_ids { + zones.entry(sled_id).or_insert_with(|| BlueprintZonesConfig { + generation: Generation::new(), + zones: vec![], + }); + } + + zones } } @@ -1269,7 +1280,7 @@ pub mod test { // existing sleds, plus Crucible zones on all pools. So if we ensure // all these zones exist, we should see no change. for (sled_id, sled_resources) in - example.input.all_sled_resources(SledFilter::All) + example.input.all_sled_resources(SledFilter::Commissioned) { builder.sled_ensure_zone_ntp(sled_id).unwrap(); for pool_id in sled_resources.zpools.keys() { @@ -1384,7 +1395,7 @@ pub mod test { // Start with an empty blueprint (sleds with no zones). let parent = BlueprintBuilder::build_empty_with_sleds_seeded( - input.all_sled_ids(SledFilter::All), + input.all_sled_ids(SledFilter::Commissioned), "test", TEST_NAME, ); @@ -1430,7 +1441,7 @@ pub mod test { let (collection, input, _) = example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); let parent = BlueprintBuilder::build_empty_with_sleds_seeded( - input.all_sled_ids(SledFilter::All), + input.all_sled_ids(SledFilter::Commissioned), "test", TEST_NAME, ); diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/zones.rs b/nexus/reconfigurator/planning/src/blueprint_builder/zones.rs index c9015f8b27..a2e577f80c 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/zones.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/zones.rs @@ -274,7 +274,7 @@ mod tests { // existing sled. let existing_sled_id = example .input - .all_sled_ids(SledFilter::All) + .all_sled_ids(SledFilter::Commissioned) .next() .expect("at least one sled present"); let change = builder.zones.change_sled_zones(existing_sled_id); @@ -351,7 +351,7 @@ mod tests { // become smarter and not do so (in which case this test will break). let control_sled_id = example .input - .all_sled_ids(SledFilter::All) + .all_sled_ids(SledFilter::Commissioned) .nth(2) .expect("at least 2 sleds present"); _ = builder.zones.change_sled_zones(control_sled_id); diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index 136302e7be..24dbbd15ac 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -51,7 +51,7 @@ impl ExampleSystem { // Start with an empty blueprint containing only our sleds, no zones. let initial_blueprint = BlueprintBuilder::build_empty_with_sleds_seeded( - base_input.all_sled_ids(SledFilter::All), + base_input.all_sled_ids(SledFilter::Commissioned), "test suite", (test_name, "ExampleSystem initial"), ); @@ -66,7 +66,7 @@ impl ExampleSystem { .unwrap(); builder.set_rng_seed((test_name, "ExampleSystem make_zones")); for (sled_id, sled_resources) in - base_input.all_sled_resources(SledFilter::All) + base_input.all_sled_resources(SledFilter::Commissioned) { let _ = builder.sled_ensure_zone_ntp(sled_id).unwrap(); let _ = builder diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 7fb8e49c31..2b45cd6074 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -85,10 +85,16 @@ impl<'a> Planner<'a> { } fn do_plan_expunge(&mut self) -> Result<(), Error> { + let mut commissioned_sled_ids = BTreeSet::new(); + // Remove services from sleds marked expunged. We use `SledFilter::All` // and have a custom `needs_zone_expungement` function that allows us // to produce better errors. - for (sled_id, sled_details) in self.input.all_sleds(SledFilter::All) { + for (sled_id, sled_details) in + self.input.all_sleds(SledFilter::Commissioned) + { + commissioned_sled_ids.insert(sled_id); + // Does this sled need zone expungement based on the details? let Some(reason) = needs_zone_expungement(sled_details.state, sled_details.policy) @@ -100,6 +106,21 @@ impl<'a> Planner<'a> { self.blueprint.expunge_all_zones_for_sled(sled_id, reason)?; } + // Check for any decommissioned sleds (i.e., sleds for which our + // blueprint has zones, but are not in the input sled list). Any zones + // for decommissioned sleds _should_ already be expunged; ensure that is + // the case. + for sled_id in self.blueprint.sled_ids_with_zones() { + if !commissioned_sled_ids.contains(&sled_id) { + self.blueprint.expunge_all_zones_for_sled( + sled_id, + // Decommissioned sleds are not present in our input, so we + // don't know what its policy was. + ZoneExpungeReason::SledDecommissioned { policy: None }, + )?; + } + } + Ok(()) } @@ -372,7 +393,9 @@ fn needs_zone_expungement( // an illegal state, but representable. If we see a sled in this // state, we should still expunge all zones in it, but parent code // should warn on it. - return Some(ZoneExpungeReason::SledDecommissioned { policy }); + return Some(ZoneExpungeReason::SledDecommissioned { + policy: Some(policy), + }); } } @@ -388,7 +411,7 @@ fn needs_zone_expungement( /// logical flow. #[derive(Copy, Clone, Debug)] pub(crate) enum ZoneExpungeReason { - SledDecommissioned { policy: SledPolicy }, + SledDecommissioned { policy: Option }, SledExpunged, } diff --git a/nexus/reconfigurator/preparation/src/lib.rs b/nexus/reconfigurator/preparation/src/lib.rs index 6346db47bd..305644bc93 100644 --- a/nexus/reconfigurator/preparation/src/lib.rs +++ b/nexus/reconfigurator/preparation/src/lib.rs @@ -182,7 +182,7 @@ pub async fn reconfigurator_state_load( ) -> Result { opctx.check_complex_operations_allowed()?; let sled_rows = datastore - .sled_list_all_batched(opctx, SledFilter::All) + .sled_list_all_batched(opctx, SledFilter::Commissioned) .await .context("listing sleds")?; let zpool_rows = datastore diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index b89fdb5a9c..98f1f84744 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -130,8 +130,9 @@ impl super::Nexus { let creator = self.id.to_string(); let datastore = self.datastore(); - let sled_rows = - datastore.sled_list_all_batched(opctx, SledFilter::All).await?; + let sled_rows = datastore + .sled_list_all_batched(opctx, SledFilter::Commissioned) + .await?; let zpool_rows = datastore.zpool_list_all_external_batched(opctx).await?; let ip_pool_range_rows = { diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index fe013b8ca4..b7b5bf6aac 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -106,19 +106,24 @@ pub struct Blueprint { /// unique identifier for this blueprint pub id: Uuid, - /// A map of sled id -> zones deployed on each sled, along with the - /// [`BlueprintZoneDisposition`] for each zone. + /// A map of sled id -> desired state of the sled. /// /// A sled is considered part of the control plane cluster iff it has an /// entry in this map. + pub sled_state: BTreeMap, + + /// A map of sled id -> zones deployed on each sled, along with the + /// [`BlueprintZoneDisposition`] for each zone. + /// + /// Unlike `sled_state`, this map may contain entries for sleds that are no + /// longer a part of the control plane cluster (e.g., sleds that have been + /// decommissioned, but still have expunged zones where cleanup has not yet + /// completed). pub blueprint_zones: BTreeMap, /// A map of sled id -> disks in use on each sled. pub blueprint_disks: BTreeMap, - /// A map of sled id -> desired state of the sled. - pub sled_state: BTreeMap, - /// which blueprint this blueprint is based on pub parent_blueprint_id: Option, diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index b932b6000e..1975cfaae0 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -244,8 +244,11 @@ pub enum SledFilter { // --- // Prefer to keep this list in alphabetical order. // --- - /// All sleds. - All, + /// All sleds that are currently part of the control plane cluster. + /// + /// Intentionally omits decommissioned sleds, but is otherwise the filter to + /// fetch "all sleds regardless of current policy or state". + Commissioned, /// Sleds that are eligible for discretionary services. Discretionary, @@ -308,7 +311,7 @@ impl SledPolicy { SledPolicy::InService { provision_policy: SledProvisionPolicy::Provisionable, } => match filter { - SledFilter::All => true, + SledFilter::Commissioned => true, SledFilter::Discretionary => true, SledFilter::InService => true, SledFilter::QueryDuringInventory => true, @@ -318,7 +321,7 @@ impl SledPolicy { SledPolicy::InService { provision_policy: SledProvisionPolicy::NonProvisionable, } => match filter { - SledFilter::All => true, + SledFilter::Commissioned => true, SledFilter::Discretionary => false, SledFilter::InService => true, SledFilter::QueryDuringInventory => true, @@ -326,7 +329,7 @@ impl SledPolicy { SledFilter::VpcFirewall => true, }, SledPolicy::Expunged => match filter { - SledFilter::All => true, + SledFilter::Commissioned => true, SledFilter::Discretionary => false, SledFilter::InService => false, SledFilter::QueryDuringInventory => false, @@ -356,7 +359,7 @@ impl SledState { // See `SledFilter::matches` above for some notes. match self { SledState::Active => match filter { - SledFilter::All => true, + SledFilter::Commissioned => true, SledFilter::Discretionary => true, SledFilter::InService => true, SledFilter::QueryDuringInventory => true, @@ -364,7 +367,7 @@ impl SledState { SledFilter::VpcFirewall => true, }, SledState::Decommissioned => match filter { - SledFilter::All => true, + SledFilter::Commissioned => false, SledFilter::Discretionary => false, SledFilter::InService => false, SledFilter::QueryDuringInventory => false, From 036f030a824ff724246b03cc1344fa789fe94a15 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 2 May 2024 17:33:57 -0400 Subject: [PATCH 09/17] planner: mark sleds decommissioned --- dev-tools/omdb/tests/usage_errors.out | 2 +- .../planning/src/blueprint_builder/builder.rs | 16 +++++ nexus/reconfigurator/planning/src/planner.rs | 63 ++++++++++++++++++- openapi/nexus-internal.json | 4 +- 4 files changed, 79 insertions(+), 6 deletions(-) diff --git a/dev-tools/omdb/tests/usage_errors.out b/dev-tools/omdb/tests/usage_errors.out index 7b45d33700..ee32167290 100644 --- a/dev-tools/omdb/tests/usage_errors.out +++ b/dev-tools/omdb/tests/usage_errors.out @@ -226,7 +226,7 @@ Options: Show sleds that match the given filter Possible values: - - all: All sleds + - commissioned: All sleds that are currently part of the control plane cluster - discretionary: Sleds that are eligible for discretionary services - in-service: Sleds that are in service (even if they might not be eligible for discretionary services) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 0c72002607..df486ac5bf 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -375,6 +375,13 @@ impl<'a> BlueprintBuilder<'a> { self.zones.sled_ids_with_zones() } + pub fn current_sled_zones( + &self, + sled_id: SledUuid, + ) -> impl Iterator { + self.zones.current_sled_zones(sled_id).map(|(config, _)| config) + } + /// Assemble a final [`Blueprint`] based on the contents of the builder pub fn build(mut self) -> Blueprint { // Collect the Omicron zones config for all sleds, including sleds that @@ -399,6 +406,15 @@ impl<'a> BlueprintBuilder<'a> { } } + /// Set the desired state of the given sled. + pub fn set_sled_state( + &mut self, + sled_id: SledUuid, + desired_state: SledState, + ) { + self.sled_state.insert(sled_id, desired_state); + } + /// Within tests, set a seeded RNG for deterministic results. /// /// This will ensure that tests that use this builder will produce the same diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 2b45cd6074..cb18a214c1 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -11,6 +11,7 @@ use crate::blueprint_builder::Ensure; use crate::blueprint_builder::EnsureMultiple; use crate::blueprint_builder::Error; use nexus_types::deployment::Blueprint; +use nexus_types::deployment::BlueprintZoneDisposition; use nexus_types::deployment::PlanningInput; use nexus_types::deployment::SledFilter; use nexus_types::deployment::ZpoolFilter; @@ -80,6 +81,62 @@ impl<'a> Planner<'a> { self.do_plan_expunge()?; self.do_plan_add()?; + self.do_plan_decommission()?; + + Ok(()) + } + + fn do_plan_decommission(&mut self) -> Result<(), Error> { + // Check for any sleds that are currently commissioned but can be + // decommissioned. Our gates for decommissioning are: + // + // 1. The policy indicates the sled has been removed (i.e., the policy + // is "expunged"; we may have other policies that satisfy this + // requirement in the future). + // 2. All zones associated with the sled have been marked expunged. + // 3. There are no instances assigned to this sled. This is blocked by + // omicron#4872, so today we omit this check entirely, as any sled + // that could be otherwise decommissioned that still has instances + // assigned to it needs support intervention for cleanup. + // 4. All disks associated with the sled have been marked expunged. This + // happens implicitly when a sled is expunged, so is covered by our + // first check. + for (sled_id, sled_details) in + self.input.all_sleds(SledFilter::Commissioned) + { + // Check 1: look for sleds that are expunged. + match (sled_details.policy, sled_details.state) { + // If the sled is still in service, don't decommission it. + (SledPolicy::InService { .. }, _) => continue, + // If the sled is already decommissioned it... why is it showing + // up when we ask for commissioned sleds? Warn, but don't try to + // decommission it again. + (SledPolicy::Expunged, SledState::Decommissioned) => { + warn!(self.log, "todo"); + continue; + } + // The sled is expunged but not yet decommissioned; fall through + // to check the rest of the criteria. + (SledPolicy::Expunged, SledState::Active) => (), + } + + // Check 2: have all this sled's zones been expunged? It's possible + // we ourselves have made this change, which is fine. + let all_zones_expunged = + self.blueprint.current_sled_zones(sled_id).all(|zone| { + zone.disposition == BlueprintZoneDisposition::Expunged + }); + + // Check 3: Are there any instances assigned to this sled? See + // comment above; while we wait for omicron#4872, we just assume + // there are no instances running. + let num_instances_assigned = 0; + + if all_zones_expunged && num_instances_assigned == 0 { + self.blueprint + .set_sled_state(sled_id, SledState::Decommissioned); + } + } Ok(()) } @@ -87,9 +144,9 @@ impl<'a> Planner<'a> { fn do_plan_expunge(&mut self) -> Result<(), Error> { let mut commissioned_sled_ids = BTreeSet::new(); - // Remove services from sleds marked expunged. We use `SledFilter::All` - // and have a custom `needs_zone_expungement` function that allows us - // to produce better errors. + // Remove services from sleds marked expunged. We use + // `SledFilter::Commissioned` and have a custom `needs_zone_expungement` + // function that allows us to produce better errors. for (sled_id, sled_details) in self.input.all_sleds(SledFilter::Commissioned) { diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index eec41b167f..3e7287cd7d 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -1600,7 +1600,7 @@ } }, "blueprint_zones": { - "description": "A map of sled id -> zones deployed on each sled, along with the [`BlueprintZoneDisposition`] for each zone.\n\nA sled is considered part of the control plane cluster iff it has an entry in this map.", + "description": "A map of sled id -> zones deployed on each sled, along with the [`BlueprintZoneDisposition`] for each zone.\n\nUnlike `sled_state`, this map may contain entries for sleds that are no longer a part of the control plane cluster (e.g., sleds that have been decommissioned, but still have expunged zones where cleanup has not yet completed).", "type": "object", "additionalProperties": { "$ref": "#/components/schemas/BlueprintZonesConfig" @@ -1642,7 +1642,7 @@ "format": "uuid" }, "sled_state": { - "description": "A map of sled id -> desired state of the sled.", + "description": "A map of sled id -> desired state of the sled.\n\nA sled is considered part of the control plane cluster iff it has an entry in this map.", "type": "object", "additionalProperties": { "$ref": "#/components/schemas/SledState" From aa715ad97f5b218580f911b805fa9d05a07b6231 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 2 May 2024 17:47:39 -0400 Subject: [PATCH 10/17] add basic test for marking sleds decommissioned --- nexus/reconfigurator/planning/src/planner.rs | 96 ++++++++++++++++++- .../planner_decommissions_sleds_1_2.txt | 81 ++++++++++++++++ .../planner_decommissions_sleds_bp2.txt | 56 +++++++++++ 3 files changed, 232 insertions(+), 1 deletion(-) create mode 100644 nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_1_2.txt create mode 100644 nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_bp2.txt diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index cb18a214c1..3d7f61e96f 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -112,7 +112,12 @@ impl<'a> Planner<'a> { // up when we ask for commissioned sleds? Warn, but don't try to // decommission it again. (SledPolicy::Expunged, SledState::Decommissioned) => { - warn!(self.log, "todo"); + warn!( + self.log, + "decommissioned sled returned by \ + SledFilter::Commissioned"; + "sled_id" => %sled_id, + ); continue; } // The sled is expunged but not yet decommissioned; fall through @@ -1190,4 +1195,93 @@ mod test { ); } } + + #[test] + fn planner_decommissions_sleds() { + static TEST_NAME: &str = "planner_decommissions_sleds"; + let logctx = test_setup_log(TEST_NAME); + + // Use our example system as a starting point. + let (collection, input, blueprint1) = + example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); + + // Expunge one of the sleds. + let mut builder = input.into_builder(); + let expunged_sled_id = { + let mut iter = builder.sleds_mut().iter_mut(); + let (sled_id, details) = iter.next().expect("at least one sled"); + details.policy = SledPolicy::Expunged; + *sled_id + }; + + let input = builder.build(); + let mut blueprint2 = Planner::new_based_on( + logctx.log.clone(), + &blueprint1, + &input, + "test_blueprint2", + &collection, + ) + .expect("created planner") + .with_rng_seed((TEST_NAME, "bp2")) + .plan() + .expect("failed to plan"); + + // Define a time_created for consistent output across runs. + blueprint2.time_created = + Utc.from_utc_datetime(&NaiveDateTime::UNIX_EPOCH); + + assert_contents( + "tests/output/planner_decommissions_sleds_bp2.txt", + &blueprint2.display().to_string(), + ); + let diff = blueprint2.diff_since_blueprint(&blueprint1).unwrap(); + println!("1 -> 2 (expunged {expunged_sled_id}):\n{}", diff.display()); + assert_contents( + "tests/output/planner_decommissions_sleds_1_2.txt", + &diff.display().to_string(), + ); + + // All the zones of the expunged sled should be expunged, and the sled + // itself should be decommissioned. + assert!(blueprint2.blueprint_zones[&expunged_sled_id] + .are_all_zones_expunged()); + assert_eq!( + blueprint2.sled_state[&expunged_sled_id], + SledState::Decommissioned + ); + + // Remove the now-decommissioned sled from the planning input. + let mut builder = input.into_builder(); + builder.sleds_mut().remove(&expunged_sled_id); + let input = builder.build(); + + let blueprint3 = Planner::new_based_on( + logctx.log.clone(), + &blueprint2, + &input, + "test_blueprint3", + &collection, + ) + .expect("created planner") + .with_rng_seed((TEST_NAME, "bp3")) + .plan() + .expect("failed to plan"); + + // There should be no changes to the blueprint; we don't yet garbage + // collect zones, so we should still have the sled's expunged zones + // (even though the sled itself is no longer present in the list of + // commissioned sleds). + let diff = blueprint3.diff_since_blueprint(&blueprint2).unwrap(); + println!( + "2 -> 3 (decommissioned {expunged_sled_id}):\n{}", + diff.display() + ); + assert_eq!(diff.sleds_added().count(), 0); + assert_eq!(diff.sleds_removed().count(), 0); + assert_eq!(diff.sleds_modified().count(), 0); + assert_eq!(diff.sleds_unchanged().count(), DEFAULT_N_SLEDS); + + logctx.cleanup_successful(); + } } diff --git a/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_1_2.txt b/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_1_2.txt new file mode 100644 index 0000000000..28f08c9c78 --- /dev/null +++ b/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_1_2.txt @@ -0,0 +1,81 @@ +from: blueprint 516e80a3-b362-4fac-bd3c-4559717120dd +to: blueprint 1ac2d88f-27dd-4506-8585-6b2be832528e + + -------------------------------------------------------------------------------------------------------- + zone type zone ID disposition underlay IP status + -------------------------------------------------------------------------------------------------------- + + UNCHANGED SLEDS: + + sled d67ce8f0-a691-4010-b414-420d82e80527: blueprint zones at generation 2 + crucible 15dbaa30-1539-49d6-970d-ba5962960f33 in service fd00:1122:3344:101::27 + crucible 1ec4cc7b-2f00-4d13-8176-3b9815533ae9 in service fd00:1122:3344:101::24 + crucible 2e65b765-5c41-4519-bf4e-e2a68569afc1 in service fd00:1122:3344:101::23 + crucible 3d4143df-e212-4774-9258-7d9b421fac2e in service fd00:1122:3344:101::25 + crucible 5d9d8fa7-8379-470b-90ba-fe84a3c45512 in service fd00:1122:3344:101::2a + crucible 70232a6d-6c9d-4fa6-a34d-9c73d940db33 in service fd00:1122:3344:101::28 + crucible 8567a616-a709-4c8c-a323-4474675dad5c in service fd00:1122:3344:101::2c + crucible 8b0b8623-930a-41af-9f9b-ca28b1b11139 in service fd00:1122:3344:101::29 + crucible cf87d2a3-d323-44a3-a87e-adc4ef6c75f4 in service fd00:1122:3344:101::2b + crucible eac6c0a0-baa5-4490-9cee-65198b7fbd9c in service fd00:1122:3344:101::26 + internal_ntp ad76d200-5675-444b-b19c-684689ff421f in service fd00:1122:3344:101::21 + nexus e9bf2525-5fa0-4c1b-b52d-481225083845 in service fd00:1122:3344:101::22 + + MODIFIED SLEDS: + +* sled a1b477db-b629-48eb-911d-1ccdafca75b9: blueprint zones at generation: 2 -> 3 +- crucible 1e1ed0cc-1adc-410f-943a-d1a3107de619 in service fd00:1122:3344:103::27 modified ++ ├─ expunged fd00:1122:3344:103::27 +* └─ changed: disposition +- crucible 2307bbed-02ba-493b-89e3-46585c74c8fc in service fd00:1122:3344:103::28 modified ++ ├─ expunged fd00:1122:3344:103::28 +* └─ changed: disposition +- crucible 4e36b7ef-5684-4304-b7c3-3c31aaf83d4f in service fd00:1122:3344:103::23 modified ++ ├─ expunged fd00:1122:3344:103::23 +* └─ changed: disposition +- crucible 603e629d-2599-400e-b879-4134d4cc426e in service fd00:1122:3344:103::2c modified ++ ├─ expunged fd00:1122:3344:103::2c +* └─ changed: disposition +- crucible 9179d6dc-387d-424e-8d62-ed59b2c728f6 in service fd00:1122:3344:103::2a modified ++ ├─ expunged fd00:1122:3344:103::2a +* └─ changed: disposition +- crucible c28d7b4b-a259-45ad-945d-f19ca3c6964c in service fd00:1122:3344:103::29 modified ++ ├─ expunged fd00:1122:3344:103::29 +* └─ changed: disposition +- crucible e29998e7-9ed2-46b6-bb70-4118159fe07f in service fd00:1122:3344:103::26 modified ++ ├─ expunged fd00:1122:3344:103::26 +* └─ changed: disposition +- crucible f06e91a1-0c17-4cca-adbc-1c9b67bdb11d in service fd00:1122:3344:103::2b modified ++ ├─ expunged fd00:1122:3344:103::2b +* └─ changed: disposition +- crucible f11f5c60-1ac7-4630-9a3a-a9bc85c75203 in service fd00:1122:3344:103::25 modified ++ ├─ expunged fd00:1122:3344:103::25 +* └─ changed: disposition +- crucible f231e4eb-3fc9-4964-9d71-2c41644852d9 in service fd00:1122:3344:103::24 modified ++ ├─ expunged fd00:1122:3344:103::24 +* └─ changed: disposition +- internal_ntp c62b87b6-b98d-4d22-ba4f-cee4499e2ba8 in service fd00:1122:3344:103::21 modified ++ ├─ expunged fd00:1122:3344:103::21 +* └─ changed: disposition +- nexus 6a70a233-1900-43c0-9c00-aa9d1f7adfbc in service fd00:1122:3344:103::22 modified ++ ├─ expunged fd00:1122:3344:103::22 +* └─ changed: disposition + +* sled fefcf4cf-f7e7-46b3-b629-058526ce440e: blueprint zones at generation: 2 -> 3 + crucible 0e2b035e-1de1-48af-8ac0-5316418e3de1 in service fd00:1122:3344:102::2a + crucible 4f8ce495-21dd-48a1-859c-80d34ce394ed in service fd00:1122:3344:102::23 + crucible 5c78756d-6182-4c27-a507-3419e8dbe76b in service fd00:1122:3344:102::28 + crucible a1ae92ac-e1f1-4654-ab54-5b75ba7c44d6 in service fd00:1122:3344:102::24 + crucible a308d3e1-118c-440a-947a-8b6ab7d833ab in service fd00:1122:3344:102::25 + crucible b7402110-d88f-4ca4-8391-4a2fda6ad271 in service fd00:1122:3344:102::29 + crucible b7ae596e-0c85-40b2-bb47-df9f76db3cca in service fd00:1122:3344:102::2b + crucible c552280f-ba02-4f8d-9049-bd269e6b7845 in service fd00:1122:3344:102::26 + crucible cf13b878-47f1-4ba0-b8c2-9f3e15f2ee87 in service fd00:1122:3344:102::2c + crucible e6d0df1f-9f98-4c5a-9540-8444d1185c7d in service fd00:1122:3344:102::27 + internal_ntp f68846ad-4619-4747-8293-a2b4aeeafc5b in service fd00:1122:3344:102::21 + nexus 99c6401d-9796-4ae1-bf0c-9a097cf21c33 in service fd00:1122:3344:102::22 ++ nexus c8851a11-a4f7-4b21-9281-6182fd15dc8d in service fd00:1122:3344:102::2d added + + METADATA: + internal DNS version: 1 (unchanged) + external DNS version: 1 (unchanged) diff --git a/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_bp2.txt b/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_bp2.txt new file mode 100644 index 0000000000..ca08bd5c33 --- /dev/null +++ b/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_bp2.txt @@ -0,0 +1,56 @@ +blueprint 1ac2d88f-27dd-4506-8585-6b2be832528e +parent: 516e80a3-b362-4fac-bd3c-4559717120dd + + -------------------------------------------------------------------------------------------- + zone type zone ID disposition underlay IP + -------------------------------------------------------------------------------------------- + + sled a1b477db-b629-48eb-911d-1ccdafca75b9: blueprint zones at generation 3 + crucible 1e1ed0cc-1adc-410f-943a-d1a3107de619 expunged fd00:1122:3344:103::27 + crucible 2307bbed-02ba-493b-89e3-46585c74c8fc expunged fd00:1122:3344:103::28 + crucible 4e36b7ef-5684-4304-b7c3-3c31aaf83d4f expunged fd00:1122:3344:103::23 + crucible 603e629d-2599-400e-b879-4134d4cc426e expunged fd00:1122:3344:103::2c + crucible 9179d6dc-387d-424e-8d62-ed59b2c728f6 expunged fd00:1122:3344:103::2a + crucible c28d7b4b-a259-45ad-945d-f19ca3c6964c expunged fd00:1122:3344:103::29 + crucible e29998e7-9ed2-46b6-bb70-4118159fe07f expunged fd00:1122:3344:103::26 + crucible f06e91a1-0c17-4cca-adbc-1c9b67bdb11d expunged fd00:1122:3344:103::2b + crucible f11f5c60-1ac7-4630-9a3a-a9bc85c75203 expunged fd00:1122:3344:103::25 + crucible f231e4eb-3fc9-4964-9d71-2c41644852d9 expunged fd00:1122:3344:103::24 + internal_ntp c62b87b6-b98d-4d22-ba4f-cee4499e2ba8 expunged fd00:1122:3344:103::21 + nexus 6a70a233-1900-43c0-9c00-aa9d1f7adfbc expunged fd00:1122:3344:103::22 + + sled d67ce8f0-a691-4010-b414-420d82e80527: blueprint zones at generation 2 + crucible 15dbaa30-1539-49d6-970d-ba5962960f33 in service fd00:1122:3344:101::27 + crucible 1ec4cc7b-2f00-4d13-8176-3b9815533ae9 in service fd00:1122:3344:101::24 + crucible 2e65b765-5c41-4519-bf4e-e2a68569afc1 in service fd00:1122:3344:101::23 + crucible 3d4143df-e212-4774-9258-7d9b421fac2e in service fd00:1122:3344:101::25 + crucible 5d9d8fa7-8379-470b-90ba-fe84a3c45512 in service fd00:1122:3344:101::2a + crucible 70232a6d-6c9d-4fa6-a34d-9c73d940db33 in service fd00:1122:3344:101::28 + crucible 8567a616-a709-4c8c-a323-4474675dad5c in service fd00:1122:3344:101::2c + crucible 8b0b8623-930a-41af-9f9b-ca28b1b11139 in service fd00:1122:3344:101::29 + crucible cf87d2a3-d323-44a3-a87e-adc4ef6c75f4 in service fd00:1122:3344:101::2b + crucible eac6c0a0-baa5-4490-9cee-65198b7fbd9c in service fd00:1122:3344:101::26 + internal_ntp ad76d200-5675-444b-b19c-684689ff421f in service fd00:1122:3344:101::21 + nexus e9bf2525-5fa0-4c1b-b52d-481225083845 in service fd00:1122:3344:101::22 + + sled fefcf4cf-f7e7-46b3-b629-058526ce440e: blueprint zones at generation 3 + crucible 0e2b035e-1de1-48af-8ac0-5316418e3de1 in service fd00:1122:3344:102::2a + crucible 4f8ce495-21dd-48a1-859c-80d34ce394ed in service fd00:1122:3344:102::23 + crucible 5c78756d-6182-4c27-a507-3419e8dbe76b in service fd00:1122:3344:102::28 + crucible a1ae92ac-e1f1-4654-ab54-5b75ba7c44d6 in service fd00:1122:3344:102::24 + crucible a308d3e1-118c-440a-947a-8b6ab7d833ab in service fd00:1122:3344:102::25 + crucible b7402110-d88f-4ca4-8391-4a2fda6ad271 in service fd00:1122:3344:102::29 + crucible b7ae596e-0c85-40b2-bb47-df9f76db3cca in service fd00:1122:3344:102::2b + crucible c552280f-ba02-4f8d-9049-bd269e6b7845 in service fd00:1122:3344:102::26 + crucible cf13b878-47f1-4ba0-b8c2-9f3e15f2ee87 in service fd00:1122:3344:102::2c + crucible e6d0df1f-9f98-4c5a-9540-8444d1185c7d in service fd00:1122:3344:102::27 + internal_ntp f68846ad-4619-4747-8293-a2b4aeeafc5b in service fd00:1122:3344:102::21 + nexus 99c6401d-9796-4ae1-bf0c-9a097cf21c33 in service fd00:1122:3344:102::22 + nexus c8851a11-a4f7-4b21-9281-6182fd15dc8d in service fd00:1122:3344:102::2d + +METADATA: + created by: test_blueprint2 + created at: 1970-01-01T00:00:00.000Z + comment: sled a1b477db-b629-48eb-911d-1ccdafca75b9 (sled policy is expunged): 12 zones expunged, sled d67ce8f0-a691-4010-b414-420d82e80527: altered disks, sled fefcf4cf-f7e7-46b3-b629-058526ce440e: altered disks + internal DNS version: 1 + external DNS version: 1 From 6eadb5f50c45c49528a49afdde610897f1def3b9 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 3 May 2024 08:56:25 -0400 Subject: [PATCH 11/17] decommissioning a sled decommissions its disks --- nexus/db-queries/src/db/datastore/sled.rs | 203 +++++++++++++++------- nexus/reconfigurator/execution/src/lib.rs | 2 +- 2 files changed, 144 insertions(+), 61 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index 8706c7377e..8162d31504 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -454,12 +454,13 @@ impl DataStore { UpdateStatus::NotUpdatedButExists, ) => { // Two reasons this can happen: - // 1. An idempotent update: this is treated as a success. + // 1. An idempotent update: this is treated as a + // success. // 2. Invalid state transition: a failure. // - // To differentiate between the two, check that the new policy - // is the same as the old policy, and that the old state is - // valid. + // To differentiate between the two, check that the + // new policy is the same as the old policy, and + // that the old state is valid. if result.found.policy() == new_sled_policy && valid_old_states .contains(&result.found.state()) @@ -548,7 +549,7 @@ impl DataStore { &self, opctx: &OpContext, authz_sled: &authz::Sled, - new_state: SledState, + new_sled_state: SledState, check: ValidateTransition, ) -> Result { use db::schema::sled::dsl; @@ -556,62 +557,120 @@ impl DataStore { opctx.authorize(authz::Action::Modify, authz_sled).await?; let sled_id = authz_sled.id(); - let query = diesel::update(dsl::sled) - .filter(dsl::time_deleted.is_null()) - .filter(dsl::id.eq(sled_id)); + let err = OptionalError::new(); + let conn = self.pool_connection_authorized(opctx).await?; + let old_state = self + .transaction_retry_wrapper("sled_set_state") + .transaction(&conn, |conn| { + let err = err.clone(); - let t = SledTransition::State(new_state); - let valid_old_policies = t.valid_old_policies(); - let valid_old_states = t.valid_old_states(); + async move { + let query = diesel::update(dsl::sled) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::id.eq(sled_id)); - let query = match check { - ValidateTransition::Yes => query - .filter(dsl::sled_policy.eq_any( - valid_old_policies.iter().copied().map(to_db_sled_policy), - )) - .filter(dsl::sled_state.eq_any(valid_old_states)) - .into_boxed(), - #[cfg(test)] - ValidateTransition::No => query.into_boxed(), - }; + let t = SledTransition::State(new_sled_state); + let valid_old_policies = t.valid_old_policies(); + let valid_old_states = t.valid_old_states(); - let query = query - .set(( - dsl::sled_state.eq(new_state), - dsl::time_modified.eq(Utc::now()), - )) - .check_if_exists::(sled_id); + let query = match check { + ValidateTransition::Yes => query + .filter( + dsl::sled_policy.eq_any( + valid_old_policies + .iter() + .copied() + .map(to_db_sled_policy), + ), + ) + .filter(dsl::sled_state.eq_any(valid_old_states)) + .into_boxed(), + #[cfg(test)] + ValidateTransition::No => query.into_boxed(), + }; - let result = query - .execute_and_check(&*self.pool_connection_authorized(opctx).await?) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + let query = query + .set(( + dsl::sled_state.eq(new_sled_state), + dsl::time_modified.eq(Utc::now()), + )) + .check_if_exists::(sled_id); - match (check, result.status) { - (ValidateTransition::Yes, UpdateStatus::Updated) => { - Ok(result.found.state()) - } - (ValidateTransition::Yes, UpdateStatus::NotUpdatedButExists) => { - // Two reasons this can happen: - // 1. An idempotent update: this is treated as a success. - // 2. Invalid state transition: a failure. - // - // To differentiate between the two, check that the new state - // is the same as the old state, and the found policy is valid. - if result.found.state() == new_state - && valid_old_policies.contains(&result.found.policy()) - { - Ok(result.found.state()) - } else { - Err(TransitionError::InvalidTransition { - current: result.found, - transition: SledTransition::State(new_state), - }) + let result = query.execute_and_check(&conn).await?; + + let old_state = match (check, result.status) { + (ValidateTransition::Yes, UpdateStatus::Updated) => { + result.found.state() + } + ( + ValidateTransition::Yes, + UpdateStatus::NotUpdatedButExists, + ) => { + // Two reasons this can happen: + // 1. An idempotent update: this is treated as a + // success. + // 2. Invalid state transition: a failure. + // + // To differentiate between the two, check that the + // new state is the same as the old state, and the + // found policy is valid. + if result.found.state() == new_sled_state + && valid_old_policies + .contains(&result.found.policy()) + { + result.found.state() + } else { + return Err(err.bail( + TransitionError::InvalidTransition { + current: result.found, + transition: SledTransition::State( + new_sled_state, + ), + }, + )); + } + } + #[cfg(test)] + (ValidateTransition::No, _) => result.found.state(), + }; + + // When a sled is decommissioned, the associated disks with + // that sled should also be implicitly set to + // decommissioned. + let new_disk_state = match new_sled_state { + SledState::Active => None, + SledState::Decommissioned => Some( + nexus_db_model::PhysicalDiskState::Decommissioned, + ), + }; + if let Some(new_disk_state) = new_disk_state { + use db::schema::physical_disk::dsl as physical_disk_dsl; + diesel::update(physical_disk_dsl::physical_disk) + .filter(physical_disk_dsl::time_deleted.is_null()) + .filter(physical_disk_dsl::sled_id.eq(sled_id)) + .set( + physical_disk_dsl::disk_state + .eq(new_disk_state), + ) + .execute_async(&conn) + .await?; + } + + Ok(old_state) } - } - #[cfg(test)] - (ValidateTransition::No, _) => Ok(result.found.state()), - } + }) + .await + .map_err(|e| { + if let Some(err) = err.take() { + return err; + } + TransitionError::from(public_error_from_diesel( + e, + ErrorHandler::Server, + )) + })?; + + Ok(old_state) } } @@ -778,6 +837,7 @@ mod test { use nexus_db_model::PhysicalDisk; use nexus_db_model::PhysicalDiskKind; use nexus_db_model::PhysicalDiskPolicy; + use nexus_db_model::PhysicalDiskState; use nexus_test_utils::db::test_setup_database; use nexus_types::identity::Asset; use omicron_common::api::external; @@ -1108,7 +1168,7 @@ mod test { // Set up a sled to test against. let (sled, _) = datastore.sled_upsert(test_new_sled_update()).await.unwrap(); - let sled_id = sled.id(); + let sled_id = SledUuid::from_untyped_uuid(sled.id()); // Add a couple disks to this sled. // @@ -1121,7 +1181,7 @@ mod test { "serial1".to_string(), "model1".to_string(), PhysicalDiskKind::U2, - sled_id, + sled_id.into_untyped_uuid(), ); let disk2 = PhysicalDisk::new( Uuid::new_v4(), @@ -1129,7 +1189,7 @@ mod test { "serial2".to_string(), "model2".to_string(), PhysicalDiskKind::U2, - sled_id, + sled_id.into_untyped_uuid(), ); datastore @@ -1158,7 +1218,7 @@ mod test { sled_set_policy( &opctx, &datastore, - SledUuid::from_untyped_uuid(sled_id), + sled_id, SledPolicy::Expunged, ValidateTransition::Yes, Expected::Ok(SledPolicy::provisionable()), @@ -1166,7 +1226,7 @@ mod test { .await .expect("Could not expunge sled"); - // Observe that the disk state is now expunged + // Observe that the disk policy is now expunged assert_eq!( PhysicalDiskPolicy::Expunged, lookup_physical_disk(&datastore, disk1.id()).await.disk_policy @@ -1176,6 +1236,29 @@ mod test { lookup_physical_disk(&datastore, disk2.id()).await.disk_policy ); + // We can now decommission the sled, which should also decommission the + // disks. + sled_set_state( + &opctx, + &datastore, + sled_id, + SledState::Decommissioned, + ValidateTransition::Yes, + Expected::Ok(SledState::Active), + ) + .await + .expect("decommissioned sled"); + + // Observe that the disk state is now decommissioned + assert_eq!( + PhysicalDiskState::Decommissioned, + lookup_physical_disk(&datastore, disk1.id()).await.disk_state + ); + assert_eq!( + PhysicalDiskState::Decommissioned, + lookup_physical_disk(&datastore, disk2.id()).await.disk_state + ); + db.cleanup().await.unwrap(); logctx.cleanup_successful(); } diff --git a/nexus/reconfigurator/execution/src/lib.rs b/nexus/reconfigurator/execution/src/lib.rs index 118907390b..f4b257add7 100644 --- a/nexus/reconfigurator/execution/src/lib.rs +++ b/nexus/reconfigurator/execution/src/lib.rs @@ -196,7 +196,7 @@ where String::from(nexus_label), blueprint, &sleds_by_id, - &overrides, + overrides, ) .await .map_err(|e| vec![anyhow!("{}", InlineErrorChain::new(&e))])?; From 30bff3340f20d441bd85b65f12228b0700e0af78 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 3 May 2024 13:48:54 -0400 Subject: [PATCH 12/17] blueprint realization: decommission sleds --- nexus/db-queries/src/db/datastore/mod.rs | 2 + nexus/db-queries/src/db/datastore/sled.rs | 7 +- nexus/reconfigurator/execution/src/lib.rs | 13 ++ .../execution/src/sled_state.rs | 146 ++++++++++++++++++ 4 files changed, 164 insertions(+), 4 deletions(-) create mode 100644 nexus/reconfigurator/execution/src/sled_state.rs diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 4d6b16483d..66599f4703 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -108,6 +108,8 @@ use nexus_db_model::AllSchemaVersions; pub use probe::ProbeInfo; pub use rack::RackInit; pub use silo::Discoverability; +pub use sled::SledTransition; +pub use sled::TransitionError; pub use switch_port::SwitchPortSettingsCombinedResult; pub use virtual_provisioning_collection::StorageType; pub use volume::read_only_resources_associated_with_volume; diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index 8162d31504..2c73149397 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -534,7 +534,7 @@ impl DataStore { &self, opctx: &OpContext, authz_sled: &authz::Sled, - ) -> Result { + ) -> Result { self.sled_set_state_impl( opctx, authz_sled, @@ -542,7 +542,6 @@ impl DataStore { ValidateTransition::Yes, ) .await - .map_err(|error| error.into_external_error()) } pub(super) async fn sled_set_state_impl( @@ -682,7 +681,7 @@ impl DataStore { // valid for a new policy or state, except idempotent transitions. #[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(super) enum SledTransition { +pub enum SledTransition { Policy(SledPolicy), State(SledState), } @@ -773,7 +772,7 @@ impl IntoEnumIterator for SledTransition { /// An error that occurred while setting a policy or state. #[derive(Debug, Error)] #[must_use] -pub(super) enum TransitionError { +pub enum TransitionError { /// The state transition check failed. /// /// The sled is returned. diff --git a/nexus/reconfigurator/execution/src/lib.rs b/nexus/reconfigurator/execution/src/lib.rs index f4b257add7..8ac8bc4399 100644 --- a/nexus/reconfigurator/execution/src/lib.rs +++ b/nexus/reconfigurator/execution/src/lib.rs @@ -12,6 +12,7 @@ use nexus_db_queries::db::DataStore; use nexus_types::deployment::Blueprint; use nexus_types::deployment::BlueprintZoneFilter; use nexus_types::deployment::SledFilter; +use nexus_types::external_api::views::SledState; use nexus_types::identity::Asset; use omicron_common::address::Ipv6Subnet; use omicron_common::address::SLED_PREFIX; @@ -29,6 +30,7 @@ mod external_networking; mod omicron_physical_disks; mod omicron_zones; mod overridables; +mod sled_state; pub use dns::blueprint_external_dns_config; pub use dns::blueprint_internal_dns_config; @@ -201,5 +203,16 @@ where .await .map_err(|e| vec![anyhow!("{}", InlineErrorChain::new(&e))])?; + sled_state::decommission_sleds( + &opctx, + datastore, + blueprint + .sled_state + .iter() + .filter(|&(_, &state)| state == SledState::Decommissioned) + .map(|(&sled_id, _)| sled_id), + ) + .await?; + Ok(()) } diff --git a/nexus/reconfigurator/execution/src/sled_state.rs b/nexus/reconfigurator/execution/src/sled_state.rs new file mode 100644 index 0000000000..aaa5b6bc26 --- /dev/null +++ b/nexus/reconfigurator/execution/src/sled_state.rs @@ -0,0 +1,146 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Updates sled states required by a given blueprint + +use anyhow::Context; +use nexus_db_model::SledState; +use nexus_db_queries::authz::Action; +use nexus_db_queries::context::OpContext; +use nexus_db_queries::db::datastore::TransitionError; +use nexus_db_queries::db::lookup::LookupPath; +use nexus_db_queries::db::DataStore; +use omicron_uuid_kinds::GenericUuid; +use omicron_uuid_kinds::SledUuid; + +pub(crate) async fn decommission_sleds( + opctx: &OpContext, + datastore: &DataStore, + sled_ids_to_decommission: impl Iterator, +) -> Result<(), Vec> { + let mut errors = Vec::new(); + + for sled_id in sled_ids_to_decommission { + if let Err(err) = decommission_one_sled(opctx, datastore, sled_id).await + { + errors.push(err); + } + } + + if errors.is_empty() { + Ok(()) + } else { + Err(errors) + } +} + +async fn decommission_one_sled( + opctx: &OpContext, + datastore: &DataStore, + sled_id: SledUuid, +) -> anyhow::Result<()> { + let (authz_sled,) = LookupPath::new(opctx, datastore) + .sled_id(sled_id.into_untyped_uuid()) + .lookup_for(Action::Modify) + .await + .with_context(|| { + format!("failed to look up sled {sled_id} for modification") + })?; + match datastore.sled_set_state_to_decommissioned(opctx, &authz_sled).await { + Ok(_) => Ok(()), + // `sled_set_state_to_decommissioned` is not idempotent. If we're racing + // another Nexus (or we're repeating realization of a blueprint we've + // already realized), this sled may already be decommissioned; that's + // fine. + Err(TransitionError::InvalidTransition { current, .. }) + if current.state() == SledState::Decommissioned => + { + Ok(()) + } + Err(err) => Err(anyhow::Error::new(err) + .context(format!("failed to decommission sled {sled_id}"))), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use nexus_test_utils_macros::nexus_test; + use nexus_types::deployment::SledFilter; + use nexus_types::identity::Asset; + + type ControlPlaneTestContext = + nexus_test_utils::ControlPlaneTestContext; + + async fn list_all_commissioned_sled_ids( + opctx: &OpContext, + datastore: &DataStore, + ) -> Vec { + datastore + .sled_list_all_batched(&opctx, SledFilter::Commissioned) + .await + .expect("listing sleds") + .into_iter() + .map(|sled| SledUuid::from_untyped_uuid(sled.id())) + .collect() + } + + #[nexus_test] + async fn test_decommission_is_idempotent( + cptestctx: &ControlPlaneTestContext, + ) { + let nexus = &cptestctx.server.apictx().nexus; + let datastore = nexus.datastore(); + let opctx = OpContext::for_tests( + cptestctx.logctx.log.clone(), + datastore.clone(), + ); + + let mut commissioned_sled_ids = + list_all_commissioned_sled_ids(&opctx, datastore).await; + + // Pick a sled to decommission. + let decommissioned_sled_id = + commissioned_sled_ids.pop().expect("at least one sled"); + + // Expunge the sled (required prior to decommissioning). + let (authz_sled,) = LookupPath::new(&opctx, datastore) + .sled_id(decommissioned_sled_id.into_untyped_uuid()) + .lookup_for(Action::Modify) + .await + .expect("lookup authz_sled"); + datastore + .sled_set_policy_to_expunged(&opctx, &authz_sled) + .await + .expect("expunged sled"); + + // Decommission the sled. + decommission_sleds( + &opctx, + datastore, + std::iter::once(decommissioned_sled_id), + ) + .await + .expect("decommissioned sled"); + + // Ensure the sled was marked decommissioned in the db. + assert_eq!( + commissioned_sled_ids, + list_all_commissioned_sled_ids(&opctx, datastore).await + ); + + // Try to decommission the sled again; this should be fine. + decommission_sleds( + &opctx, + datastore, + std::iter::once(decommissioned_sled_id), + ) + .await + .expect("decommissioned sled"); + assert_eq!( + commissioned_sled_ids, + list_all_commissioned_sled_ids(&opctx, datastore).await + ); + } +} From e6e6fefd21a2b6731bd4da8927c7fb630594378e Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 3 May 2024 14:43:12 -0400 Subject: [PATCH 13/17] prune fully decommissioned sleds from blueprints --- .../planning/src/blueprint_builder/builder.rs | 97 ++++++++++++++++++- 1 file changed, 95 insertions(+), 2 deletions(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index df486ac5bf..bb20b38df7 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -343,13 +343,28 @@ impl<'a> BlueprintBuilder<'a> { // Prefer the sled state from our parent blueprint for sleds // that were in it; there may be new sleds in `input`, in which // case we'll use their current state as our starting point. - // - // TODO-correctness When can we drop decommissioned sleds from this map? let mut sled_state = parent_blueprint.sled_state.clone(); + let mut commissioned_sled_ids = BTreeSet::new(); for (sled_id, details) in input.all_sleds(SledFilter::Commissioned) { + commissioned_sled_ids.insert(sled_id); sled_state.entry(sled_id).or_insert(details.state); } + // Make a garbage collection pass through `sled_state`. We want to keep + // any sleds which either: + // + // 1. do not have a desired state of `Decommissioned` + // 2. do have a desired state of `Decommissioned` and are still included + // in our input's list of commissioned sleds + // + // Sleds that don't fall into either of these cases have reached the + // actual `Decommissioned` state, which means we no longer need to carry + // forward that desired state. + sled_state.retain(|sled_id, state| { + *state != SledState::Decommissioned + || commissioned_sled_ids.contains(sled_id) + }); + Ok(BlueprintBuilder { log, parent_blueprint, @@ -1197,6 +1212,7 @@ pub mod test { use crate::system::SledBuilder; use expectorate::assert_contents; use nexus_types::deployment::BlueprintZoneFilter; + use nexus_types::external_api::views::SledPolicy; use omicron_common::address::IpRange; use omicron_test_utils::dev::test_setup_log; use std::collections::BTreeSet; @@ -1403,6 +1419,83 @@ pub mod test { logctx.cleanup_successful(); } + #[test] + fn test_prune_decommissioned_sleds() { + static TEST_NAME: &str = + "blueprint_builder_test_prune_decommissioned_sleds"; + let logctx = test_setup_log(TEST_NAME); + let (_, input, mut blueprint1) = + example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); + verify_blueprint(&blueprint1); + + // Mark one sled as having a desired state of decommissioned. + let decommision_sled_id = blueprint1 + .sled_state + .keys() + .copied() + .next() + .expect("at least one sled"); + *blueprint1.sled_state.get_mut(&decommision_sled_id).unwrap() = + SledState::Decommissioned; + + // Change the input to note that the sled is expunged, but still active. + let mut builder = input.into_builder(); + builder.sleds_mut().get_mut(&decommision_sled_id).unwrap().policy = + SledPolicy::Expunged; + builder.sleds_mut().get_mut(&decommision_sled_id).unwrap().state = + SledState::Active; + let input = builder.build(); + + // Generate a new blueprint. This sled should still be included: even + // though the desired state is decommissioned, the current state is + // still active, so we should carry it forward. + let blueprint2 = BlueprintBuilder::new_based_on( + &logctx.log, + &blueprint1, + &input, + "test_prune_decommissioned_sleds", + ) + .expect("created builder") + .build(); + verify_blueprint(&blueprint2); + + // We carried forward the desired state. + assert_eq!( + blueprint2.sled_state.get(&decommision_sled_id).copied(), + Some(SledState::Decommissioned) + ); + + // Change the input to mark the sled decommissioned. (Normally realizing + // blueprint2 would make this change.) + let mut builder = input.into_builder(); + builder.sleds_mut().get_mut(&decommision_sled_id).unwrap().state = + SledState::Decommissioned; + let input = builder.build(); + + // Generate a new blueprint. This desired sled state should no longer be + // present: it has reached the terminal decommissioned state, so there's + // no more work to be done. + let blueprint3 = BlueprintBuilder::new_based_on( + &logctx.log, + &blueprint2, + &input, + "test_prune_decommissioned_sleds", + ) + .expect("created builder") + .build(); + verify_blueprint(&blueprint3); + + // Ensure we've dropped the decommissioned sled. (We may still have + // _zones_ for it that need cleanup work, but all state transitions for + // it are complete.) + assert_eq!( + blueprint3.sled_state.get(&decommision_sled_id).copied(), + None, + ); + + logctx.cleanup_successful(); + } + #[test] fn test_add_physical_disks() { static TEST_NAME: &str = "blueprint_builder_test_add_physical_disks"; From 8d08368d5cf4b0c1e10cb51e8ff3b56b14223650 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 13 May 2024 13:49:21 -0400 Subject: [PATCH 14/17] clarify omdb db sleds output --- dev-tools/omdb/src/bin/omdb/db.rs | 5 ++++- dev-tools/omdb/tests/env.out | 6 +++--- dev-tools/omdb/tests/successes.out | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index ae94442437..5930788cd2 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -1455,7 +1455,10 @@ async fn cmd_db_sleds( let filter = match args.filter { Some(filter) => filter, None => { - eprintln!("note: listing all sleds (use -F to filter, e.g. -F in-service)"); + eprintln!( + "note: listing all commissioned sleds \ + (use -F to filter, e.g. -F in-service)" + ); SledFilter::Commissioned } }; diff --git a/dev-tools/omdb/tests/env.out b/dev-tools/omdb/tests/env.out index a224155bf9..5716510602 100644 --- a/dev-tools/omdb/tests/env.out +++ b/dev-tools/omdb/tests/env.out @@ -9,7 +9,7 @@ stdout: stderr: note: using database URL postgresql://root@[::1]:REDACTED_PORT/omicron?sslmode=disable note: database schema version matches expected () -note: listing all sleds (use -F to filter, e.g. -F in-service) +note: listing all commissioned sleds (use -F to filter, e.g. -F in-service) ============================================= EXECUTING COMMAND: omdb ["db", "--db-url", "junk", "sleds"] termination: Exited(2) @@ -341,7 +341,7 @@ note: database URL not specified. Will search DNS. note: (override with --db-url or OMDB_DB_URL) note: using database URL postgresql://root@[::1]:REDACTED_PORT/omicron?sslmode=disable note: database schema version matches expected () -note: listing all sleds (use -F to filter, e.g. -F in-service) +note: listing all commissioned sleds (use -F to filter, e.g. -F in-service) ============================================= EXECUTING COMMAND: omdb ["--dns-server", "[::1]:REDACTED_PORT", "db", "sleds"] termination: Exited(0) @@ -356,5 +356,5 @@ note: database URL not specified. Will search DNS. note: (override with --db-url or OMDB_DB_URL) note: using database URL postgresql://root@[::1]:REDACTED_PORT/omicron?sslmode=disable note: database schema version matches expected () -note: listing all sleds (use -F to filter, e.g. -F in-service) +note: listing all commissioned sleds (use -F to filter, e.g. -F in-service) ============================================= diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out index d7711610bd..0aa47f2712 100644 --- a/dev-tools/omdb/tests/successes.out +++ b/dev-tools/omdb/tests/successes.out @@ -80,7 +80,7 @@ stdout: stderr: note: using database URL postgresql://root@[::1]:REDACTED_PORT/omicron?sslmode=disable note: database schema version matches expected () -note: listing all sleds (use -F to filter, e.g. -F in-service) +note: listing all commissioned sleds (use -F to filter, e.g. -F in-service) ============================================= EXECUTING COMMAND: omdb ["db", "sleds", "-F", "discretionary"] termination: Exited(0) From 64e8660246d803a3ac9a9e72ff7ffd38f126b42c Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 13 May 2024 13:49:47 -0400 Subject: [PATCH 15/17] clarify BlueprintBuilder::into_zones_map() --- .../planning/src/blueprint_builder/builder.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index bb20b38df7..ca3f70bd89 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -1080,10 +1080,11 @@ impl<'a> BlueprintZonesBuilder<'a> { } } - /// Produces an owned map of zones for the requested sleds + /// Produces an owned map of zones for the sleds recorded in this blueprint + /// plus any newly-added sleds pub fn into_zones_map( self, - commissioned_sled_ids: impl Iterator, + added_sled_ids: impl Iterator, ) -> BTreeMap { // Start with self.changed_zones, which contains entries for any // sled whose zones config is changing in this blueprint. @@ -1100,7 +1101,7 @@ impl<'a> BlueprintZonesBuilder<'a> { } // Finally, insert any newly-added sleds. - for sled_id in commissioned_sled_ids { + for sled_id in added_sled_ids { zones.entry(sled_id).or_insert_with(|| BlueprintZonesConfig { generation: Generation::new(), zones: vec![], From de8a0274195312698ef8c2081aff6e52ae4c23af Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 13 May 2024 13:51:32 -0400 Subject: [PATCH 16/17] minor cleanup from PR feedback --- nexus/db-queries/src/db/datastore/sled.rs | 4 ++++ nexus/reconfigurator/planning/src/planner.rs | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index 2c73149397..bf43b9182d 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -636,6 +636,10 @@ impl DataStore { // When a sled is decommissioned, the associated disks with // that sled should also be implicitly set to // decommissioned. + // + // We use an explicit `match` to force ourselves to consider + // disk state if we add any addition sled states in the + // future. let new_disk_state = match new_sled_state { SledState::Active => None, SledState::Decommissioned => Some( diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 3d7f61e96f..b879fb99f8 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -19,6 +19,7 @@ use nexus_types::external_api::views::SledPolicy; use nexus_types::external_api::views::SledState; use nexus_types::inventory::Collection; use omicron_uuid_kinds::SledUuid; +use slog::error; use slog::{info, warn, Logger}; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -112,7 +113,7 @@ impl<'a> Planner<'a> { // up when we ask for commissioned sleds? Warn, but don't try to // decommission it again. (SledPolicy::Expunged, SledState::Decommissioned) => { - warn!( + error!( self.log, "decommissioned sled returned by \ SledFilter::Commissioned"; From 1e629df40bbf75bf0af592f7e209eed16dc749ab Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 13 May 2024 14:00:37 -0400 Subject: [PATCH 17/17] planner: finding a decommissioned sleds with non-expunged zones is an error --- .../planning/src/blueprint_builder/builder.rs | 8 +++ nexus/reconfigurator/planning/src/planner.rs | 52 ++++++++++++------- .../output/planner_nonprovisionable_1_2.txt | 52 +++++-------------- .../output/planner_nonprovisionable_2_2a.txt | 2 +- .../output/planner_nonprovisionable_bp2.txt | 12 ++--- 5 files changed, 61 insertions(+), 65 deletions(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index ca3f70bd89..7e34bf9691 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -88,6 +88,14 @@ pub enum Error { NoSystemMacAddressAvailable, #[error("exhausted available Nexus IP addresses")] ExhaustedNexusIps, + #[error( + "invariant violation: found decommissioned sled with \ + {num_zones} non-expunged zones: {sled_id}" + )] + DecommissionedSledWithNonExpungedZones { + sled_id: SledUuid, + num_zones: usize, + }, #[error("programming error in planner")] Planner(#[from] anyhow::Error), } diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index b879fb99f8..5535b28910 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -171,16 +171,26 @@ impl<'a> Planner<'a> { // Check for any decommissioned sleds (i.e., sleds for which our // blueprint has zones, but are not in the input sled list). Any zones - // for decommissioned sleds _should_ already be expunged; ensure that is - // the case. + // for decommissioned sleds must have already be expunged for + // decommissioning to have happened; fail if we find non-expunged zones + // associated with a decommissioned sled. for sled_id in self.blueprint.sled_ids_with_zones() { if !commissioned_sled_ids.contains(&sled_id) { - self.blueprint.expunge_all_zones_for_sled( - sled_id, - // Decommissioned sleds are not present in our input, so we - // don't know what its policy was. - ZoneExpungeReason::SledDecommissioned { policy: None }, - )?; + let num_zones = self + .blueprint + .current_sled_zones(sled_id) + .filter(|zone| { + zone.disposition != BlueprintZoneDisposition::Expunged + }) + .count(); + if num_zones > 0 { + return Err( + Error::DecommissionedSledWithNonExpungedZones { + sled_id, + num_zones, + }, + ); + } } } @@ -456,9 +466,7 @@ fn needs_zone_expungement( // an illegal state, but representable. If we see a sled in this // state, we should still expunge all zones in it, but parent code // should warn on it. - return Some(ZoneExpungeReason::SledDecommissioned { - policy: Some(policy), - }); + return Some(ZoneExpungeReason::SledDecommissioned { policy }); } } @@ -474,7 +482,7 @@ fn needs_zone_expungement( /// logical flow. #[derive(Copy, Clone, Debug)] pub(crate) enum ZoneExpungeReason { - SledDecommissioned { policy: Option }, + SledDecommissioned { policy: SledPolicy }, SledExpunged, } @@ -938,7 +946,7 @@ mod test { // and decommissioned sleds. (When we add more kinds of // non-provisionable states in the future, we'll have to add more // sleds.) - let (collection, input, blueprint1) = + let (collection, input, mut blueprint1) = example(&logctx.log, TEST_NAME, 5); // This blueprint should only have 5 Nexus zones: one on each sled. @@ -976,6 +984,17 @@ mod test { let decommissioned_sled_id = { let (sled_id, details) = sleds_iter.next().expect("no sleds"); details.state = SledState::Decommissioned; + + // Decommissioned sleds can only occur if their zones have been + // expunged, so lie and pretend like that already happened + // (otherwise the planner will rightfully fail to generate a new + // blueprint, because we're feeding it invalid inputs). + for zone in + &mut blueprint1.blueprint_zones.get_mut(sled_id).unwrap().zones + { + zone.disposition = BlueprintZoneDisposition::Expunged; + } + *sled_id }; println!("1 -> 2: decommissioned {decommissioned_sled_id}"); @@ -1037,13 +1056,6 @@ mod test { let expunged_modified = sleds.remove(&expunged_sled_id).unwrap(); assert_all_zones_expunged(&expunged_modified, "expunged sled"); - let decommissioned_modified = - sleds.remove(&decommissioned_sled_id).unwrap(); - assert_all_zones_expunged( - &decommissioned_modified, - "decommissioned sled", - ); - // Only 2 of the 3 remaining sleds (not the non-provisionable sled) // should get additional Nexus zones. We expect a total of 6 new Nexus // zones, which should be split evenly between the two sleds, while the diff --git a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_1_2.txt b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_1_2.txt index ecc5b125d9..a87243733f 100644 --- a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_1_2.txt +++ b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_1_2.txt @@ -21,6 +21,20 @@ to: blueprint 9f71f5d3-a272-4382-9154-6ea2e171a6c6 internal_ntp 7f4e9f9f-08f8-4d14-885d-e977c05525ad in service fd00:1122:3344:105::21 nexus 6dff7633-66bb-4924-a6ff-2c896e66964b in service fd00:1122:3344:105::22 + sled 68d24ac5-f341-49ea-a92a-0381b52ab387: blueprint zones at generation 2 + crucible 3b3c14b6-a8e2-4054-a577-8d96cb576230 expunged fd00:1122:3344:102::2c + crucible 47a87c6e-ef45-4d52-9a3e-69cdd96737cc expunged fd00:1122:3344:102::23 + crucible 6464d025-4652-4948-919e-740bec5699b1 expunged fd00:1122:3344:102::24 + crucible 6939ce48-b17c-4616-b176-8a419a7697be expunged fd00:1122:3344:102::29 + crucible 878dfddd-3113-4197-a3ea-e0d4dbe9b476 expunged fd00:1122:3344:102::25 + crucible 8d4d2b28-82bb-4e36-80da-1408d8c35d82 expunged fd00:1122:3344:102::2b + crucible 9fd52961-426f-4e62-a644-b70871103fca expunged fd00:1122:3344:102::26 + crucible b44cdbc0-0ce0-46eb-8b21-a09e113aa1d0 expunged fd00:1122:3344:102::27 + crucible b6b759d0-f60d-42b7-bbbc-9d61c9e895a9 expunged fd00:1122:3344:102::28 + crucible c407795c-6c8b-428e-8ab8-b962913c447f expunged fd00:1122:3344:102::2a + internal_ntp f3f2e4f3-0985-4ef6-8336-ce479382d05d expunged fd00:1122:3344:102::21 + nexus 01d58626-e1b0-480f-96be-ac784863c7dc expunged fd00:1122:3344:102::22 + MODIFIED SLEDS: * sled 48d95fef-bc9f-4f50-9a53-1e075836291d: blueprint zones at generation: 2 -> 3 @@ -61,44 +75,6 @@ to: blueprint 9f71f5d3-a272-4382-9154-6ea2e171a6c6 + ├─ expunged fd00:1122:3344:103::22 * └─ changed: disposition -* sled 68d24ac5-f341-49ea-a92a-0381b52ab387: blueprint zones at generation: 2 -> 3 -- crucible 3b3c14b6-a8e2-4054-a577-8d96cb576230 in service fd00:1122:3344:102::2c modified -+ ├─ expunged fd00:1122:3344:102::2c -* └─ changed: disposition -- crucible 47a87c6e-ef45-4d52-9a3e-69cdd96737cc in service fd00:1122:3344:102::23 modified -+ ├─ expunged fd00:1122:3344:102::23 -* └─ changed: disposition -- crucible 6464d025-4652-4948-919e-740bec5699b1 in service fd00:1122:3344:102::24 modified -+ ├─ expunged fd00:1122:3344:102::24 -* └─ changed: disposition -- crucible 6939ce48-b17c-4616-b176-8a419a7697be in service fd00:1122:3344:102::29 modified -+ ├─ expunged fd00:1122:3344:102::29 -* └─ changed: disposition -- crucible 878dfddd-3113-4197-a3ea-e0d4dbe9b476 in service fd00:1122:3344:102::25 modified -+ ├─ expunged fd00:1122:3344:102::25 -* └─ changed: disposition -- crucible 8d4d2b28-82bb-4e36-80da-1408d8c35d82 in service fd00:1122:3344:102::2b modified -+ ├─ expunged fd00:1122:3344:102::2b -* └─ changed: disposition -- crucible 9fd52961-426f-4e62-a644-b70871103fca in service fd00:1122:3344:102::26 modified -+ ├─ expunged fd00:1122:3344:102::26 -* └─ changed: disposition -- crucible b44cdbc0-0ce0-46eb-8b21-a09e113aa1d0 in service fd00:1122:3344:102::27 modified -+ ├─ expunged fd00:1122:3344:102::27 -* └─ changed: disposition -- crucible b6b759d0-f60d-42b7-bbbc-9d61c9e895a9 in service fd00:1122:3344:102::28 modified -+ ├─ expunged fd00:1122:3344:102::28 -* └─ changed: disposition -- crucible c407795c-6c8b-428e-8ab8-b962913c447f in service fd00:1122:3344:102::2a modified -+ ├─ expunged fd00:1122:3344:102::2a -* └─ changed: disposition -- internal_ntp f3f2e4f3-0985-4ef6-8336-ce479382d05d in service fd00:1122:3344:102::21 modified -+ ├─ expunged fd00:1122:3344:102::21 -* └─ changed: disposition -- nexus 01d58626-e1b0-480f-96be-ac784863c7dc in service fd00:1122:3344:102::22 modified -+ ├─ expunged fd00:1122:3344:102::22 -* └─ changed: disposition - * sled 75bc286f-2b4b-482c-9431-59272af529da: blueprint zones at generation: 2 -> 3 crucible 15bb9def-69b8-4d2e-b04f-9fee1143387c in service fd00:1122:3344:104::25 crucible 23a8fa2b-ef3e-4017-a43f-f7a83953bd7c in service fd00:1122:3344:104::2c diff --git a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_2_2a.txt b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_2_2a.txt index 00ca05b4b8..ec6c505c87 100644 --- a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_2_2a.txt +++ b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_2_2a.txt @@ -43,7 +43,7 @@ to: blueprint 9f71f5d3-a272-4382-9154-6ea2e171a6c6 REMOVED SLEDS: -- sled 68d24ac5-f341-49ea-a92a-0381b52ab387: blueprint zones at generation 3 +- sled 68d24ac5-f341-49ea-a92a-0381b52ab387: blueprint zones at generation 2 - crucible 3b3c14b6-a8e2-4054-a577-8d96cb576230 expunged fd00:1122:3344:102::2c removed - crucible 47a87c6e-ef45-4d52-9a3e-69cdd96737cc expunged fd00:1122:3344:102::23 removed - crucible 6464d025-4652-4948-919e-740bec5699b1 expunged fd00:1122:3344:102::24 removed diff --git a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt index 623bf0a756..294a12f77a 100644 --- a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt +++ b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt @@ -33,7 +33,7 @@ parent: 4d4e6c38-cd95-4c4e-8f45-6af4d686964b internal_ntp 67d913e0-0005-4599-9b28-0abbf6cc2916 expunged fd00:1122:3344:103::21 nexus 2aa0ea4f-3561-4989-a98c-9ab7d9a240fb expunged fd00:1122:3344:103::22 - sled 68d24ac5-f341-49ea-a92a-0381b52ab387: blueprint zones at generation 3 + sled 68d24ac5-f341-49ea-a92a-0381b52ab387: blueprint zones at generation 2 crucible 3b3c14b6-a8e2-4054-a577-8d96cb576230 expunged fd00:1122:3344:102::2c crucible 47a87c6e-ef45-4d52-9a3e-69cdd96737cc expunged fd00:1122:3344:102::23 crucible 6464d025-4652-4948-919e-740bec5699b1 expunged fd00:1122:3344:102::24 @@ -82,8 +82,8 @@ parent: 4d4e6c38-cd95-4c4e-8f45-6af4d686964b nexus c26b3bda-5561-44a1-a69f-22103fe209a1 in service fd00:1122:3344:101::2f METADATA: - created by: test_blueprint2 - created at: 1970-01-01T00:00:00.000Z - comment: sled 48d95fef-bc9f-4f50-9a53-1e075836291d (sled policy is expunged): 12 zones expunged, sled 68d24ac5-f341-49ea-a92a-0381b52ab387 (sled state is decommissioned): 12 zones expunged, sled 2d1cb4f2-cf44-40fc-b118-85036eb732a9: altered disks, sled 75bc286f-2b4b-482c-9431-59272af529da: altered disks, sled affab35f-600a-4109-8ea0-34a067a4e0bc: altered disks - internal DNS version: 1 - external DNS version: 1 + created by: test_blueprint2 + created at: 1970-01-01T00:00:00.000Z + comment: sled 48d95fef-bc9f-4f50-9a53-1e075836291d (sled policy is expunged): 12 zones expunged, sled 2d1cb4f2-cf44-40fc-b118-85036eb732a9: altered disks, sled 75bc286f-2b4b-482c-9431-59272af529da: altered disks, sled affab35f-600a-4109-8ea0-34a067a4e0bc: altered disks + internal DNS version: 1 + external DNS version: 1