Skip to content

Commit f498e47

Browse files
jgallagherjgallagher
authored
Remove OmicronZoneConfig::underlay_address (#6794)
This field (an IP only, no port) was effectively duplicated by every `OmicronZoneType` variant, which themselves carry one (or more than one) `SocketAddrV6` whose IP was always expected to match its containing `OmicronZoneConfig`'s `underlay_address`, and we had parts of `sled-agent` that was relying on that fact. This is purely a cleanup PR that removes the field. Most of the changes here are mechanical: removing the field and instead using the socket address from within `OmicronZoneType` instead. The changes to `sled-agent/src/services.rs` warrant the closest look, because these were the spots where we were combining `underlay_address` with constant port numbers and ignoring the full socket address. ~~(We're still constructing socket addresses via constant port numbers for some zone types that run multiple servers on different ports; these are all marked `TODO-john`, and I'll update those comments to refer to a new issue shortly.)~~ Filed #6796 I'm marking this as a draft both because it's a change to a fairly fundamental sled-agent type that I'd like to test on real hardware before merging, and because it should land after we cut R11. But this passes CI and a4x2, so I believe it's ready for review despite its draft status. Fixes #6651.
1 parent d2c686d commit f498e47

File tree

36 files changed

+347
-380
lines changed

36 files changed

+347
-380
lines changed

‎internal-dns/types/src/config.rs

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ use omicron_uuid_kinds::{OmicronZoneUuid, SledUuid};
6969
use schemars::JsonSchema;
7070
use serde::{Deserialize, Serialize};
7171
use std::collections::{BTreeMap, HashMap};
72-
use std::net::{Ipv4Addr, Ipv6Addr};
72+
use std::net::{Ipv4Addr, Ipv6Addr, SocketAddrV6};
7373

7474
/// Used to construct the DNS name for a control plane host
7575
#[derive(Clone, Debug, PartialEq, PartialOrd)]
@@ -367,12 +367,11 @@ impl DnsConfigBuilder {
367367
pub fn host_zone_with_one_backend(
368368
&mut self,
369369
zone_id: OmicronZoneUuid,
370-
addr: Ipv6Addr,
371370
service: ServiceName,
372-
port: u16,
371+
addr: SocketAddrV6,
373372
) -> anyhow::Result<()> {
374-
let zone = self.host_zone(zone_id, addr)?;
375-
self.service_backend_zone(service, &zone, port)
373+
let zone = self.host_zone(zone_id, *addr.ip())?;
374+
self.service_backend_zone(service, &zone, addr.port())
376375
}
377376

378377
/// Higher-level shorthand for adding a "switch" zone with its usual set of
@@ -423,18 +422,17 @@ impl DnsConfigBuilder {
423422
pub fn host_zone_clickhouse(
424423
&mut self,
425424
zone_id: OmicronZoneUuid,
426-
underlay_address: Ipv6Addr,
427425
http_service: ServiceName,
428-
http_port: u16,
426+
http_address: SocketAddrV6,
429427
) -> anyhow::Result<()> {
430428
anyhow::ensure!(
431429
http_service == ServiceName::Clickhouse
432430
|| http_service == ServiceName::ClickhouseServer,
433431
"This method is only valid for ClickHouse replica servers, \
434432
but we were provided the service '{http_service:?}'",
435433
);
436-
let zone = self.host_zone(zone_id, underlay_address)?;
437-
self.service_backend_zone(http_service, &zone, http_port)?;
434+
let zone = self.host_zone(zone_id, *http_address.ip())?;
435+
self.service_backend_zone(http_service, &zone, http_address.port())?;
438436
self.service_backend_zone(
439437
ServiceName::ClickhouseNative,
440438
&zone,
@@ -457,23 +455,22 @@ impl DnsConfigBuilder {
457455
///
458456
/// # Errors
459457
///
460-
/// This fails if the provided `http_service` is not for a ClickhouseKeeper
458+
/// This fails if the provided `service` is not for a ClickhouseKeeper
461459
/// replica server. It also fails if the given zone has already been added
462460
/// to the configuration.
463461
pub fn host_zone_clickhouse_keeper(
464462
&mut self,
465463
zone_id: OmicronZoneUuid,
466-
underlay_address: Ipv6Addr,
467464
service: ServiceName,
468-
port: u16,
465+
address: SocketAddrV6,
469466
) -> anyhow::Result<()> {
470467
anyhow::ensure!(
471468
service == ServiceName::ClickhouseKeeper,
472469
"This method is only valid for ClickHouse keeper servers, \
473470
but we were provided the service '{service:?}'",
474471
);
475-
let zone = self.host_zone(zone_id, underlay_address)?;
476-
self.service_backend_zone(service, &zone, port)?;
472+
let zone = self.host_zone(zone_id, *address.ip())?;
473+
self.service_backend_zone(service, &zone, address.port())?;
477474
self.service_backend_zone(
478475
ServiceName::ClickhouseAdminKeeper,
479476
&zone,

‎live-tests/tests/test_nexus_add_remove.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ async fn test_nexus_add_remove(lc: &LiveTestContext) {
9090
.first()
9191
.expect("at least one added zone on that sled");
9292
assert_eq!(new_zone.kind(), ZoneKind::Nexus);
93-
let new_zone_addr = new_zone.underlay_address();
93+
let new_zone_addr = new_zone.underlay_ip();
9494
let new_zone_sockaddr =
9595
SocketAddrV6::new(new_zone_addr, NEXUS_INTERNAL_PORT, 0, 0);
9696
let new_zone_client = lc.specific_internal_nexus_client(new_zone_sockaddr);

‎nexus-sled-agent-shared/src/inventory.rs

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,6 @@ impl OmicronZonesConfig {
151151
)]
152152
pub struct OmicronZoneConfig {
153153
pub id: OmicronZoneUuid,
154-
pub underlay_address: Ipv6Addr,
155154

156155
/// The pool on which we'll place this zone's root filesystem.
157156
///
@@ -161,6 +160,16 @@ pub struct OmicronZoneConfig {
161160
pub zone_type: OmicronZoneType,
162161
}
163162

163+
impl OmicronZoneConfig {
164+
/// Returns the underlay IP address associated with this zone.
165+
///
166+
/// Assumes all zone have exactly one underlay IP address (which is
167+
/// currently true).
168+
pub fn underlay_ip(&self) -> Ipv6Addr {
169+
self.zone_type.underlay_ip()
170+
}
171+
}
172+
164173
/// Describes a persistent ZFS dataset associated with an Omicron zone
165174
#[derive(
166175
Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq, Hash,
@@ -308,6 +317,37 @@ impl OmicronZoneType {
308317
}
309318
}
310319

320+
/// Returns the underlay IP address associated with this zone.
321+
///
322+
/// Assumes all zone have exactly one underlay IP address (which is
323+
/// currently true).
324+
pub fn underlay_ip(&self) -> Ipv6Addr {
325+
match self {
326+
OmicronZoneType::BoundaryNtp { address, .. }
327+
| OmicronZoneType::Clickhouse { address, .. }
328+
| OmicronZoneType::ClickhouseKeeper { address, .. }
329+
| OmicronZoneType::ClickhouseServer { address, .. }
330+
| OmicronZoneType::CockroachDb { address, .. }
331+
| OmicronZoneType::Crucible { address, .. }
332+
| OmicronZoneType::CruciblePantry { address }
333+
| OmicronZoneType::ExternalDns { http_address: address, .. }
334+
| OmicronZoneType::InternalNtp { address }
335+
| OmicronZoneType::Nexus { internal_address: address, .. }
336+
| OmicronZoneType::Oximeter { address } => *address.ip(),
337+
OmicronZoneType::InternalDns {
338+
http_address: address,
339+
dns_address,
340+
..
341+
} => {
342+
// InternalDns is the only variant that carries two
343+
// `SocketAddrV6`s that are both on the underlay network. We
344+
// expect these to have the same IP address.
345+
debug_assert_eq!(address.ip(), dns_address.ip());
346+
*address.ip()
347+
}
348+
}
349+
}
350+
311351
/// Identifies whether this is an NTP zone
312352
pub fn is_ntp(&self) -> bool {
313353
match self {

‎nexus/db-model/src/deployment.rs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,6 @@ pub struct BpOmicronZone {
239239
pub blueprint_id: Uuid,
240240
pub sled_id: DbTypedUuid<SledKind>,
241241
pub id: DbTypedUuid<OmicronZoneKind>,
242-
pub underlay_address: ipv6::Ipv6Addr,
243242
pub zone_type: ZoneType,
244243
pub primary_service_ip: ipv6::Ipv6Addr,
245244
pub primary_service_port: SqlU16,
@@ -282,7 +281,6 @@ impl BpOmicronZone {
282281
blueprint_id,
283282
sled_id: sled_id.into(),
284283
id: blueprint_zone.id.into(),
285-
underlay_address: blueprint_zone.underlay_address.into(),
286284
external_ip_id,
287285
filesystem_pool: blueprint_zone
288286
.filesystem_pool
@@ -675,7 +673,6 @@ impl BpOmicronZone {
675673
Ok(BlueprintZoneConfig {
676674
disposition: self.disposition.into(),
677675
id: self.id.into(),
678-
underlay_address: self.underlay_address.into(),
679676
filesystem_pool: self
680677
.filesystem_pool
681678
.map(|id| ZpoolName::new_external(id.into())),

‎nexus/db-model/src/inventory.rs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1297,7 +1297,6 @@ pub struct InvOmicronZone {
12971297
pub inv_collection_id: DbTypedUuid<CollectionKind>,
12981298
pub sled_id: DbTypedUuid<SledKind>,
12991299
pub id: DbTypedUuid<OmicronZoneKind>,
1300-
pub underlay_address: ipv6::Ipv6Addr,
13011300
pub zone_type: ZoneType,
13021301
pub primary_service_ip: ipv6::Ipv6Addr,
13031302
pub primary_service_port: SqlU16,
@@ -1332,7 +1331,6 @@ impl InvOmicronZone {
13321331
inv_collection_id: inv_collection_id.into(),
13331332
sled_id: sled_id.into(),
13341333
id: zone.id.into(),
1335-
underlay_address: zone.underlay_address.into(),
13361334
filesystem_pool: zone
13371335
.filesystem_pool
13381336
.as_ref()
@@ -1648,7 +1646,6 @@ impl InvOmicronZone {
16481646

16491647
Ok(OmicronZoneConfig {
16501648
id: self.id.into(),
1651-
underlay_address: self.underlay_address.into(),
16521649
filesystem_pool: self
16531650
.filesystem_pool
16541651
.map(|id| ZpoolName::new_external(id.into())),

‎nexus/db-model/src/schema.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1541,7 +1541,6 @@ table! {
15411541
sled_id -> Uuid,
15421542

15431543
id -> Uuid,
1544-
underlay_address -> Inet,
15451544
zone_type -> crate::ZoneTypeEnum,
15461545

15471546
primary_service_ip -> Inet,

‎nexus/db-model/src/schema_versions.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use std::collections::BTreeMap;
1717
///
1818
/// This must be updated when you change the database schema. Refer to
1919
/// schema/crdb/README.adoc in the root of this repository for details.
20-
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(110, 0, 0);
20+
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(111, 0, 0);
2121

2222
/// List of all past database schema versions, in *reverse* order
2323
///
@@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy<Vec<KnownVersion>> = Lazy::new(|| {
2929
// | leaving the first copy as an example for the next person.
3030
// v
3131
// KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
32+
KnownVersion::new(111, "drop-omicron-zone-underlay-address"),
3233
KnownVersion::new(110, "clickhouse-policy"),
3334
KnownVersion::new(109, "inv-clickhouse-keeper-membership"),
3435
KnownVersion::new(108, "internet-gateway"),

‎nexus/db-queries/src/db/datastore/deployment.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2532,7 +2532,6 @@ mod tests {
25322532
zones: vec![BlueprintZoneConfig {
25332533
disposition: BlueprintZoneDisposition::InService,
25342534
id: zone_id,
2535-
underlay_address: Ipv6Addr::LOCALHOST,
25362535
filesystem_pool: None,
25372536
zone_type: BlueprintZoneType::Nexus(
25382537
blueprint_zone_type::Nexus {

‎nexus/db-queries/src/db/datastore/deployment/external_networking.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,6 @@ mod tests {
440440
use omicron_uuid_kinds::ZpoolUuid;
441441
use oxnet::IpNet;
442442
use std::net::IpAddr;
443-
use std::net::Ipv6Addr;
444443
use std::net::SocketAddr;
445444
use uuid::Uuid;
446445

@@ -581,7 +580,6 @@ mod tests {
581580
BlueprintZoneConfig {
582581
disposition: BlueprintZoneDisposition::InService,
583582
id: self.nexus_id,
584-
underlay_address: Ipv6Addr::LOCALHOST,
585583
filesystem_pool: Some(ZpoolName::new_external(
586584
ZpoolUuid::new_v4(),
587585
)),
@@ -598,7 +596,6 @@ mod tests {
598596
BlueprintZoneConfig {
599597
disposition: BlueprintZoneDisposition::InService,
600598
id: self.dns_id,
601-
underlay_address: Ipv6Addr::LOCALHOST,
602599
filesystem_pool: Some(ZpoolName::new_external(
603600
ZpoolUuid::new_v4(),
604601
)),
@@ -618,7 +615,6 @@ mod tests {
618615
BlueprintZoneConfig {
619616
disposition: BlueprintZoneDisposition::InService,
620617
id: self.ntp_id,
621-
underlay_address: Ipv6Addr::LOCALHOST,
622618
filesystem_pool: Some(ZpoolName::new_external(
623619
ZpoolUuid::new_v4(),
624620
)),

‎nexus/db-queries/src/db/datastore/rack.rs

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1378,7 +1378,6 @@ mod test {
13781378
BlueprintZoneConfig {
13791379
disposition: BlueprintZoneDisposition::InService,
13801380
id: external_dns_id,
1381-
underlay_address: Ipv6Addr::LOCALHOST,
13821381
filesystem_pool: Some(dataset.pool_name.clone()),
13831382
zone_type: BlueprintZoneType::ExternalDns(
13841383
blueprint_zone_type::ExternalDns {
@@ -1408,7 +1407,6 @@ mod test {
14081407
BlueprintZoneConfig {
14091408
disposition: BlueprintZoneDisposition::InService,
14101409
id: ntp1_id,
1411-
underlay_address: Ipv6Addr::LOCALHOST,
14121410
filesystem_pool: Some(random_zpool()),
14131411
zone_type: BlueprintZoneType::BoundaryNtp(
14141412
blueprint_zone_type::BoundaryNtp {
@@ -1451,7 +1449,6 @@ mod test {
14511449
BlueprintZoneConfig {
14521450
disposition: BlueprintZoneDisposition::InService,
14531451
id: nexus_id,
1454-
underlay_address: Ipv6Addr::LOCALHOST,
14551452
filesystem_pool: Some(random_zpool()),
14561453
zone_type: BlueprintZoneType::Nexus(
14571454
blueprint_zone_type::Nexus {
@@ -1484,7 +1481,6 @@ mod test {
14841481
BlueprintZoneConfig {
14851482
disposition: BlueprintZoneDisposition::InService,
14861483
id: ntp2_id,
1487-
underlay_address: Ipv6Addr::LOCALHOST,
14881484
filesystem_pool: Some(random_zpool()),
14891485
zone_type: BlueprintZoneType::BoundaryNtp(
14901486
blueprint_zone_type::BoundaryNtp {
@@ -1526,7 +1522,6 @@ mod test {
15261522
zones: vec![BlueprintZoneConfig {
15271523
disposition: BlueprintZoneDisposition::InService,
15281524
id: ntp3_id,
1529-
underlay_address: Ipv6Addr::LOCALHOST,
15301525
filesystem_pool: Some(random_zpool()),
15311526
zone_type: BlueprintZoneType::InternalNtp(
15321527
blueprint_zone_type::InternalNtp {
@@ -1707,7 +1702,6 @@ mod test {
17071702
BlueprintZoneConfig {
17081703
disposition: BlueprintZoneDisposition::InService,
17091704
id: nexus_id1,
1710-
underlay_address: Ipv6Addr::LOCALHOST,
17111705
filesystem_pool: Some(random_zpool()),
17121706
zone_type: BlueprintZoneType::Nexus(
17131707
blueprint_zone_type::Nexus {
@@ -1740,7 +1734,6 @@ mod test {
17401734
BlueprintZoneConfig {
17411735
disposition: BlueprintZoneDisposition::InService,
17421736
id: nexus_id2,
1743-
underlay_address: Ipv6Addr::LOCALHOST,
17441737
filesystem_pool: Some(random_zpool()),
17451738
zone_type: BlueprintZoneType::Nexus(
17461739
blueprint_zone_type::Nexus {
@@ -1980,7 +1973,6 @@ mod test {
19801973
zones: vec![BlueprintZoneConfig {
19811974
disposition: BlueprintZoneDisposition::InService,
19821975
id: nexus_id,
1983-
underlay_address: Ipv6Addr::LOCALHOST,
19841976
filesystem_pool: Some(random_zpool()),
19851977
zone_type: BlueprintZoneType::Nexus(
19861978
blueprint_zone_type::Nexus {
@@ -2090,7 +2082,6 @@ mod test {
20902082
BlueprintZoneConfig {
20912083
disposition: BlueprintZoneDisposition::InService,
20922084
id: external_dns_id,
2093-
underlay_address: Ipv6Addr::LOCALHOST,
20942085
filesystem_pool: Some(dataset.pool_name.clone()),
20952086
zone_type: BlueprintZoneType::ExternalDns(
20962087
blueprint_zone_type::ExternalDns {
@@ -2120,7 +2111,6 @@ mod test {
21202111
BlueprintZoneConfig {
21212112
disposition: BlueprintZoneDisposition::InService,
21222113
id: nexus_id,
2123-
underlay_address: Ipv6Addr::LOCALHOST,
21242114
filesystem_pool: Some(random_zpool()),
21252115
zone_type: BlueprintZoneType::Nexus(
21262116
blueprint_zone_type::Nexus {

‎nexus/inventory/src/collector.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -576,7 +576,6 @@ mod test {
576576
generation: Generation::from(3),
577577
zones: vec![OmicronZoneConfig {
578578
id: zone_id,
579-
underlay_address: *zone_address.ip(),
580579
zone_type: OmicronZoneType::Oximeter {
581580
address: zone_address,
582581
},

0 commit comments

Comments
 (0)