test(molecule): #75 review follow-ups, and a host-wide lock on the suite

CONTRIBUTING.md

@@ -29,7 +29,7 @@ targets, so a local pass means a CI pass. Deploy targets live in
 |--------|--------------|
 | `make lint` | every pre-commit hook on every file (CI job `pre-commit`) |
 | `make lint-ansible` | install Galaxy collections + `ansible-lint` (production profile, which includes the Ansible security rules) |
-| `make molecule` | every `ansible/molecule/*/` scenario in parallel, in privileged systemd containers (needs Docker; cap with `JOBS=<n>`). `make molecule-serial` runs them one at a time for readable failures. |
+| `make molecule` | every `ansible/molecule/*/` scenario in parallel, in privileged systemd containers (needs Docker and util-linux `flock`; cap with `JOBS=<n>`). `make molecule-serial` runs them one at a time for readable failures. Both take a host-wide lock, so a second suite refuses to start (overlapping runs share container names). |
 | `make lint-helm` | chart: `helm lint --strict`, positive/negative render tests, kubeconform (digest-pinned image), the shared schema-key check (needs `helm`, `yq`, `python3` ≥ 3.11, Docker). Set `DECDN_CLI=<path to decdn>` to also run the real `decdn config validate` (CI can't). |
 | `make lint-alloy` | renders `roles/grafana_alloy`'s templates and validates them with the **real** digest-pinned Alloy binary. The molecule stub exits 0 for everything, so this is the only gate that proves the config loads. `ALLOY_BIN=<path>` skips the download. |
 | `make lint-compose` | renders `compose/compose.yaml` with its example env and asserts its security invariants |

ansible/Makefile

@@ -1,6 +1,6 @@
 # Convenience targets for the deCDN Ansible project.
 # Always run from the ansible/ directory.
-.PHONY: deps lint check deploy backup decommission molecule molecule-serial build galaxy-check
+.PHONY: deps lint check deploy backup decommission molecule molecule-serial molecule-fanout molecule-all build galaxy-check
 SHELL := /bin/bash
 
 # Install the required Galaxy collections (>= constraints in requirements.yml)
@@ -141,11 +141,40 @@ decommission:
 #     that one stops launching queued scenarios, so a single unlucky exit code would
 #     otherwise cut the suite short. So: 123 when a scenario failed, 125 if one was
 #     killed by a signal, and every scenario always gets to run.
+#
+# One suite at a time per host. Overlapping runs are not isolated from each other:
+# container names are fixed per scenario on the one Docker daemon (so this covers
+# other checkouts and worktrees too), each scenario starts by destroying its
+# container, and lifecycle/cloud-init stage files in molecule's ephemeral dir,
+# which is keyed on scenario, not run. So `molecule` and `molecule-serial` take a
+# host-wide lock and a second run refuses to start (exit 75) instead of
+# corrupting the first. `deps` runs under the lock too: it rewrites collections/,
+# which a running suite reads. `flock -o` holds the lock in flock itself, not in
+# anything molecule leaves running. molecule-fanout and molecule-all are the
+# unlocked bodies; call the locked targets, not them.
+#
+# The lock is a DIRECTORY in /tmp, shared by every user of the host (users of one
+# Docker daemon collide just the same). Not $XDG_RUNTIME_DIR, which is per user,
+# and not a regular file: with fs.protected_regular (Ubuntu/Debian default) another
+# user cannot open(O_CREAT) a file someone else created in sticky /tmp, so the
+# second user would fail even when no suite is running. Directories are exempt,
+# and flock(1) falls back to a read-only open on one, so 0755 is enough.
+MOLECULE_LOCK ?= /tmp/decdn-devops-molecule.lock
+define molecule_locked
+@command -v flock >/dev/null || { \
+  echo "make $@ needs util-linux's flock on PATH"; exit 1; }
+@test -e '$(MOLECULE_LOCK)' || mkdir -m 0755 '$(MOLECULE_LOCK)' 2>/dev/null || test -e '$(MOLECULE_LOCK)' || { \
+  echo "cannot create the molecule lock directory $(MOLECULE_LOCK)"; exit 1; }
+@flock -n -o -E 75 '$(MOLECULE_LOCK)' $(1) || { rc=$$?; \
+  test $$rc -ne 75 || echo "another molecule suite holds $(MOLECULE_LOCK) — wait for it to finish (overlapping runs share containers)"; \
+  exit $$rc; }
+endef
+
 SCENARIOS := $(notdir $(patsubst %/,%,$(dir $(wildcard molecule/*/molecule.yml))))
 SCENARIO_DIRS := $(notdir $(patsubst %/,%,$(wildcard molecule/*/)))
 JOBS ?= $(words $(SCENARIOS))
 
-molecule: deps
+molecule:
 	@test -n "$(strip $(SCENARIOS))" && test "$(strip $(SCENARIOS))" = "$(strip $(SCENARIO_DIRS))" || { \
 	  echo "scenario discovery failed: molecule.yml in [$(SCENARIOS)] but scenario dirs are [$(SCENARIO_DIRS)]"; \
 	  echo "refusing to run a silently-truncated suite (this target must run from ansible/)"; exit 1; }
@@ -154,14 +183,20 @@ molecule: deps
 	@test "$$((10#$(JOBS)))" -gt 0 || { \
 	  echo "JOBS must be a positive integer (got '$(JOBS)'): any all-zero value — 0, 00, 000 —"; \
 	  echo "reaches xargs as -P 0, which means UNLIMITED concurrency, not none"; exit 1; }
+	$(call molecule_locked,$(MAKE) --no-print-directory molecule-fanout JOBS='$(JOBS)')
+
+molecule-fanout: deps
 	@printf '%s\n' $(SCENARIOS) | xargs -P $(JOBS) -I{} \
 		bash -c 'set -euo pipefail; molecule test -s {} --no-command-borders 2>&1 | sed -u "s/^/[{}] /" \
 		  || { echo "[{}] SCENARIO FAILED"; exit 1; }'
 
 # The same suite, one scenario at a time (molecule's own --all). It stops at the FIRST
 # failing scenario (--continue-on-failure defaults off), so it reports less than the
 # parallel target — but readably, which is the point.
-molecule-serial: deps
+molecule-serial:
+	$(call molecule_locked,$(MAKE) --no-print-directory molecule-all)
+
+molecule-all: deps
 	molecule test --all
 
 # --- Galaxy collection (decdn.node) ------------------------------------------

ansible/molecule/host-env/side_effect.yml

@@ -1,8 +1,8 @@
 ---
 # Edit the host-provisioned env file OUT OF BAND, exactly as an operator rotating
 # an RPC key would, and record the daemon's current MainPID. molecule.yml runs a
-# SECOND converge after this, and verify.yml then asserts the PID changed without
-# systemd having auto-restarted the unit.
+# SECOND converge after this, and verify.yml then asserts the PID changed and that
+# systemd did not auto-restart the unit after the role's restart.
 #
 # This is the only test of the feature's headline behaviour. Every other assertion
 # about /etc/decdn/.decdn.env.sha256 checks a precondition (it exists, it is 0600
@@ -15,26 +15,26 @@
   vars:
     decdn_etc: /etc/decdn
   tasks:
-    # A read-only `systemctl show`: the module fills `status` before it acts, and
-    # check_mode stops it acting. `state` is only there because the module rejects
-    # a bare `name` — it never starts anything here. (A `systemctl` command would
-    # trip ansible-lint's command-instead-of-module.)
+    # With only `name`, systemd_service is a read-only `systemctl show`: it fills
+    # `status` and changes nothing. (A `systemctl show` command would trip
+    # ansible-lint's command-instead-of-module.)
     - name: Read the daemon's unit state before the edit
       ansible.builtin.systemd_service:
         name: decdn-node
-        state: started
-      check_mode: true
       register: decdn_unit_before
 
-    # NRestarts counts only restarts systemd made under Restart=always, never the
-    # role's handler. It must start at 0 so verify.yml can require it to still be 0:
-    # that is what tells a handler restart apart from a crash that systemd revived.
+    # NRestarts counts systemd's automatic (Restart=) restarts since the unit was
+    # last started or restarted by hand; a manual restart resets it to 0. Nonzero
+    # here means the daemon is already crash-looping, which would make the PID
+    # comparison in verify.yml meaningless, so stop now with a clear message.
+    # Compared as strings: systemctl prints a number, and `| int` would turn
+    # anything else into a passing 0.
     - name: Assert the daemon was running cleanly before the edit
       ansible.builtin.assert:
         that:
           - decdn_unit_before.status.ActiveState == 'active'
           - decdn_unit_before.status.MainPID | int > 0
-          - decdn_unit_before.status.NRestarts | int == 0
+          - decdn_unit_before.status.NRestarts == '0'
         fail_msg: >-
           Expected decdn-node active, with a MainPID and no automatic restarts,
           before the out-of-band edit (got ActiveState={{ decdn_unit_before.status.ActiveState }},

ansible/molecule/host-env/verify.yml

@@ -139,46 +139,66 @@
     # is what makes this behavioural — the checksum assertions above all remain
     # true even if the restart signal is deleted outright.
     #
-    # A changed PID alone is only the EFFECT. The unit is Restart=always, so a
-    # daemon that crashed (or was OOM-killed under parallel molecule load) between
-    # side_effect and the re-converge also comes back with a new PID. NRestarts
-    # pins the CAUSE: it counts only systemd's automatic restarts, so a new PID
-    # with NRestarts still 0 can only have come from the role's handler.
+    # A changed PID alone is only the EFFECT. A daemon that crashed (or was
+    # OOM-killed under parallel molecule load) between side_effect and the
+    # re-converge also comes back with a new PID, from systemd's Restart=always.
+    # NRestarts pins the CAUSE. It counts systemd's automatic restarts since the
+    # unit was last started or restarted by hand, and a manual restart resets it:
+    #   - handler fired (crash or not before it): new PID, NRestarts 0  -> pass
+    #   - handler missing, crash revived by systemd: new PID, NRestarts >= 1 -> fail
+    # That split only holds while the unit is Restart=always. Without it a crashed
+    # unit sits `failed`, the converge's own `state: started` starts it by hand,
+    # and a missing handler would again show a new PID with NRestarts 0 — so
+    # Restart= is asserted too.
     - name: Read the pre-edit MainPID recorded by side_effect
       ansible.builtin.slurp:
         src: "{{ decdn_etc }}/molecule-pid-before"
       register: decdn_pid_before
 
-    # Read-only `systemctl show` via check_mode — see the note in side_effect.yml.
+    # Read-only `systemctl show` — see the note in side_effect.yml.
     - name: Read the daemon's unit state after the re-converge
       ansible.builtin.systemd_service:
         name: decdn-node
-        state: started
-      check_mode: true
       register: decdn_unit_after
 
-    - name: Assert the out-of-band edit restarted decdn-node
+    # Separate from the PID comparison so a dead daemon (MainPID 0, e.g. waiting
+    # out RestartSec) is reported as dead, not blamed on the restart handler.
+    - name: Assert decdn-node is running after the re-converge
       ansible.builtin.assert:
         that:
+          - decdn_unit_after.status.ActiveState == 'active'
           - decdn_unit_after.status.MainPID | int > 0
-          - (decdn_unit_after.status.MainPID | string) != (decdn_pid_before.content | b64decode | trim)
+        fail_msg: >-
+          decdn-node is not running after the re-converge
+          (ActiveState={{ decdn_unit_after.status.ActiveState }},
+          MainPID={{ decdn_unit_after.status.MainPID }}) — check:
+          journalctl -u decdn-node -e
+
+    - name: Assert the out-of-band edit restarted decdn-node
+      ansible.builtin.assert:
+        that:
+          - decdn_unit_after.status.MainPID != (decdn_pid_before.content | b64decode | trim)
         fail_msg: >-
           decdn-node still runs as PID {{ decdn_unit_after.status.MainPID }} after
           {{ decdn_etc }}/decdn.env was edited out of band and the role re-converged
           — the daemon is on the PRE-rotation secret behind a green deploy. The
           env-file checksum record did not drive the restart handler.
 
+    # String compare for the same reason as in side_effect.yml.
     - name: Assert the restart came from the role, not from systemd
       ansible.builtin.assert:
         that:
-          - decdn_unit_after.status.NRestarts | int == 0
+          - decdn_unit_after.status.Restart == 'always'
+          - decdn_unit_after.status.NRestarts == '0'
         fail_msg: >-
-          systemd auto-restarted decdn-node {{ decdn_unit_after.status.NRestarts }}
-          time(s) (Restart=always) — the daemon died on its own at some point
-          between side_effect and verify, so the PID change above cannot prove the
-          role's handler restarted it. Find out why it died
-          (journalctl -u decdn-node -e; an OOM kill under `make molecule` load shows
-          in dmesg) rather than relaxing this check.
+          Restart={{ decdn_unit_after.status.Restart }},
+          NRestarts={{ decdn_unit_after.status.NRestarts }}. Either the unit is no
+          longer Restart=always, which makes NRestarts unable to tell a handler
+          restart from a crash (see the comment above), or systemd auto-restarted
+          decdn-node after the last manual (re)start — the daemon died on its own,
+          so the PID change above cannot prove the role's handler restarted it.
+          Find out why it died (journalctl -u decdn-node -e; an OOM kill under
+          `make molecule` load shows in dmesg) rather than relaxing this check.
 
     # Without EnvironmentFile the daemon never receives the host-provisioned
     # secret at all, and every other assertion in this scenario would still pass.
@@ -208,22 +228,30 @@
             and ansible_facts.services['decdn-node.service'].state == 'running'
         fail_msg: "decdn-node is not running — check: journalctl -u decdn-node -e"
 
+    # -p names the owning process, so the listener can be tied to the unit's
+    # current MainPID rather than to whatever holds the port.
     - name: Collect listening TCP sockets
       ansible.builtin.command:
-        cmd: ss -ltn
+        cmd: ss -ltnp
       register: listeners
       changed_when: false
 
     # The role's /metrics readiness probe only WARNS on timeout, so without this a
-    # daemon that never binds metrics would still leave this scenario green.
-    # Same check as ../default/verify.yml: bound on loopback, and on no wildcard.
-    - name: Assert metrics are up and bound to loopback only
+    # daemon that never binds metrics would still leave this scenario green. This
+    # is a liveness check on the restarted daemon: the stub hardcodes
+    # 127.0.0.1:9090 and ignores node.toml, so the wildcard lines cannot catch a
+    # bad rendered metrics_bind here (../default/verify.yml checks that value).
+    - name: Assert metrics are served by the current decdn-node, on loopback only
       ansible.builtin.assert:
         that:
-          - "'127.0.0.1:9090' in listeners.stdout"
+          - metrics_line | length == 1
+          - ('pid=' ~ decdn_unit_after.status.MainPID ~ ',') in metrics_line[0]
           - "'0.0.0.0:9090' not in listeners.stdout"
           - "'[::]:9090' not in listeners.stdout"
           - "'*:9090' not in listeners.stdout"
         fail_msg: >-
-          metrics are not up on 127.0.0.1:9090 only (check: journalctl -u decdn-node -e):
+          metrics are not served on 127.0.0.1:9090 (only) by decdn-node's MainPID
+          {{ decdn_unit_after.status.MainPID }} (check: journalctl -u decdn-node -e):
           {{ listeners.stdout }}
+      vars:
+        metrics_line: "{{ listeners.stdout_lines | select('search', '127\\.0\\.0\\.1:9090 ') | list }}"

ansible/molecule/schema/verify.yml

@@ -142,23 +142,40 @@
           echo "breadth OK: $keys keys, $(echo "$tables" | grep -c '') tables"
       changed_when: false
 
+    # With only `name`, systemd_service is a read-only `systemctl show` (a
+    # `systemctl show` command would trip ansible-lint's command-instead-of-module).
+    - name: Read the daemon's unit state
+      ansible.builtin.systemd_service:
+        name: decdn-node
+      register: decdn_unit
+
+    # -p names the owning process, so the listener can be tied to the unit's
+    # current MainPID rather than to whatever holds the port.
     - name: Collect listening TCP sockets
       ansible.builtin.command:
-        cmd: ss -ltn
+        cmd: ss -ltnp
       register: listeners
       changed_when: false
 
     # The role's /metrics readiness probe only WARNS on timeout, so without this a
-    # daemon that never binds metrics would still leave this scenario green.
-    # Same check as ../default/verify.yml: bound on loopback, and on no wildcard.
-    # It checks the daemon the LAST converge play (resolve-only) left running.
-    - name: Assert metrics are up and bound to loopback only
+    # daemon that never binds metrics would still leave this scenario green. This
+    # is a liveness check on the daemon the LAST converge play (resolve-only) left
+    # running under the rendered unit. The stub hardcodes 127.0.0.1:9090 and
+    # ignores node.toml, so it says nothing about the maximal config, and the
+    # wildcard lines cannot catch a bad rendered metrics_bind here
+    # (../default/verify.yml checks that value).
+    - name: Assert metrics are served by the current decdn-node, on loopback only
       ansible.builtin.assert:
         that:
-          - "'127.0.0.1:9090' in listeners.stdout"
+          - decdn_unit.status.MainPID | int > 0
+          - metrics_line | length == 1
+          - ('pid=' ~ decdn_unit.status.MainPID ~ ',') in metrics_line[0]
           - "'0.0.0.0:9090' not in listeners.stdout"
           - "'[::]:9090' not in listeners.stdout"
           - "'*:9090' not in listeners.stdout"
         fail_msg: >-
-          metrics are not up on 127.0.0.1:9090 only (check: journalctl -u decdn-node -e):
+          metrics are not served on 127.0.0.1:9090 (only) by decdn-node's MainPID
+          {{ decdn_unit.status.MainPID }} (check: journalctl -u decdn-node -e):
           {{ listeners.stdout }}
+      vars:
+        metrics_line: "{{ listeners.stdout_lines | select('search', '127\\.0\\.0\\.1:9090 ') | list }}"