watchfrr,tools: add --collect-core to core dump unresponsive daemon

[nishant111]

When a daemon does not respond to the VTY ping, and watchfrr_options="--collect-core" is configured, watchfrr is going to send SIGABRT to the unresponsive daemon to dump the core.
Aim is to improve debugability of such stuck daemons.

Enable with watchfrr_options="--collect-core" in daemons.

[greptile-apps]

Greptile Summary

This PR adds an optional --collect-core mode to watchfrr that, when a daemon fails to stop gracefully, sends SIGABRT (which FRR handles to produce a core dump) before falling back to SIGKILL. It also replaces the hardcoded 120 s SIGINT wait in frrcommon.sh with a dynamic budget derived from WATCHFRR_RESTART_TIMEOUT, and wires up two new env-vars (FRR_COLLECT_CORE, WATCHFRR_RESTART_TIMEOUT) so the shell script can size each phase to fit within watchfrr's kill timer.

Key changes:

• watchfrr/watchfrr.c: New -c/--collect-core flag sets gs.collect_core; with no explicit -T, gs.restart_timeout is doubled to DEFAULT_RESTART_TIMEOUT_WITH_CORE (180 s); FRR_COLLECT_CORE and WATCHFRR_RESTART_TIMEOUT are exported into every run_job child environment.
• tools/frrcommon.sh.in: daemon_stop() computes per-phase timing from WATCHFRR_RESTART_TIMEOUT; adds SIGABRT phase (guarded by FRR_COLLECT_CORE=1) and a SIGKILL last resort with a 1 s post-kill drain.
• A timeout_explicitly_set boolean correctly guards the timeout doubling so an explicit -T 90 is not silently doubled.

One notable unresolved side effect: because PHASE_TIMEOUT is defined as 3 * gs.restart_timeout, doubling gs.restart_timeout for core collection also doubles the phase-hanging watchdog from 270 s to 540 s, affecting phased restart phases (e.g. waiting for zebra to come up) that are unrelated to core dumping.

Confidence Score: 3/5

• Functional but has an undocumented side effect on the phase-hanging watchdog that should be addressed before merging.
• The core feature logic is sound and the previously flagged issues (env-var name mismatch, cnt value, timeout_explicitly_set, unconditional setenv) appear resolved. However, doubling gs.restart_timeout unintentionally doubles PHASE_TIMEOUT (270 s → 540 s), silently extending the unrelated phase-hanging watchdog timer. This could mask zebra startup failures for up to 9 minutes when --collect-core is active, which is a meaningful operational regression.
• watchfrr/watchfrr.c — specifically the PHASE_TIMEOUT macro and its dependence on gs.restart_timeout.

Important Files Changed

Filename	Overview
watchfrr/watchfrr.c	Adds --collect-core flag, FRR_COLLECT_CORE/WATCHFRR_RESTART_TIMEOUT env-var exports, and the timeout_explicitly_set guard. The previously-flagged issues (imprecise -T detection, unconditional env export) are addressed. A new side effect exists: doubling gs.restart_timeout also doubles PHASE_TIMEOUT from 270 s to 540 s, extending the unrelated phase-hanging watchdog.
tools/frrcommon.sh.in	Replaces the hardcoded 120 s SIGINT wait with a dynamic calculation driven by WATCHFRR_RESTART_TIMEOUT. Adds optional SIGABRT phase (when FRR_COLLECT_CORE=1) and an unconditional SIGKILL last-resort with a 1 s drain loop. Previously flagged env-var name mismatch and cnt value concerns appear addressed in the current code.

Sequence Diagram

sequenceDiagram
    participant WF as watchfrr
    participant SH as frrcommon.sh (daemon_stop)
    participant D as FRR daemon

    WF->>WF: detect unresponsive daemon
    WF->>WF: setenv FRR_COLLECT_CORE, WATCHFRR_RESTART_TIMEOUT
    WF->>SH: run stop script (restart_kill timer = restart_timeout)

    SH->>SH: compute phase_sec = (restart_timeout - 2) / 2  [if collect-core]
    SH->>D: SIGINT
    SH->>SH: wait up to phase_sec seconds

    alt daemon still alive AND FRR_COLLECT_CORE=1
        SH->>D: SIGABRT (triggers core dump)
        SH->>SH: wait up to phase_sec seconds
    end

    alt daemon still alive
        SH->>D: SIGKILL
        SH->>SH: wait up to 1 second
    end

    SH-->>WF: script exits (within restart_timeout window)

Loading

Comments Outside Diff (1)

1. watchfrr/watchfrr.c, line 88 (link)

   --collect-core unintentionally doubles the phase-hanging watchdog

   PHASE_TIMEOUT is defined as 3 * gs.restart_timeout. When --collect-core doubles gs.restart_timeout from 90 → 180, PHASE_TIMEOUT also doubles from 270 s → 540 s. This macro is used in set_phase() (line 909) to arm the phase_hanging watchdog timer — the safeguard that aborts a stuck phased restart.

   Phases like "Waiting for zebra to come up" or "Waiting for other daemons to come down" are unrelated to core collection; they rely on a healthy daemon startup, not on a deliberate signal-wait window. Silently stretching this watchdog to 9 minutes means that if zebra fails to start for an unrelated reason while --collect-core is active, watchfrr will wait twice as long before declaring the phase hung and recovering.

   Consider using a separate constant or keeping a copy of the original restart_timeout for the phase calculation so PHASE_TIMEOUT stays at its intended 3 × 90 = 270 s:

   /* Keep using the default restart timeout for PHASE_TIMEOUT so that
    * --collect-core doesn't accidentally extend the phase-hanging watchdog.
    */
   #define PHASE_TIMEOUT (3 * DEFAULT_RESTART_TIMEOUT)

   Alternatively, introduce a dedicated gs.phase_timeout field set once at initialisation before the collect_core doubling is applied.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: watchfrr/watchfrr.c
   Line: 88
   
   Comment:
   **`--collect-core` unintentionally doubles the phase-hanging watchdog**
   
   `PHASE_TIMEOUT` is defined as `3 * gs.restart_timeout`. When `--collect-core` doubles `gs.restart_timeout` from 90 → 180, `PHASE_TIMEOUT` also doubles from 270 s → 540 s. This macro is used in `set_phase()` (line 909) to arm the `phase_hanging` watchdog timer — the safeguard that aborts a stuck phased restart.
   
   Phases like *"Waiting for zebra to come up"* or *"Waiting for other daemons to come down"* are unrelated to core collection; they rely on a healthy daemon startup, not on a deliberate signal-wait window. Silently stretching this watchdog to 9 minutes means that if zebra fails to start for an unrelated reason while `--collect-core` is active, watchfrr will wait twice as long before declaring the phase hung and recovering.
   
   Consider using a separate constant or keeping a copy of the original `restart_timeout` for the phase calculation so `PHASE_TIMEOUT` stays at its intended `3 × 90 = 270 s`:
   
   ```c
   /* Keep using the default restart timeout for PHASE_TIMEOUT so that
    * --collect-core doesn't accidentally extend the phase-hanging watchdog.
    */
   #define PHASE_TIMEOUT (3 * DEFAULT_RESTART_TIMEOUT)
   ```
   
   Alternatively, introduce a dedicated `gs.phase_timeout` field set once at initialisation before the `collect_core` doubling is applied.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: watchfrr/watchfrr.c
Line: 88

Comment:
**`--collect-core` unintentionally doubles the phase-hanging watchdog**

`PHASE_TIMEOUT` is defined as `3 * gs.restart_timeout`. When `--collect-core` doubles `gs.restart_timeout` from 90 → 180, `PHASE_TIMEOUT` also doubles from 270 s → 540 s. This macro is used in `set_phase()` (line 909) to arm the `phase_hanging` watchdog timer — the safeguard that aborts a stuck phased restart.

Phases like *"Waiting for zebra to come up"* or *"Waiting for other daemons to come down"* are unrelated to core collection; they rely on a healthy daemon startup, not on a deliberate signal-wait window. Silently stretching this watchdog to 9 minutes means that if zebra fails to start for an unrelated reason while `--collect-core` is active, watchfrr will wait twice as long before declaring the phase hung and recovering.

Consider using a separate constant or keeping a copy of the original `restart_timeout` for the phase calculation so `PHASE_TIMEOUT` stays at its intended `3 × 90 = 270 s`:

```c
/* Keep using the default restart timeout for PHASE_TIMEOUT so that
 * --collect-core doesn't accidentally extend the phase-hanging watchdog.
 */
#define PHASE_TIMEOUT (3 * DEFAULT_RESTART_TIMEOUT)
```

Alternatively, introduce a dedicated `gs.phase_timeout` field set once at initialisation before the `collect_core` doubling is applied.

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

_{Last reviewed commit: cfad111}

[nishant111]

@greptile review

[nishant111]

@greptile review

[nishant111]

ci:rerun

[nishant111]

ci:rerun

[Jafaral]

@greptile review

[choppsv1]

This feels a little over-engineered, will come back with suggested simplification.

[choppsv1]

I think the --collect-core to watchfrr is a good idea for triaging issues.

But this change seems overly complex, it creates and environment variable that messes with the timer restart values in some non-simple ways, it still SIGINTs the PID, and only when that times out as well (i.e., in addition to missing a heartbeats) it then does a SIGABRT to collect the core when the env variable is set.

This seems too complex.

I think we should just add a direct core function for frrcommon.sh. If watchfrr misses the heartbeat it would just invoke core for that PID and collect the core for whatever the PID is doing to miss its heartbeat.

This is also debugging the direct issue.

The current method in the PR is actually debugging why SIGINT didn't work rather than what it's doing to miss the hearbeat, so this could be getting a core for a secondary problem rather than the primary one.

[nishant111]

@choppsv1 Thank you for the review, I have updated the PR to be more specific and simple as per your suggestion.

I still see some scope of improvement in frrcommon.sh.in which I feel I can address in a seperate PR to keep this PR specific and simple. for example :

1. if a daemon is stuck and "--collect-core" is not present then watchfrr is going to send a SIGINT which will never be caught as signal are handled in event loop context. watchfrr will keep retrying but will never be able to kill it.
2. The current timeout of 120 seconds in frrcommon.sh.in is not correct , it should be based on the timeout configured in watchfrr.c under this command option "--restart-timeout".