Agent: BFD visibility and optional management for faster failover detection

[berendt]

One of four independent issues improving multi-node FIP failover speed
(BFD visibility/management, active drain handshake, standby pre-staging,
failover hot-path tightening). They can be implemented in any order.

Problem

Ungraceful failover (node crash, partition, ovn-controller crash) is
dominated by OVN's BFD-based detection of the dead gateway chassis —
typically 3×1s ≈ 3s before the chassisredirect Port_Binding is even moved.
The agent's own reaction after that is already <1s (immediateStateRefresh
bypasses the event debounce). The BFD timers are currently invisible to the
agent and untuned, so operators have no signal on what the real failover
floor is, and no agent-side lever to lower it.

Proposal

Add a new bfd.go with three parts. Part 1 is on by default; parts 2 and 3
are opt-in and toggle independently.

1. BFD check (default on). On each reconcile:
   • OVN tunnel BFD: enumerate local Geneve tunnel interfaces via ovs-vsctl
     (find Interface type=geneve), read the bfd column, estimate
     detection time = detect multiplier × negotiated min_rx/min_tx.
   • FRR BGP BFD: read vtysh -c "show bfd peers json".
   • Export ovn_network_bfd_detect_seconds{layer="ovn"|"frr"} and
     slog.Warn when the estimate exceeds bfd_check_max_detect.
2. OVN tunnel BFD management (default off, ovn_bfd_manage). Set
   bfd:min_rx/bfd:min_tx on the local Geneve tunnel interfaces via
   ovs-vsctl. ovn-controller sets only bfd:enable=true; verify
   against the deployed OVN version that ovn-controller preserves
   operator-set timer keys before relying on this.
3. FRR BGP-session BFD management (default off, frr_bfd_manage).
   Discover VRF BGP neighbors (show bgp vrf <vrf> neighbors json), then set
   neighbor <peer> bfd plus a bfd profile with the configured timers.
   Closes the "stale /32 from the dead node" gap on a full node crash.
   Document that the fabric side must enable BFD too.

Scope

• New file bfd.go; OVS helpers extend ovs.go; vtysh helpers reuse
  runVtysh (routing.go:66) and the ovsWrapper prefix (routing.go:30).
• New config options in config.go (with defaults; docgen.go regenerates
  docs/reference/configuration.md):
  • bfd_check_enabled (default true)
  • bfd_check_max_detect (default 1s)
  • ovn_bfd_manage (default false), ovn_bfd_min_rx_ms /
    ovn_bfd_min_tx_ms (default 150)
  • frr_bfd_manage (default false), frr_bfd_min_rx_ms /
    frr_bfd_min_tx_ms (default 150), frr_bfd_multiplier (default 3)
• New metrics in metrics.go.
• Wire the check/manage calls into reconcile (agent.go:230).

Out of scope

• BGP neighbor peering configuration — the agent only manages static
  routes and prefix-lists today; this issue adds only the bfd knob to
  already-configured neighbors.
• Changing OVN's HA model (Gateway_Chassis vs HA_Chassis_Group).

Acceptance criteria

• With defaults, the agent logs a warning and sets
  ovn_network_bfd_detect_seconds when BFD detection is estimated above
  bfd_check_max_detect; no OVS/FRR state is modified.
• ovn_bfd_manage=true sets the timer keys on local Geneve tunnels and
  is idempotent across reconciles.
• frr_bfd_manage=true enables BFD on the VRF's BGP neighbors and is
  idempotent.
• Both manage flags are independently toggleable and default off.
• Table tests for bfd.go with stubbed ovs-vsctl/vtysh (pattern:
  execOVSHook / execVtyshHook).
• docs/explanation/ describes the failover-detection floor and the
  recommended BFD values; docs/reference/configuration.md regenerated.

Implementation hints

• The repo docs already assert "typically 3×1s = 3 seconds"
  (docs/explanation/gateway-drain.md) — use that as the baseline to beat.
• OVS BFD detection time = detect multiplier × negotiated min_rx/min_tx;
  with min_rx=min_tx=150ms and mult 3 the floor drops to ~450ms.
• Keep the check cheap: it runs every reconcile, so cache the tunnel list and
  only re-enumerate on an event-triggered reconcile.

[berendt]

Implementation Plan (issue #128)

Summary

• Add a new bfd.go (flat package main, like every other source file) containing the three BFD parts: a default-on detection-time check for OVN Geneve tunnels and FRR BFD peers, an opt-in OVN tunnel timer manager, and an opt-in FRR BGP-session BFD manager. All external I/O goes through the existing runOVS/runVtysh seams so the mandated table tests use the existing execOVSHook/execVtyshHook recorders unchanged.
• Nine new config options land in config.go following the existing four-layer pattern (defaults literal → applyFileConfig → applyEnvConfig → fs.Visit), one new GaugeVec lands in metrics.go, and one a.reconcileBFD(trigger) call is wired at the end of Agent.reconcile so the failover hot path (route programming) is never delayed. Reference docs are regenerated by the existing tools/docgen; a new Diátaxis explanation page documents the detection floor and recommended values.
• This is the smallest change set covering all three work packages and all six acceptance criteria: no new packages, no refactors, one new source file plus targeted extensions to the files the issue itself names.

Ground-Truth Notes

• bfd.go — missing (to be created), no file of that name exists; package main flat layout confirmed.
• runVtysh (routing.go:66 per issue) — confirmed, moved to routing.go:79; dispatches through execVtyshHook (routing.go:75).
• ovsWrapper (routing.go:30 per issue) — confirmed, now routing.go:31; already honored by runOVS (ovs.go:73) via ovsCmd (ovs.go:62).
• reconcile (agent.go:230) — confirmed at exactly that line; receives a trigger label ("startup", "event", "periodic"), which is the lever for the "re-enumerate only on event-triggered reconcile" caching hint.
• config.go — confirmed; docgen (tools/docgen/parse.go) parses the Config struct, the cfg := Config{...} defaults literal, the flag var block, the fs.Visit switch, applyFileConfig, and applyEnvConfig — a new option must appear in all of these plus the configFile struct for docs/reference/configuration.md to regenerate correctly.
• docgen.go — confirmed; anchors go generate ./..., implemented in tools/docgen, exposed as make docs-gen / make docs-gen-check (CI fails on stale docs/reference/).
• metrics.go — confirmed; namespace const metricsNamespace = "ovn_network_agent" (metrics.go:15). The issue's metric name ovn_network_bfd_detect_seconds cannot be produced without breaking the repo-wide namespace; the plan uses ovn_network_agent_bfd_detect_seconds (see Risks).
• execOVSHook / execVtyshHook test pattern — confirmed: ovsRecorder (ovs_test.go:16) and vtyshRecorder (routing_test.go:16), both with .on(args, out, err) and .hook(); agent-level tests construct &Agent{cfg: ..., routing: &RouteManager{...hooks...}} directly (agent_test.go:239-262).
• docs/explanation/gateway-drain.md — confirmed; asserts "typically 3×1s = 3 seconds" at line 12.
• docs/reference/configuration.md — confirmed, generated ("do not edit by hand" banner); likewise cli.md and metrics.md.
• ovn-network-agent.default — states it "lists every env var consumed by the agent", so the new env vars must be added there; ovn-network-agent.yaml.sample documents every YAML key as commented defaults.
• docs/.vitepress/config.ts and README.md — both enumerate every explanation page; a new page must be registered in both.
• immediateStateRefresh (cited in the issue's problem statement) — confirmed at ovn.go:819; no change needed there.

Work Breakdown Coverage

• Part 1 — BFD check (default on) — covered by Change Set entries for bfd.go, ovs.go, routing.go, agent.go, metrics.go, config.go; Commits 1–3; Test Plan entries for parsing, estimates, cache, read-only defaults, and metrics; Documentation Plan (explanation page + regenerated reference).
• Part 2 — OVN tunnel BFD management (ovn_bfd_manage, default off) — covered by bfd.go EnsureOVNBFDTimers, ovs.go setInterfaceBFDTimers, config options; Commit 4; idempotency table tests; explanation-page caveat on ovn-controller ownership.
• Part 3 — FRR BGP-session BFD management (frr_bfd_manage, default off) — covered by bfd.go EnsureFRRBFD/VRFBGPNeighbors, config options; Commit 5; idempotency table tests; explanation-page note that the fabric side must enable BFD too.
• Cross-cutting scope bullets (config options + docgen regeneration, new metric, reconcile wiring) — covered by Commits 1, 2, 3 respectively.

User Stories

• As a cloud operator running OVN gateway nodes, I want the agent to estimate and export the BFD detection floor per layer (OVN tunnels, FRR BGP) and warn when it exceeds my target, so that I know what ungraceful FIP failover actually costs before tuning anything — derived.
• As a cloud operator, I want to opt into agent-managed bfd:min_rx/bfd:min_tx on local Geneve tunnels, so that dead-gateway detection drops from ~3 s toward ~450 ms without hand-editing OVS on every node — derived.
• As a cloud operator, I want to opt into BFD on the VRF's already-configured BGP sessions, so that the fabric withdraws a crashed node's stale /32 announcements in sub-second time instead of waiting for BGP hold timers — derived.

Change Set

• config.go — modify: add nine fields to Config with doc comments (docgen surfaces the trailing/leading comment): BFDCheckEnabled bool, BFDCheckMaxDetect time.Duration, OVNBFDManage bool, OVNBFDMinRxMs int, OVNBFDMinTxMs int, FRRBFDManage bool, FRRBFDMinRxMs int, FRRBFDMinTxMs int, FRRBFDMultiplier int. Add matching configFile fields with YAML tags bfd_check_enabled (*bool), bfd_check_max_detect (string, duration), ovn_bfd_manage (*bool), ovn_bfd_min_rx_ms/ovn_bfd_min_tx_ms (*int), frr_bfd_manage (*bool), frr_bfd_min_rx_ms/frr_bfd_min_tx_ms (*int), frr_bfd_multiplier (*int). Declare flags in the loadConfig var block: --bfd-check-enabled (fs.Bool, default true), --bfd-check-max-detect (fs.String, default "", usage "Max acceptable estimated BFD detection time before warning (e.g. 1s)"), --ovn-bfd-manage (fs.Bool, false), --ovn-bfd-min-rx-ms/--ovn-bfd-min-tx-ms (fs.Int, 150), --frr-bfd-manage (fs.Bool, false), --frr-bfd-min-rx-ms/--frr-bfd-min-tx-ms (fs.Int, 150), --frr-bfd-multiplier (fs.Int, 3). Add defaults to the cfg := Config{...} literal: BFDCheckEnabled: true, BFDCheckMaxDetect: time.Second, OVNBFDMinRxMs: 150, OVNBFDMinTxMs: 150, FRRBFDMinRxMs: 150, FRRBFDMinTxMs: 150, FRRBFDMultiplier: 3 (manage bools stay zero-value false, mirroring DryRun). Add the nine case clauses to the fs.Visit switch (duration parsed via time.ParseDuration like drain-timeout). Extend applyFileConfig (duration with warn-on-invalid like DrainTimeout) and applyEnvConfig: OVN_NETWORK_BFD_CHECK_ENABLED ("0"/"false" → false, mirroring VethLeakEnabled), OVN_NETWORK_BFD_CHECK_MAX_DETECT (duration, warn on invalid), OVN_NETWORK_OVN_BFD_MANAGE and OVN_NETWORK_FRR_BFD_MANAGE ("1"/"true" → true, mirroring DryRun), OVN_NETWORK_OVN_BFD_MIN_RX_MS, OVN_NETWORK_OVN_BFD_MIN_TX_MS, OVN_NETWORK_FRR_BFD_MIN_RX_MS, OVN_NETWORK_FRR_BFD_MIN_TX_MS, OVN_NETWORK_FRR_BFD_MULTIPLIER (fmt.Sscanf int pattern). Extend validateConfig (gated on the relevant enable flag, mirroring the VethLeakEnabled precedent): BFDCheckEnabled requires BFDCheckMaxDetect > 0; OVNBFDManage requires OVNBFDMinRxMs/OVNBFDMinTxMs in 1–60000; FRRBFDManage requires FRRBFDMinRxMs/FRRBFDMinTxMs in 10–60000 and FRRBFDMultiplier in 2–255 (FRR's accepted ranges). Serves Part 1–3 config scope.

• metrics.go — modify: add bfdDetectSeconds *prometheus.GaugeVec to metricsRegistry (comment group "BFD detection"); construct in newMetricsRegistry with Name: "bfd_detect_seconds", label layer, Help: "Estimated BFD failure-detection time in seconds per layer (worst case across OVN Geneve tunnels / FRR BFD peers in the VRF). 0 = not measured."; add to reg.MustRegister(...); bootstrap m.bfdDetectSeconds.WithLabelValues("ovn").Set(0) and WithLabelValues("frr").Set(0) (docgen reads these bootstrap calls to render label values); add nil-safe helper setBFDDetectSeconds(layer string, seconds float64) in the recording-helpers section. Full metric name: ovn_network_agent_bfd_detect_seconds.

• routing.go — modify: add two fields to the RouteManager struct next to the segments cache: bfdTunnels []bfdTunnel and bfdTunnelsScanned bool, with a comment stating the constraint: the tunnel cache is refreshed only on startup/event-triggered reconciles so the per-cycle BFD check costs zero OVS execs on periodic cycles, and bfdTunnelsScanned distinguishes "never enumerated" from "enumerated, none found". No other changes.

• ovs.go — modify (the issue's "OVS helpers extend ovs.go"): add two thin exec helpers at the end of the file: func (rm *RouteManager) findGeneveInterfaces() ([]byte, error) running rm.runOVS("ovs-vsctl", "--format=json", "--columns=name,bfd,bfd_status", "find", "Interface", "type=geneve"), and func (rm *RouteManager) setInterfaceBFDTimers(name string, minRxMs, minTxMs int) error running rm.runOVS("ovs-vsctl", "set", "Interface", name, fmt.Sprintf("bfd:min_rx=%d", minRxMs), fmt.Sprintf("bfd:min_tx=%d", minTxMs)) and wrapping errors with output per house style.

• bfd.go — create: all BFD logic, structured as:

  • Constants: frrBFDProfileName = "ovn-network-agent", OVS BFD defaults ovsBFDDefaultMinRxMs = 1000, ovsBFDDefaultMinTxMs = 100, ovsBFDDefaultMult = 3 (the vswitchd defaults that yield the documented 3×1 s floor).
  • type bfdTunnel struct — Name string; Enabled bool; MinRxMs, MinTxMs, Mult int parsed from the bfd column (absent keys fall back to the OVS defaults); method detectTime() time.Duration = Mult × max(MinRxMs, MinTxMs) ms (the issue's estimate formula; with 150/150/3 → 450 ms).
  • JSON parsing for ovs-vsctl --format=json find output: a struct {Data [][]json.RawMessage; Headings []string} plus a helper decoding the OVSDB ["map",[[k,v],...]] notation into map[string]string; parseGeneveTunnels([]byte) ([]bfdTunnel, error) as a pure function (table-testable without hooks).
  • func (rm *RouteManager) GeneveTunnels(refresh bool) ([]bfdTunnel, error) — returns the cache; enumerates via findGeneveInterfaces + parseGeneveTunnels when refresh is true or !rm.bfdTunnelsScanned, then stores it.
  • func (rm *RouteManager) EnsureOVNBFDTimers(minRxMs, minTxMs int) (int, error) — Part 2: honors rm.dryRun ("[dry-run] would set BFD timers …"); for each cached tunnel whose MinRxMs/MinTxMs differ from the desired values, calls setInterfaceBFDTimers and updates the cached entry in place (this is what makes the second reconcile issue zero writes — AC 2 idempotency); returns the changed count for logging. Never touches bfd:enable (ovn-controller owns it).
  • type frrBFDPeer struct — parsed from vtysh -c "show bfd peers json" (a JSON array; bfdd uses kebab-case keys): Peer string \json:"peer"`, VRF string `json:"vrf"`, Status string `json:"status"`, ReceiveInterval, TransmitInterval, RemoteTransmitInterval, DetectMultiplier int(tagsreceive-interval, transmit-interval, remote-transmit-interval, detect-multiplier); method detectTime() time.Duration=DetectMultiplier × max(ReceiveInterval, RemoteTransmitInterval when > 0 else TransmitInterval)` ms.
  • func (rm *RouteManager) ListFRRBFDPeers() ([]frrBFDPeer, error) — runs rm.runVtysh("-c", "show bfd peers json"); tolerates non-JSON output (bfdd not running, "% ..." messages) by returning nil, nil with a debug log rather than an error — the check must not spam errors on fabrics without BFD.
  • type frrBGPNeighbor struct — Addr string; LocalAS int64; HasBFD bool; RxMinInterval, TxMinInterval, DetectMultiplier int parsed from vtysh -c "show bgp vrf <vrf> neighbors json" (object keyed by neighbor address; camelCase fields localAs, peerBfdInfo{detectMultiplier, rxMinInterval, txMinInterval}; HasBFD = peerBfdInfo present; missing timer fields parse to 0 = unknown, compared only when > 0).
  • func (rm *RouteManager) VRFBGPNeighbors() ([]frrBGPNeighbor, int64, error) — runs the neighbors command with rm.vrfName; returns neighbors plus the discovered localAs; treats non-JSON output (no BGP instance for the VRF) as empty with a debug log. Neighbor keys are defensively validated with net.ParseIP or the existing isValidIdentifier (interface-based neighbors) before ever being interpolated into a vtysh command.
  • func (rm *RouteManager) EnsureFRRBFD(minRxMs, minTxMs, multiplier int) error — Part 3: honors rm.dryRun; calls VRFBGPNeighbors; neighbors needing work are those with HasBFD == false or with reported timers > 0 differing from the configured values; if none, returns without issuing any config (AC 3 idempotency). Otherwise issues ONE batched runVtysh call, mirroring the AddFRRRoutes batching style: -c "conf t" -c "bfd" -c "profile ovn-network-agent" -c "receive-interval <rx>" -c "transmit-interval <tx>" -c "detect-multiplier <mult>" -c "exit" -c "exit" -c "router bgp <localAs> vrf <vrf>" then per needing neighbor (sorted for determinism) -c "neighbor <addr> bfd" -c "neighbor <addr> bfd profile ovn-network-agent", ending -c "end" — exactly the "neighbor <peer> bfd plus a bfd profile" shape the issue prescribes.
  • func (a *Agent) reconcileBFD(trigger string) — the orchestrator wired into reconcile: returns immediately when !a.cfg.BFDCheckEnabled && !a.cfg.OVNBFDManage && !a.cfg.FRRBFDManage. OVN layer (skipped when a.cfg.PortForwardOnly — no OVS on such nodes): tunnels, err := a.routing.GeneveTunnels(trigger != "periodic"); on success, run EnsureOVNBFDTimers when OVNBFDManage, then when BFDCheckEnabled compute the worst detectTime() across Enabled tunnels, setBFDDetectSeconds("ovn", est.Seconds()) when > 0, and slog.Warn("estimated BFD detection time exceeds bfd_check_max_detect", "layer", "ovn", "estimate", est, "max_detect", ..., "tunnel", worstName) when est > a.cfg.BFDCheckMaxDetect. FRR layer (both modes — FRR announces routes in port-forward-only mode too): run EnsureFRRBFD when FRRBFDManage; when BFDCheckEnabled, ListFRRBFDPeers(), filter to VRF == a.cfg.VRFName, compute worst detectTime(), set the frr gauge and warn above threshold analogously. All failures are logged (slog.Warn) and never abort the reconcile.

• agent.go — modify: insert exactly one call a.reconcileBFD(trigger) in reconcile, after a.checkFRRRouteActivity(desiredIPs) (currently agent.go:444) and before the stale-chassis block — after route programming so the event-triggered failover hot path is not delayed.

• bfd_test.go — create: table tests per the Test Plan below, reusing ovsRecorder/vtyshRecorder.

• config_test.go — modify: extend the existing default/env/file/flag/validation tests (see Test Plan).

• metrics_test.go — modify: extend registry, nil-safety, and gauge tests (see Test Plan).

• ovn-network-agent.yaml.sample — modify: add a commented "BFD failover detection" section documenting all nine keys with defaults, in the established comment style.

• ovn-network-agent.default — modify: add the nine commented OVN_NETWORK_BFD_*/OVN_NETWORK_OVN_BFD_*/OVN_NETWORK_FRR_BFD_* env vars with defaults (the file claims to list every env var).

• docs/reference/configuration.md, docs/reference/cli.md, docs/reference/metrics.md — modify (generated): regenerate via make docs-gen; never hand-edit.

• docs/explanation/bfd-failover-detection.md — create: explanation-mode page (see Documentation Plan).

• docs/explanation/gateway-drain.md — modify: one-sentence cross-link after the "typically 3×1s = 3 seconds" statement (line 12) pointing to the new page.

• docs/.vitepress/config.ts — modify: add { text: 'BFD failover detection', link: '/explanation/bfd-failover-detection' } to the Explanation sidebar group.

• README.md — modify: add the page to the Explanation link list (README mirrors every published page).

Commit Sequence

1. config: add BFD check and manage options — config.go, config_test.go, ovn-network-agent.yaml.sample, ovn-network-agent.default, regenerated docs/reference/configuration.md + cli.md. Why: land the whole option surface first so every later commit compiles against final config; regen in the same commit keeps make docs-gen-check green per commit.
2. metrics: add bfd_detect_seconds gauge — metrics.go, metrics_test.go, regenerated docs/reference/metrics.md. Why: the recording helper must exist before the check uses it; mirrors the metrics: add localnet_segments gauge precedent.
3. bfd: add per-reconcile BFD detection-time check — bfd.go (Part 1: types, parsers, estimates, cache, GeneveTunnels, ListFRRBFDPeers, reconcileBFD check paths), ovs.go (findGeneveInterfaces), routing.go (cache fields), agent.go (wiring), bfd_test.go. Why: default-on visibility lands complete and read-only (AC 1).
4. bfd: add opt-in OVN tunnel BFD timer management — bfd.go (EnsureOVNBFDTimers + manage branch in reconcileBFD), ovs.go (setInterfaceBFDTimers), bfd_test.go. Why: Part 2 isolated for review, idempotency proven by tests (AC 2).
5. bfd: add opt-in FRR BGP-session BFD management — bfd.go (frrBGPNeighbor, VRFBGPNeighbors, EnsureFRRBFD + manage branch), bfd_test.go. Why: Part 3 isolated for review (AC 3, AC 4).
6. docs: describe the BFD failover-detection floor — docs/explanation/bfd-failover-detection.md, docs/.vitepress/config.ts, README.md, docs/explanation/gateway-drain.md cross-link. Why: AC 6 documentation, matching the repo's docs: commit convention.

Commit trailers per repo convention: AI-assisted: Claude Code and Signed-off-by: Christian Berendt <berendt@osism.tech> (not Co-Authored-By).

Test Plan

All in bfd_test.go unless noted, table-driven with t.Run subtests per house style:

• TestParseGeneveTunnels — parses the --format=json fixture with OVSDB ["map",...] columns: explicit timers, absent keys falling back to 1000/100/3 defaults, enable absent → Enabled=false, empty data, malformed JSON → error.
• TestBFDTunnelDetectTime — estimate table: (1000,100,3) → 3 s (the documented floor), (150,150,3) → 450 ms (the issue's target), disabled tunnels excluded from the worst-case aggregate.
• TestGeneveTunnelsCache — GeneveTunnels(true) issues the exact ovs-vsctl --format=json --columns=name,bfd,bfd_status find Interface type=geneve command; GeneveTunnels(false) afterwards issues zero OVS calls (locks the periodic-reconcile zero-exec property); false with an unscanned cache still enumerates.
• TestReconcileBFDDefaultsIsReadOnly — AC 1: &Agent{cfg: Config{BFDCheckEnabled: true, BFDCheckMaxDetect: time.Second, VRFName: "vrf-provider"}, routing: rm} with both recorders canned (default-timer tunnels → 3 s estimate; one FRR peer); after a.reconcileBFD("startup") no recorded command contains set Interface, conf t, or router bgp; withTestMetrics(t) (metrics_test.go:16) gauge shows bfd_detect_seconds{layer="ovn"} == 3 and the expected frr value. (Warn-log emission is not asserted — the repo has no log-capture precedent; the threshold comparison feeding the warn is covered via the gauge and estimate tables.)
• TestEnsureOVNBFDTimers — AC 2: tunnel with 1000/100 → exactly one ovs-vsctl set Interface <name> bfd:min_rx=150 bfd:min_tx=150 call; second invocation → zero calls (cache updated in place); mixed already-correct/incorrect tunnels → only the incorrect one written; exec error propagated; dry-run issues nothing.
• TestListFRRBFDPeers — parses the kebab-case bfdd JSON array; VRF filtering by caller; remote-transmit-interval refinement in detectTime; "% BFD daemon not running"-style non-JSON output → nil, nil; empty output → nil, nil.
• TestVRFBGPNeighbors — parses neighbors JSON: localAs extraction, peerBfdInfo presence → HasBFD, timer fields, interface-named neighbors accepted via isValidIdentifier, hostile key rejected, "% No BGP process"-style output → empty without error.
• TestEnsureFRRBFD — AC 3: one of two neighbors lacking peerBfdInfo → single batched vtysh call containing the profile block (profile ovn-network-agent, receive-interval 150, transmit-interval 150, detect-multiplier 3), router bgp 64512 vrf vrf-provider, and neighbor <addr> bfd + neighbor <addr> bfd profile ovn-network-agent for the missing neighbor only; all neighbors with matching timers → zero config calls; drifted reported timers → rewrite issued; no neighbors → no-op; dry-run issues nothing.
• TestReconcileBFDManageFlagsIndependent — AC 4: table over the four (OVNBFDManage, FRRBFDManage) combinations asserting which mutating command classes appear in the recorders; also PortForwardOnly: true skips the OVN layer but still runs the FRR layer, and all-flags-off issues no calls at all.
• config_test.go — extend TestLoadConfigDefaults (check BFDCheckEnabled=true, BFDCheckMaxDetect=1s, manage flags false, 150/150/150/150/3), TestApplyEnvConfig, TestApplyFileConfig, TestLoadConfigCLIFlags with the nine new options, and TestValidateConfig with: bfd_check_max_detect=0 with check enabled → error; ovn_bfd_min_rx_ms=0 with ovn_bfd_manage=true → error; frr_bfd_multiplier=1 with frr_bfd_manage=true → error; same invalid values with manage flags off → accepted (gated validation, VethLeakEnabled precedent).
• metrics_test.go — add ovn_network_agent_bfd_detect_seconds to wantMetrics in TestNewMetricsRegistryRegistersAllCollectors; add setBFDDetectSeconds("ovn", 3) to TestRecordingHelpersAreNilSafe; add TestSetBFDDetectSeconds asserting per-layer values after withTestMetrics(t).
• Integration (test/integration) and E2E (test/e2e) suites are deliberately not extended: the issue's acceptance criteria require only table tests with stubbed ovs-vsctl/vtysh.

Documentation Plan

• docs/explanation/bfd-failover-detection.md — new explanation-mode page (single H1 "BFD failover detection", no how-to steps): why ungraceful failover is dominated by BFD (the 3×1 s floor already asserted in gateway-drain.md, agent reaction < 1 s via immediateStateRefresh); the estimate formulas per layer (OVS: mult × max(min_rx, min_tx), local values only — the negotiated time can be higher if the peer keeps slow defaults, so timers must be lowered fleet-wide; FRR: multiplier × max of receive/remote-transmit intervals); recommended values (150 ms / 150 ms / mult 3 → ~450 ms) and the CPU-cost trade-off of aggressive timers; the ovn_bfd_manage caveat verbatim from the issue (ovn-controller sets only bfd:enable=true — verify against the deployed OVN version that operator-set timer keys are preserved; the agent re-applies on the next event-triggered reconcile if they are reverted); the frr_bfd_manage notes (closes the stale-/32 gap on full node crash; the fabric side must enable BFD too; FRR must run bfdd); pointers to the configuration and metrics reference pages.
• docs/explanation/gateway-drain.md — append one sentence + link at the line-12 BFD mention: "see BFD failover detection for measuring and lowering this floor."
• docs/reference/configuration.md, docs/reference/cli.md, docs/reference/metrics.md — regenerated by make docs-gen (nine option rows, nine flags, one metric row appear automatically).
• docs/.vitepress/config.ts — Explanation sidebar entry; README.md — Explanation link-list entry.
• ovn-network-agent.yaml.sample and ovn-network-agent.default — new commented sections as described in the Change Set.

Verification Commands

• gofmt -l . (must print nothing)
• go vet ./...
• go test -race ./... (CI additionally enforces a total-coverage floor via .github/workflows/test.yml)
• make docs-gen && git diff --exit-code -- docs/reference/ (CI's stale-docs gate)
• go build ./... and GOOS=linux go build ./... (matches make build target platform)
• go mod tidy && git diff --exit-code go.mod go.sum (no new dependencies expected)

Risks & Open Questions

• Metric name deviates from the issue's literal text. The issue writes ovn_network_bfd_detect_seconds, but every metric in this repo is prefixed by metricsNamespace = "ovn_network_agent" (metrics.go:15) and docgen derives the rendered name from that constant. The plan uses ovn_network_agent_bfd_detect_seconds; the issue's shorter name appears to be shorthand. If the literal name is a hard requirement, that contradicts the repo's metric convention and needs a maintainer call.
• ovn-controller ownership of the bfd column (issue's own caveat): whether operator-set bfd:min_rx/bfd:min_tx survive ovn-controller's tunnel management varies by OVN version and cannot be verified from this repo. Mitigated by: default off, documentation caveat, and self-healing re-apply on the next event-triggered reconcile (worst case: a rewrite per event cycle, logged).
• FRR JSON shape variance across versions (peerBfdInfo field names, kebab-vs-camel case between bfdd and bgpd output): parsers treat missing fields as "unknown" and degrade to presence-only idempotency rather than erroring; fixtures encode FRR 8/9-style output. A fabric running a much older FRR may see profile rewrites more often than strictly necessary, never incorrect config.
• One-sided OVS timer estimate is a lower bound: bfd_status does not expose the remote side's intervals, so the ovn layer estimate uses local configuration only; documented in the explanation page.
• localAs discovery takes the first neighbor's localAs from the neighbors JSON; multiple local-AS setups (confederations) in one VRF are not handled — considered out of scope alongside the issue's "no BGP peering configuration" non-goal.
• Port-forward-only mode: the issue doesn't address it; the plan skips the OVN layer (no OVS on such nodes) but runs the FRR layer (FRR announces VIP /32s there and the stale-route gap applies identically). This is one if gate; flagged here since it's an interpretation.
• Repeated warn logging (once per reconcile while above threshold) matches the repo's existing behavior for persistent conditions (checkFRRRouteActivity logs every cycle).

Status

STATUS: PLAN_READY

---

Implementation plan generated by planwerk-agent ffc4daf implement with Claude

[berendt]

Implementation Report (issue #128)

Work Breakdown Coverage

• 1. BFD check (default on) — done — bfd.go (parseGeneveTunnels, GeneveTunnels with the event-only re-enumeration cache, ListFRRBFDPeers, detectTime on both layers, reconcileBFD), ovs.go:findGeneveInterfaces, routing.go cache fields, agent.go:447 wiring, metrics.go gauge. Commits f88eee5, a103ce6, 113ce96. Tests: TestParseGeneveTunnels, TestBFDTunnelDetectTime, TestWorstTunnelDetectTime, TestGeneveTunnelsCache, TestListFRRBFDPeers, TestReconcileBFDDefaultsIsReadOnly, TestReconcileBFDToleratesLayerFailures. Docs: new explanation page.
• 2. OVN tunnel BFD management (ovn_bfd_manage, default off) — done — bfd.go:EnsureOVNBFDTimers, ovs.go:setInterfaceBFDTimers, manage branch in reconcileOVNBFD. Commit 73bd177. Tests: TestEnsureOVNBFDTimers (5 subtests incl. idempotency, partial drift, exec error, dry-run), TestReconcileBFDOVNManageWritesTimersAndEstimatesFromThem. Docs: the ovn-controller ownership caveat.
• 3. FRR BGP-session BFD management (frr_bfd_manage, default off) — done — bfd.go:VRFBGPNeighbors, EnsureFRRBFD, needsBFD, isValidBGPNeighbor, manage branch in reconcileFRRBFD. Commit fa448e3. Tests: TestVRFBGPNeighbors, TestFRRBGPNeighborNeedsBFD, TestEnsureFRRBFD (7 subtests), TestReconcileBFDFRRManageWithoutCheck. Docs: the "fabric side must enable BFD too" note.

Acceptance Criteria

• With defaults, the agent logs a warning and sets ovn_network_bfd_detect_seconds when BFD detection is estimated above bfd_check_max_detect; no OVS/FRR state is modified.
  • Status: satisfied (metric renamed — see Deviations)
  • Evidence: bfd.go:298-337 (reconcileOVNBFD/reconcileFRRBFD). TestReconcileBFDDefaultsIsReadOnly (bfd_test.go) asserts no set Interface/conf t/router bgp command is issued and both gauges read 3s. TestReconcileBFDNoEnabledTunnelsLeavesGaugeUnmeasured covers the edge case where no session runs.
• ovn_bfd_manage=true sets the timer keys on local Geneve tunnels and is idempotent across reconciles.
  • Status: satisfied
  • Evidence: bfd.go:170-196. TestEnsureOVNBFDTimers/drifted_tunnel_is_written_once_and_then_idempotent asserts the second call issues zero commands; /exec_error_is_propagated_and_the_cache_is_not_advanced covers the error path.
• frr_bfd_manage=true enables BFD on the VRF's BGP neighbors and is idempotent.
  • Status: satisfied
  • Evidence: bfd.go:361-411. TestEnsureFRRBFD/idempotent_when_every_neighbor_already_matches asserts zero config calls; /a_neighbor_without_a_local_AS_is_an_error,_not_a_broken_command and /neighbor_discovery_failure_is_propagated cover error paths.
• Both manage flags are independently toggleable and default off.
  • Status: satisfied
  • Evidence: TestReconcileBFDManageFlagsIndependent (4-combination table asserting which command classes appear), TestLoadConfigBFDManageFlagsIndependent, TestLoadConfigDefaults.
• Table tests for bfd.go with stubbed ovs-vsctl/vtysh (pattern: execOVSHook/execVtyshHook).
  • Status: satisfied
  • Evidence: bfd_test.go reuses ovsRecorder/vtyshRecorder unchanged. Error/edge paths include malformed JSON, a bare scalar where a map is expected, a hostile neighbor identifier (TestVRFBGPNeighbors/hostile_neighbor_identifier_is_skipped), and bfdd-not-running output.
• docs/explanation/ describes the failover-detection floor and the recommended BFD values; docs/reference/configuration.md regenerated.
  • Status: satisfied
  • Evidence: docs/explanation/bfd-failover-detection.md (new, registered in docs/.vitepress/config.ts and README.md, cross-linked from gateway-drain.md); reference pages regenerated by make docs-gen.

Commits

• f88eee5 config: add BFD check and manage options
• a103ce6 metrics: add bfd_detect_seconds gauge
• 113ce96 bfd: add per-reconcile BFD detection-time check
• 73bd177 bfd: add opt-in OVN tunnel BFD timer management
• fa448e3 bfd: add opt-in FRR BGP-session BFD management
• 1b2f590 docs: describe the BFD failover-detection floor

Local verification

• gofmt -l . — pass (no output)
• go vet ./... — pass
• go build ./... && GOOS=linux go build ./... — pass
• go test -race -count=1 ./... — pass
• go test -coverprofile total coverage 83.5% — pass (CI floor is 68.0)
• make docs-gen && git diff --exit-code -- docs/reference/ — pass (CI's stale-docs gate)
• go mod tidy && git diff --exit-code go.mod go.sum — pass (no new dependencies)
• npm run docs:build — pass (VitePress fails on dead links, so this validates the new page's cross-links)
• Each of the six commits builds and vets independently (checked in a temporary worktree)

Branch

• implement/issue-128-bfd-visibility-management (committed, not pushed — the finalize step opens the PR)

Deviations from the issue

• Metric name is ovn_network_agent_bfd_detect_seconds, not ovn_network_bfd_detect_seconds. Every metric in this repo is prefixed by metricsNamespace = "ovn_network_agent" (metrics.go:15), and tools/docgen derives the rendered name from that constant. Producing the issue's literal name would require bypassing the namespace for this one metric, breaking a repo-wide convention and the Prometheus naming pattern. The issue's shorter name reads as shorthand. Flagged by the planning session as needing a maintainer call if the literal name is a hard requirement.
• The bfd_status column is not requested. The plan had --columns=name,bfd,bfd_status, but the issue only asks to "read the bfd column", and nothing consumes bfd_status — bfd_status does not expose the remote endpoint's intervals, so it cannot refine the estimate. Requesting an unused column would be speculative scope; the check uses --columns=name,bfd.
• EnsureOVNBFDTimers returns error, not (int, error). The plan returned a changed-count for logging, but each write is already logged per-tunnel and the tests assert idempotency via the command recorder, so the count had no consumer.
• Port-forward-only mode skips the OVN layer (no OVS on such nodes) but still runs the FRR layer, since FRR announces the VIP /32s there and the stale-route gap applies identically. The issue does not address this mode; the interpretation is one if gate, covered by TestReconcileBFDPortForwardOnlySkipsOVNLayer.
• bfd_check_max_detect default is written 1 * time.Second rather than time.Second: docgen's durationLiteral regex (render.go:235) only matches the N * time.X form, and the bare form rendered as a literal time.Second in the docs table. Writing it in the existing 3 * time.Second style fixes the rendering without touching docgen.

Noticed but not touching

• tools/docgen/render.go:235 — durationPattern does not match a bare time.Second / time.Minute default, silently emitting the Go expression into docs/reference/configuration.md. Worked around in config.go rather than fixed; docgen is not in the issue's Affected Areas.
• config_test.go:321 — the comment "VethLeakEnabled was overridden to false for this test" in TestLoadConfigDefaults is stale: fullModeArgs() sets no such flag, so VethLeakEnabled is true there. Pre-existing, unrelated to this issue.
• ListFRRBFDPeers and VRFBGPNeighbors each cost one vtysh exec per reconcile when their feature is enabled. The issue's caching hint targets the tunnel list specifically (done), and checkFRRRouteActivity already runs a vtysh read every cycle, so this matches existing behavior. Left as-is rather than adding unrequested caching.
• EnsureFRRBFD takes the local AS from the first neighbor in sorted order; a confederation with multiple local-AS values in one VRF is not handled. Out of scope under the issue's "BGP neighbor peering configuration" non-goal.

Status

STATUS: DONE

---

Implementation report generated by planwerk-agent ffc4daf implement with Claude

[berendt]

Simplification Report

Applied

• The Geneve tunnel cache optimizes away one exec on a path that already execs — Deleted the bfdTunnels/bfdTunnelsScanned fields and their comment from RouteManager (routing.go); collapsed GeneveTunnels(refresh bool) to GeneveTunnels() with no cache; changed EnsureOVNBFDTimers to take the tunnel slice as a parameter and mutate it in place instead of reaching into the private cache; removed the trigger parameter from reconcileBFD and reconcileOVNBFD, and the tunnels = a.routing.bfdTunnels re-read. The metrics label no longer has control-flow authority over OVS state freshness. Split across the two commits that introduced the pieces: cache + trigger threading folded into 191558e bfd: add per-reconcile BFD detection-time check; the EnsureOVNBFDTimers signature and the cross-package field read folded into 69e99f1 bfd: add opt-in OVN tunnel BFD timer management. The doc sentence describing the cache's refresh window folded into 0084ef2 docs: describe the BFD failover-detection floor.

• frrBGPNeighbor is a field-for-field forwarding copy of the JSON structs — Deleted frrNeighborJSON, kept frrBGPNeighbor as the single type carrying the json tags plus an Addr string \json:"-"`for the map key, and hungneedsBFDoff the nil-ness ofPeerBfdInfoinstead of a duplicatedHasBFDbool. The copy loop inVRFBGPNeighborsis gone. The> 0guards are unchanged, as the finding specified. Folded into36631f3 bfd: add opt-in FRR BGP-session BFD management`.

I kept frrBGPNeighbor as the surviving type name rather than frrNeighborJSON as the finding literally worded it — same two-types-to-one collapse, but it keeps VRFBGPNeighbors() ([]frrBGPNeighbor, ...) reading as a domain call and leaves the signature untouched.

Skipped

None. Neither finding touched validation, error handling, security controls, or accessibility.

Two things worth flagging rather than skipping:

• Tests were rewritten, not weakened. The subtests I dropped asserted on the removed fields and parameter (bfdTunnelsScanned, the refresh argument, cached call counts). Every behavioral assertion survived, and two got stronger: "a node with no tunnels" now asserts the returned slice is empty rather than only counting execs, and the reconcile-level idempotency check now feeds the fake OVS the timers that were just written and asserts no second write — testing the real drift check against live state instead of the cache. I mutation-tested both by deleting the drift check: they fail, and pass again when restored.

• One behavior does change, in the drift case only. With the cache, if ovn-controller reverted the operator-set timer keys, the agent re-applied them on the next event-triggered reconcile. Without it, the agent re-applies on the next reconcile, periodic included — it converges within the reconcile interval instead of waiting for a chassis event. Steady state (nothing external touching the timers) is unchanged: same OVS writes, same bfd_detect_seconds values. I updated the one doc sentence and the two commit-message paragraphs that described the old window so the branch doesn't ship prose contradicting its code.

Diff summary

• Files: agent.go, bfd.go, bfd_test.go, routing.go, docs/explanation/bfd-failover-detection.md
• Approx lines added/removed: +107/-181 (net −74)

Verification

Run on the branch tip and, separately, on each of the six commits individually:

Command	Result
gofmt -l *.go	clean
go vet ./...	PASS
go build ./... and GOOS=linux go build ./...	PASS
go test ./...	PASS
GOOS=linux go vet -tags=integration ./test/integration/...	PASS
make docs-gen-check	clean (no regeneration drift)

All six commits build and pass on their own, so the history bisects. git diff between the pre-rebase working tree I verified and the rebuilt HEAD is empty, confirming the folding changed history without changing the result. The branch still has exactly 6 commits, each ending Assisted-by: Claude:claude-opus-4-8 then Signed-off-by:, with no Co-authored-by. Nothing was pushed and no PR was opened; the pre-rebase state remains recoverable from the reflog.

The one thing I could not run here is the integration suite itself (make test-integration needs Linux + root/CAP_NET_ADMIN); it only vets/compiles on this macOS host.

Status

STATUS: DONE_WITH_CONCERNS

The concern for a human to confirm is the drift-case timing change noted above: dropping the cache means ovn_bfd_manage now re-applies reverted timers on periodic reconciles too, not only event-triggered ones. That is faster self-healing and is what the code, docs, and commit messages now all say — but it is a real difference from what the original commits promised, and the finding was explicitly marked needs-discussion for this reason.

---

Simplification report generated by planwerk-agent ffc4daf implement with Claude

[berendt]

Review Report

Resolved

• bfd_detect_seconds freezes at its last good value when measurement fails — The gauge was only ever written on the success path, so a failed ovs-vsctl find or a tunnel losing bfd:enable left the last estimate exported forever. Both reconcile paths now export worst.Seconds() unconditionally (0 = not measured) and reset the layer's gauge to 0 on the enumeration error path. New test TestReconcileBFDClearsTheEstimateWhenMeasurementStops measures 3s, then fails the next cycle and asserts 0. (folded into 039bf27 bfd: add per-reconcile BFD detection-time check)

• Integer overflow in detectTime() makes the worst tunnel invisible — mult × interval × time.Millisecond wrapped negative past ~9.2e12 ms, and worstTunnelDetectTime then discarded the wrapped value because it starts at 0. Root cause was an unguarded multiplication into time.Duration; both bfdTunnel.detectTime and frrBFDPeer.detectTime had it, so both now go through one saturating helper bfdDetectTime(mult, intervalMs). Tests cover saturation on each type and, critically, that a saturating tunnel wins worstTunnelDetectTime over a healthy 450ms one. (folded into 039bf27)

• Three un-timeouted subprocesses on the single-threaded reconcile loop — runVtysh and runOVS bottomed out in cmd.CombinedOutput() with no deadline, so a vtysh blocked on FRR's configuration lock would keep reconcile() on the stack forever and the SIGTERM drain would never run. Both now use exec.CommandContext with a bounded deadline (10s vtysh, 30s OVS). See the caveat below. (folded into 039bf27)

• The exported estimate reports the timers the agent wanted, not the ones OVS kept — EnsureOVNBFDTimers mutated the caller's slice on a successful write, and reconcileOVNBFD then estimated from that mutated slice. It now leaves tunnels untouched, and the estimate is taken from the bfd column as OVSDB reported it at the top of the cycle. I deliberately did not take the suggested "re-read after write" fix: re-reading milliseconds after the write can still observe the agent's own value before ovn-controller reverts it, whereas reading at the top of the next cycle observes what was actually in force for the whole interval — and costs no extra subprocess. TestReconcileBFDOVNManageReportsRevertedTimers pins this: with OVS always reporting the defaults back, the metric stays at 3, not 0.45. (folded into d845f60 bfd: add opt-in OVN tunnel BFD timer management)

• One vanished tunnel leaves every remaining tunnel untuned — setInterfaceBFDTimers now passes --if-exists, and the loop logs-and-continues on a write failure, returning the first error rather than aborting. TestEnsureOVNBFDTimers/a vanished tunnel does not leave the remaining tunnels untuned fails on the old return err. (folded into d845f60)

• needsBFD compares timers EnsureFRRBFD never writes, so FRR is reconfigured on every reconcile forever — Confirmed empirically: I checked out the pre-fix commit in a scratch worktree and drove EnsureFRRBFD four times with a neighbor reporting bgpd's defaults; it issued four conf t invocations. The timers live in the shared bfd profile that bfdd applies, so bgpd keeps reporting its own session args (3/300/300) per neighbor — comparing them finds every neighbor drifted, forever. Idempotency is now presence-based: hasBFD() checks that bgpd emitted peerBfdInfo at all. The finding's second half (reconcile the profile's timers separately) needs a profileName field I could not verify against a real FRR release, so instead the profile is written once per process (tracked by frrBFDProfileApplied) and again whenever a neighbor must be attached. That keeps the steady-state reconcile free of conf t while still applying a changed timer config on restart — the only time the agent reads its config anyway. Documented in the explanation page. (folded into 4bcb276 bfd: add opt-in FRR BGP-session BFD management)

• localAS is read from a neighbor and can create a second BGP instance with the wrong ASN — Also confirmed empirically: with neighbor 192.0.2.1 local-as 64999 under router bgp 65000, the pre-fix code emitted router bgp 64999 vrf vrf-provider. The instance ASN now comes from parseFRRBGPInstanceAS(show running-config), matching the router bgp <as> vrf <vrf> stanza header — version-stable, immune to the per-session local-as override, and it proves the instance exists. A VRF with no such stanza is an error, not an instance the agent creates; and router bgp is only entered when there is actually a neighbor to attach, so the profile-only path never touches it. (folded into 4bcb276)

• Documentation — Updated the explanation page for the three behaviour changes operators can observe: the gauge resets to 0 when a layer cannot be measured; the OVN estimate is read back from OVSDB, so a reverting ovn-controller keeps the metric pinned at the untuned floor; and an frr_bfd_manage timer change takes effect on agent restart. (folded into 14ba468 docs: describe the BFD failover-detection floor)

Skipped

• bfd_check_max_detect silently keeps the default on an unparseable value — The finding's premise is that this is inconsistent with "every other malformed setting, which fails validation." It isn't: reconcile_interval, drain_timeout, drain_settle_delay, and stale_chassis_grace_period all warn-and-default too (and reconcile_interval at the file layer doesn't even warn). bfd_check_max_detect is the most verbose of them. The finding's concrete scenario — bfd_check_max_detect: 500 in YAML — does not silently default either: the field is a string, so yaml.Unmarshal rejects the bare int and loadConfig returns an error before validation is reached. Making only this one setting fatal would create the inconsistency the finding objects to; fixing all of them changes pre-existing behaviour outside this branch. Worth a separate issue if the project wants strict duration parsing everywhere.

Diff summary

• Files: bfd.go, bfd_test.go, ovs.go, ovs_test.go, routing.go, docs/explanation/bfd-failover-detection.md
• Approx lines added/removed: +514 / −222 (relative to the pre-review branch tip)
• routing.go and ovs_test.go were outside the branch's original file set. runVtysh lives in routing.go (the timeout, plus the frrBFDProfileApplied field), and ovsCmd gained a context.Context parameter, which three pre-existing ovs_test.go call sites pass t.Context() to.

Verification

gofmt -l .        → clean
go vet ./...      → clean
go build ./...    → clean
go test ./...     → ok
go test -race .   → ok
make docs-gen-check → no drift

Each of the six commits was also checked out into an isolated worktree and independently built + tested: all six pass. The folded tree hash (65032e0) is byte-identical to the tree I tested before rebasing.

Status

STATUS: DONE_WITH_CONCERNS

Two things a human should look at:

1. The timeout fix bounds the hang but does not propagate cancellation. runVtysh/runOVS create their own context.WithTimeout(context.Background(), …) rather than threading the reconcile's ctx. Threading it properly means adding ctx to ~11 RouteManager methods across routing.go and ovs.go and their ~20 callers — a refactor well beyond this pass, and the repo has no CommandContext usage anywhere today. What is fixed is the fatal part: reconcile() now always returns, so the SIGTERM drain runs. What remains is that a hung subprocess is not killed early on shutdown; it burns up to its deadline first. The context.Background() call would itself be flagged by the project's Go Context Propagation pattern — I judged the bounded hang to be strictly better than the unbounded one, but the proper fix is a follow-up.

2. The FRR profile-timer reconciliation is restart-scoped, not reconcile-scoped. Finding 5 asked for hasBFD and a profile-name check, verified against the target FRR release with vtysh -c "show bgp vrf <vrf> neighbors json". I could not run that here, and bfd_sess_show does not reliably emit profileName across FRR versions, so I did not build idempotency on a field I can't confirm exists. The consequence is stated plainly in the docs: changing frr_bfd_min_rx_ms takes effect on agent restart. If someone can confirm profileName is present on the deployed FRR, the sentinel can be replaced with a real drift check.

No push, no PR — the commits are local on implement/issue-128-bfd-visibility-management.

---

Review report generated by planwerk-agent ffc4daf implement with Claude

[berendt]

Review Report

Resolved

• EnsureOVNBFDTimers can stall the reconcile goroutine for hours — ovsCommandTimeout bounds one ovs-vsctl invocation, not one reconcile, so a per-tunnel loop had no bound at all. setInterfaceBFDTimers now takes the list of drifted interfaces and writes them in a single ovs-vsctl transaction joined by --; --if-exists makes a tunnel ovn-controller destroyed since the enumeration a no-op clause rather than a failed transaction, so the "one vanished tunnel must not leave the rest untuned" property survives without the error-swallowing loop. A regression test asserts N drifted tunnels produce exactly one invocation. (folded into 5ddb0f9 bfd: add opt-in OVN tunnel BFD timer management)

• bfd_detect_seconds reports the worst and the best state as the same number — the gauge now takes +Inf when a layer runs no BFD session (the fleet's default state, and the one the obvious alert bfd_detect_seconds > 1 must catch) and NaN when the layer could not be read, paired with a new ovn_network_agent_bfd_check_errors_total{layer} counter. The label series bootstrap as NaN rather than 0, so a never-measured layer does not read as instantaneous detection. (folded into aa22e80 metrics: add bfd_detect_seconds gauge for the collector and generated reference, and f4cd63e bfd: add per-reconcile BFD detection-time check for the reconcile paths)

• hasBFD() cannot see a neighbor that has BFD but no profile — hasBFD() answered "does bgpd have any BFD here", while the caller needs "is this neighbor attached to our profile with our timers". peerBfdInfo is now dropped entirely and attachment is read from show running-config: a neighbor counts as configured only when the VRF's router bgp stanza carries neighbor <addr> bfd profile ovn-network-agent. Worth calling out — I checked FRR's source rather than taking the suggested fix as written: lib/bfd.c emits the profile under the JSON key profile, not profileName, and the key has moved across releases. Depending on it would have made the agent re-attach every neighbor on every reconcile in some FRR versions, bouncing the very sessions this feature protects. The config line is the one the agent itself writes and FRR renders back verbatim, so it is version-stable. (folded into 733e6e3 bfd: add opt-in FRR BGP-session BFD management)

• frrBFDProfileApplied is process-lifetime state standing in for FRR-side state — EnsureFRRBFD now also requires frrBFDProfileConfigured(runningConfig) before it returns early, so a profile that an frr-reload dropped out from under its still-attached neighbors gets rewritten on the next reconcile. I kept the frrBFDProfileApplied flag: it is what applies a changed timer configuration at startup, and dropping it (as a pure existence check would) would have meant a retuned frr_bfd_min_rx never taking effect. Its doc comment is corrected — the profile's existence is readable; only its timers are not, because FRR omits values matching bfdd's defaults. (folded into 733e6e3)

• The new command timeouts do not fire when ovs_wrapper does not propagate SIGKILL — cmd.WaitDelay = commandWaitDelay (5 s) on both runOVS and runVtysh. exec.CommandContext kills only the direct child; CombinedOutput's Wait then blocks until every process holding the pipe's write end exits, which nsenter/sudo/ip netns exec wrappers do not guarantee. Regression tests assert the field is set on both paths. (folded into f4cd63e, which introduced both timeouts)

• TOCTOU between reading the BGP AS and entering the router bgp node — frrBGPInstanceAS() is gone; EnsureFRRBFD reads show running-config once and answers all three questions (profile present, instance AS, neighbors attached) from that snapshot. This narrows the window rather than closing it — router bgp <as> vrf <vrf> is still create-or-enter across two vtysh processes — and the function's doc comment now says so explicitly instead of implying the contract is enforced. (folded into 733e6e3)

Skipped

None.

Diff summary

• Files: bfd.go, bfd_test.go, ovs.go, ovs_test.go, routing.go, routing_test.go, metrics.go, metrics_test.go, docs/reference/metrics.md, docs/explanation/bfd-failover-detection.md
• Approx lines added/removed for this pass: +578/-249

One note on docs/reference/metrics.md: it is generated by make docs-gen from the prometheus.New* Help strings. My first hand-edit added alert bullets that tools/docgen/render.go hardcodes and would have wiped, which make docs-gen-check caught. I took the generator's output instead of touching render.go — the +Inf/NaN alerting guidance lives in the hand-written explanation page.

Verification

Run on the final tree, and separately on every commit in origin/main..HEAD:

go build ./...      OK
go vet ./...        OK
gofmt -l *.go       OK (empty)
go test ./...       ok github.com/osism/ovn-network-agent
go test -race .     ok  (2.6s)
make docs-gen-check OK

Each of the five new regression tests was confirmed to fail against the pre-fix behavior by reverting the production change and re-running: TestRunOVSBoundsWaitOnTheOutputPipe, TestRunVtyshBoundsWaitOnTheOutputPipe, TestEnsureOVNBFDTimers/every_drifted_tunnel_is_written_by_a_single_ovs-vsctl_invocation, TestEnsureFRRBFD/a_neighbor_with_BFD_but_no_profile_is_attached, TestEnsureFRRBFD/a_profile_FRR_lost_is_written_again, and TestReconcileBFDNoSessionReportsUnboundedDetection.

The --autosquash rebase conflicted on bfd.go/bfd_test.go (the check commit predates the two manage commits that later edited the same functions). I resolved each conflict to that commit's own content plus only its fix, which is why the per-commit build+test loop above matters — it confirms no later commit's code leaked backwards.

Status

STATUS: DONE

---

Review report generated by planwerk-agent ffc4daf implement with Claude

[berendt]

Review Report

Resolved

• FRR configuration and JSON are parsed out of a stderr-merged stream — runVtysh used CombinedOutput(), so vtysh's daemon diagnostics interleaved with the data three new readers parse as structured input. Root cause fixed in runVtysh: stdout is returned, stderr is captured separately and folded into the error on failure. I also hardened the two JSON sniffs — ListFRRBFDPeers and VRFBGPNeighbors now treat only a % notice or empty output as "nothing to report" and return an error on anything else, instead of silently reporting zero peers/neighbors. (folded into 09115d4 and 1a80664)

• EnsureFRRBFD has no convergence guard — needing diffed operational state (show bgp … neighbors json) against configuration state (instance.Profiled) with nothing guaranteeing the two can ever agree. Two changes: RouteManager.frrBFDAttached remembers each neighbor whose attach command FRR accepted, so a neighbor that never appears in the running configuration is logged and left alone rather than reconfigured every 60 s; and the attach is now one vtysh invocation per neighbor instead of one batch, because vtysh stops at the first command FRR rejects — a dynamic peer from bgp listen range would otherwise starve every neighbor sorted after it. The profile write is now its own invocation and is skipped once applied and still present, so a permanently-failing neighbor no longer drags it along. (folded into 1a80664)

• Command timeouts kill only the direct child — added killProcessGroup, used by both runOVS and runVtysh: the child gets its own process group and cancellation sends SIGKILL to the group, reaping nsenter/sudo/chroot/vtysh-daemon grandchildren. A group kill cannot reach the container the documented docker exec openvswitch_vswitchd wrapper starts, so runOVS also passes --timeout=25 to ovs-vsctl/ovs-ofctl, which bounds the command wherever it runs. I verified the group kill against real processes: a child leaving a backgrounded grandchild on the output pipe now returns at the deadline instead of waiting out WaitDelay. (folded into 09115d4)

• worstPeerDetectTime is blind to the BGP neighbor with no BFD session — reconcileFRRBFD now cross-checks the VRF's BGP neighbors against the sessions bfdd reports, via a new neighborsWithoutBFD. One neighbor without a session drives the gauge to +Inf, since the fabric withdraws this node's /32s over every session and the missing one bounds the failover. Unnumbered peers are matched on bfdd's interface field as well as peer, so an unnumbered fabric with healthy BFD is not falsely reported. (folded into 1a80664 — the fix needs VRFBGPNeighbors, which that commit introduces; the symptom's home, 09115d4, cannot reference it)

• frrBFDPeer.VRF's zero value is compared — added frrBFDPeer.inVRF, which honors the type's own contract that an unreported field means "unknown", and used it in both worstPeerDetectTime and neighborsWithoutBFD. (folded into 09115d4)

• OVS interface names reach ovs-vsctl argv unvalidated — setInterfaceBFDTimers now skips names that would re-partition the command line, and issues nothing when no name survives. Note the suggested isValidIdentifier(name) guard alone does not close this: it permits -, so -- passes it. The guard rejects a leading - explicitly. (folded into 20abe15)

Skipped

None — all six findings were real and applied to the current diff.

Diff summary

• Files: bfd.go, bfd_test.go, ovs.go, ovs_test.go, routing.go, routing_test.go, metrics.go, docs/reference/metrics.md, docs/explanation/bfd-failover-detection.md
• Approx lines added/removed: +544/-80 (relative to the pre-fix branch tip)

Notes for a human

• Regression guards: every production fix has a test that fails before it and passes after — I confirmed this empirically for the interface-name guard, the neighbor-without-session check, the unnumbered-interface match, and the attach-once guard by reverting each fix in isolation and watching the test go red.
• Test-recorder change: ovsRecorder.hook() strips the injected --timeout= flag before matching fixtures, so the 86 existing OVS argv fixtures stayed untouched. The flag itself is covered by a dedicated TestRunOVSBoundsTheOVSCommandItself, so it is asserted rather than hidden.
• Doc sync: the +Inf semantics changed (it now also fires when one BGP neighbor lacks a session), so I updated the gauge's Help string, regenerated docs/reference/metrics.md via go run ./tools/docgen, and adjusted the explanation page. make docs-gen-check is clean.
• Out of scope, worth knowing: runOVS still uses CombinedOutput(), and parseGeneveTunnels parses that merged stream as JSON — the same class of defect as finding 1, but on ovs-vsctl, which no finding raised. It fails loudly (a parse error, bfd_check_errors_total, NaN) rather than silently, and changing runOVS's output semantics would touch many pre-existing callers, so I left it.

Verification

• go build ./..., go vet ./..., gofmt -l — all clean
• go test -count=1 ./... — pass (ovn-network-agent and tools/docgen)
• go run ./tools/docgen && git diff --exit-code -- docs/reference/ — clean
• Each of the six rewritten commits builds and tests green independently (checked in a temporary worktree)
• Integration tests under test/integration were not run; they need a live OVN/OVS environment this machine does not have.

Nothing was pushed and no PR was opened — the six commits sit rewritten on the local branch.

Status

STATUS: DONE

---

Review report generated by planwerk-agent ffc4daf implement with Claude