From 6d734fdfa6ea429597a32fcbdf509b9e356539a9 Mon Sep 17 00:00:00 2001 From: Ant Somers Date: Thu, 24 Sep 2026 08:32:39 +0300 Subject: [PATCH 1/3] test(molecule): act on the #75 review of host-env, schema and validation host-env: - read the unit with a bare `systemd_service: name=` (a read-only `systemctl show`); the earlier claim that the module rejects a bare name was false, so `state: started` + check_mode are gone - document NRestarts correctly: a manual restart resets it, which is why "crash, then handler" passes and "crash, no handler" fails - assert Restart=always, which that split depends on: without it a crashed unit is started by the converge's `state: started` and a missing handler again shows a new PID with NRestarts 0 - split "daemon running" from "PID changed", so a dead daemon is not blamed on the restart handler - compare NRestarts as a string; `| int` turns garbage into 0 host-env and schema: tie the metrics listener to the unit's MainPID (`ss -ltnp`), and say what the check is: liveness of the daemon under the rendered unit. The stub hardcodes 127.0.0.1:9090 and ignores node.toml. validation: create the per-run tempdirs before their blocks, so a controller-side failure stops the play with its own error rather than being rescued and reported as a validation regression; sweep tempdirs older than a day that interrupted runs leaked; correct the comment on what a fixed path shared. Co-Authored-By: Claude Opus 5.5 (1M context) --- ansible/molecule/host-env/side_effect.yml | 24 +++---- ansible/molecule/host-env/verify.yml | 74 ++++++++++++++------- ansible/molecule/schema/verify.yml | 31 +++++++-- ansible/molecule/validation/converge.yml | 78 ++++++++++++++++------- 4 files changed, 141 insertions(+), 66 deletions(-) diff --git a/ansible/molecule/host-env/side_effect.yml b/ansible/molecule/host-env/side_effect.yml index 57b2854..44ce26e 100644 --- a/ansible/molecule/host-env/side_effect.yml +++ b/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 }}, diff --git a/ansible/molecule/host-env/verify.yml b/ansible/molecule/host-env/verify.yml index 010f9da..b7fd254 100644 --- a/ansible/molecule/host-env/verify.yml +++ b/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 }}" diff --git a/ansible/molecule/schema/verify.yml b/ansible/molecule/schema/verify.yml index a695ca1..047b8b2 100644 --- a/ansible/molecule/schema/verify.yml +++ b/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 }}" diff --git a/ansible/molecule/validation/converge.yml b/ansible/molecule/validation/converge.yml index d87b7a8..ee302f7 100644 --- a/ansible/molecule/validation/converge.yml +++ b/ansible/molecule/validation/converge.yml @@ -28,6 +28,9 @@ become: true vars: stub_bin: "{{ lookup('ansible.builtin.env', 'MOLECULE_PROJECT_DIRECTORY') }}/molecule/default/files/decdn-node-stub" + # The controller's temp dir, named explicitly so the binary-* cases' tempdirs + # and the sweep for leaked ones look in the same place. + _decdn_ctl_tmp: "{{ lookup('ansible.builtin.env', 'TMPDIR') | default('/tmp', true) }}" # Valid baseline so execution reaches the OPTIONAL-knob asserts (required-var # asserts all pass); each case below overrides exactly one optional knob. decdn_node_install_method: manual @@ -70,21 +73,50 @@ # format assert rejects it — the "built a macOS Mach-O and shipped it" guard. # Staged + validated on the controller (delegate_to: localhost), so it aborts # before any host mutation, like the other assert-only cases. - # The staging dir is a fresh tempdir, not a fixed path: a fixed one is shared - # by every run on the controller (MOLECULE_EPHEMERAL_DIRECTORY is too — it is - # keyed on the scenario, not the run), so one run's `always:` cleanup could - # delete another's pair mid-case. + # The staging root is a fresh tempdir, not a fixed path under the project dir, + # which every run from this checkout shares: one run's `always:` cleanup could + # delete another's pair mid-case. (MOLECULE_EPHEMERAL_DIRECTORY would not help: + # without --parallel molecule keys it on the project dir's basename and the + # scenario, so it is shared across runs and same-named worktrees too.) + # Created OUTSIDE the block so a controller-side failure here stops the play + # with its own error instead of being swallowed by `rescue:` and reported by + # the final tally as a validation regression. + # An interrupted run (Ctrl-C, CI timeout) never reaches `always:`, so its + # tempdir outlives it. Sweep ones older than a day: far longer than any run, + # so a concurrent run's live pair is never touched. + - name: Find staging roots leaked by interrupted runs + ansible.builtin.find: + paths: "{{ _decdn_ctl_tmp }}" + patterns: ["decdn-molecule-badbin.*", "decdn-molecule-checkbin.*"] + file_type: directory + age: 1d + register: _decdn_leaked_tmp + delegate_to: localhost + become: false + + - name: Remove staging roots leaked by interrupted runs + ansible.builtin.file: + path: "{{ item.path }}" + state: absent + loop: "{{ _decdn_leaked_tmp.files }}" + loop_control: + label: "{{ item.path }}" + delegate_to: localhost + become: false + + - name: Create a per-run staging root on the control machine (binary-format) + ansible.builtin.tempfile: + state: directory + path: "{{ _decdn_ctl_tmp }}" + prefix: decdn-molecule-badbin. + register: _decdn_badbin_tmp + delegate_to: localhost + become: false + - name: "Case binary-format — dir-mode resolves a non-ELF binary" vars: _decdn_badbin_dir: "{{ _decdn_badbin_tmp.path }}/target/release" block: - - name: Create a per-run staging root on the control machine - ansible.builtin.tempfile: - state: directory - prefix: decdn-molecule-badbin. - register: _decdn_badbin_tmp - delegate_to: localhost - become: false - name: Create the staging directory on the control machine ansible.builtin.file: path: "{{ _decdn_badbin_dir }}" @@ -113,13 +145,10 @@ decdn_rejected: "{{ decdn_rejected + ['binary-format'] }}" when: ansible_failed_task.name is match('^Require both binaries to be ELF') always: - # Guarded: if tempfile itself failed there is nothing to remove, and an - # undefined-variable error here would bury the real failure. - name: Remove the staged non-ELF binaries ansible.builtin.file: path: "{{ _decdn_badbin_tmp.path }}" state: absent - when: _decdn_badbin_tmp.path is defined delegate_to: localhost become: false # The role's derive-paths set_fact left decdn_node_manual_bin_src pointing at @@ -134,19 +163,21 @@ # A read-only `file -b` probe must still execute under --check. Before this # regression guard it was skipped and the following ELF assert saw stdout="". - # Per-run tempdir for the same reason as binary-format above. + # Per-run tempdir, created outside the block, for the same reasons as + # binary-format above. + - name: Create a per-run check-mode root on the control machine (binary-check-mode) + ansible.builtin.tempfile: + state: directory + path: "{{ _decdn_ctl_tmp }}" + prefix: decdn-molecule-checkbin. + register: _decdn_checkbin_tmp + delegate_to: localhost + become: false + - name: "Case binary-check-mode — dir-mode format probe runs during dry-run" vars: _decdn_check_dir: "{{ _decdn_checkbin_tmp.path }}/target/release" block: - - name: Create a per-run check-mode root on the control machine - ansible.builtin.tempfile: - state: directory - prefix: decdn-molecule-checkbin. - register: _decdn_checkbin_tmp - delegate_to: localhost - become: false - - name: Create the check-mode binary directory on the control machine ansible.builtin.file: path: "{{ _decdn_check_dir }}" @@ -189,7 +220,6 @@ ansible.builtin.file: path: "{{ _decdn_checkbin_tmp.path }}" state: absent - when: _decdn_checkbin_tmp.path is defined delegate_to: localhost become: false - name: Restore binary-source facts after the check-mode probe From 0e50e8d6420f3a0eb459e010377bbceff64aa006 Mon Sep 17 00:00:00 2001 From: Ant Somers Date: Thu, 24 Sep 2026 08:32:41 +0300 Subject: [PATCH 2/3] build(molecule): refuse to overlap two suites on one host Overlapping `make molecule` runs are not isolated: container names are fixed per scenario on the one Docker daemon (across checkouts and worktrees too), every scenario starts by destroying its container, and lifecycle/cloud-init stage files in molecule's ephemeral dir, which is keyed on scenario, not run. `molecule` and `molecule-serial` now take a host-wide flock ($XDG_RUNTIME_DIR, else /tmp) and a second run exits 75 with a message instead of corrupting the first. `flock -o` keeps the lock out of anything molecule leaves running. scripts-test covers the refusal for both targets. Co-Authored-By: Claude Opus 5.5 (1M context) --- CONTRIBUTING.md | 2 +- ansible/Makefile | 26 ++++++++++++++++++++++++-- tests/scripts-test.sh | 18 +++++++++++++++++- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0ab117b..9a19682 100644 --- a/CONTRIBUTING.md +++ b/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=`). `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=`). `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=` 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=` skips the download. | | `make lint-compose` | renders `compose/compose.yaml` with its example env and asserts its security invariants | diff --git a/ansible/Makefile b/ansible/Makefile index 8a8d6ba..24b79a5 100644 --- a/ansible/Makefile +++ b/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 build galaxy-check SHELL := /bin/bash # Install the required Galaxy collections (>= constraints in requirements.yml) @@ -141,6 +141,25 @@ 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. `flock -o` holds the lock in flock itself, not in anything +# molecule leaves running. molecule-fanout is the unlocked fan-out; call the +# locked targets, not it. +MOLECULE_LOCK ?= $(or $(XDG_RUNTIME_DIR),/tmp)/decdn-devops-molecule.lock +define molecule_locked +@command -v flock >/dev/null || { \ + echo "make $@ needs util-linux's flock on PATH"; 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)) @@ -154,6 +173,9 @@ 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: @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; }' @@ -162,7 +184,7 @@ molecule: deps # 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 test --all + $(call molecule_locked,molecule test --all) # --- Galaxy collection (decdn.node) ------------------------------------------ # Stage baseline + decdn_node + grafana_alloy into a clean collection tree and build the diff --git a/tests/scripts-test.sh b/tests/scripts-test.sh index 15edb5b..1717908 100755 --- a/tests/scripts-test.sh +++ b/tests/scripts-test.sh @@ -4,7 +4,7 @@ # lint-compose and lint-cloud-init invariants (negative cases), and — with # UPSTREAM= — the upstream-mirror generators' exit codes. # `make test-scripts` runs it; CI's `scripts` job does too. Needs make, docker -# (compose v2), jq; the cloud-init cases also need cloud-init, shellcheck and yq. +# (compose v2), jq, flock; the cloud-init cases also need cloud-init, shellcheck and yq. set -euo pipefail repo="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" @@ -35,6 +35,22 @@ pass "decommission LIMIT=h scopes playbooks/decommission.yml" mk backup | grep -q -- "playbooks/backup.yml" || fail "backup does not run playbooks/backup.yml" pass "backup runs playbooks/backup.yml fleet-wide by default" +# --- molecule suite lock: a second run must refuse, not share the containers ---------- +# Hold the lock, then start the suite for real (-o deps: no galaxy install). flock +# -n refuses at once, so no scenario runs; make reports the recipe failure as 2. +lock="$work/molecule.lock" +# The sleep itself holds the lock fd, so killing it releases the lock (a +# `flock sleep` holder would leave an orphaned sleep holding it). +( exec 9>"$lock"; flock 9; exec sleep 60 ) & holder=$! +until ! flock -n "$lock" true; do sleep 0.1; done +for target in molecule molecule-serial; do + expect 2 "$target refuses to start while another suite holds the lock" \ + make -s -C "$repo/ansible" -o deps "$target" MOLECULE_LOCK="$lock" + grep -q "another molecule suite holds $lock" "$work/out" \ + || { cat "$work/out" >&2; fail "$target lock refusal does not say why"; } +done +kill "$holder"; wait "$holder" 2>/dev/null || true + # --- release gate ---------------------------------------------------------------------- gate="$repo/scripts/check-release-version.sh" expect 2 "release gate rejects a stray argument" "$gate" v0.1.0 notes.md From 3ee7c5989a4201654e4525426e2b5eadf771fc32 Mon Sep 17 00:00:00 2001 From: Ant Somers Date: Thu, 24 Sep 2026 09:05:33 +0300 Subject: [PATCH 3/3] build(molecule): lock across users, and install deps under the lock Review of #76: - $XDG_RUNTIME_DIR is per user, so two users of one Docker daemon got different locks and could still destroy each other's fixed-name containers. The default is now /tmp/decdn-devops-molecule.lock, and it is a DIRECTORY: with fs.protected_regular (the Ubuntu/Debian default, 2 here) another user cannot open(O_CREAT) a regular file someone else created in sticky /tmp, so a file lock would fail the second user even with no suite running. flock(1) falls back to a read-only open on a directory, so 0755 is enough. - `deps` ran as a prerequisite, before the lock, so a second run could rewrite collections/ under a running suite. It is now a prerequisite of the unlocked bodies (molecule-fanout, and the new molecule-all for the serial target), which only run under the lock. scripts-test now holds a directory lock like the default, drops `-o deps`, and fails if deps runs while another suite holds the lock (it does against the previous Makefile). Co-Authored-By: Claude Opus 5.5 (1M context) --- ansible/Makefile | 31 ++++++++++++++++++++++--------- tests/scripts-test.sh | 14 +++++++++----- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/ansible/Makefile b/ansible/Makefile index 24b79a5..f8b1ee6 100644 --- a/ansible/Makefile +++ b/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 molecule-fanout 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) @@ -148,13 +148,23 @@ decommission: # 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. `flock -o` holds the lock in flock itself, not in anything -# molecule leaves running. molecule-fanout is the unlocked fan-out; call the -# locked targets, not it. -MOLECULE_LOCK ?= $(or $(XDG_RUNTIME_DIR),/tmp)/decdn-devops-molecule.lock +# 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; } @@ -164,7 +174,7 @@ 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; } @@ -175,7 +185,7 @@ molecule: deps 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: +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; }' @@ -183,8 +193,11 @@ molecule-fanout: # 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 - $(call molecule_locked,molecule test --all) +molecule-serial: + $(call molecule_locked,$(MAKE) --no-print-directory molecule-all) + +molecule-all: deps + molecule test --all # --- Galaxy collection (decdn.node) ------------------------------------------ # Stage baseline + decdn_node + grafana_alloy into a clean collection tree and build the diff --git a/tests/scripts-test.sh b/tests/scripts-test.sh index 1717908..47c0533 100755 --- a/tests/scripts-test.sh +++ b/tests/scripts-test.sh @@ -36,18 +36,22 @@ mk backup | grep -q -- "playbooks/backup.yml" || fail "backup does not run playb pass "backup runs playbooks/backup.yml fleet-wide by default" # --- molecule suite lock: a second run must refuse, not share the containers ---------- -# Hold the lock, then start the suite for real (-o deps: no galaxy install). flock -# -n refuses at once, so no scenario runs; make reports the recipe failure as 2. +# Hold the lock, then start the suite for real. flock -n refuses at once, so no +# scenario runs and — since deps sits behind the lock — no galaxy install either; +# make reports the recipe failure as 2. A directory lock, like the default. lock="$work/molecule.lock" +mkdir "$lock" # The sleep itself holds the lock fd, so killing it releases the lock (a -# `flock sleep` holder would leave an orphaned sleep holding it). -( exec 9>"$lock"; flock 9; exec sleep 60 ) & holder=$! +# `flock sleep` holder would leave an orphaned sleep holding it). +( exec 9<"$lock"; flock 9; exec sleep 60 ) & holder=$! until ! flock -n "$lock" true; do sleep 0.1; done for target in molecule molecule-serial; do expect 2 "$target refuses to start while another suite holds the lock" \ - make -s -C "$repo/ansible" -o deps "$target" MOLECULE_LOCK="$lock" + make -C "$repo/ansible" "$target" MOLECULE_LOCK="$lock" grep -q "another molecule suite holds $lock" "$work/out" \ || { cat "$work/out" >&2; fail "$target lock refusal does not say why"; } + ! grep -q "ansible-galaxy" "$work/out" \ + || { cat "$work/out" >&2; fail "$target ran deps outside the lock"; } done kill "$holder"; wait "$holder" 2>/dev/null || true