[CASCL-1386] (11/11) Create temporary PodDisruptionBudgets during eviction#3178
Conversation
🛑 Gate Violations
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## lenaic/CASCL-1386-evict-10-karpenter-user #3178 +/- ##
=============================================================================
+ Coverage 44.41% 44.57% +0.15%
=============================================================================
Files 391 391
Lines 31316 31483 +167
=============================================================================
+ Hits 13910 14033 +123
- Misses 16508 16544 +36
- Partials 898 906 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
6a978ac to
1a070c5
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
from
e6ebd52 to
c0b814f
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
1a070c5 to
3cdcaf0
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
2 times, most recently
from
08904e2 to
43a0d78
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
3cdcaf0 to
2b49dd2
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
2b49dd2 to
d88c10a
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
from
43a0d78 to
1505efe
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
d88c10a to
6b94424
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
2 times, most recently
from
045f39b to
f21be8d
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
6b94424 to
d391f26
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
from
f21be8d to
4fbbbac
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
2 times, most recently
from
8b7a293 to
9f7e339
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
2 times, most recently
from
4ba4925 to
e5f9e42
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
9f7e339 to
ee84e12
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
from
e5f9e42 to
bc86be8
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
ee84e12 to
8d436ee
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.
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
8d436ee to
e6bca70
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
from
bc86be8 to
a6e1990
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.
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
from
a6e1990 to
768f105
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
e6bca70 to
7564579
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
from
768f105 to
af9257f
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
7564579 to
2b67827
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.
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.
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
2b67827 to
c79a29e
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-10-karpenter-user
branch
from
af9257f to
72d803c
Compare
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
c79a29e to
071f5d7
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: 071f5d7ed5
ℹ️ 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".
| info, err := resolveTopLevelController(ctx, clientset, pod, depCache, rsCache, stsCache) | ||
| if err != nil { | ||
| log.Printf("Warning: cannot resolve controller for pod %s/%s: %v", pod.Namespace, pod.Name, err) | ||
| return nil |
There was a problem hiding this comment.
When a supported controller lookup fails for reasons other than a deletion race, such as a Forbidden from missing get RBAC on Deployments/ReplicaSets/StatefulSets or a transient API error, this branch only logs and returns nil to the pager. ensureTempPDBs can then succeed while leaving that workload without the requested temporary PDB, and the later drain evicts it as if protection was in place. Propagate non-NotFound lookup errors so users either get the PDBs they asked for or the command fails before draining.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. discoverControllers now distinguishes the deletion race from real failures: an IsNotFound (a Pod's owner deleted mid-scan) is logged and skipped, but any other error is propagated — a Forbidden (missing get RBAC on Deployments/ReplicaSets/StatefulSets) or a transient API error now fails ensureTempPDBs before the drain, rather than silently leaving that workload without its temporary PDB and evicting it as if protected.
if err != nil {
if apierrors.IsNotFound(err) {
log.Printf("Warning: controller for pod %s/%s not found (deleted mid-scan); skipping", pod.Namespace, pod.Name)
return nil
}
return fmt.Errorf("resolve controller for pod %s/%s: %w", pod.Namespace, pod.Name, err)
}Added a test (TestDiscoverControllers_LookupErrors) covering both the NotFound-skip and the Forbidden-propagate paths.
There was a problem hiding this comment.
Pushed in 38c7019.
Note a refinement from a follow-up review pass: the NotFound handling now distinguishes which lookup failed. An IsNotFound on a Pod's direct owner is still skipped (genuine deletion race), but if the owning ReplicaSet's grandparent Deployment returns NotFound (e.g. the Deployment was deleted with --cascade=orphan while its ReplicaSet keeps running), we now fall back to protecting the still-live ReplicaSet rather than skipping the workload. Non-NotFound errors (Forbidden/RBAC, transient, …) still propagate and fail the command before draining, as you asked.
| func tempPDBName(kind, controllerName string) string { | ||
| prefix := strings.ToLower(kind) + "-" + controllerName | ||
| if len(prefix)+len(pdbNameSuffix) > validation.DNS1123LabelMaxLength { | ||
| prefix = prefix[:validation.DNS1123LabelMaxLength-len(pdbNameSuffix)] |
There was a problem hiding this comment.
For two controllers of the same kind in the same namespace whose names exceed the truncation point and share the same leading characters, this produces the same PDB name. The first one creates a temporary PDB, and createTempPDB treats that existing temp PDB as idempotent for the second controller without checking the selector, so one workload is drained without its own PDB. Include a hash or otherwise disambiguate truncated names before appending the suffix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. tempPDBName now appends a short SHA-256 hash of the full base when the name must be truncated, so two controllers of the same kind + namespace whose long names share a truncated prefix no longer collide on the same PDB name (previously the second was treated as idempotent by createTempPDB and drained without its own selector-matching PDB). Names that fit are unchanged.
base := strings.ToLower(kind) + "-" + controllerName
if len(base)+len(pdbNameSuffix) <= validation.DNS1123LabelMaxLength {
return base + pdbNameSuffix
}
const hashLen = 8
sum := sha256.Sum256([]byte(base))
hash := hex.EncodeToString(sum[:])[:hashLen]
keep := validation.DNS1123LabelMaxLength - len(pdbNameSuffix) - hashLen - 1
return base[:keep] + "-" + hash + pdbNameSuffixAdded a test asserting two long names sharing a truncated prefix produce distinct, DNS1123-valid names.
There was a problem hiding this comment.
Pushed in 38c7019 — truncated names now get a short SHA-256 hash suffix, so two controllers of the same kind + namespace sharing a truncated prefix no longer collide on the same PDB name.
| pdbTempLabelKey = "autoscaling.datadoghq.com/temporary-pdb" | ||
| pdbTempLabelValue = "true" |
There was a problem hiding this comment.
This is used for selector so maybe its better to encode the scope of PDB, e.g. autoscaling.datadoghq.com/kubectl-datadog-command=evict-legacy-nodes
| // DaemonSet (skipped before reaching here), Job (TTL), CronJob, | ||
| // custom controllers — none get a temporary PDB. |
There was a problem hiding this comment.
Does it make sense to log skipped owners?
There was a problem hiding this comment.
I fear it could be very verbose.
| return prefix + pdbNameSuffix | ||
| } | ||
|
|
||
| func formatSelector(s *metav1.LabelSelector) string { |
There was a problem hiding this comment.
Why not use k8s native https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#LabelSelector.String ?
There was a problem hiding this comment.
Good catched, I’ve replaced this by: https://github.com/DataDog/datadog-operator/pull/3178/changes#diff-ad787fe13d9b37231306f213b92666b8504219e2a8c04d56b8293f7988f2b156R297
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.
L3n41c
force-pushed
the
lenaic/CASCL-1386-evict-11-pdb
branch
from
071f5d7 to
38c7019
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38c7019f49
ℹ️ 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".
| } | ||
| } | ||
| if !drainOpts.DryRun && len(errs) == 0 { | ||
| log.Printf("Drained %d node(s) from user NodePool %s; Karpenter will terminate their NodeClaims once empty.", len(cordoned), nodePoolName) |
There was a problem hiding this comment.
When the target is a user-managed Karpenter NodePool whose consolidation is disabled or blocked by disruption settings, draining and cordoning the nodes is not enough to make Karpenter terminate the backing NodeClaims/instances. This path can therefore return success and let Run print the final success message while the legacy NodePool still contains cordoned empty nodes; a rerun just drains the same empty nodes again because nothing here deletes or waits on the NodeClaims/Nodes disappearing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex This file isn’t touched by this PR, is it?
There was a problem hiding this comment.
No — cmd/kubectl-datadog/autoscaling/cluster/evict/karpenter_user.go is not touched by this PR’s diff.
I checked the PR base/head diff (8cf0aa8...f650f70), and the only changed files are:
cmd/kubectl-datadog/autoscaling/cluster/evict/pdb.gocmd/kubectl-datadog/autoscaling/cluster/evict/pdb_test.gocmd/kubectl-datadog/autoscaling/cluster/evict/run.gocmd/kubectl-datadog/autoscaling/cluster/evict/run_test.go
The referenced line in karpenter_user.go is existing code: it logs that drained user NodePool nodes will be terminated by Karpenter once empty. cmd/kubectl-datadog/autoscaling/cluster/evict/karpenter_user.goL12-L22
Testing
- ✅
git diff --name-status 8cf0aa852ecd296d09c7d5a89edc5bea336683a3...f650f7006ae89284d1a14111810672faf1dd7777 -- - ✅
git diff --stat 8cf0aa852ecd296d09c7d5a89edc5bea336683a3...f650f7006ae89284d1a14111810672faf1dd7777 -- cmd/kubectl-datadog/autoscaling/cluster/evict/karpenter_user.go - ✅
nl -ba cmd/kubectl-datadog/autoscaling/cluster/evict/karpenter_user.go | sed -n '1,60p'
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f650f7006a
ℹ️ 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".
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.
…to lenaic/CASCL-1386-evict-11-pdb
…into lenaic/CASCL-1386-evict-11-pdb
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5300c5ac10
ℹ️ 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".
| if hasUserPDB(existing, c.PodLabels) { | ||
| continue |
There was a problem hiding this comment.
When a previous interrupted run left our temporary PDB behind and the user has since added their own PDB for the same workload, hasUserPDB returns true here because it ignores temporary PDBs, so this continue leaves the leaked temp PDB in place. The subsequent drain then sees the pod selected by both the user PDB and our stale temp PDB, which the eviction path is explicitly trying to avoid, so crash recovery can get stuck until the user manually deletes our labeled PDB. Clean up or replace our matching temp PDB before skipping creation because of a user PDB.
Useful? React with 👍 / 👎.
3 participants
What does this PR do?
Implements creation and label-based cleanup of temporary PodDisruptionBudgets (
maxUnavailable: 1) that protect cross-group workloads during the drain.Motivation
PR #3026 is too large to review as a single change. This is part 11 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-10-karpenter-user, notmain, so the diff shows only this step's change. Lands last because its controller discovery (discoverControllers) reuses theshouldSkipEvictionpredicate introduced in part 8. 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)