Repository navigation
fix: keep OFED DaemonSet while its node pool is momentarily empty - #3362
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if _, live := livePools[poolName]; live { | ||
| // The pool has eligible nodes and still does not want this DaemonSet, so something | ||
| // changed on purpose. | ||
| return 0, nil | ||
| } | ||
|
|
||
| staleSince, err := s.markStaleSince(ctx, ds, now) |
There was a problem hiding this comment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Deleting the DaemonSet on a version change with an empty pool wouldn't give those nodes the new driver; it would give them no driver at all, because no replacement can be rendered while the pool is empty.
There was a problem hiding this comment.
You're right — deleting the old DaemonSet in that situation would leave the affected nodes without any OFED driver, since no replacement can be rendered while the pool is empty. The version-change behavior in this PR is intentional: retain the existing DaemonSet through the grace period, apply the new image in place when the pool reappears, and reap it only if the pool remains absent.
My previous comment incorrectly treated this as an immediate replacement/deletion case; it should be withdrawn.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
918bd57 to
3ac11cd
Compare
|
Addressed both findings (force-pushed
Verification: |
0f17a9d to
4f2a536
Compare
|
Hey @rmilshtein89 thanks for the PR! What will happen if we:
Will the new mofed daemonset (from the second policy) load immediately or will it be prevented from loading by antiAffinity? |
Thanks @almaslennikov — good question, and the answer differs depending on whether the two nodes are in the same pool. policy B's DaemonSet is created immediately, but its pod on node1 is blocked by the anti-affinity until policy A's pod there is fully gone. It's a transient block, never permanent. The anti-affinity is cross-policy by design — it selects on nvidia.com/ofed-driver existence with topologyKey: kubernetes.io/hostname and no namespace restriction, and both policies render into the operator namespace. So it enforces "at most one driver pod per node" regardless of which policy owns it. ds-owner scopes cleanup, not scheduling. And because it's IgnoredDuringExecution, A's running pod is never evicted to make room; B just waits. If node1 and node2 share OS + kernel version (same pool): pool name is osName+osVersion-kernel and the DaemonSet name carries no node identity, so policy A re-renders the same DaemonSet with an updated nodeSelector. planStaleRetention sees it as still desired, so nothing is retained. The DaemonSet controller then removes A's pod from node1 — note updateStrategy: OnDelete doesn't block this, since it only governs pod-template updates on nodes that should run the daemon. B's pod schedules as soon as A's finishes terminating: seconds to a couple of minutes, bounded by terminationGracePeriodSeconds (default 300) while the driver unloads its modules. If they have different kernels or OS versions (different pools): policy A's live pool is now node2's, so its old DaemonSet isn't rendered anymore. planStaleRetention recovers that DaemonSet's pool from its own nodeSelector, finds it absent from livePools, and retains it — the retained DaemonSet keeps its old nodeSelector, so its pod keeps running on node1 and keeps blocking B for up to the full 20m staleOFEDGracePeriod. Policy B reports NotReady that whole time. Worth being upfront that this is a limitation of the current check: it asks "does this DaemonSet's pool have eligible nodes?", which cannot distinguish "the policy was intentionally pointed away from this pool" from "this pool's nodes went momentarily invisible". Both look identical from the object. Deleting A's old DaemonSet by hand short-circuits the wait. |
One OFED DaemonSet is rendered per node pool, and pool membership is re-derived from NFD labels on every reconciliation. A pool therefore has no eligible nodes whenever its nodes are momentarily invisible to the operator: NFD labels missing right after a reboot, an untolerated taint, or an API server blip. An empty pool renders no DaemonSet, and the generic stale-object cleanup read that as "no longer wanted" and deleted a DaemonSet whose workload was still running and still needed. An undesired DaemonSet has two causes that the object alone cannot tell apart -- genuinely unwanted, or pool discovery found no eligible nodes -- and only the second is recoverable. Decide between them by asking whether the DaemonSet's own pool is still live, never by looking at node conditions or pod readiness, which say nothing about intent either. The pool is recovered from the DaemonSet's own nodeSelector, which already pins the three NFD labels that define a pool, so the check needs no stored state and works for DaemonSets rendered by earlier operator versions. When the pool is empty the DaemonSet is kept and stamped with network.nvidia.com/stale-since, and the shared ServiceAccount, RBAC and init container ConfigMap are kept alongside it so the retained pods can restart. The marker lives on the object so the deferral survives operator restarts and leader-election handovers instead of restarting the grace period, which would leave a genuinely retired pool's DaemonSet behind forever. A marker is preferred over a finalizer-guarded deletion because a deletion cannot be abandoned once deletionTimestamp is set, and recovery must be able to abandon it. If the pool comes back the marker is dropped; if the pool is still absent after staleOFEDGracePeriod it is treated as retired and the DaemonSet is deleted. Dropping ofedDriver or deleting the policy still removes the DaemonSet at once, pool or no pool: the deferral only ever covers a DaemonSet the operator stopped rendering because discovery came up empty. A driver version change is applied in place, since a DaemonSet is named after its pool and not after the version. It lands on the next reconciliation while the pool has nodes; while the pool has none there is no rendered DaemonSet to carry the new version, and deleting the old one would take the driver away from nodes that are still running it, so the upgrade waits for the pool and is applied the moment it reappears. Retargeting a NicNodePolicy is intent too and is likewise exempt, because deferring it would hold the old driver pod on a node for the whole grace period, and the driver pods are mutually anti-affine per node, so a policy that had taken that node over could not start there until the deferral expired. A rendered nodeSelector is the policy's own nodeSelector laid over the pool labels, so ask whether a DaemonSet's nodeSelector still covers everything its policy selects for; if it does not, it was built for different nodes and goes to the stale cleanup at once, abandoning a deferral already counting down. The pool labels are not reserved to the manifest -- targeting one OS build is an ordinary thing for a policy to write -- and entries a policy sets count as its own, which is why the question is about covering rather than about subtracting a list of label keys from the rendered selector. Covering is one-directional on purpose: a policy that drops one of several selectors has widened, every node it matched before it matches still, so the pool check alone decides. Node labels changing is not intent either, since a label disappearing is indistinguishable from the transient label loss the deferral absorbs, whereas a policy edit is unambiguous. A NicClusterPolicy has no nodeSelector at all, so its DaemonSets never look retargeted. A passing deadline produces no cluster event, so the OFED state reports its remaining delay up through the state manager and both policy controllers schedule a RequeueAfter for it, including on an otherwise ready reconciliation where nothing else would bring the policy back. Finally, make the driver pod tolerate the NoSchedule halves of the not-ready and unreachable taints. Without them a node going briefly NotReady -- which the driver's own openibd restart can cause -- makes itself ineligible and empties its own pool. The DaemonSet controller injects only the NoExecute halves, so a running pod survives the taint but a restarting one could not be placed again. Node eligibility is judged against what the driver pod tolerates, and the two were kept as separate lists -- one in Go for filtering, one hardcoded in the DaemonSet manifest -- with nothing holding them in agreement. Derive both from a single definition instead: driverPodTolerations is what the pod template is rendered with, and schedulableNodeTolerations is that plus what the DaemonSet controller injects. A filter stricter than scheduling drops nodes that could have run the driver; a looser one claims nodes the pod could never be placed on. Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Ruven Milshtein <rmilshtein@nvidia.com>
72a13c8 to
20d620a
Compare
almaslennikov
left a comment
almaslennikov
left a comment
There was a problem hiding this comment.
@rmilshtein89 Thank you for working on this! LGTM
4 participants
Summary
One OFED DaemonSet is rendered per node pool, and pool membership is re-derived from NFD labels on every reconciliation. A pool therefore has no eligible nodes whenever its nodes are momentarily invisible to the operator — NFD labels missing right after a reboot, an untolerated taint, or an API server blip. An empty pool renders no DaemonSet, and the generic stale-object cleanup read that as "no longer wanted" and deleted a DaemonSet whose workload was still running and still needed.
An undesired DaemonSet has two causes the object alone cannot tell apart: it is genuinely unwanted, or pool discovery found no eligible nodes for it. Only the second is recoverable, so this change decides between them by asking whether the DaemonSet's own pool is still live — never by looking at node conditions or pod readiness, which say nothing about intent either.
nodeSelector, which already pins the three NFD labels (OS name, OS version, full kernel version) that define a pool. The check needs no stored state and works for DaemonSets rendered by earlier operator versions.nodeinfo.PoolNameis extracted so pool naming has a single definition.network.nvidia.com/stale-since(RFC3339 UTC). The shared ServiceAccount, RBAC objects, and init container ConfigMap are kept alongside it so the retained pods can restart. The marker lives on the object so the deferral survives operator restarts and leader-election handovers, instead of restarting the grace period — which would leave a genuinely retired pool's DaemonSet behind forever. A marker is preferred over a finalizer-guarded deletion because a deletion cannot be abandoned oncedeletionTimestampis set, and recovery must be able to abandon it.staleOFEDGracePeriod(20m), the pool is treated as retired and the DaemonSet is deleted.state.RequeueProviderand reports its remaining delay up throughResults.RequeueAfter. Both policy controllers schedule aRequeueAfterfor it, including on an otherwise ready reconciliation — where nothing else would bring the policy back.The generic cleanup in
state_skel.gogains an opt-inretainStaleFuncveto; retained objects are reported as neither stale nor pending removal, so a state that keeps an object does not report itselfnotReadyforever because of it.handleStaleStateObjectskeeps its existing signature and behavior for every other state.What the deferral does and does not cover
Dropping
ofedDriverfrom a policy, or deleting the policy, still removes the DaemonSet at once whether or not the pool has nodes. The deferral only ever covers a DaemonSet the operator stopped rendering because pool discovery came up empty.A driver version change is applied in place, because a DaemonSet is named after its pool (
mofed-<os><ver>-<kernelhash>) and not after the version. While the pool has nodes it lands on the next reconciliation, as before. While the pool has none there is no rendered DaemonSet to carry the new version, and deleting the old one would take the driver away from nodes that are still running it — the failure this change exists to prevent. The upgrade therefore waits and is applied the moment the pool reappears; if the pool never does, the DaemonSet is reaped with it at the end of the grace period.Tolerations
The driver pod now tolerates the
NoSchedulehalves ofnode.kubernetes.io/not-readyandnode.kubernetes.io/unreachable. Without them a node going brieflyNotReady— which the driver's ownopenibdrestart can cause — makes itself ineligible and empties its own pool. The DaemonSet controller injects only theNoExecutehalves, so a running pod survives the taint but a restarting one could not be placed again.Node eligibility is judged against what the pod tolerates, and the two were kept as separate hand-maintained lists — one in Go for filtering, one hardcoded in the DaemonSet manifest — with nothing holding them in agreement. Both are now derived from a single definition:
driverPodTolerations= CR tolerations +operatorDriverPodTolerations(nvidia.com/gpu:NoScheduleand thenot-ready/unreachableNoSchedulepair). This is what the pod template is rendered with; the hardcoded GPU toleration is gone from the manifest.schedulableNodeTolerations=driverPodTolerations+daemonSetControllerTolerations. This is what node eligibility is judged against.Because the filter set is derived from the pod set it is a superset by construction, so it cannot drift into claiming a node the pod could not be scheduled onto. A filter stricter than scheduling would drop nodes that could run the driver; a looser one would claim nodes the pod could never be placed on.
Validation
go build ./...go test ./pkg/state/... ./pkg/nodeinfo/...— includes the newpkg/state/state_ofed_stale_test.gocovering first observation, deferral within the grace period, cancellation when the pool returns, reaping after expiry, persistence of the marker across restarts, an unparsable annotation, a DaemonSet with no pool in itsnodeSelector, immediate deletion on intentional removal, and the three driver-version-change cases (applied in place with a live pool, deferred then applied across an empty-pool window, abandoned with the pool after expiry). Also asserts the rendered DaemonSet's tolerations equaldriverPodTolerationsand are covered byschedulableNodeTolerations.go test ./controllers/...(envtest) — the taint/toleration integration spec asserts the DaemonSet survives a pool-emptying taint with the same UID and gains the annotation, rather than being deleted.gofmt -l ./controllers ./pkg— cleanDocs
docs/heterogeneous-cluster-support.mdgains a "Node Pool Disappearance and Deferred Cleanup" section describing the behavior, the annotation, the grace period, and exactly which deletions are and are not deferred, plus a "Tolerations and Pool Eligibility" section describing the two derived toleration sets.