Bugfix (8206) - Don't fail switch slot resolution when one switch zone is unresponsive

nexus/src/app/background/tasks/sync_switch_configuration.rs

@@ -335,8 +335,8 @@ impl BackgroundTask for SwitchPortSettingsManager {
                 };
 
                 // TODO https://github.com/oxidecomputer/omicron/issues/5201
-                // build sled agent clients
-                let sled_agent_clients = build_sled_agent_clients(&mappings, &log);
+                // build sled agent clients for sleds that are connected to the switches
+                let scrimlet_sled_agent_clients = build_sled_agent_clients(&mappings, &log);
 
                 // TODO https://github.com/oxidecomputer/omicron/issues/5201
                 // build dpd clients
@@ -455,7 +455,7 @@ impl BackgroundTask for SwitchPortSettingsManager {
                 // yeet the messages
                 for (location, config) in &uplinks {
                     let client: &sled_agent_client::Client =
-                        match sled_agent_clients.get(location) {
+                        match scrimlet_sled_agent_clients.get(location) {
                             Some(client) => client,
                             None => {
                                 error!(log, "sled-agent client is missing, cannot send updates"; "location" => %location);
@@ -844,7 +844,7 @@ impl BackgroundTask for SwitchPortSettingsManager {
 
                 // Since we update the first scrimlet we can reach (we failover to the second one
                 // if updating the first one fails) we need to check them both.
-                for (_location, client) in &sled_agent_clients {
+                for (_location, client) in &scrimlet_sled_agent_clients {
                     let scrimlet_cfg  = match client.read_network_bootstore_config_cache().await {
                         Ok(config) => config,
                         Err(e) => {
@@ -1286,7 +1286,7 @@ impl BackgroundTask for SwitchPortSettingsManager {
                     // push the updates to both scrimlets
                     // if both scrimlets are down, bootstore updates aren't happening anyway
                     let mut one_succeeded = false;
-                    for (location, client) in &sled_agent_clients {
+                    for (location, client) in &scrimlet_sled_agent_clients {
                         if let Err(e) = client.write_network_bootstore_config(&desired_config).await {
                             error!(
                                 log,

nexus/src/app/instance_network.rs

@@ -408,15 +408,26 @@ pub(crate) async fn instance_ensure_dpd_config(
         return Err(e);
     }
 
-    notify_dendrite_nat_state(
+    // We should not bail out if there is an error while notifying dendrite.
+    // If there is an error communicating with one dendrite instance but the
+    // other is operational and we bail here, it will prevent users from starting
+    // an instance. Dendrite should still catch back up via a RPW if we fail to
+    // notify it here.
+    if let Err(e) = notify_dendrite_nat_state(
         datastore,
         log,
         resolver,
         opctx_alloc,
         Some(instance_id),
-        true,
     )
-    .await?;
+    .await
+    {
+        warn!(
+            log,
+            "error encountered when notifying dendrite";
+            "error" => %e
+        )
+    };
 
     Ok(nat_entries)
 }
@@ -559,7 +570,6 @@ pub(crate) async fn instance_delete_dpd_config(
         resolver,
         opctx_alloc,
         Some(instance_id),
-        false,
     )
     .await
 }
@@ -684,15 +694,7 @@ pub(crate) async fn delete_dpd_config_by_entry(
         },
     }
 
-    notify_dendrite_nat_state(
-        datastore,
-        log,
-        resolver,
-        opctx_alloc,
-        None,
-        false,
-    )
-    .await
+    notify_dendrite_nat_state(datastore, log, resolver, opctx_alloc, None).await
 }
 
 /// Soft-delete an individual external IP from the NAT RPW, without
@@ -724,30 +726,29 @@ async fn external_ip_delete_dpd_config_inner(
 /// Informs all available boundary switches that the set of NAT entries
 /// has changed.
 ///
-/// When `fail_fast` is set, this function will return on any error when
-/// acquiring a handle to a DPD client. Otherwise, it will attempt to notify
-/// all clients and then finally return the first error.
+/// It will attempt to notify all dpd daemons and then finally return the first error,
+/// if any errors were encountered.
 async fn notify_dendrite_nat_state(
     datastore: &DataStore,
     log: &slog::Logger,
     resolver: &internal_dns_resolver::Resolver,
     opctx_alloc: &OpContext,
     instance_id: Option<InstanceUuid>,
-    fail_fast: bool,
 ) -> Result<(), Error> {
     // Querying boundary switches also requires fleet access and the use of the
     // instance allocator context.
     let boundary_switches = boundary_switches(datastore, opctx_alloc).await?;
 
+    let clients = super::dpd_clients(resolver, log).await.map_err(|e| {
+        Error::internal_error(&format!("failed to get dpd clients: {e}"))
+    })?;
+
     let mut errors = vec![];
     for switch in &boundary_switches {
         debug!(log, "notifying dendrite of updates";
                     "instance_id" => ?instance_id,
                     "switch" => switch.to_string());
 
-        let clients = super::dpd_clients(resolver, log).await.map_err(|e| {
-            Error::internal_error(&format!("failed to get dpd clients: {e}"))
-        })?;
         let client_result = clients.get(switch).ok_or_else(|| {
             Error::internal_error(&format!(
                 "unable to find dendrite client for {switch}"
@@ -758,11 +759,7 @@ async fn notify_dendrite_nat_state(
             Ok(client) => client,
             Err(new_error) => {
                 errors.push(new_error);
-                if fail_fast {
-                    break;
-                } else {
-                    continue;
-                }
+                continue;
             }
         };
 

nexus/src/app/mod.rs

@@ -28,7 +28,6 @@ use nexus_mgs_updates::ArtifactCache;
 use nexus_mgs_updates::MgsUpdateDriver;
 use nexus_types::deployment::PendingMgsUpdates;
 use nexus_types::deployment::ReconfiguratorConfigParam;
-use omicron_common::address::DENDRITE_PORT;
 use omicron_common::address::MGD_PORT;
 use omicron_common::address::MGS_PORT;
 use omicron_common::api::external::ByteCount;
@@ -1131,37 +1130,83 @@ pub enum Unimpl {
     ProtectedLookup(Error),
 }
 
+/// Returns a mapping of clients for the Dendrite daemons of reachable switch zones.
+/// If we are unable to communicate with the switch zone and determine the mapping
+/// of SwitchLocation -> Zone Underlay Address, we omit an entry for that client.
 pub(crate) async fn dpd_clients(
     resolver: &internal_dns_resolver::Resolver,
     log: &slog::Logger,
 ) -> Result<HashMap<SwitchLocation, dpd_client::Client>, String> {
-    let mappings = switch_zone_address_mappings(resolver, log).await?;
-    let clients: HashMap<SwitchLocation, dpd_client::Client> = mappings
-        .iter()
-        .map(|(location, addr)| {
-            let port = DENDRITE_PORT;
+    let dpd_socketaddrs = match resolver
+        .lookup_all_socket_v6(ServiceName::Dendrite)
+        .await
+    {
+        Ok(addrs) => addrs,
+        Err(e) => {
+            error!(log, "failed to resolve addresses for Dendrite services"; "error" => %e);
+            return Err(e.to_string());
+        }
+    };
 
+    let clients: Vec<(SocketAddrV6, dpd_client::Client)> = dpd_socketaddrs
+        .iter()
+        .map(|socket_addr| {
             let client_state = dpd_client::ClientState {
                 tag: String::from("nexus"),
                 log: log.new(o!(
                     "component" => "DpdClient"
                 )),
             };
 
-            let dpd_client = dpd_client::Client::new(
-                &format!("http://[{addr}]:{port}"),
+            let client = dpd_client::Client::new(
+                &format!("http://{socket_addr}"),
                 client_state,
             );
-            (*location, dpd_client)
+
+            (*socket_addr, client)
         })
         .collect();
-    Ok(clients)
+
+    let mut mappings: HashMap<SwitchLocation, dpd_client::Client> =
+        HashMap::new();
+
+    for (addr, client) in clients {
+        let switch_slot = match client.switch_identifiers().await {
+            Ok(response) => response.slot,
+            Err(e) => {
+                error!(
+                    log,
+                    "failed to determine switch slot for dendrite";
+                    "error" => %e,
+                    "addr" => %addr,
+                );
+                continue;
+            }
+        };
+
+        let location = match switch_slot {
+            0 => SwitchLocation::Switch0,
+            1 => SwitchLocation::Switch1,
+            _ => {
+                warn!(log, "unexpected value for switch slot: {switch_slot}");
+                continue;
+            }
+        };
+
+        mappings.insert(location, client);
+    }
+
+    Ok(mappings)
 }
 
 // We currently ignore the rack_id argument here, as the shared
 // switch_zone_address_mappings function doesn't allow filtering on the rack ID.
 // Since we only have a single rack, this is OK for now.
 // TODO: https://github.com/oxidecomputer/omicron/issues/1276
+//
+/// Returns a mapping of clients for the LLDP daemons of reachable switch zones.
+/// If we are unable to communicate with the switch zone and determine the mapping
+/// of SwitchLocation -> Zone Underlay Address, we omit an entry for that client.
 pub(crate) async fn lldpd_clients(
     resolver: &internal_dns_resolver::Resolver,
     _rack_id: Uuid,
@@ -1183,35 +1228,29 @@ pub(crate) async fn lldpd_clients(
     Ok(clients)
 }
 
-// Look up Dendrite addresses in DNS then determine the switch location for
-// each address DNS returns.  If we fail to lookup ServiceName::Dendrite, we
-// return error, otherwise we keep looping until we can determine the switch
-// location of all addresses returned to us from the lookup.
+/// Look up Dendrite addresses in DNS then determine the switch location of
+/// any addresses we're able to resolve the SwitchLocation for. If a switch
+/// zone is down, the resolution process will fail and the entry will be
+/// missing from the result.
+///
+/// # Errors
+/// If we fail to resolve the ipv6 addresses of the Dendrite service we
+/// return an error
 async fn switch_zone_address_mappings(
     resolver: &internal_dns_resolver::Resolver,
     log: &slog::Logger,
 ) -> Result<HashMap<SwitchLocation, Ipv6Addr>, String> {
-    loop {
-        let switch_zone_addresses = match resolver
-            .lookup_all_ipv6(ServiceName::Dendrite)
-            .await
-        {
-            Ok(addrs) => addrs,
-            Err(e) => {
-                error!(log, "failed to resolve addresses for Dendrite services"; "error" => %e);
-                return Err(e.to_string());
-            }
-        };
-        match map_switch_zone_addrs(&log, switch_zone_addresses).await {
-            Ok(mappings) => {
-                return Ok(mappings);
-            }
-            Err(e) => {
-                warn!(log, "Failed to map switch zone addr: {e}, retrying");
-                tokio::time::sleep(std::time::Duration::from_secs(2)).await;
-            }
+    let switch_zone_addresses = match resolver
+        .lookup_all_ipv6(ServiceName::Dendrite)
+        .await
+    {
+        Ok(addrs) => addrs,
+        Err(e) => {
+            error!(log, "failed to resolve addresses for Dendrite services"; "error" => %e);
+            return Err(e.to_string());
         }
-    }
+    };
+    Ok(map_switch_zone_addrs(&log, switch_zone_addresses).await)
 }
 
 // TODO: #3596 Allow updating of Nexus from `handoff_to_nexus()`
@@ -1222,10 +1261,20 @@ async fn switch_zone_address_mappings(
 // via an API call. We probably will need to rethink how we're looking
 // up switch addresses as a whole, since how DNS is currently setup for
 // Dendrite is insufficient for what we need.
+//
+/// Query MGS in each switch zone to learn which switch slot is being managed by
+/// the services located on a given ipv6 address. This information can be used
+/// along with the well known port numbers to target a specific switch + service
+/// combination.
+///
+/// We return whatever we're able to successfully resolve. In the event of
+/// a communication timeout or other failure with MGS, the SwitchLocation -> Ipv6Addr
+/// mapping will be missing from the returned HashMap. Callers will need to inspect
+/// the contents to ensure what they expect to be there is actually there.
 async fn map_switch_zone_addrs(
     log: &Logger,
     switch_zone_addresses: Vec<Ipv6Addr>,
-) -> Result<HashMap<SwitchLocation, Ipv6Addr>, String> {
+) -> HashMap<SwitchLocation, Ipv6Addr> {
     use gateway_client::Client as MgsClient;
     info!(log, "Determining switch slots managed by switch zones");
     let mut switch_zone_addrs = HashMap::new();
@@ -1235,25 +1284,25 @@ async fn map_switch_zone_addrs(
             log.new(o!("component" => "MgsClient")),
         );
 
-        info!(log, "determining switch slot managed by dendrite zone"; "zone_address" => #?addr);
+        info!(log, "determining switch slot managed by switch zone"; "zone_address" => #?addr);
         let switch_slot = match mgs_client.sp_local_switch_id().await {
             Ok(switch) => {
                 info!(
                     log,
-                    "identified switch slot for dendrite zone";
+                    "identified switch slot for switch zone";
                     "slot" => #?switch,
                     "zone_address" => #?addr
                 );
                 switch.slot
             }
             Err(e) => {
-                warn!(
+                error!(
                     log,
-                    "failed to identify switch slot for dendrite";
+                    "failed to identify switch slot for switch zone";
                     "zone_address" => #?addr,
                     "reason" => #?e
                 );
-                return Err(e.to_string());
+                continue;
             }
         };
 
@@ -1272,12 +1321,14 @@ async fn map_switch_zone_addrs(
             }
         };
     }
+
     info!(
         log,
-        "completed mapping dendrite zones to switch slots";
+        "completed mapping switch zones to switch slots";
         "mappings" => #?switch_zone_addrs
     );
-    Ok(switch_zone_addrs)
+
+    switch_zone_addrs
 }
 
 /// Begin configuring an external HTTP client, returning a