diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 34816548d4..fd297fa9c9 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -60,6 +60,7 @@ use omicron_common::api::external::TufRepoDescription; use omicron_common::api::external::Vni; use omicron_common::api::internal::shared::NetworkInterface; use omicron_common::api::internal::shared::NetworkInterfaceKind; +use omicron_common::policy::COCKROACHDB_REDUNDANCY; use omicron_common::policy::INTERNAL_DNS_REDUNDANCY; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::MupdateOverrideUuid; @@ -387,14 +388,12 @@ impl fmt::Display for Operation { /// However, the new blueprint can only be made the system's target if its /// parent is the current target. pub struct BlueprintBuilder<'a> { - #[allow(dead_code)] log: Logger, /// previous blueprint, on which this one will be based parent_blueprint: &'a Blueprint, /// The latest inventory collection - #[allow(unused)] collection: &'a Collection, // These fields are used to allocate resources for sleds. @@ -1179,11 +1178,12 @@ impl<'a> BlueprintBuilder<'a> { gz_address: dns_subnet.gz_address(), gz_address_index, }); - let image_source = self.zone_image_source(zone_type.kind()); + let id = self.rng.sled_rng(sled_id).next_zone(); + let image_source = self.zone_image_source(id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, - id: self.rng.sled_rng(sled_id).next_zone(), + id, filesystem_pool: zpool, zone_type, image_source, @@ -1232,7 +1232,7 @@ impl<'a> BlueprintBuilder<'a> { dns_address, nic, }); - let image_source = self.zone_image_source(zone_type.kind()); + let image_source = self.zone_image_source(id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -1272,11 +1272,12 @@ impl<'a> BlueprintBuilder<'a> { }); let filesystem_pool = self.sled_select_zpool(sled_id, zone_type.kind())?; - let image_source = self.zone_image_source(zone_type.kind()); + let id = self.rng.sled_rng(sled_id).next_zone(); + let image_source = self.zone_image_source(id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, - id: self.rng.sled_rng(sled_id).next_zone(), + id, filesystem_pool, zone_type, image_source, @@ -1425,7 +1426,7 @@ impl<'a> BlueprintBuilder<'a> { }); let filesystem_pool = self.sled_select_zpool(sled_id, zone_type.kind())?; - let image_source = self.zone_image_source(zone_type.kind()); + let image_source = self.zone_image_source(nexus_id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -1451,7 +1452,8 @@ impl<'a> BlueprintBuilder<'a> { }); let filesystem_pool = self.sled_select_zpool(sled_id, zone_type.kind())?; - let image_source = self.zone_image_source(zone_type.kind()); + let image_source = + self.zone_image_source(oximeter_id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -1476,7 +1478,7 @@ impl<'a> BlueprintBuilder<'a> { ); let filesystem_pool = self.sled_select_zpool(sled_id, zone_type.kind())?; - let image_source = self.zone_image_source(zone_type.kind()); + let image_source = self.zone_image_source(pantry_id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -1511,7 +1513,7 @@ impl<'a> BlueprintBuilder<'a> { dataset: OmicronZoneDataset { pool_name }, }); let filesystem_pool = pool_name; - let image_source = self.zone_image_source(zone_type.kind()); + let image_source = self.zone_image_source(zone_id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -1538,7 +1540,7 @@ impl<'a> BlueprintBuilder<'a> { address, dataset: OmicronZoneDataset { pool_name }, }); - let image_source = self.zone_image_source(zone_type.kind()); + let image_source = self.zone_image_source(id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -1567,7 +1569,7 @@ impl<'a> BlueprintBuilder<'a> { }, ); let filesystem_pool = pool_name; - let image_source = self.zone_image_source(zone_type.kind()); + let image_source = self.zone_image_source(zone_id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -1596,7 +1598,7 @@ impl<'a> BlueprintBuilder<'a> { }, ); let filesystem_pool = pool_name; - let image_source = self.zone_image_source(zone_type.kind()); + let image_source = self.zone_image_source(zone_id, zone_type.kind()); let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -1723,7 +1725,8 @@ impl<'a> BlueprintBuilder<'a> { }); let filesystem_pool = self.sled_select_zpool(sled_id, zone_type.kind())?; - let image_source = self.zone_image_source(zone_type.kind()); + let image_source = + self.zone_image_source(new_zone_id, zone_type.kind()); self.sled_add_zone( sled_id, @@ -1999,15 +2002,19 @@ impl<'a> BlueprintBuilder<'a> { /// Try to find an artifact in either the current or previous release repo /// that contains an image for a zone of the given kind; see RFD 565 ยง9. /// Defaults to the install dataset. + /// + /// Takes an optional "zone_id" as input, to avoid downgrading zones which + /// have already committed to using the new repo. pub(crate) fn zone_image_source( &self, + zone_id: OmicronZoneUuid, zone_kind: ZoneKind, ) -> BlueprintZoneImageSource { let new_repo = self.input.tuf_repo().description(); let old_repo = self.input.old_repo().and_then(|repo| repo.description()); Self::zone_image_artifact( - if self.zone_is_ready_for_update(zone_kind, new_repo) { + if self.zone_should_use_new_repo(zone_id, zone_kind, new_repo) { new_repo } else { old_repo @@ -2016,14 +2023,31 @@ impl<'a> BlueprintBuilder<'a> { ) } - /// Return `true` iff a zone of the given kind is ready to be updated; + /// Return `true` iff a zone of the given kind is ready to use the new repo, /// i.e., its dependencies have been updated, or its data sufficiently /// replicated, etc. - fn zone_is_ready_for_update( + fn zone_should_use_new_repo( &self, + zone_id: OmicronZoneUuid, zone_kind: ZoneKind, new_repo: Option<&TufRepoDescription>, ) -> bool { + // If the zone is already using the new repo, keep using it. + // + // Otherwise, consider whether we should upgrade. + if new_repo.is_some() + && self + .parent_blueprint + .all_omicron_zones(BlueprintZoneDisposition::is_in_service) + .any(|(_, zone)| { + zone.id == zone_id + && zone.image_source + == Self::zone_image_artifact(new_repo, zone_kind) + }) + { + return true; + }; + match zone_kind { ZoneKind::Nexus => { // Nexus can only be updated if all non-Nexus zones have been updated, @@ -2040,8 +2064,31 @@ impl<'a> BlueprintBuilder<'a> { }) }) } - // - // ZoneKind::CockroachDb => todo!("check cluster status in inventory"), + ZoneKind::CockroachDb => { + // We only update CockroachDb if the latest inventory + // collection has observed "no underreplicated ranges". + // + // It's always possible that a TOCTTOU issue causes this to + // change after we decide the zone can be updated - the + // control plane must have some degree of tolerance to + // failures, even mid-update - but this prevents our system + // from self-sabotage when range underreplication is already + // present. + if let (Some(ranges_underreplicated), Some(live_nodes)) = ( + self.collection.cockroach_status.ranges_underreplicated, + self.collection.cockroach_status.liveness_live_nodes, + ) { + return ranges_underreplicated == 0 + && live_nodes == COCKROACHDB_REDUNDANCY as u64; + } + + // If we can't read stats from Cockroach, OR the number of + // under-replicated ranges is non-zero, refuse to update. + // + // It's possible in either of these scenarios that we have + // degraded redundancy, and need to recover. + false + } _ => true, // other zone kinds have no special dependencies } } diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 215cdbc488..4fbf296bd3 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -1063,8 +1063,10 @@ impl<'a> Planner<'a> { ) .filter_map(move |zone| { (zone.image_source - != blueprint - .zone_image_source(zone.zone_type.kind())) + != blueprint.zone_image_source( + zone.id, + zone.zone_type.kind(), + )) .then(|| (sled_id, zone.clone())) }) }) @@ -1085,7 +1087,7 @@ impl<'a> Planner<'a> { zone: &BlueprintZoneConfig, ) -> Result<(), Error> { let zone_kind = zone.zone_type.kind(); - let image_source = self.blueprint.zone_image_source(zone_kind); + let image_source = self.blueprint.zone_image_source(zone.id, zone_kind); if zone.image_source == image_source { // This should only happen in the event of a planning error above. error!( @@ -1289,6 +1291,7 @@ pub(crate) mod test { use omicron_common::api::external::TufRepoMeta; use omicron_common::disk::DatasetKind; use omicron_common::disk::DiskIdentity; + use omicron_common::policy::COCKROACHDB_REDUNDANCY; use omicron_common::policy::CRUCIBLE_PANTRY_REDUNDANCY; use omicron_common::policy::NEXUS_REDUNDANCY; use omicron_common::update::ArtifactId; @@ -5022,6 +5025,225 @@ pub(crate) mod test { logctx.cleanup_successful(); } + #[test] + fn test_update_cockroach() { + static TEST_NAME: &str = "update_cockroach"; + let logctx = test_setup_log(TEST_NAME); + let log = logctx.log.clone(); + + // Use our example system. + let mut rng = SimRngState::from_seed(TEST_NAME); + let (mut example, mut blueprint) = ExampleSystemBuilder::new_with_rng( + &logctx.log, + rng.next_system_rng(), + ) + .build(); + + // Update the example system and blueprint, as a part of test set-up. + // + // Ask for COCKROACHDB_REDUNDANCY cockroach nodes + + let mut input_builder = example.input.clone().into_builder(); + input_builder.policy_mut().target_cockroachdb_zone_count = + COCKROACHDB_REDUNDANCY; + example.input = input_builder.build(); + + let blueprint_name = "blueprint_with_cockroach"; + let new_blueprint = Planner::new_based_on( + log.clone(), + &blueprint, + &example.input, + &blueprint_name, + &example.collection, + ) + .expect("can't create planner") + .with_rng(PlannerRng::from_seed((TEST_NAME, &blueprint_name))) + .plan() + .unwrap_or_else(|_| panic!("can't plan to include Cockroach nodes")); + + let summary = new_blueprint.diff_since_blueprint(&blueprint); + assert_eq!(summary.total_zones_added(), COCKROACHDB_REDUNDANCY); + assert_eq!(summary.total_zones_removed(), 0); + assert_eq!(summary.total_zones_modified(), 0); + blueprint = new_blueprint; + update_collection_from_blueprint(&mut example, &blueprint); + + // We should have started with no specified TUF repo and nothing to do. + assert_planning_makes_no_changes( + &logctx.log, + &blueprint, + &example.input, + &example.collection, + TEST_NAME, + ); + + // All zones should be sourced from the install dataset by default. + assert!( + blueprint + .all_omicron_zones(BlueprintZoneDisposition::is_in_service) + .all(|(_, z)| matches!( + z.image_source, + BlueprintZoneImageSource::InstallDataset + )) + ); + + // This test "starts" here -- we specify a new TUF repo with an updated + // CockroachDB image. We create a new TUF repo where version of + // CockroachDB has been updated out of the install dataset. + // + // The planner should avoid doing this update until it has confirmation + // from inventory that the cluster is healthy. + + let mut input_builder = example.input.clone().into_builder(); + let version = ArtifactVersion::new_static("1.0.0-freeform") + .expect("can't parse artifact version"); + let fake_hash = ArtifactHash([0; 32]); + let image_source = BlueprintZoneImageSource::Artifact { + version: BlueprintZoneImageVersion::Available { + version: version.clone(), + }, + hash: fake_hash, + }; + let artifacts = vec![fake_zone_artifact!(CockroachDb, version.clone())]; + let target_release_generation = Generation::from_u32(2); + input_builder.policy_mut().tuf_repo = TufRepoPolicy { + target_release_generation, + description: Some(TufRepoDescription { + repo: TufRepoMeta { + hash: fake_hash, + targets_role_version: 0, + valid_until: Utc::now(), + system_version: Version::new(1, 0, 0), + file_name: String::from(""), + }, + artifacts, + }), + }; + example.input = input_builder.build(); + + // Some helper predicates for the assertions below. + let is_old_cockroach = |zone: &BlueprintZoneConfig| -> bool { + zone.zone_type.is_cockroach() + && matches!( + zone.image_source, + BlueprintZoneImageSource::InstallDataset + ) + }; + let is_up_to_date_cockroach = |zone: &BlueprintZoneConfig| -> bool { + zone.zone_type.is_cockroach() && zone.image_source == image_source + }; + + // If we have missing info in our inventory, the + // planner will not update any Cockroach zones. + example.collection.cockroach_status.ranges_underreplicated = None; + example.collection.cockroach_status.liveness_live_nodes = None; + assert_planning_makes_no_changes( + &log, + &blueprint, + &example.input, + &example.collection, + TEST_NAME, + ); + + let goal_redundancy = COCKROACHDB_REDUNDANCY as u64; + + // If we have any non-zero "ranges_underreplicated" in in our inventory, + // the planner will not update any Cockroach zones. + example.collection.cockroach_status.ranges_underreplicated = Some(1); + example.collection.cockroach_status.liveness_live_nodes = + Some(goal_redundancy); + assert_planning_makes_no_changes( + &log, + &blueprint, + &example.input, + &example.collection, + TEST_NAME, + ); + + // If we don't have enough live nodes, we won't update Cockroach zones. + example.collection.cockroach_status.ranges_underreplicated = Some(0); + example.collection.cockroach_status.liveness_live_nodes = + Some(goal_redundancy - 1); + assert_planning_makes_no_changes( + &log, + &blueprint, + &example.input, + &example.collection, + TEST_NAME, + ); + + // Once we have zero underreplicated ranges, we can start to update + // Cockroach zones. + // + // We'll update one zone at a time, from the install dataset to the + // new TUF repo artifact. + for i in 1..=COCKROACHDB_REDUNDANCY { + // Keep setting this value in a loop; + // "update_collection_from_blueprint" resets it to "None". + example.collection.cockroach_status.ranges_underreplicated = + Some(0); + example.collection.cockroach_status.liveness_live_nodes = + Some(goal_redundancy); + + println!("Updating cockroach {i} of {COCKROACHDB_REDUNDANCY}"); + let new_blueprint = Planner::new_based_on( + log.clone(), + &blueprint, + &example.input, + &format!("test_blueprint_cockroach_{i}"), + &example.collection, + ) + .expect("can't create planner") + .with_rng(PlannerRng::from_seed((TEST_NAME, "bp_crdb"))) + .plan() + .expect("plan for trivial TUF repo"); + + blueprint = new_blueprint; + + assert_eq!( + blueprint + .all_omicron_zones(BlueprintZoneDisposition::is_in_service) + .filter(|(_, z)| is_old_cockroach(z)) + .count(), + COCKROACHDB_REDUNDANCY - i + ); + assert_eq!( + blueprint + .all_omicron_zones(BlueprintZoneDisposition::is_in_service) + .filter(|(_, z)| is_up_to_date_cockroach(z)) + .count(), + i + ); + update_collection_from_blueprint(&mut example, &blueprint); + } + + // Validate that we have no further changes to make, once all Cockroach + // zones have been updated. + + example.collection.cockroach_status.ranges_underreplicated = Some(0); + assert_planning_makes_no_changes( + &log, + &blueprint, + &example.input, + &example.collection, + TEST_NAME, + ); + + // Validate that we do not flip back to the install dataset after + // performing the update. + + example.collection.cockroach_status.ranges_underreplicated = Some(1); + assert_planning_makes_no_changes( + &log, + &blueprint, + &example.input, + &example.collection, + TEST_NAME, + ); + + logctx.cleanup_successful(); + } + /// Ensure that planning to update all zones terminates. #[test] fn test_update_all_zones() {