[CASCL-1386] Document the autoscaling cluster sub-commands - #3404
gh-worker-dd-mergequeue-cf854d[bot] merged 6 commits into
Conversation
Document `evict-legacy-nodes` and `update`, which were both missing from docs/kubectl-plugin.md, and refresh the `install` / `uninstall` sections against the current implementation. - add the `autoscaling cluster --help` listing so the four sub-commands are discoverable from the section header - document `evict-legacy-nodes`: scope, pre-conditions, the three phases, the per-manager capacity retirement, and what the command does not undo - document `update`: the parameters read back from the CloudFormation stack rather than exposed as flags, and the `none` default for --create-karpenter-resources - install: add --install-mode and --fargate-subnets, and describe both authentication modes - uninstall: enumerate the steps, state that Karpenter-provisioned nodes are drained and terminated, and explain why no ownership pre-check is performed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
L3n41c
added
documentation
|
🎯 Code Coverage (details) 🔗 Commit SHA: fe105bc | Docs | View more details | Give us feedback! |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Cut the implementation detail that had crept into install, evict-legacy-nodes and uninstall. Keep only what a user acts on: for install, that there are CloudFormation stacks and a Helm release to inspect when something breaks, rather than the full IRSA / OIDC / Fargate profile / aws-auth setup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@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: 76601a42fe
ℹ️ 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".
| 2. Installs Karpenter via Helm from the OCI registry. By default the controller runs on dedicated Fargate nodes, so that it never runs on the nodes it manages; pass `--install-mode=existing-nodes` to run it on the cluster's own nodes instead. | ||
| 3. Optionally creates `EC2NodeClass` and `NodePool` Karpenter resources, inferred from existing cluster nodes or EKS node groups. | ||
|
|
||
| If something goes wrong, those CloudFormation stacks and that Helm release are the two places to look. The command installs nothing and exits successfully with an explanatory message when EKS auto-mode is active, or when Karpenter is already installed on the cluster. |
There was a problem hiding this comment.
When the existing Karpenter deployment is owned by kubectl-datadog and is in the requested namespace, apply.Run deliberately proceeds past its installation guard, updates the CloudFormation stacks, upgrades the Helm release, and—with install defaults—recreates all Karpenter resources. A user relying on this sentence could rerun install expecting no changes and instead upgrade Karpenter and overwrite manual EC2NodeClass or NodePool edits; describe the no-op as applying only to foreign installations or installations in another namespace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in d81e5db.
Confirmed in cmd/kubectl-datadog/autoscaling/cluster/apply/run.go:153: the guard is if k != nil && (!k.IsOwn() || k.Namespace != opts.KarpenterNamespace), so it only short-circuits for an installation kubectl-datadog does not own, or one it owns in a different namespace. The sentence now scopes the no-op to those two cases and states explicitly that re-running install against its own installation in the requested namespace converges the CloudFormation stacks, upgrades the Helm release and — with the default --create-karpenter-resources=all — re-creates the EC2NodeClass and NodePool resources, discarding manual edits.
| Removes Karpenter and all associated resources from an EKS cluster. Deletes `NodePool` and `EC2NodeClass` resources, waits for the corresponding EC2 instances to terminate, uninstalls the Karpenter Helm release, cleans up IAM roles, and removes the CloudFormation stacks. Only resources originally created by `kubectl datadog` are affected. | ||
| Removes Karpenter and the associated resources from an EKS cluster. Deletes the `NodePool` and `EC2NodeClass` resources it created, waits for the corresponding EC2 instances to terminate, uninstalls the Karpenter Helm release, cleans up IAM, and removes the CloudFormation stacks. | ||
|
|
||
| Every node Karpenter provisioned is drained and terminated in the process, so make sure the cluster has other capacity first. Each step is independent and best-effort, so an interrupted run can simply be re-run. |
There was a problem hiding this comment.
When the cluster also has hand-created or third-party NodePools, deleteKarpenterNodePools selects only resources bearing the two kubectl-datadog ownership labels, and the termination wait similarly considers only those selected pool names. Nodes from other Karpenter NodePools can therefore remain running after the controller is uninstalled, so the unqualified promise that every Karpenter-provisioned node is drained and terminated can leave users expecting a complete EC2 cleanup; scope this statement to nodes provisioned from the resources the command created.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in d81e5db.
Confirmed in cmd/kubectl-datadog/autoscaling/cluster/uninstall/uninstall.go:304: deleteKarpenterNodePools lists with client.MatchingLabels{"app.kubernetes.io/managed-by": "kubectl-datadog", "autoscaling.datadoghq.com/created": "true"}, so hand-created and third-party NodePool resources are never selected and their nodes keep running. The statement is now scoped to nodes provisioned from the NodePool resources the command created, and says explicitly that other NodePool resources and their nodes are left running.
Address Codex review feedback on PR #3404: - install: the guard in apply.Run only short-circuits when the existing Karpenter installation is not owned by kubectl-datadog, or lives in a namespace other than the requested one. Re-running install over its own installation converges the CloudFormation stacks, upgrades the Helm release and, with the default --create-karpenter-resources=all, re-creates the EC2NodeClass and NodePool resources. Say so instead of promising a no-op. - uninstall: deleteKarpenterNodePools selects only NodePools carrying the two kubectl-datadog ownership labels, so hand-created and third-party NodePools and their nodes survive. Scope the termination statement accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d81e5dbad3
ℹ️ 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".
|
|
||
| Migrates a cluster off the node groups Datadog does not manage — EC2 Auto Scaling groups, EKS managed node groups, user Karpenter NodePools and standalone EC2 instances — so that their workloads end up on the Datadog-managed Karpenter NodePools. It scales down the `cluster-autoscaler` if there is one, then cordons and drains each target's nodes and scales the target down to zero, one target at a time. | ||
|
|
||
| Select the targets with `--all` or with one or more `--target`, and preview a run with `--dry-run`. The command displays its plan and asks for confirmation before touching anything. It is re-runnable: a node that fails to drain keeps its workloads and its instance is never terminated, so a later run can pick up where this one stopped. |
There was a problem hiding this comment.
On a normal non-dry-run invocation, evict.Run calls clusterinfo.Persist before displaying the plan and prompting, and that function creates or updates the cluster-info ConfigMap. Therefore, if the user answers “no,” the command has still modified the cluster; describe confirmation as occurring before the destructive migration steps, or move the ConfigMap write after confirmation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in d169eef.
Confirmed in cmd/kubectl-datadog/autoscaling/cluster/evict/run.go: clusterinfo.Persist is called at line 74, while printPlan / promptConfirmation only run at lines 103-108 — so on a non-dry-run invocation the cluster-info ConfigMap is already written when the user is asked, and answering "no" leaves that snapshot refreshed. (The in-code comment above the call makes the same point for the dry-run branch.)
Rather than move the write after confirmation — the snapshot is deliberately taken from the pre-eviction classify result, so deferring it would change what gets recorded — I scoped the doc claim: confirmation now precedes "any destructive step", with the one pre-prompt write called out explicitly and a note that --dry-run skips it.
Address Codex review feedback on PR #3404: evict.Run calls clusterinfo.Persist before printPlan/promptConfirmation, so on a non-dry-run invocation the cluster-info ConfigMap is already written by the time the user is asked. Declining the prompt therefore does not leave the cluster wholly untouched, so "asks for confirmation before touching anything" was wrong. Scope the claim to destructive steps and call out the one pre-prompt write, noting that --dry-run skips it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keep the scoping the review rounds established, but state it in one clause instead of three sentences: install's no-op case folds the foreign-install and wrong-namespace guards into one condition and points at `update`, and uninstall's NodePool scoping becomes a single sentence. Drop the cluster-info ConfigMap caveat from evict-legacy-nodes: the claim only needed the caveat because it said "before touching anything". Saying "before draining anything" is accurate on its own and spares the reader an implementation detail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4420b24c5
ℹ️ 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".
|
|
||
| #### `autoscaling cluster evict-legacy-nodes` | ||
|
|
||
| Migrates a cluster off the node groups Datadog does not manage — EC2 Auto Scaling groups, EKS managed node groups, user Karpenter NodePools and standalone EC2 instances — so that their workloads end up on the Datadog-managed Karpenter NodePools. It scales down the `cluster-autoscaler` if there is one, then cordons and drains each target's nodes and scales the target down to zero, one target at a time. |
There was a problem hiding this comment.
When --target selects only one legacy group, or the cluster retains other schedulable capacity such as Fargate or user Karpenter pools, the evicted pods can be scheduled there instead. The implementation only verifies that a Datadog-managed NodePool exists and cordons the selected target; it adds no scheduling constraint that forces workloads onto that pool. Scope this statement to draining workloads while making Datadog-managed NodePools available, rather than promising where the scheduler places them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in fe105bc.
Verified: there is no NodeSelector, Affinity or Toleration anywhere in cmd/kubectl-datadog/autoscaling/cluster/evict/ — the command only asserts a Datadog-managed NodePool exists (hasDatadogManagedNodePool, run.go:83) and cordons the selected targets. So with --target selecting a subset, or with Fargate or uncordoned user Karpenter pools still schedulable, the scheduler can place evicted pods elsewhere.
Changed "so that their workloads end up on the Datadog-managed Karpenter NodePools" to "so that their workloads can reschedule onto" them — the sentence now describes what the command makes possible rather than guaranteeing a placement the scheduler owns. Kept as a verb change rather than an added caveat, since these descriptions are deliberately being held short.
Address Codex review feedback on PR #3404: the evict package adds no NodeSelector, Affinity or Toleration — it only verifies that a Datadog-managed NodePool exists and cordons the selected targets. With --target selecting a subset, or with Fargate or uncordoned user Karpenter pools still schedulable, evicted pods can land somewhere other than the Datadog NodePools. Say "can reschedule onto" rather than "end up on", so the sentence describes what the command makes possible instead of guaranteeing a placement the scheduler owns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe105bc525
ℹ️ 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".
|
|
||
| #### `autoscaling cluster evict-legacy-nodes` | ||
|
|
||
| Migrates a cluster off the node groups Datadog does not manage — EC2 Auto Scaling groups, EKS managed node groups, user Karpenter NodePools and standalone EC2 instances — so that their workloads can reschedule onto the Datadog-managed Karpenter NodePools. It scales down the `cluster-autoscaler` if there is one, then cordons and drains each target's nodes and scales the target down to zero, one target at a time. |
There was a problem hiding this comment.
When the target is a user-managed Karpenter NodePool, evictKarpenterUserNodePool only cordons and drains its current nodes; it neither changes the NodePool nor deletes its NodeClaims. Consequently, configurations without automatic consolidation can retain the empty instances, and the still-enabled NodePool can provision replacements, so the statement that every target is scaled to zero is incorrect. Qualify this behavior by target type, consistent with the warning below that user NodePools are only drained.
Useful? React with 👍 / 👎.
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
and removed
mergequeue-status: queued
labels
3 participants
What does this PR do?
Documents the
kubectl datadog autoscaling clustersub-commands that were missing fromdocs/kubectl-plugin.md, and refreshes the ones that had drifted from the implementation.autoscaling cluster --helplisting to the section header, so the four sub-commands are discoverable.evict-legacy-nodes: what it migrates, how targets are selected, and the two things worth knowing before running it (the migration is one-way, and a user Karpenter NodePool is drained but not disabled).update, which was already implemented but never documented: the parameters read back from the CloudFormation stack instead of being exposed as flags, and why--create-karpenter-resourcesdefaults tonone.install: adds the missing--install-modeand--fargate-subnetsflags, and points at the CloudFormation stacks and the Helm release as the places to look when an install misbehaves.uninstall: notes that Karpenter-provisioned nodes are drained and terminated, and that an interrupted run can simply be re-run.The descriptions stay deliberately short — enough for a user to know what the command does and what to inspect when it does not, without restating the implementation. All four
consoleblocks were regenerated from the built plugin and match--helpverbatim, minus the global kubeconfig flags that the file already omits by convention.Motivation
evict-legacy-nodeswas implemented across #3160-#3164, #3172-#3178 and #3207 (the split of #3026) but never documented. Reviewing the surrounding sections surfaced thatupdatewas undocumented too, and that theinstallsection predated the Fargate install mode.Additional Notes
Two stale strings remain in
cmd/kubectl-datadog/autoscaling/cluster/evict/evict.goand are reproduced verbatim in the--helpexcerpt, so they are not fixed here:--allexample calls the ASG targets "cluster-autoscaler ASGs", butresolveASGsbuckets a node underasgwhenever it belongs to any Auto Scaling group;--node-timeoutis described as a per-node budget, yetwaitEKSNodegroupEmptyapplies the same value to a whole managed node group.Both are code changes and would be worth a follow-up.
Minimum Agent Versions
None — documentation only.
Describe your test plan
Documentation-only change; no code touched. Verified by diffing every
consoleblock against the real output of the built plugin:Every claim left in the prose was checked against the implementation under
cmd/kubectl-datadog/autoscaling/cluster/.Checklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabel🤖 Generated with Claude Code