Repository navigation
Fix stale BMP peer retention with automatic snapshot reconciliation - #14
nuclearcat wants to merge 4 commits into
Conversation
Assisted-by: OpenAI Codex
Assisted-by: OpenAI Codex
Assisted-by: OpenAI Codex
Assisted-by: OpenAI Codex
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| data = self.socket.recv(1) | ||
| if data: | ||
| raise RuntimeError("collector unexpectedly sent BMP data") | ||
| except ConnectionResetError: |
| self.socket.recv(1) | ||
| except socket.timeout: | ||
| return | ||
| except ConnectionResetError: |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It introduces an always-on, non-disableable behavior change to core BMP processing that periodically forces full-table replay for every exporter, with broad operational impact that warrants human judgment despite the clean, well-tested implementation.
Review effort: Balanced
Findings: None
What changed in this PR
This PR addresses issue #11, where a misaddressed or missing BMP Peer Down (e.g. FRR reporting :: after an unnumbered peer loses its link-local address) leaves a stale peer and its Adj-RIB-In connected indefinitely. Rather than guessing which same-router-ID peer to delete, it introduces an always-on "reconciliation" mechanism: when an anomaly is detected (unmatched Peer Down, possible unnumbered-peer replacement, peer-state capacity reached, or a periodic/idle timer), the speaker's BMP transport is closed so the exporter replays an authoritative snapshot on reconnect. The logic lives in a new reconciliation module wired into the per-connection RouterHandler read loop and the BMP state machine.
Changes:
- New
reconciliationmodule: validatedConfig(bounds enforced viaTryFrom, cannot be disabled), jittered periodic deadline, and anomaly/capacity deadline coalescing. - State-machine
reconciliation_reasonclassifies Peer Up/Down against logical peer identity (type/distinguisher/ASN/BGP-id, excluding policy flags) and exposespeer_state_count; the read loop times out on the deadline and closes the transport for a fresh snapshot. - Config plumbing through
BmpTcpIn/BmpTcpInRunner/RouterHandler(new connections only), plus extensive Rust tests, a Python churn simulator, and documentation.
| File | Description |
|---|---|
src/units/bmp_tcp_in/reconciliation.rs |
New module: config validation, jitter, deadline/observe logic, unit tests. |
src/units/bmp_tcp_in/state_machine/machine.rs |
Adds peer_states/peer_state_count/reconciliation_reason for anomaly detection. |
src/units/bmp_tcp_in/router_handler.rs |
Read loop enforces reconciliation deadline/timeout, observes per-message, drops transport. |
src/units/bmp_tcp_in/unit.rs |
Threads reconciliation config through construction/reconfigure; adds active redial test. |
src/units/bmp_tcp_in/mod.rs |
Exposes the new reconciliation module. |
src/units/bmp_tcp_in/state_machine/tests.rs |
Tests for identity scoping and non-destructive detection of parallel peers. |
scripts/bmp-peer-churn.py |
New stdlib-only simulator exercising seven reconciliation scenarios. |
docs/bmp-tcp-in.md |
Documents triggers, defaults, RFC 7854 basis, and the monitoring-gap/replay caveat. |
TESTING.md |
Describes the simulator and Rust regression suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Fixes #11.
A misaddressed or missing BMP Peer Down can leave the old peer connected with its routes indefinitely. Recover automatically by renewing the affected speaker’s BMP connection and rebuilding from its current snapshot, rather than guessing which same-router-ID peer to delete. Includes periodic renewal for silent disappearance, startup replay protection, coalesced anomaly deadlines, and a peer-state capacity limit. No opt-in is required.
Reproduction and coverage: our synthetic BMP exporter repeatedly replaces a link-local peer while retaining the speaker connection. Before the fix, 20 replacements with missing or
::Peer Down retained 21 peers and route records after GC. The replay-aware simulation now verifies reclamation, legitimate parallel peers, silent disappearance, partial-frame timeouts, and capacity recovery. Rust regressions cover scheduling, identity scopes, policy/ADD-PATH cleanup, and active reconnects.Validation on this main-based branch:
cargo test --lib --offline(354 passed, 31 ignored), daemon build, and all seven live scenarios with 20 replacements per churn scenario.Caveat: reconciliation withdraws the speaker’s monitored routes until reconnect/replay restores them, causing a monitoring gap and full-table replay cost; it does not reset the router’s BGP sessions. It requires an exporter that reconnects and supplies current state. Startup grace and the peer-state cap are bounded heuristics, not a total memory limit. This follows RFC 7854 §3.2, §3.3, and §5; defaults and operational limitations are documented in
docs/bmp-tcp-in.md.Assisted-by: OpenAI Codex