diff --git a/asic/src/chaos/mod.rs b/asic/src/chaos/mod.rs index e5d58831..d7204c63 100644 --- a/asic/src/chaos/mod.rs +++ b/asic/src/chaos/mod.rs @@ -8,7 +8,7 @@ use rand::random; use serde::{Deserialize, Serialize}; use slog::Logger; use std::collections::HashMap; -use std::sync::Mutex; +use std::sync::{Mutex, OnceLock}; use tokio::sync::mpsc; #[cfg(feature = "multicast")] @@ -344,6 +344,7 @@ pub struct Handle { ports: Mutex>, config: AsicConfig, log: Logger, + updates: OnceLock>, } impl Handle { @@ -353,6 +354,7 @@ impl Handle { ports: Mutex::new(HashMap::new()), config: config.clone(), log: log.clone(), + updates: OnceLock::new(), }) } /// Chaos ASICs always report as a model. @@ -494,6 +496,14 @@ impl AsicOps for Handle { unfurl!(self, port_enable_set); let mut ports = self.ports.lock().unwrap(); get_port_mut(&mut ports, port_hdl)?.enabled = val; + + if let Some(chan) = self.updates.get() { + let _ = chan.send(PortUpdate::Enable { + asic_port_id: port_hdl.connector.as_u16(), + enabled: val, + }); + }; + Ok(()) } @@ -579,9 +589,12 @@ impl AsicOps for Handle { fn register_port_update_handler( &self, - _tx_channel: mpsc::UnboundedSender, + tx_channel: mpsc::UnboundedSender, ) -> AsicResult<()> { unfurl!(self, register_port_update_handler); + if self.updates.set(tx_channel).is_err() { + return Err(AsicError::Exists); + } Ok(()) } diff --git a/dpd-client/tests/chaos_tests/port_settings.rs b/dpd-client/tests/chaos_tests/port_settings.rs index 3873689f..0386467f 100644 --- a/dpd-client/tests/chaos_tests/port_settings.rs +++ b/dpd-client/tests/chaos_tests/port_settings.rs @@ -13,7 +13,7 @@ use crate::chaos_tests::harness; use crate::chaos_tests::util::HttpResponseCheck; use crate::chaos_tests::util::IpRng; -use anyhow::bail; +use anyhow::{Context, bail}; use asic::chaos::{AsicConfig, Chaos, TableChaos}; use asic::table_chaos; use common::table::TableType; @@ -38,6 +38,10 @@ const TESTING_RADIX: usize = 33; const RETRY_INTERVAL: Duration = Duration::from_millis(200); const RETRY_MAX: Duration = Duration::from_secs(5); +// It might be a DPD wedge. It might be unbelievable +// RNG misfortune. Regardless, it's time to move on. +const LONG_ENOUGH: Duration = Duration::from_secs(90); + /// A `LinkCreate` config with common defaults. const LINK_CREATE: LinkCreate = LinkCreate { lane: None, @@ -49,6 +53,9 @@ const LINK_CREATE: LinkCreate = LinkCreate { allow_ddm_traffic: false, }; +const TAG1: &str = "chaos1"; +const TAG2: &str = "chaos2"; + #[cfg(test)] mod retry { use std::future::Future; @@ -519,8 +526,92 @@ fn random_port_settings() -> PortSettings { } } -const TAG1: &str = "chaos1"; -const TAG2: &str = "chaos2"; +/// A simplified version of txn_sweep that ensures `port_settings_*` +/// functions can succeed after partial failures. +#[tokio::test] +async fn settings_eventually_reconcile() -> anyhow::Result<()> { + let mut apply = 0; + let mut clear = 0; + let mut get = 0; + + // Limit the test duration so a failure doesn't run indefinitely in CI. + // + // This is mathematically capable of flaking due to RNG, but the + // timeout should be high enough that it's safe for CI. + let status = tokio::time::timeout(LONG_ENOUGH, async { + self::settings_eventually_reconcile_unbounded( + &mut apply, &mut clear, &mut get, + ) + .await + }) + .await + .context("Timed out waiting for successful reconciliation"); + + println!( + " +Reconciliation retries: + - port_settings_apply: {apply} + - port_settings_clear: {clear} + - port_settings_get: {get} +" + ); + + status??; + Ok(()) +} + +async fn settings_eventually_reconcile_unbounded( + apply_ct: &mut usize, + clear_ct: &mut usize, + get_ct: &mut usize, +) -> anyhow::Result<()> { + let config = AsicConfig::uniform_set(TESTING_RADIX, 0.4); + let (_guard, client) = + harness::init_harness("settings_eventually_reconcile", &config); + let mut rng = IpRng::new(1046); + + let port_id: PortId = "qsfp0".parse()?; + let link_id = LinkId(0); + + let settings = PortSettings { + links: [( + link_id.to_string(), + LinkSettings { + params: LINK_CREATE, + addrs: vec![rng.unique_ipv4().into(), rng.unique_ipv6().into()], + }, + )] + .into_iter() + .collect(), + }; + + for _ in 0..3 { + while client + .port_settings_apply(&port_id, Some(TAG1), &settings) + .await + .is_err() + { + *apply_ct += 1; + self::slow_down().await; + } + + while client.port_settings_clear(&port_id, Some(TAG1)).await.is_err() { + *clear_ct += 1; + self::slow_down().await; + } + + while !client + .port_settings_get(&port_id, Some(TAG1)) + .await + .is_ok_and(|s| s.links.is_empty()) + { + *get_ct += 1; + self::slow_down().await; + } + } + + Ok(()) +} /// Verifies tagged port_settings_apply actions don't affect /// resources from other tags. @@ -883,6 +974,202 @@ async fn apply_fails_on_tag_conflict() -> anyhow::Result<()> { Ok(()) } +/// A pathological sequence of table errors in dpd must +/// not poison future valid operations. +/// +/// This test port_settings_applies two addresses. The IPv6 +/// table op fails, and then the IPv4 table op in the rollback +/// fails. After that rollback failure, the problematic IPv4 +/// table slot should still be usable. +#[tokio::test] +async fn partial_failures_are_recoverable() -> anyhow::Result<()> { + let conf = AsicConfig { + radix: TESTING_RADIX, + table_entry_add: table_chaos!((TableType::PortAddrIpv6, 1.0)), + table_entry_del: table_chaos!((TableType::PortAddrIpv4, 1.0)), + ..Default::default() + }; + + let (_guard, client) = + harness::init_harness("partial_failures_are_recoverable", &conf); + let mut rng = IpRng::new(731); + let port_id: PortId = "qsfp0".parse()?; + let link_id = LinkId(0); + + let addrs = TestAddrs::new( + &mut rng, + TAG1.to_string(), + &client, + port_id.clone(), + link_id, + ); + + let apply_err = addrs + .apply_addrs() + .await + .expect_err("Apply should fail because IPv6 table ops fail"); + assert!( + self::is_rollback_error(&apply_err), + "IPv4 addr couldn't be rolled back from table" + ); + + // Not sure this is the best behavior between apply and clear amid rollback + // failures, but this test at least ensures we can reclaim the entry later. + let empty = client.port_settings_clear(&port_id, Some(TAG1)).await?; + assert!( + empty.links.is_empty(), + "Unsuccessful apply was rolled back. The IPv4 entry is still stuck in the table because rollback partially failed, but clear doesn't know about that." + ); + + let mut v4_only = addrs.settings(); + for link in v4_only.links.values_mut() { + link.addrs.retain(|a| a.is_ipv4()); + } + + let settings = client + .port_settings_apply(&port_id, Some(TAG2), &v4_only) + .await + .context("Apply should succeed because we can at least overwrite the IPv4 table entry")?; + + let registered_v4 = settings + .links + .values() + .any(|link| link.addrs.contains(&addrs.v4_entry.addr.into())); + assert!(registered_v4, "Address was reclaimed"); + + Ok(()) +} + +/// A flaky table write must not poison link initialization. +#[tokio::test] +#[cfg(feature = "multicast")] +async fn link_init_recovers() -> anyhow::Result<()> { + tokio::time::timeout(LONG_ENOUGH, async { + self::link_init_recovers_unbounded().await + }) + .await + .context("Test timed out. DPD is probably wedged due to a bug.")? +} + +async fn link_init_recovers_unbounded() -> anyhow::Result<()> { + let conf = AsicConfig { + radix: TESTING_RADIX, + table_entry_add: table_chaos!((TableType::McastEgressPortMapping, 0.8)), + ..Default::default() + }; + + let (_guard, client) = harness::init_harness("link_init_recovers", &conf); + let port_id: PortId = "qsfp0".parse()?; + + let link_id = + client.link_create(&port_id, &LINK_CREATE).await?.into_inner(); + + while !client.link_enabled_get(&port_id, &link_id).await?.into_inner() { + // This pokes the reconciler and thus speeds up the test. + client.link_enabled_set(&port_id, &link_id, true).await?; + self::slow_down().await; + } + + Ok(()) +} + +/// One does not simply double-register a static address on loopback. +/// +/// This tests the order where static registration wins. +#[tokio::test] +async fn static_addrs_are_isolated() -> anyhow::Result<()> { + let no_failures = AsicConfig::uniform_set(TESTING_RADIX, 0.); + let (_guard, client) = + harness::init_harness("static_addrs_are_isolated", &no_failures); + let port_id: PortId = "qsfp0".parse()?; + let mut rng = IpRng::new(2238); + + let link_id = + client.link_create(&port_id, &LINK_CREATE).await?.into_inner(); + + let tag1 = + TestAddrs::new(&mut rng, TAG1.to_string(), &client, port_id, link_id); + + tag1.create_addrs().await?; + + client + .loopback_ipv4_create(&tag1.v4_entry) + .await + .expect_err("This static address IPv4 is already registered"); + + client + .loopback_ipv6_create(&tag1.v6_entry) + .await + .expect_err("This static address IPv6 is already registered"); + + tag1.verify_addrs_exist(Verify::Exhaustive).await?; + + Ok(()) +} + +/// Reverse of [`addrs_are_isolated`]. +/// +/// This tests the order where loopback registration wins. +#[tokio::test] +async fn loopback_addrs_are_isolated() -> anyhow::Result<()> { + let no_failures = AsicConfig::uniform_set(TESTING_RADIX, 0.); + let (_guard, client) = + harness::init_harness("loopback_addrs_are_isolated", &no_failures); + let port_id: PortId = "qsfp0".parse()?; + let mut rng = IpRng::new(2250); + + let link_id = + client.link_create(&port_id, &LINK_CREATE).await?.into_inner(); + + let tag1 = TestAddrs::new( + &mut rng, + TAG1.to_string(), + &client, + port_id.clone(), + link_id, + ); + + client.loopback_ipv4_create(&tag1.v4_entry).await?; + client.loopback_ipv6_create(&tag1.v6_entry).await?; + + client + .link_ipv4_create(&port_id, &link_id, &tag1.v4_entry) + .await + .expect_err("Address already exists on IPv4 loopback"); + client + .link_ipv6_create(&port_id, &link_id, &tag1.v6_entry) + .await + .expect_err("Address already exists on IPv6 loopback"); + + tag1.apply_addrs().await.expect("Addrs collide and cannot be created"); + + let v4_list = client.loopback_ipv4_list().await?; + assert_eq!( + &v4_list.into_inner(), + std::slice::from_ref(&tag1.v4_entry), + "Loopback should have IPv4 addr" + ); + + let v6_list = client.loopback_ipv6_list().await?; + assert_eq!( + &v6_list.into_inner(), + std::slice::from_ref(&tag1.v6_entry), + "Loopback should have IPv6 addr" + ); + + Ok(()) +} + +/// Tests should not rely on sleep for correctness/synchronization. +/// +/// However, when following logs, tests that sleep in-between +/// fallible operations are a lot more fun to follow and debug. +/// +/// This is an arbitrary sleep to make logs more digestable. +async fn slow_down() { + tokio::time::sleep(Duration::from_millis(250)).await; +} + /// This struct simplifies repetitive CRUD operations /// on tagged links with random address registrations. struct TestAddrs<'a> { @@ -923,6 +1210,22 @@ impl<'a> TestAddrs<'a> { Ok(()) } + /// Creates a [`PortSettings`] instance for these addresses. + fn settings(&self) -> PortSettings { + PortSettings { + links: HashMap::from([( + self.link_id.to_string(), + LinkSettings { + params: LINK_CREATE, + addrs: vec![ + self.v4_entry.addr.into(), + self.v6_entry.addr.into(), + ], + }, + )]), + } + } + /// Adds both tagged addresses to this link using dpd's `port_settings_apply` endpoint. async fn apply_addrs( &self, @@ -931,18 +1234,7 @@ impl<'a> TestAddrs<'a> { .port_settings_apply( &self.port_id, Some(&self.v4_entry.tag), - &PortSettings { - links: HashMap::from([( - self.link_id.to_string(), - LinkSettings { - params: LINK_CREATE, - addrs: vec![ - self.v4_entry.addr.into(), - self.v6_entry.addr.into(), - ], - }, - )]), - }, + &self.settings(), ) .await?; diff --git a/dpd/src/api_server.rs b/dpd/src/api_server.rs index d939a970..c73ae304 100644 --- a/dpd/src/api_server.rs +++ b/dpd/src/api_server.rs @@ -100,6 +100,7 @@ use common::ports::TxEqSwHw; use crate::attached_subnet; use crate::counters; +use crate::link; #[cfg(feature = "multicast")] use crate::mcast; use crate::nat; @@ -904,8 +905,19 @@ impl DpdApi for DpdApiImpl { let path = path.into_inner(); let port_id = path.port_id; let link_id = path.link_id; + + #[cfg(not(feature = "chaos"))] + let src = link::Source::Config; + + // Let chaos tests inspect SDE state to detect divergences + // with link soft state. I question why this isn't the default + // behavior, but flagging this saves that conversation for + // another day. + #[cfg(feature = "chaos")] + let src = link::Source::Switch; + switch - .link_enabled(port_id, link_id) + .link_enabled(port_id, link_id, src) .map(HttpResponseOk) .map_err(|e| e.into()) } @@ -1710,7 +1722,7 @@ impl DpdApi for DpdApiImpl { route::reset_ipv4_tag(switch, &tag).await; route::reset_ipv6_tag(switch, &tag).await; switch - .clear_link_addresses(Some(&tag)) + .clear_link_state(Some(&tag)) .map(|_| HttpResponseUpdatedNoContent()) .map_err(|e| e.into()) } @@ -1734,7 +1746,7 @@ impl DpdApi for DpdApiImpl { error!(switch.log, "failed to reset route data: {:?}", e); err = Some(e); } - if let Err(e) = switch.clear_link_state() { + if let Err(e) = switch.clear_link_state(None) { error!(switch.log, "failed to clear all link state: {:?}", e); err = Some(e); } diff --git a/dpd/src/link.rs b/dpd/src/link.rs index 6d9f7fab..989c28ee 100644 --- a/dpd/src/link.rs +++ b/dpd/src/link.rs @@ -21,6 +21,7 @@ use crate::table::uplink; use crate::transceivers::qsfp_xcvr_mpn; use crate::types::DpdError; use crate::types::DpdResult; +use aal::AsicError; use aal::AsicId; use aal::AsicOps; use aal::AsicResult; @@ -54,6 +55,7 @@ use std::collections::btree_map; use std::collections::btree_map::Entry; use std::net::Ipv4Addr; use std::net::Ipv6Addr; +use std::ops::Bound; use std::sync::Arc; use std::sync::Mutex; use std::time::Duration; @@ -708,15 +710,7 @@ impl Switch { let link_lock = self.get_link_lock(port_id, link_id)?; let mut link = link_lock.lock().unwrap(); - // Delete all addresses in the switch tables for this link. - if !link.ipv4.is_empty() { - let to_delete = std::mem::take(&mut link.ipv4).into_keys(); - port_ip::ipv4_delete_many(self, link.asic_port_id, to_delete)?; - } - if !link.ipv6.is_empty() { - let to_delete = std::mem::take(&mut link.ipv6).into_keys(); - port_ip::ipv6_delete_many(self, link.asic_port_id, to_delete)?; - } + self.clear_link_addresses_locked(&mut link, None)?; // Notify the reconciliation task that this link's ASIC resources need // to be released. @@ -727,62 +721,61 @@ impl Switch { } /// Clear all the state associated with all data links. - pub fn clear_link_state(&self) -> DpdResult<()> { + /// + /// If `Some(tag)` is given, all state associated with that tag + /// is cleared from the link. + pub fn clear_link_state(&self, tag: Option<&str>) -> DpdResult<()> { let links = self.links.lock().unwrap(); for link_lock in links.0.values() { let mut link = link_lock.lock().unwrap(); - // Clear all IP addresses. - // - // Swap out an empty map with the existing one, so that we can - // retain an iterable for calling `ipv{4,6}_delete_many`. - if !link.ipv4.is_empty() { - let to_delete = std::mem::take(&mut link.ipv4).into_keys(); - port_ip::ipv4_delete_many(self, link.asic_port_id, to_delete)?; - } - if !link.ipv6.is_empty() { - let to_delete = std::mem::take(&mut link.ipv6).into_keys(); - port_ip::ipv6_delete_many(self, link.asic_port_id, to_delete)?; - } + self.clear_link_addresses_locked(&mut link, tag)?; } Ok(()) } - /// Clear any IP addresses associated with all links, optionally restricted - /// to a specified string `tag`. - pub fn clear_link_addresses(&self, tag: Option<&str>) -> DpdResult<()> { - if let Some(tag) = tag { - let links = self.links.lock().unwrap(); - for link_lock in links.0.values() { - let mut link = link_lock.lock().unwrap(); - self.clear_link_addresses_locked(&mut link, tag); + /// Removes tracked IPv4 and IPv6 addresses from asic and link tables. + /// + /// If `Some(tag)` is provided, only those addresses belonging + /// to the named tag are deleted. + /// + /// If an unexpected asic failure occurs, this returns Err without + /// attempting any further progress. + fn clear_link_addresses_locked( + &self, + link: &mut Link, + tag: Option<&str>, + ) -> DpdResult<()> { + let mut cursor = Bound::Unbounded; + while let Some(entry) = + link.ipv4.range((cursor, Bound::Unbounded)).next() + { + let addr = *entry.0; + cursor = Bound::Excluded(addr); + + if tag.is_some_and(|t| t != entry.1) { + continue; } - Ok(()) - } else { - self.clear_link_state() + + port_ip::ipv4_clear(self, link.asic_port_id, addr)?; + link.ipv4.remove(&addr); } - } - fn clear_link_addresses_locked(&self, link: &mut Link, tag: &str) { - // Delete the entries from the ASIC tables. - let _ = port_ip::ipv4_delete_many( - self, - link.asic_port_id, - link.ipv4 - .iter() - .filter(|entry| entry.1 == tag) - .map(|entry| *entry.0), - ); - link.ipv4.retain(|_, t| t != tag); + let mut cursor = Bound::Unbounded; + while let Some(entry) = + link.ipv6.range((cursor, Bound::Unbounded)).next() + { + let addr = *entry.0; + cursor = Bound::Excluded(addr); - let _ = port_ip::ipv6_delete_many( - self, - link.asic_port_id, - link.ipv6 - .iter() - .filter(|entry| entry.1 == tag) - .map(|entry| *entry.0), - ); - link.ipv6.retain(|_, t| t != tag); + if tag.is_some_and(|t| t != entry.1) { + continue; + } + + port_ip::ipv6_clear(self, link.asic_port_id, addr)?; + link.ipv6.remove(&addr); + } + + Ok(()) } // Update the state of a link with a closure. @@ -1018,22 +1011,31 @@ impl Switch { self.link_fetch(port_id, link_id, |link| link.asic_port_id) } - /// Add an IPv4 address to the provided link. - pub fn create_ipv4_address_locked( + /// Add an IPv4 address to the provided link if it does not currently exist. + /// + /// On success, returns whether any tables were modified. + pub fn set_ipv4_address_locked( &self, link: &mut Link, addr: Ipv4Addr, tag: String, - ) -> DpdResult<()> { + ) -> DpdResult { match link.ipv4.entry(addr) { - btree_map::Entry::Occupied(curr) => Err(DpdError::Exists(format!( - "IP address {addr} already exists under tag {}", - curr.get() - ))), + // We could merge this with the block below to recover from any condition + // in which an entry exists in soft state but not switch tables. + // But that solves a problem we should not currently have at the cost + // of touching the dreaded SDE layer more frequently. + btree_map::Entry::Occupied(curr) if curr.get() == &tag => Ok(false), btree_map::Entry::Vacant(slot) => { - port_ip::ipv4_add(self, link.asic_port_id, addr)?; + port_ip::ipv4_set(self, link.asic_port_id, addr)?; slot.insert(tag); - Ok(()) + Ok(true) + } + btree_map::Entry::Occupied(curr) => { + Err(DpdError::AddrTagConflict { + addr: addr.into(), + tag: curr.get().to_string(), + }) } } } @@ -1047,7 +1049,12 @@ impl Switch { tag: String, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - self.create_ipv4_address_locked(link, addr, tag) + if !self.set_ipv4_address_locked(link, addr, tag)? { + return Err(DpdError::Exists(format!( + "This tag has already registered address {addr:?}" + ))); + } + Ok(()) }) } @@ -1074,38 +1081,32 @@ impl Switch { }) } - /// Deletes this IPv4 address from the link. + /// Removes the link from soft state and switch tables if it exists. /// - /// Returns Err if the address is not found. - /// - /// If tag is None, the address is deleted regardless of tag. - /// If tag is Some, the address is only deleted if its registration - /// tag matches the given tag. - pub fn delete_ipv4_address_locked( + /// On success, returns whether anything was actually deleted. + pub fn clear_ipv4_address_locked( &self, link: &mut Link, addr: Ipv4Addr, tag: Option<&str>, - ) -> DpdResult<()> { + ) -> DpdResult { match link.ipv4.entry(addr) { - btree_map::Entry::Vacant(_) => Err(DpdError::NoSuchAddress { - port_id: link.port_id, - link_id: link.link_id, - address: addr.into(), - }), btree_map::Entry::Occupied(slot) - if tag.is_some_and(|t| t != slot.get()) => + if tag.is_none_or(|t| t == slot.get()) => { + port_ip::ipv4_clear(self, link.asic_port_id, addr)?; + slot.remove(); + Ok(true) + } + btree_map::Entry::Occupied(slot) => { Err(DpdError::AddrTagConflict { addr: addr.into(), tag: slot.get().to_string(), }) } - btree_map::Entry::Occupied(slot) => { - port_ip::ipv4_delete(self, link.asic_port_id, addr)?; - slot.remove(); - Ok(()) - } + // If the asic layer is capable of diverging from this soft state, + // we could call `port_ip::ipv4_clear` here as well. + btree_map::Entry::Vacant(_) => Ok(false), } } @@ -1118,7 +1119,14 @@ impl Switch { tag: Option<&str>, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - self.delete_ipv4_address_locked(link, addr, tag) + if !self.clear_ipv4_address_locked(link, addr, tag)? { + return Err(DpdError::NoSuchAddress { + port_id, + link_id, + address: addr.into(), + }); + } + Ok(()) }) } @@ -1130,29 +1138,32 @@ impl Switch { ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { while let Some(entry) = link.ipv4.first_entry() { - port_ip::ipv4_delete(self, link.asic_port_id, *entry.key())?; + port_ip::ipv4_clear(self, link.asic_port_id, *entry.key())?; entry.remove(); } Ok(()) }) } - /// Add an IPv6 address to the provided link. - pub fn create_ipv6_address_locked( + /// An IPv6 equivalent to [`Self::set_ipv4_address_locked`]. + pub fn set_ipv6_address_locked( &self, link: &mut Link, addr: Ipv6Addr, tag: String, - ) -> DpdResult<()> { + ) -> DpdResult { match link.ipv6.entry(addr) { - btree_map::Entry::Occupied(curr) => Err(DpdError::Exists(format!( - "IP address {addr} already exists under tag {}", - curr.get() - ))), btree_map::Entry::Vacant(slot) => { - port_ip::ipv6_add(self, link.asic_port_id, addr)?; + port_ip::ipv6_set(self, link.asic_port_id, addr)?; slot.insert(tag); - Ok(()) + Ok(true) + } + btree_map::Entry::Occupied(curr) if curr.get() == &tag => Ok(false), + btree_map::Entry::Occupied(curr) => { + Err(DpdError::AddrTagConflict { + addr: addr.into(), + tag: curr.get().to_string(), + }) } } } @@ -1166,7 +1177,12 @@ impl Switch { tag: String, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - self.create_ipv6_address_locked(link, addr, tag) + if !self.set_ipv6_address_locked(link, addr, tag)? { + return Err(DpdError::Exists(format!( + "This tag has already registered address {addr:?}" + ))); + } + Ok(()) }) } @@ -1193,38 +1209,28 @@ impl Switch { }) } - /// Deletes this IPv6 address from the link. - /// - /// Returns Err if the address is not found. - /// - /// If tag is None, the address is deleted regardless of tag. - /// If tag is Some, the address is only deleted if its registration - /// tag matches the given tag. - pub fn delete_ipv6_address_locked( + /// IPv6 equivalent of [`Self::clear_ipv4_address_locked`]. + pub fn clear_ipv6_address_locked( &self, link: &mut Link, address: Ipv6Addr, tag: Option<&str>, - ) -> DpdResult<()> { + ) -> DpdResult { match link.ipv6.entry(address) { - btree_map::Entry::Vacant(_) => Err(DpdError::NoSuchAddress { - port_id: link.port_id, - link_id: link.link_id, - address: address.into(), - }), + btree_map::Entry::Vacant(_) => Ok(false), btree_map::Entry::Occupied(slot) - if tag.is_some_and(|t| t != slot.get()) => + if tag.is_none_or(|t| t == slot.get()) => { + port_ip::ipv6_clear(self, link.asic_port_id, address)?; + slot.remove(); + Ok(true) + } + btree_map::Entry::Occupied(slot) => { Err(DpdError::AddrTagConflict { addr: address.into(), tag: slot.get().to_string(), }) } - btree_map::Entry::Occupied(slot) => { - port_ip::ipv6_delete(self, link.asic_port_id, address)?; - slot.remove(); - Ok(()) - } } } @@ -1237,7 +1243,14 @@ impl Switch { tag: Option<&str>, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - self.delete_ipv6_address_locked(link, address, tag) + if !self.clear_ipv6_address_locked(link, address, tag)? { + return Err(DpdError::NoSuchAddress { + port_id, + link_id, + address: address.into(), + }); + } + Ok(()) }) } @@ -1249,7 +1262,7 @@ impl Switch { ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { while let Some(entry) = link.ipv6.first_entry() { - port_ip::ipv6_delete(self, link.asic_port_id, *entry.key())?; + port_ip::ipv6_clear(self, link.asic_port_id, *entry.key())?; entry.remove(); } Ok(()) @@ -1349,8 +1362,12 @@ impl Switch { &self, port_id: PortId, link_id: LinkId, + src: Source, ) -> DpdResult { - self.link_fetch(port_id, link_id, |link| link.config.enabled) + self.link_fetch(port_id, link_id, |link| match src { + Source::Config => link.config.enabled, + Source::Switch => link.plumbed.enabled, + }) } /// Set whether a link is enabled. @@ -1449,7 +1466,7 @@ impl Switch { prbs: PortPrbsMode, ) -> DpdResult<()> { if prbs != PortPrbsMode::Mission - && self.link_enabled(port_id, link_id)? + && self.link_enabled(port_id, link_id, Source::Config)? { Err(DpdError::Invalid( "PRBS cannot be set on an enabled port".into(), @@ -1652,7 +1669,7 @@ fn clear_mac_config(switch: &Switch, asic_id: AsicId) -> DpdResult<()> { #[cfg(feature = "multicast")] { mac::mcast_mac_clear(switch, asic_id)?; - mcast::mcast_egress::del_port_mapping_entry(switch, asic_id)?; + mcast::mcast_egress::clear_port_mapping_entry(switch, asic_id)?; } Ok(()) } @@ -1683,14 +1700,21 @@ fn unplumb_link( } if link.plumbed.link_created { - if let Err(e) = switch.asic_hdl.port_delete(link.port_hdl) { - error!(log, "failed to delete ASIC port: {e:?}"); - return Err(e.into()); + match switch.asic_hdl.port_delete(link.port_hdl) { + Ok(()) | Err(AsicError::Missing(_)) => { + link.plumbed.link_created = false; + switch.record_event( + link.asic_port_id, + Event::Admin(AdminEvent::Delete), + ); + } + Err(e) => { + error!(log, "failed to delete ASIC port: {e:?}"); + return Err(e.into()); + } } - link.plumbed.link_created = false; - switch - .record_event(link.asic_port_id, Event::Admin(AdminEvent::Delete)); } + Ok(()) } @@ -2026,6 +2050,17 @@ async fn reconcile_link( } } +/// Differentiates the source of truth when querying +/// link information. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum Source { + /// What value the switch is configured for. + Config, + + /// What value the switch currently has. + Switch, +} + pub enum LinkTrigger { Update(PortId, LinkId), Timeout, diff --git a/dpd/src/main.rs b/dpd/src/main.rs index 5fc08ec5..3315902d 100644 --- a/dpd/src/main.rs +++ b/dpd/src/main.rs @@ -382,6 +382,28 @@ impl Switch { }) } + /// Calls [`Self::table_entry_add`]. + /// If add fails due to conflict, tries [`Self::table_entry_update`]. + /// Returns err if the first failure is fatal or both calls fail. + pub fn table_entry_set( + &self, + table_type: TableType, + key: &M, + data: &A, + ) -> DpdResult<()> { + match self.table_entry_add(table_type, key, data) { + Err(DpdError::Switch(AsicError::Exists)) => { + self.table_entry_update(table_type, key, data) + } + Err(full @ DpdError::TableFull(_)) => { + // If the table is full and this key does not exist, + // a "does not exist" error is just distracting. + self.table_entry_update(table_type, key, data).map_err(|_| full) + } + other => other, + } + } + /// Delete a single table entry. pub fn table_entry_del( &self, @@ -398,6 +420,21 @@ impl Switch { }) } + /// A variant of [`Self::table_entry_del`] that returns Ok + /// if the deleted resource already does not exist. + pub fn table_entry_clear( + &self, + table_type: TableType, + key: &M, + ) -> DpdResult<()> { + let maybe_deleted = self.table_entry_del(table_type, key); + if matches!(maybe_deleted, Err(DpdError::Switch(AsicError::Missing(_)))) + { + return Ok(()); + } + maybe_deleted + } + /// Fetch all of the entries in a P4 table and return them pub fn table_dump( &self, diff --git a/dpd/src/port_settings.rs b/dpd/src/port_settings.rs index 9ee97283..f3b0fddc 100644 --- a/dpd/src/port_settings.rs +++ b/dpd/src/port_settings.rs @@ -402,7 +402,7 @@ impl PortSettingsDiff { // Create the IPv4 addresses for (addr, tag) in &spec.ipv4 { - Self::addr_add_v4(ctx, &mut link, rb, *addr, tag.clone())?; + Self::addr_set_v4(ctx, &mut link, rb, *addr, tag.clone())?; } // Create the IPv6 addresses @@ -433,7 +433,7 @@ impl PortSettingsDiff { // Delete the IPv4 addresses for (addr, tag) in &spec.ipv4 { - Self::addr_del_v4(ctx, &mut link, rb, *addr, tag.clone())?; + Self::addr_clear_v4(ctx, &mut link, rb, *addr, tag.clone())?; } // Delete the IPv6 addresses @@ -530,11 +530,11 @@ impl PortSettingsDiff { .filter(|(addr, _)| !ipv4_after.contains_key(addr)); for (addr, tag) in v4_add { - Self::addr_add_v4(ctx, &mut link, rb, *addr, tag.clone())?; + Self::addr_set_v4(ctx, &mut link, rb, *addr, tag.clone())?; } for (addr, tag) in v4_del { - Self::addr_del_v4(ctx, &mut link, rb, *addr, tag.clone())?; + Self::addr_clear_v4(ctx, &mut link, rb, *addr, tag.clone())?; } let v6_add = ipv6_after @@ -555,7 +555,7 @@ impl PortSettingsDiff { Ok(()) } - fn addr_add_v4( + fn addr_set_v4( ctx: &mut Context<'_>, link: &mut Link, rb: &mut Rollback, @@ -565,19 +565,20 @@ impl PortSettingsDiff { trace!(ctx.log, "ipv4 add ({addr}: {tag})"); // Create address on ASIC first. let switch = ctx.switch; - switch.create_ipv4_address_locked(link, addr, tag.clone())?; + switch.set_ipv4_address_locked(link, addr, tag.clone())?; let link_id = link.link_id; rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> { let switch = ctx.switch; let link_lock = ctx.link(link_id)?; let mut link = link_lock.lock().unwrap(); - switch.delete_ipv4_address_locked(&mut link, addr, Some(&tag)) + switch.clear_ipv4_address_locked(&mut link, addr, Some(&tag))?; + Ok(()) }); Ok(()) } - fn addr_del_v4( + fn addr_clear_v4( ctx: &mut Context<'_>, link: &mut Link, rb: &mut Rollback, @@ -587,13 +588,14 @@ impl PortSettingsDiff { trace!(ctx.log, "ipv4 del ({addr}: {tag})"); let switch = ctx.switch; let link_id = link.link_id; - switch.delete_ipv4_address_locked(link, addr, Some(&tag))?; + switch.clear_ipv4_address_locked(link, addr, Some(&tag))?; rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> { let switch = ctx.switch; let link_lock = ctx.link(link_id)?; let mut link = link_lock.lock().unwrap(); - switch.create_ipv4_address_locked(&mut link, addr, tag) + switch.set_ipv4_address_locked(&mut link, addr, tag)?; + Ok(()) }); Ok(()) } @@ -609,13 +611,14 @@ impl PortSettingsDiff { // Create address on ASIC first. let switch = ctx.switch; let link_id = link.link_id; - switch.create_ipv6_address_locked(link, addr, tag.clone())?; + switch.set_ipv6_address_locked(link, addr, tag.clone())?; rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> { let switch = ctx.switch; let link_lock = ctx.link(link_id)?; let mut link = link_lock.lock().unwrap(); - switch.delete_ipv6_address_locked(&mut link, addr, Some(&tag)) + switch.clear_ipv6_address_locked(&mut link, addr, Some(&tag))?; + Ok(()) }); Ok(()) } @@ -630,13 +633,14 @@ impl PortSettingsDiff { trace!(ctx.log, "ipv6 del ({addr}: {tag})"); let switch = ctx.switch; let link_id = link.link_id; - switch.delete_ipv6_address_locked(link, addr, Some(&tag))?; + switch.clear_ipv6_address_locked(link, addr, Some(&tag))?; rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> { let switch = ctx.switch; let link_lock = ctx.link(link_id)?; let mut link = link_lock.lock().unwrap(); - switch.create_ipv6_address_locked(&mut link, addr, tag) + switch.set_ipv6_address_locked(&mut link, addr, tag)?; + Ok(()) }); Ok(()) } diff --git a/dpd/src/table/mac.rs b/dpd/src/table/mac.rs index 533577f5..c7bbab5d 100644 --- a/dpd/src/table/mac.rs +++ b/dpd/src/table/mac.rs @@ -55,7 +55,7 @@ fn mac_set_common( fn mac_clear_common(s: &Switch, type_: TableType, port: u16) -> DpdResult<()> { let match_key = MacMatchKey { port }; - match s.table_entry_del(type_, &match_key) { + match s.table_entry_clear(type_, &match_key) { Ok(_) => { info!(s.log, "cleared mac on {port} in table {type_}",); Ok(()) diff --git a/dpd/src/table/mcast/mcast_egress.rs b/dpd/src/table/mcast/mcast_egress.rs index 10eed2e8..b1055857 100644 --- a/dpd/src/table/mcast/mcast_egress.rs +++ b/dpd/src/table/mcast/mcast_egress.rs @@ -231,7 +231,7 @@ pub(crate) fn update_port_mapping_entry( /// Delete a port ID entry from the port ID table for converting ASIC port IDs /// to port numbers. -pub(crate) fn del_port_mapping_entry( +pub(crate) fn clear_port_mapping_entry( s: &Switch, asic_port_id: u16, ) -> DpdResult<()> { @@ -239,7 +239,7 @@ pub(crate) fn del_port_mapping_entry( debug!(s.log, "delete port id entry {match_key} -> {asic_port_id}"); - s.table_entry_del(TableType::McastEgressPortMapping, &match_key) + s.table_entry_clear(TableType::McastEgressPortMapping, &match_key) } /// Dump the multicast port mapping table. diff --git a/dpd/src/table/port_ip.rs b/dpd/src/table/port_ip.rs index 322493a5..de22640b 100644 --- a/dpd/src/table/port_ip.rs +++ b/dpd/src/table/port_ip.rs @@ -165,27 +165,30 @@ fn endeavour_to_repair( panic!("Repeated repair attempts failed. Giving up."); } -fn ipv4_add_work(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { +fn ipv4_set_work(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { let (claim_key, drop_key) = match_keys_ipv4(ipv4, port); - s.table_entry_add( + s.table_entry_set( TableType::PortAddrIpv4, &claim_key, &ActionV4::ClaimIpv4, )?; - s.table_entry_add(TableType::PortAddrIpv4, &drop_key, &ActionV4::DropIpv4) + s.table_entry_set(TableType::PortAddrIpv4, &drop_key, &ActionV4::DropIpv4) .inspect_err(|_| { endeavour_to_repair( s, format!("ipv4 address {ipv4} only half added"), - || s.table_entry_del(TableType::PortAddrIpv4, &claim_key), + || s.table_entry_clear(TableType::PortAddrIpv4, &claim_key), ); }) } -/// Add one IPv4 address to the ASIC tables. -pub fn ipv4_add(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { - ipv4_add_work(s, port, ipv4) +/// Puts this IPv4 address in the appropriate ASIC tables. +/// +/// If the address does not yet exist, an entry is created. +/// If it does, the entry is updated. +pub fn ipv4_set(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { + ipv4_set_work(s, port, ipv4) .map(|_| { info!(s.log, "added ipv4 address"; "addr" => %ipv4, @@ -199,16 +202,16 @@ pub fn ipv4_add(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { }) } -fn ipv4_delete_work(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { +fn ipv4_clear_work(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { let (claim_key, drop_key) = match_keys_ipv4(ipv4, port); - s.table_entry_del(TableType::PortAddrIpv4, &claim_key)?; - s.table_entry_del(TableType::PortAddrIpv4, &drop_key).inspect_err(|_| { + s.table_entry_clear(TableType::PortAddrIpv4, &claim_key)?; + s.table_entry_clear(TableType::PortAddrIpv4, &drop_key).inspect_err(|_| { endeavour_to_repair( s, format!("ipv4 address {ipv4} only half deleted"), || { - s.table_entry_add( + s.table_entry_set( TableType::PortAddrIpv4, &claim_key, &ActionV4::ClaimIpv4, @@ -219,8 +222,9 @@ fn ipv4_delete_work(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { } /// Delete one IPv4 address from the ASIC tables. -pub fn ipv4_delete(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { - ipv4_delete_work(s, port, ipv4) +/// Returns Ok if the address is already not present. +pub fn ipv4_clear(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { + ipv4_clear_work(s, port, ipv4) .map(|_| { info!(s.log, "deleted ipv4 address"; "addr" => %ipv4, @@ -234,39 +238,27 @@ pub fn ipv4_delete(s: &Switch, port: u16, ipv4: Ipv4Addr) -> DpdResult<()> { }) } -/// Delete many IPv4 address from the ASIC tables. -pub fn ipv4_delete_many<'a>( - s: &'a Switch, - port: u16, - addrs: impl Iterator + 'a, -) -> DpdResult<()> { - for addr in addrs { - let _ = ipv4_delete(s, port, addr); - } - Ok(()) -} - -fn ipv6_add_work(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { +fn ipv6_set_work(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { let (claim_key, drop_key) = match_keys_ipv6(ipv6, port); - s.table_entry_add( + s.table_entry_set( TableType::PortAddrIpv6, &claim_key, &ActionV6::ClaimIpv6, )?; - s.table_entry_add(TableType::PortAddrIpv6, &drop_key, &ActionV6::DropIpv6) + s.table_entry_set(TableType::PortAddrIpv6, &drop_key, &ActionV6::DropIpv6) .inspect_err(|_| { endeavour_to_repair( s, format!("ipv6 address {ipv6} only half added"), - || s.table_entry_del(TableType::PortAddrIpv6, &claim_key), + || s.table_entry_clear(TableType::PortAddrIpv6, &claim_key), ); }) } -/// Add one IPv6 address to the ASIC tables. -pub fn ipv6_add(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { - ipv6_add_work(s, port, ipv6) +/// IPv6 equivalent of [`ipv4_set`]. +pub fn ipv6_set(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { + ipv6_set_work(s, port, ipv6) .map(|_| { info!(s.log, "added ipv6 address"; "addr" => %ipv6, @@ -280,16 +272,16 @@ pub fn ipv6_add(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { }) } -fn ipv6_delete_work(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { +fn ipv6_clear_work(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { let (claim_key, drop_key) = match_keys_ipv6(ipv6, port); - s.table_entry_del(TableType::PortAddrIpv6, &claim_key)?; - s.table_entry_del(TableType::PortAddrIpv6, &drop_key).inspect_err(|_| { + s.table_entry_clear(TableType::PortAddrIpv6, &claim_key)?; + s.table_entry_clear(TableType::PortAddrIpv6, &drop_key).inspect_err(|_| { endeavour_to_repair( s, format!("ipv6 address {ipv6} only half deleted"), || { - s.table_entry_add( + s.table_entry_set( TableType::PortAddrIpv6, &claim_key, &ActionV6::ClaimIpv6, @@ -300,8 +292,8 @@ fn ipv6_delete_work(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { } /// Delete one IPv6 address from the ASIC tables. -pub fn ipv6_delete(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { - ipv6_delete_work(s, port, ipv6) +pub fn ipv6_clear(s: &Switch, port: u16, ipv6: Ipv6Addr) -> DpdResult<()> { + ipv6_clear_work(s, port, ipv6) .map(|_| { info!(s.log, "deleted ipv6 address"; "addr" => %ipv6, @@ -356,15 +348,3 @@ pub fn ipv6_counter_fetch( ) -> DpdResult> { s.counter_fetch::(force_sync, TableType::PortAddrIpv6) } - -/// Delete many IPv6 address from the ASIC tables. -pub fn ipv6_delete_many<'a>( - s: &'a Switch, - port: u16, - addrs: impl Iterator + 'a, -) -> DpdResult<()> { - for addr in addrs { - let _ = ipv6_delete(s, port, addr); - } - Ok(()) -} diff --git a/dpd/src/table/uplink.rs b/dpd/src/table/uplink.rs index d8f04abd..818c70ff 100644 --- a/dpd/src/table/uplink.rs +++ b/dpd/src/table/uplink.rs @@ -42,7 +42,7 @@ fn set_ingress_uplink(s: &Switch, port: u16) -> DpdResult<()> { let match_key = IngressMatchKey { in_port: port }; let action_data = IngressAction::UplinkPort; - match s.table_entry_add(TableType::UplinkIngress, &match_key, &action_data) + match s.table_entry_set(TableType::UplinkIngress, &match_key, &action_data) { Ok(_) => { info!(s.log, "set uplink on {}", port); @@ -58,7 +58,7 @@ fn set_ingress_uplink(s: &Switch, port: u16) -> DpdResult<()> { fn clear_ingress_uplink(s: &Switch, port: u16) -> DpdResult<()> { let match_key = IngressMatchKey { in_port: port }; - match s.table_entry_del(TableType::UplinkIngress, &match_key) { + match s.table_entry_clear(TableType::UplinkIngress, &match_key) { Ok(_) => { info!(s.log, "cleared uplink on {}", port); Ok(()) @@ -77,7 +77,7 @@ pub fn uplink_set(s: &Switch, port: u16) -> DpdResult<()> { let match_key = EgressMatchKey { out_port: port }; let action_data = EgressAction::Allowed; - match s.table_entry_add(TableType::UplinkEgress, &match_key, &action_data) { + match s.table_entry_set(TableType::UplinkEgress, &match_key, &action_data) { Ok(_) => { info!(s.log, "set guest_traffic_allowed on {}", port); Ok(()) @@ -94,11 +94,12 @@ pub fn uplink_set(s: &Switch, port: u16) -> DpdResult<()> { } /// Remove an entry from the uplink tables. +/// Returns Ok if the entry was not found. pub fn uplink_clear(s: &Switch, port: u16) -> DpdResult<()> { clear_ingress_uplink(s, port)?; let match_key = EgressMatchKey { out_port: port }; - match s.table_entry_del(TableType::UplinkEgress, &match_key) { + match s.table_entry_clear(TableType::UplinkEgress, &match_key) { Ok(_) => { info!(s.log, "cleared guest_traffic_allowed on {}", port); Ok(()) diff --git a/dpd/src/types.rs b/dpd/src/types.rs index 8938546a..d9fb5807 100644 --- a/dpd/src/types.rs +++ b/dpd/src/types.rs @@ -89,7 +89,11 @@ pub enum DpdError { #[error("Tag is required for idempotent validation")] MissingTag, #[error("Address {addr} exists, but it is owned by another tag: {tag}")] - AddrTagConflict { addr: IpAddr, tag: String }, + AddrTagConflict { + addr: IpAddr, + /// The tag that currently owns the address. + tag: String, + }, } impl From for DpdError {