[CASCL-1386] Add evict-legacy-nodes subcommand to drain non-Datadog node groups#3026
Draft
L3n41c wants to merge 24 commits into
Draft
[CASCL-1386] Add evict-legacy-nodes subcommand to drain non-Datadog node groups#3026L3n41c wants to merge 24 commits into
L3n41c wants to merge 24 commits into
Conversation
… node groups
Introduces `kubectl datadog autoscaling cluster evict-legacy-nodes`, which
drains workloads from cluster-autoscaler ASGs, EKS managed node groups,
user-created Karpenter NodePools and standalone EC2 instances onto the
Datadog-managed Karpenter NodePools created by `install`, then scales the
legacy groups to zero.
Highlights:
- Re-classifies the cluster (reuses `clusterinfo.Classify`) and updates the
`dd-cluster-info` ConfigMap before any destructive work.
- Scales the legacy cluster-autoscaler Deployment to 0 replicas (opt-out
via `--skip-cluster-autoscaler`).
- Creates temporary `maxUnavailable: 1` PodDisruptionBudgets for workloads
without one; cleanup is label-based, so a SIGKILL'd run is reaped on the
next invocation.
- Evicts in parallel by manager type:
* ASG: cordon + evict + UpdateAutoScalingGroup(0,0,0) (only when all
nodes drained; avoids AZ-rebalance and MinSize/DesiredCapacity
hazards).
* EKS managed node group: UpdateNodegroupConfig(0,0,0), then waits
for the K8s nodes to disappear so the EKS-side drain finishes
before temp PDBs are removed.
* Karpenter user NodePool: cordon + evict (NodePool spec untouched).
* Standalone EC2: cordon + evict + TerminateInstances.
- Pre-flight refuses when no Datadog-managed NodePool exists; warns on
user NodePool weight conflicts.
- Every K8s read-modify-write wrapped in retry.RetryOnConflict.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🛑 Gate Violations
ℹ️ Info🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: e75e7bf | Docs | Datadog PR Page | Give us feedback! |
Trailing alignment / extra blank line cleaned up by `gofmt -s` and `golangci-lint --fix` on three of the evict-package test files. No behavior change; restores `git diff --exit-code` after `make fmt` for the `check_formatting` CI gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds three small test files to lift `evict/` package coverage from 55% to 66% so the patch-coverage gate passes: - `preflight_test.go`: covers `warnKarpenterWeightConflicts` for no Karpenter targets, no conflict, equal-weight conflict, nil-weight defaulting to 0, and an unknown user-NodePool name. - `prompt_test.go`: covers `printPlan` rendering of each section (CA, PDB, per-manager evictions, dry-run skips) and `promptConfirmation`'s y/N/yes parsing. - `pdb_test.go`: covers `uniqueNodes` excluding EKS MNG entries and deduplicating shared node names. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@codex review |
![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: 7ee9656a7e
ℹ️ 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".
P1: `shouldSkipPod` collapsed two distinct concerns — "do not call Evict() on this pod" (drainNode) and "this pod no longer occupies the node" (waitForNodeEmpty). A pod with DeletionTimestamp set was treated as absent, so for ASG/standalone targets the orchestrator could terminate the EC2 instance before the container finished its grace period. Splits the predicate: - `shouldSkipEviction`: skip the Eviction call for DS / mirror / terminating / completed pods. - `podOccupiesNode`: a terminating pod STILL occupies the node and must keep `waitForNodeEmpty` blocking. DS / mirror / completed pods don't. P2: `uniqueNodes` no longer filters out EKS managed node group entries when discovering controllers for temporary PDBs. The orchestrator now blocks on `waitEKSNodegroupEmpty` before cleaning up the PDBs, so EKS observes them throughout its drain. Excluding MNG nodes left a workload whose every replica lived on a single MNG without any PDB protection; EKS could then disrupt all replicas at once. Test coverage: - New `TestPodOccupiesNode` locks in the "terminating pods still occupy the node" semantics. - `TestShouldSkipPod` renamed to `TestShouldSkipEviction`. - `TestUniqueNodes_ExcludesEKSMNG` renamed to `TestUniqueNodes_IncludesAllManagerTypes` and inverted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…can dedup `ExtractEC2InstanceID` and `LabelEKSNodegroup` were introduced in `common/clusterinfo` so the evict package could reuse them, but two more copies of the same `aws:///[^/]+/(i-[0-9a-f]+)$` regex remained inline: in `common/clusterinfo/classify.go:197` and `common/karpenter/fromnodes.go:65`. `common/clusterinfo` already imports `common/karpenter` (for the Karpenter detection helpers), so karpenter can't reach back into clusterinfo without producing an import cycle. Move the helper to a neutral home — `common/aws/node.go` — that both can import. The unit test moves with the function. Net result: one regex definition, four call sites, three packages dedup'd. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing tests under `cmd/kubectl-datadog/autoscaling/cluster/` (e.g.
`install/install_test.go:TestValidate`, `apply/install_mode_test.go`,
`apply/inference_method_test.go`, `apply/create_karpenter_resources_test.go`)
all declare the table inline inside `for _, tc := range []struct{ … }{ … } {`
rather than via an intermediate `tests :=` variable.
Aligns four newly-added tests with that convention:
- `common/aws/node_test.go: TestExtractEC2InstanceID`
- `evict/evict_pods_test.go: TestShouldSkipEviction`, `TestPodOccupiesNode`
- `evict/plan_test.go: TestParseTargetSpec`
- `evict/pdb_test.go: TestTempPDBName`
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`evictASG` extracted the EC2 instance ID and warned on an unexpected providerID, but discarded the ID immediately afterwards. The check was load-bearing in an earlier revision that terminated instances per-instance via `TerminateInstanceInAutoScalingGroup`; after the round-2 Codex review replaced that loop with a single `UpdateAutoScalingGroup(0,0,0)`, the ID is no longer used here and the warning misleadingly suggested otherwise. The `standalone` and `karpenter/fromnodes` call sites still use the extracted ID, so the helper stays. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tASG` The five `TestEvictASG_*` functions shared the same setup (fake clientset + stubAutoscaling), the same call (`evictASG(ctx, client, stub, …)`) and the same axes of assertion (`err`, `stub.scaledASGs`, optionally `Spec.Unschedulable`). They only differed on the initial K8s objects, whether an Eviction reactor was wired, and the drain timeouts. Collapsing them into one `TestEvictASG` with an inline case slice keeps the matrix of (objects × reactor × dry-run × wantErr × wantScaledASGs × wantUnschedulable) visible at a glance, aligns with the convention used in `install/install_test.go`, `apply/install_mode_test.go`, etc., and makes it cheap to add a new scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same shape as the earlier `TestEvictASG` consolidation, applied to the rest of the package. Tests that exercised the same target function with different inputs are now grouped into one `Test<Function>` with an inline case slice; one test per *function* rather than per *scenario* collapses ~65 cousin functions down to ~25. - `clusterautoscaler_test.go`: 5 → 1 (`TestScaleDownClusterAutoscaler`) - `cordon_test.go`: 4 → 1 (`TestCordonNode`) - `eks_mng_test.go`: 5 → 1 (`TestEvictEKSManagedNodeGroup`) - `evict_pods_test.go`: 5 EvictPodWithRetry tests → 1 (`TestEvictPodWithRetry`) with a typed `evictionResponder` closure capturing each scenario's reactor - `karpenter_user_test.go`: 2 → 1 (`TestEvictKarpenterUserNodePool`) - `standalone_test.go`: 5 → 1 (`TestEvictStandalone`) - `preflight_test.go`: 5 conflict tests → 1 (`TestWarnKarpenterWeightConflicts`) - `prompt_test.go`: 4 printPlan + 2 promptConfirmation → 2 (`TestPrintPlan`, `TestPromptConfirmation`) - `run_test.go`: 5 → 1 (`TestEvictAllTargetsParallel`) - `plan_test.go`: 3 BuildPlan tests + the inline-subtests in `TestHasDatadogManagedNodePool` → `TestBuildPlan` + table-driven `TestHasDatadogManagedNodePool` - `pdb_test.go`: per-helper consolidations (`TestUniqueNodes`, `TestIsTemporaryPDB`, `TestHasUserPDB`, `TestCleanupTempPDBs`, `TestCreateTempPDB`); `TestTempPDBName` was already table-driven; `TestDiscoverControllers_FiltersByNodeSet` left as a single scenario since the fixtures don't vary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the hand-rolled for/return-true loop with the stdlib `slices.ContainsFunc` (Go 1.21+). Same semantics, half the lines, no extra dependency — `slices` is already imported elsewhere in the plugin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e boolean expression
Both functions were cascades of `if cond { return true/false }` short-circuiting
the same way `||` and `&&` already do. The two predicates collapse to a single
return line each, which makes the asymmetry between the two — `||` includes
`p.DeletionTimestamp \!= nil`, `&&` doesn't — visible at a glance.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the hand-rolled accumulator loop in `waitForNodeEmpty` with `lo.CountBy`. Same semantics, fewer lines, intent visible at the function name. The stdlib `slices` package has `ContainsFunc` / `IndexFunc` but no `CountFunc`, so `lo.CountBy` is the right call — `lo` is already a project dependency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the if/else cascade in evictPodWithRetry with a tagless switch and move the retry-sleep into the default branch so the four dispatch outcomes (success, 404, non-429 error, deadline) sit as peers next to the retry path. Tightens listPodsOnNode with named returns.
PollInterval is an internal field with a single construction site (run.go) that never set it, leaving two divergent defensive defaults (2s in drainNode, 10s in waitEKSNodegroupEmpty) to paper over the gap. Set it explicitly to 2s in run.go and remove both fallbacks; tests still inject their own short intervals.
Drop the per-manager-type goroutines, mutex and WaitGroup in favour of a plain loop over targets. The original rationale for parallelism was asynchronous EKS draining (UpdateNodegroupConfig returning while EKS worked in the background), but waitEKSNodegroupEmpty now blocks until the EKS-managed nodes disappear, so every manager already runs synchronously. Sequential execution yields linear logs, bounded apiserver pressure, and easier debugging when a target fails.
Realign inline struct literals after recent field-list changes. Adding fields whose name is longer than any existing one widens the column gofmt computes for the whole literal; the previous commits introduced such fields without re-running gofmt, so check_formatting on the CI pipeline reported diffs in five test files. Pure whitespace, no semantic change.
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96d5f50e4f
ℹ️ 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".
Two fixes triggered by Codex review of 96d5f50: 1. EKS managed node group: the API rejects `maxSize < 1`, so the previous `min=max=desired=0` Update would have failed before any drain started. DescribeNodegroup the target first, then preserve its current MaxSize while still zeroing min/desired (defaulting to 1 if the described scaling config is nil). The EKS-side drain behaviour is unchanged — desired=0 still triggers it — but the API call now succeeds. 2. Dry-run: both `clusterinfo.Persist` call sites in Run mutated the cluster-info ConfigMap unconditionally, before any dry-run gate. A preview must not require write RBAC on the Karpenter namespace and must not leave behind state; gate both calls on `\!opts.DryRun` and log the would-be persistence instead. Test updates: extend stubEKS with DescribeNodegroup, add cases for DescribeNodegroup failure (short-circuits Update) and for a nil ScalingConfig (falls back to max=1).
Replace context.Background() with t.Context() across the eviction test files. t.Context() (Go 1.24+) returns a context that is canceled when the test completes, so any goroutine still using the context is notified instead of leaking past the test boundary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…elpers - Embed a controllerKey (Namespace/Kind/Name) into controllerInfo and key the seen map on it with the selector as value, removing the stringly-typed key and the key/value redundancy. - Key the per-kind caches on client.ObjectKey instead of string concatenation. - Collapse getReplicaSet/getDeployment/getStatefulSet into one generic getCached[T] taking the namespace-bound typed accessor as a func value. - Rewrite hasUserPDB with slices.ContainsFunc and formatSelector/discoverControllers map-to-slice loops with lo.MapToSlice; inline uniqueNodes via lo.FlatMap/SliceToMap. - Inline throwaway locals in tempPDBName; drop the redundant client-side label re-check in cleanupTempPDBs (List already filters by MatchingLabels). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The pdb.go refactor (struct-key dedup + generic getCached) rewrote the controllerInfo construction in every arm of resolveTopLevelController, but TestDiscoverControllers_FiltersByNodeSet only exercised the Deployment-via-ReplicaSet path, dropping patch coverage from 100% to 78%. Extend the discovery test to also cover the StatefulSet arm, the bare ReplicaSet arm (no owning Deployment), the default arm (DaemonSet -> no PDB), and the orphan-pod path (no controller owner). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d77c9cc3a7
ℹ️ 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".
Comment on lines
+93
to
+95
| if len(targets) == 0 { | ||
| display.PrintBox(streams.Out, "Nothing to evict — the cluster is already on Datadog-managed Karpenter NodePools.") | ||
| return nil |
There was a problem hiding this comment.
When a previous run leaves temporary PDBs in place because an EKS managed node group timed out, the documented recovery path is to rerun after EKS finishes. In that scenario classification now finds zero legacy targets, so this early return exits before cleanupTempPDBs runs, leaving the temporary maxUnavailable: 1 PDBs in the cluster indefinitely and potentially throttling later rollouts/disruptions. Please run the label-based cleanup on the no-target path when --ensure-pdbs is enabled.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Good catch — fixed in 7093d74. The no-target early-return now runs the idempotent label-based cleanupTempPDBs (when --ensure-pdbs is set) before exiting, so temp PDBs leaked by a prior interrupted run are reclaimed once the cluster has fully migrated. Extracted into a reclaimLeakedTempPDBs helper with unit tests for the enabled/disabled/dry-run branches.
Addresses Codex review on run.go: when a prior run was interrupted (e.g. an EKS managed node group timed out, leaving its temporary PDBs in place for a later rerun), the rerun finds the cluster fully migrated and exits early via the no-target no-op — before the temp-PDB cleanup runs. The leaked maxUnavailable:1 PDBs would then persist indefinitely and throttle future rollouts/disruptions. Run the idempotent, label-based cleanup on the no-target path when --ensure-pdbs is enabled, via a small reclaimLeakedTempPDBs helper, and unit-test all three branches (enabled/disabled/dry-run). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The patch_coverage gate (threshold 80%, carryforward:false) evaluates the head commit's changed lines; reclaimLeakedTempPDBs' error-logging branch was uncovered because cleanupTempPDBs never errored with the plain fake client. Add a case using an interceptor client whose List fails, so the swallow-and-log path is exercised, bringing the helper to 100%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- buildAllPlan: replace the manual key-extract + sort + filter + append loop with lo.FilterMap over slices.Sorted(maps.Keys(bucket)). - printPlan: iterate managers via slices.Sorted(maps.Keys(...)) and inline the grouping with lo.GroupBy, removing the now-trivial single-use groupByManager helper (and its test, which would only exercise lo.GroupBy). - Drop the sort import in both files; add maps/slices (and lo in prompt.go). Behavior-preserving: same sorted-by-name ordering within a manager, same manager string ordering, same ManagedByDatadog filtering and Nodes defensive copy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
kubectl datadog autoscaling cluster evict-legacy-nodes, which migrates workloads off non-Datadog-managed node groups (cluster-autoscaler ASGs, EKS managed node groups, user-created Karpenter NodePools, standalone EC2 instances) onto the Datadog-managed Karpenter NodePools created byinstall, then scales the legacy groups to zero.Jira: CASCL-1386
Builds on: CASCL-1304
Highlights
clusterinfo.Classifyand updates thedd-cluster-infoConfigMap before any destructive work.--skip-cluster-autoscaler).maxUnavailable: 1PodDisruptionBudgets for workloads without one. Cleanup is label-based (app.kubernetes.io/managed-by=kubectl-datadog+autoscaling.datadoghq.com/temporary-pdb=true), so a SIGKILL'd run is reaped by the next invocation.UpdateAutoScalingGroup(0,0,0)(only when all nodes drained — avoids AZ-rebalance andMinSize > DesiredCapacityhazards).UpdateNodegroupConfig(0,0,0), then waits for the K8s nodes carryingeks.amazonaws.com/nodegroup=<name>to disappear so the EKS-side drain finishes before temp PDBs are removed (sentinel error `errEKSDrainIncomplete`).TerminateInstances.retry.RetryOnConflict.Out of scope (by design)
Test plan
-race(go test -race ./cmd/kubectl-datadog/autoscaling/cluster/evict/...).make lint— 0 issues.make kubectl-datadogbuilds.🤖 Generated with Claude Code