test(agent): run the service interrupt against a real agent in the operator-agent suite

[rice-riley]

Description

Closes #679.

Adds k8s-tests/operator-agent/service_restart/, which runs the service interrupt against a real agent. Until now the only interrupt type ever run against a real agent in this suite was noop; the main chainsaw suite's reboot cases use the agentless image and cover the operator's side only. So the agent's own interrupt command path — systemctl daemon-reload, the per-service restart, and the one completion marker per operation that a retry resumes from — had no end-to-end coverage on either agent going into #222.

Why service and not reboot. Kind nodes run systemd as PID 1, and job_builder.go mounts the host root with MountPropagation: HostToContainer, so systemctl inside the agent's chroot reaches the real manager. reboot would terminate the kind node container. That path stays unit-tested (#678); the scenario and the doc say so explicitly rather than let a green run imply more than it proves.

What it asserts, all gated with set -e:

• The package's apply.sh installs the unit twice. The first definition (Type=oneshot, RemainAfterExit=yes) is loaded and started, and its ExecStart appends a line to a stale marker; the file is then overwritten with the real definition, whose ExecStart appends to the real marker, without a daemon-reload. The interrupt runs daemon-reload as operation 0 and the restart as operation 1, so:
  • Exactly one real line (grep -c . … = 1) proves the interrupt ran the real service manager exactly once.
  • Exactly one stale line proves operation 0 reached the manager: a restart whose daemon-reload never took effect would have re-run the stale in-memory definition, leaving two stale lines and no real one. The stale line is also the proof apply.sh ran, whose own log both agents rotate away by completion (5 logs kept, this package writes 6).
• Both service_restart_0.complete and service_restart_1.complete exist — the per-operation layout both agents share once an interrupt finishes. The comment records that Python writes a marker before running its operation and Go after it succeeds, so a Python pod killed mid-restart can leave a marker for a restart that never completed; this scenario is a single clean pass and asserts the layout.
• post-interrupt and post-interrupt-check ran, asserted the way post_interrupt/ asserts them for noop: post interrupt ran and post interrupt checked in the logs, post-interrupt-check_ALL_CHECKED under the flags dir.
• The "Could not find file" inverted guard from ci(agent): run the operator-agent suite against the Go agent too #605, made meaningful by supplying every stage the package traverses. It sees only the surviving shellscript_run.sh logs, not the interrupt's own log under $log/interrupts/.

Cleanup runs both before the run and in finally: stop the unit, remove the file, daemon-reload, reset-failed, drop both markers. A reused local cluster is left as it was found. Runs against both agents through the existing path routing.

restartAllServices (service procps force-reload) is deliberately not included here; it is worth a second scenario, and if service/procps turn out to be absent on the kind node image that is a finding to record rather than shim around.

Checklist

• I am familiar with the Contributing Guidelines.
• My commits are signed off per the DCO and cryptographically signed: git commit -s -S.
• New or existing tests cover these changes. This PR is a test.
• The documentation is up to date with these changes. docs/contributing/ci-test-pools.md's operator-agent coverage paragraph now names the service interrupt and why reboot is not covered here.

Verification

Locally: YAML parses, the CR validates against the CRD schema (including the interrupt.services field), assert.yaml is structurally identical to interrupt/'s, license headers clean. The e2e itself needs a cluster and runs in CI against both agents. The first CI run settled the one open question: chrooted systemctl on the kind node works on both agents (the marker was written exactly once and both completion markers were present). That run failed only on a log assertion for apply.sh, whose log had been reaped by the 5-file retention; the stale marker now covers that. The two-definition unit from review depends on systemd keeping the RemainAfterExit unit in memory and using that stale definition on a restart without a reload, which I could not run locally either; the next CI run is the proof. AI assistance: produced with Claude Code.

🤖 Generated with Claude Code

[github-actions]

🌿 Preview your docs: https://nvidia-preview-test-service-interrupt-e2e-679.docs.buildwithfern.com/nodewright

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository: NVIDIA/nodewright/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 0c510d7b-0a6f-4e77-97fc-64483dca990b

📥 Commits

Reviewing files that changed from the base of the PR and between 0306161 and dc29ca4.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: NVIDIA/nodewright/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: b05f106e-1557-421d-9d16-bbdaff268c0c

📥 Commits

Reviewing files that changed from the base of the PR and between 945ae14 and 0306161.

📒 Files selected for processing (3)

• docs/contributing/ci-test-pools.md
• k8s-tests/operator-agent/service_restart/chainsaw-test.yaml
• k8s-tests/operator-agent/service_restart/nodewright.yaml

Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

Adds a Chainsaw scenario that runs the service interrupt with a systemd unit installed by the test resource. The test checks the real and stale service markers, interrupt completion markers, resource status, node conditions, and stage log messages. The contributor documentation describes the scenario and explains why reboot remains unit-tested and restart_all_services is untested on the kind image.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 03061

The service-restart scenario has no identified merge-blocking issue; it is ready for normal checks.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The pull request satisfies the coding requirements in [#679]. The new service_restart Chainsaw scenario targets a real service interrupt and the nodewright-e2e service. Its assertions check the …
Out of Scope Changes check	✅ Passed	The changes are limited to the linked scenario, its lifecycle scripts and assertions, and the related operator-agent coverage documentation. The stale-unit setup, cleanup, log checks, and stage script…
Title check	✅ Passed	The title clearly identifies the main change: adding an operator-agent test that runs the service interrupt against a real agent.
Description check	✅ Passed	The description directly explains the new service-restart scenario, its assertions, coverage scope, documentation updates, and verification results.

✨ Finishing Touches 💡 1

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[coveralls]

Coverage Report for CI Build 36481834386

Coverage increased (+0.06%) to 82.462%

Details

• Coverage increased (+0.06%) from the base build.
• Patch coverage: No coverable lines changed in this PR.
• 15 coverage regressions across 3 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

15 previously-covered lines in 3 files lost coverage.

File	Lines Losing Coverage	Coverage
operator/internal/cli/utils/utils.go	8	77.49%
operator/cmd/cli/app/lifecycle.go	5	78.34%
operator/internal/controller/skyhook_controller.go	2	80.55%

---

Coverage Stats

Relevant Lines:	11358
Covered Lines:	9366
Line Coverage:	82.46%
Coverage Strength:	7.63 hits per line

---

💛 - Coveralls

[lockwobr]

Running a real service interrupt against both agents is a good addition, and the gating (set -e on every block, exact-count marker) looks right. Inline are the few things that affect what the scenario actually proves. Everything below is a nit, and fine as follow-up tickets rather than changes here:

• The Could not find file guard doesn't see the interrupt logs (chainsaw-test.yaml:77). They go to $log/interrupts/, which $log/*.log doesn't match, and the apply log has been rotated away by the time the guard runs. A systemctl that prints errors but exits 0 wouldn't be caught.
• The marker comment (chainsaw-test.yaml:57-59) implies the two agents behave the same. Python writes service_restart_N.complete before running the command, and Go writes it after success. Both produce the same layout once the interrupt finishes, but a Python pod killed mid-restart leaves a marker for a restart that never completed. Worth saying so, since the comment frames this as the handoff contract.
• apply.sh has no set -e (nodewright.yaml:47). A failed unit write still exits 0 after echo "unit installed". The failure then shows up later as a Unit not found interrupt error or an assert timeout, which points away from the real cause.
• The unit-file check (chainsaw-test.yaml:71) is implied by the marker check. The marker can only exist if this run installed and restarted the unit.
• The marker poll's 30 retries (chainsaw-test.yaml:53). assert.yaml has already waited for complete, and systemctl restart on a oneshot unit is synchronous, so about 10 would do, like the sibling checks.
• Pre-existing and not specific to this PR: the Python agent names step logs with one-second resolution (%Y-%m-%d-%H%M%S) and opens them with 'w'. Every stage here runs the same shellscript_run.sh, so two stages starting in the same second would share a file, and the second would truncate the first. Init-container startup makes that rare, but it could flake any scenario that greps stage logs, including this one's post interrupt ran. That's an agent ticket rather than a test one.

[rice-riley]

The nits, in the order you listed them:

• Guard and the interrupt logs. Stated in the comment rather than fixed: the guard exists for missing configmap keys, which the interrupt's own log under $log/interrupts/ cannot contain, and the apply log's rotation is now covered by the stale marker.
• Marker comment. Now says Python writes the marker before running the operation and Go after it succeeds, and that this scenario is a clean pass asserting the layout.
• apply.sh has set -e.
• Unit-file check is gone; the stale marker proves apply.sh ran.
• Marker poll is 10 retries.
• One-second log names. Moot once feat(agent)!: cut over to the Go agent and remove the Python implementation #685 lands, since the Python agent goes with it; not filed.

Inline threads are answered individually. The two-definition unit is your design from the nodewright.yaml thread; this CI run is its first execution.