Repository navigation
Ignore already cordoned nodes during driver upgrades #282
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,8 +50,16 @@ const ( | |
| computeDomainDRADriverName = "compute-domain." + nvidiaDomainPrefix | ||
| computeDomainDaemonConfigKind = "ComputeDomainDaemonConfig" | ||
|
|
||
| // Presence means a GPU component recorded this cordon; the value is the | ||
| // node's prior unschedulable state, not an ownership flag. | ||
| nodeInitialUnschedulableAnnotationKey = nvidiaDomainPrefix + "/" + "driver-manager.node-initial-state.unschedulable" | ||
|
|
||
| // Signals the upgrade controller (k8s-operator-libs) publishes while a node | ||
| // is mid-upgrade. | ||
| gpuUpgradeInitialUnschedulableAnnotationKey = nvidiaDomainPrefix + "/" + "gpu-driver-upgrade.node-initial-state.unschedulable" | ||
| gpuUpgradeWaitForSafeLoadAnnotationKey = nvidiaDomainPrefix + "/" + "gpu-driver-upgrade.driver-wait-for-safe-load" | ||
| gpuUpgradeStateLabelKey = nvidiaDomainPrefix + "/" + "gpu-driver-upgrade-state" | ||
|
|
||
| kubeClientPollInterval = 5 * time.Second | ||
| ) | ||
|
|
||
|
|
@@ -61,6 +69,9 @@ type Client struct { | |
| log *logrus.Logger | ||
|
|
||
| clientset kubernetes.Interface | ||
| // pollInterval is the wait between retries for long-running watches such as | ||
| // WaitUntilNoExternalCordon. Zero means kubeClientPollInterval. | ||
| pollInterval time.Duration | ||
| } | ||
|
|
||
| // DrainOptions represents the option parameters that can passed to the drain.Helper struct | ||
|
|
@@ -150,6 +161,198 @@ func (c *Client) GetNodeAnnotationValue(nodeName, annotation string) (string, er | |
| return node.Annotations[annotation], nil | ||
| } | ||
|
|
||
| func annotationTrue(annotations map[string]string, key string) bool { | ||
| return annotations[key] == "true" | ||
| } | ||
|
|
||
| func annotationPresent(annotations map[string]string, key string) bool { | ||
| _, ok := annotations[key] | ||
| return ok | ||
| } | ||
|
|
||
| func gpuUpgradeInProgress(labels map[string]string) bool { | ||
| switch labels[gpuUpgradeStateLabelKey] { | ||
| case "cordon-required", "wait-for-jobs-required", "pod-deletion-required", | ||
| "drain-required", "node-maintenance-required", "post-maintenance-required", | ||
| "pod-restart-required", "validation-required", "uncordon-required", | ||
| "upgrade-failed": | ||
| return true | ||
|
Comment on lines
+173
to
+179
There was a problem hiding this comment. 🔴 Pending upgrade bypasses administrator cordon On an administrator-cordoned node labeled Learn moreAn upgrade-state label identifies a stage of work, not the actor that set Example: An administrator cordons Recommended fix: Do not infer cordon ownership from an upgrade state alone. Coordinate an explicit controller ownership marker with the upgrade controller, and require a confirmed owned cordon before allowing a pre-cordoned node through the wait or acquire checks. Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // gpuCordonClaimed reports whether a GPU Operator component owns the cordon. | ||
| // Initial-state keys are presence checks because their values are not ownership. | ||
| func gpuCordonClaimed(node *corev1.Node, claimAnnotation, peerClaimAnnotation string) bool { | ||
| if node == nil { | ||
| return false | ||
| } | ||
| annotations := node.Annotations | ||
| if annotationTrue(annotations, claimAnnotation) || annotationTrue(annotations, peerClaimAnnotation) { | ||
| return true | ||
| } | ||
| if annotationPresent(annotations, nodeInitialUnschedulableAnnotationKey) || | ||
| annotationPresent(annotations, gpuUpgradeInitialUnschedulableAnnotationKey) || | ||
| annotationPresent(annotations, gpuUpgradeWaitForSafeLoadAnnotationKey) { | ||
| return true | ||
|
Comment on lines
+195
to
+198
There was a problem hiding this comment. 🔴 Recorded administrator cordons treated as GPU-owned When an initial-state annotation says Learn moreThe initial-state annotations record the value of Example: An administrator cordons Recommended fix: Distinguish an initial-state recording from an active ownership claim. For legacy recordings, require evidence the node was schedulable before the component cordoned it; for controller ownership use a marker written atomically with its cordon. Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+195
to
+198
There was a problem hiding this comment. 🔴 Recorded administrator cordon becomes manager-owned When Learn moreThe existing CordonNode records the node's prior scheduling state, including Example: An older driver-manager run records Recommended fix: Distinguish a Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| } | ||
| return gpuUpgradeInProgress(node.Labels) | ||
| } | ||
|
|
||
| // gpuPeerOwnsCordon reports whether another GPU component still holds the cordon | ||
| // once this component drops its claim. | ||
| func gpuPeerOwnsCordon(node *corev1.Node, peerClaimAnnotation string) bool { | ||
| if node == nil { | ||
| return false | ||
| } | ||
| annotations := node.Annotations | ||
| if annotationTrue(annotations, peerClaimAnnotation) || | ||
| annotationPresent(annotations, gpuUpgradeInitialUnschedulableAnnotationKey) || | ||
| annotationPresent(annotations, gpuUpgradeWaitForSafeLoadAnnotationKey) { | ||
| return true | ||
| } | ||
| return gpuUpgradeInProgress(node.Labels) | ||
| } | ||
|
|
||
| func initialUnschedulableRecording(node *corev1.Node) (string, bool) { | ||
| _, recorded := node.Annotations[nodeInitialUnschedulableAnnotationKey] | ||
| if recorded && node.Spec.Unschedulable { | ||
| return "", false | ||
| } | ||
| return strconv.FormatBool(node.Spec.Unschedulable), true | ||
| } | ||
|
|
||
| // WaitUntilNoExternalCordon blocks until the node is schedulable or a GPU | ||
| // component has claimed the cordon, or until c.ctx is cancelled. | ||
| func (c *Client) WaitUntilNoExternalCordon(nodeName, claimAnnotation, peerClaimAnnotation string) error { | ||
| interval := kubeClientPollInterval | ||
| if c.pollInterval > 0 { | ||
| interval = c.pollInterval | ||
| } | ||
| loggedWaiting := false | ||
| return wait.PollUntilContextCancel(c.ctx, interval, true, func(ctx context.Context) (bool, error) { | ||
| node, err := c.clientset.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| c.log.Warnf("Failed to inspect cordon on node %s, retrying: %v", nodeName, err) | ||
| return false, nil | ||
| } | ||
| if !node.Spec.Unschedulable || gpuCordonClaimed(node, claimAnnotation, peerClaimAnnotation) { | ||
| if loggedWaiting { | ||
| c.log.Infof("External cordon on node %s has cleared, continuing driver rotation", nodeName) | ||
| } | ||
| return true, nil | ||
| } | ||
| if !loggedWaiting { | ||
| c.log.Infof("Node %s is already cordoned by an external actor, waiting until it is uncordoned or a GPU component claims the cordon", nodeName) | ||
| loggedWaiting = true | ||
| } else { | ||
| c.log.Infof("Still waiting for external cordon on node %s to be released", nodeName) | ||
| } | ||
| return false, nil | ||
| }) | ||
| } | ||
|
|
||
| // AcquireNodeCordon atomically claims the cordon and marks the Node unschedulable. | ||
| // An externally owned cordon is left unchanged and returns false. | ||
| func (c *Client) AcquireNodeCordon(nodeName, claimAnnotation, peerClaimAnnotation string) (bool, error) { | ||
| acquired := false | ||
| err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { | ||
| acquired = false | ||
| node, err := c.clientset.CoreV1().Nodes().Get(c.ctx, nodeName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get node %s: %w", nodeName, err) | ||
| } | ||
|
|
||
| if node.Spec.Unschedulable && !gpuCordonClaimed(node, claimAnnotation, peerClaimAnnotation) { | ||
| return nil | ||
| } | ||
| if node.Spec.Unschedulable && annotationTrue(node.Annotations, claimAnnotation) { | ||
| acquired = true | ||
| return nil | ||
| } | ||
|
|
||
| annotations := map[string]any{ | ||
| claimAnnotation: "true", | ||
| } | ||
| if initialState, write := initialUnschedulableRecording(node); write { | ||
| annotations[nodeInitialUnschedulableAnnotationKey] = initialState | ||
| } | ||
| if err := c.patchNodeSchedulingState(node, true, annotations); err != nil { | ||
| return err | ||
| } | ||
| acquired = true | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to acquire cordon claim on node %s: %w", nodeName, err) | ||
| } | ||
| return acquired, nil | ||
| } | ||
|
|
||
| // ReleaseNodeCordon atomically removes this component's claim, uncordoning the | ||
| // Node only when no other GPU component still owns the cordon. | ||
| func (c *Client) ReleaseNodeCordon(nodeName, claimAnnotation, peerClaimAnnotation string) error { | ||
| err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { | ||
| node, err := c.clientset.CoreV1().Nodes().Get(c.ctx, nodeName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get node %s: %w", nodeName, err) | ||
| } | ||
| if !annotationTrue(node.Annotations, claimAnnotation) { | ||
| return nil | ||
|
Comment on lines
+301
to
+302
There was a problem hiding this comment. 🔴 Legacy driver cordons stay stuck If a previous manager cordoned the node, Learn moreThe previous CordonNode wrote only Example: The old manager cordons Recommended fix: Migrate legacy Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+301
to
+302
There was a problem hiding this comment. 🔴 Legacy cordon remains after interrupted upgrade If an older manager cordoned the node before unloading its driver, the replacement's Learn moreBefore this change, CordonNode recorded only Example: Version A cordons Recommended fix: Migrate legacy initial-state recordings when safe, especially Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| } | ||
| if !node.Spec.Unschedulable { | ||
| return c.clearStaleClaim(node, claimAnnotation) | ||
| } | ||
|
|
||
| annotations := map[string]any{ | ||
| claimAnnotation: nil, | ||
| } | ||
| unschedulable := gpuPeerOwnsCordon(node, peerClaimAnnotation) | ||
| if !unschedulable { | ||
| annotations[nodeInitialUnschedulableAnnotationKey] = nil | ||
| } | ||
| return c.patchNodeSchedulingState(node, unschedulable, annotations) | ||
|
Comment on lines
+311
to
+315
There was a problem hiding this comment. 🔴 Shared cordon release restores wrong state If a peer's cordon predates Learn moreAcquireNodeCordon records Example: Recommended fix: Preserve the initial scheduling state separately from peer ownership and restore it when the final claim is removed. Define how to handle peers that preserve their own initial state so the manager does not erase it. Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to release cordon claim on node %s: %w", nodeName, err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // ClearStaleCordonClaim removes a claim left on a schedulable Node, since | ||
| // `kubectl uncordon` does not clear annotations. A cordoned Node is unchanged. | ||
| func (c *Client) ClearStaleCordonClaim(nodeName, claimAnnotation string) error { | ||
| err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { | ||
| node, err := c.clientset.CoreV1().Nodes().Get(c.ctx, nodeName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get node %s: %w", nodeName, err) | ||
| } | ||
| if node.Spec.Unschedulable { | ||
| return nil | ||
|
Comment on lines
+331
to
+332
There was a problem hiding this comment. 🔴 Reapplied administrator cordon inherits stale claim If an administrator uncordons then recordons a node between manager runs, Learn moreA successful AcquireNodeCordon leaves a manager ownership annotation on the Node until release. If the manager stops, an administrator can uncordon and later cordon that node again without changing the annotation. The next startup sees an already-cordoned Node and skips cleanup, although the active cordon is now external. WaitUntilNoExternalCordon and ReleaseNodeCordon then treat the stale marker as ownership and can remove the administrator's cordon. Example: A failed upgrade leaves Recommended fix: Do not treat an indefinitely persistent claim as evidence of ownership across an unobserved uncordon/recordon cycle. Use a controller-coordinated ownership protocol or an observable cordon generation/lease, and avoid releasing the node when ownership cannot be established. Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| } | ||
| if !annotationPresent(node.Annotations, claimAnnotation) && | ||
| !annotationPresent(node.Annotations, nodeInitialUnschedulableAnnotationKey) { | ||
| return nil | ||
| } | ||
| return c.clearStaleClaim(node, claimAnnotation) | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to clear stale cordon claim on node %s: %w", nodeName, err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // clearStaleClaim drops the claim from a schedulable Node. The resourceVersion | ||
| // precondition makes a concurrent cordon conflict instead of being undone. | ||
| func (c *Client) clearStaleClaim(node *corev1.Node, claimAnnotation string) error { | ||
| c.log.Infof("Node %s is schedulable but carries a cordon claim from an earlier rotation, clearing it", node.Name) | ||
| return c.patchNodeSchedulingState(node, false, map[string]any{ | ||
| claimAnnotation: nil, | ||
| nodeInitialUnschedulableAnnotationKey: nil, | ||
| }) | ||
| } | ||
|
|
||
| // CordonNode atomically records a Node's initial schedulable state and marks it | ||
| // Unschedulable. An existing recording on an Unschedulable Node is retained so | ||
| // that restarting driver-manager does not lose the initial state. | ||
|
|
@@ -159,14 +362,15 @@ func (c *Client) CordonNode(nodeName string) error { | |
| return fmt.Errorf("failed to get node %s: %w", nodeName, err) | ||
| } | ||
|
|
||
| initialState, write := initialUnschedulableRecording(node) | ||
| if node.Spec.Unschedulable && !write { | ||
| return nil | ||
| } | ||
|
|
||
| c.log.Infof("Cordoning node %s and recording initial state in annotation %s=%s", nodeName, nodeInitialUnschedulableAnnotationKey, initialState) | ||
| return c.patchNodeSchedulingState(node, true, map[string]any{ | ||
| nodeInitialUnschedulableAnnotationKey: initialState, | ||
| }) | ||
| } | ||
|
|
||
| // UncordonNode atomically marks a Node as schedulable and removes the initial | ||
|
|
@@ -184,16 +388,16 @@ func (c *Client) UncordonNode(nodeName string) error { | |
| } | ||
|
|
||
| c.log.Infof("Uncordoning node %s", nodeName) | ||
| return c.patchNodeSchedulingState(node, false, map[string]any{ | ||
| nodeInitialUnschedulableAnnotationKey: nil, | ||
| }) | ||
| } | ||
|
|
||
| func (c *Client) patchNodeSchedulingState(node *corev1.Node, unschedulable bool, annotations map[string]any) error { | ||
| patch := map[string]any{ | ||
| "metadata": map[string]any{ | ||
| "resourceVersion": node.ResourceVersion, | ||
| "annotations": annotations, | ||
| }, | ||
| "spec": map[string]any{ | ||
| "unschedulable": unschedulable, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Pending upgrade bypasses administrator cordon
When an administrator cordons a node labeled
cordon-required,gpuUpgradeInProgresstreats the pending state as an existing GPU cordon. Driver rotation proceeds during administrator maintenance.Learn more
The upgrade-state label describes progress, not ownership of
spec.unschedulable.cordon-requiredidentifies the phase before the upgrade controller applies its cordon. A node already cordoned by an administrator can reach that phase without the controller owning the cordon. WaitUntilNoExternalCordon then returns, allowing uninstall to proceed despite the external cordon.Example: An administrator cordons
gpu-1; the controller setsgpu-driver-upgrade-state=cordon-requiredwhile waiting to cordon it. The manager starts and proceeds rather than waiting for the administrator to uncordongpu-1.Recommended fix: Do not accept pre-cordon upgrade phases as evidence of ownership. Use an atomic controller-owned claim when the controller actually cordons the node, and gate both waiting and acquisition on that claim.
Was this helpful? React with 👍 or 👎 to provide feedback.