diff --git a/Cargo.lock b/Cargo.lock index 6712c2c2e..cbf119931 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1108,6 +1108,16 @@ dependencies = [ "tracing", ] +[[package]] +name = "dataplane-left-right-tlcache" +version = "0.1.0" +dependencies = [ + "ahash 0.8.10", + "left-right", + "serial_test", + "thiserror 2.0.17", +] + [[package]] name = "dataplane-lpm" version = "0.1.0" @@ -1269,7 +1279,9 @@ dependencies = [ "bytes", "chrono", "dataplane-cli", + "dataplane-concurrency", "dataplane-config", + "dataplane-left-right-tlcache", "dataplane-lpm", "dataplane-net", "dataplane-tracectl", @@ -1282,6 +1294,7 @@ dependencies = [ "mio", "netdev", "procfs", + "rand 0.9.2", "serde", "thiserror 2.0.17", "tokio", diff --git a/Cargo.toml b/Cargo.toml index ac7f6635d..fa4c7e757 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,6 +16,7 @@ members = [ "id", "init", "interface-manager", + "left-right-tlcache", "mgmt", "nat", "net", @@ -51,6 +52,7 @@ hardware = { path = "./hardware", package = "dataplane-hardware" } id = { path = "./id", package = "dataplane-id" } init = { path = "./init", package = "dataplane-init" } interface-manager = { path = "./interface-manager", package = "dataplane-interface-manager" } +left-right-tlcache = { path = "./left-right-tlcache", package = "dataplane-left-right-tlcache" } lpm = { path = "./lpm", package = "dataplane-lpm" } mgmt = { path = "./mgmt", package = "dataplane-mgmt" } nat = { path = "./nat", package = "dataplane-nat" } diff --git a/dataplane/src/packet_processor/ipforward.rs b/dataplane/src/packet_processor/ipforward.rs index 1e4bf4e3d..274ef9db0 100644 --- a/dataplane/src/packet_processor/ipforward.rs +++ b/dataplane/src/packet_processor/ipforward.rs @@ -14,9 +14,8 @@ use std::net::IpAddr; use tracing::{debug, error, trace, warn}; use routing::fib::fibobjects::{EgressObject, FibEntry, PktInstruction}; -use routing::fib::fibtable::FibTable; use routing::fib::fibtable::FibTableReader; -use routing::fib::fibtype::FibId; +use routing::fib::fibtype::FibKey; use routing::evpn::Vtep; use routing::rib::encapsulation::{Encapsulation, VxlanEncapsulation}; @@ -56,11 +55,11 @@ impl IpForwarder { /// Forward a [`Packet`] fn forward_packet(&self, packet: &mut Packet, vrfid: VrfId) { let nfi = &self.name; - let fibid = if let Some(dst_vpcd) = packet.get_meta().dst_vpcd { + let fibkey = if let Some(dst_vpcd) = packet.get_meta().dst_vpcd { let VpcDiscriminant::VNI(dst_vni) = dst_vpcd; - FibId::from_vni(dst_vni) + FibKey::from_vni(dst_vni) } else { - FibId::from_vrfid(vrfid) + FibKey::from_vrfid(vrfid) }; /* get destination ip address */ @@ -71,57 +70,39 @@ impl IpForwarder { }; debug!("{nfi}: processing packet to {dst} with vrf {vrfid}"); - /* Get the fib to use: this lookup could be avoided since - we know the interface the packet came from and it has to be - attached to a certain fib if the vrf metadata value was set. - This extra lookup is a side effect of splitting into stages. - */ - - /* Read-only access to the fib table */ - let Some(fibtr) = self.fibtr.enter() else { - error!("{nfi}: Unable to lookup fib for vrf {vrfid}"); - packet.done(DoneReason::InternalFailure); - return; - }; - /* Lookup the fib which needs to be consulted */ - let Some(fibr) = fibtr.get_fib(&fibid) else { - warn!("{nfi}: Unable to find fib with id {fibid} for vrf {vrfid}"); + /* access fib, by fetching FibReader from cache */ + let Ok(fibr) = &self.fibtr.get_fib_reader(fibkey) else { + warn!("{nfi}: Unable to read fib. Key={fibkey}"); packet.done(DoneReason::InternalFailure); return; }; - /* Read-only access to fib */ let Some(fib) = fibr.enter() else { - warn!("{nfi}: Unable to read from fib {fibid}"); + warn!("{nfi}: Unable to read from fib. Key={fibkey}"); packet.done(DoneReason::InternalFailure); return; }; - /* Perform lookup in the fib */ + + /* Perform lookup in the fib. This always returns a FibEntry */ let (prefix, fibentry) = fib.lpm_entry_prefix(packet); - if let Some(fibentry) = &fibentry { - debug!("{nfi}: Packet hits prefix {prefix} in fib {fibid}"); - debug!("{nfi}: Entry is:\n{fibentry}"); - - /* decrement packet TTL, unless the packet is for us */ - if !fibentry.is_iplocal() { - Self::decrement_ttl(packet, dst); - if packet.is_done() { - debug!("TTL/Hop-count limit exceeded!"); - return; - } + debug!("{nfi}: Packet hits prefix {prefix} in fib {fibkey}"); + debug!("{nfi}: Entry is:\n{fibentry}"); + + /* decrement packet TTL, unless the packet is for us */ + if !fibentry.is_iplocal() { + Self::decrement_ttl(packet, dst); + if packet.is_done() { + debug!("TTL/Hop-count limit exceeded!"); + return; } - /* execute instructions according to FIB */ - self.packet_exec_instructions(&fibtr, packet, fibentry, fib.get_vtep()); - } else { - debug!("Could not get fib group for {prefix}. Will drop packet..."); - packet.done(DoneReason::InternalFailure); } + /* execute instructions according to FIB */ + self.packet_exec_instructions(packet, fibentry, fib.get_vtep()); } /// Execute a local packet instruction fn packet_exec_instruction_local( &self, packet: &mut Packet, - fibtable: &FibTable, _ifindex: InterfaceIndex, /* we get it from metadata */ ) { let nfi = &self.name; @@ -134,14 +115,18 @@ impl IpForwarder { let vni = vxlan.vni(); debug!("{nfi}: DECAPSULATED vxlan packet:\n {packet}"); debug!("{nfi}: Packet comes with vni {vni}"); - let fibid = FibId::from_vni(vni); - let Some(fib) = fibtable.get_fib(&fibid) else { - error!("{nfi}: Failed to find fib {fibid} associated to vni {vni}"); + + // access fib for Vni vni + let fibkey = FibKey::from_vni(vni); + let Ok(fibr) = self.fibtr.get_fib_reader(fibkey) else { + error!("{nfi}: Failed to find fib associated to vni {vni}. Fib key = {fibkey}"); packet.done(DoneReason::Unroutable); return; }; - let Some(next_vrf) = fib.get_id().map(|id| id.as_u32()) else { - debug!("{nfi}: Failed to access fib {fibid} to determine vrf"); + let Some(next_vrf) = fibr.get_id().map(|id| id.as_u32()) else { + debug!( + "{nfi}: Failed to access fib {fibkey} to determine vrf. Fib Key={fibkey}" + ); packet.done(DoneReason::InternalFailure); return; }; @@ -331,7 +316,6 @@ impl IpForwarder { /// Execute a [`PktInstruction`] on the packet fn packet_exec_instruction( &self, - fibtable: &FibTable, vtep: &Vtep, packet: &mut Packet, instruction: &PktInstruction, @@ -339,7 +323,7 @@ impl IpForwarder { match instruction { PktInstruction::Drop => self.packet_exec_instruction_drop(packet), PktInstruction::Local(ifindex) => { - self.packet_exec_instruction_local(packet, fibtable, *ifindex); + self.packet_exec_instruction_local(packet, *ifindex); } PktInstruction::Encap(encap) => self.packet_exec_instruction_encap(packet, encap, vtep), PktInstruction::Egress(egress) => self.packet_exec_instruction_egress(packet, egress), @@ -349,13 +333,12 @@ impl IpForwarder { /// Execute all of the [`PktInstruction`]s indicated by the given [`FibEntry`] on the packet fn packet_exec_instructions( &self, - fibtable: &FibTable, packet: &mut Packet, fibentry: &FibEntry, vtep: &Vtep, ) { for inst in fibentry.iter() { - self.packet_exec_instruction(fibtable, vtep, packet, inst); + self.packet_exec_instruction(vtep, packet, inst); if packet.is_done() { return; } diff --git a/left-right-tlcache/Cargo.toml b/left-right-tlcache/Cargo.toml new file mode 100644 index 000000000..9a58573fa --- /dev/null +++ b/left-right-tlcache/Cargo.toml @@ -0,0 +1,14 @@ +[package] +name = "dataplane-left-right-tlcache" +version = "0.1.0" +edition = "2024" +publish = false +license = "Apache-2.0" + +[dependencies] +left-right = { workspace = true } +ahash = { workspace = true } +thiserror = { workspace = true } + +[dev-dependencies] +serial_test = { workspace = true } diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs new file mode 100644 index 000000000..6476c3217 --- /dev/null +++ b/left-right-tlcache/src/lib.rs @@ -0,0 +1,779 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Small crate to provide thread-local storage of left-right "read handles". +//! TL;DR: the `ReadHandle` type that allows concurrent reads over some type T cannot +//! be safely shared among distinct threads. There are cases where multiple threads may need +//! to read multiple T's but the set of T's (and read handles) may not be known upfront and +//! change over time. +//! +//! So, a mechanism is needed to: +//! - ensure that threads discover read handles for multiple instances of T +//! - ensure they are provided with non-shared read handles for those T's +//! +//! This crate provides a type to that end that relies on a thread-local cache of read-handles. +//! Having a thread-local set of read-handles avoids the potentially-expensive creation of those +//! every time an instance of T needs to be read (e.g. a FIB). This type assumes that each of the +//! T instances (or their handles) can be identified by some Key. In order to populate the local +//! cache of read handles a read-handle "provider" is needed. Neither the key types, T or the provider +//! are prescribed here. The provider functionality is implemented as a trait. +//! +//! An alternative approach to the problem would use `thread_local::ThreadLocal`. However, the latter +//! requires Q to be `Send`, which is not necessarily the case of `ReadHandle`. Specifically, +//! `ReadHandle` is Send only if T is Sync. +//! +//! The use of this type requires: +//! - implementing trait `ReadHandleProvider` to provide `ReadHandle` 's based on some key value. +//! - implementing trait `Identity` for T in `ReadHandle` +//! - declaring a thread-local `ReadHandleCache` object +//! +//! Note: providers must be Sync since the thread-local caches for distinct threads will poll them. + +use ahash::RandomState; +use left_right::{ReadHandle, ReadHandleFactory}; +use std::cell::RefCell; +use std::collections::HashMap; +use std::hash::Hash; +use std::rc::Rc; +use std::thread::LocalKey; +use thiserror::Error; + +pub trait ReadHandleProvider: Sync { + type Data; + type Key; + + /// A provider should provide per thread `ReadHandles`. However, it is safer if we require providers + /// to give us factories as that shields us from buggy providers returning ReadHandles that are not + /// unique per thread. There should be no performance penalty of offering factories instead of read handles + /// since factory::handle() is identical to rhandle::clone() + #[allow(clippy::type_complexity)] + fn get_factory( + &self, + key: &Self::Key, + ) -> Option<(&ReadHandleFactory, Self::Key, u64)>; + + /// Ask the provider about the identity of T for the `ReadHandle` accessible via some key. + /// This is needed for the cache to be able to invalidate entries that point to `ReadHandle`'s + /// for T's that should no longer be accessed by that key. + fn get_identity(&self, key: &Self::Key) -> Option; + + /// Get version. Provider should promise to provide a distinct value (e.g. monotonically increasing) + /// anytime there's a change in the collection of read handles / factories it owns. + fn get_version(&self) -> u64; + + /// Method for a provider to produce an iterator over the read handles (factories) + /// This returns (version, iterator) + #[allow(clippy::type_complexity)] + fn get_iter( + &self, + ) -> ( + u64, + impl Iterator, Self::Key)>, + ); +} + +/// Trait to determine the real identity of a `T` wrapped in left-right. That is, +/// the identity of `T` in a `ReadHandle`. This is needed to invalidate cache entries +/// with keys that are alias of their identity. +pub trait Identity { + fn identity(&self) -> K; +} + +#[derive(Debug, PartialEq, Error)] +pub enum ReadHandleCacheError { + #[error("Reader not found for key {0}")] + NotFound(K), + #[error("Reader for key {0} is not accessible")] + NotAccessible(K), +} + +/// An entry in a thread-local `ReadHandleCache` to hold a `ReadHandle` +/// along with its identity, which may match the key to find it or not. +struct ReadHandleEntry { + rhandle: Rc>, + identity: K, + version: u64, +} +impl, K: PartialEq> ReadHandleEntry { + fn new(identity: K, rhandle: Rc>, version: u64) -> Self { + Self { + rhandle, + identity, + version, + } + } + fn is_valid(&self, key: &K, provider: &impl ReadHandleProvider) -> bool { + if self.rhandle.was_dropped() { + return false; + } + if *key == self.identity { + return true; + } + if self.version == provider.get_version() { + return true; + } + let Some(identity) = provider.get_identity(key) else { + return false; + }; + if self.identity != identity { + return false; + } + // this is just extra sanity + match self.rhandle.enter() { + Some(t) => t.identity() == identity, + None => false, + } + } +} + +pub struct ReadHandleCache { + handles: RefCell, RandomState>>, + refresh_version: RefCell, // version when last refresh mas made +} +impl ReadHandleCache +where + K: Hash + Eq + Clone, + T: Identity, +{ + #[allow(clippy::new_without_default)] + pub fn new() -> Self { + Self { + handles: RefCell::new(HashMap::with_hasher(RandomState::with_seed(0))), + refresh_version: RefCell::new(0), + } + } + pub fn get_reader( + thread_local: &'static LocalKey, + key: K, + provider: &impl ReadHandleProvider, + ) -> Result>, ReadHandleCacheError> { + thread_local.with(|local| { + let mut map = local.handles.borrow_mut(); + + // cache has a valid handle for that key + if let Some(entry) = map.get(&key) + && entry.is_valid(&key, provider) + { + return Ok(Rc::clone(&entry.rhandle)); + } + + // get a factory for the key from the provider to build a fresh handle from it + // provider returns identity of object and version for entry invalidation + let (factory, identity, version) = provider.get_factory(&key).ok_or_else(|| { + map.remove(&key); + ReadHandleCacheError::NotFound(key.clone()) + })?; + + // obtain handle but don't store it nor return it if there is no writer / data + let rhandle = factory.handle(); + if rhandle.was_dropped() { + // can remove element with key, but also all which point to the same identity + map.retain(|_key, entry| entry.identity != identity); + return Err(ReadHandleCacheError::NotAccessible(key.clone())); + } + + // store a new entry locally with a handle, its identity and version, for the given key + let rhandle = Rc::new(rhandle); + let entry = ReadHandleEntry::new(identity.clone(), Rc::clone(&rhandle), version); + map.insert(key.clone(), entry); + + // if the querying key is not the identity, update entry for key = identity. This helps in consistency + // and avoids having duplicate readhandles for the same T, which should expedite checks with many read handles + // if T's are accessed by multiple keys. + if key != identity { + map.insert( + identity.clone(), + ReadHandleEntry::new(identity, Rc::clone(&rhandle), version), + ); + } + Ok(rhandle) + }) + } + + pub fn purge(thread_local: &'static LocalKey) { + thread_local.with(|local| { + local.handles.borrow_mut().clear(); + *local.refresh_version.borrow_mut() = 0; + }); + } + + #[allow(unused)] + fn purge_unreadable(thread_local: &'static LocalKey) { + thread_local.with(|local| { + let mut handles = local.handles.borrow_mut(); + handles.retain(|_, e| !e.rhandle.was_dropped()); + }); + } + + // Do a full refresh of the cache + pub fn refresh( + thread_local: &'static LocalKey, + provider: &impl ReadHandleProvider, + ) { + // skip refresh if the version has not changed + let cache_refresh_version = thread_local.with(|local| *local.refresh_version.borrow()); + let provider_version = provider.get_version(); + if cache_refresh_version == provider_version { + // this should not be needed + Self::purge_unreadable(thread_local); + return; + } + + // get all readers (factories) from the provider + let (version, iterator) = provider.get_iter(); + + // theoretically, it could happen that while we call get_version() and get_iter(), the underlying collection + // has changed and both differ + if version != provider_version { + Self::refresh(thread_local, provider); + return; + } + + // filter out all unusable readers from iterator + let iterator = iterator.filter(|(_key, factory, _id)| { + let rhandle = factory.handle(); + !rhandle.was_dropped() + }); + + // split the iterator in two: primaries and aliases + let (primaries, aliases): (Vec<_>, Vec<_>) = iterator.partition(|(key, _, id)| key == id); + + // update local cache, consuming the iterator + thread_local.with(|local| { + let mut handles = local.handles.borrow_mut(); + + // purge all unusable readers + handles.retain(|_key, entry| !entry.rhandle.was_dropped()); + + // update primaries first and store an Rc of the latest rhandles in a temporary map + let mut temporary = HashMap::new(); + for (key, factory, id) in primaries { + handles + .entry(key.clone()) + .and_modify(|e| { + if e.version != version { + *e = ReadHandleEntry::new( + id.clone(), + Rc::new(factory.handle()), + version, + ); + } + temporary.insert(id.clone(), Rc::clone(&e.rhandle)); + }) + .or_insert_with(|| { + let rhandle = Rc::new(factory.handle()); + temporary.insert(key, Rc::clone(&rhandle)); + ReadHandleEntry::new(id, rhandle, version) + }); + } + // update entries for aliases to reuse primaries' handles, using the temporary map + for (key, _factory, id) in aliases { + if let Some(rhandle) = temporary.get(&id) { + handles.insert( + key.clone(), + ReadHandleEntry::new(id, Rc::clone(rhandle), version), + ); + } else { + // we should only get here if we got a key (alias) and could not find + // the primary object. This would be a provider bug. + // TODO: determine what to do here + } + } + + *local.refresh_version.borrow_mut() = version; + }); + } + + /// Get an iterator of read handles from the cache. If refresh is true, the cache will be refreshed first. + /// This function is mostly useful if we want to iterate over the objects that the cache represents, optionally + /// refreshing it first. This is useful when the caller does not know _what_ objects are there. + /// Since `ReadHandleProvider`s must be Sync, threads could simply call `ReadHandleProvider::get_iter()` + /// directly. However, that would not refresh the cache and would create a new reader for every item returned + /// by the provider, on each call. + pub fn iter( + thread_local: &'static LocalKey, + provider: &impl ReadHandleProvider, + refresh: bool, + ) -> impl Iterator>)> { + if refresh { + ReadHandleCache::refresh(thread_local, provider); + } + thread_local.with(|local| { + let vector: Vec<(K, Rc>)> = local + .handles + .borrow() + .iter() + .map(|(key, e)| (key.clone(), Rc::clone(&e.rhandle))) + .collect(); + vector.into_iter() + }) + } +} + +/// Create a thread-local `ReadHandleCache` with a given name, to access +/// `ReadHandle`'s identified with some type of key. +/// Example: +/// ``` +/// use left_right::{ReadHandle, ReadHandleFactory}; +/// use dataplane_left_right_tlcache::make_thread_local_readhandle_cache; +/// use dataplane_left_right_tlcache::ReadHandleCache; +/// use dataplane_left_right_tlcache::Identity; +/// +/// struct LeftRightWrappedType; +/// impl Identity for LeftRightWrappedType { +/// fn identity(&self) -> u32 {0} +/// } +/// +/// make_thread_local_readhandle_cache!(MYCACHE, u32, LeftRightWrappedType); +/// ``` +#[macro_export] +macro_rules! make_thread_local_readhandle_cache { + ($name:ident, $key_t:ty, $rhandle_t:ty) => { + thread_local! { + static $name: ReadHandleCache<$key_t, $rhandle_t> = ReadHandleCache::new(); + } + }; +} + +#[cfg(test)] +mod tests { + #![allow(clippy::collapsible_if)] + + use super::*; + use left_right::{Absorb, ReadHandleFactory, WriteHandle}; + use serial_test::serial; + use std::sync::Mutex; + // Our left-right protected struct + #[derive(Debug, Clone)] + struct TestStruct { + id: u64, + data: String, + } + impl TestStruct { + fn new(id: u64, data: &str) -> Self { + Self { + id, + data: data.to_string(), + } + } + } + // Implement identity for TestStruct + impl Identity for TestStruct { + fn identity(&self) -> u64 { + self.id + } + } + + // Dummy implementation of Absorb for TestStruct + #[derive(Debug)] + enum TestStructChange { + Update(String), + } + impl Absorb for TestStruct { + fn absorb_first(&mut self, op: &mut TestStructChange, _other: &Self) { + match op { + TestStructChange::Update(data) => { + self.data = data.clone(); + } + } + } + fn sync_with(&mut self, first: &Self) { + *self = first.clone(); + } + } + + // create local cache + make_thread_local_readhandle_cache!(TEST_CACHE, u64, TestStruct); + + // ReadHandle "owner" implementing ReadHandleProvider + #[derive(Debug)] + struct TestProviderEntry, TestStructChange> { + id: u64, + factory: ReadHandleFactory, + // writer owning the TestStruct. We use option to easily drop it + // and Mutex to make the provider Sync. + writer: Option>>, + } + impl TestProviderEntry { + fn new( + id: u64, + factory: ReadHandleFactory, + writer: Option>>, + ) -> Self { + Self { + id, + factory, + writer, + } + } + } + #[derive(Debug)] + struct TestProvider { + data: HashMap>, + version: u64, + } + impl TestProvider { + fn new() -> Self { + Self { + data: HashMap::new(), + version: 0, + } + } + fn add_object(&mut self, key: u64, identity: u64) { + if key != identity { + let entry = self.data.get(&identity).unwrap(); + let new = TestProviderEntry::new(identity, entry.factory.clone(), None); + self.data.insert(key, new); + } else { + let object = TestStruct::new(identity, "unset"); + let (w, r) = left_right::new_from_empty(object); + let entry = TestProviderEntry::new(identity, r.factory(), Some(Mutex::new(w))); + self.data.insert(key, entry); + let stored = self.data.get(&key).unwrap(); + assert_eq!(stored.id, identity); + } + self.version = self.version.wrapping_add(1); + } + fn mod_object(&mut self, key: u64, data: &str) { + if let Some(object) = self.data.get_mut(&key) { + if let Some(writer_lock) = &mut object.writer { + #[allow(clippy::mut_mutex_lock)] // lock exists just to make provider Sync + let mut writer = writer_lock.lock().unwrap(); + writer.append(TestStructChange::Update(data.to_owned())); + writer.publish(); + } + } + } + fn drop_writer(&mut self, key: u64) { + if let Some(object) = self.data.get_mut(&key) { + let x = object.writer.take(); + drop(x); + self.version = self.version.wrapping_add(1); + } + } + } + + // Implement trait ReadHandleProvider + impl ReadHandleProvider for TestProvider { + type Data = TestStruct; + type Key = u64; + fn get_version(&self) -> u64 { + self.version + } + fn get_factory( + &self, + key: &Self::Key, + ) -> Option<(&ReadHandleFactory, Self::Key, u64)> { + self.data + .get(key) + .map(|entry| (&entry.factory, entry.id, self.version)) + } + fn get_identity(&self, key: &Self::Key) -> Option { + self.data.get(key).map(|entry| entry.id) + } + fn get_iter( + &self, + ) -> ( + u64, + impl Iterator, Self::Key)>, + ) { + let iterator = self + .data + .iter() + .map(|(key, entry)| (*key, &entry.factory, entry.id)); + + (self.version, iterator) + } + } + + #[serial] + #[test] + fn test_readhandle_cache_basic() { + // start fresh + ReadHandleCache::purge(&TEST_CACHE); + + // build provider + let mut provider = TestProvider::new(); + provider.add_object(1, 1); + provider.add_object(2, 2); + provider.mod_object(1, "object-1"); + provider.mod_object(2, "object-2"); + + // add alias for 1 + provider.add_object(6000, 1); + + { + let key = 1; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 1); + assert_eq!(obj.data, "object-1"); + } + + { + let key = 2; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 2); + assert_eq!(obj.data, "object-2"); + } + + { + // 6000 is alias for 1: should access object with id 1 + let key = 6000; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 1); + assert_eq!(obj.data, "object-1"); + } + + TEST_CACHE.with(|x| { + let x = x.handles.borrow(); + assert!(x.contains_key(&6000)); + assert!(x.contains_key(&1)); + assert!(x.contains_key(&2)); + assert_eq!(x.len(), 3); + println!("Test: cache contains entries"); + }); + + println!("Change: Let 6000 be a key for 2 instead of 1"); + provider.add_object(6000, 2); + provider.mod_object(2, "object-2-modified"); + { + let key = 6000; + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 2); + assert_eq!(obj.data, "object-2-modified"); + } + { + let key = 6000; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 2); + assert_eq!(obj.data, "object-2-modified"); + } + + println!("Change: drop data for object 1. It should not be accessible"); + provider.drop_writer(1); + { + let key = 1; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(key))); + } + + println!("Change: drop data for object 2: should not be accessible by keys 2 and 6000"); + provider.drop_writer(2); + { + let key = 2; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(key))); + } + { + let key = 6000; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(key))); + } + + // ensure cache is clean + TEST_CACHE.with(|x| { + let x = x.handles.borrow(); + assert!(!x.contains_key(&6000)); + assert!(!x.contains_key(&1)); + assert!(!x.contains_key(&2)); + assert!(x.is_empty()); + println!("Test: cache is empty"); + }); + } + + #[serial] + #[test] + fn test_readhandle_cache_multi_invalidation() { + // start fresh + ReadHandleCache::purge(&TEST_CACHE); + + const NUM_ALIASES: u64 = 10; + + // build provider + let mut provider = TestProvider::new(); + provider.add_object(1, 1); + provider.mod_object(1, "object-1"); + + // add aliases + for k in 1..=NUM_ALIASES { + let alias = 100 + k; + provider.add_object(alias, 1); + } + + // query for identity and all aliases + ReadHandleCache::get_reader(&TEST_CACHE, 1, &provider).unwrap(); + for k in 1..=NUM_ALIASES { + let alias = 100 + k; + ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider).unwrap(); + } + + // cache should contain NUM_ALIASES + 1 entries + TEST_CACHE + .with(|cache| assert_eq!(cache.handles.borrow().len() as u64, (NUM_ALIASES + 1u64))); + + provider.drop_writer(1); + + // do single query for identity + let h = ReadHandleCache::get_reader(&TEST_CACHE, 1, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(1))); + + // all entries should have been invalidated + TEST_CACHE.with(|cache| assert!(cache.handles.borrow().is_empty())); + + // querying again with key = identity should fail + let h = ReadHandleCache::get_reader(&TEST_CACHE, 1, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(1))); + + // querying again with aliases should fail too. Aliases are 100 + k k=1..=NUM_ALIASES + let alias = 100 + 1; + let h = ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(alias))); + } + + #[serial] + #[test] + fn test_readhandle_cache() { + // start fresh + ReadHandleCache::purge(&TEST_CACHE); + + // build provider and populate it + const NUM_HANDLES: u64 = 1000; + let mut provider = TestProvider::new(); + for id in 0..=NUM_HANDLES { + provider.add_object(id, id); + provider.mod_object(id, format!("object-id-{id}").as_ref()); + provider.add_object(id + NUM_HANDLES + 1, id); + } + + // access all objects by id + for id in 0..=NUM_HANDLES { + let h = ReadHandleCache::get_reader(&TEST_CACHE, id, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, id); + } + + // access all objects by alias + for id in 0..=NUM_HANDLES { + let alias = id + NUM_HANDLES + 1; + let h = ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, id); + } + + // modify all objects and replace all aliases + for id in 0..=NUM_HANDLES { + let alias = 2 * NUM_HANDLES + 1 - id; + provider.mod_object(id, format!("modified-id-{id}").as_ref()); + provider.add_object(alias, id); + } + + // access objects with re-assigned aliases + for id in 0..=NUM_HANDLES { + let alias = 2 * NUM_HANDLES + 1 - id; + let h = ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, id); + assert_eq!(obj.data, format!("modified-id-{id}")); + } + + // invalidate all writers + for id in 0..=NUM_HANDLES { + provider.drop_writer(id); + } + + // access objects from alias: none should be accessible + for id in 0..=NUM_HANDLES { + let alias = 2 * NUM_HANDLES + 1 - id; + let h = ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(alias))); + } + + // all handles from cache should have been removed as we looked up them all + TEST_CACHE.with(|cache| assert!(cache.handles.borrow().is_empty())); + } + + #[serial] + #[test] + fn test_readhandle_cache_iter() { + // start fresh + ReadHandleCache::purge(&TEST_CACHE); + + // build provider and populate it + const NUM_HANDLES: u64 = 20; + let mut provider = TestProvider::new(); + for id in 1..=NUM_HANDLES { + let alias = NUM_HANDLES + id; + provider.add_object(id, id); + provider.mod_object(id, format!("object-{id}").as_ref()); + provider.add_object(alias, id); + } + + // should be empty + TEST_CACHE.with(|cache| assert!(cache.handles.borrow().is_empty())); + + // request iterator without refresh. Nothing should be returned since cache is empty. + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, false); + assert_eq!(iterator.count(), 0); + + // request iterator with refresh + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, true); + + // consume it + let mut count = 0; + for (_key, rhandle) in iterator { + count += 1; + let _guard = rhandle.enter().unwrap(); + } + + // we got all + assert_eq!(count, NUM_HANDLES * 2); + + // test that if the version of the provider does not change, then the cache is not unnecessarily refreshed. + // Our test provider does not change version unless we add/drop elements and we don't now. + { + // empty cache for testing purposes. Since this will reset iter_version, we save it first + let saved_iter_version = TEST_CACHE.with(|local| *local.refresh_version.borrow()); + ReadHandleCache::purge(&TEST_CACHE); + TEST_CACHE.with(|local| *local.refresh_version.borrow_mut() = saved_iter_version); + } + // since version did not change, we should not get anything after iterating. + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, true); + assert_eq!(iterator.count(), 0); + assert_eq!(TEST_CACHE.with(|local| local.handles.borrow().len()), 0); + + // if we reset the version, then the iterator should refresh the cache. + TEST_CACHE.with(|local| *local.refresh_version.borrow_mut() = 0); + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, true); + assert_eq!(iterator.count() as u64, 2 * NUM_HANDLES); + assert_eq!( + TEST_CACHE.with(|local| local.handles.borrow().len() as u64), + 2 * NUM_HANDLES + ); + + // test that refresh/iter filters out invalid handles (need refresh) 2 should have been invalidated + provider.drop_writer(1); + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, true); + let vec: Vec<(u64, Rc>)> = iterator.collect(); + assert_eq!(vec.len() as u64, (NUM_HANDLES - 1) * 2); + } +} diff --git a/routing/Cargo.toml b/routing/Cargo.toml index 7cde0afd5..d3d146995 100644 --- a/routing/Cargo.toml +++ b/routing/Cargo.toml @@ -13,6 +13,7 @@ testing = [] cli = { workspace = true } config = { workspace = true } dplane-rpc = { workspace = true } +left-right-tlcache = { workspace = true } lpm = { workspace = true } net = { workspace = true } tracectl = { workspace = true } @@ -39,6 +40,8 @@ netdev = { workspace = true } [dev-dependencies] bolero = { workspace = true, default-features = false } +concurrency = { workspace = true } lpm = { workspace = true, features = ["testing"] } +rand = { workspace = true, default-features = false, features = ["thread_rng"] } tracing-test = { workspace = true, features = [] } diff --git a/routing/src/display.rs b/routing/src/display.rs index c5d769fb9..4d4d7e5be 100644 --- a/routing/src/display.rs +++ b/routing/src/display.rs @@ -2,13 +2,21 @@ // Copyright Open Network Fabric Authors //! Module that implements Display for routing objects - +//! +//! Note: most of the objects for which Display is implemented here belong to +//! the routing database which is fully and only owned by the routing thread. +//! This includes the Fib contents since fibs belong to vrfs. +//! So: +/// - it is Okay to call any of this from the routing thread (cli) +/// - other threads may not be able to call Display's for routing objects. +/// - Display for Fib objects visible from dataplane workers can be safely called. +/// - Cli thread does not need a read handle cache to inspect Fib contents +/// - Still, FIXME(fredi): make that distinction clearer use crate::atable::adjacency::{Adjacency, AdjacencyTable}; use crate::cpi::{CpiStats, CpiStatus, StatsRow}; use crate::fib::fibgroupstore::FibRoute; use crate::fib::fibobjects::{EgressObject, FibEntry, FibGroup, PktInstruction}; -use crate::fib::fibtable::FibTable; -use crate::fib::fibtype::{Fib, FibId}; +use crate::fib::fibtype::{Fib, FibKey}; use crate::frr::frrmi::{FrrAppliedConfig, Frrmi, FrrmiStats}; use crate::rib::VrfTable; @@ -638,12 +646,12 @@ impl Display for AdjacencyTable { } //========================= Fib ================================// -impl Display for FibId { +impl Display for FibKey { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { match self { - FibId::Id(vrfid) => write!(f, "vrfid: {vrfid}")?, - FibId::Vni(vni) => write!(f, "vni: {vni:?}")?, - FibId::Unset => write!(f, "Unset!")?, + FibKey::Id(vrfid) => write!(f, "vrfid: {vrfid}")?, + FibKey::Vni(vni) => write!(f, "vni: {vni:?}")?, + FibKey::Unset => write!(f, "Unset!")?, } Ok(()) } @@ -700,7 +708,7 @@ impl Display for FibRoute { fn fmt_fib_trie bool>( f: &mut std::fmt::Formatter<'_>, - fibid: FibId, + fibid: FibKey, show_string: &str, trie: &PrefixMapTrie, group_filter: F, @@ -723,17 +731,6 @@ impl Display for Fib { Ok(()) } } -impl Display for FibTable { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { - Heading(format!(" Fib Table ({} fibs)", self.len())).fmt(f)?; - for (_fibid, fibr) in self.iter() { - if let Some(fib) = fibr.enter() { - write!(f, "{}", *fib)?; - } - } - Ok(()) - } -} pub struct FibViewV4<'a, F> where diff --git a/routing/src/errors.rs b/routing/src/errors.rs index 34e3759ba..f23894183 100644 --- a/routing/src/errors.rs +++ b/routing/src/errors.rs @@ -3,6 +3,7 @@ //! The error results used by this library. +use crate::fib::fibtype::FibKey; use net::interface::InterfaceIndex; use thiserror::Error; @@ -40,4 +41,10 @@ pub enum RouterError { #[error("Invalid configuration: {0}")] InvalidConfig(&'static str), + + #[error("Fibtable is not accessible")] + FibTableError, + + #[error("Fib error: {0}")] + FibError(#[from] left_right_tlcache::ReadHandleCacheError), } diff --git a/routing/src/fib/fibgroupstore.rs b/routing/src/fib/fibgroupstore.rs index fbf742e68..1705dc16f 100644 --- a/routing/src/fib/fibgroupstore.rs +++ b/routing/src/fib/fibgroupstore.rs @@ -9,48 +9,43 @@ //! Fibgroups when routing changes occur for all the affected routes without needing to explicitly //! change any of the routes. This allows, upon routing changes, updating the FIB in O(Nh) instead //! of O(Routes), potentially reducing the number of updates by several orders of magnitude. -//! This is achieved by means of an `RefCell`, which allows us to mutate fibgroups. -//! This data structure is, therefore, NOT thread-safe. Thread-safety is achieved by wrapping -//! this structure in left-right. +//! This is achieved by means of an `UnsafeCell`, which allows us to mutate fibgroups. +//! This data structure is, therefore, **NOT** thread-safe. //! -//! Any modification of the FibGroupStore or the `Rc>`s it contains -//! must be done while holding a `left_right::WriteGuard` to the `Fib` that owns it. +//! Safety is achieved by wrapping the structure in left-right. Safety for readers is granted +//! by the fact that the fib will not be modifed as long as readers have a read guard to it. +//! It is also guaranteed despite the use of Rc because readers never get to see those Rc's +//! (they are internal) and only the left-right writer deals with cloning them. +//! An implementation without the protection given by left-right would require, instead, +//! something like `Arc>`. //! -//! Any use of the `FibGroupStore` or the `Rc>`s it contains -//! must be done while holding a `left_right::ReadGuard` or `left_right::WriteGuard` to -//! the `Fib` that owns it. - -#![allow(unused)] +//! **NOTE**: Throughout the documentation it is assumed that the structure is wrapped in left-right. use crate::fib::fibobjects::{FibEntry, FibGroup}; use crate::rib::nexthop::NhopKey; use ahash::RandomState; -use std::cell::{Ref, RefCell}; +use std::cell::UnsafeCell; use std::collections::HashMap; use std::rc::Rc; use thiserror::Error; #[allow(unused)] -use tracing::{debug, warn}; +use tracing::{debug, error, warn}; #[derive(Error, Debug, PartialEq)] pub enum FibError { - #[error("Failed to find fibgroup for nh {0}")] + #[error("Failed to find fibgroup for nh key {0:?}")] NoFibGroup(NhopKey), } -#[derive(Default, Debug)] -pub(crate) struct FibGroupStore(HashMap>, RandomState>); +#[derive(Debug, Default)] +pub(crate) struct FibGroupStore(HashMap>, RandomState>); impl FibGroupStore { #[must_use] pub(crate) fn new() -> Self { let mut store = Self(HashMap::with_hasher(RandomState::with_seed(0))); - unsafe { - // This is safe because we have exclusive access to the store we just created - // so no one else can have a reference to any Rc's it contains - store.add_mod_group(&NhopKey::with_drop(), Self::drop_fibgroup()); - } + store.add_mod_group(&NhopKey::with_drop(), FibGroup::drop_fibgroup()); store } #[must_use] @@ -59,50 +54,52 @@ impl FibGroupStore { self.0.len() } #[must_use] - fn drop_fibgroup() -> FibGroup { - FibGroup::with_entry(FibEntry::drop_fibentry()) - } - #[must_use] /// get an Rc for the drop `Fibgroup`. The drop fibgroup is unique. - pub fn get_drop_fibgroup_ref(&self) -> Rc> { + pub fn get_drop_fibgroup_ref(&self) -> Rc> { self.get_ref(&NhopKey::with_drop()) .unwrap_or_else(|| unreachable!()) } //////////////////////////////////////////////////////////////////////////////// /// Add a `FibGroup` for a given `NhopKey` or replace it if it exists. - /// - /// Safety: - /// - /// This function is only safe if there are no references to the `Rc` - /// indexed by `key`, as returned by `get_ref`. - /// - /// In our actual usage, this is the case because we call this function - /// when everything is protected by the `left_right::WriteGuard` of the `Fib` - /// Any use of the `Rc>` entries in the `Fib` is protected by - /// `left_right::ReadGuard` or `left_right::WriteGuard` to the `Fib` that owns it. - /// + /// Safety: Only the left-right writer should use this method. //////////////////////////////////////////////////////////////////////////////// - pub(super) unsafe fn add_mod_group(&mut self, key: &NhopKey, fibgroup: FibGroup) { + pub(super) fn add_mod_group(&mut self, key: &NhopKey, fibgroup: FibGroup) { + if fibgroup.is_empty() { + error!("Refusing to add fibgroup without entries for key {key:?}"); + return; + } if let Some(group) = self.0.get(key) { - *group.borrow_mut() = fibgroup; + unsafe { + *group.get() = fibgroup; + } } else { - let fg = Rc::new(RefCell::new(fibgroup)); + let fg = Rc::new(UnsafeCell::new(fibgroup)); self.0.insert(key.clone(), fg); } } //////////////////////////////////////////////////////////////////////////////// /// Get a refcounted reference to the `Fibgroup` for a given `NhopKey`. This is /// used by `FibRoutes` to point to the current `FibGroups`. + /// + /// Safety: Only the left-right writer should use this method. //////////////////////////////////////////////////////////////////////////////// #[must_use] - fn get_ref(&self, key: &NhopKey) -> Option>> { + fn get_ref(&self, key: &NhopKey) -> Option>> { self.0.get(key).map(|group| Rc::clone(group)) } + + #[cfg(test)] #[must_use] - pub(crate) fn get(&self, key: &NhopKey) -> Option> { - self.0.get(key).map(|group| group.borrow()) + pub(crate) fn get(&self, key: &NhopKey) -> Option<&FibGroup> { + self.0.get(key).map(|group| unsafe { &*group.get() }) } + + //////////////////////////////////////////////////////////////////////////////// + /// Remove the fibgroup with the provided key. + /// + /// Safety: Only the left-right writer should use this method. + //////////////////////////////////////////////////////////////////////////////// pub(crate) fn del(&mut self, key: &NhopKey) { if key == &NhopKey::with_drop() { return; @@ -122,13 +119,16 @@ impl FibGroupStore { /// Remove unused `FibGroup`s. This should not be needed, but we may use it to expedite N deletions /// in batch, as it avoids lookups (at the expense of traversal). /// Returns the number of groups removed. + /// + /// Safety: Only the left-right writer should use this method. //////////////////////////////////////////////////////////////////////////////////////////////////// pub fn purge(&mut self) -> usize { let len = self.len(); self.0.retain(|key, group| { let keep = Rc::strong_count(group) > 1 || key == &NhopKey::with_drop(); if !keep { - debug!("Will purge fibgroup for nhop '{key}'"); + #[cfg(not(test))] + debug!("Purging fibgroup for nhop '{key}'"); } keep }); @@ -138,49 +138,52 @@ impl FibGroupStore { //////////////////////////////////////////////////////////////////////////////////////////////////// /// Iterate over the `FibGroups` in the store //////////////////////////////////////////////////////////////////////////////////////////////////// - pub(crate) fn values(&self) -> impl Iterator> { - self.0.values().map(|group| group.borrow()) + pub(crate) fn values(&self) -> impl Iterator { + unsafe { self.0.values().map(|group| &*group.get()) } } + //////////////////////////////////////////////////////////////////////////////////////////////////// /// Iterate over the `FibGroups` in the store //////////////////////////////////////////////////////////////////////////////////////////////////// - pub(crate) fn iter(&self) -> impl Iterator)> { - self.0.iter().map(|(key, group)| (key, group.borrow())) + #[cfg(test)] + pub(crate) fn iter(&self) -> impl Iterator { + unsafe { self.0.iter().map(|(key, group)| (key, &*group.get())) } } } #[derive(Debug, Clone)] -pub struct FibRoute(Vec>>); +pub struct FibRoute(Vec>>); impl FibRoute { #[must_use] pub(crate) fn new() -> Self { Self(vec![]) } #[must_use] - pub fn with_fibgroup(fg_ref: Rc>) -> Self { + pub fn with_fibgroup(fg_ref: Rc>) -> Self { Self(vec![fg_ref]) } + + #[cfg(test)] /// Add a reference to a `FibGroup` to a `FibRoute` - pub(crate) fn add_fibgroup_ref(&mut self, fg_ref: Rc>) { + pub(crate) fn add_fibgroup_ref(&mut self, fg_ref: Rc>) { self.0.push(fg_ref); } - #[must_use] - #[cfg(test)] - /// Get a reference to the `FibGroup` at the group-level index. This is only for testing. - pub(crate) fn get_fibgroup(&self, index: usize) -> Option> { - if index < self.len() { - unsafe { Some(self.0[index].borrow()) } - } else { - None - } - } ///////////////////////////////////////////////////////////////////////////////////////////////// /// Tells the total number of `FibEntry`s in a `FibRoute`, as the sum of the lengths of its `FibGroup`s ///////////////////////////////////////////////////////////////////////////////////////////////// #[must_use] pub(crate) fn len(&self) -> usize { - self.0.iter().fold(0, |val, g| val + g.borrow().len()) + self.0 + .iter() + .fold(0, |val, g| unsafe { val + (&*g.get()).len() }) + } + + #[allow(unused)] + #[must_use] + pub(crate) fn has_entries(&self) -> bool { + // empty vector returns false + self.0.iter().any(|g| unsafe { !(&*g.get()).is_empty() }) } ///////////////////////////////////////////////////////////////////////////////////////////////// @@ -207,29 +210,34 @@ impl FibRoute { /// to a real one, in the corresponding fibgroup, as shown in the next example for 4 groups. /// 0 1 2 3 4 | 5 6 7 | 8 9 | 10 11 12 | virtual entry indices /// 0 1 2 3 4 | 0 1 2 | 0 1 | 0 1 2 | real entry indices within each group. + /// + /// Panics: + /// This method will panic if index is >= FibRoute::len(). This is so to keep this method + /// infallible, which simplifies its use in several cases. //////////////////////////////////////////////////////////////////////////////////////////////// #[must_use] - pub(crate) fn get_fibentry(&self, index: usize) -> Option> { + pub(crate) fn get_fibentry(&self, index: usize) -> &FibEntry { let mut index = index; for g in self.0.iter() { - let group = g.borrow(); + let group = unsafe { &*g.get() }; if index < group.len() { - return Some(Ref::map(group, |g| &g.entries[index])); + return &group.entries[index]; } index -= group.len(); } - None + // There cannot exist a route without fib groups. + // Fibgroups must have at least one entry. + // Developer error: did not check FibRoute::len() + unreachable!() } //////////////////////////////////////////////////////////////////////////////////////////////// /// Provide iterator over the `FibGroups` that a `Fibroute` refers to //////////////////////////////////////////////////////////////////////////////////////////////// - pub(crate) fn iter(&self) -> impl Iterator> { - self.0.iter().map(|group| group.borrow()) + pub(crate) fn iter(&self) -> impl Iterator { + unsafe { self.0.iter().map(|group| &*group.get()) } } -} -impl FibRoute { #[must_use] /////////////////////////////////////////////////////////////////////////////////// /// Creates a `FibRoute` with the `FibGroups` corresponding to a set of `NhopKey`s. @@ -252,17 +260,14 @@ pub mod tests { use net::interface::InterfaceIndex; use crate::fib::fibgroupstore::{FibError, FibGroupStore, FibRoute}; - use crate::fib::fibobjects::EgressObject; - use crate::fib::fibobjects::FibEntry; - use crate::fib::fibobjects::FibGroup; - use crate::fib::fibobjects::PktInstruction; + use crate::fib::fibobjects::{EgressObject, FibEntry, FibGroup, PktInstruction}; use crate::rib::nexthop::NhopKey; use std::net::IpAddr; use std::str::FromStr; // builds fib entry with single egress instruction - fn build_fib_entry_egress(ifindex: u32, address: &str, ifname: &str) -> FibEntry { + pub(crate) fn build_fib_entry_egress(ifindex: u32, address: &str, ifname: &str) -> FibEntry { let addr = Some(IpAddr::from_str(address).unwrap()); let ifname = Some(ifname.to_string()); let inst = PktInstruction::Egress(EgressObject::new( @@ -272,8 +277,8 @@ pub mod tests { )); FibEntry::with_inst(inst) } - // builds fibgroup with a single entry - fn build_fibgroup(entries: &[FibEntry]) -> FibGroup { + // builds a fibgroup with several entries + pub(crate) fn build_fibgroup(entries: &[FibEntry]) -> FibGroup { let mut fibgroup = FibGroup::new(); fibgroup.entries.extend_from_slice(entries); fibgroup @@ -292,17 +297,16 @@ pub mod tests { let nhkey = NhopKey::with_address(&IpAddr::from_str("7.0.0.1").unwrap()); // store the fibgroup - unsafe { - store.add_mod_group(&nhkey, fibgroup); - } + store.add_mod_group(&nhkey, fibgroup); { // retrieve the fibgrop from the store from its key + // (scoped since we mutate the store later on) let stored = store.get(&nhkey).unwrap(); assert_eq!(stored.len(), 1); assert_eq!(stored.entries[0].len(), 1); assert_eq!(stored.entries[0], entry); - println!("{stored:#?}"); + println!("{stored}"); } // create fibroute to have a shared ref to that fibgroup @@ -311,26 +315,26 @@ pub mod tests { assert_eq!(fibroute.0.len(), 1); assert_eq!(fibroute.len(), 1); + // create a second fibroute object sharing the same fibgroup let mut fibroute2 = FibRoute::new(); fibroute2.add_fibgroup_ref(store.get_ref(&nhkey).unwrap()); // mutate/replace fibgroup, without modifying fibroute let entry2 = build_fib_entry_egress(100, "10.0.2.2", "eth1"); let fibgroup = FibGroup::with_entry(entry2.clone()); - unsafe { - store.add_mod_group(&nhkey, fibgroup); - } + store.add_mod_group(&nhkey, fibgroup); // check that the fibroute has been internally modified - let found1 = fibroute.get_fibentry(0).unwrap(); - let found2 = fibroute2.get_fibentry(0).unwrap(); + let found1 = fibroute.get_fibentry(0); + let found2 = fibroute2.get_fibentry(0); assert_eq!(*found1, *found2); assert_eq!(*found1, entry2); - println!("hit:\n{found1}"); + + println!("updated fibgroup entry:\n{found1}"); } #[test] - fn test_multi_fibgroup_route_entry_selection() { + fn test_fibgroup_multigroup_route_entry_selection() { // create multiple fib entries let e1 = build_fib_entry_egress(1, "10.0.1.1", "eth1"); let e2 = build_fib_entry_egress(2, "10.0.2.1", "eth2"); @@ -347,7 +351,7 @@ pub mod tests { let g3 = build_fibgroup(&[e7.clone()]); let g4 = build_fibgroup(&[e8.clone()]); - // create a dummy nhop key whose contents do not matter other than for storing each fibgroup + // create dummy nhop keys whose contents do not matter other than for storing each fibgroup let key1 = NhopKey::with_address(&IpAddr::from_str("8.0.0.1").unwrap()); let key2 = NhopKey::with_address(&IpAddr::from_str("8.0.0.2").unwrap()); let key3 = NhopKey::with_address(&IpAddr::from_str("8.0.0.3").unwrap()); @@ -355,12 +359,11 @@ pub mod tests { // create a fibgroup store and store the fibgroups with the respective nhop keys let mut store = FibGroupStore::new(); - unsafe { - store.add_mod_group(&key1, g1.clone()); - store.add_mod_group(&key2, g2.clone()); - store.add_mod_group(&key3, g3.clone()); - store.add_mod_group(&key4, g4.clone()); - } + store.add_mod_group(&key1, g1.clone()); + store.add_mod_group(&key2, g2.clone()); + store.add_mod_group(&key3, g3.clone()); + store.add_mod_group(&key4, g4.clone()); + assert_eq!(store.len(), 4 + 1); // +1 is for drop group println!("{store:#?}"); @@ -379,15 +382,16 @@ pub mod tests { assert_eq!(fibroute.len(), g1.len() + g2.len() + g3.len() + g4.len()); // select one entry in the fibroute by index and check that it is correct - assert_eq!(fibroute.get_fibentry(0).map(|e| e.clone()), Some(e1)); - assert_eq!(fibroute.get_fibentry(1).map(|e| e.clone()), Some(e2)); - assert_eq!(fibroute.get_fibentry(2).map(|e| e.clone()), Some(e3)); - assert_eq!(fibroute.get_fibentry(3).map(|e| e.clone()), Some(e4)); - assert_eq!(fibroute.get_fibentry(4).map(|e| e.clone()), Some(e5)); - assert_eq!(fibroute.get_fibentry(5).map(|e| e.clone()), Some(e6)); - assert_eq!(fibroute.get_fibentry(6).map(|e| e.clone()), Some(e7)); - assert_eq!(fibroute.get_fibentry(7).map(|e| e.clone()), Some(e8)); - assert!(fibroute.get_fibentry(8).is_none()); + // this route has all of the fibgroups + assert_eq!(&*fibroute.get_fibentry(0), &e1); + assert_eq!(&*fibroute.get_fibentry(0), &e1); + assert_eq!(&*fibroute.get_fibentry(1), &e2); + assert_eq!(&*fibroute.get_fibentry(2), &e3); + assert_eq!(&*fibroute.get_fibentry(3), &e4); + assert_eq!(&*fibroute.get_fibentry(4), &e5); + assert_eq!(&*fibroute.get_fibentry(5), &e6); + assert_eq!(&*fibroute.get_fibentry(6), &e7); + assert_eq!(&*fibroute.get_fibentry(7), &e8); // attempt to remove fibgroups: none should be removed from the store store.del(&key1); @@ -442,12 +446,10 @@ pub mod tests { // create a fibgroup store and store the fibgroups with the respective nhop keys let mut store = FibGroupStore::new(); - unsafe { - store.add_mod_group(&key1, g1.clone()); - store.add_mod_group(&key2, g2.clone()); - store.add_mod_group(&key3, g3.clone()); - store.add_mod_group(&key4, g4.clone()); - } + store.add_mod_group(&key1, g1.clone()); + store.add_mod_group(&key2, g2.clone()); + store.add_mod_group(&key3, g3.clone()); + store.add_mod_group(&key4, g4.clone()); assert_eq!(store.len(), 4 + 1); // +1 is for drop group // create a FibRoute from the keys @@ -458,16 +460,18 @@ pub mod tests { .unwrap(); assert_eq!(fibroute.0.len(), 4); - assert_eq!(*fibroute.0[0].borrow(), *store.get(&key1).unwrap()); - assert_eq!(*fibroute.0[1].borrow(), *store.get(&key2).unwrap()); - assert_eq!(*fibroute.0[2].borrow(), *store.get(&key3).unwrap()); - assert_eq!(*fibroute.0[3].borrow(), *store.get(&key4).unwrap()); + assert_eq!(unsafe { &*fibroute.0[0].get() }, store.get(&key1).unwrap()); + assert_eq!(unsafe { &*fibroute.0[1].get() }, store.get(&key2).unwrap()); + assert_eq!(unsafe { &*fibroute.0[2].get() }, store.get(&key3).unwrap()); + assert_eq!(unsafe { &*fibroute.0[3].get() }, store.get(&key4).unwrap()); + println!("{fibroute:#?}"); // Attempt to create a Fibroute with a key for which no fibgroup exists: should fail let key = NhopKey::with_address(&IpAddr::from_str("9.0.0.1").unwrap()); - let fibroute = FibRoute::from_nhopkeys(&store, &[key1, key2, key]); - assert!(matches!(fibroute, Err(FibError::NoFibGroup(ref key)))); + println!("Creating fibroute from non-registered nhop key {key}..."); + let fibroute = FibRoute::from_nhopkeys(&store, &[key1, key2, key.clone()]); println!("{fibroute:#?}"); + assert!(fibroute.is_err_and(|e| e == FibError::NoFibGroup(key))); } } diff --git a/routing/src/fib/fibobjects.rs b/routing/src/fib/fibobjects.rs index 8234836d0..0371b2629 100644 --- a/routing/src/fib/fibobjects.rs +++ b/routing/src/fib/fibobjects.rs @@ -10,6 +10,7 @@ use net::interface::InterfaceIndex; use std::net::IpAddr; #[derive(Debug, Default, Clone, Ord, PartialOrd, Eq, PartialEq)] +#[cfg_attr(test, derive(Hash))] /// An `EgressObject` indicates the interface over which a packet /// has to be sent and, optionally, a next-hop ip address. If /// no address is provided, ND/ARP is required. @@ -84,6 +85,11 @@ impl FibGroup { entries: vec![entry], } } + #[must_use] + pub(crate) fn drop_fibgroup() -> FibGroup { + FibGroup::with_entry(FibEntry::drop_fibentry()) + } + /// Add a [`FibEntry`] to a [`FibGroup`] pub fn add(&mut self, entry: FibEntry) { self.entries.push(entry); @@ -124,6 +130,8 @@ impl FibGroup { } #[derive(Debug, Default, Clone, Ord, PartialOrd, Eq, PartialEq)] +#[cfg_attr(test, derive(Hash))] + /// A Fib entry is made of a sequence of [`PktInstruction`] s to be executed for an IP packet /// in order to forward it. pub struct FibEntry { @@ -213,6 +221,7 @@ impl FibEntry { } #[derive(Clone, Default, Debug, Ord, PartialOrd, Eq, PartialEq)] +#[cfg_attr(test, derive(Hash))] #[allow(unused)] /// A `PktInstruction` represents an action to be performed by the packet processor on a packet. pub enum PktInstruction { diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index 9bdc6e938..7913fd646 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -3,120 +3,148 @@ //! The Fib table, which allows accessing all FIBs -use crate::fib::fibtype::{FibId, FibReader, FibWriter}; +use crate::rib::vrf::VrfId; +use crate::{ + RouterError, + fib::fibtype::{FibKey, FibReader, FibReaderFactory, FibWriter}, +}; + use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; use net::vxlan::Vni; use std::collections::BTreeMap; +use std::rc::Rc; use std::sync::Arc; -use tracing::{debug, error}; +#[allow(unused)] +use tracing::{debug, error, info, warn}; -#[derive(Clone, Default, Debug)] -pub struct FibTable(BTreeMap>); +#[derive(Debug)] +struct FibTableEntry { + id: FibKey, + factory: FibReaderFactory, +} +impl FibTableEntry { + const fn new(id: FibKey, factory: FibReaderFactory) -> Self { + Self { id, factory } + } +} + +#[derive(Default, Debug)] +pub struct FibTable { + version: u64, + entries: BTreeMap>, +} impl FibTable { - /// Add a new Fib ([`FibReader`]) - pub fn add_fib(&mut self, id: FibId, fibr: Arc) { - debug!("Creating FIB with id {id}"); - self.0.insert(id, fibr); - } - /// Del a Fib ([`FibReader`]) - pub fn del_fib(&mut self, id: &FibId) { - debug!("Deleting FIB reference with id {id}"); - self.0.remove(id); - } - /// Register a Fib ([`FibReader`]) with a given [`Vni`] - /// This allows finding the Fib from the [`Vni`] - pub fn register_by_vni(&mut self, id: &FibId, vni: &Vni) { - if let Some(fibr) = self.get_fib(id) { - self.0.insert(FibId::Vni(*vni), fibr.clone()); - debug!("Registered Fib {id} with vni {vni}"); + /// Register a `Fib` by adding a `FibReaderFactory` for it + fn add_fib(&mut self, id: FibKey, factory: FibReaderFactory) { + info!("Registering Fib with id {id} in the FibTable"); + self.entries + .insert(id, Arc::new(FibTableEntry::new(id, factory))); + } + /// Delete a `Fib`, by unregistering a `FibReaderFactory` for it + fn del_fib(&mut self, id: &FibKey) { + info!("Unregistering Fib with id {id} from the FibTable"); + self.entries.remove(id); + } + /// Register an existing `Fib` with a given [`Vni`]. + /// This allows looking up a Fib (`FibReaderFactory`) from a [`Vni`] + fn register_by_vni(&mut self, id: &FibKey, vni: Vni) { + if let Some(entry) = self.get_entry(id) { + self.entries + .insert(FibKey::from_vni(vni), Arc::clone(entry)); + info!("Registering Fib with id {id} in the FibTable with vni {vni}"); } else { - error!("Failed to register Fib {id} with vni {vni}: no fib"); + error!("Failed to register Fib {id} with vni {vni}: no fib with id {id} found"); } } - - /// Remove any entry referring to the given Vni - pub fn unregister_vni(&mut self, vni: &Vni) { - let id = FibId::Vni(*vni); - debug!("Deleting FIB reference with id {id}"); - self.0.remove(&id); + /// Remove any entry keyed by a [`Vni`] + fn unregister_vni(&mut self, vni: Vni) { + let key = FibKey::from_vni(vni); + info!("Unregistered key = {key} from the FibTable"); + self.entries.remove(&key); } - /// Get the [`FibReader`] for the fib with the given [`FibId`] + /// Get the entry for the fib with the given [`FibKey`] #[must_use] - pub fn get_fib(&self, id: &FibId) -> Option<&Arc> { - self.0.get(id) + fn get_entry(&self, key: &FibKey) -> Option<&Arc> { + self.entries.get(key) } - /// Number of [`FibReader`]s in the fib table + + /// Get a [`FibReader`] for the fib with the given [`FibKey`]. This method should only + /// be called in the existing tests, as it creates a new `FibReader` on every call. #[must_use] - pub fn len(&self) -> usize { - self.0.len() + #[cfg(test)] + pub fn get_fib(&self, key: &FibKey) -> Option { + self.get_entry(key).map(|entry| entry.factory.handle()) } - /// Tell if fib table is empty + + /// Number of entries in this table #[must_use] - pub fn is_empty(&self) -> bool { - self.0.is_empty() - } - /// Provide iterator - pub fn iter(&self) -> impl Iterator)> { - self.0.iter() + #[cfg(test)] + pub(crate) fn len(&self) -> usize { + self.entries.len() } } enum FibTableChange { - Add((FibId, Arc)), - Del(FibId), - RegisterByVni((FibId, Vni)), + Add((FibKey, FibReaderFactory)), + Del(FibKey), + RegisterByVni((FibKey, Vni)), UnRegisterVni(Vni), } impl Absorb for FibTable { fn absorb_first(&mut self, change: &mut FibTableChange, _: &Self) { + self.version = self.version.wrapping_add(1); match change { - FibTableChange::Add((id, fibr)) => self.add_fib(*id, fibr.clone()), + FibTableChange::Add((id, factory)) => self.add_fib(*id, factory.clone()), FibTableChange::Del(id) => self.del_fib(id), - FibTableChange::RegisterByVni((id, vni)) => self.register_by_vni(id, vni), - FibTableChange::UnRegisterVni(vni) => self.unregister_vni(vni), + FibTableChange::RegisterByVni((id, vni)) => self.register_by_vni(id, *vni), + FibTableChange::UnRegisterVni(vni) => self.unregister_vni(*vni), } } - fn drop_first(self: Box) {} - fn sync_with(&mut self, first: &Self) { - *self = first.clone(); - } + fn sync_with(&mut self, _first: &Self) {} } pub struct FibTableWriter(WriteHandle); impl FibTableWriter { #[must_use] pub fn new() -> (FibTableWriter, FibTableReader) { - let (write, read) = left_right::new::(); + let (mut write, read) = left_right::new::(); + write.publish(); /* avoid needing to impl sync_with() so that no need to impl Clone */ (FibTableWriter(write), FibTableReader(read)) } pub fn enter(&self) -> Option> { self.0.enter() } + #[must_use] + pub fn as_fibtable_reader(&self) -> FibTableReader { + FibTableReader(self.0.clone()) + } #[allow(clippy::arc_with_non_send_sync)] #[must_use] - pub fn add_fib(&mut self, id: FibId, vni: Option) -> (FibWriter, Arc) { - let (fibw, fibr) = FibWriter::new(id); - let fibr_arc = Arc::new(fibr); - self.0.append(FibTableChange::Add((id, fibr_arc.clone()))); + pub fn add_fib(&mut self, vrfid: VrfId, vni: Option) -> FibWriter { + let fibid = FibKey::from_vrfid(vrfid); + let (fibw, fibr) = FibWriter::new(fibid); + self.0.append(FibTableChange::Add((fibid, fibr.factory()))); if let Some(vni) = vni { - self.0.append(FibTableChange::RegisterByVni((id, vni))); + self.0.append(FibTableChange::RegisterByVni((fibid, vni))); } self.0.publish(); - (fibw, fibr_arc) + fibw } - pub fn register_fib_by_vni(&mut self, id: FibId, vni: Vni) { - self.0.append(FibTableChange::RegisterByVni((id, vni))); + pub fn register_fib_by_vni(&mut self, vrfid: VrfId, vni: Vni) { + let fibid = FibKey::from_vrfid(vrfid); + self.0.append(FibTableChange::RegisterByVni((fibid, vni))); self.0.publish(); } pub fn unregister_vni(&mut self, vni: Vni) { self.0.append(FibTableChange::UnRegisterVni(vni)); self.0.publish(); } - pub fn del_fib(&mut self, id: &FibId, vni: Option) { - self.0.append(FibTableChange::Del(*id)); + pub fn del_fib(&mut self, vrfid: VrfId, vni: Option) { + let fibid = FibKey::from_vrfid(vrfid); + self.0.append(FibTableChange::Del(fibid)); if let Some(vni) = vni { self.0.append(FibTableChange::UnRegisterVni(vni)); } @@ -136,10 +164,11 @@ impl FibTableReaderFactory { #[derive(Clone, Debug)] pub struct FibTableReader(ReadHandle); impl FibTableReader { - /// Access the fib table from its reader + #[must_use] pub fn enter(&self) -> Option> { self.0.enter() } + #[must_use] pub fn factory(&self) -> FibTableReaderFactory { FibTableReaderFactory(self.0.factory()) } @@ -147,3 +176,59 @@ impl FibTableReader { #[allow(unsafe_code)] unsafe impl Send for FibTableWriter {} + +/* + * Thread-local cache or readhandles for the fibtable + */ + +// declare thread-local cache for fibtable +use crate::fib::fibtype::Fib; +use left_right_tlcache::make_thread_local_readhandle_cache; +use left_right_tlcache::{ReadHandleCache, ReadHandleProvider}; +make_thread_local_readhandle_cache!(FIBTABLE_CACHE, FibKey, Fib); + +impl ReadHandleProvider for FibTable { + type Data = Fib; + type Key = FibKey; + fn get_factory( + &self, + key: &Self::Key, + ) -> Option<(&ReadHandleFactory, Self::Key, u64)> { + let entry = self.get_entry(key)?.as_ref(); + let factory = entry.factory.as_ref(); + Some((factory, entry.id, self.version)) + } + fn get_version(&self) -> u64 { + self.version + } + fn get_identity(&self, key: &Self::Key) -> Option { + self.get_entry(key).map(|entry| entry.id) + } + fn get_iter( + &self, + ) -> ( + u64, + impl Iterator, FibKey)>, + ) { + let iter = self + .entries + .iter() + .map(|(key, entry)| (*key, &*entry.factory.as_ref(), entry.as_ref().id)); + (self.version, iter) + } +} + +impl FibTableReader { + /// Main method for threads to get a reference to a FibReader from their thread-local cache. + /// Note 1: the cache stores `ReadHandle`'s. This method returns `FibReader` for convenience. This is zero cost + /// Note 2: we make this a method of [`FibTableReader`], as each thread is assumed to have its own read handle to the `FibTable`. + /// Note 3: we map ReadHandleCacheError to RouterError + pub fn get_fib_reader(&self, id: FibKey) -> Result, RouterError> { + let Some(fibtable) = self.enter() else { + warn!("Unable to access fib table!"); + return Err(RouterError::FibTableError); + }; + let rhandle = ReadHandleCache::get_reader(&FIBTABLE_CACHE, id, &*fibtable)?; + Ok(FibReader::rc_from_rc_rhandle(rhandle)) + } +} diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index dd56e6052..cc8c8f608 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -5,9 +5,11 @@ #![allow(clippy::collapsible_if)] -use left_right::{Absorb, ReadGuard, ReadHandle, WriteHandle}; -use std::cell::Ref; +use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; +use left_right_tlcache::Identity; use std::net::IpAddr; +use std::rc::Rc; +use std::{hash::Hash, sync::atomic::AtomicBool}; use lpm::prefix::{Ipv4Prefix, Ipv6Prefix, Prefix}; use lpm::trie::{PrefixMapTrie, TrieMap, TrieMapFactory}; @@ -22,50 +24,66 @@ use crate::rib::nexthop::NhopKey; use crate::rib::vrf::VrfId; #[allow(unused)] -use tracing::{debug, error, info, warn}; +use tracing::{debug, error, info, trace, warn}; -#[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] -/// An id we use to idenfify a FIB -pub enum FibId { +#[derive(Copy, Clone, Debug, Hash, Ord, PartialOrd, Eq, PartialEq)] +/// A type used to access a [`Fib`] or to identify it. +/// As an identifier, only the variant `FibKey::Id` is allowed. +pub enum FibKey { Unset, Id(VrfId), Vni(Vni), } -impl FibId { +impl FibKey { #[must_use] pub fn from_vrfid(vrfid: VrfId) -> Self { - FibId::Id(vrfid) + FibKey::Id(vrfid) } #[must_use] pub fn from_vni(vni: Vni) -> Self { - FibId::Vni(vni) + FibKey::Vni(vni) } #[must_use] pub fn as_u32(&self) -> u32 { match self { - FibId::Id(value) => *value, - FibId::Vni(value) => value.as_u32(), - FibId::Unset => unreachable!(), + FibKey::Id(value) => *value, + FibKey::Vni(value) => value.as_u32(), + FibKey::Unset => unreachable!(), } } } pub struct Fib { - id: FibId, + id: FibKey, routesv4: PrefixMapTrie, routesv6: PrefixMapTrie, groupstore: FibGroupStore, vtep: Vtep, + valid: AtomicBool, +} +impl Hash for Fib { + // We implement explicitly `std::hash::Hash` for `Fib` instead of deriving it because: + // - this avoids the need to implement/derive it for all internal components + // - it is actually not possible to do so since some types are defined externally (prefixes) + // - the Id suffices to identify them and the implementation is possibly faster. + fn hash(&self, state: &mut H) { + self.id.hash(state); + } +} +impl Identity for Fib { + fn identity(&self) -> FibKey { + self.get_id() + } } - impl Default for Fib { fn default() -> Self { let mut fib = Self { - id: FibId::Unset, + id: FibKey::Unset, routesv4: PrefixMapTrie::create(), routesv6: PrefixMapTrie::create(), groupstore: FibGroupStore::new(), vtep: Vtep::new(), + valid: AtomicBool::new(true), }; // default route let route = FibRoute::with_fibgroup(fib.groupstore.get_drop_fibgroup_ref()); @@ -79,16 +97,20 @@ pub type FibRouteV4Filter = Box bool>; pub type FibRouteV6Filter = Box bool>; impl Fib { - /// Set the [`FibId`] for this [`Fib`] - fn set_id(&mut self, id: FibId) { + /// Set the id for this [`Fib`] + fn set_id(&mut self, id: FibKey) { + if !matches!(id, FibKey::Id(_)) { + panic!("Attempting to set invalid Id of {id} to fib"); + } self.id = id; } #[must_use] - /// Get the [`FibId`] for this [`Fib`] - pub fn get_id(&self) -> FibId { - if self.id == FibId::Unset { - panic!("Hit fib with unset FibId!!"); + /// Get the id for this [`Fib`] + pub fn get_id(&self) -> FibKey { + if !matches!(self.id, FibKey::Id(_)) { + error!("Hit fib with invalid Id {}", self.id); + unreachable!() } self.id } @@ -103,11 +125,17 @@ impl Fib { /// Add a [`FibRoute`] fn build_add_fibroute(&mut self, prefix: Prefix, keys: &[NhopKey]) { - let Ok(route) = FibRoute::from_nhopkeys(&self.groupstore, keys) else { - error!("Failed to build fibroute for keys {keys:#?}"); + if keys.is_empty() { + error!("Rejecting fibroute creation: no keys provided"); return; - }; - self.add_fibroute(prefix, route); + } + match FibRoute::from_nhopkeys(&self.groupstore, keys) { + Ok(route) => { + debug_assert!(route.len() > 0); + self.add_fibroute(prefix, route); + } + Err(e) => error!("Failed to build fibroute for keys {keys:#?}: {e}"), + } } /// Delete the [`FibRoute`] for a prefix @@ -192,7 +220,7 @@ impl Fib { } /// Iterate over [`FibGroup`]s - pub fn group_iter(&self) -> impl Iterator> { + pub fn group_iter(&self) -> impl Iterator { self.groupstore.values() } @@ -234,36 +262,24 @@ impl Fib { /// However, instead of returning the entire [`FibRoute`], returns a single [`FibEntry`] out of /// those in the `FibGroup`s that make up the [`FibRoute`]. The entry selected is chosen by /// computing a hash on the invariant header fields of the IP and L4 headers. - #[must_use] - #[allow(clippy::cast_possible_truncation)] - pub fn lpm_entry( - &self, - packet: &Packet, - ) -> Option> { - let (_, entry) = self.lpm_entry_prefix(packet); - entry - } - - /// Same as `lpm_entry` but reporting prefix #[allow(clippy::cast_possible_truncation)] pub fn lpm_entry_prefix( &self, packet: &Packet, - ) -> (Prefix, Option>) { + ) -> (Prefix, &FibEntry) { if let Some(destination) = packet.ip_destination() { let (prefix, route) = self.lpm_with_prefix(&destination); - match route.len() { - 0 => { - warn!("Can't forward packet: no route to {prefix}. This is a bug."); - (prefix, None) - } - 1 => (prefix, route.get_fibentry(0)), - k => { - let entry_index = packet.packet_hash_ecmp(0, (k - 1) as u8); - debug!("Selected FibEntry {entry_index}/{k} to forward packet"); - (prefix, route.get_fibentry(entry_index as usize)) - } + let num_entries = route.len(); + if num_entries == 0 { + let bad = "Warning, hit route without fibgroups/entries. This is a bug."; + warn!("{bad}"); + panic!("{bad}"); + } + let mut entry_index = 0; + if num_entries > 1 { + entry_index = packet.packet_hash_ecmp(0, (num_entries - 1) as u8); } + (prefix, route.get_fibentry(entry_index as usize)) } else { error!("Failed to get destination IP address!"); unreachable!() @@ -278,25 +294,28 @@ enum FibChange { AddFibRoute((Prefix, Vec)), DelFibRoute(Prefix), SetVtep(Vtep), + Invalidate, } impl Absorb for Fib { fn absorb_first(&mut self, change: &mut FibChange, _: &Self) { match change { - FibChange::RegisterFibGroup((key, fibgroup)) => unsafe { + FibChange::RegisterFibGroup((key, fibgroup)) => { self.groupstore.add_mod_group(key, fibgroup.clone()); - }, + } FibChange::UnregisterFibGroup(key) => { self.groupstore.del(key); } FibChange::AddFibRoute((prefix, keys)) => self.build_add_fibroute(*prefix, keys), FibChange::DelFibRoute(prefix) => self.del_fibroute(*prefix), FibChange::SetVtep(vtep) => self.set_vtep(vtep), + FibChange::Invalidate => { + self.valid.store(false, std::sync::atomic::Ordering::SeqCst); + } } } - fn drop_first(self: Box) {} fn sync_with(&mut self, first: &Self) { - assert!(self.id != FibId::Unset); + assert!(self.id != FibKey::Unset); assert_eq!(self.id, first.id); debug!("Internal LR state for fib {} is now synced", self.id); } @@ -306,9 +325,9 @@ pub struct FibWriter(WriteHandle); impl FibWriter { /// create a fib, providing a writer and a reader #[must_use] - pub fn new(id: FibId) -> (FibWriter, FibReader) { + pub fn new(id: FibKey) -> (FibWriter, FibReader) { let (mut w, r) = left_right::new::(); - // Set the FibId in the read and write copies, created Fib::default() that sets it to FibId::Unset. + // Set the Id in the read and write copies, created Fib::default() that sets it to FibKey::Unset. unsafe { // It is safe to call raw_handle() and raw_write_handle() here let fib_rcopy = r.raw_handle().unwrap_or_else(|| unreachable!()).as_mut(); @@ -318,15 +337,12 @@ impl FibWriter { // this is needed to avoid needing to clone the fib w.publish(); } + info!("Created Fib with id {id}"); (FibWriter(w), FibReader(r)) } pub fn enter(&self) -> Option> { self.0.enter() } - #[must_use] - pub fn get_id(&self) -> Option { - self.0.enter().map(|fib| fib.get_id()) - } pub fn register_fibgroup(&mut self, key: &NhopKey, fibgroup: &FibGroup, publish: bool) { self.0 .append(FibChange::RegisterFibGroup((key.clone(), fibgroup.clone()))); @@ -341,6 +357,10 @@ impl FibWriter { } } pub fn add_fibroute(&mut self, prefix: Prefix, keys: Vec, publish: bool) { + if keys.is_empty() { + error!("Rejected route to prefix {prefix}: no next-hop keys provided"); + return; + } self.0.append(FibChange::AddFibRoute((prefix, keys))); if publish { self.0.publish(); @@ -365,19 +385,128 @@ impl FibWriter { pub fn as_fibreader(&self) -> FibReader { FibReader::new(self.0.clone()) } + pub fn destroy(mut self) { + self.0.append(FibChange::Invalidate); + self.publish(); + // this is sanity and should not be needed + while self.as_fibreader().enter().is_some() {} + } } #[derive(Clone, Debug)] +#[repr(transparent)] pub struct FibReader(ReadHandle); impl FibReader { #[must_use] pub fn new(rhandle: ReadHandle) -> Self { FibReader(rhandle) } + #[must_use] + #[inline] + pub fn is_valid(&self) -> bool { + match self.0.enter() { + Some(fib) => fib.valid.load(std::sync::atomic::Ordering::Acquire), + None => false, + } + } pub fn enter(&self) -> Option> { - self.0.enter() + self.0.enter().map(|fib| { + if fib.valid.load(std::sync::atomic::Ordering::Acquire) { + Some(fib) + } else { + None + } + })? + } + #[inline(always)] + /// Convert Rc> -> FibReader + /// We need this conversion because the cache of read-handles stores ReadHandle, + /// but the FibTable provides FibReader. + pub(crate) fn rc_from_rc_rhandle(rc: Rc>) -> Rc { + unsafe { + // the conversion is safe because FibReader is a transparent wrapper of ReadHandle + let ptr = Rc::into_raw(rc) as *const Self; + Rc::from_raw(ptr) + } } - pub fn get_id(&self) -> Option { - self.0.enter().map(|fib| fib.get_id()) + pub fn get_id(&self) -> Option { + self.enter().map(|fib| fib.get_id()) + } + #[must_use] + pub fn factory(&self) -> FibReaderFactory { + FibReaderFactory(self.0.factory()) + } + + /// Get a reference to the `FibRoute` best matching address `destination` + /// Return value: this method may only return None if the `FibReader` cannot + /// be entered, which should only occur if the `FibWriter` has been dropped. + /// + /// Safety: the `FibRoute` reference returned will remain valid and immutable + /// as long as the returned `ReadGuard` is alive. + pub fn lpm_route(&self, destination: IpAddr) -> Option> { + self.enter() + .map(|guard| ReadGuard::map(guard, |fib| fib.lpm(&destination))) + } + + /// Same as `FibReader::lpm_route`, but reporting the longest `Prefix` matched. + /// Notes: no prefix will be returned if we fail to "enter" in the fib. + pub fn lpm_route_with_prefix( + &self, + destination: IpAddr, + ) -> Option<(Prefix, ReadGuard<'_, FibRoute>)> { + let mut prefix = Prefix::root_v4(); + let guarded_route = self.enter().map(|guard| { + ReadGuard::map(guard, |fib| { + let (hit, route) = fib.lpm_with_prefix(&destination); + prefix = hit; + route + }) + }); + guarded_route.map(|guarded_route| (prefix, guarded_route)) + } + + /// Similar to `FibReader::lpm_route_with_prefix()`, but receing a `Packet` and selecting + /// a `FibEntry` based on a hash of the packet if more than one FibEntries could be used to + /// forward the packet + pub fn lpm_entry_prefix( + &self, + packet: &Packet, + ) -> Option<(Prefix, ReadGuard<'_, FibEntry>)> { + let mut prefix = Prefix::root_v4(); + let guarded_entry = self.enter().map(|guard| { + ReadGuard::map(guard, |fib| { + let (hit, entry) = fib.lpm_entry_prefix(packet); + prefix = hit; + entry + }) + }); + guarded_entry.map(|guarded_entry| (prefix, guarded_entry)) + } +} + +// make FibReader a zero-cost wrap of ReadHandle +impl AsRef for ReadHandle { + #[inline] + fn as_ref(&self) -> &FibReader { + // safe because FibReader is repr(transparent) wrapper of ReadHandle + unsafe { &*(self as *const ReadHandle as *const FibReader) } + } +} +#[derive(Debug, Clone)] +#[repr(transparent)] +pub struct FibReaderFactory(pub(crate) ReadHandleFactory); + +// make FibReaderFactory a zero-cost wrap of ReadHandleFactory +impl AsRef> for FibReaderFactory { + #[inline] + fn as_ref(&self) -> &ReadHandleFactory { + &self.0 + } +} + +impl FibReaderFactory { + #[must_use] + pub fn handle(&self) -> FibReader { + FibReader(self.0.handle()) } } diff --git a/routing/src/fib/mod.rs b/routing/src/fib/mod.rs index 9e7549480..da56a8854 100644 --- a/routing/src/fib/mod.rs +++ b/routing/src/fib/mod.rs @@ -7,6 +7,7 @@ pub mod fibgroupstore; pub mod fibobjects; pub mod fibtable; pub mod fibtype; +mod test; use tracectl::trace_target; trace_target!("fib", LevelFilter::WARN, &["pipeline"]); diff --git a/routing/src/fib/test.rs b/routing/src/fib/test.rs new file mode 100644 index 000000000..6803e3af4 --- /dev/null +++ b/routing/src/fib/test.rs @@ -0,0 +1,415 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Fib module tests + +#![cfg(test)] +use concurrency::concurrency_mode; + +#[concurrency_mode(std)] +mod tests { + use crate::fib::fibobjects::FibEntry; + use crate::fib::fibobjects::FibGroup; + use crate::fib::fibobjects::PktInstruction; + use crate::fib::fibtable::FibTableWriter; + use crate::fib::fibtype::FibKey; + use crate::fib::fibtype::FibWriter; + use crate::rib::nexthop::NhopKey; + + use net::ip::NextHeader; + use net::packet::Packet; + use net::packet::test_utils::build_test_ipv4_packet_with_transport; + use net::udp::UdpPort; + use net::{buffer::TestBuffer, interface::InterfaceIndex}; + + use lpm::prefix::{IpAddr, Prefix}; + + use rand::Rng; + use rand::rngs::ThreadRng; + use std::str::FromStr; + use std::sync::Arc; + use std::sync::atomic::AtomicU16; + use std::thread; + use std::thread::Builder; + use std::time::{Duration, Instant}; + use std::{collections::HashMap, collections::HashSet, sync::atomic::Ordering}; + + use crate::fib::fibgroupstore::tests::build_fib_entry_egress; + use crate::fib::fibgroupstore::tests::build_fibgroup; + + /// An object that contains a preset collection of entries and fibgroups. + /// This is shared by fuzzer and workers. Fuzzer uses it to randomly select fibgroups to update the fib. + /// Workers use it to check what they read. + struct RandomRouter { + entries: HashSet, + fibgroups: HashMap, + } + impl RandomRouter { + fn load() -> Self { + let mut entries = HashSet::new(); + let e1 = build_fib_entry_egress(1, "10.0.1.1", "eth1"); + let e2 = build_fib_entry_egress(2, "10.0.2.1", "eth2"); + let e3 = build_fib_entry_egress(3, "10.0.3.1", "eth3"); + let e4 = build_fib_entry_egress(4, "10.0.3.4", "eth4"); + let e5 = build_fib_entry_egress(5, "10.0.3.5", "eth5"); + let e6 = build_fib_entry_egress(6, "10.0.3.6", "eth6"); + let e7 = build_fib_entry_egress(7, "10.0.3.7", "eth7"); + let e8 = build_fib_entry_egress(8, "10.0.3.8", "eth8"); + + entries.insert(e1.clone()); + entries.insert(e2.clone()); + entries.insert(e3.clone()); + entries.insert(e4.clone()); + entries.insert(e5.clone()); + entries.insert(e6.clone()); + entries.insert(e7.clone()); + entries.insert(e8.clone()); + + let mut fibgroups = HashMap::new(); + fibgroups.insert(1, build_fibgroup(&[e1.clone(), e2.clone()])); + fibgroups.insert(2, build_fibgroup(&[e4.clone()])); + fibgroups.insert(3, build_fibgroup(&[e7.clone()])); + fibgroups.insert(4, build_fibgroup(&[e8.clone()])); + fibgroups.insert(5, build_fibgroup(&[e5.clone()])); + fibgroups.insert(6, build_fibgroup(&[e5.clone(), e6.clone()])); + fibgroups.insert(7, build_fibgroup(&[e3.clone()])); + fibgroups.insert( + 8, + build_fibgroup(&[e3.clone(), e4.clone(), e5.clone(), e6.clone(), e7.clone()]), + ); + + Self { entries, fibgroups } + } + fn random_pick_fibgroup(&self, rng: &mut ThreadRng) -> &FibGroup { + let chosen = rng.random_range(1..=self.fibgroups.len()); + self.fibgroups.get(&chosen).unwrap() + } + } + + fn get_entry_interface_index(entry: &FibEntry) -> InterfaceIndex { + if let PktInstruction::Egress(egress) = &entry.instructions[0] { + egress.ifindex.unwrap() + } else { + unreachable!() + } + } + fn test_packet() -> Packet { + let mut packet = build_test_ipv4_packet_with_transport(64, Some(NextHeader::UDP)).unwrap(); + let destination = IpAddr::from_str("192.168.1.1").expect("Bad dst ip address"); + packet.set_ip_destination(destination).unwrap(); + packet + } + fn mutate_packet(rng: &mut ThreadRng, packet: &mut Packet) { + packet + .set_udp_destination_port(UdpPort::new_checked(rng.random_range(20..=100)).unwrap()) + .unwrap(); + } + + // Test the concurrency of a SINGLE fib. NUM_WORKERS workers perform LPM lookups on a single FIB for + // a test packet while another thread fuzzes the route to forward the packet, by removing the route + // or aggressively changing the fibgroup (and fib entries) used for the prefix of that route. + + #[test] + fn test_concurrency_fib() { + const NUM_PACKETS: u64 = 1000_00; + const NUM_WORKERS: u16 = 4; + + // sync main thread - worker thread(s) + let done = Arc::new(AtomicU16::new(0)); + + // create fib with writer and readers + let (mut fibw, fibr) = FibWriter::new(FibKey::Id(0)); + + // the prefix of the route that will be used to process a packet + let prefix = Prefix::from("192.168.1.0/24"); + + // the next-hop - we keep this throughout + let nhkey = NhopKey::with_address(&IpAddr::from_str("7.0.0.1").unwrap()); + + // spawn a reader thread (worker) + let rfactory = fibr.factory(); + let worker_done = done.clone(); + + // build shared database and randomizer + let randomrouter = Arc::new(RandomRouter::load()); + + /*************************/ + /* worker thread closure */ + /*************************/ + let worker_code = Arc::new(move |randomrouter: Arc| { + let fibreader = rfactory.handle(); + let mut prefix_hits = 0u64; // number of times lpm yielded prefix 192.168.1.0/24 + let mut drop_hits = 0u64; // number of times lpm yielded 0.0.0.0/0 - Drop + let mut stats: HashMap = HashMap::new(); + let mut rng = rand::rng(); + + // The packet each worker will repeatedly try to forward using the fib. + // The worker mutates the packet (udp dst port) to alter the hash outcome + // so that distinct fib entries of the route hit get a chance of being selected. + let mut packet = test_packet(); + + loop { + if let Some(fib) = fibreader.enter() { + mutate_packet(&mut rng, &mut packet); + + // do LPM + let (hit, entry) = fib.lpm_entry_prefix(&packet); + if hit != prefix { + assert_eq!(hit, Prefix::root_v4()); // fuzzer can remove route + drop_hits += 1; + continue; + } + assert!(randomrouter.entries.contains(entry)); + prefix_hits += 1; + + // get interface from fib entry. This is to keep some stats to see + // if the main thread really had a chance to fuzz the fib once the test ends. + let ifindex = get_entry_interface_index(entry); + stats + .entry(ifindex) + .and_modify(|counter| *counter += 1) + .or_insert(0); + + // stop after processing NUM_PACKETS with the target route + if prefix_hits >= NUM_PACKETS { + println!(" -- Worker --"); + println!("target hits: {prefix_hits} pkts"); + println!("drop hits : {drop_hits} pkts"); + println!("Stats: {stats:#?}"); + worker_done.fetch_add(1, Ordering::Relaxed); + break; + } + } + } + }); + + /* Spawn workers */ + for n in 1..=NUM_WORKERS { + let rand_router = randomrouter.clone(); + let value = worker_code.clone(); + let _ = Builder::new() + .name(format!("WORKER-{n}")) + .spawn(move || value(rand_router.clone())) + .unwrap(); + } + + /********************************************************/ + /* main thread: loops fuzzing / deleting a single route */ + /********************************************************/ + let mut updates = 0u64; + let mut route_adds = 0u64; + let mut route_replaces = 0u64; + let mut route_deletions = 0u64; + let mut rng = rand::rng(); + loop { + // randomly select a fib-group, register it and add the route to the + // target prefix. Fuzzer changes the fibgroup on every loop. + let fibgroup = randomrouter.random_pick_fibgroup(&mut rng); + fibw.register_fibgroup(&nhkey, fibgroup, true); + if route_adds == 0 { + fibw.add_fibroute(prefix, vec![nhkey.clone()], true); + route_adds += 1; + } + + // every 10 loops replace route and fibgroup + if updates % 10 == 0 { + fibw.register_fibgroup(&nhkey, fibgroup, false); + fibw.add_fibroute(prefix, vec![nhkey.clone()], false); + route_replaces += 1; + fibw.publish(); + } + if updates % 101 == 0 { + fibw.del_fibroute(prefix); + fibw.publish(); + route_deletions += 1; + } + + // iterations + updates += 1; + + // stop when all workers are done + if done.load(Ordering::Relaxed) == NUM_WORKERS { + println!("All workers finished!"); + break; + } + } + println!(" --- Fib fuzzer stats ---"); + println!("fibgroup updates:{updates}"); + println!("route adds: {route_adds}"); + println!("route replaces: {route_replaces}"); + println!("route deletions: {route_deletions}"); + } + + // Test the concurrency of a SINGLE fib within a FIBTABLE. NUM_WORKERS workers perform LPM lookups + // on a single FIB for a test packet while another thread fuzzes the route to forward the packet, by removing the route + // or aggressively changing the fibgroup (and fib entries) used for the prefix of that route. The fuzzer in this + // test also removes the FIB and adds it again. The workers use a thread-local cache to access the FIB. + #[test] + fn test_concurrency_fibtable() { + // number of threads looking up fibtable + const NUM_WORKERS: u16 = 7; + const NUM_PACKETS: u64 = 1_000_000; + const TENTH: u64 = NUM_PACKETS / 10; + + // create fibtable (empty, without any fib) + let (mut fibtw, fibtr) = FibTableWriter::new(); + let fibtrfactory = fibtr.factory(); + + // prefix to be hit by packets + let prefix = Prefix::from("192.168.1.0/24"); + + // shared counter of workers that finished + let done = Arc::new(AtomicU16::new(0)); + + let vrfid = 1; + + /* Spawn workers: each has its own reader for the fibtable */ + for n in 1..=NUM_WORKERS { + let fibtr = fibtrfactory.handle(); + let worker_done = done.clone(); + + Builder::new() + .name(format!("WORKER-{n}")) + .spawn(move || { + let mut rng = rand::rng(); + let mut packet = test_packet(); + let mut prefix_hits: u64 = 0; + let mut other_hits: u64 = 0; + let mut nofibs: u64 = 0; + let mut nofib_enter: u64 = 0; + loop { + mutate_packet(&mut rng, &mut packet); + if let Ok(fib) = fibtr.get_fib_reader(FibKey::Id(vrfid)) { + if let Some(fib) = fib.enter() { + let (hit, _fibentry) = fib.lpm_entry_prefix(&packet); + if hit == prefix { + prefix_hits += 1; + if prefix_hits % TENTH == 0 { + println!("Worker {n} is {} % done", prefix_hits * 100 / NUM_PACKETS); + } + + if prefix_hits >= NUM_PACKETS { + println!("=== Worker {n} finished ===="); + println!("Stats:"); + println!(" {prefix_hits:>8} packets hit {prefix}"); + println!(" {other_hits:>8} packets hit other prefix (0.0.0.0/0)"); + println!(" {nofibs:>8} packets found no fib"); + println!(" {nofib_enter:>8} packets found fib but could not enter"); + worker_done.fetch_add(1, Ordering::Relaxed); + break; + } + } else { + other_hits += 1; + } + } else { + nofib_enter += 1; + } + } else { + nofibs += 1; + } + } + }) + .unwrap(); + } + + /*****************************************************************/ + /* main thread (fuzzer): adds / deletes route / fibgroup and fib */ + /*****************************************************************/ + let nhkey = NhopKey::with_address(&IpAddr::from_str("7.0.0.1").unwrap()); + let mut rng = rand::rng(); + let randomrouter = RandomRouter::load(); + let mut updates = 0u64; + + let mut fibw = Some(fibtw.add_fib(vrfid, None)); + let fibgroup = randomrouter.random_pick_fibgroup(&mut rng); + if let Some(fibw) = &mut fibw { + fibw.register_fibgroup(&nhkey, fibgroup, true); + fibw.add_fibroute(prefix, vec![nhkey.clone()], true); + } + let start = Instant::now(); + loop { + if fibw.is_none() { + fibw = Some(fibtw.add_fib(vrfid, None)); + } + if let Some(fibw) = &mut fibw { + if updates % 100 == 0 { + let fibgroup = randomrouter.random_pick_fibgroup(&mut rng); + fibw.register_fibgroup(&nhkey, fibgroup, true); + fibw.add_fibroute(prefix, vec![nhkey.clone()], true); + } + if updates % 150 == 0 { + fibw.del_fibroute(prefix); + fibw.publish(); + } + } + + if updates % 50 == 0 && fibw.is_some() { + fibtw.del_fib(1, None); + thread::sleep(Duration::from_millis(15)); + if true { + // fib gets deleted here + let fib = fibw.take(); + fib.unwrap().destroy(); + } + } + + // iterations + updates += 1; + + // stop when all workers are done + if done.load(Ordering::Relaxed) == NUM_WORKERS { + println!("All workers finished!"); + break; + } + } + let duration = start.elapsed(); + println!("Test duration: {:?}", duration); + } + + // Tests fib reader utilities returning guards + #[test] + fn test_fib_guards() { + // create fib + let (mut fibw, fibr) = FibWriter::new(FibKey::Id(0)); + + // add a route + let prefix = Prefix::from("192.168.1.0/24"); + let nhkey = NhopKey::with_address(&IpAddr::from_str("7.0.0.1").unwrap()); + let e1 = build_fib_entry_egress(1, "10.0.1.1", "eth1"); + let fibgroup1 = build_fibgroup(&[e1.clone()]); + fibw.register_fibgroup(&nhkey, &fibgroup1, false); + fibw.add_fibroute(prefix, vec![nhkey.clone()], false); + fibw.publish(); + + // use the fib: do lpm for some destination and get a route + let destination = IpAddr::from_str("192.168.1.1").expect("Bad dst ip address"); + let route = &*fibr.lpm_route(destination).unwrap(); + assert!(route.has_entries()); + assert_eq!(route.len(), fibgroup1.len()); + assert!(route.iter().any(|g| g == &fibgroup1)); + + // use again the fib with a packet + let packet = test_packet(); + let (matched, entry1) = fibr.lpm_entry_prefix(&packet).unwrap(); + assert_eq!(matched, prefix); + assert!(fibgroup1.entries().contains(&*entry1)); + + // attempt to modify the route by modifying the fibgroup + let e2 = build_fib_entry_egress(2, "10.0.2.1", "eth2"); + let fibgroup2 = build_fibgroup(&[e2.clone()]); + fibw.register_fibgroup(&nhkey, &fibgroup2, false); + fibw.add_fibroute(prefix, vec![nhkey.clone()], false); + fibw.publish(); + + // a second query to the fib yields the updated value. This is counter-intuitive but correct, + // because of the number of readers we have and the fact that the original fib guard was forgotten. + let (_, entry2) = fibr.lpm_entry_prefix(&packet).unwrap(); + assert_eq!(&*entry2, &e2); + + // ... but the guard to the entry wasn't. + assert_eq!(&*entry1, &e1); + + // Additional queries while holding the guards would cause the writer to block. + // We can't test this here since there's a single thread and it would block forever. + } +} diff --git a/routing/src/interfaces/iftable.rs b/routing/src/interfaces/iftable.rs index 5d4ca7245..19573d7eb 100644 --- a/routing/src/interfaces/iftable.rs +++ b/routing/src/interfaces/iftable.rs @@ -4,7 +4,7 @@ //! A table of interfaces use crate::errors::RouterError; -use crate::fib::fibtype::{FibId, FibReader}; +use crate::fib::fibtype::{FibKey, FibReader}; use crate::interfaces::interface::{IfAddress, IfState, Interface, RouterInterfaceConfig}; use ahash::RandomState; use std::collections::HashMap; @@ -151,9 +151,9 @@ impl IfTable { } ////////////////////////////////////////////////////////////////////// - /// Detach all interfaces attached to the Vrf whose fib has id `FibId` + /// Detach all interfaces attached to the Vrf whose fib has the given Id ////////////////////////////////////////////////////////////////////// - pub fn detach_interfaces_from_vrf(&mut self, fibid: FibId) { + pub fn detach_interfaces_from_vrf(&mut self, fibid: FibKey) { for iface in self.by_index.values_mut() { iface.detach_from_fib(fibid); } diff --git a/routing/src/interfaces/iftablerw.rs b/routing/src/interfaces/iftablerw.rs index cb22287c0..417b24e66 100644 --- a/routing/src/interfaces/iftablerw.rs +++ b/routing/src/interfaces/iftablerw.rs @@ -4,7 +4,7 @@ //! Interface to the interfaces module use crate::errors::RouterError; -use crate::fib::fibtype::FibId; +use crate::fib::fibtype::FibKey; use crate::fib::fibtype::FibReader; use crate::interfaces::iftable::IfTable; use crate::interfaces::interface::{IfAddress, IfState, RouterInterfaceConfig}; @@ -14,13 +14,15 @@ use left_right::ReadHandleFactory; use left_right::{Absorb, ReadGuard, ReadHandle, WriteHandle}; use net::interface::InterfaceIndex; +use tracing::debug; + enum IfTableChange { Add(RouterInterfaceConfig), Mod(RouterInterfaceConfig), Del(InterfaceIndex), Attach((InterfaceIndex, FibReader)), Detach(InterfaceIndex), - DetachFromVrf(FibId), + DetachFromVrf(FibKey), AddIpAddress((InterfaceIndex, IfAddress)), DelIpAddress((InterfaceIndex, IfAddress)), UpdateOpState((InterfaceIndex, IfState)), @@ -168,8 +170,10 @@ impl IfTableWriter { self.0.append(IfTableChange::Detach(ifindex)); self.0.publish(); } - pub fn detach_interfaces_from_vrf(&mut self, fibid: FibId) { - self.0.append(IfTableChange::DetachFromVrf(fibid)); + pub fn detach_interfaces_from_vrf(&mut self, vrfid: VrfId) { + debug!("Scheduling detach of interfaces from vrf {vrfid}"); + self.0 + .append(IfTableChange::DetachFromVrf(FibKey::Id(vrfid))); self.0.publish(); } } diff --git a/routing/src/interfaces/interface.rs b/routing/src/interfaces/interface.rs index 16dd63ebe..ac03bf71e 100644 --- a/routing/src/interfaces/interface.rs +++ b/routing/src/interfaces/interface.rs @@ -5,7 +5,7 @@ #![allow(clippy::collapsible_if)] -use crate::fib::fibtype::{FibId, FibReader}; +use crate::fib::fibtype::{FibKey, FibReader}; use crate::rib::vrf::VrfId; use net::eth::mac::Mac; use net::interface::{InterfaceIndex, Mtu}; @@ -210,7 +210,7 @@ impl Interface { /// Tell if an [`Interface`] is attached to a Fib with the given Id ////////////////////////////////////////////////////////////////// #[must_use] - pub fn is_attached_to_fib(&self, fibid: FibId) -> bool { + pub fn is_attached_to_fib(&self, fibid: FibKey) -> bool { if let Some(Attachment::VRF(fibr)) = &self.attachment { fibr.get_id() == Some(fibid) } else { @@ -221,7 +221,7 @@ impl Interface { ////////////////////////////////////////////////////////////////// /// Detach an [`Interface`] from VRF if the associated fib has the given id ////////////////////////////////////////////////////////////////// - pub fn detach_from_fib(&mut self, fibid: FibId) { + pub fn detach_from_fib(&mut self, fibid: FibKey) { self.attachment.take_if(|attachment| { if let Attachment::VRF(fibr) = &attachment { if fibr.get_id() == Some(fibid) { diff --git a/routing/src/interfaces/mod.rs b/routing/src/interfaces/mod.rs index 70208789d..7929f0b20 100644 --- a/routing/src/interfaces/mod.rs +++ b/routing/src/interfaces/mod.rs @@ -10,7 +10,7 @@ pub mod interface; #[cfg(test)] pub mod tests { use crate::RouterError; - use crate::fib::fibtype::{FibId, FibWriter}; + use crate::fib::fibtype::{FibKey, FibWriter}; use crate::interfaces::iftable::IfTable; use crate::interfaces::iftablerw::{IfTableReader, IfTableWriter}; use crate::interfaces::interface::{ @@ -113,7 +113,7 @@ pub mod tests { let mut iftable = build_test_iftable(); /* Create a fib for the vrf created next */ - let (fibw, _fibr) = FibWriter::new(FibId::Id(0)); + let (fibw, _fibr) = FibWriter::new(FibKey::Id(0)); /* Create a VRF for that fib */ let vrf_cfg = RouterVrfConfig::new(0, "default"); diff --git a/routing/src/rib/rib2fib.rs b/routing/src/rib/rib2fib.rs index e04982932..4f3e01b58 100644 --- a/routing/src/rib/rib2fib.rs +++ b/routing/src/rib/rib2fib.rs @@ -4,7 +4,7 @@ //! Rib to fib route processor #[allow(unused)] -use tracing::{debug, warn}; +use tracing::{debug, trace, warn}; use crate::evpn::RmacStore; use crate::rib::encapsulation::{Encapsulation, VxlanEncapsulation}; @@ -103,23 +103,24 @@ impl Nhop { } ////////////////////////////////////////////////////////////////////// - /// Determine instructions for a next-hop and build its fibgroup + /// Determine instructions for a next-hop and build its `FibGroup`. + /// Returns true if the `Fibgroup` associated to a next-hop changed. ////////////////////////////////////////////////////////////////////// pub(crate) fn set_fibgroup(&self, rstore: &RmacStore) -> bool { // determine nhop pkt instructions. This is independent of the routing table self.build_nhop_instructions(rstore); + // build the fibgroup for a next-hop. This requires the nhop to be resolved // and its resolvers too, and that these have packet instructions up to date let fibgroup = self.build_nhop_fibgroup(); - let changed = fibgroup != *self.fibgroup.borrow(); + let changed = fibgroup != *(self.fibgroup.borrow()); if changed { - // FIXME(fredi): we need a way of enabling these logs at runtime - //debug!("Fibgroup for nhop {self}\nchanged!"); - //debug!("\nold:\n{}", self.fibgroup.borrow()); - //debug!("\nnew:\n{}", fibgroup); + trace!("Fibgroup for nhop {self}\nchanged. Will replace.."); + trace!("\nold:\n{}", self.fibgroup.borrow()); + trace!("\nnew:\n{}", fibgroup); self.fibgroup.replace(fibgroup); } else { - //debug!("Fibgroup for nhop {self} did NOT change"); + trace!("Fibgroup for nhop {self} did NOT change"); } changed } diff --git a/routing/src/rib/vrf.rs b/routing/src/rib/vrf.rs index da6905f17..6b70dc24b 100644 --- a/routing/src/rib/vrf.rs +++ b/routing/src/rib/vrf.rs @@ -15,7 +15,7 @@ use crate::pretty_utils::Frame; use super::nexthop::{FwAction, Nhop, NhopKey, NhopStore}; use crate::evpn::{RmacStore, Vtep}; -use crate::fib::fibtype::{FibId, FibReader, FibWriter}; +use crate::fib::fibtype::{FibKey, FibReader, FibWriter}; use lpm::prefix::{Ipv4Prefix, Ipv6Prefix, Prefix}; use lpm::trie::{PrefixMapTrie, TrieMap, TrieMapFactory}; use net::route::RouteTableId; @@ -256,9 +256,9 @@ impl Vrf { } //////////////////////////////////////////////////////////////////////// - /// Get the `FibId` of the Fib associated to this [`Vrf`] + /// Get the id (`FibKey`) of the Fib associated to this [`Vrf`] ///////////////////////////////////////////////////////////////////////// - pub fn get_vrf_fibid(&self) -> Option { + pub fn get_vrf_fibid(&self) -> Option { self.get_vrf_fibr()?.get_id() } @@ -267,7 +267,7 @@ impl Vrf { ///////////////////////////////////////////////////////////////////////// pub fn set_vni(&mut self, vni: Vni) { self.vni = Some(vni); - debug!("Associated vni {vni} to Vrf '{}'", self.name); + debug!("Set vni {vni} to Vrf {} ({})", self.vrfid, self.name); } ///////////////////////////////////////////////////////////////////////// @@ -438,9 +438,9 @@ impl Vrf { if let Some(fibw) = &mut self.fibw { let mut count = 0; for nhop in changed { - let fibgroup = &*nhop.fibgroup.borrow(); + let fibgroup = &nhop.fibgroup.borrow(); debug!("Updating fib group for nhop {}...", nhop.key); - fibw.register_fibgroup(&nhop.as_ref().key, fibgroup, false); + fibw.register_fibgroup(&nhop.key, fibgroup, false); count += 1; } if count > 0 { diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index c992e8106..66ef38675 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -7,7 +7,7 @@ use crate::RouterError; use crate::evpn::RmacStore; use crate::fib::fibtable::FibTableWriter; -use crate::fib::fibtype::FibId; +use crate::fib::fibtype::FibKey; use crate::interfaces::iftablerw::IfTableWriter; use crate::rib::vrf::{RouterVrfConfig, Vrf, VrfId}; @@ -72,7 +72,7 @@ impl VrfTable { } /* create fib */ - let (fibw, _) = self.fibtablew.add_fib(FibId::Id(vrf.vrfid), vrf.vni); + let fibw = self.fibtablew.add_fib(vrf.vrfid, vrf.vni); vrf.set_fibw(fibw); /* store */ @@ -95,21 +95,21 @@ impl VrfTable { return Ok(()); /* vrf already has that vni */ } // No vrf has the requested vni, including the vrf with id vrfId. - // However the vrf with id VrfId may have another vni associated, - - /* remove vni from vrf if it has one */ + // However the vrf with id VrfId may have another vni associated. + // So, unset any vni that the vrf may have associated first. self.unset_vni(vrfid)?; - /* set the vni to the vrf */ + /* lookup vrf */ let vrf = self.get_vrf_mut(vrfid)?; + + /* set the vni to the vrf */ vrf.set_vni(vni); - /* register vni */ + /* register vni as key for vrf vrfid in the vrf table */ self.by_vni.insert(vni, vrfid); - /* register fib */ - self.fibtablew - .register_fib_by_vni(FibId::from_vrfid(vrfid), vni); + /* make fib accessible from vni in the fib table */ + self.fibtablew.register_fib_by_vni(vrfid, vni); Ok(()) } @@ -118,13 +118,18 @@ impl VrfTable { /// removes it from the `by_vni` map. It also unindexes the vrf's FIB by the vni. /////////////////////////////////////////////////////////////////////////////////// pub fn unset_vni(&mut self, vrfid: VrfId) -> Result<(), RouterError> { + debug!("Unsetting any vni configuration for vrf {vrfid}.."); let vrf = self.get_vrf_mut(vrfid)?; + + // check if vrf has vni configured if let Some(vni) = vrf.vni { - debug!("Removing vni {vni} from vrf {vrfid}..."); + debug!("Vrf {vrfid} has vni {vni} associated. Removing..."); vrf.vni.take(); self.by_vni.remove(&vni); self.fibtablew.unregister_vni(vni); - debug!("Vrf with Id {vrfid} no longer has a VNI associated"); + debug!("Vrf with Id {vrfid} no longer has a vni {vni} associated"); + } else { + debug!("Vrf {vrfid} has no vni configured"); } Ok(()) } @@ -135,38 +140,33 @@ impl VrfTable { /// a [`Vni`] configured or it does but the internal state is not the expected. /////////////////////////////////////////////////////////////////////////////////// pub fn check_vni(&self, vrfid: VrfId) -> Result<(), RouterError> { + // lookup vrf: should be found let vrf = self.get_vrf(vrfid)?; + + // Vrf must have a vni configured: should succeed let Some(vni) = &vrf.vni else { return Err(RouterError::Internal("No vni found")); }; + + // must be able to look up [`Vrf`] by vni and we must find a [`Vrf`] with same [`VrfId`] let found = self.get_vrfid_by_vni(*vni)?; if found != vrfid { error!("Vni {vni} refers to vrfid {found} and not {vrfid}"); return Err(RouterError::Internal("Inconsistent vni mapping")); } - // look up fib -- from fibtable - let fibtable = self - .fibtablew - .enter() - .ok_or(RouterError::Internal("Failed to access fib table"))?; - let fib = fibtable - .get_fib(&FibId::Vni(*vni)) - .ok_or(RouterError::Internal("No fib for vni found"))?; - let fib = fib - .enter() - .ok_or(RouterError::Internal("Unable to read fib"))?; - let found_fibid = fib.get_id(); - - // look up fib - direct (TODO: make fib mandatory for VRF) - if let Some(fibw) = &vrf.fibw { - let fib = fibw - .enter() - .ok_or(RouterError::Internal("Unable to access Fib for vrf"))?; - let fibid = fib.get_id(); - if fibid != found_fibid { - error!("Expected: {found_fibid} found: {fibid}"); - return Err(RouterError::Internal("Inconsistent fib id found!")); - } + + // access fib table: it should always be possible for us to enter the fib table since + // the vrf table owns the `FibTableWriter`. Also, as a reader, we should be able to see the latest + // changes since we published and would otherwise got blocked. To test for correctness, we check + // the two keys via which this vrf should be accessible and from our thread-local cache. + let fibtabler = self.fibtablew.as_fibtable_reader(); + fibtabler.get_fib_reader(FibKey::from_vrfid(vrfid))?; + + let fibid = FibKey::from_vrfid(vrfid); // The id it should have + if let Some(key) = fibtabler.get_fib_reader(FibKey::from_vni(*vni))?.get_id() + && key != fibid + { + return Err(RouterError::Internal("Inconsistent fib id found!")); } Ok(()) } @@ -179,17 +179,21 @@ impl VrfTable { vrfid: VrfId, iftablew: &mut IfTableWriter, ) -> Result<(), RouterError> { - debug!("Removing VRF with vrfid {vrfid}..."); - let Some(vrf) = self.by_id.remove(&vrfid) else { + // remove the vrf from the vrf table + debug!("Removing VRF {vrfid}..."); + let Some(mut vrf) = self.by_id.remove(&vrfid) else { error!("No vrf with id {vrfid} exists"); return Err(RouterError::NoSuchVrf); }; + + // detach interfaces + iftablew.detach_interfaces_from_vrf(vrfid); + // delete the corresponding fib - if vrf.fibw.is_some() { - let fib_id = FibId::Id(vrfid); - debug!("Requesting deletion of vrf {vrfid} FIB. Id is '{fib_id}'"); - self.fibtablew.del_fib(&fib_id, vrf.vni); - iftablew.detach_interfaces_from_vrf(fib_id); + if let Some(fibw) = vrf.fibw.take() { + debug!("Deleting Fib for vrf {vrfid} from the FibTable"); + self.fibtablew.del_fib(vrfid, vrf.vni); + fibw.destroy(); } // if the VRF had a vni assigned, unregister it @@ -197,6 +201,7 @@ impl VrfTable { debug!("Unregistering vni {vni}"); self.by_vni.remove(&vni); } + debug!("Vrf {vrfid} has been removed"); Ok(()) } @@ -223,6 +228,7 @@ impl VrfTable { /// Remove all of the VRFs with status `Deleted` ////////////////////////////////////////////////////////////////// pub fn remove_deleted_vrfs(&mut self, iftablew: &mut IfTableWriter) { + debug!("Removing deletable vrfs..."); self.remove_vrfs(Vrf::can_be_deleted, iftablew); } @@ -230,6 +236,7 @@ impl VrfTable { /// Remove all of the VRFs with status `Deleting` ////////////////////////////////////////////////////////////////// pub fn remove_deleting_vrfs(&mut self, iftablew: &mut IfTableWriter) { + debug!("Removing vrfs with deleting status..."); self.remove_vrfs(Vrf::is_deleting, iftablew); } @@ -420,7 +427,7 @@ impl VrfTable { mod tests { use super::*; use crate::fib::fibobjects::{EgressObject, PktInstruction}; - use crate::fib::fibtype::FibId; + use crate::fib::fibtype::FibKey; use crate::interfaces::tests::build_test_iftable_left_right; use crate::pretty_utils::Frame; use crate::rib::encapsulation::Encapsulation; @@ -522,7 +529,7 @@ mod tests { .expect("Should succeed"); let ift = iftr.enter().unwrap(); let eth0 = ift.get_interface(idx2).expect("Should find interface"); - assert!(eth0.is_attached_to_fib(FibId::Id(vrfid))); + assert!(eth0.is_attached_to_fib(FibKey::Id(vrfid))); println!("{}", *ift); drop(ift); @@ -534,7 +541,7 @@ mod tests { .expect("Should succeed"); let ift = iftr.enter().unwrap(); let eth1 = ift.get_interface(idx3).expect("Should find interface"); - assert!(eth1.is_attached_to_fib(FibId::Id(vrfid))); + assert!(eth1.is_attached_to_fib(FibKey::Id(vrfid))); println!("{}", *ift); drop(ift); @@ -546,7 +553,7 @@ mod tests { .expect("Should succeed"); let ift = iftr.enter().unwrap(); let eth2 = ift.get_interface(idx4).expect("Should find interface"); - assert!(eth2.is_attached_to_fib(FibId::Id(vrfid))); + assert!(eth2.is_attached_to_fib(FibKey::Id(vrfid))); println!("{}", *ift); drop(ift); @@ -558,7 +565,7 @@ mod tests { .expect("Should succeed"); let ift = iftr.enter().unwrap(); let iface = ift.get_interface(idx5).expect("Should find interface"); - assert!(iface.is_attached_to_fib(FibId::Id(vrfid))); + assert!(iface.is_attached_to_fib(FibKey::Id(vrfid))); println!("{}", *ift); drop(ift); @@ -576,10 +583,10 @@ mod tests { println!("{vrftable}"); let ift = iftr.enter().unwrap(); let iface = ift.get_interface(idx4).expect("Should be there"); - assert!(!iface.is_attached_to_fib(FibId::Id(vrfid))); + assert!(!iface.is_attached_to_fib(FibKey::Id(vrfid))); assert!(iface.attachment.is_none()); let iface = ift.get_interface(idx5).expect("Should be there"); - assert!(!iface.is_attached_to_fib(FibId::Id(vrfid))); + assert!(!iface.is_attached_to_fib(FibKey::Id(vrfid))); assert!(iface.attachment.is_none()); println!("{}", *ift); drop(ift); @@ -608,10 +615,10 @@ mod tests { ); let ift = iftr.enter().unwrap(); let eth0 = ift.get_interface(idx2).expect("Should be there"); - assert!(!eth0.is_attached_to_fib(FibId::Id(vrfid))); + assert!(!eth0.is_attached_to_fib(FibKey::Id(vrfid))); assert!(eth0.attachment.is_none()); let eth1 = ift.get_interface(idx3).expect("Should be there"); - assert!(!eth1.is_attached_to_fib(FibId::Id(vrfid))); + assert!(!eth1.is_attached_to_fib(FibKey::Id(vrfid))); assert!(eth1.attachment.is_none()); println!("{}", *ift); drop(ift); @@ -664,11 +671,10 @@ mod tests { .expect("Should find vrfid by vni"); assert_eq!(id, vrfid); debug!("\n{vrftable}"); + if let Some(fibtable) = fibtr.enter() { - let fib = fibtable.get_fib(&FibId::from_vrfid(vrfid)); - assert!(fib.is_some()); - let fib = fibtable.get_fib(&FibId::from_vni(vni)); - assert!(fib.is_some()); + assert!(fibtable.get_fib(&FibKey::from_vrfid(vrfid)).is_some()); + assert!(fibtable.get_fib(&FibKey::from_vni(vni)).is_some()); } debug!("━━━━Test: Unset vni {vni} from the vrf"); @@ -681,36 +687,72 @@ mod tests { assert!((id.is_err_and(|e| e == RouterError::NoSuchVrf))); debug!("\n{vrftable}"); if let Some(fibtable) = fibtr.enter() { - let fib = fibtable.get_fib(&FibId::from_vrfid(vrfid)); - assert!(fib.is_some()); - let fib = fibtable.get_fib(&FibId::from_vni(vni)); - assert!(fib.is_none()); + assert!(fibtable.get_fib(&FibKey::from_vrfid(vrfid)).is_some()); + assert!(fibtable.get_fib(&FibKey::from_vni(vni)).is_none()); } } #[traced_test] #[test] fn vrf_table_deletions() { + debug!("━━━━Test: Create testing interface table"); + let (mut iftw, iftr) = build_test_iftable_left_right(); + debug!("━━━━Test: Create vrf table"); let (fibtw, fibtr) = FibTableWriter::new(); - let (mut iftw, iftr) = build_test_iftable_left_right(); let mut vrftable = VrfTable::new(fibtw); + debug!("━━━━Test: Check fib access for vrf default"); + if let Some(fibtable) = fibtr.enter() { + let fibkey = FibKey::from_vrfid(0); + let fibr = fibtable.get_fib(&fibkey).unwrap(); + let fib = fibr.enter().unwrap(); + assert_eq!(fib.as_ref().get_id(), fibkey); + } + let vrfid = 999; let vni = mk_vni(3000); - debug!("━━━━Test: Add a VRF without Vni"); + debug!("━━━━Test: Add a VRF with id {vrfid} but no Vni"); let vrf_cfg = RouterVrfConfig::new(vrfid, "VPC-1"); vrftable.add_vrf(&vrf_cfg).expect("Should be created"); + assert_eq!(vrftable.len(), 2); // default is always there + debug!("\n{vrftable}"); - debug!("━━━━Test: Associate VNI {vni}"); + debug!("━━━━Test: Check fib access for vrf {vrfid}"); + if let Some(fibtable) = fibtr.enter() { + // check that fib is accessible from vrfid + let fibkey = FibKey::from_vrfid(vrfid); + let fibr = fibtable.get_fib(&fibkey).unwrap(); + let fib = fibr.enter().unwrap(); + assert_eq!(fib.as_ref().get_id(), fibkey); + assert_eq!(fibtable.as_ref().len(), 2); + } + + debug!("━━━━Test: Associate vni {vni} to vrf {vrfid}"); vrftable.set_vni(vrfid, vni).expect("Should succeed"); - assert_eq!(vrftable.len(), 2); // default is always there debug!("\n{vrftable}"); + debug!("━━━━Test: Check fib access for vrf {vrfid} and vni {vni}"); + if let Some(fibtable) = fibtr.enter() { + // check that fib continues to be accessible via vrfid + let fibkey = FibKey::from_vrfid(vrfid); + let fibr = fibtable.get_fib(&fibkey).unwrap(); + let fib = fibr.enter().unwrap(); + assert_eq!(fib.as_ref().get_id(), fibkey); + + // check that fib is accessible from vni + let fibkey = FibKey::from_vni(vni); + let fibr = fibtable.get_fib(&fibkey).unwrap(); + let fib = fibr.enter().unwrap(); + assert_eq!(fib.as_ref().get_id(), FibKey::from_vrfid(vrfid)); + } + debug!("━━━━Test: deleting removed VRFs: nothing should be removed"); vrftable.remove_deleted_vrfs(&mut iftw); assert_eq!(vrftable.len(), 2); // default is always there + assert_eq!(fibtr.enter().unwrap().len(), 3); + debug!("\n{vrftable}"); debug!("━━━━Test: Get interface from iftable"); let idx = InterfaceIndex::try_new(2).unwrap(); @@ -720,7 +762,7 @@ mod tests { debug!("\n{}", *iftable); } - debug!("━━━━Test: Attach interface to vrf"); + debug!("━━━━Test: Attach interface to vrf {vrfid}"); iftw.attach_interface_to_vrf(idx, vrfid, &vrftable) .expect("Should succeed"); if let Some(iftable) = iftr.enter() { @@ -734,18 +776,22 @@ mod tests { vrf.set_status(VrfStatus::Deleted); debug!("\n{vrftable}"); - debug!("━━━━Test: remove vrfs marked as deleted again - VPC-1 vrf should be gone"); + debug!("━━━━Test: remove vrfs marked as deleted: VPC-1 vrf should be gone"); vrftable.remove_deleted_vrfs(&mut iftw); assert_eq!(vrftable.len(), 1, "should be gone"); + assert_eq!( + fibtr.enter().unwrap().len(), + 1, + "only default fib should remain" + ); - // check fib table + debug!("━━━━Test: Check that no fib is accessible vrfid:{vrfid} nor vni:{vni}"); if let Some(fibtable) = fibtr.enter() { - let fib = fibtable.get_fib(&FibId::from_vrfid(vrfid)); - assert!(fib.is_none()); - let fib = fibtable.get_fib(&FibId::from_vni(vni)); - assert!(fib.is_none()); - assert_eq!(fibtable.len(), 1); + assert!(fibtable.get_fib(&FibKey::from_vrfid(vrfid)).is_none()); + assert!(fibtable.get_fib(&FibKey::from_vni(vni)).is_none()); + assert!(fibtable.get_fib(&FibKey::from_vrfid(0)).is_some()); } + debug!("━━━━Test: Interface {idx} should no longer be attached"); if let Some(iftable) = iftr.enter() { let iface = iftable.get_interface(idx).expect("Should be there"); assert!(iface.attachment.is_none(), "Should have been detached");