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

2026-08-07T17:08:01Z

claude-bot commented

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

Follow-up to #469. Applying it to edge-1 exposed two defects that only show up on real hardware — neither was visible from the diff or from keepalived -t.

1. The VRRP address was unusable

keepalived took MASTER and assigned fd40::123, but the kernel left it:

inet6 fd40::123/128 scope global dadfailed deprecated tentative proto keepalived
       valid_lft forever preferred_lft 0sec

-nodad did not take effect — no nodad flag on the address, and net.ipv6.conf.enp1s0/40.accept_dad = 1 — so DAD ran and found a genuine duplicate: jupiter, saturn and neptune still hold the address statically.

edge-1 believed it was the v6 master while serving nothing. Compare jupiter, which is clean: scope global nodad, preferred_lft forever.

Dropping the flag rather than fixing it

-nodad was inherited from the previous design, where every NTP server carried the address simultaneously and duplicate detection had to be suppressed for that to work at all. Under single-master VRRP the opposite holds: exactly one host should have it, so a duplicate is a real fault and detecting it is the point.

What that costs

The migration can no longer be done host-by-host. A static copy anywhere and a VRRP copy elsewhere cannot coexist, and dadfailed is sticky — the kernel does not retry, so it would not self-heal.

install-ntp-ha.yml is therefore split into two plays:

Withdraw every static assignment across the whole group. Deliberately not serial, since it must finish everywhere before any host claims the address.
Install chrony + hand the address to keepalived (serial: 1 as before).

There is a gap between the phases where nobody holds the address, bounded by one play's run time; clients retry. That is the price of correct duplicate detection, and it is preferable to the dadfailed state, which does not resolve on its own.

The withdrawal tasks move to roles/keepalived-ntp/tasks/withdraw-static.yml for the first play to include. main.yml keeps the detect-and-assert guards, which still run on every execution and fail loudly if any static assignment survives.

2. keepalived warned about the VIP form

(NTP_VIP6) the first IPv6 VIP address should be link local

RFC 5798 gives an IPv6 virtual router a link-local primary. keepalived carried on regardless, but the instance was sourcing VRRPv3 from something the protocol does not expect. fe80::123 is now the first virtual_ipaddress — it mirrors the service address and cannot collide with a NIC's EUI-64 link-local.

Also

ntp_vip6 and ntp_vip6_linklocal move to group_vars/ntp_cluster.yml, so both plays read one definition instead of duplicating a service address across them.

Verification

keepalived -t on edge-1 (keepalived 2.4.1) parses the two-address form clean
ansible-playbook --syntax-check passes on the restructured play
the dadfailed state above is observed, not theoretical — it is the current state of edge-1

Current state of the fleet

edge-1 — makestep applied and live; static [Address] removed; holds a dadfailed VRRP address, i.e. serving nothing on v6
jupiter / saturn / neptune — unchanged, still holding fd40::123 statically and serving normally

So internal NTP is healthy, on the three PVE nodes. This PR is what makes it safe to finish the migration.

🤖 Generated with Claude Code

Follow-up to #469. Applying it to edge-1 exposed two defects that only show up on real hardware — neither was visible from the diff or from `keepalived -t`. ## 1. The VRRP address was unusable keepalived took MASTER and assigned `fd40::123`, but the kernel left it: ``` inet6 fd40::123/128 scope global dadfailed deprecated tentative proto keepalived valid_lft forever preferred_lft 0sec ``` `-nodad` **did not take effect** — no `nodad` flag on the address, and `net.ipv6.conf.enp1s0/40.accept_dad = 1` — so DAD ran and found a genuine duplicate: jupiter, saturn and neptune still hold the address statically. **edge-1 believed it was the v6 master while serving nothing.** Compare jupiter, which is clean: `scope global nodad`, `preferred_lft forever`. ### Dropping the flag rather than fixing it `-nodad` was inherited from the previous design, where every NTP server carried the address simultaneously and duplicate detection *had* to be suppressed for that to work at all. Under single-master VRRP the opposite holds: exactly one host should have it, so a duplicate is a real fault and detecting it is the point. ### What that costs The migration can no longer be done host-by-host. A static copy anywhere and a VRRP copy elsewhere cannot coexist, and `dadfailed` is **sticky** — the kernel does not retry, so it would not self-heal. `install-ntp-ha.yml` is therefore split into two plays: 1. **Withdraw** every static assignment across the whole group. Deliberately **not** `serial`, since it must finish everywhere before any host claims the address. 2. **Install** chrony + hand the address to keepalived (`serial: 1` as before). There is a gap between the phases where nobody holds the address, bounded by one play's run time; clients retry. That is the price of correct duplicate detection, and it is preferable to the `dadfailed` state, which does not resolve on its own. The withdrawal tasks move to `roles/keepalived-ntp/tasks/withdraw-static.yml` for the first play to include. `main.yml` keeps the detect-and-assert guards, which still run on every execution and fail loudly if any static assignment survives. ## 2. keepalived warned about the VIP form ``` (NTP_VIP6) the first IPv6 VIP address should be link local ``` RFC 5798 gives an IPv6 virtual router a link-local primary. keepalived carried on regardless, but the instance was sourcing VRRPv3 from something the protocol does not expect. `fe80::123` is now the first `virtual_ipaddress` — it mirrors the service address and cannot collide with a NIC's EUI-64 link-local. ## Also `ntp_vip6` and `ntp_vip6_linklocal` move to `group_vars/ntp_cluster.yml`, so both plays read one definition instead of duplicating a service address across them. ## Verification - `keepalived -t` on edge-1 (keepalived 2.4.1) parses the two-address form clean - `ansible-playbook --syntax-check` passes on the restructured play - the `dadfailed` state above is observed, not theoretical — it is the current state of edge-1 ## Current state of the fleet - **edge-1** — `makestep` applied and live; static `[Address]` removed; holds a `dadfailed` VRRP address, i.e. serving nothing on v6 - **jupiter / saturn / neptune** — unchanged, still holding `fd40::123` statically and serving normally So internal NTP is healthy, on the three PVE nodes. This PR is what makes it safe to finish the migration. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-08-07 17:08:02 +00:00

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

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

Details

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

Details

ea439b110d

Applying #469 to edge-1 exposed two defects that only show up on real
hardware.

1. The VRRP address was unusable.

   keepalived took MASTER and assigned fd40::123, but the kernel left it:

     inet6 fd40::123/128 scope global dadfailed deprecated tentative
           proto keepalived
           valid_lft forever preferred_lft 0sec

   `-nodad` did not take effect -- no nodad flag on the address, and
   net.ipv6.conf.enp1s0/40.accept_dad = 1 -- so DAD ran and found a real
   duplicate: jupiter, saturn and neptune still held the address statically.
   edge-1 believed it was the v6 master while serving nothing.

   The flag is dropped rather than fixed. It was inherited from the previous
   design, where every NTP server carried the address at once and duplicate
   detection had to be suppressed for that to work. Under single-master VRRP
   the opposite holds: exactly one host should have it, so a duplicate is a
   genuine fault and detecting it is the point.

   What that costs is that the migration can no longer be done host by host.
   A static copy anywhere and a VRRP copy elsewhere cannot coexist, and the
   dadfailed state is sticky -- the kernel does not retry. install-ntp-ha.yml
   is therefore split in two: a first play withdraws every static assignment
   across the whole group, then the second hands the address to keepalived.
   Deliberately not serial, since it must finish everywhere before any host
   claims the address. There is a gap between the phases where nobody holds
   it, bounded by one play's run time; clients retry.

   The withdrawal tasks move to roles/keepalived-ntp/tasks/withdraw-static.yml
   for the first play to include. main.yml keeps the detect-and-assert guards,
   which still run every time and fail loudly if any static assignment
   survives.

2. keepalived warned:

     (NTP_VIP6) the first IPv6 VIP address should be link local

   RFC 5798 gives an IPv6 virtual router a link-local primary. keepalived
   carried on regardless, but the instance was sourcing VRRPv3 from something
   the protocol does not expect. fe80::123 is now the first
   virtual_ipaddress: it mirrors the service address and cannot collide with
   a NIC's EUI-64 link-local.

ntp_vip6 and ntp_vip6_linklocal move to group_vars/ntp_cluster.yml so both
plays read one definition rather than duplicating a service address.

Verified: `keepalived -t` on edge-1 (keepalived 2.4.1) parses the two-address
form clean, and the playbook passes --syntax-check.

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

review-bot reviewed

2026-08-07 17:12:42 +00:00

review-bot left a comment

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

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

Summary

Follow-up to #469, fixing two real defects found on hardware: (1) -nodad wasn't actually suppressing DAD on edge-1 (accept_dad=1 on that interface), so the fix flips the design to enabled DAD + a fleet-wide "withdraw all static assignments before anyone claims the address via VRRP" phase, and (2) adds a link-local address as the first virtual_ipaddress entry to satisfy RFC 5798 / silence keepalived's warning. I traced the play restructuring, the new withdraw-static.yml task file, the group_vars/ntp_cluster.yml variable move, and the template change against the rest of the role (main.yml, defaults/main.yml, inventory) and didn't find a defect — the logic is consistent and the gating (ntp_keepalived_enabled, the .changed-gated withdrawal, the Debian/Arch split) lines up correctly end-to-end. No blocking issues.

🟡 Suggestions

ansible/roles/keepalived-ntp/defaults/main.yml:29 and ansible/inventory/group_vars/ntp_cluster.yml:24 both hardcode fe80::123/64. install-ntp-ha.yml always passes the group_vars value explicitly (keepalived_ntp_vip6_linklocal: "{{ ntp_vip6_linklocal }}"), so the role default is currently dead in this playbook and only matters for standalone use of the role — but that's exactly the drift risk the PR itself calls out for ntp_vip6 ("Duplicating a service address ... is exactly the sort of thing that drifts", ansible/inventory/group_vars/ntp_cluster.yml:15). Worth a comment noting the two must stay in sync, or defaulting the role value to "" the way keepalived_ntp_vip6 does, forcing callers to set it explicitly.
The runtime delete task in the new withdraw-static.yml (ansible/roles/keepalived-ntp/tasks/withdraw-static.yml:373-387) dropped the notify: restart keepalived that the equivalent task had in old main.yml. That's almost certainly correct — on a first-time migration keepalived isn't installed yet when phase 1 runs, so notifying a restart there could error against a nonexistent unit — but since it's a behavior change from the code being moved, it'd be worth a one-line comment saying so explicitly (the rest of this diff is very good about explaining why not just what).

Tests

No automated tests exist for this Ansible tree (no molecule setup, and .woodpecker/deploy-infrastructure.yml has no ansible syntax-check gate for PRs). That matches the rest of the repo, not a gap introduced by this PR. The author verified with ansible-playbook --syntax-check and keepalived -t per the PR description, and I independently checked all touched variable references (ntp_vip6, ntp_vip6_linklocal, ntp_keepalived_enabled, ntp_keepalived_interface) resolve correctly across group_vars/ntp_cluster.yml, group_vars/pve_cluster.yml, group_vars/edge_host/vars.yml, and the inventory's [ntp_cluster:children] group — no undefined-variable or precedence issues found.

Recommendation

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

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `ea439b110d879350544063516ee4c9565a9a6053`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Follow-up to #469, fixing two real defects found on hardware: (1) `-nodad` wasn't actually suppressing DAD on edge-1 (`accept_dad=1` on that interface), so the fix flips the design to enabled DAD + a fleet-wide "withdraw all static assignments before anyone claims the address via VRRP" phase, and (2) adds a link-local address as the first `virtual_ipaddress` entry to satisfy RFC 5798 / silence keepalived's warning. I traced the play restructuring, the new `withdraw-static.yml` task file, the `group_vars/ntp_cluster.yml` variable move, and the template change against the rest of the role (`main.yml`, `defaults/main.yml`, inventory) and didn't find a defect — the logic is consistent and the gating (`ntp_keepalived_enabled`, the `.changed`-gated withdrawal, the Debian/Arch split) lines up correctly end-to-end. No blocking issues. ## 🟡 Suggestions - `ansible/roles/keepalived-ntp/defaults/main.yml:29` and `ansible/inventory/group_vars/ntp_cluster.yml:24` both hardcode `fe80::123/64`. `install-ntp-ha.yml` always passes the group_vars value explicitly (`keepalived_ntp_vip6_linklocal: "{{ ntp_vip6_linklocal }}"`), so the role default is currently dead in this playbook and only matters for standalone use of the role — but that's exactly the drift risk the PR itself calls out for `ntp_vip6` ("Duplicating a service address ... is exactly the sort of thing that drifts", `ansible/inventory/group_vars/ntp_cluster.yml:15`). Worth a comment noting the two must stay in sync, or defaulting the role value to `""` the way `keepalived_ntp_vip6` does, forcing callers to set it explicitly. - The runtime delete task in the new `withdraw-static.yml` (`ansible/roles/keepalived-ntp/tasks/withdraw-static.yml:373-387`) dropped the `notify: restart keepalived` that the equivalent task had in old `main.yml`. That's almost certainly correct — on a first-time migration keepalived isn't installed yet when phase 1 runs, so notifying a restart there could error against a nonexistent unit — but since it's a behavior change from the code being moved, it'd be worth a one-line comment saying so explicitly (the rest of this diff is very good about explaining *why* not just *what*). ## Tests No automated tests exist for this Ansible tree (no molecule setup, and `.woodpecker/deploy-infrastructure.yml` has no ansible syntax-check gate for PRs). That matches the rest of the repo, not a gap introduced by this PR. The author verified with `ansible-playbook --syntax-check` and `keepalived -t` per the PR description, and I independently checked all touched variable references (`ntp_vip6`, `ntp_vip6_linklocal`, `ntp_keepalived_enabled`, `ntp_keepalived_interface`) resolve correctly across `group_vars/ntp_cluster.yml`, `group_vars/pve_cluster.yml`, `group_vars/edge_host/vars.yml`, and the inventory's `[ntp_cluster:children]` group — no undefined-variable or precedence issues found. ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

k.renner merged commit 6628f6eaef into master

2026-08-07 17:18:12 +00:00

k.renner referenced this issue from a commit

2026-08-07 17:18:12 +00:00

Merge pull request 'fix(ntp): link-local VRRP primary, and sequence the migration around DAD' (#470) from ntp-vrrp-linklocal-and-dad-sequencing into master

claude-bot referenced this pull request

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

fix(chrony): allow IPv6 clients -- the allow list was IPv4-only #471

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#470