fix (DPU-agent): make BgpPeeringRouteServer redundancy aware - #4708
fix (DPU-agent): make BgpPeeringRouteServer redundancy aware#4708hatamzad-nv wants to merge 4 commits into
Conversation
Our route servers are redundant and all advertise the same EVPN routes, so losing one doesn't actually affect the DPU's routing. But NICo doesn't account for this, and if one route server has an issue, it raises a BgpPeeringRouteServer health alert that falls under PreventAllocations and PreventHostStateChanges. That reduces the site's capacity and blocks host operations. Instead, we should track how many route servers have failed. When redundancy is truly gone, either a single route server being down or all of them failing, NICo raises the full health alert and marks the machines unhealthy. Fixes NVIDIA#4113
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughBGP route-server health checks now account for redundancy. The health data records configured sessions, and alerts become critical only when fewer than two healthy route servers remain. New fixtures, tests, and documentation cover failure and missing-session scenarios. ChangesBGP route-server health
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BgpSummary
participant BgpHealthEvaluator
participant RouteServerAlert
BgpSummary->>BgpHealthEvaluator: provide configured and session-state data
BgpHealthEvaluator->>BgpHealthEvaluator: calculate healthy route servers
BgpHealthEvaluator->>RouteServerAlert: classify route-server alert
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4708.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/architecture/health/health_probe_ids.md`:
- Line 87: Update the route-server alert description in the health probe
documentation: capitalize “With,” define a healthy session as one whose state
equals "Established," state that every missing or non-"Established" configured
peer emits an alert, and specify that PreventAllocations and
PreventHostStateChanges apply only when fewer than two sessions are healthy.
Reconcile related Markdown, run rumdl, and inspect the rendered page.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c0f5dec9-84fa-483e-9265-6f097d559d09
📒 Files selected for processing (5)
crates/agent/src/hbn_bgp_summary_with_route_servers_one_failed.jsoncrates/agent/src/hbn_bgp_summary_with_route_servers_two_failed.jsoncrates/agent/src/hbn_bgp_summary_with_two_route_servers_one_failed.jsoncrates/agent/src/health/bgp.rsdocs/architecture/health/health_probe_ids.md
|
I think there is reason it was built that way. At least in the pre-FNN world there were still issues when one route server that was supposed to be there was not responding. Maybe with FNN it changed. @bcavnvidia might know more. |
polarweasel
left a comment
polarweasel
left a comment
There was a problem hiding this comment.
Just a couple of little changes, then good to go
polarweasel
left a comment
polarweasel
left a comment
There was a problem hiding this comment.
I prefer to have the paragraph break, but if you really don't want it, good enough...
There was a problem hiding this comment.
So unless we changed something, and I'll need to dig into this, the route servers don't peer with each other, and have completely independent tables.
The scenario here is:
DPU-A peers with RS1 + RS2.
DPU-B peers with RS2 + RS3.
If RS2 fails, DPU-A and DPU-B cannot see eachother.
We therefore have to have monitoring at all times to make sure all DPUs share the same subset of route server connections (at least one common route server connection).
If configured_route_servers is not all route servers, then we indeed set ourselves up for a partitioning risk.
I put up a design doc about enhancing/revamping our route server architecture back in August 2025, and that turned into a meeting with the network team.
The outcome of the network team meeting was that we'd get rid of our route servers entirely, and instead lean on existing TOR peering (which gives us built in redundancy thanks to each PF on the DPU) with the network team to distribute routes for us. They already had/have BGP EVPN plumbing in place down to the leaves for other reasons, and said they could take on our load.
THAT said, I don't think anything came of it, or did it @bcavnvidia?
If not, I actually think we'll want to revive the route server design doc. I actually had an MVP/PoC tested for it, and it would give us what we want in terms of redundancy, and THEN we could say that 1 route server being up is "safe".
All said, I'll double check with @bcavnvidia to see if anything changed between now and then.
Fwiw, thanks for this PR! I know you had no clue about any of the history -- good to capture it here!
cc @Matthias247
Correct
Also correct, and we've been switching off route-servers internally. |
Got it, okay, so that leaves us with:
It doesn't affect our sites, since we're moving away from them, but for people who do their own deployments with them, the architecture as-is doesn't work, and they would need to do their own adjustments. And fwiw @hatamzad-nv, if/once we DO actually fix the architecture, then yes, this PR is good! |
5 participants
Our route servers are redundant and all advertise the same EVPN routes, so losing one doesn't actually affect the DPU's routing. But NICo doesn't account for this, and if one route server has an issue, it raises a BgpPeeringRouteServer health alert that falls under PreventAllocations and PreventHostStateChanges. That reduces the site's capacity and blocks host operations.
Instead, we should track how many route servers have failed. When redundancy is truly gone, either a single route server being down or all of them failing, NICo raises the full health alert and marks the machines unhealthy.
Fixes #4113
Related issues
Fixes #4113
Type of Change
Breaking Changes
Testing
Four new rows in the existing table-driven
verify_bgp_summary_emits_expected_alertstest, backed by three new fixtures:
covering the missing-session path under redundancy
The existing single-route-server-down row still expects a critical alert, so the no-redundancy behaviour stays regression covered.
cargo test -p carbide-agent --lib health::bgp