static route bfd admin down state handling improvements

[sougatahitcs]

Prevent static route deletion/re-addition during BFD admin down/up transitions

Skip route removal when BFD transitions from Admin Down to Down state

Add check to avoid re-installing already installed routes on BFD Up

Enhanced debug logging for BFD state transitions and path status

[greptile-apps]

Greptile Summary

This PR fixes unnecessary route churn in staticd when a BFD session transitions through Admin Down → Down → Up after an operator removes a shutdown on a BFD profile or peer. Previously, staticd would remove the static route during the transient Down state and re-add it on Up, even though the route was never truly unreachable. The fix adds two guards in static_next_hop_bfd_change: (1) skip route removal in BSS_DOWN when previous_state == BSS_ADMIN_DOWN && !path_down, and (2) skip route re-installation in BSS_UP when the route is already installed. A new topotest (static_bfd_admin_down) validates both profile-level and per-peer shutdown cycles via debug-log inspection.

Key changes:

• staticd/static_bfd.c: Two early-exit guards in static_next_hop_bfd_change prevent redundant RIB updates around admin-down cycles; a new top-level DEBUGD log added on every state-change call for observability.
• tests/topotests/static_bfd_admin_down/: New topotest with IPv4/IPv6 coverage for profile shutdown, per-peer shutdown, and real-failure scenarios.

Concern:

• The previous_state == BSS_ADMIN_DOWN guard in BSS_DOWN cannot distinguish a transient Down (on the way to Up) from a permanent Down (peer genuinely unreachable). Because lib/bfd.c only fires the update callback on state changes and BFD stays in DOWN when the peer is unreachable, this guard can leave the static route permanently installed after admin-down is removed while the peer is down — defeating BFD's core purpose of withdrawing routes on path failure.

Confidence Score: 4/5

Needs review of the ADMIN_DOWN→DOWN guard before merging; a real-failure scenario following admin-down removal can leave routes permanently installed.

The BSS_UP early-exit guard and all test infrastructure look correct. The BSS_DOWN guard introduces a P1 correctness regression: when admin-down is removed and the peer is genuinely unreachable, the BFD callback fires once (ADMIN_DOWN→DOWN), the fix skips route removal, and because lib/bfd.c only notifies on state changes (line 994–996), no further callbacks fire. The route stays installed indefinitely despite the path being down.

staticd/static_bfd.c — the BSS_DOWN early-exit guard at line 46 needs to handle the permanent-DOWN case.

Important Files Changed

Filename	Overview
staticd/static_bfd.c	Adds two optimizations to prevent route churn around admin-down cycles. The BSS_UP guard is correct. The BSS_DOWN guard introduces a P1 regression: if admin-down is removed while the peer is genuinely unreachable, the route stays installed permanently because BFD stays in DOWN with no further state-change callbacks.
tests/topotests/static_bfd_admin_down/test_static_bfd_admin_down.py	New topotest verifying the admin-down fix via debug log inspection. Covers profile shutdown, per-peer shutdown, and real failure (UP→DOWN) scenarios. Missing coverage for the admin-down-removed-while-peer-unreachable edge case.
tests/topotests/static_bfd_admin_down/r1/frr.conf	r1 router config with static BFD-monitored routes and a BFD profile — looks correct.
tests/topotests/static_bfd_admin_down/r2/frr.conf	r2 router config matching r1's BFD peers; loopback supplies the destination prefixes — looks correct.
tests/topotests/static_bfd_admin_down/init.py	Empty init file required for Python package discovery — no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[BFD state-change callback fires] --> B{bss->state?}
    B -->|BSS_UNKNOWN| C[No action]
    B -->|BSS_ADMIN_DOWN| C
    B -->|BSS_DOWN| D{previous_state == BSS_ADMIN_DOWN\nAND !sn->path_down?}
    D -->|YES – NEW guard| E[Break: keep route installed\n⚠️ Also skips removal if peer is\ngenuinely unreachable]
    D -->|NO| F[path_down = true\nstatic_zebra_route_add\nremove route from RIB]
    B -->|BSS_UP| G{!sn->path_down?\nroute already installed}
    G -->|YES – NEW guard| H[Break: no redundant add]
    G -->|NO| I[path_down = false\nstatic_zebra_route_add\nadd route to RIB]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: staticd/static_bfd.c
Line: 46-51

Comment:
**Route stays installed permanently when peer is unreachable after admin-down is lifted**

The fix correctly prevents unnecessary route churn during the `ADMIN_DOWN → DOWN → UP` sequence, but it introduces a correctness regression when the peer is genuinely unreachable after admin-down is removed.

**Problematic scenario:**
1. BFD is UP, route installed (`path_down = false`)
2. Operator shuts down BFD session → `BSS_ADMIN_DOWN` event fires, route stays installed (correct)
3. Meanwhile, the remote peer becomes unreachable (link failure, daemon crash, etc.)
4. Operator removes admin-down → BFD transitions `ADMIN_DOWN → DOWN` immediately
5. The callback fires with `bss->previous_state == BSS_ADMIN_DOWN` and `!sn->path_down` → **route removal is skipped**
6. BFD stays in `DOWN` state indefinitely because the remote is unreachable
7. **No further callbacks fire**, confirmed by `lib/bfd.c` line 994–996:
   ```c
   if ((int)bsp->bss.state == state)
       continue;
   ```
   The `updatecb` is only triggered on state *changes*. Since BFD stays in `DOWN`, the route **remains installed forever** even though the peer is genuinely down — defeating the entire purpose of BFD route tracking.

The condition `previous_state == BSS_ADMIN_DOWN` cannot distinguish between a transient DOWN (on the way to UP) and a permanent DOWN (peer unreachable). At the time the `BSS_DOWN` event fires, the reachability of the peer is not yet known.

One approach: use a short hold-down timer — delay route removal briefly when `previous_state == BSS_ADMIN_DOWN`, and cancel the timer if `BSS_UP` arrives. Another option: always remove the route on `BSS_DOWN` (restoring the original behaviour) but rely solely on the `BSS_UP` guard to avoid the double-add, which the second half of this fix already handles correctly.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: tests/topotests/static_bfd_admin_down/test_static_bfd_admin_down.py
Line: 326-340

Comment:
**Missing test coverage for real-failure-after-admin-down scenario**

The test suite includes `test_bfd_real_failure_removes_routes`, which verifies that a real link failure starting from `BSS_UP` still removes the route. However, there is no test for the scenario identified in the core logic issue: admin-down is removed while the peer is genuinely unreachable (i.e., `ADMIN_DOWN → DOWN` where DOWN is permanent, not transient).

A test along these lines would be valuable:
1. Bring the link down to simulate a real failure.
2. While the session is in DOWN state, admin-down the BFD profile on r1.
3. Remove admin-down on r1 (link still down).
4. Verify that the route is eventually removed (since the peer is still unreachable).

This case maps directly to the regression introduced by the `previous_state == BSS_ADMIN_DOWN` guard in `BSS_DOWN`.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "tests: add topotest for static route BFD..." | Re-trigger Greptile}

[mjstapp]

please correct the PR headline to match our conventions

[riw777]

headline needs to be fixed ...

[sougatahitcs]

@mjstapp @riw777 I have corrected the headline , please review and let me know if you have any comments.

[rzalamena]

Please merge the third commit into the first so we have less commits (and less logic change)

[rzalamena]

Let's assume a situation where static route started installed and BFD session administratively down.

What happens when we transition to BFD session down and then back to administratively down? It seems to me that the BSS_DOWN timer would remain and after 5 seconds in BSS_ADMIN_DOWN the route would be uninstalled. Is this the desired behavior? I think that when in BSS_ADMIN_DOWN state the route should be kept installed (BFD session is "working" its just administratively shutdown).

[sougatahitcs]

What happens when we transition to BFD session down and then back to administratively down?
Sougata>>
holddown timer starts only when bss->previous_state == BSS_ADMIN_DOWN && !sn->path_down. So in this case no change, route already removed when BFD session went down. admin down will only cancle the holddown timer.
@rzalamena

[riw777]

looks good, waiting on @rzalamena 's comments

[rzalamena]

Looks good, please just apply frrbot formating patch before we can merge it.

[riw777]

looks good