[CASCL-1386] (6½/11) Implement shared node cordon and drain primitives#3207
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: 39bbf26 | Docs | Datadog PR Page | Give us feedback! |
L3n41c
changed the title
Introduce the reusable node-cordon and pod-eviction/drain primitives that the per-manager evictors (EKS managed node groups, ASG, Karpenter, standalone) build on: - cordonNodes/cordonNode: idempotent, conflict-retrying cordon that treats an already-gone node as a silent skip and returns the cordoned Node objects. - drainNode: evicts every evictable pod (skipping mirror, DaemonSet, completed and terminating pods), then waits for the node to empty. - evictPodWithRetry: PDB-aware eviction that retries on 429 and treats 404 as success. - listPodsOnNode, waitForNodeEmpty and the pod-classification predicates (podOccupiesNode/shouldSkipEviction/isMirrorPod/isDaemonSetPod/isCompleted). These primitives have no production caller yet; the first (evictEKSManagedNode- Group) lands in the next PR in the stack. A temporary blank-identifier reference keeps golangci-lint's `unused` quiet until then (the linter runs with tests = false, so the unit tests do not count as use) and is removed by that PR.
47f7ed9 to
39bbf26
Compare
| cordoned = append(cordoned, node) | ||
| } | ||
| } | ||
| return cordoned, errs |
There was a problem hiding this comment.
There was a problem hiding this comment.
It’s actually convenient to keep a slice at this point as it allows to append other errors at the call site like here: https://github.com/DataDog/datadog-operator/pull/3174/changes#diff-6b899156345b292b05d4951862034449f4a5413e7b2506f88fb821db60bf90bdR27-R38
Thanks to that, we can have, at the end of the cordon+drain sequence a slice of errors with exactly one error per node which couldn’t be either cordoned or drained.
It can potentially be interesting in the future to know precisely how many nodes failed to be cordoned or drained.
| // initialUnschedulable is the node's starting `Spec.Unschedulable`. | ||
| initialUnschedulable bool | ||
| // conflictFirstUpdate forces a Conflict on the first Update so | ||
| // RetryOnConflict has to refetch and re-apply. | ||
| conflictFirstUpdate bool | ||
| // updateNotFound makes the Update return NotFound, simulating the node | ||
| // being deleted between the Get and the Update. | ||
| updateNotFound bool | ||
| dryRun bool | ||
| // wantUpdateCalls is the minimum number of Update invocations | ||
| // expected on the Nodes endpoint. | ||
| wantMinUpdateCalls int | ||
| wantUnschedulable bool | ||
| // wantNilNode expects cordonNode to return a nil Node (the node is gone). | ||
| wantNilNode bool |
There was a problem hiding this comment.
Note: for test suites which vary greatly in setup/behaviour it might be better to use nested t.Runs instead of table tests.
| if shouldSkipEviction(&p) { | ||
| continue | ||
| } | ||
| if err := evictPodWithRetry(ctx, clientset, &p, opts.EvictionTimeout, opts.PollInterval); err != nil { |
There was a problem hiding this comment.
Note: here it would add up retry latency on each pod with pdb, the alternative approach could be to make several passes over the pod list to amortize that.
There was a problem hiding this comment.
Agreed that the current algorithm is very naive and slow.
Processing node by node is also sub-optimal.
In a future version, I’d like to have a queue of “pods to evict” that would allow to constantly have x pods being evicted at a time even if they are spread on different nodes.
2 participants
What does this PR do?
Introduces the shared node-cordon and pod-eviction/drain primitives used by the
evict-legacy-nodescommand's per-manager evictors (EKS managed node groups, ASG, Karpenter, standalone):cordonNodes/cordonNode— idempotent, conflict-retrying cordon that treats an already-gone node as a silent skip and returns the cordonedNodeobjects.drainNode— evicts every evictable pod (skipping mirror, DaemonSet, completed and terminating pods), then waits for the node to empty.evictPodWithRetry— PDB-aware eviction that retries on 429 and treats 404 as success.listPodsOnNode,waitForNodeEmpty, and the pod-classification predicates (podOccupiesNode/shouldSkipEviction/isMirrorPod/isDaemonSetPod/isCompleted).Motivation
Every per-manager evictor needs these primitives. Extracting them into a dedicated PR at the base of the stack keeps each downstream PR focused on its manager-specific logic and lets them all build on a single, reviewed implementation.
Additional Notes
[run] tests = false, so unit-test usage does not count toward theunusedlinter and there is no production caller yet in this PR. A small, documentedvar ( _ = drainNode; _ = cordonNodes )block keepsunusedquiet; it is removed by the next PR in the stack whenevictEKSManagedNodeGroupbecomes the first caller.Minimum Agent Versions
N/A —
kubectl-datadogplugin only; no Datadog Agent or Cluster Agent change.Describe your test plan
Unit tests (
go test ./cmd/kubectl-datadog/autoscaling/cluster/evict/) cover: cordon (idempotent / conflict-retry / already-gone / dry-run), pod eviction (429 retry, 404 success, non-retryable error, 429 exhaustion, zero-interval guard), node-empty polling (empty / becomes-empty / timeout / list-error), pod classification, and thespec.nodeNamefield-selector scoping oflistPodsOnNode.make lintis clean.Checklist
enhancementqa/skip-qalabel