fix(roles): daemon-reload inside the restart handlers - #79
Merged
Merged
Conversation
"Install decdn-node systemd unit" notifies "Reload systemd" and then "Restart decdn-node", relying on the reload running first. On site.yml it does not: baseline loads devsec.hardening.os_hardening with include_role, which appends that role's own "Reload systemd" handler to the play at run time. Being loaded last, it shadows the role's handler of the same name and runs after the restart, so systemd restarts the node from its cached copy of the unit it just rewrote. Seen on a real fleet deploy: WatchdogSec=60 on disk, NeedDaemonReload=no afterwards, and the running node had no watchdog until a manual restart. "Restart decdn-node" and "Restart alloy" now set daemon_reload: true, so handler order no longer matters. The default scenario reproduces it: prepare.yml leaves a stale unit loaded and running, converge.yml include_role's a test role with a same-named "Reload systemd" handler and arms the watchdog, and verify.yml asserts the running process is the stub with WATCHDOG_USEC set. The stub heartbeats WATCHDOG=1 like the real daemon. Without the fix, verify finds /bin/sleep from the stale unit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The fix is narrowly scoped, aligns with Ansible/systemd behavior, and is backed by a targeted Molecule regression that reproduces the handler-shadowing failure mode.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR hardens Ansible systemd restart behavior so updated unit files take effect immediately, even when a later-loaded role (e.g., devsec.hardening.os_hardening via include_role) shadows the Reload systemd handler name and changes handler ordering at runtime.
Changes:
- Make
Restart decdn-nodeandRestart alloyhandlers performdaemon_reload: trueto decouple correctness from handler ordering. - Add/extend Molecule (
default+os-matrix) regression coverage by staging a stale loaded unit and intentionally shadowingReload systemd, then asserting the running process reflects the updated unit/watchdog. - Document the fix in the Galaxy changelog.
| File | Description |
|---|---|
| ansible/roles/grafana_alloy/handlers/main.yml | Ensures Alloy restart reloads systemd so unit rewrites apply immediately. |
| ansible/roles/decdn_node/handlers/main.yml | Ensures decdn-node restart reloads systemd so unit rewrites apply immediately. |
| ansible/molecule/default/molecule.yml | Extends roles path so the test-only shadow role can be included. |
| ansible/molecule/os-matrix/molecule.yml | Mirrors the default scenario roles-path change for matrix reuse. |
| ansible/molecule/default/prepare.yml | Stages and starts a stale, loaded decdn-node unit to reproduce the cached-unit restart behavior. |
| ansible/molecule/default/converge.yml | Enables watchdog and loads a shadow Reload systemd handler to reproduce the ordering issue. |
| ansible/molecule/default/verify.yml | Verifies the running process reflects the updated unit by checking cmdline and WATCHDOG_USEC. |
| ansible/molecule/default/roles/shadow_reload_systemd/handlers/main.yml | Adds a test-only role that shadows Reload systemd to simulate devsec’s behavior. |
| ansible/molecule/default/files/decdn-node-stub | Adds watchdog heartbeating so the stub survives watchdog-enabled units. |
| ansible/galaxy/CHANGELOG.md | Records the behavioral fix for unit updates taking effect on the converge that writes them. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Opus 5.5 <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.

Problem
Install decdn-node systemd unitnotifiesReload systemdthenRestart decdn-node, assuming the reload runs first. Onsite.ymlit doesn't.baselineloadsdevsec.hardening.os_hardeningviainclude_role, which appends that role's ownReload systemdhandler to the play at run time. Handler lookup takes the last loaded handler of a name, so devsec's shadows the role's, and it runs at the end of the handler list, after the restart.The internal fleet hit this (internal-devops #24):
systemd restarted the node from its cached unit. The new unit was on disk and
NeedDaemonReload=no, but the running node hadWatchdogUSec=0until a manual restart. Any unit change is silently not applied until the next restart.Fix
Restart decdn-nodeandRestart alloynow setdaemon_reload: true, so handler order no longer matters. This matches the internal fix (146163e).Regression test (
defaultscenario, also coversos-matrix)prepare.ymlstages a staledecdn-node.service(/bin/sleep infinity, noWatchdogSec), loaded and running, so converge changes a loaded unit. A brand-new unit is read from disk on first use and would not reproduce the bug.converge.ymlloads a test-only role (molecule/default/roles/shadow_reload_systemd) withinclude_role, giving the same handler shadowing and order as devsec. It also setsdecdn_node_watchdog_sec: 60.WATCHDOG=1toNOTIFY_SOCKETeveryWATCHDOG_USEC/2, like the real daemon, so it survives the watchdog.verify.ymlreads/proc/<MainPID>and asserts the running process is the stub and that it was started withWATCHDOG_USEC=60000000. OnlyWATCHDOG_*variables are read, never the RPC URL.Checked locally both ways:
Command line: /bin/sleep infinity.molecule test -s defaultpasses (converge, idempotence and verify).Checks
make lint-ansiblepasses (production profile)pre-commit run --all-files: every hook exceptmarkdownlint-cli2passes. That hook couldn't install locally (node 24.14 < 24.15 required), so CI covers the CHANGELOG entry.molecule test -s defaultpasses locally;os-matrixshares this prepare/converge and is left to CI.ansible/galaxy/CHANGELOG.md[Unreleased] / Fixedupdated🤖 Generated with Claude Code