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 e5d163623c..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 @@ -193,9 +194,22 @@ 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, 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) } func (n *NodePoolState) ensureNodePoolEntry(np string) { diff --git a/pkg/controllers/state/suite_test.go b/pkg/controllers/state/suite_test.go index 2812e470ec..6e7b02d4a3 100644 --- a/pkg/controllers/state/suite_test.go +++ b/pkg/controllers/state/suite_test.go @@ -2727,6 +2727,59 @@ 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)) + }) + + 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 {