[nko-cpu]

Summary

Part of #776. Follow-up requested by @lockwobr on #776 (from #788).

HandleConfigUpdates clears executors on every node it resets (deleteConfigUpdateExecutors), so the config stage re-runs on the new ConfigMap. No spec covered that wiring: dropping or moving the call would let a reset node finish on a Job still running the old data without failing a test.

Change

One envtest spec next to the other HandleConfigUpdates ones. It seeds an unfinished config Job on each of three nodes:

• an erroring node at config, which opens the gate and is reset;
• a node at interrupt (in progress), which is reset without erroring, so cleanup cannot depend on the gate-opening node;
• a node still at apply, which the reset leaves alone.

It runs HandleConfigUpdates and asserts the first two Jobs are deleted and the third is kept. envtest runs no garbage collector, so a foreground delete shows as a deletionTimestamp.

Checked it fails when the deleteConfigUpdateExecutors call is removed.

Tested locally (from operator/)

• make vet lint
• go test ./internal/controller/ (the whole package, envtest 1.34.1)

The full make unit-tests target runs in CI.

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: NVIDIA/nodewright
• Branch: main
• File: .coderabbit.yaml
• Commit: 9811385
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Enterprise
• Run ID: e5ec8a9f-ca17-424c-a858-a6466ac11e9d

📥 Commits

Reviewing files that changed from the base of the PR and between 9811385 and ecb9d37.

📒 Files selected for processing: 1

• operator/internal/controller/skyhook_controller_test.go

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 4 remain after this review.

📝 Walkthrough

Adds a test for HandleConfigUpdates. The test checks that unfinished config Jobs for erroring and interrupt-stage nodes are deleted or marked for deletion after a config update resets those nodes. It also checks that the Job for an apply-stage node remains.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Possibly related issues

• [BUG]: Config updates can skip a package's own interrupt, reset nodes mid-apply, and leave stale keys #776: The issue describes HandleConfigUpdates deleting unfinished executor Jobs when a config update resets node state. This test checks that behavior.

Possibly related PRs

• NVIDIA/nodewright#879: This PR adds an envtest for the same HandleConfigUpdates config-reset Job cleanup behavior.

Merge Risk: ⚪ Minimal · up to ecb9d

No merge-blocking issue is identified in this test change; it is ready for normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title names the change accurately: a test spec that covers the config-update reset clearing executors. The changeset adds one envtest spec for HandleConfigUpdates that checks config Jobs are delet…
Description check		The description states the purpose (cover the deleteConfigUpdateExecutors wiring in HandleConfigUpdates), the change (one envtest spec with three nodes at config, interrupt, and apply), the expected a…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the change accurately: a test spec that covers the config-update reset clearing executors. The changeset adds one envtest spec for HandleConfigUpdates that checks config Jobs are deleted on reset nodes and kept on non-reset nodes. The title is concise, specific, and follows the conventional test(scope) prefix. No search was needed for this check.

Full details: Description check

Explanation

The description states the purpose (cover the deleteConfigUpdateExecutors wiring in HandleConfigUpdates), the change (one envtest spec with three nodes at config, interrupt, and apply), the expected assertions, and the local test steps. It matches the changeset, so the check passes.

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[nko-cpu]

This replaces #879 (same branch, now pushed to NVIDIA/nodewright so CI runs without fork approval). The review there is addressed and the replies are on #879:

• lockwobr: comment reworded, description now covers all three seeded nodes and lists the checks run, and /assign is done on [BUG]: Config updates can skip a package's own interrupt, reset nodes mid-apply, and leave stale keys #776.
• CodeRabbit: the spec now seeds a second, non-erroring reset node.

The threads there are left open for the reviewers to resolve.