[nexus] Don't assumed hardcoded MGD port (#10435)

jgallagher · web-flow · commit a9bfb1fa6721 · 2026-05-13T09:05:00.000-04:00
Prior to this PR, we had two different implementations that build up a `HashMap<SwitchSlot, MgdClient>` via the same process: 1. Call `switch_zone_address_mappings()` to get a `HashMap<SwitchSlot, Ipv6Addr>` via a couple different DNS lookups + a query to each switch zone's MGS 2. Look up MGD in DNS 3. Pair up the IP addresses from 1 and 2 to build a `HashMap<SwitchSlot, MgdClient>` using the ports from 2; for any IPs in 1 that we didn't find in 2, construct a client pointed to the IP from 1 with the hardcoded `MGD_PORT` I believe the history here is that we used to not have step 2 at all, and we just slapped `MGD_PORT` on all the IPs from 1 (back when MGD didn't have its own DNS entries). And all of this predates being able to ask MGD itself which switch slot it is (internally, it asks MGS over `::1` within its own switch zone). This PR makes two changes: it squishes us down to a single implementation, and changes the mechanics of that implementation to: 1. Look up MGD in DNS 2. For each entry returned, construct an MgdClient and ask it for its switch slot This potentially introduces a new failure mode: if MGD doesn't know its own switch slot but we would've been able to ask MGS at the same IP for the switch slot, the prior implementation would've worked and this PR won't. But that failure mode ought to only be possible if MGD itself is buggy: it asking MGS over localhost for the switch slot should, in general, be more successful than Nexus finding MGS in DNS then asking MGS for the switch slot, since there are a lot more moving pieces in that path. But I think the upsides are enough to make up for that risk - we streamline the process, reduce duplication, the new implementation has fewer places to fail transiently overall, and we always use port numbers from DNS (which should make this more reliable in tests - in tests we basically never want to assume the fixed `MGD_PORT`, and pairing up IP addresses also doesn't usually work since they're all `::1`). I'd like to remove `switch_zone_address_mappings()` altogether, but removing the remaining two uses of it are both blocked - lldp by #10361, and building up the scrimlet clients by #10167.
diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out
@@ -626,7 +626,7 @@ task: "bfd_manager"
   configured period: every <REDACTED_DURATION>s
   last completed activation: <REDACTED ITERATIONS>, triggered by <TRIGGERED_BY_REDACTED>
     started at <REDACTED_TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
-    last completion reported error: failed to resolve addresses for Dendrite services: proto error: no records found for Query { name: Name("_dendrite._tcp.control-plane.oxide.internal."), query_type: SRV, query_class: IN }
+    last completion reported error: proto error: no records found for Query { name: Name("_mgd._tcp.control-plane.oxide.internal."), query_type: SRV, query_class: IN }
 
 task: "blueprint_planner"
   configured period: every <REDACTED_DURATION>m
@@ -1309,7 +1309,7 @@ task: "bfd_manager"
   configured period: every <REDACTED_DURATION>s
   last completed activation: <REDACTED ITERATIONS>, triggered by <TRIGGERED_BY_REDACTED>
     started at <REDACTED_TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
-    last completion reported error: failed to resolve addresses for Dendrite services: proto error: no records found for Query { name: Name("_dendrite._tcp.control-plane.oxide.internal."), query_type: SRV, query_class: IN }
+    last completion reported error: proto error: no records found for Query { name: Name("_mgd._tcp.control-plane.oxide.internal."), query_type: SRV, query_class: IN }
 
 task: "blueprint_planner"
   configured period: every <REDACTED_DURATION>m
diff --git a/nexus/src/app/background/mod.rs b/nexus/src/app/background/mod.rs
@@ -142,6 +142,7 @@ pub(crate) use init::BackgroundTasksInternal;
 pub use nexus_background_task_interface::Activator;
 pub(crate) use tasks::blueprint_load::LoadedTargetBlueprint;
 pub use tasks::fm_sitrep_load::CurrentSitrep;
+pub(crate) use tasks::networking::resolve_mgd_clients;
 pub use tasks::saga_recovery::SagaRecoveryHelpers;
 
 use futures::future::BoxFuture;
diff --git a/nexus/src/app/background/tasks/bfd.rs b/nexus/src/app/background/tasks/bfd.rs
@@ -5,12 +5,8 @@
 //! Background task for managing switch bidirectional forwarding detection
 //! (BFD) sessions.
 
-use crate::app::{
-    background::tasks::networking::build_mgd_clients,
-    switch_zone_address_mappings,
-};
-
 use crate::app::background::BackgroundTask;
+use crate::app::background::tasks::networking::resolve_mgd_clients;
 use futures::FutureExt;
 use futures::future::BoxFuture;
 use internal_dns_resolver::Resolver;
@@ -120,29 +116,27 @@ impl BackgroundTask for BfdManager {
 
             let mut current: HashSet<BfdSessionKey> = HashSet::new();
 
-            let mappings = match switch_zone_address_mappings(&self.resolver, log).await {
-                Ok(mappings) => mappings,
-                Err(e) => {
-                    error!(log, "failed to resolve addresses for Dendrite services"; "error" => %e);
-                    return json!({
-                        "error":
-                            format!(
-                                "failed to resolve addresses for Dendrite services: {:#}",
-                                e
-                            )
-                    });
-                },
-            };
-
-            let mgd_clients = build_mgd_clients(mappings, log, &self.resolver).await;
+            let mgd_clients =
+                match resolve_mgd_clients(&self.resolver, log).await {
+                    Ok(clients) => clients,
+                    Err(e) => {
+                        let e = InlineErrorChain::new(&e);
+                        error!(
+                            log,
+                            "failed to resolve addresses for MGD services";
+                            &e,
+                        );
+                        return json!({ "error": e.to_string() });
+                    }
+                };
 
             for (location, c) in &mgd_clients {
                 let client_current = match c.get_bfd_peers().await {
                     Ok(x) => x.into_inner(),
                     Err(e) => {
                         error!(&log, "failed to get bfd sessions from mgd: {}",
                             c.baseurl();
-                            "error" => e.to_string()
+                            InlineErrorChain::new(&e),
                         );
                         continue;
                     }
diff --git a/nexus/src/app/background/tasks/networking.rs b/nexus/src/app/background/tasks/networking.rs
@@ -6,47 +6,68 @@ use db::datastore::SwitchPortSettingsCombinedResult;
 use dpd_client::types::{
     LinkCreate, LinkId, LinkSettings, PortFec, PortSettings, PortSpeed, TxEq,
 };
+use internal_dns_resolver::ResolveError;
 use internal_dns_types::names::ServiceName;
 use nexus_db_model::{SwitchLinkFec, SwitchLinkSpeed};
 use nexus_db_queries::db;
-use omicron_common::address::MGD_PORT;
 use sled_agent_types::early_networking::SwitchSlot;
-use std::{
-    collections::HashMap,
-    net::{Ipv6Addr, SocketAddrV6},
-};
+use slog_error_chain::InlineErrorChain;
+use std::collections::HashMap;
 
-pub(crate) async fn build_mgd_clients(
-    mappings: HashMap<SwitchSlot, std::net::Ipv6Addr>,
-    log: &slog::Logger,
+/// Get all MGD known `SwitchSlot -> MGD client` pairs.
+///
+/// # Errors
+///
+/// Fails if we cannot resolve MGD in DNS.
+///
+/// For any MGD instance we resolve via DNS, if the MGD instance does not know
+/// its own switch slot, the switch slot -> client mapping for that instance
+/// will be omitted from the returned map. Callers must not assume an `Ok(_)`
+/// return value contains any client.
+pub(crate) async fn resolve_mgd_clients(
     resolver: &internal_dns_resolver::Resolver,
-) -> HashMap<SwitchSlot, mg_admin_client::Client> {
-    let mut clients: Vec<(SwitchSlot, mg_admin_client::Client)> = vec![];
-    for (switch_slot, addr) in &mappings {
-        let port = match resolver.lookup_all_socket_v6(ServiceName::Mgd).await {
-            Ok(addrs) => {
-                let port_map: HashMap<Ipv6Addr, u16> = addrs
-                    .into_iter()
-                    .map(|sockaddr| (*sockaddr.ip(), sockaddr.port()))
-                    .collect();
-
-                *port_map.get(&addr).unwrap_or(&MGD_PORT)
-            }
-            Err(e) => {
-                error!(log, "failed to  addresses"; "error" => %e);
-                MGD_PORT
-            }
-        };
-
-        let socketaddr =
-            std::net::SocketAddr::V6(SocketAddrV6::new(*addr, port, 0, 0));
+    log: &slog::Logger,
+) -> Result<HashMap<SwitchSlot, mg_admin_client::Client>, ResolveError> {
+    let mgd_addrs = resolver.lookup_all_socket_v6(ServiceName::Mgd).await?;
+    let mut clients = HashMap::new();
+    for addr in mgd_addrs {
         let client = mg_admin_client::Client::new(
-            format!("http://{}", socketaddr).as_str(),
+            &format!("http://{addr}"),
             log.clone(),
         );
-        clients.push((*switch_slot, client));
+        let switch_slot = match client.switch_identifiers().await {
+            Ok(response) => match response.slot {
+                Some(0) => SwitchSlot::Switch0,
+                Some(1) => SwitchSlot::Switch1,
+                Some(n) => {
+                    warn!(
+                        log, "failed to determine switch slot for mgd";
+                        "addr" => %addr,
+                        "error" => format!("mgd returned unknown slot {n}"),
+                    );
+                    continue;
+                }
+                None => {
+                    warn!(
+                        log, "failed to determine switch slot for mgd";
+                        "addr" => %addr,
+                        "error" => "mgd does not yet know its switch slot",
+                    );
+                    continue;
+                }
+            },
+            Err(err) => {
+                warn!(
+                    log, "failed to determine switch slot for mgd";
+                    "addr" => %addr,
+                    InlineErrorChain::new(&err),
+                );
+                continue;
+            }
+        };
+        clients.insert(switch_slot, client);
     }
-    clients.into_iter().collect::<HashMap<_, _>>()
+    Ok(clients)
 }
 
 pub(crate) fn api_to_dpd_port_settings(
diff --git a/nexus/src/app/background/tasks/sync_switch_configuration.rs b/nexus/src/app/background/tasks/sync_switch_configuration.rs
@@ -8,7 +8,7 @@
 use crate::app::{
     background::{
         LoadedTargetBlueprint,
-        tasks::networking::{api_to_dpd_port_settings, build_mgd_clients},
+        tasks::networking::{api_to_dpd_port_settings, resolve_mgd_clients},
     },
     dpd_clients, switch_zone_address_mappings,
 };
@@ -364,7 +364,7 @@ impl BackgroundTask for SwitchPortSettingsManager {
                 let dpd_clients = match
                     dpd_clients(&self.resolver, &log).await
                 {
-                    Ok(mappings) => mappings,
+                    Ok(clients) => clients,
                     Err(e) => {
                         error!(
                             log,
@@ -376,7 +376,18 @@ impl BackgroundTask for SwitchPortSettingsManager {
 
                 // TODO https://github.com/oxidecomputer/omicron/issues/5201
                 // build mgd clients
-                let mgd_clients = build_mgd_clients(mappings, &log, &self.resolver).await;
+                let mgd_clients =
+                    match resolve_mgd_clients(&self.resolver, &log).await {
+                        Ok(clients) => clients,
+                        Err(e) => {
+                            error!(
+                                log,
+                                "failed to resolve addresses for MGD";
+                                InlineErrorChain::new(&e),
+                            );
+                            continue;
+                        },
+                    };
 
                 let port_list = match self.switch_ports(opctx, &log).await {
                     Ok(value) => value,
diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs
@@ -10,12 +10,14 @@ use crate::DropshotServer;
 use crate::app::background::BackgroundTasksData;
 use crate::app::background::CurrentSitrep;
 use crate::app::background::SagaRecoveryHelpers;
+use crate::app::background::resolve_mgd_clients;
 use crate::app::update::UpdateStatusHandle;
 use crate::populate::PopulateArgs;
 use crate::populate::PopulateStatus;
 use crate::populate::populate_start;
 use ::oximeter::types::ProducerRegistry;
 use anyhow::anyhow;
+use internal_dns_resolver::ResolveError;
 use internal_dns_types::names::ServiceName;
 use nexus_background_task_interface::BackgroundTasks;
 use nexus_config::NexusConfig;
@@ -32,7 +34,6 @@ use nexus_mgs_updates::MgsUpdateDriver;
 use nexus_types::deployment::PendingMgsUpdates;
 use nexus_types::deployment::ReconfiguratorConfigParam;
 
-use omicron_common::address::MGD_PORT;
 use omicron_common::address::MGS_PORT;
 use omicron_common::api::external::ByteCount;
 use omicron_common::api::external::Error;
@@ -44,7 +45,6 @@ use sled_agent_types::early_networking::SwitchSlot;
 use slog::Logger;
 use slog_error_chain::InlineErrorChain;
 use std::collections::HashMap;
-use std::net::SocketAddr;
 use std::net::SocketAddrV6;
 use std::net::{IpAddr, Ipv6Addr};
 use std::num::NonZeroU32;
@@ -1178,47 +1178,21 @@ impl Nexus {
         lldpd_clients(resolver, rack_id, &self.log).await
     }
 
+    /// Get all MGD known `SwitchSlot -> MGD client` pairs.
+    ///
+    /// # Errors
+    ///
+    /// Fails if we cannot resolve MGD in DNS.
+    ///
+    /// For any MGD instance we resolve via DNS, if the MGD instance does not
+    /// know its own switch slot, the switch slot -> client mapping for that
+    /// instance will be omitted from the returned map. Callers must not
+    /// assume an `Ok(_)` return value contains any client.
     pub(crate) async fn mg_clients(
         &self,
-    ) -> Result<HashMap<SwitchSlot, mg_admin_client::Client>, String> {
-        let resolver = self.resolver();
-        let mappings =
-            switch_zone_address_mappings(resolver, &self.log).await?;
-        let mgd_addrs = resolver
-            .lookup_all_socket_v6(ServiceName::Mgd)
-            .await
-            .map_err(|err| {
-            format!(
-                "failed to resolve mgd in DNS: {}",
-                InlineErrorChain::new(&err)
-            )
-        })?;
-        let mut clients = HashMap::new();
-        for (switch_slot, ip) in mappings {
-            let addr =
-                match mgd_addrs.iter().copied().find(|addr| *addr.ip() == ip) {
-                    Some(addr) => SocketAddr::V6(addr),
-                    None => {
-                        warn!(
-                            self.log,
-                            "no MGD DNS entry found matching switch slot \
-                             IP address; assuming default port";
-                            "switch-slot" => ?switch_slot,
-                            "switch-ip" => %ip,
-                            "mgd-dns-entries" => ?mgd_addrs,
-                        );
-                        SocketAddr::V6(SocketAddrV6::new(ip, MGD_PORT, 0, 0))
-                    }
-                };
-            clients.insert(
-                switch_slot,
-                mg_admin_client::Client::new(
-                    &format!("http://{addr}"),
-                    self.log.clone(),
-                ),
-            );
-        }
-        Ok(clients)
+    ) -> Result<HashMap<SwitchSlot, mg_admin_client::Client>, ResolveError>
+    {
+        resolve_mgd_clients(self.resolver(), &self.log).await
     }
 
     pub(crate) fn demo_sagas(
diff --git a/nexus/test-utils/src/starter.rs b/nexus/test-utils/src/starter.rs
@@ -448,9 +448,13 @@ impl<'a, N: NexusServer> ControlPlaneStarter<'a, N> {
     pub async fn start_mgd(&mut self, switch_slot: SwitchSlot) {
         let log = &self.logctx.log;
         debug!(log, "Starting mgd"; "switch_slot" => ?switch_slot);
+        let mgs = self.gateway.get(&switch_slot).unwrap();
+        let mgs_addr =
+            SocketAddrV6::new(Ipv6Addr::LOCALHOST, mgs.port, 0, 0).into();
 
         // Set up an instance of mgd
-        let mgd = dev::maghemite::MgdInstance::start(0).await.unwrap();
+        let mgd =
+            dev::maghemite::MgdInstance::start(0, mgs_addr).await.unwrap();
         let port = mgd.port;
         self.mgd.insert(switch_slot, mgd);
         let address = SocketAddrV6::new(Ipv6Addr::LOCALHOST, port, 0, 0);
diff --git a/test-utils/src/dev/maghemite.rs b/test-utils/src/dev/maghemite.rs
@@ -4,6 +4,7 @@
 
 //! Tools for managing Maghemite during development
 
+use std::net::SocketAddr;
 use std::path::{Path, PathBuf};
 use std::process::Stdio;
 use std::time::Duration;
@@ -35,7 +36,10 @@ pub struct MgdInstance {
 }
 
 impl MgdInstance {
-    pub async fn start(mut port: u16) -> Result<Self, anyhow::Error> {
+    pub async fn start(
+        mut port: u16,
+        mgs_addr: SocketAddr,
+    ) -> Result<Self, anyhow::Error> {
         let temp_dir = TempDir::new()?;
 
         let args = vec![
@@ -51,6 +55,8 @@ impl MgdInstance {
             uuid::Uuid::new_v4().to_string(),
             "--sled-uuid".into(),
             uuid::Uuid::new_v4().to_string(),
+            "--mgs-addr".into(),
+            mgs_addr.to_string(),
         ];
 
         let child = tokio::process::Command::new("mgd")
