From 5b0191fe44d826fc53f905be9d83f066966ea5bd Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 17 Oct 2025 11:48:45 -0400 Subject: [PATCH] BlueprintBuilder: require caller to provide internal DNS subnet This is part of #9238: we'd like to pull `BlueprintResourceAllocator`'s weird construction out of `BlueprintBuilder`, and this removes _one_ use of it. When adding an internal DNS zone, the caller must now specify the `DnsSubet` instead of `BlueprintBuilder` choosing internally. To assist with this, adds `BlueprintBuilder::available_internal_dns_subnets()`, which returns an iterator of available subnets. I'm not sure this is the right interface; will leave a comment inline with some other ideas. --- .../planning/src/blueprint_builder/builder.rs | 47 ++++- .../planning/src/blueprint_editor.rs | 1 - .../src/blueprint_editor/allocators.rs | 37 +--- .../allocators/internal_dns.rs | 195 ------------------ nexus/reconfigurator/planning/src/example.rs | 5 + nexus/reconfigurator/planning/src/planner.rs | 21 +- 6 files changed, 66 insertions(+), 240 deletions(-) delete mode 100644 nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index aa0f1ae99ab..27be01663af 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -11,7 +11,6 @@ use crate::blueprint_editor::EditedSled; use crate::blueprint_editor::ExternalNetworkingChoice; use crate::blueprint_editor::ExternalNetworkingError; use crate::blueprint_editor::ExternalSnatNetworkingChoice; -use crate::blueprint_editor::NoAvailableDnsSubnets; use crate::blueprint_editor::SledEditError; use crate::blueprint_editor::SledEditor; use crate::mgs_updates::PendingHostPhase2Changes; @@ -65,6 +64,7 @@ use nexus_types::inventory::Collection; use omicron_common::address::CLICKHOUSE_HTTP_PORT; use omicron_common::address::DNS_HTTP_PORT; use omicron_common::address::DNS_PORT; +use omicron_common::address::DnsSubnet; use omicron_common::address::NTP_PORT; use omicron_common::address::ReservedRackSubnet; use omicron_common::api::external::Generation; @@ -138,8 +138,10 @@ pub enum Error { }, #[error("error constructing resource allocator")] AllocatorInput(#[from] BlueprintResourceAllocatorInputError), - #[error("error allocating internal DNS subnet")] - AllocateInternalDnsSubnet(#[from] NoAvailableDnsSubnets), + #[error("no commissioned sleds - rack subnet is unknown")] + RackSubnetUnknownNoSleds, + #[error("no reserved subnets available for internal DNS")] + NoAvailableDnsSubnets, #[error("error allocating external networking resources")] AllocateExternalNetworking(#[from] ExternalNetworkingError), #[error("can only have {INTERNAL_DNS_REDUNDANCY} internal DNS servers")] @@ -696,6 +698,40 @@ impl<'a> BlueprintBuilder<'a> { self.new_blueprint_id } + pub fn available_internal_dns_subnets( + &self, + ) -> Result, Error> { + // TODO-multirack We need the rack subnet to know what the reserved + // internal DNS subnets are. Pick any sled; this isn't right in + // multirack (either DNS will be on a wider subnet or we need to pick a + // particular rack subnet here?). + let any_sled_subnet = self + .input + .all_sled_resources(SledFilter::Commissioned) + .map(|(_sled_id, resources)| resources.subnet) + .next() + .ok_or(Error::RackSubnetUnknownNoSleds)?; + let rack_subnet = ReservedRackSubnet::from_subnet(any_sled_subnet); + + // Compute the "in use" subnets; this includes all in-service internal + // DNS zones _and_ any "expunged but not yet confirmed to be gone" + // zones, so we use the somewhat unusual `could_be_running` filter + // instead of the more typical `is_in_service`. + let internal_dns_subnets_in_use = self + .current_zones(BlueprintZoneDisposition::could_be_running) + .filter_map(|(_sled_id, zone)| match &zone.zone_type { + BlueprintZoneType::InternalDns(internal_dns) => { + Some(DnsSubnet::from_addr(*internal_dns.dns_address.ip())) + } + _ => None, + }) + .collect::>(); + + Ok(rack_subnet.get_dns_subnets().into_iter().filter(move |subnet| { + !internal_dns_subnets_in_use.contains(&subnet) + })) + } + fn resource_allocator( &mut self, ) -> Result<&mut BlueprintResourceAllocator, Error> { @@ -1380,12 +1416,9 @@ impl<'a> BlueprintBuilder<'a> { &mut self, sled_id: SledUuid, image_source: BlueprintZoneImageSource, + dns_subnet: DnsSubnet, ) -> Result<(), Error> { let gz_address_index = self.next_internal_dns_gz_address_index(sled_id); - let sled_subnet = self.sled_resources(sled_id)?.subnet; - let rack_subnet = ReservedRackSubnet::from_subnet(sled_subnet); - let dns_subnet = - self.resource_allocator()?.next_internal_dns_subnet(rack_subnet)?; let address = dns_subnet.dns_address(); let zpool = self.sled_select_zpool(sled_id, ZoneKind::InternalDns)?; let zone_type = diff --git a/nexus/reconfigurator/planning/src/blueprint_editor.rs b/nexus/reconfigurator/planning/src/blueprint_editor.rs index 001b51ec3b1..bb1e2dbaec1 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor.rs @@ -11,7 +11,6 @@ mod sled_editor; pub use allocators::BlueprintResourceAllocatorInputError; pub use allocators::ExternalNetworkingError; -pub use allocators::NoAvailableDnsSubnets; pub use sled_editor::DatasetsEditError; pub use sled_editor::DisksEditError; pub use sled_editor::MultipleDatasetsOfKind; diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs index 7ad4a20e398..5258fa0164a 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs @@ -8,23 +8,16 @@ use std::net::IpAddr; use super::SledEditor; use nexus_types::deployment::BlueprintZoneDisposition; -use nexus_types::deployment::BlueprintZoneType; -use nexus_types::deployment::blueprint_zone_type::InternalDns; -use omicron_common::address::DnsSubnet; use omicron_common::address::IpRange; -use omicron_common::address::ReservedRackSubnet; mod external_networking; -mod internal_dns; pub use self::external_networking::ExternalNetworkingError; -pub use self::internal_dns::NoAvailableDnsSubnets; pub(crate) use self::external_networking::ExternalNetworkingChoice; pub(crate) use self::external_networking::ExternalSnatNetworkingChoice; use self::external_networking::ExternalNetworkingAllocator; -use self::internal_dns::InternalDnsSubnetAllocator; #[derive(Debug, thiserror::Error)] pub enum BlueprintResourceAllocatorInputError { @@ -35,7 +28,6 @@ pub enum BlueprintResourceAllocatorInputError { #[derive(Debug)] pub(crate) struct BlueprintResourceAllocator { external_networking: ExternalNetworkingAllocator, - internal_dns: InternalDnsSubnetAllocator, } impl BlueprintResourceAllocator { @@ -46,26 +38,6 @@ impl BlueprintResourceAllocator { where I: Iterator + Clone, { - let internal_dns_subnets_in_use = all_sleds - .clone() - .flat_map(|editor| { - editor - // We use `could_be_running` here instead of `in_service` to - // avoid reusing an internal DNS subnet from an - // expunged-but-possibly-still-running zone. - .zones(BlueprintZoneDisposition::could_be_running) - .filter_map(|z| match z.zone_type { - BlueprintZoneType::InternalDns(InternalDns { - dns_address, - .. - }) => Some(DnsSubnet::from_addr(*dns_address.ip())), - _ => None, - }) - }) - .collect(); - let internal_dns = - InternalDnsSubnetAllocator::new(internal_dns_subnets_in_use); - let external_networking = ExternalNetworkingAllocator::new( all_sleds.clone().flat_map(|editor| { editor.zones(BlueprintZoneDisposition::is_in_service) @@ -77,14 +49,7 @@ impl BlueprintResourceAllocator { ) .map_err(BlueprintResourceAllocatorInputError::ExternalNetworking)?; - Ok(Self { external_networking, internal_dns }) - } - - pub fn next_internal_dns_subnet( - &mut self, - rack_subnet: ReservedRackSubnet, - ) -> Result { - self.internal_dns.alloc(rack_subnet) + Ok(Self { external_networking }) } pub(crate) fn next_external_ip_nexus( diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs deleted file mode 100644 index 9f2787f09a4..00000000000 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs +++ /dev/null @@ -1,195 +0,0 @@ -// 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/. - -use omicron_common::address::DnsSubnet; -use omicron_common::address::ReservedRackSubnet; -use std::collections::BTreeSet; - -#[derive(Debug, thiserror::Error)] -#[error("no reserved subnets available for internal DNS")] -pub struct NoAvailableDnsSubnets; - -/// Internal DNS zones are not allocated an address in the sled's subnet. -/// Instead, they get a /64 subnet of the "reserved" rack subnet (so that -/// it's routable with IPv6), and use the first address in that. There may -/// be at most `INTERNAL_DNS_REDUNDANCY` subnets (and so servers) -/// allocated. This structure tracks which subnets are currently allocated. -#[derive(Debug)] -pub struct InternalDnsSubnetAllocator { - in_use: BTreeSet, -} - -impl InternalDnsSubnetAllocator { - pub fn new(in_use: BTreeSet) -> Self { - Self { in_use } - } - - /// Allocate the first available DNS subnet, or call a function to generate - /// a default. The default is needed because we can't necessarily guess the - /// correct reserved rack subnet (e.g., there might not be any internal DNS - /// zones in the parent blueprint, though that would itself be odd), but we - /// can derive it at runtime from the sled address. - pub fn alloc( - &mut self, - rack_subnet: ReservedRackSubnet, - ) -> Result { - let new = if let Some(first) = self.in_use.first() { - // Take the first available DNS subnet. We currently generate - // all `INTERNAL_DNS_REDUNDANCY` subnets and subtract any - // that are in use; this is fine as long as that constant is small. - let subnets = BTreeSet::from_iter( - ReservedRackSubnet::from_subnet(first.subnet()) - .get_dns_subnets(), - ); - let mut avail = subnets.difference(&self.in_use); - if let Some(first) = avail.next() { - *first - } else { - return Err(NoAvailableDnsSubnets); - } - } else { - rack_subnet.get_dns_subnet(1) - }; - self.in_use.insert(new); - Ok(new) - } - - #[cfg(test)] - fn first(&self) -> Option { - self.in_use.first().copied() - } - - #[cfg(test)] - fn pop_first(&mut self) -> Option { - self.in_use.pop_first() - } - - #[cfg(test)] - fn last(&self) -> Option { - self.in_use.last().cloned() - } - - #[cfg(test)] - fn len(&self) -> usize { - self.in_use.len() - } -} - -#[cfg(test)] -pub mod test { - use super::*; - use crate::blueprint_builder::test::verify_blueprint; - use crate::example::ExampleSystemBuilder; - use nexus_types::deployment::BlueprintZoneDisposition; - use nexus_types::deployment::BlueprintZoneType; - use nexus_types::deployment::blueprint_zone_type::InternalDns; - use omicron_common::disk::DatasetKind; - use omicron_common::policy::INTERNAL_DNS_REDUNDANCY; - use omicron_test_utils::dev::test_setup_log; - - #[test] - fn test_dns_subnet_allocator() { - static TEST_NAME: &str = "test_dns_subnet_allocator"; - let logctx = test_setup_log(TEST_NAME); - - // Our test below assumes `INTERNAL_DNS_REDUNDANCY` is greater than 1 - // (so we can test adding more); fail fast if that changes. - assert!(INTERNAL_DNS_REDUNDANCY > 1); - - // Use our example system to create a blueprint and input. - let (_, mut blueprint1) = - ExampleSystemBuilder::new(&logctx.log, TEST_NAME) - .nsleds(INTERNAL_DNS_REDUNDANCY) - .build(); - - // `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 (_, sled_config) in blueprint1.sleds.iter_mut().skip(1) { - sled_config.zones.retain(|z| !z.zone_type.is_internal_dns()); - } - let npruned = blueprint1.sleds.len() - 1; - assert!(npruned > 0); - - // Also prune out the zones' datasets, or we're left with an invalid - // blueprint. - for (_, sled_config) in blueprint1.sleds.iter_mut().skip(1) { - sled_config.datasets.retain(|dataset| { - // This is gross; once zone configs know explicit dataset IDs, - // we should retain by ID instead. - match &dataset.kind { - DatasetKind::InternalDns => false, - DatasetKind::TransientZone { name } => { - !name.starts_with("oxz_internal_dns") - } - _ => true, - } - }); - } - - verify_blueprint(&blueprint1); - - // Create an allocator. - let mut allocator = InternalDnsSubnetAllocator::new( - blueprint1 - .all_omicron_zones(BlueprintZoneDisposition::is_in_service) - .filter_map(|(_sled_id, zone_config)| { - match zone_config.zone_type { - BlueprintZoneType::InternalDns(InternalDns { - dns_address, - .. - }) => Some(DnsSubnet::from_addr(*dns_address.ip())), - _ => None, - } - }) - .collect(), - ); - - // There should be exactly one subnet allocated. - assert_eq!(allocator.len(), 1, "should be 1 subnets allocated"); - let first = allocator.first().expect("should be a first subnet"); - let mut last = allocator.last().expect("should be a last subnet"); - assert_eq!(first, last); - let rack_subnet = first.rack_subnet(); - - // Allocate `npruned` new subnets. - for _ in 0..npruned { - let new = allocator.alloc(rack_subnet).expect("allocated a subnet"); - assert!( - new > last, - "newly allocated subnets should be after prior ones" - ); - assert_eq!( - new.rack_subnet(), - last.rack_subnet(), - "newly allocated subnets should be in the same rack subnet" - ); - last = new; - } - assert_eq!( - allocator.len(), - INTERNAL_DNS_REDUNDANCY, - "should be {INTERNAL_DNS_REDUNDANCY} subnets allocated" - ); - allocator.alloc(rack_subnet).expect_err("no subnets available"); - - // Test packing. - let first = allocator.pop_first().expect("should be a first subnet"); - let second = allocator.pop_first().expect("should be a second subnet"); - assert!(first < second, "first should be before second"); - assert_eq!( - allocator.alloc(rack_subnet).expect("allocation failed"), - first, - "should get first subnet" - ); - assert_eq!( - allocator.alloc(rack_subnet).expect("allocation failed"), - second, - "should get second subnet" - ); - allocator.alloc(rack_subnet).expect_err("no subnets available"); - - // Done! - logctx.cleanup_successful(); - } -} diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index 31e4d1d61e8..45517c54611 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -596,6 +596,8 @@ impl ExampleSystemBuilder { ) .unwrap(); } + let mut internal_dns_subnets = + builder.available_internal_dns_subnets().unwrap(); for _ in 0..self .internal_dns_count .on(discretionary_ix, discretionary_sled_count) @@ -608,6 +610,9 @@ impl ExampleSystemBuilder { .expect( "obtained InternalDNS image source", ), + internal_dns_subnets.next().expect( + "sufficient available internal DNS subnets", + ), ) .unwrap(); } diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index da6ec986921..99ea102373f 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -22,6 +22,7 @@ use crate::planner::image_source::NoopConvertHostPhase2Contents; use crate::planner::image_source::NoopConvertZoneStatus; use crate::planner::omicron_zone_placement::PlacementError; use iddqd::IdOrdMap; +use itertools::Either; use itertools::Itertools; use nexus_sled_agent_shared::inventory::ConfigReconcilerInventoryResult; use nexus_sled_agent_shared::inventory::OmicronZoneImageSource; @@ -66,6 +67,7 @@ use slog::{Logger, info, o, warn}; use slog_error_chain::InlineErrorChain; use std::collections::BTreeMap; use std::collections::BTreeSet; +use std::iter; use std::str::FromStr; use std::sync::Arc; use swrite::SWrite; @@ -1256,6 +1258,18 @@ impl<'a> Planner<'a> { image_source: BlueprintZoneImageSource, report: &mut PlanningAddStepReport, ) -> Result<(), Error> { + // If `kind` is "internal DNS", we'll need to pick subnets, but + // computing the available subnets isn't free. We could do something + // fancy with lazy construction, but that gets a little messy. Instead, + // always construct an iterator, and create an empty iterator for any + // `kind` that isn't "internal DNS". + let mut available_internal_dns_subnets = match kind { + DiscretionaryOmicronZone::InternalDns => { + Either::Left(self.blueprint.available_internal_dns_subnets()?) + } + _ => Either::Right(iter::empty()), + }; + for i in 0..num_zones_to_add { let sled_id = match zone_placement.place_zone(kind) { Ok(sled_id) => sled_id, @@ -1296,7 +1310,12 @@ impl<'a> Planner<'a> { .blueprint .sled_add_zone_crucible_pantry(sled_id, image)?, DiscretionaryOmicronZone::InternalDns => { - self.blueprint.sled_add_zone_internal_dns(sled_id, image)? + let dns_subnet = available_internal_dns_subnets + .next() + .ok_or(Error::NoAvailableDnsSubnets)?; + self.blueprint.sled_add_zone_internal_dns( + sled_id, image, dns_subnet, + )? } DiscretionaryOmicronZone::ExternalDns => { self.blueprint.sled_add_zone_external_dns(sled_id, image)?