Skip to content

feat: add runtimeRequiredCordonAfter to support persistent node cordons - #401

Open
natherz97 wants to merge 1 commit into
NVIDIA:mainfrom
natherz97:runtime-required-cordon
Open

feat: add runtimeRequiredCordonAfter to support persistent node cordons#401
natherz97 wants to merge 1 commit into
NVIDIA:mainfrom
natherz97:runtime-required-cordon

Conversation

@natherz97

Copy link
Copy Markdown

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:

  • The persistent node cordon is applied if any runtimeRequired NodeWright CR enables runtimeRequiredCordonAfter. If any NodeWright CR sets runtimeRequired to false but runtimeRequiredCordonAfter to true, the cordon will not be applied.
  • The cordon is only applied at the same time as taint removal if the taint currently exists on the node. If the taint has already been removed from a node, the cordon is not applied even if a new runtime-required NodeWright targets it.
  • The cordon applied by runtimeRequiredCordonAfter will persist through any future NodeWright CR executions with interrupts that may require cordoning the node (which themselves may have runtimeRequired set to true). This is accomplished by using a runtimeRequiredCordon annotation which ensures the cordon is preserved after the interrupt package completes. Any pre-existing cordon applied outside NodeWright is not preserved, which is consistent with current behavior.
  • Releasing this persistent cordon requires removing the runtimeRequiredCordon annotation and setting unschedulable to false in the same patch.

Closes #284

Checklist

  • I am familiar with the Contributing Guidelines.
  • My commits are signed off (git commit -s) per the DCO.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@natherz97
natherz97 requested a review from a team August 3, 2026 21:01
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Welcome to NodeWright, @natherz97! Thanks for your first pull request.

Before review, please ensure:

  • All commits are signed off per the DCO (git commit -s)
  • Commits follow Conventional Commits
  • CI checks pass (tests, lint, security scan)
  • The PR description explains the why behind your changes

A maintainer will review this soon.

@github-actions github-actions Bot added doc Documentation change (PR path label; doc issues use the Documentation type) component/operator Skyhook operator (controller-manager) component/ci CI workflows, GitHub Actions, and repo tooling component/tests End-to-end / chainsaw test suites (k8s-tests) labels Aug 3, 2026
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 1c81c812-1ff1-49cb-bc52-a315e1a6faf8

📥 Commits

Reviewing files that changed from the base of the PR and between 231c020 and 9092944.

📒 Files selected for processing (16)
  • docs/runtime_required.md
  • k8s-tests/chainsaw/nodewright/runtime-required-cordon-after/chainsaw-test.yaml
  • operator/api/nodewright/v1alpha1/nodewright_types.go
  • operator/api/v1alpha1/nodewright_conversion.go
  • operator/api/v1alpha1/nodewright_conversion_test.go
  • operator/api/v1alpha1/skyhook_types.go
  • operator/config/crd/bases/nodewright.nvidia.com_deploymentpolicies.yaml
  • operator/config/crd/bases/nodewright.nvidia.com_nodewrights.yaml
  • operator/config/crd/bases/skyhook.nvidia.com_deploymentpolicies.yaml
  • operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml
  • operator/config/rbac/role.yaml
  • operator/config/webhook/manifests.yaml
  • operator/internal/controller/skyhook_controller.go
  • operator/internal/controller/skyhook_controller_test.go
  • operator/internal/wrapper/node.go
  • operator/internal/wrapper/node_test.go
💤 Files with no reviewable changes (4)
  • operator/config/rbac/role.yaml
  • operator/config/crd/bases/skyhook.nvidia.com_deploymentpolicies.yaml
  • operator/config/crd/bases/nodewright.nvidia.com_deploymentpolicies.yaml
  • operator/config/webhook/manifests.yaml

📝 Walkthrough

Walkthrough

The change adds runtimeRequiredCordonAfter to NodeWright and Skyhook specifications and CRD schemas. Runtime-required completion can now remove the runtime-required taint, add a cordon annotation, and mark the node unschedulable. Node-wrapper logic preserves this cordon during uncordon operations. Unit, integration, and Chainsaw tests cover the behavior. Documentation describes configuration and manual release.

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

Suggested reviewers: lockwobr, ayuskauskas

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Several manifest files contain unrelated removals of standalone license-header comment lines. Remove the unrelated license-header cleanup changes from the pull request, unless they are required by an established repository-wide formatting rule.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the addition of runtimeRequiredCordonAfter and its persistent node-cordon behavior.
Description check ✅ Passed The description explains the new field, cordon behavior, persistence, release workflow, testing, and documentation updates.
Linked Issues check ✅ Passed The implementation satisfies issue #284 by cordoning tainted nodes during taint removal, preserving the cordon, and supporting external uncordoning.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e5a3ab6 and c0fa965.

📒 Files selected for processing (16)
  • docs/runtime_required.md
  • k8s-tests/chainsaw/nodewright/runtime-required-cordon-after/chainsaw-test.yaml
  • operator/api/nodewright/v1alpha1/nodewright_types.go
  • operator/api/v1alpha1/nodewright_conversion.go
  • operator/api/v1alpha1/nodewright_conversion_test.go
  • operator/api/v1alpha1/skyhook_types.go
  • operator/config/crd/bases/nodewright.nvidia.com_deploymentpolicies.yaml
  • operator/config/crd/bases/nodewright.nvidia.com_nodewrights.yaml
  • operator/config/crd/bases/skyhook.nvidia.com_deploymentpolicies.yaml
  • operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml
  • operator/config/rbac/role.yaml
  • operator/config/webhook/manifests.yaml
  • operator/internal/controller/skyhook_controller.go
  • operator/internal/controller/skyhook_controller_test.go
  • operator/internal/wrapper/node.go
  • operator/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

Comment thread docs/runtime_required.md
Comment thread operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml
@natherz97
natherz97 force-pushed the runtime-required-cordon branch 2 times, most recently from 315d32b to 231c020 Compare August 3, 2026 21:24
@lockwobr

lockwobr commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/ci CI workflows, GitHub Actions, and repo tooling component/operator Skyhook operator (controller-manager) component/tests End-to-end / chainsaw test suites (k8s-tests) doc Documentation change (PR path label; doc issues use the Documentation type)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEA]: Allow NodeWright to add a persistent node cordon

2 participants