Skip to content

BlueprintBuilder: require caller to provide internal DNS subnet #9250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+66 −240
Open

BlueprintBuilder: require caller to provide internal DNS subnet #9250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 40 additions & 7 deletions nexus/reconfigurator/planning/src/blueprint_builder/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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")]
Expand Down Expand Up @@ -696,6 +698,40 @@ impl<'a> BlueprintBuilder<'a> {
self.new_blueprint_id
}

pub fn available_internal_dns_subnets(
&self,
) -> Result<impl Iterator<Item = DnsSubnet>, 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part I like the least. Some alternatives I considered:

  1. Instead of available_internal_dns_subnets(), provide internal_dns_subnets_in_use(), and require the caller to do this work and the filtering we do based on it below. This seems the most true to a "dumb builder" pattern, but puts some annoying work on every caller.
  2. Instead of returning an error on line 713, return an empty iterator if we don't have any commissioned sleds.
  3. A mix of 1 and 2: instead of returning a raw iterator, return an AvailableInternalDnsSubnets type that is more like the InternalDnsSubnetAllocator removed in this PR: internally it only tracks the "in use" subnets, and requires the caller to pass in a sled subnet (from which we can derive a rack subnet and the set of possible DNS subnets).

Do any of these seem more likely to put us in a better position w.r.t. multirack? (How will internal DNS subnets be configured in multirack?)


// 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::<BTreeSet<_>>();

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> {
Expand Down Expand Up @@ -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 =
Expand Down
1 change: 0 additions & 1 deletion nexus/reconfigurator/planning/src/blueprint_editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
37 changes: 1 addition & 36 deletions nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -35,7 +28,6 @@ pub enum BlueprintResourceAllocatorInputError {
#[derive(Debug)]
pub(crate) struct BlueprintResourceAllocator {
external_networking: ExternalNetworkingAllocator,
internal_dns: InternalDnsSubnetAllocator,
}

impl BlueprintResourceAllocator {
Expand All @@ -46,26 +38,6 @@ impl BlueprintResourceAllocator {
where
I: Iterator<Item = &'a SledEditor> + 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)
Expand All @@ -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<DnsSubnet, NoAvailableDnsSubnets> {
self.internal_dns.alloc(rack_subnet)
Ok(Self { external_networking })
}

pub(crate) fn next_external_ip_nexus(
Expand Down
195 changes: 0 additions & 195 deletions nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs
View file
Open in desktop

This file was deleted.

5 changes: 5 additions & 0 deletions nexus/reconfigurator/planning/src/example.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -608,6 +610,9 @@ impl ExampleSystemBuilder {
.expect(
"obtained InternalDNS image source",
),
internal_dns_subnets.next().expect(
"sufficient available internal DNS subnets",
),
)
.unwrap();
}
Expand Down
Loading
Loading