fix(ntp): make the keepalived health check verify the clock, not liveness #467

2026-08-05T17:28:45Z

claude-bot commented

2026-08-05 17:28:45 +00:00

Follow-up to #465 / #466.

Scope caveat up front: this hardens the keepalived path, which is legacy. It is worth landing because that path is still live today (the PVE hosts still exist, and Shellys are pointed at the VIP via plays/configure-shelly-ntp.yml), but it is explicitly not the answer to "a dead battery must never bring down internal NTP". See the last section.

The bug

The health check asked "is chronyd alive", never "is this host safe to take time from". On 2026-08-04 those came apart.

edge-1's CMOS battery is dead. After a power loss its clock came up ~16 days in the past and, with no makestep, chronyd never corrected it. All three checks passed continuously for ~18h:

chronyd was active
chronyc tracking responded
udp/123 was listening

and local stratum 10 meant chronyd advertised itself to clients as a valid reference the entire time. keepalived would have happily kept the VIP on a host ready to serve a clock from July.

The sharpest part — check 2 was:

if ! chronyc tracking >/dev/null 2>&1; then

The one command that knew the answer was executed and its answer discarded. During the incident that output read:

System time     : 1376235.375000000 seconds slow of NTP time

The fix

Captures the output and adds two assertions:

Offset above keepalived_ntp_max_offset (new, default 1.0s) fails, so a host that knows its clock is wrong drops the VIP.
No upstream source selected in chronyc sources fails. This matters because upstream chrony docs are explicit that with local stratum N, the local reference activates immediately even if chronyd has never synchronised to anything — so a never-validated host still looks authoritative to clients.

Verification

Both predicates were validated against live output rather than assumed:

offset comparison against the real incident value (1376235.375 vs 1.0 → fail) and against a healthy value (0.000000000 → pass)
the chronyc sources selected-source column against a real run on edge-1 (^* ptbtime1.ptb.de), confirming the marker is in column 2

Failure behaviour during a WAN outage

Failing costs the configured weight (-20). When the WAN is down every host fails together and loses the same weight, so relative priority is unchanged and the VIP stays put — the local-reference fallback still works. What this prevents is one host with a broken clock winning while the others are healthy.

What this does NOT fix

plays/install-ntp-ha.yml documents the end state:

as the PVE hosts are retired for bare-metal Talos, edge-1 is intended to become the sole NTP server

In that end state this PR is nearly useless, and arguably harmful: with one candidate there is nothing to fail over to, so a failing check converts "serves the wrong time" into "serves no time". Internal NTP would then rest entirely on a Dell Wyse 3040 thin client with a known-dead RTC battery and no redundancy.

That needs an architectural answer, not a better health check. Raised separately.

🤖 Generated with Claude Code

Follow-up to #465 / #466. > **Scope caveat up front:** this hardens the **keepalived path, which is legacy.** It is worth landing because that path is still live today (the PVE hosts still exist, and Shellys are pointed at the VIP via `plays/configure-shelly-ntp.yml`), but it is explicitly **not** the answer to "a dead battery must never bring down internal NTP". See the last section. ## The bug The health check asked *"is chronyd alive"*, never *"is this host safe to take time from"*. On 2026-08-04 those came apart. edge-1's CMOS battery is dead. After a power loss its clock came up ~16 days in the past and, with no `makestep`, chronyd never corrected it. All three checks passed continuously for ~18h: - chronyd was active - `chronyc tracking` responded - udp/123 was listening and `local stratum 10` meant chronyd advertised itself to clients as a valid reference the entire time. keepalived would have happily kept the VIP on a host ready to serve a clock from July. The sharpest part — check 2 was: ```bash if ! chronyc tracking >/dev/null 2>&1; then ``` The one command that knew the answer was executed and its answer discarded. During the incident that output read: ``` System time : 1376235.375000000 seconds slow of NTP time ``` ## The fix Captures the output and adds two assertions: 1. **Offset above `keepalived_ntp_max_offset`** (new, default `1.0`s) fails, so a host that *knows* its clock is wrong drops the VIP. 2. **No upstream source selected** in `chronyc sources` fails. This matters because [upstream chrony docs](https://chrony-project.org/doc/4.5/chrony.conf.html) are explicit that with `local stratum N`, the local reference activates **immediately even if chronyd has never synchronised to anything** — so a never-validated host still looks authoritative to clients. ## Verification Both predicates were validated against live output rather than assumed: - offset comparison against the real incident value (`1376235.375` vs `1.0` → fail) and against a healthy value (`0.000000000` → pass) - the `chronyc sources` selected-source column against a real run on edge-1 (`^* ptbtime1.ptb.de`), confirming the marker is in column 2 ## Failure behaviour during a WAN outage Failing costs the configured weight (`-20`). When the WAN is down **every host fails together** and loses the same weight, so relative priority is unchanged and the VIP stays put — the local-reference fallback still works. What this prevents is one host with a broken clock winning while the others are healthy. ## What this does NOT fix `plays/install-ntp-ha.yml` documents the end state: > as the PVE hosts are retired for bare-metal Talos, edge-1 is intended to become the **sole NTP server** In that end state this PR is nearly useless, and arguably harmful: with one candidate there is nothing to fail over to, so a failing check converts *"serves the wrong time"* into *"serves no time"*. Internal NTP would then rest entirely on a Dell Wyse 3040 thin client with a known-dead RTC battery and no redundancy. That needs an architectural answer, not a better health check. Raised separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-08-05 17:28:45 +00:00

fix(ntp): make the keepalived health check verify the clock, not liveness

ci/woodpecker/pr/pr-review Pipeline was successful

Details

ci/woodpecker/pr/ci-scripts-test Pipeline was successful

Details

601290dcd9

The NTP health check asked "is chronyd alive", never "is this host safe to
take time from". Those came apart on 2026-08-04.

edge-1's CMOS battery is dead. After a power loss its clock came up ~16
days in the past and, with no makestep directive, chronyd never corrected
it. All three health checks passed continuously for ~18h:

  - chronyd was active
  - `chronyc tracking` responded
  - udp/123 was listening

and `local stratum 10` meant chronyd advertised itself to clients as a
valid reference the whole time. keepalived would have kept the VIP on a
host ready to serve a clock from July.

The sharpest part: check 2 ran `chronyc tracking >/dev/null 2>&1` and
discarded the output. The one command that knew the answer was executed
and its answer thrown away. During the incident that output read:

  System time     : 1376235.375000000 seconds slow of NTP time

This captures the output and adds two assertions:

  - offset above keepalived_ntp_max_offset (default 1.0s) fails, so a host
    that knows its clock is wrong drops the VIP
  - no upstream source selected in `chronyc sources` fails, because with
    `local stratum N` chronyd advertises itself as a usable reference even
    when it has never synchronised to anything -- upstream chrony docs are
    explicit that the local reference activates immediately in that case

Both predicates were validated against live output from edge-1 rather than
assumed: the offset comparison against the real incident value, and the
`chronyc sources` selected-source column against a real run.

Failing the check costs the configured weight (-20). When the WAN is down
every host fails together and loses the same weight, so relative priority
is unchanged and the VIP stays put -- the local-reference fallback still
works. What this prevents is one host with a broken clock winning.

Scope: this hardens the keepalived path, which is legacy. It does not
address the documented end state in plays/install-ntp-ha.yml, where edge-1
becomes the SOLE NTP server -- there, a failing check means no host serves
time at all, converting "serves the wrong time" into "serves no time".
That needs an architectural answer, not a better health check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

review-bot reviewed

2026-08-05 17:34:02 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 601290dcd90ff6cccde800ed69a882620a294b2f.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

Replaces the keepalived NTP health check's liveness-only probing (service active / chronyc tracking exit code discarded / port 123 listening) with two new assertions that actually inspect chronyd's output: the reported system-clock offset against a configurable ceiling (keepalived_ntp_max_offset, default 1.0s), and whether chronyc sources shows a selected upstream source. I traced the field/column parsing (awk '{print $4}' on the System time line, and the ^.\*|^.\+ anchor on chronyc sources) against the real chronyc output format and the incident data quoted in the PR body, and both are correct. The surrounding role/playbook context (chrony.conf.j2's local stratum 10, install-ntp-ha.yml's migration note, keepalived.conf.j2's fall 2/interval 2) all corroborate the claims made in the PR description. No blocking issues found.

🟡 Suggestions

ansible/roles/keepalived-ntp/templates/check-ntp-health.sh.j2:60-65 — Check 4 silently no-ops if OFFSET fails to parse (empty string, e.g. from an unexpected chronyc output format change). Contrast with Check 5, which fails hard when its own data is ambiguous ("no upstream yet" is explicitly treated as unsafe). For consistency with that same-PR philosophy, consider also failing when OFFSET can't be extracted, rather than treating unparseable output as "offset is fine." Low risk today since the format was validated against a live host, but it's a silent gap if chronyc's output ever changes on an OS upgrade.

Tests

No automated tests exist for this change, and none are added. That's consistent with the rest of the repo — .woodpecker/ci-manifests-test.yml's shellcheck step is explicitly scoped to kubernetes/** only (see its own comment noting the gap for "shell embedded in manifests"/other trees), and there's no ansible-lint/molecule pipeline covering ansible/roles/** at all. So this isn't a regression in this PR's testing posture, just a pre-existing gap for the whole ansible tree. The PR body does describe manual verification of both new predicates against real chronyc output (the incident's actual offset value, a healthy 0.0 value, and a live chronyc sources run on edge-1), which is reasonable given no CI harness exists to automate it.

Recommendation

Looks reasonable — no blocking issues; comment above is optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `601290dcd90ff6cccde800ed69a882620a294b2f`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Replaces the keepalived NTP health check's liveness-only probing (service active / `chronyc tracking` exit code discarded / port 123 listening) with two new assertions that actually inspect chronyd's output: the reported system-clock offset against a configurable ceiling (`keepalived_ntp_max_offset`, default 1.0s), and whether `chronyc sources` shows a selected upstream source. I traced the field/column parsing (`awk '{print $4}'` on the `System time` line, and the `^.\*|^.\+` anchor on `chronyc sources`) against the real `chronyc` output format and the incident data quoted in the PR body, and both are correct. The surrounding role/playbook context (`chrony.conf.j2`'s `local stratum 10`, `install-ntp-ha.yml`'s migration note, `keepalived.conf.j2`'s `fall 2`/`interval 2`) all corroborate the claims made in the PR description. No blocking issues found. ## 🟡 Suggestions - `ansible/roles/keepalived-ntp/templates/check-ntp-health.sh.j2:60-65` — Check 4 silently no-ops if `OFFSET` fails to parse (empty string, e.g. from an unexpected `chronyc` output format change). Contrast with Check 5, which fails hard when its own data is ambiguous ("no upstream yet" is explicitly treated as unsafe). For consistency with that same-PR philosophy, consider also failing when `OFFSET` can't be extracted, rather than treating unparseable output as "offset is fine." Low risk today since the format was validated against a live host, but it's a silent gap if `chronyc`'s output ever changes on an OS upgrade. ## Tests No automated tests exist for this change, and none are added. That's consistent with the rest of the repo — `.woodpecker/ci-manifests-test.yml`'s `shellcheck` step is explicitly scoped to `kubernetes/**` only (see its own comment noting the gap for "shell embedded in manifests"/other trees), and there's no `ansible-lint`/molecule pipeline covering `ansible/roles/**` at all. So this isn't a regression in this PR's testing posture, just a pre-existing gap for the whole ansible tree. The PR body does describe manual verification of both new predicates against real `chronyc` output (the incident's actual offset value, a healthy 0.0 value, and a live `chronyc sources` run on edge-1), which is reasonable given no CI harness exists to automate it. ## Recommendation Looks reasonable — no blocking issues; comment above is optional.

claude-bot referenced this pull request

2026-08-05 17:47:31 +00:00

fix(edge-host): track the NTP anycast address that only existed on the host #468

k.renner merged commit ea54e558fd into master

2026-08-05 17:49:36 +00:00

k.renner referenced this issue from a commit

2026-08-05 17:49:37 +00:00

Merge pull request 'fix(ntp): make the keepalived health check verify the clock, not liveness' (#467) from ntp-health-clock-correctness into master

claude-bot referenced this pull request

2026-08-05 18:00:11 +00:00

feat(ntp): hold the IPv6 service address in keepalived, gated on clock health #469

claude-bot referenced this pull request