ospfd: quick neighbor feature with BFD - #21784
Conversation
c5c405d to
057b3c6
Compare
|
@greptileai review |
Greptile SummaryThis PR introduces an OSPF "quick neighbor" feature that integrates with BFD to enable sub-hello-interval adjacency re-establishment. The main architectural change is refactoring per-neighbor BFD sessions into per-interface Confidence Score: 5/5Safe to merge; all previously flagged P1 issues appear resolved and remaining findings are style-level P2s. No new P0 or P1 issues found. The BFD session lifetime, num_q_nbrs accounting, null-guard, and wait-timer backstop concerns raised in earlier rounds are addressed. Remaining comments are code-clarity and test-style observations. ospfd/ospf_bfd.c (complex lock-accounting in entry_del/flush), ospfd/ospf_packet.c (rekey logic for PtP/VLink) Important Files Changed
Sequence DiagramsequenceDiagram
participant BFD as bfdd
participant CB as ospf_bfd_session_change
participant QN as ospf_qn_add
participant NBR as ospf_qnbr_get / ospf_nbr_add
participant NSM as NSM state machine
participant PKT as ospf_verify_header
note over BFD,PKT: Initial adjacency (quick mode)
BFD->>CB: BSS_UP (first time)
CB->>QN: ospf_qn_add(oi, endpoint)
QN->>NBR: ospf_qnbr_get → ospf_nbr_add (router_id=0)
NBR-->>QN: nbr (IS_QUICKNBR=true, num_q_nbrs++)
QN->>NSM: NSM_HelloReceived → NSM_TwoWayReceived → NSM_AdjOK
NSM-->>QN: neighbor at ExStart (quick path in nsm_adj_ok)
QN->>QN: arm t_qn_wait (100 ms poll)
note over BFD,PKT: First OSPF packet arrives (e.g. DD)
PKT->>PKT: ospf_verify_header: qnbr.router_id==0 → learn router_id
PKT->>PKT: num_q_nbrs--
PKT->>PKT: ospf_qnbr_rekey_ptp_vlink (PtP/VLink only)
PKT->>NSM: ISM_NeighborChange + ospf_hello_send
note over BFD,PKT: Link loss and recovery
BFD->>CB: BSS_DOWN
CB->>NSM: ospf_nbr_bring_down → NSM_InactivityTimer
NSM->>NBR: ospf_nbr_free → ospf_neighbor_bfd_clear (quick: keep bsp)
BFD->>CB: BSS_UP (reconnect)
CB->>QN: ospf_qn_add (new quick neighbor cycle)
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
ospfd/ospf_bfd.c:317-325
**`legacy_bsp` aliasing confusion in non-NULL-entry path**
When the entry exists but `bfd_config` is NULL (BFD disabled while a quick neighbor was live), `ospf_bfd_entry_del` correctly frees `entry->bsp`. However, `legacy_bsp` (a local copy of `nbr->bfd_session`, which aliases `entry->bsp`) is non-NULL and never zeroed in this path — the `bfd_sess_free(&legacy_bsp)` fallback on line 329 is only reached when `entry == NULL`. No double-free occurs (the double-pointer idiom in `bfd_sess_free` nulls `entry->bsp`, not `legacy_bsp`), but the stale local pointer makes the intent confusing and the code fragile if rearranged.
### Issue 2 of 3
ospfd/ospf_quicknbr.c:158-164
**`ospf_qn_add` re-fires NSM events on a stale `NSM_Init` quick neighbor without a comment**
When BFD fires UP while a prior quick neighbor is still at `NSM_Init` (rapid BFD bounce mid-transition), `NSM_HelloReceived` fires a second time. The NSM handles `NSM_Init + NSM_HelloReceived` as a no-op (stays at Init), so this is functionally safe, but `t_qn_wait` is re-armed. A brief comment clarifying this is intentional would help future readers.
### Issue 3 of 3
tests/topotests/bfd_ospf_quicknbr_topo1/test_bfd_ospf_quicknbr_topo1.py:1380-1387
**Inconsistent `sw.cmd_raises` vs `sw_net.cmd_raises` in `set_switch_blackhole`**
The `enable=True` branch uses `sw.cmd_raises(...)` (the Topogen gear abstraction) while the `enable=False` branch uses `sw_net.cmd_raises(...)` (the underlying Mininet network object directly). Both work in practice since `sw_net = sw.net`, but the inconsistency makes the enable/disable pair harder to maintain.
```suggestion
if enable:
for ifname in switch_ports:
sw.cmd_raises("tc qdisc replace dev {} root netem loss 100%".format(ifname))
else:
for ifname in switch_ports:
sw.cmd_raises("tc qdisc del dev {} root".format(ifname))
```
Reviews (7): Last reviewed commit: "tests: Add OSPF BFD quick topotest" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai review |
|
@greptileai review |
|
@greptileai review |
da123a0 to
2885628
Compare
|
@greptileai review |
|
@greptileai In regards to "the no ip ospf bfd quick command tears down all BFD sessions rather than just removing quick mode" |
|
That's a reasonable design, and looking at the code now I can see The
|
|
@greptileai review |
@greptileai You said you would withdraw this concern but it's still an issue you list in your review. |
|
You're right, and I apologize for the inconsistency. Saying "I'll withdraw that concern" in a reply thread doesn't remove it from the original review comment — the original comment remains visible on the PR. The two are independent: inline review comments persist regardless of what's said in follow-up replies. The concern about |
nabahr
changed the title
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…ghbor This allows the possibility for BFD sessions to persist after a neighbor goes away. Keeping the BFD session open means we could then potentially bring the neighbor back with a up signal from BFD. Signed-off-by: Nathan Bahr <nbahr@atcorp.com>
Quick neighbors exposes a simple interface to add/delete a neighbor based on the provided router-id, link address, and endpoint/neighbor address. On Quick neigbor delete, if we find the neighbor, we immediately set the NSM to Inactivity and the neighbor is removed, this is the same as the current BFD neighbor delete. On Quick neighbor add, if the neighbor is not found, then we add it to the neighbors list without a router id or other hello fields. These fields are either left empty to be learned later or presumed based on the local link. We artifically push the neighbor through the NSM states until it is in the ExStart state. If the interface is currently in the Waiting state, we stop the normal wait timer and restart a quick wait timer that will check for quick neighbors (those without a router id) and once we've learned the router id's of all the quick neigbors, we push the interface out of the waiting state. In order to facilitate quick neighbor learning, we detect OSPF packets from quick neighbors and update the router id as soon as possible, i.e. during verification of the OSPF header. When a new router-id is learned, we trigger a neighbor change event in the ISM and send an immediate hello. We also send another immediate hello when the DR/BDR election changes. Finally, in the NSM, we also assume that a quick neighbor should form an adjacency so that it can transition to a full neighbor. Signed-off-by: Nathan Bahr <nbahr@atcorp.com>
Updated docs to match how it's implemented. Added documentation of the quick neighbor feature. Signed-off-by: Nathan Bahr <nbahr@atcorp.com>
This topotest exercises the quick neighbor feature in OSPF using BFD sessions that live on after a neighbor goes down. This allows it to come back quickly when BFD detects the neighbor when there are long timers. Signed-off-by: Nathan Bahr <nbahr@atcorp.com>
2 participants
This PR adds a quick neighbor feature with a simple interface to add neighbors without a hello packet being received.
This PR integrates it with BFD but it could be used in other ways to trigger neighbors via external signals.
The existing BFD implementation only uses the Down state to delete inactive neighbors and does not make use of BFD to quickly re-add neighbors when connectivity is re-established.
This PR adds the ability to enable quick neighbors for the BFD session such that the session will stay active after the neighbor goes down, and when the neighbor comes back up, it is quickly re-added and Hellos are exchanged to form an adjacency.
This feature is only used when BFD is enabled with the "quick" option, otherwise it has no effect.
Update 4/28/26:
I needed to refactor BFD sessions in ospf to be stored per-interface instead of per-neighbor so that the BFD session can remain active after a neighbor is removed. This is now the first commit in the series.