diff --git a/Cargo.lock b/Cargo.lock index 3e0cbb5bd..97afc9879 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1264,6 +1264,7 @@ dependencies = [ "chrono", "dataplane-cli", "dataplane-config", + "dataplane-left-right-tlcache", "dataplane-lpm", "dataplane-net", "dataplane-tracectl", diff --git a/dataplane/src/packet_processor/ipforward.rs b/dataplane/src/packet_processor/ipforward.rs index 1e4bf4e3d..95819850b 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,34 +70,22 @@ 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 */ let (prefix, fibentry) = fib.lpm_entry_prefix(packet); if let Some(fibentry) = &fibentry { - debug!("{nfi}: Packet hits prefix {prefix} in fib {fibid}"); + debug!("{nfi}: Packet hits prefix {prefix} in fib {fibkey}"); debug!("{nfi}: Entry is:\n{fibentry}"); /* decrement packet TTL, unless the packet is for us */ @@ -110,7 +97,7 @@ impl IpForwarder { } } /* execute instructions according to FIB */ - self.packet_exec_instructions(&fibtr, packet, fibentry, fib.get_vtep()); + self.packet_exec_instructions(packet, fibentry, fib.get_vtep()); } else { debug!("Could not get fib group for {prefix}. Will drop packet..."); packet.done(DoneReason::InternalFailure); @@ -121,7 +108,6 @@ impl IpForwarder { fn packet_exec_instruction_local( &self, packet: &mut Packet, - fibtable: &FibTable, _ifindex: InterfaceIndex, /* we get it from metadata */ ) { let nfi = &self.name; @@ -134,14 +120,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 +321,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 +328,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 +338,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/src/lib.rs b/left-right-tlcache/src/lib.rs index 2b1efcade..6476c3217 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -60,6 +60,16 @@ pub trait ReadHandleProvider: Sync { /// 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, @@ -118,6 +128,7 @@ impl, K: PartialEq> ReadHandleEntry { pub struct ReadHandleCache { handles: RefCell, RandomState>>, + refresh_version: RefCell, // version when last refresh mas made } impl ReadHandleCache where @@ -128,6 +139,7 @@ where pub fn new() -> Self { Self { handles: RefCell::new(HashMap::with_hasher(RandomState::with_seed(0))), + refresh_version: RefCell::new(0), } } pub fn get_reader( @@ -181,8 +193,121 @@ where 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 @@ -346,6 +471,19 @@ mod tests { 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] @@ -573,4 +711,69 @@ mod tests { // 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..f984ccce8 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 } 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/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..4ebe46e14 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -5,9 +5,13 @@ #![allow(clippy::collapsible_if)] -use left_right::{Absorb, ReadGuard, ReadHandle, WriteHandle}; +use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; +use left_right_tlcache::Identity; + use std::cell::Ref; +use std::hash::Hash; use std::net::IpAddr; +use std::rc::Rc; use lpm::prefix::{Ipv4Prefix, Ipv6Prefix, Prefix}; use lpm::trie::{PrefixMapTrie, TrieMap, TrieMapFactory}; @@ -24,44 +28,58 @@ use crate::rib::vrf::VrfId; #[allow(unused)] use tracing::{debug, error, info, 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, } - +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(), @@ -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 } @@ -294,9 +316,8 @@ impl Absorb for Fib { FibChange::SetVtep(vtep) => self.set_vtep(vtep), } } - 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 +327,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,13 +339,14 @@ 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 { + 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) { @@ -368,6 +390,7 @@ impl FibWriter { } #[derive(Clone, Debug)] +#[repr(transparent)] pub struct FibReader(ReadHandle); impl FibReader { #[must_use] @@ -377,7 +400,49 @@ impl FibReader { pub fn enter(&self) -> Option> { self.0.enter() } - pub fn get_id(&self) -> Option { + #[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()) } + #[must_use] + pub fn factory(&self) -> FibReaderFactory { + FibReaderFactory(self.0.factory()) + } +} + +// 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/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/vrf.rs b/routing/src/rib/vrf.rs index da6905f17..0db143d87 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); } ///////////////////////////////////////////////////////////////////////// diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index c992e8106..b99627ed2 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,17 @@ impl VrfTable { vrfid: VrfId, iftablew: &mut IfTableWriter, ) -> Result<(), RouterError> { - debug!("Removing VRF with vrfid {vrfid}..."); + // remove the vrf from the vrf table + debug!("Removing VRF {vrfid}..."); let Some(vrf) = self.by_id.remove(&vrfid) else { error!("No vrf with id {vrfid} exists"); return Err(RouterError::NoSuchVrf); }; // 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); + debug!("Deleting Fib for vrf {vrfid} from the FibTable"); + self.fibtablew.del_fib(vrfid, vrf.vni); + iftablew.detach_interfaces_from_vrf(vrfid); } // if the VRF had a vni assigned, unregister it @@ -197,6 +197,7 @@ impl VrfTable { debug!("Unregistering vni {vni}"); self.by_vni.remove(&vni); } + debug!("Vrf {vrfid} has been removed"); Ok(()) } @@ -223,6 +224,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 +232,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 +423,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 +525,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 +537,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 +549,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 +561,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 +579,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 +611,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 +667,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 +683,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: 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}"); + 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 +758,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 +772,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");