track bgp session state in dataplane - #1645
Merged
Merged
Conversation
📝 WalkthroughWalkthroughChangesBGP neighbor status contracts now preserve optional reset metadata and require message counters. BMP peer transitions update dataplane state, emit router-control events, and render router events. BMP mirroring is enabled and startup now occurs after router initialization with immutable control-sender access. BGP neighbor event propagation and BMP mirroring
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds BGP neighbor session state change tracking to the dataplane by propagating BMP peer up/down notifications into the router control loop, emitting both info logs and persistent router events.
Changes:
- Introduce
BgpNeighEventgeneration from BMP PeerUp/PeerDown and forward it to the router viaRouterCtlSender. - Add a new
RouterEvent::BgpNeighStateChangevariant and expand the router event log capacity (1000 → 2000). - Make
RouterCtlSenderAPIs take&self(not&mut self) and thread the sender into the BMP server/handler.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| routing/src/router/revent.rs | Adds BGP neighbor state-change router events + increases event log capacity. |
| routing/src/router/ctl.rs | Adds ctl message for BGP neighbor status changes; makes sender methods take &self. |
| routing/src/frr/test.rs | Updates test to reflect RouterCtlSender no longer requiring mut. |
| routing/src/bmp/mod.rs | Threads RouterCtlSender into BMP server spawn path. |
| routing/src/bmp/handler.rs | Calls BMP render logic with router ctl sender to emit events. |
| routing/src/bmp/bmp_render.rs | Generates BGP neighbor state-change events from BMP messages. |
| mgmt/src/processor/proc.rs | Updates call sites to use &RouterCtlSender APIs. |
| mgmt/src/processor/launch.rs | Updates interface-event notifier to use non-mutable router ctl sender. |
| dataplane/src/runtime.rs | Starts BMP server after router so BMP handler can send router events. |
| config/src/internal/status/mod.rs | Adds Display for BGP session state; makes last_reset_reason optional. |
| config/src/converters/k8s/status/bgp.rs | Adjusts k8s status conversion/tests for optional last_reset_reason. |
Merged
Fredi-raspall
force-pushed
the
pr/fredi/bgp_sess_change_record
branch
from
July 19, 2026 10:54
f6c954b to
357d5a0
Compare
Fredi-raspall
force-pushed
the
pr/fredi/bgp_sess_change_record
branch
from
July 20, 2026 14:55
49819bc to
0ca0ef1
Compare
![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 `@routing/src/bmp/bmp_render.rs`:
- Around line 306-309: Update the PeerDown handling around
set_neighbor_session_state so last_reset_reason and last_reset_time are assigned
only when the previous neighbor state is not Idle. Preserve the existing
duplicate-message behavior and connection counter, while retaining the original
downtime start across repeated PeerDown notifications.
🪄 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: b77fc23b-f416-4325-8bc4-1d58b261a5c7
📒 Files selected for processing (13)
config/src/converters/k8s/status/bgp.rsconfig/src/internal/status/mod.rsdataplane/src/runtime.rsmgmt/src/processor/launch.rsmgmt/src/processor/proc.rsrouting/src/bmp/bmp_render.rsrouting/src/bmp/handler.rsrouting/src/bmp/mod.rsrouting/src/cli/display.rsrouting/src/cli/mod.rsrouting/src/frr/test.rsrouting/src/router/ctl.rsrouting/src/router/revent.rs
Fredi-raspall
force-pushed
the
pr/fredi/bgp_sess_change_record
branch
from
July 21, 2026 13:08
0ca0ef1 to
c7fc043
Compare
sergeymatov
approved these changes
Jul 22, 2026
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
BgpNeighEvent represents a status change of a BGP session. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Propagate a RouterCtlSender to the BMP server so that it can send events on BGP peer changes. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Mutability is not required by most methods of RouterCtlSender. Replace &mut by &self. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
We were not populating the last reset reason when parsing a peer down message. Fix this and make the field Option<String> instead of String. Also, enlarge the event buffer to 2000 events. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Reorganize the handling of peer up/down messages so that if a BGP neighbor event needs to be sent, the RwLock guard on the status has been released before attempting to send on the channel as, otherwise, the lock would unnecessarily be held if the channel was full and blocked us. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Instead of using the bgp router id, use the peer's key (address) in events. The router id is also kept. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Compute BGP session downtime and include it in the bgp events. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Add some sanity on bgp peer down to ensure that we only account for transitions from established. With FRR0s bmp implementation, we should only see peer downs when established state is left. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/bgp_sess_change_record
branch
from
July 23, 2026 08:29
c7fc043 to
3cf343a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
routing/src/router/revent.rs:104
confidence: 9
tags: [style]
The router event message uses "(downtime of …)", which reads awkwardly and doesn’t match the example output in the PR description ("downtime: …"). Consider using a consistent "downtime:" label.
} else if let Some(downtime) = &bgp_ev.last_downtime {
let downtime = PrettyDuration::new(*downtime);
write!(f, " (downtime of {downtime})")?;
}
</details>
Fredi-raspall
enabled auto-merge
July 23, 2026 09:54
Making them optional overcomplicates the code for no real benefit. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Process route mirrored packets to provide better message stats. This comes with limitations due to FRR's incomplete BMP implementation. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
This is needed for the msg stats to be populated. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
routing/src/bmp/bmp_render.rs:39
confidence: 8
tags: [other]
`BgpNeighEvent::new` is `pub`, which allows external crates to construct this internal event type. If this is only intended for intra-crate use (as suggested by `pub(crate)` fields), make the constructor `pub(crate)` as well.
pub fn new(
</details>
|
|
||
| /// A struct representing a status change of a BGP peering. Most of the information here | ||
| /// comes from `BgpNeighborStatus`. | ||
| pub struct BgpNeighEvent { |
| let msg = RouterCtlMsg::IfEvent(ev); | ||
| self.send_and_wake(msg).await | ||
| } | ||
| pub async fn send_bgp_neigh_change(&self, ev: BgpNeighEvent) -> Result<(), RouterError> { |
Comment on lines
+247
to
252
| if let BgpMessage::Open(_open) = pu.received_message() { | ||
| // amend FRR's limitation by increasing the count of opens received | ||
| // FIXME: this seems to over count the messages received. | ||
| // TODO: investigate if it is needed | ||
| // neigh.messages.received.open = neigh.messages.received.open.saturating_add(1); | ||
| } |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
routing/src/bmp/bmp_render.rs (1)
308-320: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the current reset reason in non-established PeerDown events.
For
Connect,Active,Open, orUnsetsessions, this code transitions toIdleand emits an event, but leaveslast_reset_reasonandlast_reset_timeunchanged. The event therefore contains no reason or stale downtime metadata. Record reset metadata for every state-changing PeerDown while keepingconnections_droppedlimited to Established sessions.Proposed fix
let prev_state = neigh.session_state; let prev_dropped = neigh.connections_dropped; + let state_changed = prev_state != BgpNeighborSessionState::Idle; set_neighbor_session_state(neigh, BgpNeighborSessionState::Idle); + if state_changed { + neigh.last_reset_reason = Some(pretty(pd.reason().get_type())); + neigh.last_reset_time = Some(std::time::Instant::now()); + } if prev_state == BgpNeighborSessionState::Established { neigh.connections_dropped = neigh.connections_dropped.saturating_add(1); - neigh.last_reset_reason = Some(pretty(pd.reason().get_type())); - neigh.last_reset_time = Some(std::time::Instant::now()); } else {As per coding guidelines, this is a logic-error finding; as per PR objectives, BGP state events include the reset/down reason.
🤖 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/bmp/bmp_render.rs` around lines 308 - 320, Update the PeerDown handling around the prev_state check so every state-changing event records last_reset_reason from pd.reason().get_type() and refreshes last_reset_time, including non-Established sessions. Keep connections_dropped.saturating_add(1) restricted to the Established branch, while preserving the existing non-Established diagnostic logging.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@routing/src/bmp/bmp_render.rs`:
- Around line 308-320: Update the PeerDown handling around the prev_state check
so every state-changing event records last_reset_reason from
pd.reason().get_type() and refreshes last_reset_time, including non-Established
sessions. Keep connections_dropped.saturating_add(1) restricted to the
Established branch, while preserving the existing non-Established diagnostic
logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e6e1b3f-3916-4a9b-8755-7e8fd72fb221
📒 Files selected for processing (6)
config/src/converters/k8s/status/bgp.rsconfig/src/internal/routing/bmp.rsconfig/src/internal/status/mod.rsdataplane/src/runtime.rsrouting/src/bmp/bmp_render.rsrouting/src/frr/renderer/bgp.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dataplane/src/runtime.rs
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.
This goes on top of #1643Log
(info)events for BGP session changesGenerate events so that all changes get recorded independently of logging and that these include: