feat: add runtimeRequiredCordonAfter to support persistent node cordons - #401
feat: add runtimeRequiredCordonAfter to support persistent node cordons#401natherz97 wants to merge 2 commits into
Conversation
|
Welcome to NodeWright, @natherz97! Thanks for your first pull request. Before review, please ensure:
A maintainer will review this soon. |
github-actions
Bot
added
doc
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (16)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughThe change adds Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/runtime_required.md`:
- Around line 62-63: Update the runtime-required taint-removal documentation
around the `runtimeRequiredCordonAfter` behavior to state that removing the
taint removes only that scheduling barrier. Clarify that the node becomes
schedulable only when no `nodewright.nvidia.com/cordon_*` owners remain, since
other NodeWright or external cordons may still keep it unschedulable.
In
`@k8s-tests/chainsaw/nodewright/runtime-required-cordon-after/chainsaw-test.yaml`:
- Around line 107-113: Extend the release-cordon phase after the kubectl patch
to wait for reconciliation, then add a final assertion verifying that node
kind-worker no longer has the runtimeRequiredCordon annotation and that
spec.unschedulable is false. Use the existing Chainsaw assertion conventions in
this test.
In `@operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml`:
- Around line 630-638: Regenerate the legacy CRD schema so the
runtimeRequiredCordonAfter description matches the complete documentation in the
Skyhook API type, using the repository’s standard make manifests generate
command after the API change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4968e5ff-4b5b-4194-a699-2d1caa66f381
📒 Files selected for processing (16)
docs/runtime_required.mdk8s-tests/chainsaw/nodewright/runtime-required-cordon-after/chainsaw-test.yamloperator/api/nodewright/v1alpha1/nodewright_types.gooperator/api/v1alpha1/nodewright_conversion.gooperator/api/v1alpha1/nodewright_conversion_test.gooperator/api/v1alpha1/skyhook_types.gooperator/config/crd/bases/nodewright.nvidia.com_deploymentpolicies.yamloperator/config/crd/bases/nodewright.nvidia.com_nodewrights.yamloperator/config/crd/bases/skyhook.nvidia.com_deploymentpolicies.yamloperator/config/crd/bases/skyhook.nvidia.com_skyhooks.yamloperator/config/rbac/role.yamloperator/config/webhook/manifests.yamloperator/internal/controller/skyhook_controller.gooperator/internal/controller/skyhook_controller_test.gooperator/internal/wrapper/node.gooperator/internal/wrapper/node_test.go
💤 Files with no reviewable changes (4)
- operator/config/crd/bases/skyhook.nvidia.com_deploymentpolicies.yaml
- operator/config/rbac/role.yaml
- operator/config/webhook/manifests.yaml
- operator/config/crd/bases/nodewright.nvidia.com_deploymentpolicies.yaml
315d32b to
231c020
Compare
|
@natherz97 I think maybe there is a release happening for the base image. I think you might just had bad luck. 4.0.1 does not yet exist. If we wait this should resolve itself, in the meantime i will talk to that team to see if we can fix this race condition in the base container publishing. |
Signed-off-by: Nathan Herz <nherz@nvidia.com>
231c020 to
9092944
Compare
There was a problem hiding this comment.
Chart CRDs not updated
chart/templates/nodewright-crd.yaml and chart/templates/skyhook-crd.yaml don't include the new runtimeRequiredCordonAfter field. Users deploying via Helm won't be able to set the field — it will be silently dropped by the apiserver webhook. Per repo convention, operator/config/ and chart/ must stay in sync in the same PR.
Minor
operator/api/v1alpha1/skyhook_types.go — RuntimeRequiredCordonAnnotation constant
This constant value (skyhook.nvidia.com/runtimeRequiredCordon) is never written to any node. The controller only imports api/nodewright/v1alpha1, so all nodes get nodewright.nvidia.com/runtimeRequiredCordon regardless of whether they were targeted by a legacy Skyhook CR or a NodeWright CR. After conversion, there are no legacy Skyhook CRs left in the reconciler — it always works against the converted nodewright type.
kubectl nodewright reset doesn't clear the persistent cordon
resetAnnotationKeys() in operator/cmd/cli/app/reset.go doesn't include RuntimeRequiredCordonAnnotation. After a kubectl nodewright reset, the package state is wiped and the NW re-runs, but the persistent cordon (unschedulable + annotation) remains. This creates a confusing intermediate window where the node is cordoned but has no runtime-required taint. More importantly, if the CR is deleted and recreated without runtimeRequiredCordonAfter, the cordon persists indefinitely with no self-healing path. Either add the annotation to resetAnnotationKeys() (and uncordon if no other Skyhook cordon holds it), or document this as intentional.
docs/runtime_required.md — two gaps
kubectl nodewright resetbehavior: the persistent cordon currently survives a reset (see above). Worth calling out here so users aren't surprised when they reset and find the node still cordoned.REAPPLY_ON_REBOOT=trueinteraction: theautoTaintNewNodessection already documents that a boot-ID change re-applies the taint and triggers a re-run. WithruntimeRequiredCordonAfter: true, the cordon will be re-applied after that re-run completes. That is probably the intended behavior, but it should be stated explicitly.
k8s-tests/chainsaw/nodewright/runtime-required-cordon-after/chainsaw-test.yaml — multi-CR coverage
The test exercises single-CR behavior, but the PR description says 'the cordon is applied if any runtimeRequired NodeWright enables runtimeRequiredCordonAfter.' The unit test runtimeRequiredCordonAfterEnabled should return true when any skyhook has it enabled covers this at the unit level. Consider adding a step (or a separate test) where two NodeWrights target the same node — one with runtimeRequiredCordonAfter: true, one without — to give e2e confidence for the 'any' semantic.
lockwobr
left a comment
lockwobr
left a comment
There was a problem hiding this comment.
Cross-review summary
Automated multi-reviewer analysis of 9323a0bd, cross-reviewed to consensus with adversarial verification of every finding. Nothing was run against a cluster and no tests were executed; every claim comes from reading the pinned commit. CI is green on this commit (all 36 checks, including all four e2e pools across k8s 1.33-1.36 and helm-tests).
This is scoped to what is not already covered by the existing review and the automated comments on this PR. The chart-CRD gap and the chainsaw release assertion are both already raised, so they are not repeated here.
The operator-side implementation is coherent and well tested: the taint removal and the cordon land in a single patch, taints.RemoveTaint deep-copies so the merge base is unmutated, and the taint-exists guard makes a repeat reconcile a no-op. Everything below is at a boundary.
One addition to the chart-CRD point
Agreed, and one thing worth adding: the e2e in this PR structurally cannot catch it, because make e2e-tests installs CRDs from operator/config via kustomize, not from the chart. So the test exercises a schema chart users never receive. If this class of gap is worth closing permanently, a helm-tests case asserting a new field survives an apply round trip would do it, since the hand-maintained chart mirror is easy to miss.
A doc page not yet mentioned
docs/uninstall.md:88 states "A node is uncordoned only once no cordon_* ownership annotations remain (other NodeWrights sharing the node may still hold it); the CR finalizer is removed last." That is now false: the finalizer path leaves the node unschedulable even when zero cordon_* annotations remain, because the unsuffixed runtimeRequiredCordon key gates Uncordon and CleanupSCRMetadata cannot remove it. Line 233 compounds it by pointing users at a reset command that does not clear it either.
docs/interrupt_flow.md:88 has the same problem for a different reason; see the inline comment on wrapper/node.go. A fix scoped to one page would leave the other asserting the opposite of the new behavior.
Open questions
- Is stranding the cordon after NodeWright deletion intentional? Framing release as an external-actor action is coherent, but
helm uninstalldeletes all NodeWrights via thecleanup-skyhooks-jobpre-delete hook, and after that no controller exists that could ever clear the annotation. Every affected node stays unschedulable with no self-healing path. This is the hinge for thewrapper/node.gofinding. - Should the cordon be re-converged level-triggered? Detail in the inline comment on
skyhook_controller.go. - Should the webhook reject
runtimeRequiredCordonAfter: truewithruntimeRequired: false? Silently ignored today, documented only in prose. Worth noting that neitherruntimeRequirednorautoTaintNewNodescarries a validation rule either, so omitting one is consistent with existing convention. - Should
kubectl nodewright node statussurface this cordon?cordonAnnotationPrefixis declared innode_status.go:42but never used for display, so the CLI shows no cordon state at all. New surface rather than a regression, but a node held unschedulable by an annotation the CLI cannot show is a rough edge.
Checked and clear
Two things that look like problems on a skim and are not, recorded so nobody re-derives them:
- No import-alias mismatch.
RuntimeRequiredCordonAnnotationis declared in both api packages from each package's ownMETADATA_PREFIX, so it resolves to two different strings. Bothskyhook_controller.go:35andwrapper/node.go:28importapi/nodewright/v1alpha1unaliased, so writer and reader agree onnodewright.nvidia.com/runtimeRequiredCordon, matching the docs and the e2e fixture. (This is the same fact the existing review reaches from the other direction: the legacy-package constant is dead, never written to any node.) - The license-header change in six generated YAML files is convergence, not drift.
scripts/license-header.tmplemits exactly one blank comment line between the SPDX block and "Licensed under", and those six files are the controller-gen-regenerated ones catching up to it. No action needed.
Also checked: the forward-only conversion assignment is complete, since no reverse NodeWright -> Skyhook converter exists.
| // preserve the persistent cordon. Note that any runtime-required Skyhooks that run initially are free to | ||
| // add and remove the interrupt cordon because the persistent cordon is only applied when the runtime-required | ||
| // taint is removed. | ||
| _, ok := annotations[v1alpha1.RuntimeRequiredCordonAnnotation] |
There was a problem hiding this comment.
Highest-severity finding here, and not covered elsewhere on this PR. This line does two things beyond what it looks like, and both outlive the NodeWright.
1. The cordon is never reversed by the finalizer. Finalizer cleanup is node.Uncordon() followed by node.CleanupSCRMetadata() (skyhook_controller.go:1924-1925). CleanupSCRMetadata only deletes keys matching prefix nodewright.nvidia.com/ and suffix _<nodewrightName>; runtimeRequiredCordon carries no name suffix, so it survives. And Uncordon no longer clears Spec.Unschedulable, because hasSkyhookCordon now returns true on the strength of that very annotation.
So deleting the NodeWright leaves the node unschedulable with an orphaned annotation. helm uninstall is worse: its cleanup-skyhooks-job pre-delete hook runs kubectl delete nodewrights --all, after which nothing exists that could clear it. This is the repo invariant that node-level mutations made outside the owner-reference tree must be finalizer-reversible.
2. The annotation becomes global cordon state. This is the part I have not seen raised yet. hasSkyhookCordon is consulted by every NodeWright's Uncordon, not just runtime-required ones. Once this key is on a node, any later, unrelated NodeWright that cordons it for an interrupt will remove its own cordon_<name> annotation and then silently fail to uncordon. An SRE debugging that node enumerates cordon_*, finds none, and has no explanation for why it stays unschedulable.
That is also why docs/interrupt_flow.md:88 is now false: "Only the cordon_* annotation family participates in shared cordon ownership." It no longer is. Lines 94-96 of that page tell users to run kubectl uncordon to recover, which clears spec.unschedulable but leaves the annotation, so the next interrupt cordon sticks. docs/uninstall.md:88 has the same problem (see the review body).
If the persistent cordon is meant to survive CR deletion, that is defensible, but the annotation likely needs to be scoped so it only gates the NodeWrights that asked for it, and both doc pages need updating.
| if new_node.Annotations == nil { | ||
| new_node.Annotations = make(map[string]string) | ||
| } | ||
| new_node.Annotations[v1alpha1.RuntimeRequiredCordonAnnotation] = annotationTrueValue |
There was a problem hiding this comment.
This is the only place the persistent cordon is ever written, which has a consequence worth stating alongside the wrapper/node.go finding.
The write sits inside the if updated branch, gated on the runtime-required taint existing. So the annotation is applied exactly once, at taint-removal time, and never re-converged. Combined with nothing removing it (the finalizer cannot, and neither reset command touches it), its lifecycle is write-once-never-delete from the operator's side.
The consequence: if someone runs kubectl uncordon and clears spec.unschedulable without deleting the annotation, the operator never re-cordons the node, yet the stale annotation keeps blocking Uncordon for every other NodeWright. The node lands in a state that is neither cordoned nor releasable, and the documented recovery patch (which clears both together) is the only thing that produces a clean result. Worth deciding whether reconcile should re-converge on the annotation rather than only writing it at the taint-removal edge.
To be clear, the patch itself is correct and idempotent: taints.RemoveTaint deep-copies the node so client.MergeFrom(node) is computed against an unmutated base, the taint and the cordon land atomically (closing the window where the node could be briefly schedulable), and the taint-exists guard makes a repeat reconcile a no-op. The single-patch design and the comment explaining it are good.
| kind: NodeWright | ||
| name: runtime-required-cordon-after | ||
| format: yaml | ||
| steps: |
There was a problem hiding this comment.
This test has no finally block, and it mutates node-level state that chainsaw does not own.
It taints kind-worker with skyhook.nvidia.com=runtime-required:NoSchedule in setup and relies on the operator plus the Phase 5 patch to undo the taint and the cordon. If any phase times out, none of that runs. The sibling test k8s-tests/chainsaw/nodewright/runtime-required/chainsaw-test.yaml ends with finally: ../nodes_remove_taint.sh all skyhook.nvidia.com=runtime-required:NoSchedule ... for exactly this reason.
Both tests carry pool: interrupt, and that pool runs --parallel 1 against a shared kind cluster, so leftovers are inherited by everything after. shared-cordon-ownership runs in the same pool and asserts the node ends up schedulable; it would fail, because the leftover runtimeRequiredCordon annotation makes Uncordon a no-op. Its own cordon_* check would pass first, so the failure output points at the wrong thing entirely.
One real flake here becomes a cascade of misleading failures elsewhere. A finally that removes the taint, the cordon and the annotation would contain it.
| Expect(runtimeRequiredCordonAfterEnabled([]SkyhookNodes{sh1, sh2})).To(BeTrue()) | ||
| }) | ||
| Context("HandleRuntimeRequired with runtimeRequiredCordonAfter", func() { | ||
| runtimeRequiredTaintKey := "skyhook.nvidia.com" |
There was a problem hiding this comment.
Trivial and test-only, but worth a line since it silently weakens the specs below it.
This hardcodes the runtime-required taint key as "skyhook.nvidia.com" rather than deriving it from opts.GetRuntimeRequiredTaint().Key, which the surrounding specs already use to build the taint they apply.
The literal matches today so the specs pass. But the assertions are of the form Expect(t.Key).ToNot(Equal(runtimeRequiredTaintKey)), so if the configured key ever changes they become vacuously true and stop catching a regression where the taint is not removed at all.
runtimeRequiredTaintKey := opts.GetRuntimeRequiredTaint().Key keeps the assertion coupled to the thing under test.
|
@ayuskauskas thanks for the review, working through the edits but I wanted to clarify if I should make changes to the kubectl nodewright reset command. I'm not sure if it's straightforward for us to determine if the runtimeRequiredCordon annotation and cordon can be removed when we execute a reset. We're not tracking which runtime-required NodeWright resulted in the node receiving the persistent cordon and it's possible that multiple runtime-required NodeWrights all set runtimeRequiredCordonAfter to true. Additionally, we can't reliably look this up since we preserve the annotation and cordon even the NodeWrights which set runtimeRequiredCordonAfter to true are deleted. I was planning on not allowing the reset command to touch the runtimeRequiredCordon annotation nor cordon state of a node but let me know if you disagree. If we're on the same page, I'll proceed with a documentation update. @lockwobr answering your questions
I'll push a revision if this sounds reasonable to you guys. |
|
@natherz97 this PR now has merge conflicts with |
3 participants
Description
This PR adds a runtimeRequiredCordonAfter field to the NodeWright API to support persistent node cordons. This PR implements the behavior outlined in the following comment: #284 (comment).
Feature overview:
Closes #284
Checklist
git commit -s) per the DCO.