From 2ead83c39b19c9af4abed8e48b6187c1ca732eec Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Wed, 19 Aug 2026 12:47:50 -0700 Subject: [PATCH 1/2] fix: preserve PendingDisruption state across informer reconciles Motivation: In a static NodePool, when the disruption controller starts disrupting a candidate NodeClaim it patches its DisruptionReason status condition and calls NodePoolState.MarkNodeClaimPendingDisruption so the candidate is excluded from the "Active" count while its replacement launches. Shortly after, the NodeClaim informer controller reconciles that same status patch and calls Cluster.UpdateNodeClaim, which unconditionally called MarkNodeClaimActive whenever markedForDeletion was false - clobbering PendingDisruption back to Active. This inflates the active count past the NodePool's desired replica count, so the static deprovisioner deletes the just-launched replacement NodeClaim and the disruption/replace cycle stalls with repeated "replacement was deleted, NodeClaim not found" errors. Approach: NodePoolState.UpdateNodeClaim now checks the NodeClaim's own DisruptionReason status condition before re-marking it Active. While the condition is true, the NodeClaim stays out of Active regardless of how many times the informer reconciles it. Keying off the condition (rather than internal PendingDisruption set membership) means this self-heals correctly: if the disruption controller later abandons the command and clears the condition via ClearNodeClaimsCondition, the next informer reconcile of that patch calls MarkNodeClaimActive again, so the NodeClaim doesn't get stuck PendingDisruption forever. Validation: Added a regression test in pkg/controllers/state/suite_test.go that marks a NodeClaim PendingDisruption via the same public API the disruption controller uses, then runs it through the real NodeClaimController reconciler twice: once with the DisruptionReason condition set (asserts it stays PendingDisruption) and once after the condition is cleared (asserts it recovers to Active). Confirmed by temporarily reverting statenodepool.go that this test fails (pendingdisruption count reverts from 1 to 0) without the fix and passes with it restored. Ran: KUBEBUILDER_ASSETS= go test \ ./pkg/controllers/state/... ./pkg/controllers/disruption/... \ ./pkg/controllers/static/... -race -timeout 20m all packages pass. Also ran go build ./..., go vet ./pkg/controllers/state/..., gofmt -l on changed files (clean), and golangci-lint-kube-api-linter run ./pkg/controllers/state/... (0 issues). Did not run the full `make verify` codegen/docgen pipeline since this change touches no generated files, CRDs, or API types. Report: https://github.com/kubernetes-sigs/karpenter/issues/3250 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- pkg/controllers/state/statenodepool.go | 14 +++++++++-- pkg/controllers/state/suite_test.go | 32 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/state/statenodepool.go b/pkg/controllers/state/statenodepool.go index e5d163623c..cb0896fec1 100644 --- a/pkg/controllers/state/statenodepool.go +++ b/pkg/controllers/state/statenodepool.go @@ -193,9 +193,19 @@ func (n *NodePoolState) UpdateNodeClaim(nodeClaim *v1.NodeClaim, markedForDeleti // If our node/nodeclaim is marked for deletion, we need to make sure that we delete it if markedForDeletion { n.MarkNodeClaimDeleting(npName, nodeClaim.Name) - } else { - n.MarkNodeClaimActive(npName, nodeClaim.Name) + return + } + // While the disruption controller has the NodeClaim marked as disrupting (static NodeClaims only, + // see MarkNodeClaimPendingDisruption), it must stay out of Active. Otherwise, an informer reconcile + // of this NodeClaim (e.g. of the DisruptionReason condition patch itself) would race the disruption + // controller and move it back to Active, inflating the running count and causing the static + // deprovisioner to delete the in-flight replacement NodeClaim. We key off of the condition, rather + // than our own PendingDisruption tracking, so that this self-heals once the disruption controller + // clears the condition on an abandoned/failed disruption command (see ClearNodeClaimsCondition). + if nodeClaim.StatusConditions().Get(v1.ConditionTypeDisruptionReason).IsTrue() { + return } + n.MarkNodeClaimActive(npName, nodeClaim.Name) } func (n *NodePoolState) ensureNodePoolEntry(np string) { diff --git a/pkg/controllers/state/suite_test.go b/pkg/controllers/state/suite_test.go index 2812e470ec..1935cb6cf6 100644 --- a/pkg/controllers/state/suite_test.go +++ b/pkg/controllers/state/suite_test.go @@ -2727,6 +2727,38 @@ var _ = Describe("NodePoolState Tracking", func() { Expect(deleting).To(Equal(0)) Expect(pendingdisruption).To(Equal(2)) }) + + It("should not revert a NodeClaim from PendingDisruption back to Active on an unrelated informer reconcile", func() { + cluster.NodePoolState.MarkNodeClaimPendingDisruption(nodePool.Name, nodeClaim.Name) + running, deleting, pendingdisruption := cluster.NodePoolState.GetNodeCount(nodePool.Name) + Expect(running).To(Equal(0)) + Expect(deleting).To(Equal(0)) + Expect(pendingdisruption).To(Equal(1)) + + // Simulate the NodeClaim informer reconciling a status update on the NodeClaim (e.g. the + // Disrupting status condition patch) while it's still PendingDisruption and not yet + // MarkForDeletion'd. This should not move it back into Active. + nodeClaim.StatusConditions().SetTrueWithReason(v1.ConditionTypeDisruptionReason, "Drifted", "Drifted") + ExpectApplied(ctx, env.Client, nodeClaim) + ExpectReconcileSucceeded(ctx, nodeClaimController, client.ObjectKeyFromObject(nodeClaim)) + + running, deleting, pendingdisruption = cluster.NodePoolState.GetNodeCount(nodePool.Name) + Expect(running).To(Equal(0)) + Expect(deleting).To(Equal(0)) + Expect(pendingdisruption).To(Equal(1)) + + // Once the disruption controller abandons the command and clears the DisruptionReason + // condition (state.ClearNodeClaimsCondition), the next informer reconcile should recover + // the NodeClaim back to Active rather than leaving it stuck as PendingDisruption forever. + _ = nodeClaim.StatusConditions().Clear(v1.ConditionTypeDisruptionReason) + ExpectApplied(ctx, env.Client, nodeClaim) + ExpectReconcileSucceeded(ctx, nodeClaimController, client.ObjectKeyFromObject(nodeClaim)) + + running, deleting, pendingdisruption = cluster.NodePoolState.GetNodeCount(nodePool.Name) + Expect(running).To(Equal(1)) + Expect(deleting).To(Equal(0)) + Expect(pendingdisruption).To(Equal(0)) + }) }) Context("DeleteNodeClaim", func() { From b3e8a3d4b3d47c880fd53df3c0c51a8b9e6d8086 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Tue, 25 Aug 2026 03:48:44 -0700 Subject: [PATCH 2/2] fix: address review feedback on PendingDisruption condition check Extract IsPendingDisruption into pkg/utils/nodeclaim so the condition check in UpdateNodeClaim and the disruption controller's own tests share one predicate, correct the code comment's claim about which consumers of GetNodeCount are static-NodePool-scoped, and add a test covering markedForDeletion+DisruptionReason both set to lock in that the deletion branch is checked first. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> --- pkg/controllers/disruption/suite_test.go | 3 ++- pkg/controllers/state/statenodepool.go | 20 ++++++++++++-------- pkg/controllers/state/suite_test.go | 21 +++++++++++++++++++++ pkg/utils/nodeclaim/nodeclaim.go | 11 +++++++++++ 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/pkg/controllers/disruption/suite_test.go b/pkg/controllers/disruption/suite_test.go index 7f1bba667d..aa0b543664 100644 --- a/pkg/controllers/disruption/suite_test.go +++ b/pkg/controllers/disruption/suite_test.go @@ -61,6 +61,7 @@ import ( "sigs.k8s.io/karpenter/pkg/test" . "sigs.k8s.io/karpenter/pkg/test/expectations" disruptionutils "sigs.k8s.io/karpenter/pkg/utils/disruption" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" "sigs.k8s.io/karpenter/pkg/utils/pdb" . "sigs.k8s.io/karpenter/pkg/utils/testing" ) @@ -651,7 +652,7 @@ var _ = Describe("Disruption Taints", func() { }) Expect(nodeClaims).To(HaveLen(1)) Expect(nodeClaims[0].StatusConditions().Get(v1.ConditionTypeDisruptionReason)).ToNot(BeNil()) - Expect(nodeClaims[0].StatusConditions().Get(v1.ConditionTypeDisruptionReason).IsTrue()).To(BeTrue()) + Expect(nodeclaimutils.IsPendingDisruption(nodeClaims[0])).To(BeTrue()) createdNodeClaim := lo.Reject(ExpectNodeClaims(ctx, env.Client), func(nc *v1.NodeClaim, _ int) bool { return nc.Name == nodeClaim.Name diff --git a/pkg/controllers/state/statenodepool.go b/pkg/controllers/state/statenodepool.go index cb0896fec1..085cc4d8a9 100644 --- a/pkg/controllers/state/statenodepool.go +++ b/pkg/controllers/state/statenodepool.go @@ -24,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + nodeclaimutils "sigs.k8s.io/karpenter/pkg/utils/nodeclaim" ) // Currently NodeClaims be in one of these states @@ -195,14 +196,17 @@ func (n *NodePoolState) UpdateNodeClaim(nodeClaim *v1.NodeClaim, markedForDeleti n.MarkNodeClaimDeleting(npName, nodeClaim.Name) return } - // While the disruption controller has the NodeClaim marked as disrupting (static NodeClaims only, - // see MarkNodeClaimPendingDisruption), it must stay out of Active. Otherwise, an informer reconcile - // of this NodeClaim (e.g. of the DisruptionReason condition patch itself) would race the disruption - // controller and move it back to Active, inflating the running count and causing the static - // deprovisioner to delete the in-flight replacement NodeClaim. We key off of the condition, rather - // than our own PendingDisruption tracking, so that this self-heals once the disruption controller - // clears the condition on an abandoned/failed disruption command (see ClearNodeClaimsCondition). - if nodeClaim.StatusConditions().Get(v1.ConditionTypeDisruptionReason).IsTrue() { + // While the disruption controller has the NodeClaim marked as disrupting, it must stay out of + // Active. Otherwise, an informer reconcile of this NodeClaim (e.g. of the DisruptionReason condition + // patch itself) would race the disruption controller and move it back to Active, inflating the + // running count and causing the static deprovisioner to delete the in-flight replacement NodeClaim. + // This applies to any NodeClaim with the condition set, not just ones tracked via + // MarkNodeClaimPendingDisruption (today that's static NodeClaims only, since GetNodeCount is only + // consumed by static-NodePool-scoped logic: the static provisioning/deprovisioning controllers and + // the disruption controller's static drift check). We key off of the condition, rather than our own + // PendingDisruption tracking, so that this self-heals once the disruption controller clears the + // condition on an abandoned/failed disruption command (see ClearNodeClaimsCondition). + if nodeclaimutils.IsPendingDisruption(nodeClaim) { return } n.MarkNodeClaimActive(npName, nodeClaim.Name) diff --git a/pkg/controllers/state/suite_test.go b/pkg/controllers/state/suite_test.go index 1935cb6cf6..6e7b02d4a3 100644 --- a/pkg/controllers/state/suite_test.go +++ b/pkg/controllers/state/suite_test.go @@ -2759,6 +2759,27 @@ var _ = Describe("NodePoolState Tracking", func() { Expect(deleting).To(Equal(0)) Expect(pendingdisruption).To(Equal(0)) }) + + It("should move a NodeClaim to Deleting rather than PendingDisruption when both markedForDeletion and DisruptionReason are set", func() { + cluster.NodePoolState.MarkNodeClaimPendingDisruption(nodePool.Name, nodeClaim.Name) + running, deleting, pendingdisruption := cluster.NodePoolState.GetNodeCount(nodePool.Name) + Expect(running).To(Equal(0)) + Expect(deleting).To(Equal(0)) + Expect(pendingdisruption).To(Equal(1)) + + // The deletion branch in UpdateNodeClaim must be checked before the DisruptionReason + // condition, so a NodeClaim that's both marked for deletion and still carrying the + // condition lands in Deleting, not stuck in PendingDisruption. + nodeClaim.StatusConditions().SetTrueWithReason(v1.ConditionTypeDisruptionReason, "Drifted", "Drifted") + ExpectApplied(ctx, env.Client, nodeClaim) + cluster.MarkForDeletion(nodeClaim.Status.ProviderID) + ExpectReconcileSucceeded(ctx, nodeClaimController, client.ObjectKeyFromObject(nodeClaim)) + + running, deleting, pendingdisruption = cluster.NodePoolState.GetNodeCount(nodePool.Name) + Expect(running).To(Equal(0)) + Expect(deleting).To(Equal(1)) + Expect(pendingdisruption).To(Equal(0)) + }) }) Context("DeleteNodeClaim", func() { diff --git a/pkg/utils/nodeclaim/nodeclaim.go b/pkg/utils/nodeclaim/nodeclaim.go index d45f1f65c9..e9b9a1573c 100644 --- a/pkg/utils/nodeclaim/nodeclaim.go +++ b/pkg/utils/nodeclaim/nodeclaim.go @@ -44,6 +44,17 @@ func IsManaged(nodeClaim *v1.NodeClaim, cp cloudprovider.CloudProvider) bool { }) } +// IsPendingDisruption reports whether the disruption controller has claimed this NodeClaim for an +// in-flight disruption command, as recorded by the DisruptionReason status condition. This is the +// only source of truth for "am I currently being disrupted" that callers outside the disruption +// controller should rely on: the condition and the disruption controller's own MarkNodeClaimPendingDisruption +// bookkeeping are written together when a command starts, and the condition is cleared by +// state.ClearNodeClaimsCondition if the command is abandoned or fails, so checking it self-heals rather +// than requiring separate cleanup. +func IsPendingDisruption(nodeClaim *v1.NodeClaim) bool { + return nodeClaim.StatusConditions().Get(v1.ConditionTypeDisruptionReason).IsTrue() +} + // DisruptionTerminationMode returns the termination_mode metric label value for a // disrupted NodeClaim, derived from its terminationGracePeriod. func DisruptionTerminationMode(nodeClaim *v1.NodeClaim) string {