Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion docs/runtime_required.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,21 @@ This per-node behavior prevents deadlocks where a few bad nodes would block all

## What happens when the taint is removed

1. The node becomes available for general workload scheduling (pods without the runtime-required toleration can now be scheduled on it).
1. If `runtimeRequiredCordonAfter` is false on all `runtimeRequired` NodeWrights, the node becomes available for general workload scheduling (pods without the runtime-required toleration can now be scheduled on it).
2. If `runtimeRequiredCordonAfter` is true on one or more `runtimeRequired` NodeWrights, the node will be cordoned at the same time the runtime-required taint is removed. The operator will also set the `nodewright.nvidia.com/runtimeRequiredCordon` annotation on each cordoned node. Note that this setting is only respected on a given NodeWright if `runtimeRequired` is also true. The cordon is only applied to nodes that currently hold the runtime-required taint.
Comment thread
natherz97 marked this conversation as resolved.

```yaml
spec:
runtimeRequired: true
runtimeRequiredCordonAfter: true
```

An external actor can clear the node cordon by setting `unschedulable` to false and removing the annotation:

```bash
kubectl patch node <node-name> --type=merge \
-p '{"metadata":{"annotations":{"nodewright.nvidia.com/runtimeRequiredCordon":null}},"spec":{"unschedulable":false}}'
```

## Why would you use runtime required

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json
apiVersion: chainsaw.kyverno.io/v1alpha1
kind: Test
metadata:
name: runtime-required-cordon-after
labels:
pool: interrupt
spec:
timeouts:
assert: 120s
catch:
- get:
apiVersion: v1
kind: Node
selector: nodewright.nvidia.com/runtime-required-cordon-after-test=true
format: yaml
- get:
apiVersion: nodewright.nvidia.com/v1alpha1
kind: NodeWright
name: runtime-required-cordon-after
format: yaml
steps:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

# Phase 1: Apply node label matching NodeWright selector and apply runtime-required taint to the matching node
- name: setup
try:
- script:
content: |
../nodewright-cli reset runtime-required-cordon-after --confirm 2>/dev/null || true
kubectl patch node kind-worker --type=merge \
-p '{"metadata":{"annotations":{"nodewright.nvidia.com/runtimeRequiredCordon":null}},"spec":{"unschedulable":false}}' \
2>/dev/null || true

kubectl label node kind-worker nodewright.nvidia.com/runtime-required-cordon-after-test=true --overwrite
../nodes_add_taint.sh all skyhook.nvidia.com=runtime-required:NoSchedule nodewright.nvidia.com/runtime-required-cordon-after-test=true

# Phase 2: Create the NodeWright with runtimeRequired and runtimeRequiredCordonAfter true
- name: apply-nodewright
try:
- create:
resource:
apiVersion: nodewright.nvidia.com/v1alpha1
kind: NodeWright
metadata:
labels:
app.kubernetes.io/part-of: skyhook-operator
app.kubernetes.io/created-by: skyhook-operator
name: runtime-required-cordon-after
spec:
runtimeRequired: true
runtimeRequiredCordonAfter: true
nodeSelectors:
matchLabels:
nodewright.nvidia.com/runtime-required-cordon-after-test: "true"
packages:
spencer:
version: "3.2.3"
image: ghcr.io/nvidia/skyhook/agentless
env:
- name: SLEEP_LEN
value: "2"

# Phase 3: Wait for the NodeWright to complete on the targeted node
- name: assert-nodewright-complete
try:
- assert:
resource:
apiVersion: v1
kind: Node
metadata:
name: kind-worker
labels:
nodewright.nvidia.com/status_runtime-required-cordon-after: complete
annotations:
nodewright.nvidia.com/status_runtime-required-cordon-after: complete

# Phase 4: Assert the runtime-required taint was removed and the cordon was applied
- name: assert-taint-removed-and-node-cordoned
try:
- assert:
resource:
apiVersion: v1
kind: Node
metadata:
name: kind-worker
annotations:
nodewright.nvidia.com/runtimeRequiredCordon: "true"
spec:
unschedulable: true
(!taints || length(taints[?key == 'skyhook.nvidia.com' && effect == 'NoSchedule' && value == 'runtime-required'])==`0`): true

# Phase 5: External actor releases the cordon
- name: release-cordon
try:
- script:
content: |
kubectl patch node kind-worker --type=merge \
-p '{"metadata":{"annotations":{"nodewright.nvidia.com/runtimeRequiredCordon":null}},"spec":{"unschedulable":false}}'
Comment thread
natherz97 marked this conversation as resolved.

# Phase 6: Assert the cordon annotation is removed and the node is schedulable
- name: assert-cordon-released
try:
- script:
content: |
set -eu
unschedulable=$(kubectl get node kind-worker -o jsonpath='{.spec.unschedulable}' 2>/dev/null || true)
if [ -n "$unschedulable" ] && [ "$unschedulable" != "false" ]; then
kubectl get node kind-worker -o yaml
echo "expected node to be schedulable after cordon release, got unschedulable=${unschedulable}"
exit 1
fi
annotation=$(kubectl get node kind-worker \
-o jsonpath='{.metadata.annotations.nodewright\.nvidia\.com/runtimeRequiredCordon}' 2>/dev/null || true)
if [ -n "$annotation" ]; then
kubectl get node kind-worker -o yaml
echo "expected runtimeRequiredCordon annotation to be absent after cordon release"
exit 1
fi
12 changes: 12 additions & 0 deletions operator/api/nodewright/v1alpha1/nodewright_types.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions operator/api/v1alpha1/nodewright_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ func convertSkyhookSpec(in *SkyhookSpec, out *nwv1.NodeWrightSpec) {
out.Serial = in.Serial
out.RuntimeRequired = in.RuntimeRequired
out.AutoTaintNewNodes = in.AutoTaintNewNodes
out.RuntimeRequiredCordonAfter = in.RuntimeRequiredCordonAfter
out.Priority = in.Priority
out.DeploymentPolicy = in.DeploymentPolicy
out.Sequencing = nwv1.SequencingMode(in.Sequencing)
Expand Down
13 changes: 7 additions & 6 deletions operator/api/v1alpha1/nodewright_conversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,13 @@ func fullSkyhook() *Skyhook {
Annotations: map[string]string{"skyhook.nvidia.com/pause": "true"},
},
Spec: SkyhookSpec{
Serial: true,
RuntimeRequired: true,
AutoTaintNewNodes: true,
Priority: 50,
DeploymentPolicy: "policy-a",
Sequencing: SequencingAll,
Serial: true,
RuntimeRequired: true,
RuntimeRequiredCordonAfter: true,
AutoTaintNewNodes: true,
Priority: 50,
DeploymentPolicy: "policy-a",
Sequencing: SequencingAll,
PodNonInterruptLabels: metav1.LabelSelector{
MatchLabels: map[string]string{"pod": "keep"},
MatchExpressions: []metav1.LabelSelectorRequirement{
Expand Down
12 changes: 12 additions & 0 deletions operator/api/v1alpha1/skyhook_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,14 @@ type SkyhookSpec struct {
//+kubebuilder:default=false
RuntimeRequired bool `json:"runtimeRequired,omitempty"`

// RuntimeRequiredCordonAfter will result in the operator applying a persistent node cordon
// after all runtime-required Skyhooks complete against a given node if the node currently
// has the runtime-required taint applied. The operator will apply a runtimeRequiredCordon
// annotation and mark the given node as unschedulable. This setting is only respected if
// RuntimeRequired is also true.
//+kubebuilder:default=false
RuntimeRequiredCordonAfter bool `json:"runtimeRequiredCordonAfter,omitempty"`

// AutoTaintNewNodes enables the operator to automatically apply the runtime-required taint
// to new nodes that match this Skyhook's node selector. Only meaningful when RuntimeRequired is true.
// A node is considered "new" if it has no skyhook.nvidia.com/* annotations.
Expand Down Expand Up @@ -862,6 +870,10 @@ type State string
const (
METADATA_PREFIX string = "skyhook.nvidia.com"

// The RuntimeRequiredCordonAnnotation annotation and cordon are applied to a node when RuntimeRequiredCordonAfter
// is true, a node completes all runtime-required Skyhooks, and the runtime-required taint is present.
RuntimeRequiredCordonAnnotation = METADATA_PREFIX + "/runtimeRequiredCordon"

StateComplete State = "complete"
StateInProgress State = "in_progress" // this means its actually running, pod started
StateSkipped State = "skipped" // this means this package, stage are skipped mostly for some parts of the lifecycle
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
Expand Down Expand Up @@ -628,6 +627,15 @@ spec:
description: This NodeWright is required to have been completed before
any workloads can start
type: boolean
runtimeRequiredCordonAfter:
default: false
description: |-
RuntimeRequiredCordonAfter will result in the operator applying a persistent node cordon
after all runtime-required NodeWrights complete against a given node if the node currently
has the runtime-required taint applied. The operator will apply a runtimeRequiredCordon
annotation and mark the given node as unschedulable. This setting is only respected if
RuntimeRequired is also true.
type: boolean
sequencing:
default: node
description: |-
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
Expand Down
10 changes: 9 additions & 1 deletion operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
Expand Down Expand Up @@ -628,6 +627,15 @@ spec:
description: This skyhook is required to have been completed before
any workloads can start
type: boolean
runtimeRequiredCordonAfter:
default: false
description: |-
RuntimeRequiredCordonAfter will result in the operator applying a persistent node cordon
after all runtime-required Skyhooks complete against a given node if the node currently
has the runtime-required taint applied. The operator will apply a runtimeRequiredCordon
annotation and mark the given node as unschedulable. This setting is only respected if
RuntimeRequired is also true.
type: boolean
Comment thread
natherz97 marked this conversation as resolved.
sequencing:
default: node
description: |-
Expand Down
1 change: 0 additions & 1 deletion operator/config/rbac/role.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
Expand Down
1 change: 0 additions & 1 deletion operator/config/webhook/manifests.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
Expand Down
25 changes: 23 additions & 2 deletions operator/internal/controller/skyhook_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -3168,8 +3168,19 @@ func (r *SkyhookReconciler) HandleRuntimeRequired(ctx context.Context, clusterSt
// RemoveTaint will ALWAYS return nil for its error so no need to check it
new_node, updated, _ := taints.RemoveTaint(node, &taint_to_remove)
if updated {
err := r.Patch(ctx, new_node, client.MergeFrom(node))
if err != nil {
// If any runtime-required Skyhook sets runtimeRequiredCordonAfter to true, the cordon and
// runtimeRequiredCordon annotation are only applied if the runtime-required taint exists. This means that a
// runtime-required Skyhook with runtimeRequiredCordonAfter true will not apply the persistent cordon if the
// runtime-required taint was already removed. Removing the taint and applying the node cordon in the same patch
// request ensures a scheduling gate is always applied to the targeted node.
if runtimeRequiredCordonAfterEnabled(node_to_skyhooks[node.UID]) {
if new_node.Annotations == nil {
new_node.Annotations = make(map[string]string)
}
new_node.Annotations[v1alpha1.RuntimeRequiredCordonAnnotation] = annotationTrueValue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

new_node.Spec.Unschedulable = true
}
if err := r.Patch(ctx, new_node, client.MergeFrom(node)); err != nil {
errs = append(errs, err)
}
}
Expand All @@ -3180,6 +3191,16 @@ func (r *SkyhookReconciler) HandleRuntimeRequired(ctx context.Context, clusterSt
return nil
}

func runtimeRequiredCordonAfterEnabled(skyhooks []SkyhookNodes) bool {
for _, skyhook := range skyhooks {
spec := skyhook.GetSkyhook().Spec
if spec.RuntimeRequired && spec.RuntimeRequiredCordonAfter {
return true
}
}
return false
}

// Group Skyhooks by what node they target
func groupSkyhooksByNode(clusterState *clusterState) (map[types.UID][]SkyhookNodes, map[types.UID]*corev1.Node) {
node_to_skyhooks := make(map[types.UID][]SkyhookNodes)
Expand Down
Loading
Loading