router mac revisit#1649
Merged
Merged
Conversation
Fredi-raspall
added
ci:+release
📝 WalkthroughWalkthroughChangesMAC formatting now uses a preallocated buffer. Routing retains stale RMAC entries for timed cleanup and VNI-scoped FIB refresh, while next-hop resolution records invalid state and drops unusable paths. CLI output reports both states. ChangesMAC display formatting
RMAC stale-entry lifecycle
Next-hop validity tracking
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
routing/src/evpn/rmac.rs (1)
242-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests don't cover the production stale-marking or flush paths.
rmac_store_basiconly exercisesdel_rmac(test-only immediate removal). The production pathsdel_rmac_entry(stale marking) andflush_stale(timed removal) have no test coverage. Consider adding tests that verify: (1)del_rmac_entrymarks entries stale and increments the counter, (2) duplicatedel_rmac_entrycalls are idempotent, (3)flush_staleremoves only entries older thanMAX_STALE_TIMEand correctly decrements the counter, (4)add_rmac_entryon a stale entry resets it and decrements the counter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@routing/src/evpn/rmac.rs` around lines 242 - 266, Extend the RMAC store tests around rmac_store_basic to cover the production lifecycle: verify del_rmac_entry marks an entry stale and increments the stale counter, repeated calls are idempotent, flush_stale removes only entries older than MAX_STALE_TIME while decrementing the counter, and add_rmac_entry resets a stale entry while decrementing the counter. Use controllable entry timestamps or the existing time abstractions so the age boundary is tested deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@routing/src/evpn/rmac.rs`:
- Around line 106-119: Update del_rmac_entry so it increments self.stale and
sets the stale timestamp only when current.stale is None; leave already-stale
entries unchanged, while preserving the existing MAC-match guard and logging
behavior.
- Around line 149-179: Update flush_stale to decrement self.stale by the number
of stale entries actually removed from self.table, using the successful
remove_entry results rather than stale.len(). Preserve the existing scan,
logging, and return value while ensuring the counter remains accurate after each
flush.
---
Nitpick comments:
In `@routing/src/evpn/rmac.rs`:
- Around line 242-266: Extend the RMAC store tests around rmac_store_basic to
cover the production lifecycle: verify del_rmac_entry marks an entry stale and
increments the stale counter, repeated calls are idempotent, flush_stale removes
only entries older than MAX_STALE_TIME while decrementing the counter, and
add_rmac_entry resets a stale entry while decrementing the counter. Use
controllable entry timestamps or the existing time abstractions so the age
boundary is tested deterministically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d71dc34-2ec6-46d8-b107-83c670a28c6b
📒 Files selected for processing (5)
net/src/eth/mac.rsrouting/src/cli/display.rsrouting/src/evpn/rmac.rsrouting/src/router/rio.rsrouting/src/router/rpc_adapt.rs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR revisits EVPN router-MAC (RMAC) handling by introducing a “stale” lifecycle for RMAC entries (mark on delete, flush after a timeout) and exposing that state via the CLI, while also adjusting MAC address formatting.
Changes:
- Add staleness tracking to
RmacEntry/RmacStoreand a periodicflush_stale()garbage-collection step. - Update CLI output to show RMAC entry status (
ok/stale) and include a stale counter in the heading. - Improve
MacDisplayformatting by rendering into a fixed-size buffer and using formatter padding.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| routing/src/router/rpc_adapt.rs | Initializes new RmacEntry staleness field when adapting from RPC objects. |
| routing/src/router/rio.rs | Periodically flushes expired stale RMAC entries during the main I/O loop. |
| routing/src/evpn/rmac.rs | Introduces stale marking, stale counting, and timed flushing of RMAC entries. |
| routing/src/cli/display.rs | Displays RMAC staleness state and stale count in CLI output. |
| net/src/eth/mac.rs | Updates MAC Display implementation to use a fixed-size buffer and respect formatter padding. |
Fredi-raspall
force-pushed
the
pr/fredi/router_mac_revisit
branch
from
July 20, 2026 18:12
3ddab79 to
32ab06e
Compare
Impl Display for Mac such that formatting is preserved. We use ArrayString in stack so that no allocation is needed, using the fact that a mac, as a hex string, has a fixed length. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Router macs are learnt separately from evpn routes. These are sent before the routes using them. The same occurs on deletions, e.g. when the bgp session of a leaf switch is torn down. Dataplane resolves next-hops, not routes, and does it pretty agressively to keep a correct state, even on route deletions, because next-hops are shared. When a leaf-switch bgp session (even one the gateway does not participate in) flaps or is torn down, the router macs get deleted, so are the routes. This means that dataplane may attempt to resolve next-hops (with encap info) referring to a vtep/irb for which the router mac is no longer known, producing multiple warnings. Omitting the resolution on route deletions is not viable since our resolution is per next-hop, not per route. Solve this by keeping the router macs for some time interval, (instead of deleting them immediately), so that next-hops (which may end up disapparing anyway) are still correctly resolved. This should not cause any operational issue because if router macs are gone, so are the routes using them later on. So, temporarily keeping the macs does not harm, and up-to-date router macs may be re-established when a (far) bgp session comes up again. The solution marks router macs as "stale" temporarily until they get removed or restored. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Invalidate next-hops if they have vxlan encapsulation and we fail to resolve the rmac. Such next-hops are retained and used. However, their packet instructions are set to drop the traffic. We should very rarely see an invalid next-hop. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/router_mac_revisit
branch
from
July 21, 2026 12:38
32ab06e to
c6ab732
Compare
Comment on lines
16
to
20
| pub address: IpAddr, | ||
| pub mac: Mac, | ||
| pub vni: Vni, | ||
| pub stale_t: Option<Instant>, // instant when the rmac was deleted | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
routing/src/evpn/rmac.rs (3)
98-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDouble hashmap lookup in
del_rmac.
self.table.entry(key)followed byself.table.remove_entry(&key)performs two lookups. Since the occupied entryoisn't used after the staleness check, this compiles under NLL, but it's more idiomatic (and marginally cheaper) to remove directly via the entry API.♻️ Suggested refactor
pub fn del_rmac(&mut self, address: IpAddr, vni: Vni) -> Option<RmacEntry> { let key = (address, vni); - - 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; - } + if let Entry::Occupied(o) = self.table.entry(key) { + if !o.get().is_stale() { + return None; + } + self.stale = self.stale.saturating_sub(1); + return Some(o.remove()); + } warn!("Could not delete rmac entry for vni:{vni} address:{address}: not found"); None }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@routing/src/evpn/rmac.rs` around lines 98 - 114, Update del_rmac to remove stale entries directly through the occupied Entry API instead of calling self.table.remove_entry(&key) after the initial entry lookup. Preserve the existing non-stale early return, stale counter decrement, debug assertion, return value, and not-found warning behavior.
70-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
add_rmac_entryassumes the incoming entry is never itself stale.The stale counter is only adjusted for the old entry being replaced (
old.stale_t); it never checks the newentry.stale_t. This is safe today becauseRmacEntry::try_from(&Rmac)always setsstale_t: None(seerpc_adapt.rs), but the invariant is implicit rather than enforced. Adebug_assert!would make this contract explicit and catch future misuse (e.g., if a caller ever constructs anRmacEntrywithstale_talready set).pub fn add_rmac_entry(&mut self, entry: RmacEntry) { debug_assert!(entry.stale_t.is_none(), "newly added rmac entries must not be pre-marked stale"); ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@routing/src/evpn/rmac.rs` around lines 70 - 90, Update RmacTable::add_rmac_entry to assert that the incoming RmacEntry has no pre-existing stale_t value before modifying the table, using a debug assertion with a clear contract message; preserve the existing replacement and stale-counter logic.
173-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo direct test coverage for
flush_stale_rmacs.
rmac_store_basicexercisesadd_rmac_entry,invalidate_rmac_entry, anddel_rmac, butflush_stale_rmacs(the actual periodic-cleanup entry point invoked fromrio.rs) is untested. SinceMAX_STALE_TIMEis a hardcoded const, testing the timeout path is awkward without either sleeping or making the duration injectable for tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@routing/src/evpn/rmac.rs` around lines 173 - 209, The `flush_stale_rmacs` cleanup entry point lacks direct coverage, especially for removing entries older than `MAX_STALE_TIME`. Add focused tests that exercise no stale entries, stale-but-not-expired entries, and expired stale entries with the expected VNI results; make the timeout testable without sleeping by introducing an appropriate injectable or test-configurable time source while preserving production behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@routing/src/evpn/rmac.rs`:
- Around line 98-114: Update del_rmac to remove stale entries directly through
the occupied Entry API instead of calling self.table.remove_entry(&key) after
the initial entry lookup. Preserve the existing non-stale early return, stale
counter decrement, debug assertion, return value, and not-found warning
behavior.
- Around line 70-90: Update RmacTable::add_rmac_entry to assert that the
incoming RmacEntry has no pre-existing stale_t value before modifying the table,
using a debug assertion with a clear contract message; preserve the existing
replacement and stale-counter logic.
- Around line 173-209: The `flush_stale_rmacs` cleanup entry point lacks direct
coverage, especially for removing entries older than `MAX_STALE_TIME`. Add
focused tests that exercise no stale entries, stale-but-not-expired entries, and
expired stale entries with the expected VNI results; make the timeout testable
without sleeping by introducing an appropriate injectable or test-configurable
time source while preserving production behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 420fc172-a81a-46f9-9388-4943b5caa9a5
📒 Files selected for processing (9)
net/src/eth/mac.rsrouting/src/cli/display.rsrouting/src/evpn/rmac.rsrouting/src/rib/nexthop.rsrouting/src/rib/rib2fib.rsrouting/src/rib/vrftable.rsrouting/src/router/cpi.rsrouting/src/router/rio.rsrouting/src/router/rpc_adapt.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- routing/src/router/rpc_adapt.rs
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.