From 5797b8b520c0c58db1981aa77589fc80ac2a1aed Mon Sep 17 00:00:00 2001 From: "Andrew J. Stone" Date: Wed, 8 Jan 2025 14:21:58 -0500 Subject: [PATCH] Put `BlueprintZoneConfig` into a map (#7315) This is a largely mechanical change to store zones in a Blueprint mapped by their zone uuid rather than in a Vec. This is primarily to support diffs of `BlueprintZonesConfig` via diffus. Diffus doesn't handle Vecs in a manner where inserts, removals, and modifications are tracked based on an id. To minimize the PR diff, the zone id was not removed from `BlueprintZoneConfig`. The key to the map must always match the value in `BlueprintZoneConfig`. It's a shame a mismatch is now representable, and we can go about trying to change that if necessary. We can also just make it part of blippy for now. FWIW, this matches the pattern in `BlueprintDatasetsConfig`. Putting the `BlueprintZoneConfig` into a map means we no longer require the sort method on `BlueprintZonesConfig` as the order is stable now. However, we still want to ensure the same display sort order as previously, so we make sure to sort things properly before creating the table. We'll want similar changes for `BlueprintPhysicalDisksConfig`. That can come in a follow up PR. --- .../db-queries/src/db/datastore/deployment.rs | 20 +++--- nexus/db-queries/src/db/datastore/rack.rs | 54 +++++++++------- .../src/db/datastore/support_bundle.rs | 4 +- nexus/db-queries/src/db/datastore/vpc.rs | 28 ++++++-- nexus/reconfigurator/blippy/src/checks.rs | 22 +++---- .../execution/src/clickhouse.rs | 18 ++++-- nexus/reconfigurator/execution/src/dns.rs | 11 ++-- .../execution/src/omicron_zones.rs | 38 ++++++----- .../planning/src/blueprint_builder/builder.rs | 10 +-- .../src/blueprint_builder/internal_dns.rs | 2 +- .../src/blueprint_editor/sled_editor.rs | 4 +- .../src/blueprint_editor/sled_editor/zones.rs | 33 ++-------- nexus/reconfigurator/planning/src/example.rs | 2 +- nexus/reconfigurator/planning/src/planner.rs | 26 ++++---- .../background/tasks/blueprint_execution.rs | 10 ++- .../tasks/crdb_node_id_collector.rs | 64 ++++++++++++------- nexus/test-utils/src/lib.rs | 6 +- nexus/types/src/deployment.rs | 27 ++++---- nexus/types/src/deployment/blueprint_diff.rs | 12 ++-- openapi/nexus-internal.json | 6 +- sled-agent/src/rack_setup/service.rs | 7 +- 21 files changed, 225 insertions(+), 179 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 4210f2bee2..573ba70780 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -266,7 +266,7 @@ impl DataStore { .blueprint_zones .iter() .flat_map(|(sled_id, zones_config)| { - zones_config.zones.iter().map(move |zone| { + zones_config.zones.values().map(move |zone| { BpOmicronZone::new(blueprint_id, *sled_id, zone) .map_err(|e| Error::internal_error(&format!("{:#}", e))) }) @@ -276,7 +276,7 @@ impl DataStore { .blueprint_zones .values() .flat_map(|zones_config| { - zones_config.zones.iter().filter_map(|zone| { + zones_config.zones.values().filter_map(|zone| { BpOmicronZoneNic::new(blueprint_id, zone) .with_context(|| format!("zone {}", zone.id)) .map_err(|e| Error::internal_error(&format!("{:#}", e))) @@ -587,7 +587,7 @@ impl DataStore { s.sled_id.into(), BlueprintZonesConfig { generation: *s.generation, - zones: Vec::new(), + zones: BTreeMap::new(), }, ); bail_unless!( @@ -794,16 +794,11 @@ impl DataStore { e.to_string() )) })?; - sled_zones.zones.push(zone); + sled_zones.zones.insert(zone.id, zone); } } } - // Sort all zones to match what blueprint builders do. - for (_, zones_config) in blueprint_zones.iter_mut() { - zones_config.sort(); - } - bail_unless!( omicron_zone_nics.is_empty(), "found extra Omicron zone NICs: {:?}", @@ -2807,7 +2802,7 @@ mod tests { sled_id, BlueprintZonesConfig { generation: omicron_common::api::external::Generation::new(), - zones: vec![BlueprintZoneConfig { + zones: [BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, id: zone_id, filesystem_pool: None, @@ -2843,7 +2838,10 @@ mod tests { external_dns_servers: vec![], }, ), - }], + }] + .into_iter() + .map(|z| (z.id, z)) + .collect(), }, ); diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 067b8ed3ce..fb59164a50 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -1373,7 +1373,7 @@ mod test { SledUuid::from_untyped_uuid(sled1.id()), BlueprintZonesConfig { generation: Generation::new().next(), - zones: vec![ + zones: [ BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, id: external_dns_id, @@ -1437,14 +1437,17 @@ mod test { }, ), }, - ], + ] + .into_iter() + .map(|z| (z.id, z)) + .collect(), }, ); blueprint_zones.insert( SledUuid::from_untyped_uuid(sled2.id()), BlueprintZonesConfig { generation: Generation::new().next(), - zones: vec![ + zones: [ BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, id: nexus_id, @@ -1511,14 +1514,17 @@ mod test { }, ), }, - ], + ] + .into_iter() + .map(|z| (z.id, z)) + .collect(), }, ); blueprint_zones.insert( SledUuid::from_untyped_uuid(sled3.id()), BlueprintZonesConfig { generation: Generation::new().next(), - zones: vec![BlueprintZoneConfig { + zones: [BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, id: ntp3_id, filesystem_pool: Some(random_zpool()), @@ -1527,12 +1533,12 @@ mod test { address: "[::1]:80".parse().unwrap(), }, ), - }], + }] + .into_iter() + .map(|z| (z.id, z)) + .collect(), }, ); - for zone_config in blueprint_zones.values_mut() { - zone_config.sort(); - } let blueprint = Blueprint { id: Uuid::new_v4(), sled_state: sled_states_active(blueprint_zones.keys().copied()), @@ -1698,7 +1704,7 @@ mod test { SledUuid::from_untyped_uuid(sled.id()), BlueprintZonesConfig { generation: Generation::new().next(), - zones: vec![ + zones: [ BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, id: nexus_id1, @@ -1763,7 +1769,10 @@ mod test { }, ), }, - ], + ] + .into_iter() + .map(|z| (z.id, z)) + .collect(), }, ); @@ -1791,9 +1800,6 @@ mod test { HashMap::from([("api.sys".to_string(), external_records.clone())]), ); - for zone_config in blueprint_zones.values_mut() { - zone_config.sort(); - } let blueprint = Blueprint { id: Uuid::new_v4(), sled_state: sled_states_active(blueprint_zones.keys().copied()), @@ -1971,7 +1977,7 @@ mod test { SledUuid::from_untyped_uuid(sled.id()), BlueprintZonesConfig { generation: Generation::new().next(), - zones: vec![BlueprintZoneConfig { + zones: [BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, id: nexus_id, filesystem_pool: Some(random_zpool()), @@ -2000,12 +2006,12 @@ mod test { }, }, ), - }], + }] + .into_iter() + .map(|z| (z.id, z)) + .collect(), }, ); - for zone_config in blueprint_zones.values_mut() { - zone_config.sort(); - } let blueprint = Blueprint { id: Uuid::new_v4(), sled_state: sled_states_active(blueprint_zones.keys().copied()), @@ -2080,7 +2086,7 @@ mod test { SledUuid::from_untyped_uuid(sled.id()), BlueprintZonesConfig { generation: Generation::new().next(), - zones: vec![ + zones: [ BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, id: external_dns_id, @@ -2142,13 +2148,13 @@ mod test { }, ), }, - ], + ] + .into_iter() + .map(|z| (z.id, z)) + .collect(), }, ); - for zone_config in blueprint_zones.values_mut() { - zone_config.sort(); - } let blueprint = Blueprint { id: Uuid::new_v4(), sled_state: sled_states_active(blueprint_zones.keys().copied()), diff --git a/nexus/db-queries/src/db/datastore/support_bundle.rs b/nexus/db-queries/src/db/datastore/support_bundle.rs index d616b4356b..52f120d49a 100644 --- a/nexus/db-queries/src/db/datastore/support_bundle.rs +++ b/nexus/db-queries/src/db/datastore/support_bundle.rs @@ -913,7 +913,7 @@ mod test { .values() .flat_map(|zones_config| { let mut nexus_zones = vec![]; - for zone in &zones_config.zones { + for (_, zone) in &zones_config.zones { if matches!(zone.zone_type, BlueprintZoneType::Nexus(_)) && zone.disposition.matches(filter) { @@ -957,7 +957,7 @@ mod test { fn expunge_nexus_for_bundle(bp: &mut Blueprint, bundle: &SupportBundle) { for zones in bp.blueprint_zones.values_mut() { - for zone in &mut zones.zones { + for (_, zone) in &mut zones.zones { if zone.id == bundle.assigned_nexus.unwrap().into() { zone.disposition = BlueprintZoneDisposition::Expunged; } diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 1890fd44ad..4691bede83 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -3145,7 +3145,13 @@ mod tests { let bp1_nic = datastore .service_create_network_interface_raw( &opctx, - db_nic_from_zone(&bp1.blueprint_zones[&sled_ids[2]].zones[0]), + db_nic_from_zone( + bp1.blueprint_zones[&sled_ids[2]] + .zones + .first_key_value() + .unwrap() + .1, + ), ) .await .expect("failed to insert service VNIC"); @@ -3176,7 +3182,11 @@ mod tests { datastore .service_delete_network_interface( &opctx, - bp1.blueprint_zones[&sled_ids[2]].zones[0] + bp1.blueprint_zones[&sled_ids[2]] + .zones + .first_key_value() + .unwrap() + .1 .id .into_untyped_uuid(), bp1_nic.id(), @@ -3207,7 +3217,13 @@ mod tests { datastore .service_create_network_interface_raw( &opctx, - db_nic_from_zone(&bp3.blueprint_zones[&sled_id].zones[0]), + db_nic_from_zone( + bp3.blueprint_zones[&sled_id] + .zones + .first_key_value() + .unwrap() + .1, + ), ) .await .expect("failed to insert service VNIC"); @@ -3261,7 +3277,8 @@ mod tests { .blueprint_zones .get_mut(&sled_ids[2]) .expect("zones for sled"); - sled2.zones[0].disposition = BlueprintZoneDisposition::Quiesced; + sled2.zones.values_mut().next().unwrap().disposition = + BlueprintZoneDisposition::Quiesced; sled2.generation = sled2.generation.next(); // Sled index 3's zone is expunged (should be excluded). @@ -3269,7 +3286,8 @@ mod tests { .blueprint_zones .get_mut(&sled_ids[3]) .expect("zones for sled"); - sled3.zones[0].disposition = BlueprintZoneDisposition::Expunged; + sled3.zones.values_mut().next().unwrap().disposition = + BlueprintZoneDisposition::Expunged; sled3.generation = sled3.generation.next(); bp4 diff --git a/nexus/reconfigurator/blippy/src/checks.rs b/nexus/reconfigurator/blippy/src/checks.rs index faf456c73a..f67c9082b8 100644 --- a/nexus/reconfigurator/blippy/src/checks.rs +++ b/nexus/reconfigurator/blippy/src/checks.rs @@ -640,7 +640,7 @@ mod tests { // Copy the underlay IP from one Nexus to another. let mut nexus_iter = blueprint.blueprint_zones.iter_mut().flat_map( |(sled_id, zones_config)| { - zones_config.zones.iter_mut().filter_map(move |zone| { + zones_config.zones.values_mut().filter_map(move |zone| { if zone.zone_type.is_nexus() { Some((*sled_id, zone)) } else { @@ -678,7 +678,7 @@ mod tests { .get(&nexus1_sled_id) .unwrap() .zones - .iter(); + .values(); let sled1_zone1 = sled1_zones.next().expect("at least one zone"); let sled1_zone2 = sled1_zones.next().expect("at least two zones"); if sled1_zone1.id == nexus1.id { @@ -745,7 +745,7 @@ mod tests { .blueprint_zones .iter_mut() .flat_map(|(sled_id, zones_config)| { - zones_config.zones.iter_mut().filter_map(move |zone| { + zones_config.zones.values_mut().filter_map(move |zone| { if zone.zone_type.is_internal_dns() { Some((*sled_id, zone)) } else { @@ -826,7 +826,7 @@ mod tests { // Copy the external IP from one Nexus to another. let mut nexus_iter = blueprint.blueprint_zones.iter_mut().flat_map( |(sled_id, zones_config)| { - zones_config.zones.iter_mut().filter_map(move |zone| { + zones_config.zones.values_mut().filter_map(move |zone| { if zone.zone_type.is_nexus() { Some((*sled_id, zone)) } else { @@ -896,7 +896,7 @@ mod tests { // Copy the external IP from one Nexus to another. let mut nexus_iter = blueprint.blueprint_zones.iter_mut().flat_map( |(sled_id, zones_config)| { - zones_config.zones.iter_mut().filter_map(move |zone| { + zones_config.zones.values_mut().filter_map(move |zone| { if zone.zone_type.is_nexus() { Some((*sled_id, zone)) } else { @@ -961,7 +961,7 @@ mod tests { // Copy the external IP from one Nexus to another. let mut nexus_iter = blueprint.blueprint_zones.iter_mut().flat_map( |(sled_id, zones_config)| { - zones_config.zones.iter_mut().filter_map(move |zone| { + zones_config.zones.values_mut().filter_map(move |zone| { if zone.zone_type.is_nexus() { Some((*sled_id, zone)) } else { @@ -1030,7 +1030,7 @@ mod tests { // Copy the durable zpool from one external DNS to another. let mut dns_iter = blueprint.blueprint_zones.iter_mut().flat_map( |(sled_id, zones_config)| { - zones_config.zones.iter_mut().filter_map(move |zone| { + zones_config.zones.values_mut().filter_map(move |zone| { if zone.zone_type.is_external_dns() { Some((*sled_id, zone)) } else { @@ -1110,7 +1110,7 @@ mod tests { // Copy the filesystem zpool from one external DNS to another. let mut dns_iter = blueprint.blueprint_zones.iter_mut().flat_map( |(sled_id, zones_config)| { - zones_config.zones.iter_mut().filter_map(move |zone| { + zones_config.zones.values_mut().filter_map(move |zone| { if zone.zone_type.is_external_dns() { Some((*sled_id, zone)) } else { @@ -1335,7 +1335,7 @@ mod tests { // with a filesystem_pool dataset to remove. let mut durable_zone = None; let mut root_zone = None; - for z in &zones_config.zones { + for (_, z) in &zones_config.zones { if durable_zone.is_none() { if z.zone_type.durable_zpool().is_some() { durable_zone = Some(z.clone()); @@ -1501,7 +1501,7 @@ mod tests { .expect("got zones for sled with datasets"); let mut durable_zone = None; let mut root_zone = None; - for z in &zones_config.zones { + for (_, z) in &zones_config.zones { if durable_zone.is_none() { if z.zone_type.durable_zpool().is_some() { durable_zone = Some(z.clone()); @@ -1517,7 +1517,7 @@ mod tests { root_zone.expect("found zone with root dataset to prune"); zones_config .zones - .retain(|z| z.id != durable_zone.id && z.id != root_zone.id); + .retain(|_, z| z.id != durable_zone.id && z.id != root_zone.id); let durable_dataset = durable_zone.zone_type.durable_dataset().unwrap(); let root_dataset = root_zone.filesystem_dataset().unwrap(); diff --git a/nexus/reconfigurator/execution/src/clickhouse.rs b/nexus/reconfigurator/execution/src/clickhouse.rs index 36e41aec1e..ffe6bd2b7e 100644 --- a/nexus/reconfigurator/execution/src/clickhouse.rs +++ b/nexus/reconfigurator/execution/src/clickhouse.rs @@ -209,7 +209,7 @@ fn server_configs( .flat_map(|zones_config| { zones_config .zones - .iter() + .values() .filter(|zone_config| { clickhouse_cluster_config .servers @@ -267,7 +267,7 @@ fn keeper_configs( .flat_map(|zones_config| { zones_config .zones - .iter() + .values() .filter(|zone_config| { clickhouse_cluster_config .keepers @@ -351,7 +351,7 @@ mod test { let zone_id = OmicronZoneUuid::new_v4(); let zone_config = BlueprintZonesConfig { generation: Generation::new(), - zones: vec![BlueprintZoneConfig { + zones: [BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, id: zone_id, filesystem_pool: None, @@ -379,7 +379,10 @@ mod test { }, }, ), - }], + }] + .into_iter() + .map(|z| (z.id, z)) + .collect(), }; zones.insert(sled_id, zone_config); config.keepers.insert(zone_id, keeper_id.into()); @@ -390,7 +393,7 @@ mod test { let zone_id = OmicronZoneUuid::new_v4(); let zone_config = BlueprintZonesConfig { generation: Generation::new(), - zones: vec![BlueprintZoneConfig { + zones: [BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, id: zone_id, filesystem_pool: None, @@ -418,7 +421,10 @@ mod test { }, }, ), - }], + }] + .into_iter() + .map(|z| (z.id, z)) + .collect(), }; zones.insert(sled_id, zone_config); config.servers.insert(zone_id, server_id.into()); diff --git a/nexus/reconfigurator/execution/src/dns.rs b/nexus/reconfigurator/execution/src/dns.rs index 85d1076749..56e9348f21 100644 --- a/nexus/reconfigurator/execution/src/dns.rs +++ b/nexus/reconfigurator/execution/src/dns.rs @@ -637,8 +637,8 @@ mod test { zones: sa.omicron_zones .zones .into_iter() - .map(|config| -> BlueprintZoneConfig { - deprecated_omicron_zone_config_to_blueprint_zone_config( + .map(|config| -> (OmicronZoneUuid, BlueprintZoneConfig) { + (config.id, deprecated_omicron_zone_config_to_blueprint_zone_config( config, BlueprintZoneDisposition::InService, // We don't get external IP IDs in inventory @@ -646,7 +646,7 @@ mod test { // zone that needs one here. This is gross. Some(ExternalIpUuid::new_v4()), ) - .expect("failed to convert zone config") + .expect("failed to convert zone config")) }) .collect(), }, @@ -678,7 +678,8 @@ mod test { // not currently in service. let out_of_service_id = OmicronZoneUuid::new_v4(); let out_of_service_addr = Ipv6Addr::LOCALHOST; - blueprint.blueprint_zones.values_mut().next().unwrap().zones.push( + blueprint.blueprint_zones.values_mut().next().unwrap().zones.insert( + out_of_service_id, BlueprintZoneConfig { disposition: BlueprintZoneDisposition::Quiesced, id: out_of_service_id, @@ -1031,7 +1032,7 @@ mod test { blueprint.blueprint_zones.iter_mut().next().unwrap(); let nexus_zone = bp_zones_config .zones - .iter_mut() + .values_mut() .find(|z| z.zone_type.is_nexus()) .unwrap(); nexus_zone.disposition = BlueprintZoneDisposition::Quiesced; diff --git a/nexus/reconfigurator/execution/src/omicron_zones.rs b/nexus/reconfigurator/execution/src/omicron_zones.rs index b7587377c9..98d8bed358 100644 --- a/nexus/reconfigurator/execution/src/omicron_zones.rs +++ b/nexus/reconfigurator/execution/src/omicron_zones.rs @@ -438,11 +438,12 @@ mod test { // See `rack_setup::service::ServiceInner::run` for more details. fn make_zones() -> BlueprintZonesConfig { let zpool = ZpoolName::new_external(ZpoolUuid::new_v4()); + let zone_id = OmicronZoneUuid::new_v4(); BlueprintZonesConfig { generation: Generation::new(), - zones: vec![BlueprintZoneConfig { + zones: [BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, - id: OmicronZoneUuid::new_v4(), + id: zone_id, filesystem_pool: Some(zpool.clone()), zone_type: BlueprintZoneType::InternalDns( blueprint_zone_type::InternalDns { @@ -453,7 +454,10 @@ mod test { http_address: "[::1]:0".parse().unwrap(), }, ), - }], + }] + .into_iter() + .map(|z| (z.id, z)) + .collect(), } } @@ -542,18 +546,22 @@ mod test { zones: &mut BlueprintZonesConfig, disposition: BlueprintZoneDisposition, ) { - zones.zones.push(BlueprintZoneConfig { - disposition, - id: OmicronZoneUuid::new_v4(), - filesystem_pool: Some(ZpoolName::new_external( - ZpoolUuid::new_v4(), - )), - zone_type: BlueprintZoneType::InternalNtp( - blueprint_zone_type::InternalNtp { - address: "[::1]:0".parse().unwrap(), - }, - ), - }); + let zone_id = OmicronZoneUuid::new_v4(); + zones.zones.insert( + zone_id, + BlueprintZoneConfig { + disposition, + id: zone_id, + filesystem_pool: Some(ZpoolName::new_external( + ZpoolUuid::new_v4(), + )), + zone_type: BlueprintZoneType::InternalNtp( + blueprint_zone_type::InternalNtp { + address: "[::1]:0".parse().unwrap(), + }, + ), + }, + ); } // Both in-service and quiesced zones should be deployed. diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 09a6de84ef..e9abf2e2d4 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -413,7 +413,7 @@ impl<'a> BlueprintBuilder<'a> { .map(|sled_id| { let config = BlueprintZonesConfig { generation: Generation::new(), - zones: Vec::new(), + zones: BTreeMap::new(), }; (sled_id, config) }) @@ -2142,7 +2142,7 @@ pub mod test { // We're going under the hood of the blueprint here; a sled can only get // to the decommissioned state if all its disks/datasets/zones have been // expunged, so do that too. - for zone in &mut blueprint1 + for (_, zone) in &mut blueprint1 .blueprint_zones .get_mut(&decommision_sled_id) .expect("has zones") @@ -2189,7 +2189,7 @@ pub mod test { builder.sleds_mut().get_mut(&decommision_sled_id).unwrap().state = SledState::Decommissioned; let input = builder.build(); - for z in &mut blueprint2 + for (_, z) in &mut blueprint2 .blueprint_zones .get_mut(&decommision_sled_id) .unwrap() @@ -2328,7 +2328,7 @@ pub mod test { let (_, _, blueprint) = example(&logctx.log, TEST_NAME); for (_, zone_config) in &blueprint.blueprint_zones { - for zone in &zone_config.zones { + for (_, zone) in &zone_config.zones { // The pool should only be optional for backwards compatibility. let filesystem_pool = zone .filesystem_pool @@ -2553,7 +2553,7 @@ pub mod test { .get_mut(sled_id) .expect("missing sled") .zones - .retain(|z| match &z.zone_type { + .retain(|_, z| match &z.zone_type { BlueprintZoneType::Nexus(z) => { removed_nexus = Some(z.clone()); false diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/internal_dns.rs b/nexus/reconfigurator/planning/src/blueprint_builder/internal_dns.rs index 4222481149..bcb6963a5a 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/internal_dns.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/internal_dns.rs @@ -124,7 +124,7 @@ pub mod test { // `ExampleSystem` adds an internal DNS server to every sled. Manually // prune out all but the first of them to give us space to add more. for (_, zone_config) in blueprint1.blueprint_zones.iter_mut().skip(1) { - zone_config.zones.retain(|z| !z.zone_type.is_internal_dns()); + zone_config.zones.retain(|_, z| !z.zone_type.is_internal_dns()); } let npruned = blueprint1.blueprint_zones.len() - 1; assert!(npruned > 0); diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/sled_editor.rs b/nexus/reconfigurator/planning/src/blueprint_editor/sled_editor.rs index 3355390dc9..c1e67af1b6 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/sled_editor.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/sled_editor.rs @@ -236,7 +236,7 @@ impl SledEditor { edited .zones .zones - .iter() + .values() .filter(move |zone| zone.disposition.matches(filter)), ), } @@ -319,7 +319,7 @@ impl ActiveSledEditor { preexisting_dataset_ids: DatasetIdsBackfillFromDb, ) -> Result { Ok(Self { - zones: zones.try_into()?, + zones: zones.into(), disks: disks.try_into()?, datasets: DatasetsEditor::new(datasets, preexisting_dataset_ids)?, }) diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/zones.rs b/nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/zones.rs index 5a5c7a1807..47dd8e6b0f 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/zones.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/sled_editor/zones.rs @@ -56,11 +56,7 @@ impl ZonesEditor { if self.counts.has_nonzero_counts() { generation = generation.next(); } - let mut config = BlueprintZonesConfig { - generation, - zones: self.zones.into_values().collect(), - }; - config.sort(); + let config = BlueprintZonesConfig { generation, zones: self.zones }; (config, self.counts) } @@ -153,29 +149,12 @@ impl ZonesEditor { } } -impl TryFrom for ZonesEditor { - type Error = DuplicateZoneId; - - fn try_from(config: BlueprintZonesConfig) -> Result { - let mut zones = BTreeMap::new(); - for zone in config.zones { - match zones.entry(zone.id) { - Entry::Vacant(slot) => { - slot.insert(zone); - } - Entry::Occupied(prev) => { - return Err(DuplicateZoneId { - id: zone.id, - kind1: zone.zone_type.kind(), - kind2: prev.get().zone_type.kind(), - }); - } - } - } - Ok(Self { +impl From for ZonesEditor { + fn from(config: BlueprintZonesConfig) -> Self { + Self { generation: config.generation, - zones, + zones: config.zones, counts: EditCounts::zeroes(), - }) + } } } diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index dfba3f9992..cdbef7733b 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -461,7 +461,7 @@ impl ExampleSystemBuilder { let Some(zones) = blueprint.blueprint_zones.get(&sled_id) else { continue; }; - for zone in zones.zones.iter() { + for (_, zone) in zones.zones.iter() { let service_id = zone.id; if let Some((external_ip, nic)) = zone.zone_type.external_networking() diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index ce1a1ae960..fb98563a65 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -1050,7 +1050,7 @@ mod test { .get(&sled_id) .expect("missing kept sled") .zones - .iter() + .values() .filter(|z| z.zone_type.is_nexus()) .count(), 1 @@ -1133,7 +1133,7 @@ mod test { assert_eq!( sled_config .zones - .iter() + .values() .filter(|z| z.zone_type.is_nexus()) .count(), 1 @@ -1216,7 +1216,7 @@ mod test { assert_eq!( sled_config .zones - .iter() + .values() .filter(|z| z.zone_type.is_internal_dns()) .count(), 1 @@ -1251,7 +1251,7 @@ mod test { // Remove two of the internal DNS zones; the planner should put new // zones back in their places. for (_sled_id, zones) in blueprint1.blueprint_zones.iter_mut().take(2) { - zones.zones.retain(|z| !z.zone_type.is_internal_dns()); + zones.zones.retain(|_, z| !z.zone_type.is_internal_dns()); } for (_, dataset_config) in blueprint1.blueprint_datasets.iter_mut().take(2) @@ -1365,7 +1365,7 @@ mod test { // The expunged sled should have an expunged Nexus zone. let zone = blueprint2.blueprint_zones[&sled_id] .zones - .iter() + .values() .find(|zone| matches!(zone.zone_type, BlueprintZoneType::Nexus(_))) .expect("no nexus zone found"); assert_eq!(zone.disposition, BlueprintZoneDisposition::Expunged); @@ -1401,7 +1401,7 @@ mod test { let new_zone = blueprint3 .blueprint_zones .values() - .flat_map(|c| &c.zones) + .flat_map(|c| c.zones.values()) .find(|zone| { zone.disposition == BlueprintZoneDisposition::InService && zone @@ -1560,7 +1560,7 @@ mod test { assert_eq!( blueprint3.blueprint_zones[&sled_id] .zones - .iter() + .values() .filter(|zone| { zone.disposition == BlueprintZoneDisposition::Expunged && zone.zone_type.is_external_dns() @@ -1797,7 +1797,7 @@ mod test { // multiple zones of distinct types. let mut zpool_by_zone_usage = HashMap::new(); for zones in blueprint1.blueprint_zones.values() { - for zone in &zones.zones { + for (_, zone) in &zones.zones { let pool = zone.filesystem_pool.as_ref().unwrap(); zpool_by_zone_usage .entry(pool.id()) @@ -1930,7 +1930,7 @@ mod test { .blueprint_zones .iter() .find_map(|(_, zones_config)| { - for zone_config in &zones_config.zones { + for (_, zone_config) in &zones_config.zones { if zone_config.zone_type.is_ntp() { return zone_config.filesystem_pool.clone(); } @@ -1945,7 +1945,7 @@ mod test { 0, |acc, (_, zones_config)| { let mut zones_using_zpool = 0; - for zone_config in &zones_config.zones { + for (_, zone_config) in &zones_config.zones { if let Some(pool) = &zone_config.filesystem_pool { if pool == &pool_to_expunge { zones_using_zpool += 1; @@ -2056,7 +2056,7 @@ mod test { assert_eq!( sled_config .zones - .iter() + .values() .filter(|z| z.zone_type.is_nexus()) .count(), 1 @@ -2090,7 +2090,7 @@ mod test { // 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 + for (_, zone) in &mut blueprint1.blueprint_zones.get_mut(sled_id).unwrap().zones { zone.disposition = BlueprintZoneDisposition::Expunged; @@ -2222,7 +2222,7 @@ mod test { .unwrap() .zones; - zones.retain_mut(|zone| { + zones.retain(|_, zone| { if let BlueprintZoneType::Nexus(blueprint_zone_type::Nexus { internal_address, .. diff --git a/nexus/src/app/background/tasks/blueprint_execution.rs b/nexus/src/app/background/tasks/blueprint_execution.rs index bdae1b4f20..db7192d6bf 100644 --- a/nexus/src/app/background/tasks/blueprint_execution.rs +++ b/nexus/src/app/background/tasks/blueprint_execution.rs @@ -402,11 +402,12 @@ mod test { disposition: BlueprintZoneDisposition, ) -> BlueprintZonesConfig { let pool_id = ZpoolUuid::new_v4(); + let zone_id = OmicronZoneUuid::new_v4(); BlueprintZonesConfig { generation: Generation::new(), - zones: vec![BlueprintZoneConfig { + zones: [BlueprintZoneConfig { disposition, - id: OmicronZoneUuid::new_v4(), + id: zone_id, filesystem_pool: Some(ZpoolName::new_external(pool_id)), zone_type: BlueprintZoneType::InternalDns( blueprint_zone_type::InternalDns { @@ -421,7 +422,10 @@ mod test { http_address: "[::1]:12345".parse().unwrap(), }, ), - }], + }] + .into_iter() + .map(|z| (z.id, z)) + .collect(), } } diff --git a/nexus/src/app/background/tasks/crdb_node_id_collector.rs b/nexus/src/app/background/tasks/crdb_node_id_collector.rs index 1cac4e8c5a..c421e7116a 100644 --- a/nexus/src/app/background/tasks/crdb_node_id_collector.rs +++ b/nexus/src/app/background/tasks/crdb_node_id_collector.rs @@ -293,37 +293,52 @@ mod tests { let crdb_addr1: SocketAddrV6 = "[2001:db8::1]:1111".parse().unwrap(); let crdb_addr2: SocketAddrV6 = "[2001:db8::2]:1234".parse().unwrap(); let crdb_addr3: SocketAddrV6 = "[2001:db8::3]:1234".parse().unwrap(); - bp_zones.zones.push(make_crdb_zone_config( - BlueprintZoneDisposition::InService, + bp_zones.zones.insert( crdb_id1, - crdb_addr1, - )); - bp_zones.zones.push(make_crdb_zone_config( - BlueprintZoneDisposition::Expunged, + make_crdb_zone_config( + BlueprintZoneDisposition::InService, + crdb_id1, + crdb_addr1, + ), + ); + bp_zones.zones.insert( crdb_id2, - crdb_addr2, - )); - bp_zones.zones.push(make_crdb_zone_config( - BlueprintZoneDisposition::InService, + make_crdb_zone_config( + BlueprintZoneDisposition::Expunged, + crdb_id2, + crdb_addr2, + ), + ); + bp_zones.zones.insert( crdb_id3, - crdb_addr3, - )); + make_crdb_zone_config( + BlueprintZoneDisposition::InService, + crdb_id3, + crdb_addr3, + ), + ); // Also add a non-CRDB zone to ensure it's filtered out. - bp_zones.zones.push(BlueprintZoneConfig { - disposition: BlueprintZoneDisposition::InService, - id: OmicronZoneUuid::new_v4(), - filesystem_pool: Some(ZpoolName::new_external(ZpoolUuid::new_v4())), - zone_type: BlueprintZoneType::CruciblePantry( - blueprint_zone_type::CruciblePantry { - address: "[::1]:0".parse().unwrap(), - }, - ), - }); + let zone_id = OmicronZoneUuid::new_v4(); + bp_zones.zones.insert( + zone_id, + BlueprintZoneConfig { + disposition: BlueprintZoneDisposition::InService, + id: zone_id, + filesystem_pool: Some(ZpoolName::new_external( + ZpoolUuid::new_v4(), + )), + zone_type: BlueprintZoneType::CruciblePantry( + blueprint_zone_type::CruciblePantry { + address: "[::1]:0".parse().unwrap(), + }, + ), + }, + ); // We expect to see CRDB zones 1 and 3 with their IPs but the ports // changed to `COCKROACH_ADMIN_PORT`. - let expected = vec![ + let mut expected = vec![ ( crdb_id1, SocketAddrV6::new(*crdb_addr1.ip(), COCKROACH_ADMIN_PORT, 0, 0), @@ -333,6 +348,9 @@ mod tests { SocketAddrV6::new(*crdb_addr3.ip(), COCKROACH_ADMIN_PORT, 0, 0), ), ]; + // We sort starting with zone id, since the original zones are sorted + // that way in a map. + expected.sort_unstable(); let admin_addrs = CockroachAdminFromBlueprintViaFixedPort .cockroach_admin_addrs(&blueprint) diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs index e78fb45ce6..4991d2eaf9 100644 --- a/nexus/test-utils/src/lib.rs +++ b/nexus/test-utils/src/lib.rs @@ -824,7 +824,11 @@ impl<'a, N: NexusServer> ControlPlaneTestContextBuilder<'a, N> { sled_id, BlueprintZonesConfig { generation: Generation::new().next(), - zones: zones.clone(), + zones: zones + .iter() + .cloned() + .map(|z| (z.id, z)) + .collect(), }, ); sled_state.insert(sled_id, SledState::Active); diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index 8915ac8746..33b8e102c5 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -238,7 +238,7 @@ impl Blueprint { ) -> impl Iterator { zones_by_sled_id.iter().flat_map(move |(sled_id, z)| { z.zones - .iter() + .values() .filter(move |z| z.disposition.matches(filter)) .map(|z| (*sled_id, z)) }) @@ -266,7 +266,7 @@ impl Blueprint { ) -> impl Iterator { self.blueprint_zones.iter().flat_map(move |(sled_id, z)| { z.zones - .iter() + .values() .filter(move |z| !z.disposition.matches(filter)) .map(|z| (*sled_id, z)) }) @@ -333,7 +333,10 @@ impl BpTableData for BlueprintZonesConfig { } fn rows(&self, state: BpDiffState) -> impl Iterator { - self.zones.iter().map(move |zone| { + // We want to sort by (kind, id) + let mut zones: Vec<_> = self.zones.values().cloned().collect(); + zones.sort_unstable_by_key(zone_sort_key); + zones.into_iter().map(move |zone| { BpTableRow::from_strings( state, vec![ @@ -552,28 +555,20 @@ pub struct BlueprintZonesConfig { /// [`OmicronZonesConfig::generation`] for more details. pub generation: Generation, - /// The list of running zones. - pub zones: Vec, + /// The set of running zones. + pub zones: BTreeMap, } impl From for OmicronZonesConfig { fn from(config: BlueprintZonesConfig) -> Self { Self { generation: config.generation, - zones: config.zones.into_iter().map(From::from).collect(), + zones: config.zones.into_values().map(From::from).collect(), } } } impl BlueprintZonesConfig { - /// Sorts the list of zones stored in this configuration. - /// - /// This is not strictly necessary. But for testing (particularly snapshot - /// testing), it's helpful for zones to be in sorted order. - pub fn sort(&mut self) { - self.zones.sort_unstable_by_key(zone_sort_key); - } - /// Converts self to an [`OmicronZonesConfig`], applying the provided /// [`BlueprintZoneFilter`]. /// @@ -587,7 +582,7 @@ impl BlueprintZonesConfig { generation: self.generation, zones: self .zones - .iter() + .values() .filter(|z| z.disposition.matches(filter)) .cloned() .map(OmicronZoneConfig::from) @@ -599,7 +594,7 @@ impl BlueprintZonesConfig { /// `Expunged`, false otherwise. pub fn are_all_zones_expunged(&self) -> bool { self.zones - .iter() + .values() .all(|c| c.disposition == BlueprintZoneDisposition::Expunged) } } diff --git a/nexus/types/src/deployment/blueprint_diff.rs b/nexus/types/src/deployment/blueprint_diff.rs index ff79c5fa61..3c5f7a120d 100644 --- a/nexus/types/src/deployment/blueprint_diff.rs +++ b/nexus/types/src/deployment/blueprint_diff.rs @@ -211,11 +211,15 @@ impl BpDiffZones { let before_by_id: BTreeMap<_, BlueprintZoneConfig> = before_zones .zones - .into_iter() + .into_values() .map(|z| (z.id(), z)) .collect(); let mut after_by_id: BTreeMap<_, BlueprintZoneConfig> = - after_zones.zones.into_iter().map(|z| (z.id, z)).collect(); + after_zones + .zones + .into_values() + .map(|z| (z.id, z)) + .collect(); for (zone_id, zone_before) in before_by_id { if let Some(zone_after) = after_by_id.remove(&zone_id) { @@ -302,7 +306,7 @@ impl BpDiffZones { } else { // No `after_zones` for this `sled_id`, so `before_zones` are removed assert!(removed.is_empty()); - for zone in before_zones.zones { + for (_, zone) in before_zones.zones { removed.push(zone); } @@ -329,7 +333,7 @@ impl BpDiffZones { BpDiffZoneDetails { generation_before: None, generation_after: Some(after_zones.generation), - zones: after_zones.zones, + zones: after_zones.zones.into_values().collect(), }, ); } diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 5c39652aa3..03350de8f8 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -2715,9 +2715,9 @@ ] }, "zones": { - "description": "The list of running zones.", - "type": "array", - "items": { + "description": "The set of running zones.", + "type": "object", + "additionalProperties": { "$ref": "#/components/schemas/BlueprintZoneConfig" } } diff --git a/sled-agent/src/rack_setup/service.rs b/sled-agent/src/rack_setup/service.rs index 37a49b930a..0c55f261bf 100644 --- a/sled-agent/src/rack_setup/service.rs +++ b/sled-agent/src/rack_setup/service.rs @@ -1604,7 +1604,12 @@ pub(crate) fn build_initial_blueprint_from_sled_configs( // value, we will need to revisit storing this in the serialized // RSS plan. generation: DeployStepVersion::V5_EVERYTHING, - zones: sled_config.zones.clone(), + zones: sled_config + .zones + .iter() + .cloned() + .map(|z| (z.id, z)) + .collect(), }; blueprint_zones.insert(*sled_id, zones_config);