diff --git a/net/src/eth/mac.rs b/net/src/eth/mac.rs index ea7b5bc44a..e7bef0f689 100644 --- a/net/src/eth/mac.rs +++ b/net/src/eth/mac.rs @@ -3,7 +3,7 @@ //! Mac address type and logic. -use arrayvec::ArrayVec; +use arrayvec::{ArrayString, ArrayVec}; use std::{ fmt::{Debug, Display, LowerHex, UpperHex}, str::FromStr, @@ -184,11 +184,15 @@ impl Mac { impl Display for Mac { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use std::fmt::Write as _; // bring in scope + // MAC always renders as 17 chars + let mut buf = ArrayString::<17>::new(); write!( - f, - "{:<02x}:{:<02x}:{:<02x}:{:<02x}:{:<02x}:{:<02x}", + buf, + "{:02x}:{:02x}:{:02x}:{:02x}:{:02x}:{:02x}", self.0[0], self.0[1], self.0[2], self.0[3], self.0[4], self.0[5] - ) + )?; + f.pad(&buf) } } diff --git a/routing/src/cli/display.rs b/routing/src/cli/display.rs index d5af4e8216..53531c0184 100644 --- a/routing/src/cli/display.rs +++ b/routing/src/cli/display.rs @@ -33,6 +33,7 @@ use crate::evpn::{RmacEntry, RmacStore, Vtep}; use chrono::DateTime; use common::cliprovider::{Heading, line}; + use lpm::prefix::{IpPrefix, Ipv4Prefix, Ipv6Prefix}; use lpm::trie::{PrefixMapTrie, TrieMap}; use net::vxlan::Vni; @@ -118,6 +119,9 @@ impl Display for NhopKey { impl Display for Nhop { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}", self.key)?; + if self.invalid.get() { + write!(f, " (INVALID)")?; + } fmt_nhop_resolvers(f, self, 2) } } @@ -592,29 +596,42 @@ impl Display for IfTableAddress<'_> { //========================= Rmac Store ================================// macro_rules! RMAC_TBL_FMT { () => { - " {:<5} {:<20} {:<18}" + " {:<5} {:<20} {:<18} {:<8}" }; } fn fmt_rmac_heading(f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { writeln!( f, "{}", - format_args!(RMAC_TBL_FMT!(), "vni", "address", "mac") + format_args!(RMAC_TBL_FMT!(), "vni", "address", "mac", "status") ) } impl Display for RmacEntry { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let valid = if self.is_stale() { "stale" } else { "ok" }; write!( f, "{}", - format_args!(RMAC_TBL_FMT!(), self.vni.as_u32(), self.address, self.mac) + format_args!( + RMAC_TBL_FMT!(), + self.vni.as_u32(), + self.address, + self.mac, + valid + ) ) } } impl Display for RmacStore { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - Heading(format!("Router macs ({})", self.len())).fmt(f)?; + Heading(format!( + "Router macs (entries: {} stale: {})", + self.len(), + self.stale() + )) + .fmt(f)?; + fmt_rmac_heading(f)?; for rmac in self.values() { writeln!(f, "{rmac}")?; diff --git a/routing/src/evpn/rmac.rs b/routing/src/evpn/rmac.rs index 1c948cd790..e777edb885 100644 --- a/routing/src/evpn/rmac.rs +++ b/routing/src/evpn/rmac.rs @@ -8,33 +8,51 @@ use net::eth::mac::Mac; use net::vxlan::Vni; use std::collections::{HashMap, hash_map::Entry}; use std::net::IpAddr; -use tracing::debug; +use std::time::{Duration, Instant}; +use tracing::{debug, warn}; -#[derive(Debug, Eq, Hash, PartialEq)] +#[derive(Eq, PartialEq, Clone)] pub struct RmacEntry { pub address: IpAddr, pub mac: Mac, pub vni: Vni, + pub stale_t: Option, // instant when the rmac was deleted } impl RmacEntry { #[allow(unused)] fn new(vni: Vni, address: IpAddr, mac: Mac) -> Self { - Self { address, mac, vni } + Self { + address, + mac, + vni, + stale_t: None, + } + } + #[must_use] + pub fn is_stale(&self) -> bool { + self.stale_t.is_some() } } -#[derive(Debug)] /// Type that represents a collection of EVPN Rmac - IP mappings, per Vni -pub struct RmacStore(HashMap<(IpAddr, Vni), RmacEntry, RandomState>); +pub struct RmacStore { + table: HashMap<(IpAddr, Vni), RmacEntry, RandomState>, + stale: usize, // the number of stale entries +} #[allow(clippy::new_without_default)] impl RmacStore { + const MAX_STALE_TIME: Duration = Duration::from_secs(10); + ////////////////////////////////////////////////////////////////// /// Create rmac table ////////////////////////////////////////////////////////////////// #[must_use] pub(crate) fn new() -> Self { - Self(HashMap::with_hasher(RandomState::with_seed(0))) + Self { + table: HashMap::with_hasher(RandomState::with_seed(0)), + stale: 0, + } } ////////////////////////////////////////////////////////////////// @@ -43,48 +61,75 @@ impl RmacStore { #[cfg(test)] fn add_rmac(&mut self, vni: Vni, address: IpAddr, mac: Mac) -> Option { let rmac = RmacEntry::new(vni, address, mac); - self.0.insert((address, vni), rmac) + self.table.insert((address, vni), rmac) } ////////////////////////////////////////////////////////////////// - /// Identical to `add_rmac`, but getting the entry as param + /// Add a `RmacEntry` to the rmac store. This method never fails. ////////////////////////////////////////////////////////////////// pub fn add_rmac_entry(&mut self, entry: RmacEntry) { let vni = entry.vni; let mac = entry.mac; let address = entry.address; - if self.0.insert((entry.address, entry.vni), entry).is_some() { - debug!("Updated rmac for vni={vni} ip={address} to {mac}"); + if let Some(old) = self.table.insert((entry.address, entry.vni), entry) { + if old.mac == mac { + debug!("Refreshed rmac for vni:{vni} ip:{address} as {mac}"); + } else { + debug!( + "Changed rmac for vni:{vni} ip:{address} {} -> {mac}", + old.mac + ); + } + if let Some(stale_t) = &old.stale_t { + debug!("The rmac was stale for {}s", stale_t.elapsed().as_secs()); + self.stale = self.stale.saturating_sub(1); + } } else { - debug!("Registered rmac, vni={vni} ip={address} mac={mac}"); + debug!("Registered rmac {mac} for vni:{vni} ip:{address}"); } } ////////////////////////////////////////////////////////////////// - /// Delete an [`RmacEntry`]. The mac address must match (sanity) + /// Delete the [`RmacEntry`] for a given `IpAddr` and `Vni`. + /// To be removed, the corresponding entry in the rmac store must + /// be marked as stale. This method returns a [`RmacEntry`] + /// if an entry was deleted and `None` otherwise. ////////////////////////////////////////////////////////////////// - #[cfg(test)] - pub fn del_rmac(&mut self, vni: Vni, address: IpAddr, mac: Mac) { + pub fn del_rmac(&mut self, address: IpAddr, vni: Vni) -> Option { let key = (address, vni); - if let Entry::Occupied(o) = self.0.entry(key) { - if o.get().mac == mac { - self.0.remove_entry(&key); + + if let Entry::Occupied(o) = self.table.entry(key) { + if !o.get().is_stale() { + return None; + } + let (_key, deleted) = self.table.remove_entry(&key).unzip(); + debug_assert!(deleted.is_some()); + if deleted.is_some() { + self.stale = self.stale.saturating_sub(1); } + return deleted; } + warn!("Could not delete rmac entry for vni:{vni} address:{address}: not found"); + None } ////////////////////////////////////////////////////////////////// - /// Identical to `del_rmac`, but getting the entry as param + /// Mark a `RmacEntry` as stale instead of deleting it immediately. + /// The entry will be kept for a certain amount of time and be + /// removed afterwards unless replaced by another one. ////////////////////////////////////////////////////////////////// - pub fn del_rmac_entry(&mut self, entry: &RmacEntry) { + pub fn invalidate_rmac_entry(&mut self, entry: &RmacEntry) { let key = (entry.address, entry.vni); - if let Entry::Occupied(o) = self.0.entry(key) { - if o.get().mac == entry.mac { - self.0.remove_entry(&key); + if let Entry::Occupied(mut o) = self.table.entry(key) { + let current = o.get_mut(); + if current.mac == entry.mac && !current.is_stale() { debug!( - "Removed router-mac entry for vni: {} ip: {} mac: {}", - entry.vni, entry.address, entry.mac + "Marked rmac {} for vni:{} ip:{} as stale", + entry.mac, entry.vni, entry.address, ); + // recall time when it became stale + current.stale_t = Some(Instant::now()); + self.stale = self.stale.saturating_add(1); } } } @@ -94,14 +139,14 @@ impl RmacStore { ////////////////////////////////////////////////////////////////// #[must_use] pub fn get_rmac(&self, vni: Vni, address: IpAddr) -> Option<&RmacEntry> { - self.0.get(&(address, vni)) + self.table.get(&(address, vni)) } ////////////////////////////////////////////////////////////////// /// Immutable iterator over all [`RmacEntry`]ies ////////////////////////////////////////////////////////////////// pub fn values(&self) -> impl Iterator { - self.0.values() + self.table.values() } ////////////////////////////////////////////////////////////////// @@ -110,13 +155,63 @@ impl RmacStore { #[allow(clippy::len_without_is_empty)] #[must_use] pub fn len(&self) -> usize { - self.0.len() + self.table.len() + } + + #[must_use] + ////////////////////////////////////////////////////////////////// + /// number of stale rmac entries + ////////////////////////////////////////////////////////////////// + pub(crate) fn stale(&self) -> usize { + self.stale + } + + ////////////////////////////////////////////////////////////////// + /// flush all stale `RmacEntry`s kept for more than `Self::MAX_STALE_TIME`. + /// Returns a list of `Vni`s that had rmacs removed + ////////////////////////////////////////////////////////////////// + pub fn flush_stale_rmacs(&mut self) -> Vec { + if self.stale == 0 { + // nothing to do if no stale rmacs + return vec![]; + } + // collect the keys of entries that have been stale for more than `Self::MAX_STALE_TIME` seconds + let stale: Vec<_> = self + .table + .values() + .filter_map(|e| { + if let Some(instant) = e.stale_t + && instant.elapsed() > Self::MAX_STALE_TIME + { + Some((e.address, e.vni)) + } else { + None + } + }) + .collect(); + + // there may be stale entries, but not old enough + if stale.is_empty() { + return vec![]; + } + + debug!("Flushing {} router mac entries", stale.len()); + let mut vnis = std::collections::HashSet::new(); + for key in &stale { + if let Some(deleted) = self.del_rmac(key.0, key.1) { + vnis.insert(deleted.vni); + } + } + + let vnis = vnis.into_iter().collect::>(); + debug!("Removed rmacs for vnis: {vnis:?}"); + vnis } } #[cfg(test)] pub(crate) mod tests { - use super::RmacStore; + use super::{RmacEntry, RmacStore}; use crate::evpn::vtep::Vtep; use crate::rib::vrf::tests::mk_addr; use net::eth::mac::Mac; @@ -159,68 +254,68 @@ pub(crate) mod tests { let remote = mk_addr("7.0.0.1"); - store.add_rmac( + // create 3 rmacs + let rmac1 = RmacEntry::new( new_vni(3001), remote, Mac::from([0x0, 0x0, 0x0, 0x0, 0x0, 0x01]), ); - store.add_rmac( + let rmac2 = RmacEntry::new( new_vni(3002), remote, Mac::from([0x0, 0x0, 0x0, 0x0, 0x0, 0x02]), ); - store.add_rmac( + let rmac3 = RmacEntry::new( new_vni(3003), remote, Mac::from([0x0, 0x0, 0x0, 0x0, 0x0, 0x03]), ); - assert_eq!(store.0.len(), 3); + + // add to store + store.add_rmac_entry(rmac1.clone()); + store.add_rmac_entry(rmac2.clone()); + store.add_rmac_entry(rmac3.clone()); + assert_eq!(store.len(), 3); // add duplicate - store.add_rmac( - new_vni(3003), - remote, - Mac::from([0x0, 0x0, 0x0, 0x0, 0x0, 0x03]), - ); - assert_eq!(store.0.len(), 3, "Duplicate should not be stored"); + store.add_rmac_entry(rmac3.clone()); + assert_eq!(store.len(), 3, "Duplicate should not be stored"); - // remove first - store.del_rmac( - new_vni(3001), - remote, - Mac::from([0x0, 0x0, 0x0, 0x0, 0x0, 0x01]), - ); - assert_eq!(store.0.len(), 2, "Should have one less entry"); + // remove first: won't be deleted since it is not marked as stale + let deleted = store.del_rmac(rmac1.address, rmac1.vni); + assert!(deleted.is_none()); + assert_eq!(store.len(), 3, "Shouldn't be deleted"); - // remove second, but with wrong MAC - store.del_rmac( - new_vni(3002), - remote, - Mac::from([0xb, 0xa, 0xd, 0xb, 0xa, 0xd]), - ); - assert_eq!(store.0.len(), 2, "No entry should have been deleted"); + // invalidate (make stale) and remove: should succeed + store.invalidate_rmac_entry(&rmac1); + assert_eq!(store.stale(), 1); + let deleted = store.del_rmac(rmac1.address, rmac1.vni); + assert!(deleted.is_some()); + assert_eq!(store.len(), 2, "Should have one less entry"); + assert_eq!(store.stale(), 0); // get second - let r = store.get_rmac(new_vni(3002), remote); + let r = store.get_rmac(rmac2.vni, rmac2.address); assert!(r.is_some()); - assert_eq!(r.unwrap().mac, Mac::from([0x0, 0x0, 0x0, 0x0, 0x0, 0x02])); + assert_eq!(r.unwrap().mac, rmac2.mac); + assert_eq!(r.unwrap().vni, rmac2.vni); + assert_eq!(r.unwrap().address, rmac2.address); - // replace/update second - let r = store.add_rmac( - new_vni(3002), - remote, - Mac::from([0x11, 0x22, 0x33, 0x44, 0x55, 0x66]), - ); - assert!(r.is_some()); - assert_eq!(r.unwrap().mac, Mac::from([0x0, 0x0, 0x0, 0x0, 0x0, 0x02])); + // invalidate second + store.invalidate_rmac_entry(&rmac2); + assert_eq!(store.stale(), 1); + + // replace/update second: mac should be changed and entry no longer be invalid + let mut rmac2_modified_mac = rmac2.clone(); + rmac2_modified_mac.mac = Mac::from([0x11, 0x22, 0x33, 0x44, 0x55, 0x66]); + store.add_rmac_entry(rmac2_modified_mac.clone()); + assert_eq!(store.stale(), 0); // get second and check that its MAC was updated - let r = store.get_rmac(new_vni(3002), remote); + let r = store.get_rmac(rmac2.vni, rmac2.address); assert!(r.is_some()); - assert_eq!( - r.unwrap().mac, - Mac::from([0x11, 0x22, 0x33, 0x44, 0x55, 0x66]) - ); + assert_eq!(r.unwrap().mac, rmac2_modified_mac.mac); + assert!(!r.unwrap().is_stale()); } #[test] diff --git a/routing/src/rib/nexthop.rs b/routing/src/rib/nexthop.rs index 90fededc8b..d65fdf7948 100644 --- a/routing/src/rib/nexthop.rs +++ b/routing/src/rib/nexthop.rs @@ -17,6 +17,7 @@ use std::net::IpAddr; use std::option::Option; use net::interface::InterfaceIndex; +use std::cell::Cell; use std::cell::RefCell; use std::rc::{Rc, Weak}; #[cfg(test)] @@ -39,6 +40,7 @@ pub struct Nhop { pub(crate) resolvers: RefCell>>, pub(crate) instructions: RefCell>, pub(crate) fibgroup: RefCell, + pub(crate) invalid: Cell, } #[derive(Debug, Default, Copy, Clone, Hash, Eq, PartialEq, PartialOrd, Ord)] @@ -167,6 +169,7 @@ impl Nhop { resolvers: RefCell::new(Vec::new()), instructions: RefCell::new(Vec::with_capacity(2)), fibgroup: RefCell::new(FibGroup::new()), + invalid: Cell::new(false), } } diff --git a/routing/src/rib/rib2fib.rs b/routing/src/rib/rib2fib.rs index 013a3aac6b..56be4e5577 100644 --- a/routing/src/rib/rib2fib.rs +++ b/routing/src/rib/rib2fib.rs @@ -24,31 +24,48 @@ impl Nhop { #[allow(clippy::single_match_else)] #[must_use] fn build_pkt_instructions(&self, rstore: &RmacStore) -> Vec { + // mark as valid for the time being + self.invalid.set(false); + let mut instructions = Vec::with_capacity(2); + + // local route if self.key.origin == RouteOrigin::Local { match self.key.ifindex { Some(if_index) => instructions.push(PktInstruction::Local(if_index)), None => { - warn!("packet is locally destined but has no target interface index: dropping"); + self.invalid.set(true); + warn!("Unknown ifindex for local next-hop. Will set action drop"); instructions.push(PktInstruction::Drop); } } return instructions; } + + // an explicit drop if self.key.fwaction == FwAction::Drop { instructions.push(PktInstruction::Drop); return instructions; } + + // encapsulation if let Some(encap) = self.key.encap { - let mut inst_encap = encap; - match inst_encap { + let mut encap_instr = encap; + let ok = match encap_instr { Encapsulation::Vxlan(ref mut vxlan) => vxlan.resolve(rstore), - Encapsulation::Mpls(_) => {} + Encapsulation::Mpls(_) => true, // set to true for tests, actually unsupported + }; + if ok { + instructions.push(PktInstruction::Encap(encap_instr)); + let egress = + EgressObject::new(self.key.ifindex, self.key.address, self.key.ifname.clone()); + instructions.push(PktInstruction::Egress(egress)); + } else { + // resolution of encap instructions failed. Keep the route with action drop and mark the nhop as invalid + self.invalid.set(true); + instructions = vec![PktInstruction::Drop]; + warn!("Nhop {self} became invalid"); } - instructions.push(PktInstruction::Encap(inst_encap)); - let egress = - EgressObject::new(self.key.ifindex, self.key.address, self.key.ifname.clone()); - instructions.push(PktInstruction::Egress(egress)); return instructions; } if self.key.ifindex.is_some() { @@ -131,8 +148,10 @@ impl Nhop { impl VxlanEncapsulation { /// Resolve a Vxlan encapsulation object. The local vtep information is not used - /// in this process. We only resolve the destination mac. - pub(crate) fn resolve(&mut self, rstore: &RmacStore) { + /// in this process. We only resolve the destination mac. If no entry is found, + /// the vxlan object will have no MAC. If an entry is hit, the vxlan encap object + /// surely gets a MAC, whether the entry is stale or not. + pub(crate) fn resolve(&mut self, rstore: &RmacStore) -> bool { self.dmac = rstore.get_rmac(self.vni, self.remote).map(|e| e.mac); if self.dmac.is_none() { warn!( @@ -141,5 +160,7 @@ impl VxlanEncapsulation { self.remote ); } + // ok if we found a mac, even if the entry is stale + self.dmac.is_some() } } diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index 4b80d68fcd..d77d640baa 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -390,6 +390,17 @@ impl VrfTable { } } + ////////////////////////////////////////////////////////////////// + /// Refresh the fib groups for all vrfs that have a vni in the + /// provided set + ////////////////////////////////////////////////////////////////// + pub fn refresh_fibs_by_vni(&mut self, vnis: &[Vni], rstore: &RmacStore) { + let (vrfs, vrf0) = self.values_mut_except_default(); + for vrf in vrfs.filter(|v| v.vni.is_some_and(|vni| vnis.contains(&vni))) { + vrf.refresh_fib(rstore, Some(vrf0)); + } + } + ///////////////////////////////////////////////////////////////////////// // Set/unset stale flag for all routes in all vrfs ///////////////////////////////////////////////////////////////////////// diff --git a/routing/src/router/cpi.rs b/routing/src/router/cpi.rs index 3794738a12..eb8089f336 100644 --- a/routing/src/router/cpi.rs +++ b/routing/src/router/cpi.rs @@ -256,7 +256,7 @@ impl RpcOperation for Rmac { let Ok(rmac) = RmacEntry::try_from(self) else { return RpcResultCode::Failure; }; - rmac_store.del_rmac_entry(&rmac); + rmac_store.invalidate_rmac_entry(&rmac); RpcResultCode::Ok } } diff --git a/routing/src/router/rio.rs b/routing/src/router/rio.rs index e5120eb85c..d917bfbe2b 100644 --- a/routing/src/router/rio.rs +++ b/routing/src/router/rio.rs @@ -567,6 +567,13 @@ pub(crate) fn start_rio( /* check stale timeout. If expired, remove stale routes */ rio.check_stale_timeout(&mut db); + + /* remove stale router mac entries (if aged). If rmacs were deleted, refresh the + fibs for the vrfs with the corresponding vnis */ + let vnis = db.rmac_store.flush_stale_rmacs(); + if !vnis.is_empty() { + db.vrftable.refresh_fibs_by_vni(&vnis, &db.rmac_store); + } } }; let handle = thread::Builder::new() diff --git a/routing/src/router/rpc_adapt.rs b/routing/src/router/rpc_adapt.rs index 702a937fe7..49f9852df0 100644 --- a/routing/src/router/rpc_adapt.rs +++ b/routing/src/router/rpc_adapt.rs @@ -93,6 +93,7 @@ impl TryFrom<&Rmac> for RmacEntry { error!("Received router mac with invalid vni {}", value.vni); RouterError::VniInvalid(value.vni) })?, + stale_t: None, }) } }