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

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

@@ -6,16 +6,16 @@
 //! Responsible for cleaning up soft deleted entries once they
 //! have been propagated to running dpd instances.
 
-use crate::app::switch_zone_address_mappings;
+use crate::app::dpd_clients;
 
-use super::networking::build_dpd_clients;
 use crate::app::background::BackgroundTask;
 use chrono::{Duration, Utc};
 use futures::FutureExt;
 use futures::future::BoxFuture;
 use internal_dns_resolver::Resolver;
 use nexus_db_queries::context::OpContext;
 use nexus_db_queries::db::DataStore;
+use omicron_common::api::internal::shared::SwitchLocation;
 use serde_json::json;
 use std::sync::Arc;
 
@@ -64,8 +64,8 @@ impl BackgroundTask for Ipv4NatGarbageCollector {
                 }
             };
 
-            let mappings = match
-                switch_zone_address_mappings(&self.resolver, log).await
+            let dpd_clients = match
+                dpd_clients(&self.resolver, log).await
             {
                 Ok(mappings) => mappings,
                 Err(e) => {
@@ -83,9 +83,15 @@ impl BackgroundTask for Ipv4NatGarbageCollector {
                 }
             };
 
-            let dpd_clients = build_dpd_clients(&mappings, log);
+            for location in [SwitchLocation::Switch0, SwitchLocation::Switch1] {
+                if !dpd_clients.contains_key(&location) {
+                    let message = format!("dendrite for {location} is unavailable, cannot perform nat cleanup");
+                    error!(log, "{message}");
+                    return json!({"error": message});
+                }
+            }
 
-            for (_location, client) in dpd_clients {
+            for client in dpd_clients.values() {
                 let response = client.ipv4_nat_generation().await;
                 match response {
                     Ok(gen) => min_gen = std::cmp::min(min_gen, *gen),

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

@@ -8,7 +8,6 @@ use dpd_client::types::{
 };
 use nexus_db_model::{SwitchLinkFec, SwitchLinkSpeed};
 use nexus_db_queries::db;
-use omicron_common::address::DENDRITE_PORT;
 use omicron_common::{address::MGD_PORT, api::external::SwitchLocation};
 use std::{collections::HashMap, net::SocketAddrV6};
 
@@ -30,32 +29,6 @@ pub(crate) fn build_mgd_clients(
     clients.into_iter().collect::<HashMap<_, _>>()
 }
 
-pub(crate) fn build_dpd_clients(
-    mappings: &HashMap<SwitchLocation, std::net::Ipv6Addr>,
-    log: &slog::Logger,
-) -> HashMap<SwitchLocation, dpd_client::Client> {
-    let dpd_clients: HashMap<SwitchLocation, dpd_client::Client> = mappings
-        .iter()
-        .map(|(location, addr)| {
-            let port = DENDRITE_PORT;
-
-            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}"),
-                client_state,
-            );
-            (*location, dpd_client)
-        })
-        .collect();
-    dpd_clients
-}
-
 pub(crate) fn api_to_dpd_port_settings(
     settings: &SwitchPortSettingsCombinedResult,
 ) -> Result<PortSettings, String> {

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

@@ -5,9 +5,8 @@
 //! Background task for detecting changes to service zone locations and
 //! updating the NAT rpw table accordingly
 
-use crate::app::switch_zone_address_mappings;
+use crate::app::dpd_clients;
 
-use super::networking::build_dpd_clients;
 use crate::app::background::BackgroundTask;
 use anyhow::Context;
 use futures::FutureExt;
@@ -279,9 +278,7 @@ impl BackgroundTask for ServiceZoneNatTracker {
             // notify dpd if we've added any new records
             if result > 0 {
 
-                let mappings = match
-                    switch_zone_address_mappings(&self.resolver, log).await
-                {
+                let dpd_clients = match dpd_clients(&self.resolver, log).await {
                     Ok(mappings) => mappings,
                     Err(e) => {
                         error!(log, "failed to resolve addresses for Dendrite services"; "error" => %e);
@@ -295,8 +292,6 @@ impl BackgroundTask for ServiceZoneNatTracker {
                     },
                 };
 
-                let dpd_clients = build_dpd_clients(&mappings, log);
-
                 for (_location, client) in dpd_clients {
                     if let Err(e) = client.ipv4_nat_trigger_update().await {
                         error!(

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

@@ -7,9 +7,9 @@
 
 use crate::app::{
     background::tasks::networking::{
-        api_to_dpd_port_settings, build_dpd_clients, build_mgd_clients,
+        api_to_dpd_port_settings, build_mgd_clients,
     },
-    switch_zone_address_mappings,
+    dpd_clients, switch_zone_address_mappings,
 };
 use oxnet::Ipv4Net;
 use slog::o;
@@ -328,19 +328,29 @@ impl BackgroundTask for SwitchPortSettingsManager {
                     Err(e) => {
                         error!(
                             log,
-                            "failed to resolve addresses for Dendrite services";
+                            "failed to resolve addresses for switch services";
                             "error" => %e);
                         continue;
                     },
                 };
 
                 // 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
-                let dpd_clients = build_dpd_clients(&mappings, &log);
+                let dpd_clients = match
+                    dpd_clients(&self.resolver, &log).await
+                {
+                    Ok(mappings) => mappings,
+                    Err(e) => {
+                        error!(
+                            log,
+                            "failed to resolve addresses for Dendrite";
+                            "error" => %e);
+                        continue;
+                    },
+                };
 
                 // TODO https://github.com/oxidecomputer/omicron/issues/5201
                 // build mgd clients
@@ -455,7 +465,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 +854,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 +1296,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;
             }
         };