test(molecule): fix false passes in host-env/schema and shared staging in validation - #75
Merged
Merged
Conversation
host-env asserted only that decdn-node's PID changed across the second converge. The unit is Restart=always, so a daemon that crashed or was OOM-killed in between (likelier now that `make molecule` runs scenarios in parallel) came back with a new PID and satisfied the check even with the env-file -> restart notify broken. Read the unit through a check-mode systemd_service call and also require NRestarts == 0 before and after: systemd's automatic restarts bump it, the role's handler does not. host-env and schema never asserted the daemon bound /metrics, and the role's readiness probe only warns, so a daemon that never came up left both green. Add default's `ss` listener assertion (loopback, no wildcard) to both. Closes #51 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The binary-format and binary-check-mode cases staged their fixtures at fixed paths under MOLECULE_PROJECT_DIRECTORY and removed them in `always:`, so one run's cleanup could delete another overlapping run's pair mid-case and fail it with a misleading message. Stage them in a fresh ansible.builtin.tempfile directory on the controller instead. MOLECULE_EPHEMERAL_DIRECTORY, the fix #52 proposed, would not have helped: without --parallel molecule keys it on the project basename and scenario, so it is shared across runs and across git worktrees. Closes #52 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved blocking issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Hardens Molecule validation against false passes and parallel fixture collisions.
Changes:
- Verifies restart causes via
NRestarts. - Adds loopback metrics readiness checks.
- Uses isolated temporary directories for fixtures.
| File | Description |
|---|---|
ansible/molecule/validation/converge.yml |
Isolates binary fixtures per run with guarded cleanup. |
ansible/molecule/schema/verify.yml |
Checks metrics readiness and binding. |
ansible/molecule/host-env/verify.yml |
Validates restart cause and metrics readiness. |
ansible/molecule/host-env/side_effect.yml |
Captures clean pre-edit systemd state. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
thiras
added a commit
that referenced
this pull request
Sep 24, 2026
…ite (#76) * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the three weak spots #50's review found in the molecule suite. Running scenarios in parallel (
JOBS=6) made them more likely to bite.#51: host-env / schema could pass for the wrong reason
host-envchecked only that decdn-node's PID changed across the second converge. The unit isRestart=always, so a daemon that crashed or was OOM-killed in between also gets a new PID. Bothside_effect.ymlandverify.ymlnow read the unit through a check-modesystemd_servicecall (read-only; this also replaces thepgrepworkaround forcommand-instead-of-module). They requireNRestarts == 0before and after: systemd's automatic restarts bump that counter, and the role's handler never does.host-envandschemanow carrydefault'sss -ltncheck: bound on127.0.0.1:9090, and on no wildcard address. The role's probe only warns, so a daemon that never bound metrics used to leave both scenarios green.#52: validation's shared staging path
binary-formatandbinary-check-modenow stage their fixtures in a freshansible.builtin.tempfiledirectory on the controller. Previously they used fixed.molecule-badbin/.molecule-checkbinpaths under the project dir. The cleanup is skipped if the tempdir was never created.The fix the issue proposed,
MOLECULE_EPHEMERAL_DIRECTORY, would not have worked. Without--parallel, molecule keys that directory on the project basename (ansible) and the scenario name only, so it is shared across runs and across git worktrees.Still open: two overlapping runs of the same scenario still collide on the container name, so this does not make them safe to overlap. It only removes the controller-side path collision.
Verification
make lint-ansible,make lintand the fullmake molecule(11 scenarios, parallel) all pass.notifyandpkill -KILLed the daemon inside_effect. The old PID assertion still passed, reproducing the false pass. The new NRestarts assertion failed with "systemd auto-restarted decdn-node 1 time(s)"./etc/decdn/stub-no-metricsinschema/prepare.yml. The new listener assertion failed.validation, nothing is left underansible/.molecule-*or$TMPDIR/decdn-molecule-*.Closes #51
Closes #52
🤖 Generated with Claude Code