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

This PR addresses two issues that impacts users in circumstances where a
switch zone is unavailable:

1. When resolving which switch slot is managed by which switch zone, we
would continuously retry whenever there was a communication error. This
causes RPWs to stop running / Sagas to get stuck whenever a switch zone
becomes unavailable but still has entries in DNS.

2. In the `dpd_ensure` node of the `instance_start` saga, we were
bailing out if we encountered any errors while notifying dpd of nat
changes. This means that in the event of one of our switch zones being
unavailable we would no longer allow users to start instances, which is
probably a bit too strict.

This PR makes the following adjustments to the switch slot resolution
behavior:

* Log an error instead of a warning whenever a DNS entry exists for a
switch zone service but communication fails
* Return the mappings of the Slot -> Address for the switch zones that
we were successfully able to resolve
* Let the caller decide whether or not they should retry or proceed.
After checking all of the current call sites, it appears that the logic
already accommodates scenarios where switch zone mappings are missing.

This PR makes the following changes to the behavior to the NAT
configuration steps for instance related sagas:
* Do not bail if we fail to notify a dendrite service of NAT changes.
Log a warning and rely on the NAT RPW to catch us back up whenever the
switch zone becomes available again. Still return a `Result` from the
`notify` function so that callers can decide if they want to make a
subsequent decision based on the error.

This PR makes the following changes to the behavior of NAT garbage
collection:
* Pause GC (exit early) if we detect that both switches cannot be
reached

Before now, due to a configuration issue, the instant start saga tests
weren't actually driving ensuring that NAT configurations were
eventually landing on the dendrite instances. It also turns out that
Nexus in the `TestContext` could not actually communicate with dendrite
because the port values were hard coded and our resolution logic relied
on comparing the different addresses for MGS (and all mgs instances
listen on localhost in testing). This PR introduces the following
changes:

* Use an updated version of Dendrite that allows us to provide a MGS
`SocketAddr` on startup so it can use a simulated MGS to learn switch
slot information
* Adjust the `TestContext` Builder logic to provide MGS information to
dendrite
* Have Nexus ask dendrite for its switch slot information instead of
having Nexus attempt to resolve it via MGS
* Change the test configuration to create a NIC for the instance so we
actually exercise the NAT creation logic in the `dpd_ensure` saga node
* Introduce a test that ensures we still land configurations on the
remaining switch even when one of our switches are unavailable

Related
---
#8206
#6896
#8999

Depends On
---
oxidecomputer/dendrite#117

---------

Co-authored-by: David Pacheco <[email protected]>

Author: internet-diglett

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;
@@ -261,9 +260,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);
@@ -277,8 +274,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;
             }
         };