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,25 @@ 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.
+    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, NAT entry creation may be delayed";
+            "error" => %e
+        )
+    };
 
     Ok(nat_entries)
 }
@@ -559,7 +569,6 @@ pub(crate) async fn instance_delete_dpd_config(
         resolver,
         opctx_alloc,
         Some(instance_id),
-        false,
     )
     .await
 }
@@ -684,15 +693,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,16 +725,14 @@ 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.
@@ -758,11 +757,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

@@ -1106,6 +1106,9 @@ 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,
@@ -1137,6 +1140,10 @@ pub(crate) async fn dpd_clients(
 // 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,
@@ -1158,35 +1165,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()`
@@ -1197,10 +1198,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
+/// 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 be 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();
@@ -1210,25 +1221,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;
             }
         };
 
@@ -1247,12 +1258,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