From b7c3b5ce5521f8a7b56548ae52d2016057db1d8f Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 14 May 2026 11:04:43 -0400 Subject: [PATCH 1/3] scrimlet-reconcilers: Add dpd port reconciliation --- Cargo.lock | 1 + sled-agent/scrimlet-reconcilers/Cargo.toml | 1 + .../src/dpd_reconciler.rs | 39 +- .../src/dpd_reconciler/port_reconciler.rs | 635 ++++++++++++ .../dpd_reconciler/port_reconciler/tests.rs | 937 ++++++++++++++++++ sled-agent/scrimlet-reconcilers/src/lib.rs | 2 + .../versions/src/impls/early_networking.rs | 60 ++ .../versions/src/initial/early_networking.rs | 14 +- .../early_networking.rs | 14 +- 9 files changed, 1690 insertions(+), 13 deletions(-) create mode 100644 sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler.rs create mode 100644 sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs diff --git a/Cargo.lock b/Cargo.lock index 9edd61e5495..8e20b2ae33d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13707,6 +13707,7 @@ dependencies = [ "gateway-test-utils", "gateway-types", "httpmock", + "iddqd", "macaddr", "mg-admin-client", "omicron-common", diff --git a/sled-agent/scrimlet-reconcilers/Cargo.toml b/sled-agent/scrimlet-reconcilers/Cargo.toml index 6a6aa3e52a5..34e6992aba6 100644 --- a/sled-agent/scrimlet-reconcilers/Cargo.toml +++ b/sled-agent/scrimlet-reconcilers/Cargo.toml @@ -14,6 +14,7 @@ dpd-client.workspace = true futures.workspace = true gateway-client.workspace = true gateway-types.workspace = true +iddqd.workspace = true macaddr.workspace = true mg-admin-client.workspace = true omicron-common.workspace = true diff --git a/sled-agent/scrimlet-reconcilers/src/dpd_reconciler.rs b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler.rs index a7428fddcd6..445c9144604 100644 --- a/sled-agent/scrimlet-reconcilers/src/dpd_reconciler.rs +++ b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler.rs @@ -11,17 +11,22 @@ use crate::switch_zone_slot::ThisSledSwitchSlot; use dpd_client::Client; use sled_agent_types::system_networking::SystemNetworkingConfig; use slog::Logger; -use slog::info; use std::time::Duration; mod nat; +mod port_reconciler; pub use nat::DpdNatReconcilerStatus; pub use nat::DpdNatReconcilerStatusNatEntry; pub use nat::DpdNatReconcilerStatusNatEntryFailure; +pub use port_reconciler::DpdPortOperationFailure; +pub use port_reconciler::DpdPortReconcilerStatus; +use port_reconciler::PortReconciler; #[derive(Debug, Clone)] pub struct DpdReconcilerStatus { + /// Result of reconciling port settings + pub port_settings_status: DpdPortReconcilerStatus, /// Result of reconciling service zone NAT entries pub nat_status: DpdNatReconcilerStatus, } @@ -32,15 +37,18 @@ impl slog::KV for DpdReconcilerStatus { record: &slog::Record<'_>, serializer: &mut dyn slog::Serializer, ) -> slog::Result { - let Self { nat_status } = self; - nat_status.serialize(record, serializer) + let Self { port_settings_status, nat_status } = self; + port_settings_status.serialize(record, serializer)?; + nat_status.serialize(record, serializer)?; + Ok(()) } } #[derive(Debug)] pub(crate) struct DpdReconciler { client: Client, - _switch_slot: ThisSledSwitchSlot, + switch_slot: ThisSledSwitchSlot, + port_reconciler: PortReconciler, } impl Reconciler for DpdReconciler { @@ -54,7 +62,11 @@ impl Reconciler for DpdReconciler { switch_slot: ThisSledSwitchSlot, parent_log: &Logger, ) -> Self { - Self { client: mode.dpd_client(parent_log), _switch_slot: switch_slot } + Self { + client: mode.dpd_client(parent_log), + switch_slot, + port_reconciler: PortReconciler::default(), + } } async fn do_reconciliation( @@ -62,6 +74,16 @@ impl Reconciler for DpdReconciler { system_networking_config: &SystemNetworkingConfig, log: &Logger, ) -> Self::Status { + let port_settings_status = self + .port_reconciler + .reconcile( + &self.client, + &system_networking_config.rack_network_config, + self.switch_slot, + log, + ) + .await; + let nat_status = if let Some(nat_entries) = system_networking_config .blueprint_external_networking_config .as_ref() @@ -72,11 +94,6 @@ impl Reconciler for DpdReconciler { DpdNatReconcilerStatus::NoNatEntriesConfig }; - info!( - log, "dpd reconciliation completed"; - &nat_status, - ); - - DpdReconcilerStatus { nat_status } + DpdReconcilerStatus { port_settings_status, nat_status } } } diff --git a/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler.rs b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler.rs new file mode 100644 index 00000000000..8fde1e26c24 --- /dev/null +++ b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler.rs @@ -0,0 +1,635 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Reconciliation of QSFP port settings. + +use crate::switch_zone_slot::ThisSledSwitchSlot; +use daft::Diffable; +use dpd_client::Client; +use dpd_client::types::LinkCreate as DpdLinkCreate; +use dpd_client::types::LinkId as DpdLinkId; +use dpd_client::types::LinkSettings as DpdLinkSettings; +use dpd_client::types::PortFec as DpdPortFec; +use dpd_client::types::PortId as DpdPortId; +use dpd_client::types::PortSettings as DpdPortSettings; +use dpd_client::types::PortSpeed as DpdPortSpeed; +use dpd_client::types::Qsfp as DpdQsfp; +use dpd_client::types::TxEq as DpdTxEq; +use iddqd::IdOrdItem; +use iddqd::IdOrdMap; +use iddqd::id_ord_map; +use omicron_common::OMICRON_DPD_TAG; +use sled_agent_types::early_networking::LinkFec; +use sled_agent_types::early_networking::LinkSpeed; +use sled_agent_types::early_networking::PortConfig; +use sled_agent_types::early_networking::RackNetworkConfig; +use sled_agent_types::early_networking::TxEqConfig; +use sled_agent_types::early_networking::UplinkAddress; +use slog::Logger; +use slog::info; +use slog::warn; +use slog_error_chain::InlineErrorChain; +use std::collections::BTreeMap; +use std::collections::BTreeSet; +use std::collections::HashMap; +use std::net::IpAddr; + +type DpdClientError = dpd_client::Error; + +const DPD_TAG: Option<&'static str> = Some(OMICRON_DPD_TAG); + +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct DpdPortOperationFailure { + pub port_id: DpdQsfp, + pub error: String, +} + +/// Status of reconciling QSFP port settings with `dpd`. +#[derive(Debug, Clone)] +pub enum DpdPortReconcilerStatus { + /// Reconciliation failed while attempting to read the current settings from + /// `dpd`. + FailedReadingCurrentSettings(String), + + /// Reconciliation failed because the data prevented us from constructing a + /// plan - this should be impossible absent bugs. + FailedGeneratingPlan(String), + + /// Reconciliation completed successfully. + Success { + unchanged: BTreeSet, + cleared: BTreeSet, + applied: BTreeSet, + }, + + /// Reconciliation completed but had at least one failure. + PartialSuccess { + unchanged: BTreeSet, + cleared: BTreeSet, + clear_failures: Vec, + applied: BTreeSet, + apply_failures: Vec, + }, +} + +impl slog::KV for DpdPortReconcilerStatus { + fn serialize( + &self, + _record: &slog::Record<'_>, + serializer: &mut dyn slog::Serializer, + ) -> slog::Result { + let skipped_key = "port-reconciler-skipped"; + match self { + Self::FailedReadingCurrentSettings(reason) => { + serializer.emit_str(skipped_key.into(), reason) + } + Self::FailedGeneratingPlan(reason) => { + serializer.emit_str(skipped_key.into(), reason) + } + Self::Success { unchanged, cleared, applied } => { + // Only show a summary count; we have individual log statements + // for each clear/apply. + for (key, val) in [ + ("port-settings-unchanged", unchanged.len()), + ("port-settings-successfully-cleared", cleared.len()), + ("port-settings-failed-to-clear", 0), + ("port-settings-successfully-applied", applied.len()), + ("port-settings-failed-to-apply", 0), + ] { + serializer.emit_usize(key.into(), val)?; + } + Ok(()) + } + Self::PartialSuccess { + unchanged, + cleared, + clear_failures, + applied, + apply_failures, + } => { + // Only show a summary count; we have individual log statements + // for each clear/apply. + for (key, val) in [ + ("port-settings-unchanged", unchanged.len()), + ("port-settings-successfully-cleared", cleared.len()), + ("port-settings-failed-to-clear", clear_failures.len()), + ("port-settings-successfully-applied", applied.len()), + ("port-settings-failed-to-apply", apply_failures.len()), + ] { + serializer.emit_usize(key.into(), val)?; + } + Ok(()) + } + } + } +} + +#[derive(Default, Debug)] +pub(super) struct PortReconciler { + // The set of QSFP ports is a physical property; it never changes for a + // given switch, so we can cache this value once. The first time dpd + // successfully returns the list of ports, we save that value here and reuse + // it forever; it can never change. + cached_qsfp_ports: Option>, +} + +impl PortReconciler { + pub(super) async fn reconcile( + &mut self, + client: &Client, + desired_config: &RackNetworkConfig, + our_switch_slot: ThisSledSwitchSlot, + log: &Logger, + ) -> DpdPortReconcilerStatus { + let dpd_current_settings = match self + .dpd_get_current_settings(client, log) + .await + { + Ok(settings) => settings, + Err(err) => { + return DpdPortReconcilerStatus::FailedReadingCurrentSettings( + format!( + "failed to read current port settings from dpd: {}", + InlineErrorChain::new(&err), + ), + ); + } + }; + + let plan = match ReconciliationPlan::new( + dpd_current_settings, + desired_config, + our_switch_slot, + log, + ) { + Ok(plan) => plan, + Err(err) => { + // Ensure `err` is actually a string; if it changes to a proper + // error type, we need to use `InlineErrorChain` here instead. + let err: &str = &err; + return DpdPortReconcilerStatus::FailedGeneratingPlan(format!( + "failed to generate plan to apply port settings: {err}", + )); + } + }; + + apply_plan(client, plan, log).await + } + + async fn dpd_get_current_settings( + &mut self, + client: &Client, + log: &Logger, + ) -> Result, DpdClientError> { + let qsfp_ports = match self.cached_qsfp_ports.as_mut() { + Some(cached) => cached.as_slice(), + None => { + let ports = client + .port_list() + .await? + .into_inner() + .into_iter() + .filter_map(|port| match port { + // We're only responsible for applying settings to QSFP + // ports; any other kind of port cannot have settings + // populated in `RackNetworkConfig` and is + // internal-to-dpd. + DpdPortId::Internal(_) | DpdPortId::Rear(_) => None, + DpdPortId::Qsfp(qsfp) => Some(qsfp), + }) + .collect::>(); + info!( + log, "cached set of qsfp port IDs"; + "num-qsfp-ports" => ports.len(), + ); + self.cached_qsfp_ports.insert(ports).as_slice() + } + }; + + let mut config_by_port = BTreeMap::new(); + for port_id in qsfp_ports.iter().cloned() { + let settings = client + .port_settings_get(&DpdPortId::Qsfp(port_id.clone()), DPD_TAG) + .await? + .into_inner(); + // Check for empty ports and filter those out. + // + // TODO-performance We could consider caching "ports known to have + // no links", as that _shouldn't_ change unless we apply settings to + // a port. But "shouldn't" is doing a lot of work here, and caching + // has all the usual problems! For now, we'll just fetch all the + // port settings every time, and we can tune that down in the future + // if needed. + let are_port_settings_clear = { + let DpdPortSettings { links } = &settings; + links.is_empty() + }; + if !are_port_settings_clear { + config_by_port.insert(port_id, settings); + } + } + + Ok(config_by_port) + } +} + +/// Apply the contents of `plan` to dpd via `client`. +/// +/// This requires `plan.to_clear.len() + plan.to_apply.len()` independent +/// calls to `dpd`. We do not short circuit on failure: we'll always attempt to +/// make every call required. This may not be the right choice, but some +/// arguments in favor: +/// +/// * In practice we expect the number of calls here to be small. On startup we +/// expect 1-32 `to_apply` calls (one for each configured uplink), and for +/// every reconciliation attempt after that we expect 0-1 (either no changes, +/// or a single port has had its settings changed; it's possible we'll see +/// multiple ports change at once, but at most 32). +/// * We always want to report the status of every step described by `plan`, and +/// implementing stop-on-first-failure means we'd need to record a "didn't +/// attempt because of an earlier failure" status for some steps. That's +/// doable but annoying. +async fn apply_plan( + client: &Client, + plan: ReconciliationPlan, + log: &Logger, +) -> DpdPortReconcilerStatus { + let ReconciliationPlan { unchanged, to_clear, to_apply } = plan; + + let mut cleared = BTreeSet::new(); + let mut clear_failures = Vec::new(); + for port_id in to_clear { + match client + .port_settings_clear(&DpdPortId::Qsfp(port_id.clone()), DPD_TAG) + .await + { + Ok(_) => { + info!( + log, "successfully cleared settings for port"; + "port_id" => port_id.to_string(), + ); + cleared.insert(port_id); + } + Err(err) => { + let err = InlineErrorChain::new(&err); + warn!( + log, "failed to clear port settings"; + "port_id" => port_id.to_string(), + &err, + ); + clear_failures.push(DpdPortOperationFailure { + port_id, + error: err.to_string(), + }); + } + } + } + + let mut applied = BTreeSet::new(); + let mut apply_failures = Vec::new(); + for (port_id, settings) in to_apply { + match client + .port_settings_apply( + &DpdPortId::Qsfp(port_id.clone()), + DPD_TAG, + &settings, + ) + .await + { + Ok(_) => { + info!( + log, "successfully applied settings for port"; + "port_id" => port_id.to_string(), + ); + applied.insert(port_id); + } + Err(err) => { + let err = InlineErrorChain::new(&err); + warn!( + log, "failed to apply port settings"; + "port_id" => port_id.to_string(), + &err, + ); + apply_failures.push(DpdPortOperationFailure { + port_id, + error: err.to_string(), + }); + } + } + } + + if clear_failures.is_empty() && apply_failures.is_empty() { + DpdPortReconcilerStatus::Success { unchanged, cleared, applied } + } else { + DpdPortReconcilerStatus::PartialSuccess { + unchanged, + cleared, + clear_failures, + applied, + apply_failures, + } + } +} + +#[derive(Debug, PartialEq)] +struct ReconciliationPlan { + // Set of ports whose settings are already correct in `dpd`. + unchanged: BTreeSet, + + // Set of ports that have settings in `dpd` but not in our desired config; + // these should be cleared. + to_clear: BTreeSet, + + // Set of ports whose settings in `dpd` don't match our desired config or + // don't exist at all; these need to be applied. + to_apply: BTreeMap, +} + +impl ReconciliationPlan { + fn new( + dpd_current_settings: BTreeMap, + config: &RackNetworkConfig, + our_switch_slot: ThisSledSwitchSlot, + log: &Logger, + ) -> Result { + // Helper for all the places in this method where we have to convert a + // string back into a `DpdQsfp`. We never expect this to fail in + // practice, but want to report the source of the bad port name if it + // happens. + fn parse_port_id( + port_id: &str, + source: &str, + ) -> Result { + port_id.parse().map_err(|err| { + format!( + "invalid port ID `{port_id}` in {source}: {}", + InlineErrorChain::new(&err) + ) + }) + } + + // Convert dpd settings into a diffable form. + let dpd_current_settings = dpd_current_settings + .into_iter() + .map(DiffablePortSettings::try_from) + .collect::, _>>() + .map_err(|err| InlineErrorChain::new(&err).to_string())?; + + // Convert desired config into a diffable form. + let desired_settings = config + .ports + .iter() + .filter(|p| p.switch == our_switch_slot) + .map(DiffablePortSettings::from) + .collect::>(); + + let id_ord_map::Diff { common, added, removed } = + dpd_current_settings.diff(&desired_settings); + + // Any entries removed are ports that have settings in dpd but not + // `config`; we need to clear them. + let mut to_clear = removed + .into_iter() + .map(|item| parse_port_id(&item.port_id, "dpd")) + .collect::, _>>()?; + + // Any entries added are ports that have settings in `config` but not + // dpd; we need to add them. + let mut to_apply = added + .into_iter() + .map(|p| { + let port_id = parse_port_id(&p.port_id, "rack network config")?; + Ok::<_, String>((port_id, DpdPortSettings::from(p))) + }) + .collect::, _>>()?; + let mut unchanged = BTreeSet::new(); + + // For any entries in common (i.e., the key exists in both dpd and + // `config`), we have to check whether any values changed. For every + // leaf in common, we'll either add its port ID to `unchanged` (if the + // values match) or we'll add the desired value to `to_apply`. + for leaf in common { + if leaf.is_unchanged() { + unchanged.insert(parse_port_id( + leaf.key(), + "rack network config AND dpd", + )?); + } else { + let port_id = + parse_port_id(leaf.key(), "rack network config AND dpd")?; + + // Workaround for dpd limitation: the speed and fec of a link + // require us to remove it first. Any other change can be + // applied in place. TODO: link to dendrite issue / PR + if leaf.before().speed != leaf.after().speed + || leaf.before().fec != leaf.after().fec + { + to_clear.insert(port_id.clone()); + } + + // `common` is a map of unique keys that must be distinct from + // the `added` keys used to seed `to_apply`, so these inserts + // are guaranteed to all be unique. + to_apply.insert(port_id, (*leaf.after()).into()); + } + } + + info!( + log, + "generated dpd port settings reconciliation plan"; + "ports_unchanged" => unchanged.len(), + "ports_to_clear" => to_clear.len(), + "ports_to_apply" => to_apply.len(), + ); + + Ok(Self { unchanged, to_clear, to_apply }) + } +} + +// We convert both `RackNetworkConfig`'s port settings and the `DpdPortSettings` +// we read from `dpd` into this type to compute the diff. +#[derive(Debug, Clone, PartialEq, Eq, daft::Diffable)] +struct DiffablePortSettings { + port_id: String, + autoneg: bool, + tx_eq: Option, + fec: Option, + speed: LinkSpeed, + addrs: BTreeSet, +} + +impl IdOrdItem for DiffablePortSettings { + type Key<'a> = &'a str; + + fn key(&self) -> Self::Key<'_> { + &self.port_id + } + + iddqd::id_upcast!(); +} + +impl From<&'_ PortConfig> for DiffablePortSettings { + fn from(port: &'_ PortConfig) -> Self { + Self { + port_id: port.port.clone(), + autoneg: port.autoneg, + tx_eq: port.tx_eq, + fec: port.uplink_port_fec, + speed: port.uplink_port_speed, + addrs: port + .addresses + .iter() + .filter_map(|a| { + // TODO we're discarding any vlan_id - is that okay? + match a.address { + UplinkAddress::AddrConf => None, + UplinkAddress::Static { ip_net } => { + // TODO We're discarding the `ip_net.prefix()` here + // and only using the IP address; at some point we + // probably need to give the full CIDR to dendrite? + Some(ip_net.addr()) + } + } + }) + .collect(), + } + } +} + +#[derive(Debug, thiserror::Error)] +#[error( + "expected exactly 1 link per port in dpd, but got {nlinks} on port {port}" +)] +struct UnexpectedLinkCount { + nlinks: usize, + port: String, +} + +impl TryFrom<(DpdQsfp, DpdPortSettings)> for DiffablePortSettings { + type Error = UnexpectedLinkCount; + + fn try_from( + value: (DpdQsfp, DpdPortSettings), + ) -> Result { + let (port_id, DpdPortSettings { links }) = value; + + // We only expect to be constructed if there's exactly one link + // configured: + // + // * 0 links is an empty port; those should be filtered out by + // `dpd_get_current_settings()` + // * 2 or more links cannot be represented in `RackNetworkConfig` today, + // so it shouldn't be possible for `dpd` to report that on any port + if links.len() != 1 { + return Err(UnexpectedLinkCount { + nlinks: links.len(), + port: port_id.to_string(), + }); + } + // We just confirmed there's exactly one link; take ownership of it. + let sole_link = links.into_values().next().unwrap(); + + let tx_eq = sole_link.params.tx_eq.map(|t| TxEqConfig { + main: t.main, + post1: t.post1, + post2: t.post2, + pre1: t.pre1, + pre2: t.pre2, + }); + + let fec = sole_link.params.fec.map(|f| match f { + DpdPortFec::Firecode => LinkFec::Firecode, + DpdPortFec::None => LinkFec::None, + DpdPortFec::Rs => LinkFec::Rs, + }); + + let speed = match sole_link.params.speed { + DpdPortSpeed::Speed0G => LinkSpeed::Speed0G, + DpdPortSpeed::Speed1G => LinkSpeed::Speed1G, + DpdPortSpeed::Speed10G => LinkSpeed::Speed10G, + DpdPortSpeed::Speed25G => LinkSpeed::Speed25G, + DpdPortSpeed::Speed40G => LinkSpeed::Speed40G, + DpdPortSpeed::Speed50G => LinkSpeed::Speed50G, + DpdPortSpeed::Speed100G => LinkSpeed::Speed100G, + DpdPortSpeed::Speed200G => LinkSpeed::Speed200G, + DpdPortSpeed::Speed400G => LinkSpeed::Speed400G, + }; + + // dont consider link local addresses in change computation + let addrs = sole_link + .addrs + .into_iter() + .filter(|ip| match ip { + IpAddr::V6(ip) if ip.is_unicast_link_local() => false, + _ => true, + }) + .collect(); + + Ok(Self { + port_id: port_id.to_string(), + autoneg: sole_link.params.autoneg, + tx_eq, + fec, + speed, + addrs, + }) + } +} + +impl From<&'_ DiffablePortSettings> for DpdPortSettings { + fn from(port: &DiffablePortSettings) -> Self { + let autoneg = port.autoneg; + let addrs = port.addrs.iter().copied().collect(); + let kr = false; //NOTE: kr does not apply to user configurable links. + + let fec = port.fec.map(|f| match f { + LinkFec::Firecode => DpdPortFec::Firecode, + LinkFec::None => DpdPortFec::None, + LinkFec::Rs => DpdPortFec::Rs, + }); + + let speed = match port.speed { + LinkSpeed::Speed0G => DpdPortSpeed::Speed0G, + LinkSpeed::Speed1G => DpdPortSpeed::Speed1G, + LinkSpeed::Speed10G => DpdPortSpeed::Speed10G, + LinkSpeed::Speed25G => DpdPortSpeed::Speed25G, + LinkSpeed::Speed40G => DpdPortSpeed::Speed40G, + LinkSpeed::Speed50G => DpdPortSpeed::Speed50G, + LinkSpeed::Speed100G => DpdPortSpeed::Speed100G, + LinkSpeed::Speed200G => DpdPortSpeed::Speed200G, + LinkSpeed::Speed400G => DpdPortSpeed::Speed400G, + }; + + let tx_eq = port.tx_eq.map(|t| DpdTxEq { + main: t.main, + post1: t.post1, + post2: t.post2, + pre1: t.pre1, + pre2: t.pre2, + }); + + // TODO breakouts? + let mut links = HashMap::with_capacity(1); + let link_id = DpdLinkId(0); + links.insert( + link_id.to_string(), + DpdLinkSettings { + addrs, + params: DpdLinkCreate { + autoneg, + fec, + kr, + lane: Some(link_id), + speed, + tx_eq, + }, + }, + ); + + DpdPortSettings { links } + } +} + +#[cfg(test)] +mod tests; diff --git a/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs new file mode 100644 index 00000000000..3859890649e --- /dev/null +++ b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs @@ -0,0 +1,937 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use super::*; +use dpd_client::types::LinkCreate as DpdLinkCreate; +use dpd_client::types::LinkId as DpdLinkId; +use dpd_client::types::LinkSettings as DpdLinkSettings; +use dpd_client::types::PortFec as DpdPortFec; +use dpd_client::types::PortSpeed as DpdPortSpeed; +use gateway_messages::SpPort; +use omicron_test_utils::dev; +use proptest::prelude::proptest as proptest_macro; +use proptest::strategy::Strategy; +use sled_agent_types::early_networking::LinkFec; +use sled_agent_types::early_networking::LinkSpeed; +use sled_agent_types::early_networking::PortConfig; +use sled_agent_types::early_networking::RackNetworkConfig; +use sled_agent_types::early_networking::SwitchSlot; +use sled_agent_types::early_networking::UplinkAddress; +use sled_agent_types::early_networking::UplinkAddressConfig; +use sled_agent_types::early_networking::UplinkIpNet; +use std::collections::BTreeMap; +use std::collections::BTreeSet; +use std::collections::HashMap; +use std::net::IpAddr; +use test_strategy::Arbitrary; +use test_strategy::proptest; +use tokio::sync::Mutex; +use tokio::task::block_in_place; + +use crate::switch_zone_slot::ThisSledSwitchSlot; + +/// Build a minimal `PortConfig` for a single port. +fn port_config( + port: &DpdQsfp, + switch: SwitchSlot, + speed: LinkSpeed, + fec: Option, + autoneg: bool, + addrs: &[UplinkIpNet], +) -> PortConfig { + PortConfig { + routes: Vec::new(), + addresses: addrs + .iter() + .copied() + .map(|ip_net| UplinkAddressConfig { + address: UplinkAddress::Static { ip_net }, + vlan_id: None, + }) + .collect(), + switch, + port: port.to_string(), + uplink_port_speed: speed, + uplink_port_fec: fec, + bgp_peers: Vec::new(), + autoneg, + lldp: None, + tx_eq: None, + } +} + +/// Build `DpdPortSettings` with a single link (the only layout we support). +fn dpd_port_settings( + speed: DpdPortSpeed, + fec: Option, + autoneg: bool, + addrs: Vec, +) -> DpdPortSettings { + let mut links = HashMap::new(); + let link_id = DpdLinkId(0); + links.insert( + link_id.to_string(), + DpdLinkSettings { + addrs, + params: DpdLinkCreate { + autoneg, + fec, + kr: false, + lane: Some(link_id), + speed, + tx_eq: None, + }, + }, + ); + DpdPortSettings { links } +} + +fn rack_config(ports: Vec) -> RackNetworkConfig { + RackNetworkConfig { + rack_subnet: "fd00::/48".parse().unwrap(), + infra_ip_first: "10.0.0.1".parse().unwrap(), + infra_ip_last: "10.0.0.100".parse().unwrap(), + ports, + bgp: Vec::new(), + bfd: Vec::new(), + } +} + +#[test] +fn plan_all_unchanged() { + let logctx = dev::test_setup_log("plan_all_unchanged"); + let log = &logctx.log; + + let qsfp0: DpdQsfp = "qsfp0".parse().unwrap(); + let addr: IpAddr = "10.0.0.1".parse().unwrap(); + let ip_net: UplinkIpNet = format!("{addr}/24").parse().unwrap(); + + // Desired config: one port on Switch0. + let config = rack_config(vec![port_config( + &qsfp0, + SwitchSlot::Switch0, + LinkSpeed::Speed100G, + Some(LinkFec::Rs), + true, + &[ip_net], + )]); + + // Current dpd state matches exactly. + let dpd_current = BTreeMap::from([( + qsfp0.clone(), + dpd_port_settings( + DpdPortSpeed::Speed100G, + Some(DpdPortFec::Rs), + true, + vec![addr], + ), + )]); + + let plan = ReconciliationPlan::new( + dpd_current, + &config, + ThisSledSwitchSlot::TEST_FAKE, + log, + ) + .expect("plan should succeed"); + + assert_eq!(plan.unchanged, BTreeSet::from([qsfp0])); + assert!(plan.to_clear.is_empty()); + assert!(plan.to_apply.is_empty()); + + logctx.cleanup_successful(); +} + +#[test] +fn plan_create_all() { + let logctx = dev::test_setup_log("plan_create_all"); + let log = &logctx.log; + + let qsfp0: DpdQsfp = "qsfp0".parse().unwrap(); + let qsfp1: DpdQsfp = "qsfp1".parse().unwrap(); + + // Desired config: two ports on Switch0. + let config = rack_config(vec![ + port_config( + &qsfp0, + SwitchSlot::Switch0, + LinkSpeed::Speed100G, + Some(LinkFec::Rs), + true, + &["10.0.0.1/24".parse().unwrap()], + ), + port_config( + &qsfp1, + SwitchSlot::Switch0, + LinkSpeed::Speed25G, + None, + false, + &["10.0.0.2/24".parse().unwrap()], + ), + ]); + + // dpd has no settings at all. + let dpd_current = BTreeMap::new(); + + let plan = ReconciliationPlan::new( + dpd_current, + &config, + ThisSledSwitchSlot::TEST_FAKE, + log, + ) + .expect("plan should succeed"); + + assert!(plan.unchanged.is_empty()); + assert!(plan.to_clear.is_empty()); + assert_eq!(plan.to_apply.keys().collect::>(), vec![&qsfp0, &qsfp1]); + + logctx.cleanup_successful(); +} + +#[test] +fn plan_clear_all() { + let logctx = dev::test_setup_log("plan_clear_all"); + let log = &logctx.log; + + let qsfp0: DpdQsfp = "qsfp0".parse().unwrap(); + let qsfp1: DpdQsfp = "qsfp1".parse().unwrap(); + + // Desired config: no ports. + let config = rack_config(vec![]); + + // dpd has settings for two ports. + let dpd_current = BTreeMap::from([ + ( + qsfp0.clone(), + dpd_port_settings( + DpdPortSpeed::Speed100G, + Some(DpdPortFec::Rs), + true, + vec!["10.0.0.1".parse().unwrap()], + ), + ), + ( + qsfp1.clone(), + dpd_port_settings( + DpdPortSpeed::Speed25G, + None, + false, + vec!["10.0.0.2".parse().unwrap()], + ), + ), + ]); + + let plan = ReconciliationPlan::new( + dpd_current, + &config, + ThisSledSwitchSlot::TEST_FAKE, + log, + ) + .expect("plan should succeed"); + + assert!(plan.unchanged.is_empty()); + assert_eq!(plan.to_clear, BTreeSet::from([qsfp0, qsfp1])); + assert!(plan.to_apply.is_empty()); + + logctx.cleanup_successful(); +} + +#[test] +fn plan_mix() { + let logctx = dev::test_setup_log("plan_mix"); + let log = &logctx.log; + + let qsfp0: DpdQsfp = "qsfp0".parse().unwrap(); + let qsfp1: DpdQsfp = "qsfp1".parse().unwrap(); + let qsfp2: DpdQsfp = "qsfp2".parse().unwrap(); + let qsfp3: DpdQsfp = "qsfp3".parse().unwrap(); + let ip0: IpAddr = "10.0.0.1".parse().unwrap(); + let ip1: IpAddr = "10.0.0.2".parse().unwrap(); + let ip2: IpAddr = "10.0.0.3".parse().unwrap(); + let ip3: IpAddr = "10.0.0.4".parse().unwrap(); + let ip_net0: UplinkIpNet = format!("{ip0}/24").parse().unwrap(); + let ip_net1: UplinkIpNet = format!("{ip1}/24").parse().unwrap(); + let ip_net3: UplinkIpNet = format!("{ip3}/24").parse().unwrap(); + + // Desired config: qsfp0 unchanged, qsfp1 changed speed, qsfp3 new. + // qsfp2 should be cleared (exists in dpd but not in desired config). + let config = rack_config(vec![ + port_config( + &qsfp0, + SwitchSlot::Switch0, + LinkSpeed::Speed100G, + Some(LinkFec::Rs), + true, + &[ip_net0], + ), + port_config( + &qsfp1, + SwitchSlot::Switch0, + LinkSpeed::Speed100G, + None, + false, + &[ip_net1], + ), + port_config( + &qsfp3, + SwitchSlot::Switch0, + LinkSpeed::Speed50G, + None, + true, + &[ip_net3], + ), + ]); + + let dpd_current = BTreeMap::from([ + ( + qsfp0.clone(), + dpd_port_settings( + DpdPortSpeed::Speed100G, + Some(DpdPortFec::Rs), + true, + vec![ip0], + ), + ), + ( + qsfp1.clone(), + dpd_port_settings(DpdPortSpeed::Speed25G, None, false, vec![ip1]), + ), + ( + qsfp2.clone(), + dpd_port_settings(DpdPortSpeed::Speed10G, None, false, vec![ip2]), + ), + ]); + + let plan = ReconciliationPlan::new( + dpd_current, + &config, + ThisSledSwitchSlot::TEST_FAKE, + log, + ) + .expect("plan should succeed"); + + // qsfp0: unchanged + assert_eq!(plan.unchanged, BTreeSet::from([qsfp0])); + // qsfp2: in dpd but not desired + assert_eq!(plan.to_clear, BTreeSet::from([qsfp2])); + // qsfp1: changed, qsfp3: new + assert_eq!( + plan.to_apply.keys().collect::>(), + BTreeSet::from([&qsfp1, &qsfp3]), + ); + + logctx.cleanup_successful(); +} + +#[test] +fn plan_filters_other_switch_slot() { + let logctx = dev::test_setup_log("plan_filters_other_switch_slot"); + let log = &logctx.log; + + let qsfp0: DpdQsfp = "qsfp0".parse().unwrap(); + let qsfp1: DpdQsfp = "qsfp1".parse().unwrap(); + + // Desired config has ports on both Switch0 and Switch1. Our reconciler is + // Switch0 (TEST_FAKE), so Switch1 ports should be ignored entirely. + let config = rack_config(vec![ + port_config( + &qsfp0, + SwitchSlot::Switch0, + LinkSpeed::Speed100G, + None, + true, + &["10.0.0.1/24".parse().unwrap()], + ), + port_config( + &qsfp1, + SwitchSlot::Switch1, // different switch + LinkSpeed::Speed25G, + None, + false, + &["10.0.0.2/24".parse().unwrap()], + ), + ]); + + // dpd is empty. + let dpd_current = BTreeMap::new(); + + let plan = ReconciliationPlan::new( + dpd_current, + &config, + ThisSledSwitchSlot::TEST_FAKE, + log, + ) + .expect("plan should succeed"); + + // Only qsfp0 should appear; qsfp1 is on the wrong switch. + assert!(plan.unchanged.is_empty()); + assert!(plan.to_clear.is_empty()); + assert_eq!(plan.to_apply.keys().collect::>(), vec![&qsfp0],); + + logctx.cleanup_successful(); +} + +#[test] +fn plan_link_local_addrs_ignored_from_dpd() { + let logctx = dev::test_setup_log("plan_link_local_addrs_ignored_from_dpd"); + let log = &logctx.log; + + let qsfp0: DpdQsfp = "qsfp0".parse().unwrap(); + let addr: IpAddr = "10.0.0.1".parse().unwrap(); + let ip_net: UplinkIpNet = format!("{addr}/24").parse().unwrap(); + let link_local: IpAddr = "fe80::1".parse().unwrap(); + + // Desired config: one port with one address. + let config = rack_config(vec![port_config( + &qsfp0, + SwitchSlot::Switch0, + LinkSpeed::Speed100G, + None, + true, + &[ip_net], + )]); + + // dpd has the same config but also reports a link-local address. + // The reconciler should ignore the link-local and see no diff. + let dpd_current = BTreeMap::from([( + qsfp0.clone(), + dpd_port_settings( + DpdPortSpeed::Speed100G, + None, + true, + vec![addr, link_local], + ), + )]); + + let plan = ReconciliationPlan::new( + dpd_current, + &config, + ThisSledSwitchSlot::TEST_FAKE, + log, + ) + .expect("plan should succeed"); + + assert_eq!(plan.unchanged, BTreeSet::from([qsfp0])); + assert!(plan.to_clear.is_empty()); + assert!(plan.to_apply.is_empty()); + + logctx.cleanup_successful(); +} + +#[test] +fn plan_rejects_multi_link_dpd_port() { + let logctx = dev::test_setup_log("plan_rejects_multi_link_dpd_port"); + let log = &logctx.log; + + let qsfp0: DpdQsfp = "qsfp0".parse().unwrap(); + let config = rack_config(vec![]); + + // Build DpdPortSettings with two links -- this is not representable in + // RackNetworkConfig and should cause plan generation to fail. + let mut links = HashMap::new(); + let link0 = DpdLinkId(0); + let link1 = DpdLinkId(1); + let link_params = DpdLinkCreate { + autoneg: true, + fec: None, + kr: false, + lane: Some(link0), + speed: DpdPortSpeed::Speed100G, + tx_eq: None, + }; + links.insert( + link0.to_string(), + DpdLinkSettings { + addrs: vec!["10.0.0.1".parse().unwrap()], + params: link_params.clone(), + }, + ); + links.insert( + link1.to_string(), + DpdLinkSettings { + addrs: vec!["10.0.0.2".parse().unwrap()], + params: link_params, + }, + ); + let dpd_current = BTreeMap::from([(qsfp0, DpdPortSettings { links })]); + + let err = ReconciliationPlan::new( + dpd_current, + &config, + ThisSledSwitchSlot::TEST_FAKE, + log, + ) + .expect_err("plan should fail with multi-link port"); + + assert!(err.contains("expected exactly 1 link"), "unexpected error: {err}",); + + logctx.cleanup_successful(); +} + +#[test] +fn plan_created_settings_round_trip_correctly() { + let logctx = + dev::test_setup_log("plan_created_settings_round_trip_correctly"); + let log = &logctx.log; + + let qsfp5: DpdQsfp = "qsfp5".parse().unwrap(); + let ip1: IpAddr = "10.0.0.1".parse().unwrap(); + let ip2: IpAddr = "10.0.0.2".parse().unwrap(); + let ip_net1: UplinkIpNet = format!("{ip1}/24").parse().unwrap(); + let ip_net2: UplinkIpNet = format!("{ip2}/24").parse().unwrap(); + + // Desired config with various settings populated. + let config = rack_config(vec![port_config( + &qsfp5, + SwitchSlot::Switch0, + LinkSpeed::Speed100G, + Some(LinkFec::Rs), + true, + &[ip_net1, ip_net2], + )]); + + // dpd is empty, so qsfp5 should be in `to_apply`. + let dpd_current = BTreeMap::new(); + + let plan = ReconciliationPlan::new( + dpd_current, + &config, + ThisSledSwitchSlot::TEST_FAKE, + log, + ) + .expect("plan should succeed"); + + let created = plan.to_apply.get(&qsfp5).expect("qsfp5 should exist"); + + // Verify the DpdPortSettings matches what we'd expect. + assert_eq!(created.links.len(), 1); + let link = created.links.values().next().unwrap(); + assert_eq!(link.params.speed, DpdPortSpeed::Speed100G); + assert_eq!(link.params.fec, Some(DpdPortFec::Rs)); + assert!(link.params.autoneg); + assert_eq!( + link.addrs.iter().copied().collect::>(), + BTreeSet::from([ip1, ip2]), + ); + + logctx.cleanup_successful(); +} + +#[derive(Debug, Clone, PartialEq, Eq, Arbitrary)] +struct ArbitraryPortSettings { + autoneg: bool, + tx_eq: Option, + fec: Option, + speed: LinkSpeed, + #[strategy(proptest::collection::btree_set( + proptest::arbitrary::any::(), + 0..=4, + ))] + addrs: BTreeSet, +} + +impl ArbitraryPortSettings { + fn to_dpd_settings(&self, port_id: &PortId) -> DpdPortSettings { + DpdPortSettings::from(&DiffablePortSettings { + port_id: port_id.0.clone(), + autoneg: self.autoneg, + tx_eq: self.tx_eq, + fec: self.fec, + speed: self.speed, + addrs: self + .addrs + .iter() + .filter_map(|addr| match addr.address { + UplinkAddress::AddrConf => None, + UplinkAddress::Static { ip_net } => Some(ip_net.addr()), + }) + .collect(), + }) + } +} + +#[derive(Debug, Clone, Arbitrary)] +enum SwitchPortSettingsTestInput { + // port only exists in dpd + DpdOnly(ArbitraryPortSettings), + + // port only exists in desired config for switch 0 or switch 1 + DesiredSwitch0(ArbitraryPortSettings), + DesiredSwitch1(ArbitraryPortSettings), + + // port exists in dpd and desired switch 0 with the same settings + DpdAndSwitch0Same(ArbitraryPortSettings), + + // port exists in dpd and desired switch 0, but the settings may be + // different (usually will be, but randomly could be the same still!) + DpdAndSwitch0Changed { + dpd: ArbitraryPortSettings, + switch0: ArbitraryPortSettings, + }, +} + +// We cap the arbitrary port generation here to at most `qsfp30`, because the +// `dpd` stub binary has a bug that makes `qsfp31` unusable: +// . +#[derive(Debug, Clone, Arbitrary, PartialEq, Eq, PartialOrd, Ord)] +struct PortId(#[strategy((0..31).prop_map(|n| format!("qsfp{n}")))] String); + +impl PortId { + fn to_dpd(&self) -> DpdQsfp { + self.0.parse().unwrap() + } +} + +#[derive(Debug, Clone, Arbitrary)] +struct TestInput { + ports: BTreeMap, +} + +impl TestInput { + // Map all our arbitrary inputs into just the map that should exist in dpd + // prior to reconciliation. + fn initial_dpd_settings(&self) -> BTreeMap { + self.ports + .iter() + .filter_map(|(port_id, input)| match input { + SwitchPortSettingsTestInput::DpdOnly(dpd) + | SwitchPortSettingsTestInput::DpdAndSwitch0Same(dpd) + | SwitchPortSettingsTestInput::DpdAndSwitch0Changed { + dpd, + .. + } => Some((port_id.to_dpd(), dpd.to_dpd_settings(port_id))), + SwitchPortSettingsTestInput::DesiredSwitch0(_) + | SwitchPortSettingsTestInput::DesiredSwitch1(_) => None, + }) + .collect() + } + + // Map all our arbitrary inputs into the desired `RackNetworkConfig`. + fn desired_rack_config(&self) -> RackNetworkConfig { + let switch0 = + self.ports.iter().filter_map(|(port_id, input)| match input { + SwitchPortSettingsTestInput::DesiredSwitch0(switch0) + | SwitchPortSettingsTestInput::DpdAndSwitch0Same(switch0) + | SwitchPortSettingsTestInput::DpdAndSwitch0Changed { + switch0, + .. + } => Some(diffable_to_port_config( + SwitchSlot::Switch0, + port_id, + switch0, + )), + SwitchPortSettingsTestInput::DpdOnly(_) + | SwitchPortSettingsTestInput::DesiredSwitch1(_) => None, + }); + let switch1 = + self.ports.iter().filter_map(|(port_id, input)| match input { + SwitchPortSettingsTestInput::DesiredSwitch1(switch1) => { + Some(diffable_to_port_config( + SwitchSlot::Switch1, + port_id, + switch1, + )) + } + SwitchPortSettingsTestInput::DpdOnly(_) + | SwitchPortSettingsTestInput::DesiredSwitch0(_) + | SwitchPortSettingsTestInput::DpdAndSwitch0Same(_) + | SwitchPortSettingsTestInput::DpdAndSwitch0Changed { + .. + } => None, + }); + rack_config(switch0.chain(switch1).collect()) + } + + // Build the set of expected unchanged port names based on our arbitrary + // inputs. + fn expected_unchanged(&self) -> BTreeSet { + self.ports + .iter() + .filter_map(|(port_id, input)| match input { + SwitchPortSettingsTestInput::DpdOnly(_) + | SwitchPortSettingsTestInput::DesiredSwitch0(_) + | SwitchPortSettingsTestInput::DesiredSwitch1(_) => None, + SwitchPortSettingsTestInput::DpdAndSwitch0Same(_) => { + Some(port_id.to_dpd()) + } + SwitchPortSettingsTestInput::DpdAndSwitch0Changed { + dpd, + switch0, + } => { + // We have to convert to dpd settings for this comparison; + // if addrconf addresses are involved, they need to be + // filtered out. + if dpd.to_dpd_settings(port_id) + == switch0.to_dpd_settings(port_id) + { + Some(port_id.to_dpd()) + } else { + None + } + } + }) + .collect() + } + + // Build the set of ports we expect to clear in dpd. + fn expected_to_clear(&self) -> BTreeSet { + self.ports + .iter() + .filter_map(|(port_id, input)| match input { + SwitchPortSettingsTestInput::DpdOnly(_) => { + Some(port_id.to_dpd()) + } + SwitchPortSettingsTestInput::DpdAndSwitch0Changed { + dpd, + switch0, + } => { + // Changes to speed/fec require a clear before applying. + if dpd.speed != switch0.speed || dpd.fec != switch0.fec { + Some(port_id.to_dpd()) + } else { + None + } + } + SwitchPortSettingsTestInput::DesiredSwitch0(_) + | SwitchPortSettingsTestInput::DesiredSwitch1(_) + | SwitchPortSettingsTestInput::DpdAndSwitch0Same(_) => None, + }) + .collect() + } + + // Build the set of ports we expect to apply because they're new or changed. + fn expected_to_apply(&self) -> BTreeMap { + self.ports + .iter() + .filter_map(|(port_id, input)| match input { + SwitchPortSettingsTestInput::DpdOnly(_) + | SwitchPortSettingsTestInput::DesiredSwitch1(_) + | SwitchPortSettingsTestInput::DpdAndSwitch0Same(_) => None, + SwitchPortSettingsTestInput::DesiredSwitch0(switch0) => { + Some((port_id.to_dpd(), switch0.to_dpd_settings(port_id))) + } + SwitchPortSettingsTestInput::DpdAndSwitch0Changed { + dpd, + switch0, + } => { + // We have to convert to dpd settings for this comparison; + // if addrconf addresses are involved, they need to be + // filtered out. + if dpd.to_dpd_settings(port_id) + != switch0.to_dpd_settings(port_id) + { + Some(( + port_id.to_dpd(), + switch0.to_dpd_settings(port_id), + )) + } else { + None + } + } + }) + .collect() + } + + async fn validate_post_reconciliation( + &self, + client: &dpd_client::Client, + ) -> anyhow::Result<()> { + // Fetch all settings in dpd. + let mut dpd_ports_with_settings = BTreeMap::new(); + for port_id in 0..32 { + let port_id = format!("qsfp{port_id}").parse::().unwrap(); + let settings = client + .port_settings_get(&DpdPortId::Qsfp(port_id.clone()), DPD_TAG) + .await? + .into_inner(); + if !settings.links.is_empty() { + dpd_ports_with_settings.insert(port_id, settings); + } + } + + // We expect to see all the settings we applied plus all unchanged + // ports. + let mut expected_settings = self + .ports + .iter() + .filter_map(|(port_id, input)| match input { + SwitchPortSettingsTestInput::DpdOnly(_) + | SwitchPortSettingsTestInput::DesiredSwitch1(_) => None, + SwitchPortSettingsTestInput::DesiredSwitch0(switch0) + | SwitchPortSettingsTestInput::DpdAndSwitch0Same(switch0) + | SwitchPortSettingsTestInput::DpdAndSwitch0Changed { + switch0, + .. + } => Some((port_id.to_dpd(), switch0.to_dpd_settings(port_id))), + }) + .collect::>(); + + // To compare the state in dpd with what we expect, we need to ignore + // ordering in the vec of addresses. We'll sort them all. + for links in dpd_ports_with_settings + .values_mut() + .chain(expected_settings.values_mut()) + .flat_map(|settings| settings.links.values_mut()) + { + links.addrs.sort_unstable(); + } + + assert_eq!( + dpd_ports_with_settings, expected_settings, + "unexpected settings in dpd after reconciliation" + ); + + Ok(()) + } +} + +fn diffable_to_port_config( + switch: SwitchSlot, + port_id: &PortId, + config: &ArbitraryPortSettings, +) -> PortConfig { + let ArbitraryPortSettings { autoneg, tx_eq, fec, speed, addrs } = config; + PortConfig { + addresses: addrs.into_iter().copied().collect(), + switch, + port: port_id.0.clone(), + uplink_port_speed: *speed, + uplink_port_fec: *fec, + autoneg: *autoneg, + tx_eq: *tx_eq, + + // Fields that aren't involved in dpd configuration + routes: Vec::new(), + bgp_peers: Vec::new(), + lldp: None, + } +} + +#[proptest] +fn proptest_plan(input: TestInput) { + let logctx = dev::test_setup_log("proptest_plan"); + let log = &logctx.log; + + let plan = ReconciliationPlan::new( + input.initial_dpd_settings(), + &input.desired_rack_config(), + ThisSledSwitchSlot::TEST_FAKE, + log, + ) + .expect("plan should succeed"); + + let ReconciliationPlan { unchanged, to_clear, to_apply } = plan; + + assert_eq!(unchanged, input.expected_unchanged(), "incorrect unchanged"); + assert_eq!(to_clear, input.expected_to_clear(), "incorrect to_clear"); + assert_eq!(to_apply, input.expected_to_apply(), "incorrect to_apply"); + + logctx.cleanup_successful(); +} + +#[tokio::test(flavor = "multi_thread")] +async fn proptest_full_reconciliation() { + let logctx = dev::test_setup_log("proptest_full_reconciliation"); + let mgsctx = gateway_test_utils::setup::test_setup( + "proptest_full_reconciliation", + SpPort::One, + ) + .await; + let mut dpdctx = dev::dendrite::DendriteInstance::start( + 0, + None, + Some(mgsctx.address().into()), + ) + .await + .expect("started dendrite"); + let client = dpd_client::Client::new( + &format!("http://{}", dpdctx.address()), + dpd_client::ClientState { + tag: OMICRON_DPD_TAG.to_owned(), + log: logctx.log.clone(), + }, + ); + let rt = tokio::runtime::Handle::current(); + + let reconciler = Mutex::new(PortReconciler::default()); + let one_test_invocation = async |input: TestInput| { + // Clear all ports from a previous proptest invocation. + for port_id in 0..32 { + let port_id = format!("qsfp{port_id}").parse().unwrap(); + client + .port_settings_clear(&DpdPortId::Qsfp(port_id), DPD_TAG) + .await + .expect("cleared port"); + } + + // Apply all initial settings. + for (port_id, settings) in input.initial_dpd_settings() { + client + .port_settings_apply( + &DpdPortId::Qsfp(dbg!(port_id)), + DPD_TAG, + &settings, + ) + .await + .expect("applied initial settings"); + } + + // Perform reconciliation. + let status = reconciler + .lock() + .await + .reconcile( + &client, + &input.desired_rack_config(), + ThisSledSwitchSlot::TEST_FAKE, + &logctx.log, + ) + .await; + + match status { + DpdPortReconcilerStatus::FailedReadingCurrentSettings(_) + | DpdPortReconcilerStatus::FailedGeneratingPlan(_) + | DpdPortReconcilerStatus::PartialSuccess { .. } => { + panic!("unexpected reconciler status: {status:?}"); + } + DpdPortReconcilerStatus::Success { + unchanged, + cleared, + applied, + } => { + assert_eq!( + unchanged, + input.expected_unchanged(), + "incorrect unchanged" + ); + assert_eq!( + cleared, + input.expected_to_clear(), + "incorrect cleared" + ); + let expected_applied = input + .expected_to_apply() + .keys() + .cloned() + .collect::>(); + assert_eq!(applied, expected_applied, "incorrect applied"); + + input + .validate_post_reconciliation(&client) + .await + .expect("validated post reconciliation settings"); + } + } + }; + + proptest_macro!(|(input: TestInput)| { + // Do a little dance to call our async `one_test_invocation` within the + // non-async `proptest_macro!()` context. + block_in_place(|| rt.block_on(one_test_invocation(input))); + }); + + dpdctx.cleanup().await.expect("dpd cleanup succeeded"); + mgsctx.teardown().await; + logctx.cleanup_successful(); +} diff --git a/sled-agent/scrimlet-reconcilers/src/lib.rs b/sled-agent/scrimlet-reconcilers/src/lib.rs index 2810d70ca3c..ecee043e8ee 100644 --- a/sled-agent/scrimlet-reconcilers/src/lib.rs +++ b/sled-agent/scrimlet-reconcilers/src/lib.rs @@ -52,6 +52,8 @@ mod uplinkd_reconciler; pub use dpd_reconciler::DpdNatReconcilerStatusNatEntry; pub use dpd_reconciler::DpdNatReconcilerStatusNatEntryFailure; +pub use dpd_reconciler::DpdPortOperationFailure; +pub use dpd_reconciler::DpdPortReconcilerStatus; pub use dpd_reconciler::DpdReconcilerStatus; pub use handle::ScrimletReconcilers; pub use handle::ScrimletReconcilersMode; diff --git a/sled-agent/types/versions/src/impls/early_networking.rs b/sled-agent/types/versions/src/impls/early_networking.rs index 83f5152a4e3..121a19f47d3 100644 --- a/sled-agent/types/versions/src/impls/early_networking.rs +++ b/sled-agent/types/versions/src/impls/early_networking.rs @@ -104,6 +104,66 @@ impl std::fmt::Display for UplinkIpNet { } } +#[cfg(any(test, feature = "testing"))] +impl proptest::arbitrary::Arbitrary for UplinkIpNet { + type Parameters = (); + type Strategy = proptest::prelude::BoxedStrategy; + + fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { + use proptest::prelude::*; + use std::net::Ipv4Addr; + use test_strategy::Arbitrary; + + // `UplinkIpNet::try_from()` rejects quite a lot of IP addresses that + // the default `Arbitrary` implementation likes to produce: loopback + // addresses, unspecified addresses, ipv4-mapped-ipv6, etc. `ValidIpNet` + // has restrictions on the first octet (ipv4) or segment (ipv6) that + // causes it to produce IPs that are valid `UplinkIpNet`s. We also skip + // plenty of valid `UplinkIpNet`s, but should provide sufficient + // coverage for any downstream tests. + #[derive(Debug, Clone, Copy, Arbitrary)] + enum ValidIpNet { + V4 { + #[strategy(1_u8..127)] + octet0: u8, + other_octets: [u8; 3], + #[strategy(0_u8..32)] + prefix: u8, + }, + V6 { + #[strategy(1_u16..0xfe00)] + segment0: u16, + other_segments: [u16; 7], + #[strategy(0_u8..128)] + prefix: u8, + }, + } + + any::() + .prop_map(|arb| { + let ipnet = match arb { + ValidIpNet::V4 { octet0, other_octets, prefix } => { + let mut octets = [0; 4]; + octets[0] = octet0; + octets[1..].copy_from_slice(&other_octets); + let ip = Ipv4Addr::from_octets(octets); + IpNet::new(ip.into(), prefix).expect("valid prefix") + } + ValidIpNet::V6 { segment0, other_segments, prefix } => { + let mut segments = [0; 8]; + segments[0] = segment0; + segments[1..].copy_from_slice(&other_segments); + let ip = Ipv6Addr::from_segments(segments); + IpNet::new(ip.into(), prefix).expect("valid prefix") + } + }; + UplinkIpNet::try_from(ipnet) + .expect("ValidIpNet produces valid UplinkIpNets") + }) + .boxed() + } +} + impl std::fmt::Display for RouterPeerIpAddr { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { self.0.fmt(f) diff --git a/sled-agent/types/versions/src/initial/early_networking.rs b/sled-agent/types/versions/src/initial/early_networking.rs index cd539d51424..c709a238074 100644 --- a/sled-agent/types/versions/src/initial/early_networking.rs +++ b/sled-agent/types/versions/src/initial/early_networking.rs @@ -245,6 +245,7 @@ pub struct LldpPortConfig { #[derive( Clone, Copy, Debug, Deserialize, Serialize, PartialEq, Eq, Hash, JsonSchema, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct TxEqConfig { /// Pre-cursor tap1 pub pre1: Option, @@ -307,9 +308,19 @@ pub enum SwitchSlot { /// The speed of a link. #[derive( - Copy, Clone, Debug, Deserialize, Serialize, PartialEq, Eq, JsonSchema, Hash, + Copy, + Clone, + Debug, + Deserialize, + Serialize, + PartialEq, + Eq, + JsonSchema, + Hash, + daft::Diffable, )] #[serde(rename_all = "snake_case")] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub enum LinkSpeed { /// Zero gigabits per second. #[serde(alias = "0G")] @@ -345,6 +356,7 @@ pub enum LinkSpeed { Copy, Clone, Debug, Deserialize, Serialize, PartialEq, Eq, JsonSchema, Hash, )] #[serde(rename_all = "snake_case")] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub enum LinkFec { /// Firecode forward error correction. Firecode, diff --git a/sled-agent/types/versions/src/stronger_bgp_unnumbered_types/early_networking.rs b/sled-agent/types/versions/src/stronger_bgp_unnumbered_types/early_networking.rs index 709f3d9fe0b..b0bc87873af 100644 --- a/sled-agent/types/versions/src/stronger_bgp_unnumbered_types/early_networking.rs +++ b/sled-agent/types/versions/src/stronger_bgp_unnumbered_types/early_networking.rs @@ -69,6 +69,7 @@ pub enum InvalidIpAddrError { Ord, )] #[serde(tag = "type", rename_all = "snake_case")] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub enum UplinkAddress { // `#[serde(rename)]` this to `addrconf` so when this shows up in // config-rss.toml development files, it's not phrased as `addr_conf`. This @@ -297,8 +298,19 @@ impl TryFrom for RouterPeerIpAddr { } #[derive( - Clone, Copy, Debug, Deserialize, Serialize, PartialEq, Eq, JsonSchema, Hash, + Clone, + Copy, + Debug, + Deserialize, + Serialize, + PartialEq, + Eq, + JsonSchema, + Hash, + PartialOrd, + Ord, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct UplinkAddressConfig { /// The address to be used on the uplink. pub address: UplinkAddress, From fb698e4eed08fdc0a7ac0703773f8a34787a62d2 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 18 May 2026 11:17:33 -0400 Subject: [PATCH 2/3] test fix --- .../src/dpd_reconciler/port_reconciler/tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs index 3859890649e..7c3cc42f00d 100644 --- a/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs +++ b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs @@ -313,8 +313,9 @@ fn plan_mix() { // qsfp0: unchanged assert_eq!(plan.unchanged, BTreeSet::from([qsfp0])); + // qsfp1: speed changed, so we have to remove it before applying // qsfp2: in dpd but not desired - assert_eq!(plan.to_clear, BTreeSet::from([qsfp2])); + assert_eq!(plan.to_clear, BTreeSet::from([qsfp1.clone(), qsfp2])); // qsfp1: changed, qsfp3: new assert_eq!( plan.to_apply.keys().collect::>(), From 8f9772b537b25eb7b8c65bfbc4b6371c45760f87 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 19 May 2026 14:56:04 -0400 Subject: [PATCH 3/3] remove stray dbg! --- .../src/dpd_reconciler/port_reconciler/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs index 7c3cc42f00d..fa9eb4171f1 100644 --- a/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs +++ b/sled-agent/scrimlet-reconcilers/src/dpd_reconciler/port_reconciler/tests.rs @@ -870,7 +870,7 @@ async fn proptest_full_reconciliation() { for (port_id, settings) in input.initial_dpd_settings() { client .port_settings_apply( - &DpdPortId::Qsfp(dbg!(port_id)), + &DpdPortId::Qsfp(port_id), DPD_TAG, &settings, )