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

2026-08-05T18:00:11Z

claude-bot commented

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

Supersedes #468 — that captured the drift into the edge-host role, which is the wrong home. This puts the service address in the NTP role and adds the health gating. #468 should be closed.

The gap

ntp.srv.renner.uno resolves to fd40::123 (IPv6) and the keepalived VIP (IPv4, legacy). IPv6 is primary here, so the v6 address is the path clients actually use — and it had no health gating at all.

It was assigned statically in /etc/systemd/network/ on edge-1, so networkd carried it unconditionally from boot. That is how the host answered NTP queries on 2026-08-04 with a clock ~16 days in the past: chronyd running, udp/123 listening, address up, and nothing checking whether the clock was right. Fixing the keepalived health check (#467) does not help, because keepalived was not holding that address.

The fix

A second VRRP instance tracking the same check_ntp script, so the address exists only while the host is fit to serve time.

Three details that are load-bearing rather than stylistic:

Separate vrrp_instance is mandatory. keepalived requires that "all addresses in virtual_ipaddress must be of the same family", so the v6 address cannot join NTP_VIP.
No authentication block. VRRPv3 removed authentication (RFC 5798), and keepalived always uses version 3 for IPv6 instances.
track_script { check_ntp weight 0 }, overriding the script's -20. This decides whether the feature works at all: a non-zero weight only subtracts from priority on failure, so the last remaining host — with nobody to lose the election to — stays MASTER and keeps serving a clock it has already reported as wrong. Weight 0 transitions to FAULT instead, releasing the address unconditionally. Per the docs: "The default weight equals 0, which means that any VRRP instance monitoring the script will transition to the fault state after <fall> consecutive failures."

Applies to the whole cluster, not just edge-1

ntp_vip6 is defined in plays/install-ntp-ha.yml alongside ntp_vip and passed to the role for every member of ntp_cluster. Setting it in one host's group_vars — as the first revision of this PR did — would have left the other servers outside the election entirely, reducing the health gating to "edge-1 withdraws and nobody takes over".

Confirmed the other members are live participants, not retired hosts: edge-1 is currently BACKUP and does not hold the IPv4 VIP, and a VRRP BACKUP receiving no advertisements would have promoted itself to MASTER.

Model change

This makes the address a single-master VRRP VIP rather than one held by every server simultaneously. Chosen because it gives health-gated withdrawal using tooling already deployed here; the previous arrangement had no mechanism to stop an unhealthy host answering.

Scope

The edge-host role is deliberately untouched. Its template already has no [Address] block, so applying it removes the hand-placed address, and keepalived then adds it back under health control. The service address is NTP-role configuration and now lives there.

Verification

Validated with keepalived -t against keepalived 2.4.1 on edge-1, using the config exactly as it renders for that host. Parses clean apart from the pre-existing auth_pass truncation warning on the IPv4 instance. Temp file removed afterwards.

Before merging — needs checking on the other servers

Whether jupiter/saturn/neptune also carry fd40::123 statically, as edge-1 did. If they do, that static address must be removed as this lands, or two hosts hold it outside VRRP control and the election is meaningless.

I could not check: those hosts have no DNS records — they resolve only through the *.renner.uno wildcard to the proxy — so there is no hostname path to reach them.

Supersedes #468 — that captured the drift into the edge-host role, which is the wrong home. This puts the service address in the NTP role and adds the health gating. **#468 should be closed.** ## The gap `ntp.srv.renner.uno` resolves to `fd40::123` (IPv6) and the keepalived VIP (IPv4, legacy). IPv6 is primary here, so the v6 address is the path clients actually use — and it had **no health gating at all**. It was assigned statically in `/etc/systemd/network/` on edge-1, so networkd carried it unconditionally from boot. That is how the host answered NTP queries on 2026-08-04 with a clock ~16 days in the past: chronyd running, udp/123 listening, address up, and nothing checking whether the clock was *right*. Fixing the keepalived health check (#467) does not help, because keepalived was not holding that address. ## The fix A second VRRP instance tracking the same `check_ntp` script, so the address exists only while the host is fit to serve time. Three details that are load-bearing rather than stylistic: - **Separate `vrrp_instance` is mandatory.** keepalived requires that *"all addresses in virtual_ipaddress must be of the same family"*, so the v6 address cannot join `NTP_VIP`. - **No `authentication` block.** VRRPv3 removed authentication (RFC 5798), and keepalived always uses version 3 for IPv6 instances. - **`track_script { check_ntp weight 0 }`**, overriding the script's `-20`. This decides whether the feature works at all: a non-zero weight only *subtracts from priority* on failure, so the last remaining host — with nobody to lose the election to — stays MASTER and keeps serving a clock it has already reported as wrong. Weight 0 transitions to FAULT instead, releasing the address unconditionally. Per the docs: *"The default weight equals 0, which means that any VRRP instance monitoring the script will transition to the fault state after <fall> consecutive failures."* ## Applies to the whole cluster, not just edge-1 `ntp_vip6` is defined in `plays/install-ntp-ha.yml` alongside `ntp_vip` and passed to the role for every member of `ntp_cluster`. Setting it in one host's group_vars — as the first revision of this PR did — would have left the other servers outside the election entirely, reducing the health gating to "edge-1 withdraws and nobody takes over". Confirmed the other members are live participants, not retired hosts: edge-1 is currently BACKUP and does not hold the IPv4 VIP, and a VRRP BACKUP receiving no advertisements would have promoted itself to MASTER. ## Model change This makes the address a single-master VRRP VIP rather than one held by every server simultaneously. Chosen because it gives health-gated withdrawal using tooling already deployed here; the previous arrangement had no mechanism to stop an unhealthy host answering. ## Scope The **edge-host role is deliberately untouched.** Its template already has no `[Address]` block, so applying it removes the hand-placed address, and keepalived then adds it back under health control. The service address is NTP-role configuration and now lives there. ## Verification Validated with `keepalived -t` against **keepalived 2.4.1 on edge-1**, using the config exactly as it renders for that host. Parses clean apart from the pre-existing `auth_pass` truncation warning on the IPv4 instance. Temp file removed afterwards. ## Before merging — needs checking on the other servers Whether jupiter/saturn/neptune also carry `fd40::123` statically, as edge-1 did. **If they do, that static address must be removed as this lands**, or two hosts hold it outside VRRP control and the election is meaningless. I could not check: those hosts have no DNS records — they resolve only through the `*.renner.uno` wildcard to the proxy — so there is no hostname path to reach them. ## Related - #467 makes `check_ntp` actually verify the clock rather than liveness. **Without it this PR gates on a predicate that passed throughout the incident** — the two belong together. - The IPv4 instance still uses `weight -20` and so has the same last-host-standing behaviour. Left alone as legacy. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-08-05 18:00:12 +00:00

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

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

Details

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

Details

2fbce12b0b

ntp.srv.renner.uno resolves to fd40::123 (IPv6) and to the keepalived VIP
(IPv4, legacy). IPv6 is the primary family here, so the v6 address is the
path clients actually use -- and it was the one with no health gating at
all.

It was assigned statically in /etc/systemd/network/ on edge-1, so networkd
carried it unconditionally from boot. That is how the host answered NTP
queries on 2026-08-04 while its clock was ~16 days in the past: chronyd was
running, udp/123 was listening, the address was up, and nothing anywhere
was checking whether the clock was right. Fixing the keepalived health
check does not help, because keepalived was not holding that address.

This moves it into keepalived as a second VRRP instance tracking the same
check_ntp script, so the address exists only while the host is fit to serve
time.

Three details that are load-bearing rather than stylistic:

  - A separate vrrp_instance is mandatory. keepalived requires that "all
    addresses in virtual_ipaddress must be of the same family", so the v6
    address cannot join NTP_VIP.

  - No authentication block. VRRPv3 removed authentication (RFC 5798) and
    keepalived always uses version 3 for IPv6 instances.

  - track_script uses `weight 0`, overriding the script's -20. A non-zero
    weight only subtracts from priority on failure, so the last remaining
    host -- having nobody to lose the election to -- would stay MASTER and
    keep serving a clock it had already reported as wrong. Weight 0
    transitions the instance to FAULT instead, releasing the address
    unconditionally. That is the required behaviour: a host unfit to serve
    time must stop answering whether or not a replacement exists.

Semantic change, deliberate and worth stating: VRRP is a single-master
unicast VIP, not anycast. Previously every NTP server carried fd40::123 and
clients reached whichever answered neighbour discovery first. Now exactly
one host holds it. For NTP this is the better trade -- query volume makes
load spreading pointless, and on failover keepalived sends unsolicited
Neighbour Advertisements so client neighbour caches update in seconds,
where the static address had no withdrawal signal at all and clients kept
using a dead holder until their own neighbour unreachability detection
expired. True routed anycast with health withdrawal would mean announcing
the /128 via an IGP, which the gateway would have to speak.

The edge-host role is deliberately untouched: its template already has no
[Address] block, so applying it removes the hand-placed address, and
keepalived then adds it back under health control. The service address is
NTP-role configuration and now lives there.

Validated with `keepalived -t` against keepalived 2.4.1 on edge-1 using the
config as it renders for that host; parses clean apart from the pre-existing
auth_pass truncation warning on the IPv4 instance.

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

review-bot reviewed

2026-08-05 18:04:58 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 2fbce12b0b0b551d82e2196d466688a28aaa3bf2.

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

Summary

This PR adds a second, IPv6 vrrp_instance (NTP_VIP6) to the keepalived-ntp role so that fd40::123 (what ntp.srv.renner.uno resolves to) is held by keepalived and gated on check_ntp, instead of being assigned unconditionally by networkd on edge-1. The wiring is careful and well-documented: a separate vrrp_instance (required, since v4/v6 can't share virtual_ipaddress), no authentication block (correct per VRRPv3), -nodad on the address (documented against a real DAD-race failure mode), and track_script { check_ntp weight 0 } so a failing host actually drops to FAULT and releases the address rather than just losing priority. I traced the variable wiring end-to-end (ansible/plays/install-ntp-ha.yml → edge_host vs pve_cluster group_vars → role defaults) and confirmed keepalived_ntp_vip6 is only non-empty for edge-1, so the new block only renders there — consistent with the PR's own description that this address historically only ever lived on edge-1. No blocking issues found.

🟡 Suggestions

This PR does not by itself fix the incident it describes. check-ntp-health.sh.j2 (untouched by this diff) only checks that chrony is active, chronyc tracking responds, and port 123 is listening — it never inspects offset or chronyc tracking's "System time"/leap status. A clock 16 days stale, as described in the incident, would pass all three checks today, so NTP_VIP6 would go MASTER and stay there even on a host with a wrong clock. The PR body already flags this ("#467 makes check_ntp actually verify the clock... without it this PR gates on a predicate that passed throughout the incident") and asks for the two to land together — I'd treat that as a hard prerequisite for closing out the incident, not just a nice-to-have, since without #467 this PR only fixes the mechanism (address withdrawal) and not the signal that would have caught the actual failure.
Static address removal is implied, not enacted, by this PR. The PR body states the old fd40::123 assignment lived hand-placed in /etc/systemd/network/ "out of git," and that re-applying the (already addressless) edge-host role template will strip it. That's plausible — ansible/roles/edge-host/tasks/main.yml:190-191 templates to the same 10-{{ edge_vlan40_name }}.network path — but it depends on the live file on edge-1 being drift from that exact template (rather than a separate file/mechanism), and on the edge-host role actually being re-applied to edge-1 as part of rolling this out. If the static config was placed some other way (e.g. a separate .network drop-in, or networkctl/manual ip addr add), it would keep answering NTP queries regardless of keepalived's FAULT state, silently defeating the health gating this PR adds. Worth confirming during rollout (e.g. ip -6 addr show on edge-1 before/after applying) rather than assuming the template diff alone resolves it.

Tests

No automated tests exist for this role (no molecule setup in the repo, consistent with the rest of ansible/roles/), and none are added — consistent with repo conventions for infra-as-code. The PR body documents manual verification via keepalived -t against keepalived 2.4.1 on edge-1 using the rendered config, which is the appropriate check for a templated config file like this and is the best available substitute for automated coverage here.

Recommendation

Looks reasonable — no blocking issues; comments above are optional (though the #467 dependency is worth treating as a strong prerequisite before this closes out the incident it references).

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `2fbce12b0b0b551d82e2196d466688a28aaa3bf2`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR adds a second, IPv6 `vrrp_instance` (`NTP_VIP6`) to the `keepalived-ntp` role so that `fd40::123` (what `ntp.srv.renner.uno` resolves to) is held by keepalived and gated on `check_ntp`, instead of being assigned unconditionally by networkd on edge-1. The wiring is careful and well-documented: a separate `vrrp_instance` (required, since v4/v6 can't share `virtual_ipaddress`), no `authentication` block (correct per VRRPv3), `-nodad` on the address (documented against a real DAD-race failure mode), and `track_script { check_ntp weight 0 }` so a failing host actually drops to FAULT and releases the address rather than just losing priority. I traced the variable wiring end-to-end (`ansible/plays/install-ntp-ha.yml` → `edge_host` vs `pve_cluster` group_vars → role defaults) and confirmed `keepalived_ntp_vip6` is only non-empty for edge-1, so the new block only renders there — consistent with the PR's own description that this address historically only ever lived on edge-1. No blocking issues found. ## 🟡 Suggestions - **This PR does not by itself fix the incident it describes.** `check-ntp-health.sh.j2` (untouched by this diff) only checks that chrony is active, `chronyc tracking` responds, and port 123 is listening — it never inspects offset or `chronyc tracking`'s "System time"/leap status. A clock 16 days stale, as described in the incident, would pass all three checks today, so `NTP_VIP6` would go MASTER and stay there even on a host with a wrong clock. The PR body already flags this ("#467 makes `check_ntp` actually verify the clock... without it this PR gates on a predicate that passed throughout the incident") and asks for the two to land together — I'd treat that as a hard prerequisite for closing out the incident, not just a nice-to-have, since without #467 this PR only fixes the *mechanism* (address withdrawal) and not the *signal* that would have caught the actual failure. - **Static address removal is implied, not enacted, by this PR.** The PR body states the old `fd40::123` assignment lived hand-placed in `/etc/systemd/network/` "out of git," and that re-applying the (already addressless) `edge-host` role template will strip it. That's plausible — `ansible/roles/edge-host/tasks/main.yml:190-191` templates to the same `10-{{ edge_vlan40_name }}.network` path — but it depends on the *live file on edge-1* being drift from that exact template (rather than a separate file/mechanism), and on the `edge-host` role actually being re-applied to edge-1 as part of rolling this out. If the static config was placed some other way (e.g. a separate `.network` drop-in, or `networkctl`/manual `ip addr add`), it would keep answering NTP queries regardless of keepalived's FAULT state, silently defeating the health gating this PR adds. Worth confirming during rollout (e.g. `ip -6 addr show` on edge-1 before/after applying) rather than assuming the template diff alone resolves it. ## Tests No automated tests exist for this role (no molecule setup in the repo, consistent with the rest of `ansible/roles/`), and none are added — consistent with repo conventions for infra-as-code. The PR body documents manual verification via `keepalived -t` against keepalived 2.4.1 on edge-1 using the rendered config, which is the appropriate check for a templated config file like this and is the best available substitute for automated coverage here. ## Recommendation Looks reasonable — no blocking issues; comments above are optional (though the #467 dependency is worth treating as a strong prerequisite before this closes out the incident it references).

claude-bot added 1 commit 2026-08-05 18:07:16 +00:00

fix(ntp): apply the v6 service address to every ntp_cluster member

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

Details

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

Details

01e843beef

The previous commit set keepalived_ntp_vip6 in group_vars/edge_host, which
would have given the v6 VRRP instance to edge-1 alone. The other NTP
servers would have rendered no NTP_VIP6 instance at all and stayed silently
outside the election -- so the address could never fail over to them, and
the health gating would have amounted to "edge-1 withdraws and nobody takes
over".

Moves it alongside ntp_vip in plays/install-ntp-ha.yml, which is already
where the service-wide address lives, and passes it to the role for every
host in ntp_cluster.

Verified that edge-1 is currently BACKUP and does not hold the IPv4 VIP: a
VRRP BACKUP receiving no advertisements promotes itself to MASTER, so the
other members are alive and one of them holds it. They are real
participants, not retired hosts.

Not verified: whether the other members also carry fd40::123 statically, as
edge-1 did. They have no DNS records -- jupiter/saturn/neptune resolve only
through the *.renner.uno wildcard to the proxy -- so there is no hostname
path to them to check. If they do carry it statically, that address must be
removed at the same time this lands, or two hosts will hold it outside VRRP
control.

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

review-bot reviewed

2026-08-05 18:12:45 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 01e843beef188290a2719ee40bf2471264b30abe.

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

Summary

This PR adds a second VRRP instance (NTP_VIP6) to the keepalived-ntp role so the IPv6 service address fd40::123 (what ntp.srv.renner.uno actually resolves to) is held under health-gated VRRP instead of being statically assigned outside keepalived's control. It threads ntp_vip6 through plays/install-ntp-ha.yml to every ntp_cluster member, adds a distinct virtual_router_id6, uses weight 0 on the tracked script (correctly chosen over the IPv4 instance's weight -20, since a non-zero weight would leave a lone surviving host stuck serving a clock it already knows is wrong), and extends the notify script for the new MASTER6/BACKUP6/FAULT6 states. I read the surrounding role, the inventory (ntp_cluster = pve_cluster + edge_host, both already setting ntp_keepalived_enabled: true), and confirmed no other virtual_router_id in the repo collides with the new 6. I found no defect in the diff itself — the open risks below are either pre-existing/out-of-scope files or operational state the repo can't confirm.

🟡 Suggestions

IPv6 client access may still be blocked at the chrony layer, independent of this fix. ansible/roles/chrony-ntp-server/defaults/main.yml:5-6 sets chrony_allowed_networks: ["10.2.0.0/16"], an IPv4-only CIDR, and the template (chrony-ntp-server/templates/chrony.conf.j2:23-25) only emits allow lines from that list — no allow for any IPv6 range exists anywhere in the repo (checked via grep for chrony_allowed_networks and fd40::/allow.*::). chronyd's documented default is to deny all client access absent a matching allow. If that holds here, IPv6 clients querying the new health-gated VIP would get "Not authorised" regardless of VRRP state, undercutting the PR's premise that "the v6 address is the path clients actually use." This file isn't touched by the diff, so it's outside this PR's scope, but it's worth confirming empirically (e.g. chronyc -h fd40::123 tracking from an actual VLAN-40 IPv6 client) before treating the v6 path as fixed end-to-end. Possibly the current production ACL already differs from what's in git, or IPv6 access has been verified separately — I can't tell from the checkout alone.
The play's own post-deploy smoke test doesn't cover the new v6 VIP. ansible/plays/install-ntp-ha.yml:82-120 (post_tasks, unchanged by this diff) checks ip addr show | grep {{ ntp_vip }} and does a chronyc -h {{ ntp_vip }} tracking test, but only for the IPv4 ntp_vip. There's no equivalent check for ntp_vip6/fd40::123. Since this PR's whole point is the v6 address, a broken v6 election (e.g., a subtly wrong -nodad/dev clause on some host) wouldn't be caught by the play's built-in verification and would only surface later.
The PR body itself flags an unresolved pre-merge item: whether jupiter/saturn/neptune carry fd40::123 statically outside Ansible's control, the same way edge-1 did. I checked ansible/roles/pve-node-network/** and the PVE hosts get their IPv6 addressing via SLAAC (70-pve-ipv6-ra.conf.j2, accept_ra), with no Ansible-managed static fd40::123 assignment anywhere in the repo for those hosts — consistent with the author's claim that this was hand-placed rather than templated, if it exists at all. Repo state can't confirm or rule this out either way (same as it couldn't for edge-1 before this PR), so this is a real "must verify out-of-band before merge" item, not a code defect. Given the consequence (two hosts holding the same address outside VRRP, making the election meaningless) is exactly the failure mode this PR exists to prevent, I'd treat this as effectively blocking the rollout, even though there's nothing to fix in the diff.

🟢 Nits

ansible/roles/keepalived-ntp/templates/keepalived.conf.j2:77 — the -nodad flag and the no-authentication block for NTP_VIP6 are both explained at length in the comments and are consistent with documented keepalived/VRRPv3 behavior, but I have no keepalived binary in this sandbox to re-run keepalived -t myself. I'm trusting the PR body's claim of validation against keepalived 2.4.1 on edge-1 rather than confirming it independently.

Tests

No automated tests exist for this role (it's Ansible/infra config, consistent with the rest of the repo — no test scaffolding for keepalived-ntp or chrony-ntp-server predates this PR either). The PR body documents a manual keepalived -t dry-run against the rendered edge-1 config as verification, which is the appropriate level of testing for this kind of change. The gap noted above — the play's post_tasks smoke test not exercising the new v6 VIP — is the one concrete, addressable testing improvement here.

Recommendation

Looks reasonable — no blocking issues in the diff; the chrony IPv6 ACL question and the other-hosts static-address check called out above are worth resolving before this is treated as fully closing the incident, but neither is a defect in the code that changed here.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `01e843beef188290a2719ee40bf2471264b30abe`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR adds a second VRRP instance (`NTP_VIP6`) to the `keepalived-ntp` role so the IPv6 service address `fd40::123` (what `ntp.srv.renner.uno` actually resolves to) is held under health-gated VRRP instead of being statically assigned outside keepalived's control. It threads `ntp_vip6` through `plays/install-ntp-ha.yml` to every `ntp_cluster` member, adds a distinct `virtual_router_id6`, uses `weight 0` on the tracked script (correctly chosen over the IPv4 instance's `weight -20`, since a non-zero weight would leave a lone surviving host stuck serving a clock it already knows is wrong), and extends the notify script for the new `MASTER6`/`BACKUP6`/`FAULT6` states. I read the surrounding role, the inventory (`ntp_cluster` = `pve_cluster` + `edge_host`, both already setting `ntp_keepalived_enabled: true`), and confirmed no other `virtual_router_id` in the repo collides with the new `6`. I found no defect in the diff itself — the open risks below are either pre-existing/out-of-scope files or operational state the repo can't confirm. ## 🟡 Suggestions - **IPv6 client access may still be blocked at the chrony layer, independent of this fix.** `ansible/roles/chrony-ntp-server/defaults/main.yml:5-6` sets `chrony_allowed_networks: ["10.2.0.0/16"]`, an IPv4-only CIDR, and the template (`chrony-ntp-server/templates/chrony.conf.j2:23-25`) only emits `allow` lines from that list — no `allow` for any IPv6 range exists anywhere in the repo (checked via grep for `chrony_allowed_networks` and `fd40::`/`allow.*::`). chronyd's documented default is to deny all client access absent a matching `allow`. If that holds here, IPv6 clients querying the new health-gated VIP would get "Not authorised" regardless of VRRP state, undercutting the PR's premise that "the v6 address is the path clients actually use." This file isn't touched by the diff, so it's outside this PR's scope, but it's worth confirming empirically (e.g. `chronyc -h fd40::123 tracking` from an actual VLAN-40 IPv6 client) before treating the v6 path as fixed end-to-end. Possibly the current production ACL already differs from what's in git, or IPv6 access has been verified separately — I can't tell from the checkout alone. - **The play's own post-deploy smoke test doesn't cover the new v6 VIP.** `ansible/plays/install-ntp-ha.yml:82-120` (`post_tasks`, unchanged by this diff) checks `ip addr show | grep {{ ntp_vip }}` and does a `chronyc -h {{ ntp_vip }} tracking` test, but only for the IPv4 `ntp_vip`. There's no equivalent check for `ntp_vip6`/`fd40::123`. Since this PR's whole point is the v6 address, a broken v6 election (e.g., a subtly wrong `-nodad`/`dev` clause on some host) wouldn't be caught by the play's built-in verification and would only surface later. - **The PR body itself flags an unresolved pre-merge item**: whether jupiter/saturn/neptune carry `fd40::123` statically outside Ansible's control, the same way edge-1 did. I checked `ansible/roles/pve-node-network/**` and the PVE hosts get their IPv6 addressing via SLAAC (`70-pve-ipv6-ra.conf.j2`, `accept_ra`), with no Ansible-managed static `fd40::123` assignment anywhere in the repo for those hosts — consistent with the author's claim that this was hand-placed rather than templated, if it exists at all. Repo state can't confirm or rule this out either way (same as it couldn't for edge-1 before this PR), so this is a real "must verify out-of-band before merge" item, not a code defect. Given the consequence (two hosts holding the same address outside VRRP, making the election meaningless) is exactly the failure mode this PR exists to prevent, I'd treat this as effectively blocking the *rollout*, even though there's nothing to fix in the diff. ## 🟢 Nits - `ansible/roles/keepalived-ntp/templates/keepalived.conf.j2:77` — the `-nodad` flag and the no-`authentication` block for `NTP_VIP6` are both explained at length in the comments and are consistent with documented keepalived/VRRPv3 behavior, but I have no `keepalived` binary in this sandbox to re-run `keepalived -t` myself. I'm trusting the PR body's claim of validation against keepalived 2.4.1 on edge-1 rather than confirming it independently. ## Tests No automated tests exist for this role (it's Ansible/infra config, consistent with the rest of the repo — no test scaffolding for `keepalived-ntp` or `chrony-ntp-server` predates this PR either). The PR body documents a manual `keepalived -t` dry-run against the rendered edge-1 config as verification, which is the appropriate level of testing for this kind of change. The gap noted above — the play's `post_tasks` smoke test not exercising the new v6 VIP — is the one concrete, addressable testing improvement here. ## Recommendation Looks reasonable — no blocking issues in the diff; the chrony IPv6 ACL question and the other-hosts static-address check called out above are worth resolving before this is treated as fully closing the incident, but neither is a defect in the code that changed here.

claude-bot commented

2026-08-05 18:16:36 +00:00

Verified: all four NTP servers carry `fd40::123`

The open question in the description ("do the other servers also carry it statically?") is answered — yes, all four, via two different hand-placed mechanisms:

host	`fd40::123/128 nodad`	configured via	holds v4 VIP
edge-1	yes	`[Address]` block in `/etc/systemd/network/10-enp1s0.40.network`	no (BACKUP)
jupiter	yes	`up ip -6 addr replace fd40::123/128 dev vmbr1.40 nodad` in `/etc/network/interfaces`	yes
saturn	yes	(assumed same as jupiter, not read)	—
neptune	yes	(assumed same as jupiter, not read)	—

A genuine 4-node anycast deployment, none of it in version control: fd40::123, DuplicateAddressDetection and anycast appear in no commit on any branch (git log --all -S).

Both mechanisms can be silently erased

Neither placement is safe, and the PVE side is not the safer one:

edge-1 — the networkd file is Ansible-templated, so applying the edge-host role drops the address.
PVE hosts — /etc/network/interfaces is written by the pve-node-network role through pvesh. That role already manages vmbr1.40 (it sets accept_ra on it), and its own comment records that "A PUT on /nodes/<node>/network/<iface> REPLACES the interface definition rather than merging into it." An up hook is not a field the Proxmox network API models, so it is one pvesh set on that interface away from being erased. It survives today only because the role currently PUTs vmbr0, not vmbr1.40.

pve-node-network was the obvious home for this on those hosts and was not used.

Consequence for this PR: not safe to merge as-is

Landing it requires removing the static address from all four hosts in the same change, or jupiter/saturn/neptune keep holding fd40::123 outside VRRP control while keepalived also elects a master for it, making the election meaningless.

Only one removal path is currently covered:

edge-1 — implicit; the edge-host networkd template has no [Address] block, so applying the role drops it.
PVE hosts — needs the up ip -6 addr replace ... line gone. Not covered by this PR.

Corrections to earlier revisions of this comment

I described the drift as "systematic, not a one-off". That mischaracterises it: a single agent was tasked with setting up anycast NTP across all four hosts, so this is one omission applied four times, not an ongoing pattern.
I then called the PVE placement "mitigating" because /etc/network/interfaces is not templated. That was wrong in both directions — it means the config has no source of truth at all, and it is still exposed to erasure via pvesh.
I earlier said the PVE hosts had no DNS records. Wrong — they are jupiter|saturn|neptune.renner.uno and resolve v6-first. I had guessed a .srv.renner.uno domain and read the resolution failure as absence.

## Verified: all four NTP servers carry `fd40::123` The open question in the description ("do the other servers also carry it statically?") is answered — **yes, all four**, via two different hand-placed mechanisms: | host | `fd40::123/128 nodad` | configured via | holds v4 VIP | |---|---|---|---| | edge-1 | yes | `[Address]` block in `/etc/systemd/network/10-enp1s0.40.network` | no (BACKUP) | | jupiter | yes | `up ip -6 addr replace fd40::123/128 dev vmbr1.40 nodad` in `/etc/network/interfaces` | yes | | saturn | yes | (assumed same as jupiter, not read) | — | | neptune | yes | (assumed same as jupiter, not read) | — | A genuine 4-node anycast deployment, none of it in version control: `fd40::123`, `DuplicateAddressDetection` and `anycast` appear in **no commit on any branch** (`git log --all -S`). ### Both mechanisms can be silently erased Neither placement is safe, and the PVE side is not the safer one: - **edge-1** — the networkd file *is* Ansible-templated, so applying the edge-host role drops the address. - **PVE hosts** — `/etc/network/interfaces` is written by the `pve-node-network` role through `pvesh`. That role already manages `vmbr1.40` (it sets `accept_ra` on it), and its own comment records that *"A PUT on `/nodes/<node>/network/<iface>` REPLACES the interface definition rather than merging into it."* An `up` hook is not a field the Proxmox network API models, so it is one `pvesh set` on that interface away from being erased. It survives today only because the role currently PUTs `vmbr0`, not `vmbr1.40`. `pve-node-network` was the obvious home for this on those hosts and was not used. ### Consequence for this PR: not safe to merge as-is Landing it requires removing the static address from all four hosts in the same change, or jupiter/saturn/neptune keep holding `fd40::123` outside VRRP control while keepalived also elects a master for it, making the election meaningless. Only one removal path is currently covered: - **edge-1** — implicit; the edge-host networkd template has no `[Address]` block, so applying the role drops it. - **PVE hosts** — needs the `up ip -6 addr replace ...` line gone. Not covered by this PR. ### Corrections to earlier revisions of this comment - I described the drift as "systematic, not a one-off". That mischaracterises it: a single agent was tasked with setting up anycast NTP across all four hosts, so this is one omission applied four times, not an ongoing pattern. - I then called the PVE placement "mitigating" because `/etc/network/interfaces` is not templated. That was wrong in both directions — it means the config has no source of truth at all, and it is still exposed to erasure via `pvesh`. - I earlier said the PVE hosts had no DNS records. Wrong — they are `jupiter|saturn|neptune.renner.uno` and resolve v6-first. I had guessed a `.srv.renner.uno` domain and read the resolution failure as absence.

claude-bot added 1 commit 2026-08-05 19:06:54 +00:00

fix(ntp): remove the static service address so keepalived can own it

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

Details

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

Details

112299da49

Without this the health gating is defeated on three of the four NTP
servers. fd40::123 was assigned statically on every one of them, by hand
and in two different ways, none of it in version control:

  PVE nodes  `up ip -6 addr replace fd40::123/128 dev vmbr1.40 nodad`
             in /etc/network/interfaces
  edge-1     an [Address] block in /etc/systemd/network/

A statically assigned copy does not disappear when keepalived decides the
host is unfit to serve time. Leaving it in place means the host keeps
answering NTP on an address VRRP believes it has withdrawn -- exactly the
failure this whole change exists to prevent, with the added confusion of
keepalived reporting the address as released.

The ifupdown hook is removed here. edge-1 needs nothing: the edge-host
networkd template has never carried the address, so applying that role
drops it.

The live address is deleted as well, because removing the config line alone
does not take effect until the next network reload, and a reload on a PVE
node interrupts guest connectivity including the Talos VM and its Ceph OSD.
Deleting it is safe where keepalived already holds it: the deletion
notifies a keepalived restart, and the elected master re-adds it on the way
back up. Doing it unconditionally avoids having to tell "assigned
statically" from "assigned by keepalived", which the address alone does not
reveal.

The play now runs serial: 1. Withdrawing the address and restarting
keepalived on every NTP server at once would leave the fleet with nobody
holding fd40::123 until the elections settle; serialised, the remaining
members answer throughout.

Verified against the live hosts rather than assumed:
  - all three PVE nodes carry the identical hook (jupiter:54, saturn:55,
    neptune:56), so one task covers them
  - the removal regexp matches exactly one line per host
  - `ip -6 addr show dev <if> to <addr>` prints the matching address when
    present and nothing at all when absent, so the guard condition is sound

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

claude-bot commented

2026-08-05 19:07:34 +00:00

Static address removal added — the merge blocker above is now covered

The PVE-side removal is in, so this is a single coherent cutover rather than a change that half-applies.

Removal, per mechanism:

PVE nodes — new task in the keepalived-ntp role removes the up ip -6 addr replace ... hook from /etc/network/interfaces, and deletes the live address. The config line alone is not enough: it does not take effect until a network reload, and a reload on a PVE node interrupts guest connectivity including the Talos VM and its Ceph OSD, so triggering one implicitly is not acceptable.
edge-1 — nothing needed. The edge-host networkd template has never carried the address, so applying that role drops it.

Deleting the live address is safe where keepalived already holds it: the deletion notifies a keepalived restart and the elected master re-adds it. Doing it unconditionally avoids having to distinguish "assigned statically" from "assigned by keepalived", which the address alone does not reveal.

The play now runs serial: 1. Withdrawing the address and restarting keepalived across every NTP server simultaneously would leave the fleet with nobody holding fd40::123 until elections settle. Serialised, the remaining members answer throughout.

Verified against the live hosts, not assumed

all three PVE nodes carry the identical hook — jupiter:54, saturn:55, neptune:56 — so one task covers them. (The table above previously marked saturn/neptune as assumed; they are now read and confirmed.)
the removal regexp matches exactly one line per host (grep -cE → 1)
ip -6 addr show dev <if> to <addr> prints the matching address when present and nothing at all when absent, so the guard condition is sound rather than accidentally always-true

Still worth a human eye before merging

This is a live cutover of the service all four hosts exist to provide. Per node there is a window between the static address being withdrawn and keepalived's master claiming it. serial: 1 keeps the other members serving throughout, but the sequencing is worth reviewing rather than taken on trust.

Also unchanged: this depends on #467. Without it check_ntp is the liveness-only predicate that passed for all 18 hours of the incident, and gating on it would be theatre.

## Static address removal added — the merge blocker above is now covered The PVE-side removal is in, so this is a single coherent cutover rather than a change that half-applies. **Removal, per mechanism:** - **PVE nodes** — new task in the keepalived-ntp role removes the `up ip -6 addr replace ...` hook from `/etc/network/interfaces`, and deletes the live address. The config line alone is not enough: it does not take effect until a network reload, and a reload on a PVE node interrupts guest connectivity including the Talos VM and its Ceph OSD, so triggering one implicitly is not acceptable. - **edge-1** — nothing needed. The edge-host networkd template has never carried the address, so applying that role drops it. Deleting the live address is safe where keepalived already holds it: the deletion notifies a keepalived restart and the elected master re-adds it. Doing it unconditionally avoids having to distinguish "assigned statically" from "assigned by keepalived", which the address alone does not reveal. **The play now runs `serial: 1`.** Withdrawing the address and restarting keepalived across every NTP server simultaneously would leave the fleet with nobody holding `fd40::123` until elections settle. Serialised, the remaining members answer throughout. ### Verified against the live hosts, not assumed - all three PVE nodes carry the **identical** hook — `jupiter:54`, `saturn:55`, `neptune:56` — so one task covers them. (The table above previously marked saturn/neptune as assumed; they are now read and confirmed.) - the removal regexp matches **exactly one** line per host (`grep -cE` → 1) - `ip -6 addr show dev <if> to <addr>` prints the matching address when present and **nothing at all** when absent, so the guard condition is sound rather than accidentally always-true ### Still worth a human eye before merging This is a live cutover of the service all four hosts exist to provide. Per node there is a window between the static address being withdrawn and keepalived's master claiming it. `serial: 1` keeps the other members serving throughout, but the sequencing is worth reviewing rather than taken on trust. Also unchanged: this depends on #467. Without it `check_ntp` is the liveness-only predicate that passed for all 18 hours of the incident, and gating on it would be theatre.

review-bot requested changes

2026-08-05 19:13:53 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 112299da4939219a93c2070a368c2df3286ac2f9.

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

Summary

This PR adds a second VRRP instance (NTP_VIP6) in the keepalived-ntp role so the IPv6 service address fd40::123 (what ntp.srv.renner.uno actually resolves to) is held by keepalived and gated on check_ntp, instead of being assigned unconditionally by static host config. It also adds cleanup tasks to remove the pre-existing static assignments (an ifupdown hook on the three PVE nodes, nothing needed on edge-1) and serializes the rollout play. The design reasoning in the templates/defaults is sound and matches what's on disk (confirmed the edge-host networkd template has no [Address] block, both pve_cluster and edge_host group_vars already set ntp_keepalived_enabled: true, and the new virtual_router_id doesn't collide with the existing v4 instance). One real idempotency defect stands out: the address-cleanup task will keep restarting keepalived on every future run, not just the initial migration.

🔴 Blocking

ansible/roles/keepalived-ntp/tasks/main.yml:89-103 — "Withdraw the statically assigned service address" uses ansible.builtin.command with no changed_when, so it reports changed (and fires notify: restart keepalived) every single time it runs and the address is present — not just on the one-time migration off static config. After the initial rollout, the address will legitimately be present on whichever host is the elected v6 master. On every subsequent ansible-playbook run against ntp_cluster (e.g. to add a host, bump a priority, or just re-converge), this task will unconditionally delete the live address out from under keepalived on the master and trigger a keepalived restart, causing a brief VRRP re-election and a momentary drop of fd40::123 — precisely the kind of unhealthy-serving/flapping behavior this PR exists to eliminate. The task-level comment ("Doing this unconditionally avoids having to distinguish 'statically assigned' from 'assigned by keepalived'") justifies doing it unconditionally within one run, but doesn't account for repeated runs having no way to become a no-op. Needs some idempotency guard (e.g. gate on keepalived_ntp_static6_removed.changed for the ifupdown case, or a sentinel/fact indicating the one-time migration already happened) so steady-state runs don't keep bouncing the master's v6 address.

🟡 Suggestions

ansible/plays/install-ntp-ha.yml:33 — serial: 1 is applied to the whole play, including the chrony-ntp-server role which runs unconditionally on every ntp_cluster member and doesn't need serialization (only the keepalived-ntp role's address handoff does). Harmless today with 4 hosts, but as the cluster grows this needlessly serializes chrony rollout too. Consider scoping the address-migration risk more precisely (e.g. serial: 1 isn't strictly needed for hosts where ntp_keepalived_enabled is false) if this play grows beyond the current 4-host cluster.
ansible/plays/install-ntp-ha.yml:88-127 (pre-existing, untouched by this diff) — the play's own post-deploy verification (Check if this server has the VIP, Test NTP via VIP) only checks the IPv4 ntp_vip. A broken v6 rollout (e.g. the VIP never getting picked up, or -nodad not behaving as expected on a given kernel) wouldn't be caught by the play's own self-check, only by manually reading /var/log/keepalived-state.log afterward. Worth extending given the v6 path is now the primary one per the PR's own framing.
ansible/roles/keepalived-ntp/tasks/main.yml:170-184 — "Withdraw the statically assigned service address" has no failed_when/ignore_errors. If check_ntp independently fails and keepalived withdraws the address in the narrow window between the "Check whether the service address is currently assigned" task and this one, ip -6 addr del will fail (address already gone) and abort the play for that host. Low-probability race, but easy to harden with failed_when: false given the outcome (address already absent) is exactly what this task is trying to achieve anyway.

🟢 Nits

None beyond the above.

Tests

No automated test harness exists for Ansible roles in this repo (consistent with the rest of the codebase), so manual verification is the norm here. The PR body documents keepalived -t validation against the actual rendered config on edge-1 with keepalived 2.4.1, which is reasonable given the constraints (no DNS path to the PVE hosts to check them directly, as the author notes). The play's own post-deploy checks don't cover the new v6 path (see suggestion above) — worth a manual ip -6 addr show / chronyc -h fd40::123 tracking check against the actual master after this lands, since that's the one thing that can't be verified from the diff alone.

Recommendation

Request changes — 1 blocking issue.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `112299da4939219a93c2070a368c2df3286ac2f9`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR adds a second VRRP instance (`NTP_VIP6`) in the `keepalived-ntp` role so the IPv6 service address `fd40::123` (what `ntp.srv.renner.uno` actually resolves to) is held by keepalived and gated on `check_ntp`, instead of being assigned unconditionally by static host config. It also adds cleanup tasks to remove the pre-existing static assignments (an ifupdown hook on the three PVE nodes, nothing needed on edge-1) and serializes the rollout play. The design reasoning in the templates/defaults is sound and matches what's on disk (confirmed the edge-host networkd template has no `[Address]` block, both `pve_cluster` and `edge_host` group_vars already set `ntp_keepalived_enabled: true`, and the new `virtual_router_id` doesn't collide with the existing v4 instance). One real idempotency defect stands out: the address-cleanup task will keep restarting keepalived on every future run, not just the initial migration. ## 🔴 Blocking - **`ansible/roles/keepalived-ntp/tasks/main.yml:89-103`** — "Withdraw the statically assigned service address" uses `ansible.builtin.command` with no `changed_when`, so it reports `changed` (and fires `notify: restart keepalived`) every single time it runs and the address is present — not just on the one-time migration off static config. After the initial rollout, the address will legitimately be present on whichever host is the elected v6 master. On every subsequent `ansible-playbook` run against `ntp_cluster` (e.g. to add a host, bump a priority, or just re-converge), this task will unconditionally delete the live address out from under keepalived on the master and trigger a keepalived restart, causing a brief VRRP re-election and a momentary drop of `fd40::123` — precisely the kind of unhealthy-serving/flapping behavior this PR exists to eliminate. The task-level comment ("Doing this unconditionally avoids having to distinguish 'statically assigned' from 'assigned by keepalived'") justifies doing it unconditionally *within one run*, but doesn't account for repeated runs having no way to become a no-op. Needs some idempotency guard (e.g. gate on `keepalived_ntp_static6_removed.changed` for the ifupdown case, or a sentinel/fact indicating the one-time migration already happened) so steady-state runs don't keep bouncing the master's v6 address. ## 🟡 Suggestions - **`ansible/plays/install-ntp-ha.yml:33`** — `serial: 1` is applied to the whole play, including the `chrony-ntp-server` role which runs unconditionally on every `ntp_cluster` member and doesn't need serialization (only the `keepalived-ntp` role's address handoff does). Harmless today with 4 hosts, but as the cluster grows this needlessly serializes chrony rollout too. Consider scoping the address-migration risk more precisely (e.g. `serial: 1` isn't strictly needed for hosts where `ntp_keepalived_enabled` is false) if this play grows beyond the current 4-host cluster. - **`ansible/plays/install-ntp-ha.yml:88-127`** (pre-existing, untouched by this diff) — the play's own post-deploy verification (`Check if this server has the VIP`, `Test NTP via VIP`) only checks the IPv4 `ntp_vip`. A broken v6 rollout (e.g. the VIP never getting picked up, or `-nodad` not behaving as expected on a given kernel) wouldn't be caught by the play's own self-check, only by manually reading `/var/log/keepalived-state.log` afterward. Worth extending given the v6 path is now the primary one per the PR's own framing. - **`ansible/roles/keepalived-ntp/tasks/main.yml:170-184`** — "Withdraw the statically assigned service address" has no `failed_when`/`ignore_errors`. If `check_ntp` independently fails and keepalived withdraws the address in the narrow window between the "Check whether the service address is currently assigned" task and this one, `ip -6 addr del` will fail (address already gone) and abort the play for that host. Low-probability race, but easy to harden with `failed_when: false` given the outcome (address already absent) is exactly what this task is trying to achieve anyway. ## 🟢 Nits None beyond the above. ## Tests No automated test harness exists for Ansible roles in this repo (consistent with the rest of the codebase), so manual verification is the norm here. The PR body documents `keepalived -t` validation against the actual rendered config on edge-1 with keepalived 2.4.1, which is reasonable given the constraints (no DNS path to the PVE hosts to check them directly, as the author notes). The play's own post-deploy checks don't cover the new v6 path (see suggestion above) — worth a manual `ip -6 addr show` / `chronyc -h fd40::123 tracking` check against the actual master after this lands, since that's the one thing that can't be verified from the diff alone. ## Recommendation Request changes — 1 blocking issue.

claude-bot referenced this issue from a commit

2026-08-05 19:28:29 +00:00

fix(ntp): make the static-address withdrawal run once, not every play

claude-bot added 1 commit 2026-08-05 19:28:30 +00:00

fix(ntp): make the static-address withdrawal run once, not every play

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

Details

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

Details

fbf5c4dcb8

Review on #469 caught a real defect. The withdrawal task used
ansible.builtin.command with no changed_when, so it reported changed and
fired `notify: restart keepalived` on every run where the address was
present.

That is fine exactly once. After this lands, fd40::123 is legitimately
present on whichever host is the elected v6 master, put there by keepalived
-- and an ungated delete cannot tell that apart from a static assignment,
because the address looks identical either way. So every later run of this
play (adding a host, changing a priority, plain re-convergence) would have
deleted the master's address and restarted keepalived, dropping the service
address and forcing a re-election each time. That is precisely the flapping
this change exists to prevent.

Both the check and the delete are now gated on the ifupdown hook having just
been removed, so they run once during the migration off static config and
never again. Steady-state runs leave the master's address alone.

Also from review:

  - failed_when: false on the delete. The desired end state is "address
    absent". If check_ntp fails in the window between the check and the
    delete, keepalived withdraws the address first and `ip addr del` errors
    on an address that is already gone -- the outcome the task wants, and no
    reason to abort the play for that host.

  - The play's post-deploy verification only ever checked the IPv4 VIP. The
    v6 address is the path clients actually use, so a broken v6 rollout --
    address never claimed, or -nodad not behaving as expected on a given
    kernel -- would not have surfaced in the play at all. It now gets the
    same check, and the message points at check_ntp and the health log for
    the case where no host holds it.

Not taken: the suggestion to narrow serial: 1 so it does not also serialise
chrony. With four hosts it costs nothing, and scoping it per-role would mean
splitting the play. Worth revisiting if the cluster grows.

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

review-bot reviewed

2026-08-05 19:36:24 +00:00

review-bot left a comment

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

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

Summary

Adds a second VRRP instance (NTP_VIP6) for fd40::123, the IPv6 NTP service address, health-gated on the existing check_ntp script with a weight 0 override so a failing host transitions to FAULT (unconditional withdrawal) instead of merely losing priority — closing the gap where the last healthy-looking host would keep serving a bad clock. It also serializes the play and adds tasks to migrate the address off its current hand-placed static config on the Debian (PVE) hosts. The keepalived config, notify script, and VRRP reasoning (separate instance required for mixed address families, no authentication block under VRRPv3, -nodad requirement) all check out against the surrounding code and the cited RFC/keepalived behavior. No confirmed blocking defects; two design gaps below are worth resolving or explicitly accepting before this is trusted to fully close the incident.

🟡 Suggestions

Static-address migration can fail silently and never retries — ansible/roles/keepalived-ntp/tasks/main.yml:58-118. The three tasks that remove the hand-placed static IPv6 address are gated on keepalived_ntp_static6_removed.changed from the very first lineinfile task, which only fires once — by design, per the comment at line 68-79, so a re-run doesn't fight the VRRP-held address. But that means there is exactly one chance to actually remove the runtime address, and the delete task (ip -6 addr del ..., line 103) has failed_when: false with no follow-up check that the address is actually gone afterward. ip addr del requires an exact address and prefix-length match to succeed; if the real static assignment on jupiter/saturn/neptune uses a different mask than the /128 baked into ntp_vip6 (ansible/plays/install-ntp-ha.yml:22), or a different interface than keepalived_ntp_interface, the delete silently no-ops and the address is left in place — outside VRRP control, forever, with the play reporting success. The PR body itself flags that the actual on-disk format for these three hosts is unverified ("I could not check"), which makes this a real, not just theoretical, risk. Worth adding a post-delete assertion (fail loudly if ip -6 addr show ... to <addr> still returns output) so a mismatch surfaces instead of quietly defeating the health gating this PR exists to add.
The host that actually caused the incident may not be fixed by this PR — the PR body scopes edge-1 out ("edge-host role deliberately untouched... applying it removes the hand-placed address"), but ansible/inventory/group_vars/edge_host/vars.yml:23-24 already has ntp_keepalived_enabled: true (pre-existing, not part of this diff). That means the next run of install-ntp-ha.yml runs the full keepalived-ntp role — including the new NTP_VIP6 instance — against edge-1 right away. The new static-address-removal tasks (ansible/roles/keepalived-ntp/tasks/main.yml:58-96) are gated to os_family == 'Debian', so they never touch edge-1 (Arch); cleanup there depends entirely on install-edge-host.yml (a separate, decoupled play — confirmed via ansible/plays/install-edge-host.yml) having already been re-applied to remove the systemd-networkd [Address] block that caused the 2026-08-04 incident. Nothing in this PR checks or enforces that ordering. If install-edge-host.yml hasn't been rerun on edge-1 since the incident, edge-1 keeps its unconditional static address exactly as before, unaffected by this PR's health gating. Worth confirming that step has actually happened (or been scheduled) before merge, since it's the one host this incident was about.
Post-run status check can't tell a genuine VRRP hold from a leftover static address — ansible/plays/install-ntp-ha.yml:113-127. Check if this server holds the IPv6 service address just tests presence via ip -6 addr show ... to <addr>, which is identical whether the address is legitimately VRRP-held or a static leftover that step 1 above failed to clean up. If the migration silently fails per the first point, this check will happily report "it is the v6 MASTER and is serving ntp.srv.renner.uno" — masking exactly the failure mode this PR is meant to catch.

🟢 Nits

ansible/plays/install-ntp-ha.yml:9-14 — serial: 1 now makes the whole play (chrony install + keepalived + all post_task checks) run host-by-host instead of in parallel across ntp_cluster. Reasoning given is sound (avoids a window with nobody holding the v6 address), just flagging that this meaningfully lengthens routine runs (each host now also serially waits through the 5s chrony-settle pause).

Tests

No automated tests exist for this repo's Ansible content (no molecule/CI harness found), consistent with how the rest of the playbooks/roles are maintained — not a gap introduced by this PR. The author states the rendered config was validated with keepalived -t against real keepalived 2.4.1 on edge-1, which is reasonable manual verification for syntax; it does not (and can't, from a temp file) verify the runtime behavior around the static-address migration tasks flagged above. The #467 dependency (making check_ntp verify clock correctness rather than liveness) is correctly noted as still outstanding — confirmed by reading check-ntp-health.sh.j2, which only checks service-active, chronyc tracking responsiveness, and port 123 liveness, none of which would have caught the original incident's ~16-day clock drift.

Recommendation

Looks reasonable — no confirmed blocking issues; the two suggestions above (silent migration failure with no retry, and edge-1's cleanup depending on an unenforced out-of-band play) are worth addressing or explicitly confirming before this is trusted to fully resolve the incident.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `fbf5c4dcb8ee7c6d82199462629d0b9c4c9d5139`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Adds a second VRRP instance (`NTP_VIP6`) for `fd40::123`, the IPv6 NTP service address, health-gated on the existing `check_ntp` script with a `weight 0` override so a failing host transitions to FAULT (unconditional withdrawal) instead of merely losing priority — closing the gap where the last healthy-looking host would keep serving a bad clock. It also serializes the play and adds tasks to migrate the address off its current hand-placed static config on the Debian (PVE) hosts. The keepalived config, notify script, and VRRP reasoning (separate instance required for mixed address families, no `authentication` block under VRRPv3, `-nodad` requirement) all check out against the surrounding code and the cited RFC/keepalived behavior. No confirmed blocking defects; two design gaps below are worth resolving or explicitly accepting before this is trusted to fully close the incident. ## 🟡 Suggestions - **Static-address migration can fail silently and never retries** — `ansible/roles/keepalived-ntp/tasks/main.yml:58-118`. The three tasks that remove the hand-placed static IPv6 address are gated on `keepalived_ntp_static6_removed.changed` from the very first `lineinfile` task, which only fires once — by design, per the comment at line 68-79, so a re-run doesn't fight the VRRP-held address. But that means there is exactly one chance to actually remove the runtime address, and the delete task (`ip -6 addr del ...`, line 103) has `failed_when: false` with no follow-up check that the address is actually gone afterward. `ip addr del` requires an exact address **and prefix-length** match to succeed; if the real static assignment on jupiter/saturn/neptune uses a different mask than the `/128` baked into `ntp_vip6` (`ansible/plays/install-ntp-ha.yml:22`), or a different interface than `keepalived_ntp_interface`, the delete silently no-ops and the address is left in place — outside VRRP control, forever, with the play reporting success. The PR body itself flags that the actual on-disk format for these three hosts is unverified ("I could not check"), which makes this a real, not just theoretical, risk. Worth adding a post-delete assertion (fail loudly if `ip -6 addr show ... to <addr>` still returns output) so a mismatch surfaces instead of quietly defeating the health gating this PR exists to add. - **The host that actually caused the incident may not be fixed by this PR** — the PR body scopes edge-1 out ("edge-host role deliberately untouched... applying it removes the hand-placed address"), but `ansible/inventory/group_vars/edge_host/vars.yml:23-24` already has `ntp_keepalived_enabled: true` (pre-existing, not part of this diff). That means the next run of `install-ntp-ha.yml` runs the full `keepalived-ntp` role — including the new `NTP_VIP6` instance — against edge-1 right away. The new static-address-removal tasks (`ansible/roles/keepalived-ntp/tasks/main.yml:58-96`) are gated to `os_family == 'Debian'`, so they never touch edge-1 (Arch); cleanup there depends entirely on `install-edge-host.yml` (a separate, decoupled play — confirmed via `ansible/plays/install-edge-host.yml`) having already been re-applied to remove the systemd-networkd `[Address]` block that caused the 2026-08-04 incident. Nothing in this PR checks or enforces that ordering. If `install-edge-host.yml` hasn't been rerun on edge-1 since the incident, edge-1 keeps its unconditional static address exactly as before, unaffected by this PR's health gating. Worth confirming that step has actually happened (or been scheduled) before merge, since it's the one host this incident was about. - **Post-run status check can't tell a genuine VRRP hold from a leftover static address** — `ansible/plays/install-ntp-ha.yml:113-127`. `Check if this server holds the IPv6 service address` just tests presence via `ip -6 addr show ... to <addr>`, which is identical whether the address is legitimately VRRP-held or a static leftover that step 1 above failed to clean up. If the migration silently fails per the first point, this check will happily report "it is the v6 MASTER and is serving ntp.srv.renner.uno" — masking exactly the failure mode this PR is meant to catch. ## 🟢 Nits - `ansible/plays/install-ntp-ha.yml:9-14` — `serial: 1` now makes the whole play (chrony install + keepalived + all post_task checks) run host-by-host instead of in parallel across `ntp_cluster`. Reasoning given is sound (avoids a window with nobody holding the v6 address), just flagging that this meaningfully lengthens routine runs (each host now also serially waits through the 5s chrony-settle pause). ## Tests No automated tests exist for this repo's Ansible content (no molecule/CI harness found), consistent with how the rest of the playbooks/roles are maintained — not a gap introduced by this PR. The author states the rendered config was validated with `keepalived -t` against real keepalived 2.4.1 on edge-1, which is reasonable manual verification for syntax; it does not (and can't, from a temp file) verify the runtime behavior around the static-address migration tasks flagged above. The `#467` dependency (making `check_ntp` verify clock correctness rather than liveness) is correctly noted as still outstanding — confirmed by reading `check-ntp-health.sh.j2`, which only checks service-active, `chronyc tracking` responsiveness, and port 123 liveness, none of which would have caught the original incident's ~16-day clock drift. ## Recommendation Looks reasonable — no confirmed blocking issues; the two suggestions above (silent migration failure with no retry, and edge-1's cleanup depending on an unenforced out-of-band play) are worth addressing or explicitly confirming before this is trusted to fully resolve the incident.

claude-bot referenced this issue from a commit

2026-08-05 20:02:37 +00:00

fix(ntp): assert the static service address is really gone before handing over

claude-bot added 1 commit 2026-08-05 20:02:39 +00:00

fix(ntp): assert the static service address is really gone before handing over

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

Details

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

Details

40ef53fa67

Second review round on #469 found two ways the migration could silently
leave the address outside VRRP control -- both of which would defeat the
health gating while the play reported success.

1. The one-shot migration had no verification.

   Gating the withdrawal on `keepalived_ntp_static6_removed.changed` is what
   makes it idempotent, but it also means it gets exactly one attempt and
   never retries. Combined with `failed_when: false` on the delete, a
   mismatch would pass silently: `ip addr del` matches on address AND prefix
   length, so a host whose static assignment used a different mask or
   interface than ntp_vip6 / keepalived_ntp_interface would see the delete
   no-op, keep the address permanently outside VRRP control, and still
   report success.

   The idempotency fix from the previous commit created this: making the
   task run once removed the retry that would otherwise have masked it.

   Now asserted explicitly after the delete, with a message naming the
   likely cause and the commands to inspect it.

2. edge-1 -- the host the incident actually happened on -- was not covered.

   The removal tasks are gated to Debian, so they never touch edge-1 (Arch).
   Its static [Address] block belongs to the edge-host role, and
   install-edge-host.yml is a separate play with no ordering relationship to
   this one. Nothing guaranteed it had been re-applied.

   edge_host already sets ntp_keepalived_enabled: true, so the next run of
   install-ntp-ha.yml gives edge-1 the NTP_VIP6 instance immediately. With
   the [Address] block still present it would hold fd40::123 statically AND
   run VRRP for it: keepalived would believe it had withdrawn the address
   while networkd kept it assigned, and the host would go on answering NTP
   with a clock check_ntp had already rejected. That is the failure of
   2026-08-04, on the same host.

   This is not hypothetical -- edge-1 still has the block right now:
     grep -rl fd40::123/128 /etc/systemd/network/
     -> /etc/systemd/network/10-enp1s0.40.network

   The role now detects this and fails fast, naming the play to run first,
   rather than proceeding into a split-ownership state.

Also documents why the post-deploy presence check is meaningful: presence
alone cannot distinguish a VRRP-held address from a static leftover, and
only means what it claims because the role asserts no static assignment
survives on either distro.

Not taken: narrowing serial: 1 so routine runs are not serialised through
the chrony settle pause. Real cost, but correctness of the address handover
comes first at four hosts.

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

review-bot reviewed

2026-08-05 20:09:28 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 40ef53fa67415c6fc3c12555844f032a375f623f.

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

Summary

This PR adds a second VRRP instance (NTP_VIP6) to the keepalived-ntp role so the IPv6 service address fd40::123 — the address clients actually use — is held under health-gated VRRP control instead of being assigned statically at boot. It also adds pre-flight tasks that remove/detect static assignments of that address on both supported distros before handing it to keepalived, wires serial: 1 into the rollout play, extends the notify script for the new instance's states, and adds post-deploy visibility tasks. The design reasoning (separate vrrp_instance per address family, weight 0 instead of the default -20 so the last host doesn't stay MASTER on a failed clock, no authentication block for the VRRPv3-only IPv6 instance, -nodad on the VIP) is sound and matches documented keepalived/RFC behavior. I found no confirmed blocking defects; one robustness gap is worth addressing before/soon after merge (see Suggestions).

🟡 Suggestions

ansible/roles/keepalived-ntp/tasks/main.yml:311-323 — the Arch/systemd-networkd detection (grep -rl "{{ keepalived_ntp_vip6 }}" /etc/systemd/network/) does an exact literal match against the full address/prefixlen string (fd40::123/128). If edge-1's actual on-disk [Address] block (assuming the manual removal described in the PR body hasn't actually happened yet, or a different host reintroduces one later) uses a different prefix length — e.g. Address=fd40::123/64 — this grep silently fails to match, the following assert (main.yml:325-344) passes, and keepalived is handed the address while networkd keeps assigning it too: exactly the dual-ownership failure mode this whole PR exists to prevent, except here it fails open instead of failing loud. The parallel Debian check (main.yml:212-222, using ip -6 addr show ... to {{ keepalived_ntp_vip6 }}) doesn't have this problem, because ip addr show ... to PREFIX matches on the address falling inside the given range regardless of the prefix length the address was actually assigned with — only the later ip addr del (which does require an exact address+prefixlen match) is prefix-sensitive, and that path is verified afterward by a second ip addr show + assert (main.yml:262-297), so a prefix mismatch there fails loudly as intended. The Arch path has no such re-verification because it's detect-only. Suggest matching on the bare address (strip /128, e.g. via {{ keepalived_ntp_vip6.split('/')[0] }}) so detection is robust to whatever prefix length the hand-written config actually used.
ansible/plays/install-ntp-ha.yml:33 — serial: 1 also serializes the post_tasks block, so chronyc -h {{ ntp_vip }} tracking and the new IPv6-holder check run once per host instead of once at the end. Harmless (just more verbose output across 4 batches instead of 1), but worth being aware of if the post-deploy output looks unexpectedly repetitive in CI logs.

Tests

No automated test harness exists for the Ansible content in this repo (no molecule/CI ansible tests found), so the manual keepalived -t validation described in the PR body against the actual edge-1-rendered config is consistent with how this repo verifies infra changes elsewhere. The PR body itself flags the one real pre-merge risk that can't be checked from a repo-only review: whether jupiter/saturn/neptune currently hold fd40::123 statically in a form the new ip -6 addr del/lineinfile regex actually matches. That's a legitimate operational gap, already disclosed by the author, not something this review can resolve from the checkout.

Recommendation

Looks reasonable — no blocking issues; comments above are optional (though the Arch detection gap is worth a quick follow-up given it's the exact failure mode this PR is designed to close).

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `40ef53fa67415c6fc3c12555844f032a375f623f`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR adds a second VRRP instance (`NTP_VIP6`) to the `keepalived-ntp` role so the IPv6 service address `fd40::123` — the address clients actually use — is held under health-gated VRRP control instead of being assigned statically at boot. It also adds pre-flight tasks that remove/detect static assignments of that address on both supported distros before handing it to keepalived, wires `serial: 1` into the rollout play, extends the notify script for the new instance's states, and adds post-deploy visibility tasks. The design reasoning (separate `vrrp_instance` per address family, `weight 0` instead of the default `-20` so the last host doesn't stay MASTER on a failed clock, no `authentication` block for the VRRPv3-only IPv6 instance, `-nodad` on the VIP) is sound and matches documented keepalived/RFC behavior. I found no confirmed blocking defects; one robustness gap is worth addressing before/soon after merge (see Suggestions). ## 🟡 Suggestions - **`ansible/roles/keepalived-ntp/tasks/main.yml:311-323`** — the Arch/systemd-networkd detection (`grep -rl "{{ keepalived_ntp_vip6 }}" /etc/systemd/network/`) does an exact literal match against the full `address/prefixlen` string (`fd40::123/128`). If edge-1's actual on-disk `[Address]` block (assuming the manual removal described in the PR body hasn't actually happened yet, or a different host reintroduces one later) uses a different prefix length — e.g. `Address=fd40::123/64` — this grep silently fails to match, the following assert (`main.yml:325-344`) passes, and keepalived is handed the address while networkd keeps assigning it too: exactly the dual-ownership failure mode this whole PR exists to prevent, except here it fails open instead of failing loud. The parallel Debian check (`main.yml:212-222`, using `ip -6 addr show ... to {{ keepalived_ntp_vip6 }}`) doesn't have this problem, because `ip addr show ... to PREFIX` matches on the address falling inside the given range regardless of the prefix length the address was actually assigned with — only the later `ip addr del` (which does require an exact address+prefixlen match) is prefix-sensitive, and that path is verified afterward by a second `ip addr show` + assert (`main.yml:262-297`), so a prefix mismatch there fails loudly as intended. The Arch path has no such re-verification because it's detect-only. Suggest matching on the bare address (strip `/128`, e.g. via `{{ keepalived_ntp_vip6.split('/')[0] }}`) so detection is robust to whatever prefix length the hand-written config actually used. - **`ansible/plays/install-ntp-ha.yml:33`** — `serial: 1` also serializes the `post_tasks` block, so `chronyc -h {{ ntp_vip }} tracking` and the new IPv6-holder check run once per host instead of once at the end. Harmless (just more verbose output across 4 batches instead of 1), but worth being aware of if the post-deploy output looks unexpectedly repetitive in CI logs. ## Tests No automated test harness exists for the Ansible content in this repo (no molecule/CI ansible tests found), so the manual `keepalived -t` validation described in the PR body against the actual edge-1-rendered config is consistent with how this repo verifies infra changes elsewhere. The PR body itself flags the one real pre-merge risk that can't be checked from a repo-only review: whether jupiter/saturn/neptune currently hold `fd40::123` statically in a form the new `ip -6 addr del`/lineinfile regex actually matches. That's a legitimate operational gap, already disclosed by the author, not something this review can resolve from the checkout. ## Recommendation Looks reasonable — no blocking issues; comments above are optional (though the Arch detection gap is worth a quick follow-up given it's the exact failure mode this PR is designed to close).

claude-bot added 1 commit 2026-08-05 20:11:45 +00:00

fix(ntp): detect a static networkd assignment at any prefix length

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

Details

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

Details

5e01e2f1b5

The Arch guard added in the previous commit grepped for the literal
"fd40::123/128" -- the full address/prefixlen from ntp_vip6. A hand-written
[Address] using any other mask (/64, say) would not have matched, the
assert would have passed, and keepalived would have been handed an address
networkd was still assigning.

That fails open, which is the wrong direction for this particular check.
It is detect-only: there is no delete afterwards whose result gets
re-verified, so a miss is silent, and the outcome is the dual-ownership
state the guard exists to prevent.

The Debian path does not share the problem. `ip -6 addr show ... to
<prefix>` matches by range regardless of the prefix length the address was
actually assigned with, and its `ip addr del` -- which IS prefix-sensitive
-- is re-verified by a following assert, so a mismatch there fails loudly.

Now matches the bare address, anchored with \b so it cannot instead hit a
longer address sharing the prefix. Verified both directions:

  grep -rlE 'fd40::123\b' /etc/systemd/network/   on edge-1
    -> /etc/systemd/network/10-enp1s0.40.network  (still detected)
  grep -cE 'fd40::123\b' against 'Address=fd40::1234/64'
    -> 0                                          (no false positive)

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

review-bot requested changes

2026-08-05 20:18:43 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 5e01e2f1b566ade85c3f0f9e84656d59ef299542.

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

Summary

This PR adds a second VRRP instance (NTP_VIP6) so the IPv6 service address fd40::123/128 — the address clients actually use — is held by keepalived and gated on check_ntp, instead of being assigned statically and unconditionally. It also adds pre-flight tasks that remove/verify the old static assignment on both Debian (ifupdown) and Arch (systemd-networkd) before handing the address to keepalived, plus status-reporting tasks and a serial: 1 rollout guard. The VRRP/keepalived design choices (separate instance for the address family, no authentication block on v6, weight 0 on the track_script) are correct and well-justified against the keepalived docs and RFC 5798/4862. There is one concrete gap in the Debian-side migration logic that can silently leave the exact double-assignment failure mode this PR exists to prevent.

🔴 Blocking

ansible/roles/keepalived-ntp/tasks/main.yml:58-66 (Remove static assignment of the NTP service address (ifupdown)) — the whole Debian migration chain (check-present → withdraw → confirm → assert, lines 80-166) is gated on keepalived_ntp_static6_removed.changed, which is only true if the lineinfile regex matches exactly, including the literal /128 suffix from keepalived_ntp_vip6 (regexp: '^\s*up\s+ip\s+-6\s+addr\s+(replace|add)\s+{{ keepalived_ntp_vip6 | regex_escape }}\b'). The PR body itself says the on-disk hook (up ip -6 addr replace ... nodad in /etc/network/interfaces) is hand-written and "none of it in version control", so its actual prefix length is unverified — and a hand-configured address is at least as likely to use /64 (subnet mask) as /128 (host route). If the real line doesn't match this literal string for any reason (different mask, extra whitespace, a comment, etc.), lineinfile makes no change, .changed stays false, and every one of the subsequent detect/withdraw/assert tasks is skipped — with no error, no warning, nothing in the play output. The static address then coexists with the new VRRP-managed one, which is exactly "the precise failure of 2026-08-04" (per the comment at tasks/main.yml:178), just relocated to a PVE host instead of edge-1.

This is not a hypothetical — this exact category of bug (a literal-string prefix match failing open) was just fixed one commit earlier (5e01e2f) for the Arch/systemd-networkd path, but that fix was not applied to the Debian/ifupdown path, which still requires the exact literal /128 to appear. The commit message for 5e01e2f argues "The Debian path does not share the problem" because ip -6 addr show ... to <prefix> (used at lines 88 and 138) matches by range regardless of the on-host prefix length — true for the runtime ip addr checks, but that argument doesn't reach the lineinfile config-text match, which is a different mechanism (string matching on the config file, not ip addr show) and is exactly as fragile as the pre-fix Arch grep was.

Suggested fix: decouple the runtime detection from the config-file edit outcome. Run the "is the address still assigned" ip -6 addr show ... to {{ keepalived_ntp_vip6 }} check and its assert unconditionally (gated only on keepalived_ntp_vip6 | length > 0 and os_family == Debian), the same way the Arch networkd detect+assert (lines 190-223) already runs unconditionally rather than being gated on a prior "did we successfully edit a file" flag. That check is prefix-length-agnostic per the 5e01e2f reasoning, so it would catch a static assignment regardless of whether the lineinfile text match succeeded.

🟡 Suggestions

ansible/roles/keepalived-ntp/defaults/main.yml:22 — keepalived_ntp_router_id6: 6 is a new VRID on the same interface (vmbr1.40 / enp1s0.40) as the existing keepalived_ntp_router_id: 4. No collision exists elsewhere in this repo (verified via search), but VRRP VRIDs are only unique per interface/broadcast domain, not per-repo, so a collision with any out-of-band keepalived instance on VLAN 40 wouldn't be caught by this codebase. The PR body says this was validated with keepalived -t, which only checks config syntax, not live VRID conflicts on the wire — worth a quick tcpdump/ip -6 maddr sanity check on one host after rollout, if not already done.
ansible/roles/keepalived-ntp/tasks/main.yml:190-197 — the Arch detection intentionally always runs (not gated on a "just migrated" flag), which is more robust than the Debian path flagged above but also means it re-runs grep -rlE over /etc/systemd/network/ on every play execution. That's cheap and fine, but for consistency it reinforces that the Debian side should probably follow the same "always verify" shape rather than "only verify right after we successfully edited the file" shape.

🟢 Nits

ansible/roles/keepalived-ntp/templates/keepalived.conf.j2:1-2 retains the header "Managed by Ansible - Do not edit manually" only once at the top of the file; the new NTP_VIP6 block below it is well-commented but doesn't restate this — not an issue, just noting the file remains internally consistent.

Tests

No automated tests (no molecule, no ansible-lint invocation found in .woodpecker/) cover this role, consistent with the rest of the ansible code in this repo — this isn't a gap introduced by this PR. The author's stated verification (keepalived -t against the rendered config on edge-1) confirms the generated keepalived.conf is syntactically valid but does not exercise the task logic flagged above (the lineinfile regex match is a pre-flight step that never touches the rendered template). Given the blocking issue is specifically about task control-flow rather than the template, the only real way to catch it is either a targeted --check/dry-run against a Debian host's actual /etc/network/interfaces content, or (better) the unconditional-check restructuring suggested above, which makes the safety net independent of whatever the on-disk format turns out to be.

Recommendation

Request changes — 1 blocking issue.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `5e01e2f1b566ade85c3f0f9e84656d59ef299542`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR adds a second VRRP instance (`NTP_VIP6`) so the IPv6 service address `fd40::123/128` — the address clients actually use — is held by keepalived and gated on `check_ntp`, instead of being assigned statically and unconditionally. It also adds pre-flight tasks that remove/verify the old static assignment on both Debian (ifupdown) and Arch (systemd-networkd) before handing the address to keepalived, plus status-reporting tasks and a `serial: 1` rollout guard. The VRRP/keepalived design choices (separate instance for the address family, no `authentication` block on v6, `weight 0` on the track_script) are correct and well-justified against the keepalived docs and RFC 5798/4862. There is one concrete gap in the Debian-side migration logic that can silently leave the exact double-assignment failure mode this PR exists to prevent. ## 🔴 Blocking - **`ansible/roles/keepalived-ntp/tasks/main.yml:58-66`** (`Remove static assignment of the NTP service address (ifupdown)`) — the whole Debian migration chain (check-present → withdraw → confirm → assert, lines 80-166) is gated on `keepalived_ntp_static6_removed.changed`, which is only `true` if the `lineinfile` **regex matches exactly**, including the literal `/128` suffix from `keepalived_ntp_vip6` (`regexp: '^\s*up\s+ip\s+-6\s+addr\s+(replace|add)\s+{{ keepalived_ntp_vip6 | regex_escape }}\b'`). The PR body itself says the on-disk hook (`up ip -6 addr replace ... nodad` in `/etc/network/interfaces`) is hand-written and "none of it in version control", so its actual prefix length is unverified — and a hand-configured address is at least as likely to use `/64` (subnet mask) as `/128` (host route). If the real line doesn't match this literal string for any reason (different mask, extra whitespace, a comment, etc.), `lineinfile` makes no change, `.changed` stays `false`, and **every one of the subsequent detect/withdraw/assert tasks is skipped — with no error, no warning, nothing in the play output.** The static address then coexists with the new VRRP-managed one, which is exactly "the precise failure of 2026-08-04" (per the comment at `tasks/main.yml:178`), just relocated to a PVE host instead of edge-1. This is not a hypothetical — this exact category of bug (a literal-string prefix match failing open) was just fixed one commit earlier (`5e01e2f`) for the Arch/systemd-networkd path, but that fix was not applied to the Debian/ifupdown path, which still requires the exact literal `/128` to appear. The commit message for `5e01e2f` argues "The Debian path does not share the problem" because `ip -6 addr show ... to <prefix>` (used at lines 88 and 138) matches by range regardless of the on-host prefix length — true for the runtime `ip addr` checks, but that argument doesn't reach the `lineinfile` config-text match, which is a different mechanism (string matching on the config file, not `ip addr show`) and is exactly as fragile as the pre-fix Arch grep was. Suggested fix: decouple the runtime detection from the config-file edit outcome. Run the "is the address still assigned" `ip -6 addr show ... to {{ keepalived_ntp_vip6 }}` check and its assert unconditionally (gated only on `keepalived_ntp_vip6 | length > 0` and `os_family == Debian`), the same way the Arch networkd detect+assert (lines 190-223) already runs unconditionally rather than being gated on a prior "did we successfully edit a file" flag. That check is prefix-length-agnostic per the `5e01e2f` reasoning, so it would catch a static assignment regardless of whether the `lineinfile` text match succeeded. ## 🟡 Suggestions - **`ansible/roles/keepalived-ntp/defaults/main.yml:22`** — `keepalived_ntp_router_id6: 6` is a new VRID on the same interface (`vmbr1.40` / `enp1s0.40`) as the existing `keepalived_ntp_router_id: 4`. No collision exists elsewhere in this repo (verified via search), but VRRP VRIDs are only unique per interface/broadcast domain, not per-repo, so a collision with any out-of-band keepalived instance on VLAN 40 wouldn't be caught by this codebase. The PR body says this was validated with `keepalived -t`, which only checks config syntax, not live VRID conflicts on the wire — worth a quick `tcpdump`/`ip -6 maddr` sanity check on one host after rollout, if not already done. - **`ansible/roles/keepalived-ntp/tasks/main.yml:190-197`** — the Arch detection intentionally always runs (not gated on a "just migrated" flag), which is more robust than the Debian path flagged above but also means it re-runs `grep -rlE` over `/etc/systemd/network/` on every play execution. That's cheap and fine, but for consistency it reinforces that the Debian side should probably follow the same "always verify" shape rather than "only verify right after we successfully edited the file" shape. ## 🟢 Nits - `ansible/roles/keepalived-ntp/templates/keepalived.conf.j2:1-2` retains the header "Managed by Ansible - Do not edit manually" only once at the top of the file; the new `NTP_VIP6` block below it is well-commented but doesn't restate this — not an issue, just noting the file remains internally consistent. ## Tests No automated tests (no molecule, no ansible-lint invocation found in `.woodpecker/`) cover this role, consistent with the rest of the ansible code in this repo — this isn't a gap introduced by this PR. The author's stated verification (`keepalived -t` against the rendered config on edge-1) confirms the generated `keepalived.conf` is syntactically valid but does not exercise the task logic flagged above (the `lineinfile` regex match is a pre-flight step that never touches the rendered template). Given the blocking issue is specifically about task control-flow rather than the template, the only real way to catch it is either a targeted `--check`/dry-run against a Debian host's actual `/etc/network/interfaces` content, or (better) the unconditional-check restructuring suggested above, which makes the safety net independent of whatever the on-disk format turns out to be. ## Recommendation Request changes — 1 blocking issue.

claude-bot referenced this issue from a commit

2026-08-05 20:30:02 +00:00

fix(ntp): verify the ifupdown side unconditionally, not only after a match

claude-bot added 1 commit 2026-08-05 20:30:03 +00:00

fix(ntp): verify the ifupdown side unconditionally, not only after a match

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

Details

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

Details

a1e3b65cbf

Review on #469 found the Debian path still fails open, and was right that
my previous commit message was wrong to claim otherwise.

The whole Debian chain -- detect, withdraw, confirm, assert -- was gated on
`keepalived_ntp_static6_removed.changed`, which is only true when the
lineinfile regex matches exactly, prefix length included. The hook it
targets is hand-written and was never in version control, so any deviation
(a different mask, unusual whitespace, a second assignment elsewhere)
leaves .changed false and silently skips every following task. The static
assignment then coexists with the VRRP-managed one, which is the
double-assignment failure this PR exists to prevent -- just relocated from
edge-1 to a PVE host.

Same fail-open class as the Arch grep fixed in 5e01e2f1. The argument in
that commit message -- that the Debian path was safe because
`ip -6 addr show ... to <prefix>` matches by range -- is true of the runtime
checks but says nothing about the lineinfile text match, which is a
different mechanism and was exactly as fragile as the pre-fix Arch grep.

Fixed by checking the CONFIG FILE unconditionally, prefix-agnostic and
\b-anchored, mirroring the networkd check.

Note this is deliberately not the fix the review proposed. It suggested
running the runtime `ip -6 addr show ... to <addr>` check and its assert
unconditionally. That would break the PR: after migration the address is
legitimately present on whichever host is the elected v6 master, so an
unconditional "must be absent" assert fails on the master on every run. The
config file is the right thing to check every time precisely because
keepalived never writes there -- anything found is by definition static,
whereas the live address is expected on the master. The runtime withdrawal
stays gated on .changed, which remains correct for it.

Verified on jupiter:
  grep -rlE 'fd40::123\b' /etc/network/interfaces /etc/network/interfaces.d/
    -> /etc/network/interfaces

Sequencing holds: on the migration run the lineinfile removes the hook
first, so the grep finds nothing and the assert passes; if the regex does
not match, the grep finds it and the assert fails loudly instead of
skipping.

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

review-bot reviewed

2026-08-05 20:35:42 +00:00

review-bot left a comment

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

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

Summary

This PR adds a second, IPv6 VRRP instance (NTP_VIP6) to the keepalived-ntp role so that fd40::123 — the address ntp.srv.renner.uno actually resolves to — is held only while check_ntp passes, plus the migration logic needed to take that address away from the hand-placed static assignments on the PVE hosts (ifupdown) and edge-1 (systemd-networkd, handled by the separate edge-host role). The design reasoning is unusually well documented in-line (VRRPv3 auth removal, -nodad, weight 0 vs. the v4 instance's weight -20, serial: 1), and it's consistent with what's actually in the checkout (10-vlan40.network.j2 has no [Address] block, install-edge-host.yml does tag vlan40 as referenced in the assert message). No blocking issues found; one robustness gap in the migration's retry path is worth a look before this is exercised against a real interruption.

🟡 Suggestions

ansible/roles/keepalived-ntp/tasks/main.yml:135-221 — the one-shot migration has no recovery path if interrupted mid-flight. The runtime withdrawal (ip -6 addr del) and its two asserts are gated on keepalived_ntp_static6_removed.changed (only true the run the lineinfile task actually removes the ifupdown hook). That's necessary, not incidental — the comments at lines 127-134 explain that an unconditional delete-and-check can't tell "keepalived legitimately holds it" apart from "static leftover" once the config line is gone, so it deliberately only fires once. The gap: if the Ansible run dies between the lineinfile task succeeding and the "Confirm the static service address was actually withdrawn" assert completing (SSH drop, control-node timeout, serial: 1 batch abort) — the config line is already gone, so on the next run .changed is false and the entire withdraw/confirm/assert sequence is skipped. The only remaining guard is the unconditional grep-based ifupdown check (lines 86-121), which only inspects the config file and is already clean at that point — it can't see a live kernel address. The host can end up holding fd40::123 statically forever, outside VRRP control, with the play reporting success — the exact failure mode (2026-08-04) this PR exists to close, just reached via an interrupted retry instead of a first deploy. Worth a durable signal that survives past the single run where lineinfile fired (e.g. gate on whether keepalived.conf already renders the NTP_VIP6 block / whether keepalived.service is active, rather than solely on that one task's .changed).
ansible/plays/install-ntp-ha.yml:33 — serial: 1 now serializes every run of this play, not just the migration. The comment justifies it for the one-time static-address handover, but as written it also permanently slows every future chrony-ntp-server/keepalived-ntp convergence run across all 4 ntp_cluster members to one host at a time, even once no host has a static leftover left to withdraw. With only 4 hosts the cost is small, but if this is meant to be migration-only, consider scoping it (e.g. serial: 1 only needed while keepalived_ntp_static6_removed can still fire) or explicitly noting it's a permanent, accepted tradeoff.

Tests

No automated test harness covers Ansible roles/plays in this repo (no molecule, no CI syntax-check wired up for install-ntp-ha.yml), consistent with how the rest of the codebase is reviewed. The author validated the rendered keepalived.conf with keepalived -t against real edge-1 output, which is the right manual check for the template half. The address-migration logic (lineinfile regex against the real /etc/network/interfaces hook format, the Arch/networkd detection) is unverified against the actual PVE/edge-1 file contents — the PR body itself flags this ("needs checking on the other servers... I could not check"), and the extensive assert tasks are the intended safety net for exactly that uncertainty. That's a reasonable mitigation, not a gap in this review, but it does mean the first real run against jupiter/saturn/neptune is where the lineinfile regex assumption gets its first real test.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `a1e3b65cbfdc1ee6ba7178f4716ceb2ca4b06e7d`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR adds a second, IPv6 VRRP instance (`NTP_VIP6`) to the `keepalived-ntp` role so that `fd40::123` — the address `ntp.srv.renner.uno` actually resolves to — is held only while `check_ntp` passes, plus the migration logic needed to take that address away from the hand-placed static assignments on the PVE hosts (ifupdown) and edge-1 (systemd-networkd, handled by the separate edge-host role). The design reasoning is unusually well documented in-line (VRRPv3 auth removal, `-nodad`, `weight 0` vs. the v4 instance's `weight -20`, `serial: 1`), and it's consistent with what's actually in the checkout (`10-vlan40.network.j2` has no `[Address]` block, `install-edge-host.yml` does tag `vlan40` as referenced in the assert message). No blocking issues found; one robustness gap in the migration's retry path is worth a look before this is exercised against a real interruption. ## 🟡 Suggestions - **`ansible/roles/keepalived-ntp/tasks/main.yml:135-221` — the one-shot migration has no recovery path if interrupted mid-flight.** The runtime withdrawal (`ip -6 addr del`) and its two asserts are gated on `keepalived_ntp_static6_removed.changed` (only true the run the `lineinfile` task actually removes the ifupdown hook). That's necessary, not incidental — the comments at lines 127-134 explain that an *unconditional* delete-and-check can't tell "keepalived legitimately holds it" apart from "static leftover" once the config line is gone, so it deliberately only fires once. The gap: if the Ansible run dies between the `lineinfile` task succeeding and the "Confirm the static service address was actually withdrawn" assert completing (SSH drop, control-node timeout, `serial: 1` batch abort) — the config line is already gone, so on the next run `.changed` is `false` and the entire withdraw/confirm/assert sequence is skipped. The only remaining guard is the unconditional `grep`-based ifupdown check (lines 86-121), which only inspects the config file and is already clean at that point — it can't see a live kernel address. The host can end up holding `fd40::123` statically forever, outside VRRP control, with the play reporting success — the exact failure mode (2026-08-04) this PR exists to close, just reached via an interrupted retry instead of a first deploy. Worth a durable signal that survives past the single run where `lineinfile` fired (e.g. gate on whether `keepalived.conf` already renders the `NTP_VIP6` block / whether `keepalived.service` is active, rather than solely on that one task's `.changed`). - **`ansible/plays/install-ntp-ha.yml:33` — `serial: 1` now serializes every run of this play, not just the migration.** The comment justifies it for the one-time static-address handover, but as written it also permanently slows every future `chrony-ntp-server`/`keepalived-ntp` convergence run across all 4 `ntp_cluster` members to one host at a time, even once no host has a static leftover left to withdraw. With only 4 hosts the cost is small, but if this is meant to be migration-only, consider scoping it (e.g. `serial: 1` only needed while `keepalived_ntp_static6_removed` can still fire) or explicitly noting it's a permanent, accepted tradeoff. ## Tests No automated test harness covers Ansible roles/plays in this repo (no molecule, no CI syntax-check wired up for `install-ntp-ha.yml`), consistent with how the rest of the codebase is reviewed. The author validated the rendered `keepalived.conf` with `keepalived -t` against real edge-1 output, which is the right manual check for the template half. The address-migration logic (`lineinfile` regex against the real `/etc/network/interfaces` hook format, the Arch/networkd detection) is unverified against the actual PVE/edge-1 file contents — the PR body itself flags this ("needs checking on the other servers... I could not check"), and the extensive `assert` tasks are the intended safety net for exactly that uncertainty. That's a reasonable mitigation, not a gap in this review, but it does mean the first real run against jupiter/saturn/neptune is where the `lineinfile` regex assumption gets its first real test. ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

k.renner merged commit b1c06b9d43 into master

2026-08-07 11:27:31 +00:00

k.renner referenced this issue from a commit

2026-08-07 11:27:32 +00:00

Merge pull request 'feat(ntp): hold the IPv6 service address in keepalived, gated on clock health' (#469) from ntp-service-address-health-gated into master

claude-bot referenced this pull request

2026-08-07 11:29:09 +00:00

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

claude-bot referenced this issue from a commit

2026-08-07 17:07:33 +00:00

fix(ntp): link-local VRRP primary, and sequence the migration around DAD

claude-bot referenced this pull request