Repository navigation
Conversation
8d60745 to
5baae47
Compare
235d371 to
43fbecd
Compare
|
Concern: waiting on an external cordon before install means a node cordoned for a suspected hardware fault reboots with no driver, so |
43fbecd to
b39ed89
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 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": |
There was a problem hiding this comment.
🔴 Pending upgrade bypasses administrator cordon
When an administrator cordons a node labeled cordon-required, gpuUpgradeInProgress treats 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-required identifies 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 sets gpu-driver-upgrade-state=cordon-required while waiting to cordon it. The manager starts and proceeds rather than waiting for the administrator to uncordon gpu-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.
| if annotationPresent(annotations, nodeInitialUnschedulableAnnotationKey) || | ||
| annotationPresent(annotations, gpuUpgradeInitialUnschedulableAnnotationKey) || | ||
| annotationPresent(annotations, gpuUpgradeWaitForSafeLoadAnnotationKey) { | ||
| return true |
There was a problem hiding this comment.
🔴 Recorded administrator cordons treated as GPU-owned
When an initial-state annotation says true, gpuCordonClaimed still accepts it as GPU ownership. The value records a pre-existing cordon, so uninstall proceeds on an administrator-cordoned node.
Learn more
The initial-state annotations record the value of spec.unschedulable before a GPU component performed its update. A value of true means the node was cordoned before that update; presence alone cannot identify the current cordon owner. The legacy CordonNode explicitly writes true for an already cordoned node. Treating either key as an unconditional claim bypasses both the initial wait and the acquisition guard.
Example: An administrator cordons gpu-1; an earlier manager writes driver-manager.node-initial-state.unschedulable=true. A new manager sees that annotation and rotates the driver without waiting for the administrator.
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.
| unschedulable := gpuPeerOwnsCordon(node, peerClaimAnnotation) | ||
| if !unschedulable { | ||
| annotations[nodeInitialUnschedulableAnnotationKey] = nil | ||
| } | ||
| return c.patchNodeSchedulingState(node, unschedulable, annotations) |
There was a problem hiding this comment.
🔴 Shared cordon release restores wrong state
If a peer's cordon predates AcquireNodeCordon and clears first, ReleaseNodeCordon sets the node schedulable. It ignores the recorded initial true, undoing a cordon that existed before this manager acquired it.
Learn more
AcquireNodeCordon records node-initial-state.unschedulable=true when it joins a peer's existing cordon. ReleaseNodeCordon bases the final scheduling state only on whether that peer still has a claim. When the peer drops its claim first, the recorded initial state is still true, yet this function removes it and uncordons the node. This can also override an administrator's original cordon if the peer had cordoned an already unschedulable node.
Example: gpu-1 starts unschedulable under a controller claim. The manager acquires, recording true; the controller drops its claim. The manager releases and makes gpu-1 schedulable, although it was unschedulable before the manager joined.
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 !annotationTrue(node.Annotations, claimAnnotation) { | ||
| return nil |
There was a problem hiding this comment.
🔴 Legacy driver cordons stay stuck
If a previous manager cordoned the node, ReleaseNodeCordon finds no new ownership claim and does nothing. A restarted manager's no-driver path completes without uncordoning that node.
Learn more
The previous CordonNode wrote only driver-manager.node-initial-state.unschedulable. On restart, the new uninstallDriver can take the no-driver path and call uncordonNode without acquiring a new claim. ReleaseNodeCordon then returns without cleaning up the old cordon. This leaves the node unschedulable after the successful driver rotation.
Example: The old manager cordons gpu-1, writes initial-state false, and its pod restarts during driver rotation. The replacement sees no loaded driver, finishes cleanup, but leaves gpu-1 cordoned because the new claim annotation is absent.
Recommended fix: Migrate legacy initial-state=false recordings into an ownership claim before the no-driver/skip paths, or explicitly release these legacy cordons when their prior state was schedulable; avoid claiming initial-state=true administrator cordons.
Was this helpful? React with 👍 or 👎 to provide feedback.
b39ed89 to
2a552bb
Compare
| 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 |
There was a problem hiding this comment.
🔴 Pending upgrade bypasses administrator cordon
On an administrator-cordoned node labeled cordon-required, gpuCordonClaimed authorizes rotation before the controller claims the cordon. The manager evicts pods despite the administrator's cordon and can later uncordon that node.
Learn more
An upgrade-state label identifies a stage of work, not the actor that set spec.unschedulable. The state cordon-required can be present before the upgrade controller applies a cordon. WaitUntilNoExternalCordon accepts the label as a GPU claim, and AcquireNodeCordon can then add driver-manager's claim to an administrator's cordon. The upgrade proceeds under the administrator's maintenance cordon; if the label clears before release, driver-manager can also uncordon it.
Example: An administrator cordons gpu-1 while its upgrade label is cordon-required. The driver manager immediately evicts GPU pods rather than waiting for the administrator to uncordon gpu-1.
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.
| if annotationPresent(annotations, nodeInitialUnschedulableAnnotationKey) || | ||
| annotationPresent(annotations, gpuUpgradeInitialUnschedulableAnnotationKey) || | ||
| annotationPresent(annotations, gpuUpgradeWaitForSafeLoadAnnotationKey) { | ||
| return true |
There was a problem hiding this comment.
🔴 Recorded administrator cordon becomes manager-owned
When CordonNode previously recorded true for an administrator-cordoned node, gpuCordonClaimed accepts that recording as ownership. The manager can claim, rotate, and uncordon the administrator's node.
Learn more
The existing CordonNode records the node's prior scheduling state, including true when the node was already cordoned. A recording of true therefore confirms the cordon was present before driver-manager touched the node; it is not a claim of ownership. The new presence check passes that node through WaitUntilNoExternalCordon, and AcquireNodeCordon can claim it. If no peer claim exists at release, ReleaseNodeCordon makes the administrator's node schedulable.
Example: An older driver-manager run records driver-manager.node-initial-state.unschedulable=true on an already cordoned node. A subsequent run sees that annotation, evicts workloads, and releases the administrator's cordon.
Recommended fix: Distinguish a true initial-state recording from ownership. Only an explicit validated owner claim or an initial state of false associated with an active manager cordon can justify proceeding or uncordoning; preserve the original state when releasing.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if node.Spec.Unschedulable { | ||
| return nil |
There was a problem hiding this comment.
🔴 Reapplied administrator cordon inherits stale claim
If an administrator uncordons then recordons a node between manager runs, ClearStaleCordonClaim retains the old claim. The next run treats that administrator cordon as its own and can uncordon it.
Learn more
A 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 gpu-driver-manager.node-cordoned=true. An administrator uncordons gpu-1, then cordons it for maintenance before restarting the driver pod. The manager skips cleanup and later uncordons gpu-1.
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 !annotationTrue(node.Annotations, claimAnnotation) { | ||
| return nil |
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 ReleaseNodeCordon finds no new claim. The no-driver recovery path never acquires one, leaving the node cordoned after installation.
Learn more
Before this change, CordonNode recorded only driver-manager.node-initial-state.unschedulable, not the new ownership annotation. If the driver container restarts after the driver has been unloaded, uninstallDriver takes its no-driver path and later invokes uncordonNode without acquiring the new annotation. The release guard returns without removing the legacy cordon, so scheduling stays disabled.
Example: Version A cordons gpu-1 and records initial state false, then the driver unloads and the pod restarts with version B. Version B completes no-driver cleanup, but its uncordon call does nothing because gpu-driver-manager.node-cordoned is absent.
Recommended fix: Migrate legacy initial-state recordings when safe, especially false on an unschedulable node, before the no-driver release path. Ensure migration does not convert an initial true administrator cordon into a manager-owned cordon.
Was this helpful? React with 👍 or 👎 to provide feedback.
Signed-off-by: Arjun <agadiyar@nvidia.com>
2a552bb to
8a5636b
Compare
This PR is part of this feature request. Essentially we want to ignore nodes that are already cordoned (not by any of the GPU operator components) during upgrades from upgrade controller/k8s-driver-manager.
SOLUTION
internal/kubernetes/client.go
WaitUntilNoExternalCordon polls the node until it is safe to rotate. If schedulable you proceed, if unschedulable and GPU-claimed you proceed as upgrade is already in progress, or if unschedulable and no GPU claim keep waiting (admin cordon). API GET failures are retried. The wait ends when the pod context is cancelled, so an admin-cordoned node holds the init container instead of crashlooping.
AcquireNodeCordon is a conflict-retried update that either writes this component’s claim and sets unschedulable=true in one update, or sees an unclaimed cordon and returns acquired=false without changing the node
ReleaseNodeCordon removes only this component’s claim. The node is uncordoned only if the peer GPU claim is also gone. No claim → no-op, so an admin cordon is left in place.
cmd/driver-manager/main.go - Uninstall always starts with waitForExternalCordonToClear, before host-driver checks, operand pause, drain, or rmmod. That wait is required on every path, including auto-upgrade, where cordonNode is never called because the controller already cordoned and restarted this pod. For cordonNode (drain / eviction path only) loops: wait until the cordon is gone or GPU-claimed, then AcquireNodeCordon. If acquire returns false, an admin recoredoned between the wait and the update; wait again rather than rotate under that cordon. For uncordonNode calls ReleaseNodeCordon. It runs on success, the no-driver path, and failure cleanup. If driver-manager never claimed, the node stays unschedulable.
TESTING:
TestAcquireNodeCordon — claims a schedulable node with annotation plus cordon in one update, and refuses to claim an already-cordoned node that GPU Operator does not own.
TestWaitUntilNoExternalCordonReturnsImmediately — wait returns at once if the node is schedulable or already GPU-claimed, so a normal or in-progress upgrade is not blocked.
TestWaitUntilNoExternalCordonReturnsOnceCleared — on an unclaimed cordon, wait blocks until the node is uncordoned or a GPU claim appears.
TestWaitUntilNoExternalCordonStopsWhenContextCancelled — cancelling the client context ends the wait instead of spinning forever.
TestWaitUntilNoExternalCordonRetriesGetFailures — transient GET failures are retried so a brief API error does not fail uninstall.
TestReleaseNodeCordonPreservesPeerClaim — releasing our claim leaves the node cordoned if the peer GPU component still holds it.