Repository navigation
feat: Add feature-gated pod deletion cost management controller - #2894
Merged
kubernetes-prow[bot] merged 104 commits intoOct 1, 2026
Merged
kubernetes-prow[bot] merged 104 commits into
kubernetes-prow[bot] merged 104 commits into
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nathangeology The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
Contributor
|
Hi @nathangeology. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
size/XXL
3 tasks
nathangeology
added a commit
to nathangeology/karpenter-core
that referenced
this pull request
Apr 17, 2026
…lysis and recommendations (kp-v9h)
nathangeology
added a commit
to nathangeology/karpenter-core
that referenced
this pull request
Apr 17, 2026
…s, missing pieces (kp-2za)
nathangeology
added a commit
to nathangeology/karpenter-core
that referenced
this pull request
Apr 17, 2026
nathangeology
added a commit
to nathangeology/karpenter-core
that referenced
this pull request
Apr 17, 2026
…README, third-party detection, bounded labeling (kp-2iw) Fix test compilation: - Remove RankingStrategyRandom references (symbol removed) - Use NewRankingEngine() with no args (API simplified) - Compute BaseRank dynamically as -len(nodes) (not exported) - Remove PodDeletionCostRankingStrategy from test setup (field removed) - Add PodDeletionCostManagement to test FeatureGates Remove orphaned IPVS files: - Delete ipvs_steadystate.go referencing removed SteadyState labels - Delete all ipvs_*_test.go files left from IPVS feature removal Add third-party annotation conflict detection: - Track lastAssignedValues per pod UID in AnnotationManager - Detect when current value differs from what Karpenter last set - Remove sentinel annotation on conflict, skip pod, emit warning event - Clean up tracking map when pods are deleted Add bounded node labeling with cleanup: - Limit annotation updates to top 50 nodes per cycle - Track previouslyLabeledNodes across reconciles - Clean up pod annotations when nodes drop out of top-N Add tests for new features: - Third-party detection: value modified externally → sentinel removed - Bounded labeling: nodes under limit → all annotated Rewrite README to describe PodCount-only with three-tier drift ranking. Remove all references to Random, LargestToSmallest, SmallestToLargest, UnallocatedVCPUPerPodCost strategies.
nathangeology
force-pushed
the
feat/pod-deletion-cost-management
branch
from
April 20, 2026 19:00
44158c8 to
6c67abd
Compare
nathangeology
marked this pull request as ready for review
April 20, 2026 20:16
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
nathangeology
force-pushed
the
feat/pod-deletion-cost-management
branch
3 times, most recently
from
April 29, 2026 21:22
1e3924f to
5f4c236
Compare
|
|
k8s-ci-robot
added
cncf-cla: no
nathangeology
force-pushed
the
feat/pod-deletion-cost-management
branch
3 times, most recently
from
May 8, 2026 21:29
f12031f to
b63cb9d
Compare
k8s-ci-robot
added
cncf-cla: yes
nathangeology
force-pushed
the
feat/pod-deletion-cost-management
branch
from
May 12, 2026 17:08
beec33f to
5835712
Compare
k8s-ci-robot
removed
the
cncf-cla: yes
Remove comments that restate the code they sit above and keep the ones carrying context the code cannot: rejected alternatives, cross-controller coupling, and upstream constraints. Deleted: the queueItem doc, nodePartition, isGoingAway, tryEnqueueNode, nodeMutatesAnyPod, a duplicated MergeFromWithOptions note, and a duplicate skip-cursor line. Trimmed the narration out of RankNodes, classifyNode, applyPerNodePoolBudget, enqueueAnnotationWrites, sortBySavingsRatio, and the pending-writes gauge, all of which restated their own bodies or Help strings. Fixed stale references that no longer name anything: fetchPDBs (the call is pdb.NewLimits), NodeRank (the struct was removed), shouldSkipUnchanged and UpdatePodDeletionCosts. Dropped line-number citations and a branch-local commit SHA that will not survive a squash. Repaired run-on comment sentences left behind when em-dashes were stripped. Comments only; no behaviour change.
Resolves conflicts with kubernetes-sigs#3311 (node repair as voluntary disruption) and kubernetes-sigs#3312 (terminate-first drift), both merged 2026-09-25. Feature gates, pkg/operator/options/options.go and pkg/test/options.go: TerminateFirstDrift and PodDeletionCostManagement are independent gates that landed in the same struct, the same FEATURE_GATES default string, the same DefaultFeatureGates literal and the same ParseFeatureGates block. Both kept, TerminateFirstDrift first to match main, PodDeletionCostManagement appended. Disruption budget accounting, pkg/controllers/disruption/helpers.go and pkg/controllers/pod/deletioncost/ranking.go: kubernetes-sigs#3311 made the already-disrupting count reason-dependent, excluding merely-NotReady nodes from the Unhealthy budget. This branch had extracted the same loop into the reason-agnostic NodePoolStats / NodePoolStatsFromNodes helpers, so git grafted kubernetes-sigs#3311's reason check into a function with no reason in scope. Both helpers now take a reason and forward it. BuildDisruptionBudgetMapping passes its own reason, which reproduces kubernetes-sigs#3311 exactly. RankNodes computes stats and budget together per reason instead of sharing one snapshot across Drifted and Underutilized.
DerekFrank
reviewed
Sep 29, 2026
| } | ||
|
|
||
| // sortBySavingsRatio orders nodes by disruptionutils.SavingsRatio DESC with a | ||
| // node-name tie-break for determinism. Mirrors |
Collaborator
There was a problem hiding this comment.
we decided we didn't want to pull out the actual function into a helper so theres one source of truth, yea?
DerekFrank
reviewed
Sep 29, 2026
DerekFrank
reviewed
Sep 29, 2026
| // gates reconciles at manager startup, so cache-not-synced does not occur in | ||
| // steady state. | ||
| func classifyNode(ctx context.Context, kubeClient client.Client, clk clock.Clock, node *state.StateNode, nodePoolMap map[string]*v1.NodePool, nodePoolToInstanceTypesMap map[string]map[string]*cloudprovider.InstanceType, pdbs pdb.Limits) nodePartition { | ||
| if isGoingAway(node) { |
Collaborator
There was a problem hiding this comment.
I think we need a check here for node.Managed() and we do nothing in that case
DerekFrank
reviewed
Sep 29, 2026
| return partitionNormal | ||
| } | ||
|
|
||
| // isInstanceTypeUnresolvable reports whether disruption.NewCandidate would |
Collaborator
There was a problem hiding this comment.
I don't love a function that predicts what another function will do
Co-authored-by: Derek Frank <dfrankmn@protonmail.com>
1d4fd31 accepted a review suggestion verbatim. The suggestion was directional pseudocode and did not compile: controller.go:174:32: undefined: i controller.go:174:43: undefined: replicaset Implement the intent. Only kube-controller-manager's ReplicaSet controller reads corev1.PodDeletionCost, and it reads it on the pods it claims by CONTROLLER owner reference. A pod that merely carries a non-controller ReplicaSet reference is never ranked by it, so a write there has no effect. Gate on metav1.GetControllerOfNoCopy rather than scanning every owner reference. Reuse the existing precedent for the kind check: scheduling's defaultconstraints.go already declares appsv1.SchemeGroupVersion.WithKind("ReplicaSet") for the same purpose, so no new string literal. Filter the pod slice once in tryEnqueueNode, before the nodeMutatesAnyPod guard, so the guard, the enqueue loop and the per-cycle cap all read the same set. Filtering at the Add site alone would let a node hosting only non-ReplicaSet pods pass the guard, spend a maxNodesPerCycle slot and inflate nodes_with_pending_annotation_writes while enqueueing nothing. Tests: two specs covering the gate and the cap interaction. Existing specs are unaffected because rsOwnedPod already sets Controller=true on an apps/v1 ReplicaSet reference. go build ./... clean. go vet clean. Package suite 67 passed, 0 failed.
kubernetes-prow
Bot
added
the
needs-rebase
Six conflict hunks, all the same shape and all purely additive. Upstream kubernetes-sigs#3342 added the TerminateFirstRepair feature gate; this branch added PodDeletionCostManagement. Each site lists the gates, so adding one at each end produced a whole-block conflict that gofmt realignment widened. Resolution keeps both gates everywhere, upstream ordering preserved and PodDeletionCostManagement appended last so TerminateFirstDrift and TerminateFirstRepair stay adjacent: pkg/operator/options/options.go FeatureGates struct feature-gates flag default string and help text DefaultFeatureGates() ParseFeatureGates() pkg/test/options.go test.FeatureGates struct the lo.FromPtrOr block in Options() No behavioural decision. The two gates do not interact. Verification on the merge result: go build ./... clean go vet options, test, deletioncost clean gofmt -l on both resolved files clean go test ./pkg/operator/options/... ok go test ./pkg/controllers/pod/deletioncost/... ok go test ./pkg/controllers/disruption/... 369 passed, 1 failed The single disruption failure is Consolidation/DRA "reclaims only the candidate's share of a shared device, leaving the live pod's share (B, partial)" at consolidation_dra_test.go:283. It is pre-existing and not from this merge: the same spec fails in isolation on pristine upstream/main 8de44cf and on the pre-merge branch head e01575c. The file is upstream's, added by kubernetes-sigs#3113, and this merge does not touch it.
kubernetes-prow
Bot
removed
the
needs-rebase
classifyNode calls isGoingAway before ValidateNodeDisruptable can reject
anything. isGoingAway reads StateNode.MarkedForDeletion, which is
markedForDeletion || Deleted(), and Deleted()'s second clause is
in.Node != nil && in.NodeClaim == nil && !in.Node.DeletionTimestamp.IsZero()
StateNode.Managed() is exactly NodeClaim != nil, so that clause fires
precisely for a node Karpenter does not own that carries a deletion
timestamp. classifyNode returned partitionDisrupted for it, i.e. Group A.
Group A is enqueued directly via tryEnqueueNode(..., math.MinInt32, ...)
and does not pass through enqueueCapped, so the node was also exempt from
maxNodesPerCycle. Net effect: any node Karpenter does not own, being
deleted by anyone, got the deletion-cost annotation stamped on every one
of its pods with no per-cycle cap.
ValidateNodeDisruptable does reject NodeClaim == nil with "node isn't
managed by karpenter", but ordering means it never sees the node. Filter
at RankNodes' entry instead, so sortBySavingsRatio, classifyNode and the
budget computation all inherit the exclusion from one place.
Group D is not the right destination: it issues a cleanup write that
clears the annotation, which on a node Karpenter does not own would strip
a value some other actor set. Exclusion has to precede partitioning.
Budgets are unaffected. disruption.NodePoolStatsFromNodes already skips
!node.Managed(), and after the filter the slice is managed-only anyway.
The regression test drives Controller.Reconcile with an unmanaged node
(NodeClaim nil, Node non-nil, deletion timestamp set) hosting one
ReplicaSet-owned pod, and asserts the pod carries no annotation. Before
this change the spec fails with pod-deletion-cost=-2147483648 and the
ranking log reports disruptedNodes=1.
Verification:
go build ./... clean
go vet ./pkg/controllers/pod/deletioncost/... clean
go test ./pkg/controllers/pod/deletioncost/... ok, 68 passed 1 pending
new spec fails before, passes after
Comment-only pass. Added // comment lines across this PR's changed files go from 674 to 209, measured as diff-added lines whose trimmed text starts with // against merge-base 8de44cf. Builds on 6b33468d from the earlier pass and extends it to controller.go (38 -> 22) and controller_test.go (108 -> 39), which that pass deliberately skipped to avoid colliding with in-flight work. A comment survived only if it carries something the code cannot: an external contract (the ReplicaSet controller is the only in-tree reader of pod-deletion-cost; it evicts the lowest value first), a refactor-hostile invariant (lo.GroupBy preserves per-partition order; applyPerNodePoolBudget needs a pre-sorted slice; MergeFromWithOptions captures by pointer), a deliberate absence (why there is no feature-gate spec, why the abort-all shape is not asserted for healthy NodePools), a magic-number justification (2^25 priority band, maxNodesPerCycle against the RFC's 1,500-write target), or regression provenance. Restatements of the next line are gone. Left intact: Apache headers, the gate-semantics docs on EvictionCost and DisruptionCostAnnotationKey, the drift-dedupe TODO, the PENDING queue-defect note, and the comment on the unmanaged-node filter in RankNodes. Pre-existing upstream comments that only appear in the diff because a function moved or a symbol was renamed are untouched: BuildDisruptionBudgets in disruption/helpers.go, the offering-price note in disruption/types.go, and the disruptionCost-range pair in utils/disruption. Verification: comments-only proof go/scanner mode 0 token streams identical per file, directive comments unchanged (see commit body note) gofmt -l pkg/ clean go build ./... clean go vet ./pkg/... clean go test deletioncost ok
…eable The old name and doc comment justified the Group D route by predicting what disruption.NewCandidate would reject. The prediction was wrong for one branch. NewCandidate's only instance-type rejection is a nil NodePool or a nil instance-type map (types.go:178); the instance-type name itself is a bare map lookup (types.go:183), so a missing name leaves instanceType nil and the candidate is still built with Price 0. Calling NewCandidate instead of predicting it is not available here. It publishes user-visible Blocked events on every rejection path (types.go:164, 179, 193), and it calls node.ValidatePodsDisruptable, which classifyDisruptableNode has already called. So the justification is now local and makes no claim about another function. A node with no resolvable instance type gets Price 0 from ResolveOfferingPrice, and groupBC is ordered by savings ratio, so its position in that ordering carries no information. It routes to partitionCleanupOnly, where rank is unused. "unpriceable" is the word already used for this state at pkg/controllers/disruption/types.go:370. No behaviour change. Both branches still return true and the nodes still route to partitionCleanupOnly. Neither branch was covered, so this adds a test that pins them, with a priced control node to show the filter is selective: a node whose instance-type label is absent from its NodePool's map, and a node whose NodePool is absent from the instance-type map, must both land in Group D. isUnpriceable now has full statement coverage; package coverage 93.9% to 94.7%.
…s/pod
The "is this pod ReplicaSet-controlled" determination was written twice.
isControlledByReplicaSet in controller.go did it properly, via
metav1.GetControllerOfNoCopy compared against apps/v1 ReplicaSet.
hasNonRSOwnedPods in ranking.go did it again and worse, comparing owner.Kind to
the bare string "ReplicaSet" inside a loop over every owner reference with no
APIVersion check, so a CRD defining its own ReplicaSet kind would have been
mistaken for apps/v1.
pkg/utils/pod already had the any-owner-reference primitive, IsOwnedBy, with
IsOwnedByStatefulSet, IsOwnedByDaemonSet and IsOwnedByNode composing it. It had
no controller-reference equivalent, which is why controller.go hand-rolled one.
This adds IsControlledBy beside IsOwnedBy, and IsControlledByReplicaSet beside
the other wrappers. Both call sites now compose their own question from the
primitive, and neither walks owner references itself.
The two sites still ask different questions and so still carry different kind
sets. The enqueue gate asks whether the ReplicaSet controller will read the
annotation, over {ReplicaSet}, because kube-controller-manager's ReplicaSet
controller is the only in-tree consumer of pod-deletion-cost. Ranking asks
whether any pod pins the node, over {ReplicaSet, Job, DaemonSet}, because all
three of those controllers replace a deleted pod elsewhere. A node of
all-DaemonSet pods is therefore ranked and enqueues nothing, and both answers
are right for their own question. hasNonRSOwnedPods is renamed hasPinningPods,
because it tolerated Job and DaemonSet and the old name said otherwise.
BEHAVIOUR CHANGE, deliberate, and not a side effect of the extraction. The
pinning check now reads the controller reference instead of any owner reference.
A pod whose only ReplicaSet reference is not its controller reference used to
count as ReplicaSet-owned and did not pin its node. It now pins it, the same as
a bare pod. Only the controller replaces a deleted pod, so a non-controller
reference does not make a pod recreatable, and a pod that no controller claims
sits in exactly the position this function already treated as pinning when there
were no owner references at all. The old any-ref read was wrong in both
directions: it failed to pin a pod nothing controls, and it pinned a node on
account of a pod whose controller is a ReplicaSet but which carries some
unrelated extra reference.
The existing controller spec asserted the old behaviour through its
nonControllerRSPod fixture. That fixture moves to its own spec asserting the new
behaviour, with a control node proving the route is caused by the non-controller
reference. The original spec keeps the Job pod that was already carrying its
load-bearing case. IsControlledBy and hasPinningPods both have full statement
coverage.
This was referenced Oct 1, 2026
kubernetes-prow
Bot
added
the
lgtm
DerekFrank
enabled auto-merge (squash)
October 1, 2026 22:51
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DerekFrank, nathangeology The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
1 of 3 tasks
This was referenced Oct 18, 2026
3 tasks done
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
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.
Pod Deletion Cost Management Controller
RFC: #2935
Status: Implementation aligned with RFC.
What this PR does
Adds a feature-gated controller (
PodDeletionCostManagement) that managescontroller.kubernetes.io/pod-deletion-costannotations on pods running on Karpenter-managed nodes. The controller ranks nodes by consolidation priority and annotates pods so that ReplicaSet scale-down concentrates deletions on nodes Karpenter wants to consolidate.This PR also introduces
karpenter.sh/disruption-costas the user-facing annotation for steering Karpenter consolidation. Customers who today setcontroller.kubernetes.io/pod-deletion-costto influence consolidation should migrate to the new annotation.How it works
controller.kubernetes.io/pod-deletion-costdirectly. There is no overwrite-protection for customer-set PDC values: enabling the gate is the operator's opt-in to Karpenter managing that annotation. Customers steering consolidation are expected to be onkarpenter.sh/disruption-costby then.Feature gate
Gate ON: controller writes
controller.kubernetes.io/pod-deletion-cost; Karpenter's consolidation scoring reads onlykarpenter.sh/disruption-cost.Gate OFF (default): controller does not run; Karpenter's consolidation scoring reads
karpenter.sh/disruption-costfirst and falls back tocontroller.kubernetes.io/pod-deletion-costif the new annotation is absent. This preserves current behavior during the migration window.Key design decisions
karpenter.sh/disruption-cost: User-facing annotation for consolidation steering. The auto-managedpod-deletion-costis not interpreted as consolidation cost when the gate is on.What's included
ranking.go- Four-tier ranking with budget limitingannotation.go- Pod annotation writes (no overwrite-protection per v4)controller.go- Reconciler with ConsolidationState change detection, budget enforcement, 50-node capqueue.go- Controller-runtime-backed fire-and-forget queue for per-pod annotation writes (modeled afterterminator.Queue)metrics.go- Prometheus metrics for ranking duration, pods updated, cycles skippedevents.go- Kubernetes events for ranking completion and failurespkg/utils/disruption/disruption.go-karpenter.sh/disruption-costannotation with feature-gate-aware EvictionCost precedenceFollow-up work (not in this PR)
AI Disclosure
I used an LLM to parallel program with me on this. The original controller (Pod Deletion Cost Management) and its earlier revisions were drafted collaboratively over multiple sessions.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.