Repository navigation
fix(ansible): escalate in service_units.yml instead of relying on the caller (#14693) - #14694
Merged
Merged
Conversation
… caller (#14693) The self-update play failed one task on the SLM node: TASK [slm_manager : SLM | Deploy backend systemd service] fatal: FAILED! => "Destination /etc/systemd/system not writable" service_units.yml holds two tasks that both need root -- a template into /etc/systemd/system and a systemd enable with daemon_reload -- and neither declared become. That was survivable while its only caller was main.yml, which includes it from an escalating context. #14668 added a second caller, the Play 1 include in update-all-nodes.yml, where no play sets a play-level become and every task declares its own; the include did not, so the template ran unprivileged. The failure aborted Play 1 before the SLM restart, so the service never restarted, the update-all resume hook never ran, and the job wedged (#14683). It also means the unit refresh #14624 and #14668 exist to perform has never once run on the self-update path -- the drift they closed was still open. Fixed in the task file rather than at the call site: writing a systemd unit needs root whoever asks for it, and a task file whose correctness depends on the caller supplying escalation is exactly what broke. Fixing only the call site would leave the next caller to rediscover this. The guard pins all three parts -- the tasks escalate, the playbook still has no play-level become (the reason they must), and Play 1 still includes the file, so deleting the include cannot turn the other two green while restoring the drift.
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
…t tasks (#14693) Review found the fix was only half of the failure class. The two tasks now escalate, but the handlers they notify -- `reload systemd` and `restart slm backend` -- did not, so a changed unit would flush handlers that run `daemon-reload` and `systemctl restart` unprivileged in the same non-escalating play. The failure would simply have moved one step later. `restart slm backend` is the worse of the two: it carries `failed_when: false`, so an unprivileged restart does not fail the play. The service just never restarts and nothing reports it -- the same silent no-restart the wedge in #14683 turns on. All six handlers perform privileged operations, so all six now declare it. Root cause of why none of them needed to before: ansible.cfg puts `become = True` under [defaults], where ansible does not read it -- the keys belong in [privilege_escalation], which in this file carries only become_flags. So the global default has never applied and only explicit per-task escalation has ever worked. Filed as #14699 rather than corrected here: moving those keys switches every task in the tree to root at once, which needs an audit of what currently relies on running unprivileged. The guard now also covers the handlers, and walks pre_tasks/post_tasks/block rather than only `tasks` -- the previous version would have reported the include as missing if it were merely moved into a block, a false alarm shaped exactly like real removal.
This was referenced Aug 21, 2026
Open
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
1 participant
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.
Closes #14693
Thinking Path
This is the root cause of the stuck code-sync on the test deployment. #14683 explains why the job never gave up; this explains why the play failed at all.
The self-update log recap showed
failed=1on the SLM node and clean on localhost. The fatal wasDestination /etc/systemd/system not writablefromslm_manager : SLM | Deploy backend systemd service— a permission failure, not a logic one, which pointed at escalation rather than the task.service_units.ymlis a file I added in #14668 to stop the unit definition being duplicated. Its two tasks both need root and neither declaresbecome. That was invisible whilemain.ymlwas the only caller, because that caller escalates. #14668 introduced a second caller — the Play 1 include inupdate-all-nodes.yml— and I checked all five plays in that playbook: none sets a play-levelbecome, every task declares its own, including the task immediately above my include. Mine did not.I fixed the task file rather than the call site. Writing a systemd unit needs root whoever asks for it, and a file whose correctness depends on the caller supplying escalation is precisely what failed here — patching the one call site would leave the next caller to rediscover it. I also checked the other includes in that playbook for the same shape; the rest either escalate or delegate to roles whose tasks do.
What Changed
roles/slm_manager/tasks/service_units.yml—become: trueon both tasks, with a comment recording which caller exposed the gap and why the requirement belongs in the file.Plus a guard pinning all three parts of the invariant.
Verification
Each guard was mutation-checked against a targeted mutation, with the mutation target asserted present first so a missing target fails loudly rather than reporting a false pass:
Mutant B matters: without that third assertion, deleting the include would turn the other two tests green while silently restoring the stale-unit drift #14624 closed.
Live evidence for the failure itself is in #14693 — the log recap and the fatal line, taken from the deployment rather than reasoned about.
Consequence Worth Noting
Because this task has been failing since #14668 landed, the unit refresh that #14624 and #14668 exist to perform has never actually run on the self-update path. The drift those issues closed was still open in practice. This PR is what makes them real.
Model Used
Claude Opus 5