From 40ef53fa67415c6fc3c12555844f032a375f623f Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Wed, 5 Aug 2026 22:02:26 +0200 Subject: [PATCH] fix(ntp): assert the static service address is really gone before handing over 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) --- ansible/plays/install-ntp-ha.yml | 5 ++ ansible/roles/keepalived-ntp/tasks/main.yml | 95 +++++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/ansible/plays/install-ntp-ha.yml b/ansible/plays/install-ntp-ha.yml index 5f9d5781..aacceb82 100644 --- a/ansible/plays/install-ntp-ha.yml +++ b/ansible/plays/install-ntp-ha.yml @@ -110,6 +110,11 @@ # # `ip -6 addr show ... to ` prints the address when present and # nothing when absent, so emptiness is the test. + # + # Presence cannot itself distinguish a VRRP-held address from a static + # leftover -- they look identical. It is only meaningful because the + # keepalived-ntp role asserts, on both distros, that no static assignment + # survives: the play fails before reaching here if one does. - name: Check if this server holds the IPv6 service address ansible.builtin.command: argv: diff --git a/ansible/roles/keepalived-ntp/tasks/main.yml b/ansible/roles/keepalived-ntp/tasks/main.yml index 259f4a4e..e00724ea 100644 --- a/ansible/roles/keepalived-ntp/tasks/main.yml +++ b/ansible/roles/keepalived-ntp/tasks/main.yml @@ -117,6 +117,101 @@ failed_when: false notify: restart keepalived +# The migration above gets exactly one attempt -- the gate that makes it +# idempotent also means it never retries -- and the delete swallows its own +# errors. So the result has to be asserted rather than assumed. +# +# `ip addr del` matches on address AND prefix length: if a host's static +# assignment used a different mask or interface than ntp_vip6 / +# keepalived_ntp_interface, the delete is a silent no-op. The address would +# then stay outside VRRP control permanently, with the play reporting success +# and the post-deploy check happily calling it a healthy v6 master -- the +# health gating silently defeated in exactly the way this change exists to +# prevent. Fail loudly here instead. +- name: Confirm the static service address was actually withdrawn + ansible.builtin.command: + argv: + - ip + - -6 + - addr + - show + - dev + - "{{ keepalived_ntp_interface }}" + - to + - "{{ keepalived_ntp_vip6 }}" + register: keepalived_ntp_vip6_after + changed_when: false + failed_when: false + when: + - keepalived_ntp_vip6 | length > 0 + - keepalived_ntp_static6_removed.changed | default(false) + +- name: Assert the service address is no longer assigned outside keepalived + ansible.builtin.assert: + that: + - keepalived_ntp_vip6_after.stdout | default('') | length == 0 + fail_msg: >- + {{ keepalived_ntp_vip6 }} is still assigned on + {{ keepalived_ntp_interface }} after the static assignment was removed. + `ip -6 addr del` matches address AND prefix length, so this usually means + the on-host assignment differs from ntp_vip6 or uses another interface. + Leaving it would keep the address outside VRRP control, so it would not + be withdrawn when this host is unfit to serve time. Inspect with + `ip -6 addr show dev {{ keepalived_ntp_interface }}` and + `grep -rn {{ keepalived_ntp_vip6 }} /etc/network/ /etc/systemd/network/`. + success_msg: >- + {{ keepalived_ntp_vip6 }} withdrawn; keepalived now owns it. + when: + - keepalived_ntp_vip6 | length > 0 + - keepalived_ntp_static6_removed.changed | default(false) + +# On Arch (edge-1) the static assignment lives in a systemd-networkd file owned +# by the edge-host role, so this role must not rewrite it -- but it must not +# quietly proceed either. install-edge-host.yml is a separate play with no +# ordering relationship to this one, so nothing guarantees it has been re-run +# to drop the [Address] block. If it has not, edge-1 would end up holding +# fd40::123 statically AND running a VRRP instance for the same address: +# keepalived would believe it had withdrawn the address while networkd kept it +# assigned, and the host would go on answering NTP with a clock the health +# check had already rejected. +# +# That is the precise failure of 2026-08-04, on the host it happened to, so it +# fails fast rather than being left to inspection. +- name: Detect a static assignment of the service address (systemd-networkd) + ansible.builtin.command: + argv: + - grep + - -rl + - "{{ keepalived_ntp_vip6 }}" + - /etc/systemd/network/ + register: keepalived_ntp_static6_networkd + changed_when: false + failed_when: false + when: + - keepalived_ntp_vip6 | length > 0 + - ansible_facts['os_family'] == 'Archlinux' + +- name: Assert networkd is not also assigning the service address + ansible.builtin.assert: + that: + - keepalived_ntp_static6_networkd.stdout | default('') | length == 0 + fail_msg: >- + {{ keepalived_ntp_vip6 }} is still assigned statically by + systemd-networkd on this host + ({{ keepalived_ntp_static6_networkd.stdout_lines | default([]) | join(', ') }}). + Handing the address to keepalived while networkd also assigns it means + it would NOT be withdrawn when check_ntp fails -- the host would keep + serving time on an address VRRP believes it has released. + Re-apply the edge-host role first, whose template no longer carries the + [Address] block: ansible-playbook -i inventory/ plays/install-edge-host.yml + --limit {{ inventory_hostname }} --tags vlan40 + success_msg: >- + No static networkd assignment of {{ keepalived_ntp_vip6 }}; safe for + keepalived to own it. + when: + - keepalived_ntp_vip6 | length > 0 + - ansible_facts['os_family'] == 'Archlinux' + - name: Create keepalived configuration ansible.builtin.template: src: keepalived.conf.j2