fix: close StageInterrupt trap + harden core e2e pool by ayuskauskas · Pull Request #242 · NVIDIA/nodewright

ayuskauskas · 2026-05-19T21:30:32Z

Summary

Two changes — one operator fix, one test-infra hardening — that together address the failure mode that hit chainsaw/config-skyhook on the prior CI run.

Operator: close the `StageInterrupt` trap when `ConfigUpdates` decays

A package whose only interrupt is a configInterrupts entry (no top-level interrupt block) could get permanently stuck at StageInterrupt/StateSkipped when Status.ConfigUpdates cleared or was never persisted (e.g. due to a 409 on the spec patch — the HandleMigrations stale-snapshot bug + the processSkyhooksPerNode node-status conflict, both visible in the failing run's logs).

Root cause: the state machine queried the dynamic HasInterrupt(config) signal at four sites — ProgressSkipped, NextStage, GetComplete, IsPackageComplete. Once the signal decayed to false mid-lifecycle, the package was untouchable from every angle: not promoted out of Skipped, not advanced past Interrupt, not counted as complete.

Fix decouples progression-past-Interrupt from the dynamic signal at all three "no documented intent" sites (git log -L shows the gates have been there since the initial reorg in 2025-01-23 with no commit motivating them). The fourth site — NextStage's StageUninstall → StageApply gate — is preserved as-is, because PR #200 deliberately removed that transition from the with-interrupt map so with-interrupt uninstalls route via StageUninstallInterrupt instead. Collapsing the maps would silently undo that.

Reproducer in skyhook_types_test.go exercises all three failure modes; full make unit-tests passes (10 suites, all green).

Tests: harden the `core` chainsaw pool

timeouts.cleanup: 120s on all 6 core-pool tests. Chainsaw's default cleanup window (~30s) is too tight for the project's finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same class of failure that hit downgrade-after-uninstall (fix(test): clean up downgraded package in downgrade-after-uninstall #241).
catch: blocks on depends-on and simple-skyhook (the only two core tests missing them). When flakes recur, CI now captures Node, Skyhook CR, and package Pod state for diagnosis.

Test plan

make unit-tests passes locally (10 suites)
New v1alpha1 regression test (StageInterrupt trap when configUpdates signal decays) fails before the fix, passes after
All 6 runnable core chainsaw tests pass in CI (the core pool job)
chainsaw/config-skyhook "update while running" specifically reaches a clean assert (no longer hits the trap)
No regression in adjacent pools (uninstall, interrupt, lifecycle)

…mprove diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]>

coderabbitai · 2026-05-19T21:31:49Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 02c8fa71-fd7e-43ab-8f8a-bb034c9fbbe0

📥 Commits

Reviewing files that changed from the base of the PR and between 1f75c4e and e2c7aca.

📒 Files selected for processing (1)

operator/api/v1alpha1/skyhook_types.go

📝 Walkthrough

Walkthrough

This PR adds spec.timeouts.cleanup: 120s to several Chainsaw skyhook tests and introduces catch handlers in two tests to dump Node, Skyhook CR, and Pod YAML on failures. It also changes Skyhook NodeState logic so StageInterrupt always advances to StagePostInterrupt, treats StagePostInterrupt as terminal, relaxes promotion of skipped packages, and adds regression tests covering the decayed-interrupt scenario.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

lockwobr
mskalka
rice-riley

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the two main changes: an operator fix closing the StageInterrupt trap and hardening the core e2e test pool with timeouts and error handling.
Description check	✅ Passed	The description is well-detailed and directly related to the changeset, explaining both the operator fix and test infrastructure changes with context about root causes and test plans.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/core-e2e-flakiness-cleanup-timeout-catch

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

…cays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]>

coveralls · 2026-05-20T00:00:27Z

Coverage Report for CI Build 26307526057

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Warning

No base build found for commit c004c13 on main.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 81.672%

Details

Patch coverage: 13 of 13 lines across 1 file are fully covered (100%).

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →

Coverage Stats


Relevant Lines:	9461
Covered Lines:	7727
Line Coverage:	81.67%
Coverage Strength:	9.59 hits per line

💛 - Coveralls

lockwobr · 2026-05-20T20:00:21Z

@ayuskauskas should this be included in the next release v0.16.0?

GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]>

* fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]>

* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>

* fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: ayuskauskas <[email protected]>

The `glob update` step in config-skyhook/chainsaw-test.yaml is the canonical reproducer for issue #245 (Owned ConfigMap can permanently desync from spec when a stale Status.ConfigUpdates entry drives a spurious interrupt cycle). The operator gets exactly one good reconcile window per spec change to satisfy the CM-write gate, and timing on kind regularly misses it. The issue itself documents this CI workaround: "Skip the `glob update` step in config-skyhook/chainsaw-test.yaml with a # TODO(<this-issue>) comment until the fix lands." This is unrelated to PR #258's CLI changes; the test flake was previously masked by other failures in the same file and surfaced after PR #242 closed the StageInterrupt trap. Signed-off-by: Alex Yuskauskas <[email protected]>

* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>

* feat(agent): port enums and steps for agent go rewrite Signed-off-by: Riley Rice <[email protected]> * chore: Cherry pick (#244) * ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]> --------- Signed-off-by: Riley Rice <[email protected]> Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Brian Lockwood <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>

ayuskauskas requested review from lockwobr, mskalka and rice-riley as code owners May 19, 2026 21:30

ayuskauskas changed the title ~~fix(test): harden core e2e pool against finalizer cleanup races~~ fix: close StageInterrupt trap + harden core e2e pool May 19, 2026

lockwobr assigned ayuskauskas May 22, 2026

lockwobr approved these changes May 22, 2026

View reviewed changes

lockwobr merged commit bdfa3e6 into main May 22, 2026
53 of 55 checks passed

lockwobr deleted the fix/core-e2e-flakiness-cleanup-timeout-catch branch May 22, 2026 19:40

coderabbitai Bot mentioned this pull request May 22, 2026

chore: Cherry pick #244

Merged

ayuskauskas mentioned this pull request May 22, 2026

Owned ConfigMap can permanently desync from spec when a stale Status.ConfigUpdates entry drives a spurious interrupt cycle #245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: close StageInterrupt trap + harden core e2e pool#242

fix: close StageInterrupt trap + harden core e2e pool#242
lockwobr merged 3 commits into
mainfrom
fix/core-e2e-flakiness-cleanup-timeout-catch

ayuskauskas commented May 19, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented May 19, 2026 •

edited

Loading

Walkthrough

Estimated code review effort

Suggested reviewers

Uh oh!

coveralls commented May 20, 2026 •

edited

Loading

Uh oh!

lockwobr commented May 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ayuskauskas commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Operator: close the StageInterrupt trap when ConfigUpdates decays

Tests: harden the core chainsaw pool

Test plan

Uh oh!

coderabbitai Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Suggested reviewers

Uh oh!

coveralls commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 26307526057

Coverage: 81.672%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

lockwobr commented May 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ayuskauskas commented May 19, 2026 •

edited

Loading

Operator: close the `StageInterrupt` trap when `ConfigUpdates` decays

Tests: harden the `core` chainsaw pool

coderabbitai Bot commented May 19, 2026 •

edited

Loading

coveralls commented May 20, 2026 •

edited

Loading