[CASCL-1386] (8/11) Add node drain primitives and evict ASG nodes#3175
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## lenaic/CASCL-1386-evict-07-eks-mng #3175 +/- ##
======================================================================
+ Coverage 44.12% 44.36% +0.24%
======================================================================
Files 389 391 +2
Lines 31132 31296 +164
======================================================================
+ Hits 13737 13886 +149
- Misses 16501 16511 +10
- Partials 894 899 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: 3757a85 | Docs | Datadog PR Page | Give us feedback! |
22c07f1 to
5bad6e2
Compare
c0c8b6a to
9250b71
Compare
5bad6e2 to
e2962ee
Compare
9250b71 to
f9c10d1
Compare
e2962ee to
0b04574
Compare
f9c10d1 to
8e43ad1
Compare
1e9f157 to
ef93860
Compare
8e43ad1 to
3718447
Compare
ef93860 to
bc40e36
Compare
6c79f69 to
0816a9f
Compare
1b6d08b to
9b63eda
Compare
f91f53d to
7322549
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b63edab8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // return values); a node that fails to cordon is recorded in errs and left out | ||
| // of cordoned so the caller never drains a node that can still receive pods. | ||
| func cordonNodes(ctx context.Context, clientset kubernetes.Interface, nodes []string, dryRun bool) (cordoned []*corev1.Node, errs []error) { | ||
| for _, nodeName := range nodes { |
There was a problem hiding this comment.
Maybe use https://pkg.go.dev/golang.org/x/sync/errgroup here to parallelize/collect errors?
There was a problem hiding this comment.
The benefit of parallelizing the cordoning in term of wall clock time would be minimal because cordoning the nodes is blazing fast compared to draining their pods.
And I’d prefer to keep nodes draining sequential in this first iteration to:
- minimize the disruption caused to the customer’s workload,
- minimize the stress caused to the API server.
Thanks to the PDB protection, it should theoretically be possible to parallelize the drain of all the nodes, but I’m a bit afraid of the load it might generate on the API server.
Using errgroup would also change the way errors are managed.
Current behavior is best-effort: if cordoning a single node fails, we register that error to surface it later, but we keep on cordoning the remaining nodes.
errgroup behavior is more fail-early: it makes all the goroutines abort as soon as one of them returns an error.
In order to avoid too many back and forth with the user, I think it’s better to clean up as much nodes as possible before asking the user to manually fix something and relaunch the script.
There was a problem hiding this comment.
errgroup behavior is more fail-early
You can code worker to not return error but, yeah, I agree its better start simple and then optimize the bottlenecks.
3d6b702 to
409d632
Compare
9b63eda to
7dc54d8
Compare
409d632 to
5884694
Compare
7dc54d8 to
21505f9
Compare
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.
Part of a stack splitting #3026 (too large to review in one piece) into small pieces that each build and pass tests on their own. The command is fully functional only once the whole stack lands.
5884694 to
eec5e58
Compare
21505f9 to
b346c58
Compare
Part of a stack splitting #3026 (too large to review in one piece) into small pieces that each build and pass tests on their own. The command is fully functional only once the whole stack lands.
b346c58 to
3757a85
Compare
4 participants
What does this PR do?
Introduces the shared node-drain primitives — node cordoning (
cordonNodes), pod eviction/draining (drainNode) and the pod-eligibility predicates (shouldSkipEviction…) — together with their first consumer, ASG eviction (cordon, drain, terminate the instances, lock the ASG to 0).Motivation
PR #3026 is too large to review as a single change. This is part 8 of 11 of a stack that splits it into small, individually-reviewable pieces that each build and pass tests on their own. See #3026 for the full feature context and end-to-end QA.
Additional Notes
Stacked PR — the base branch is
lenaic/CASCL-1386-evict-07-eks-mng, notmain, so the diff shows only this step's change. The drain primitives are bundled with the ASG evictor on purpose:golangci-lint'sunusedlinter rejects an unexported helper that has no caller in the same commit, so the primitives land in the same PR as the first evictor that calls them. Parts 9–10 reuse them. The code is taken verbatim from #3026 (rebased ontomain); there are no logic changes versus #3026.Minimum Agent Versions
N/A —
kubectl-datadogplugin only.Describe your test plan
go test ./cmd/kubectl-datadog/autoscaling/cluster/...passes at this commit. Full end-to-end QA is tracked on #3026.Checklist
qa/skip-qalabel applied (not independently shippable; QA tracked on [CASCL-1386] Add evict-legacy-nodes subcommand to drain non-Datadog node groups #3026)