fix: prevent static capacity controllers from modifying NodeClaims during NodePool deletion (kubernetes-sigs#2840)

Author: tallaxes

pkg/controllers/static/deprovisioning/controller.go

@@ -81,7 +81,7 @@ func (c *Controller) Name() string {
 func (c *Controller) Reconcile(ctx context.Context, np *v1.NodePool) (reconcile.Result, error) {
 	ctx = injection.WithControllerName(ctx, c.Name())
 
-	if !nodepoolutils.IsManaged(np, c.cloudProvider) || np.Spec.Replicas == nil {
+	if !nodepoolutils.IsManaged(np, c.cloudProvider) || np.Spec.Replicas == nil || !np.DeletionTimestamp.IsZero() {
 		return reconcile.Result{}, nil
 	}
 

pkg/controllers/static/deprovisioning/suite_test.go

@@ -154,6 +154,43 @@ var _ = Describe("Static Deprovisioning Controller", func() {
 
 			Expect(result.RequeueAfter).To(BeZero())
 		})
+		It("should return early if nodepool is being deleted", func() {
+			nodePool := test.StaticNodePool()
+			nodePool.Spec.Replicas = lo.ToPtr(int64(1))
+
+			// Create 2 nodeclaims (more than desired replicas of 1)
+			nodeClaims, nodes := test.NodeClaimsAndNodes(2, v1.NodeClaim{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						v1.NodePoolLabelKey:            nodePool.Name,
+						v1.NodeInitializedLabelKey:     "true",
+						corev1.LabelInstanceTypeStable: "stable.instance",
+					},
+				},
+				Status: v1.NodeClaimStatus{
+					Capacity: corev1.ResourceList{
+						corev1.ResourceCPU:    resource.MustParse("10"),
+						corev1.ResourceMemory: resource.MustParse("1000Mi"),
+					},
+				},
+			})
+
+			ExpectApplied(ctx, env.Client, nodePool, nodeClaims[0], nodeClaims[1], nodes[0], nodes[1])
+			ExpectDeletionTimestampSet(ctx, env.Client, nodePool)
+
+			// Update cluster state to track the nodes
+			ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeController, nodeClaimStateController, []*corev1.Node{nodes[0], nodes[1]}, []*v1.NodeClaim{nodeClaims[0], nodeClaims[1]})
+			Expect(cluster.Nodes()).To(HaveLen(2))
+			ExpectStateNodePoolCount(cluster, nodePool.Name, 2, 0, 0)
+
+			result := ExpectObjectReconciled(ctx, env.Client, controller, nodePool)
+			Expect(result.RequeueAfter).To(BeZero())
+
+			// Should not delete any NodeClaims since nodepool is being deleted
+			existingNodeClaims := &v1.NodeClaimList{}
+			Expect(env.Client.List(ctx, existingNodeClaims)).To(Succeed())
+			Expect(existingNodeClaims.Items).To(HaveLen(2))
+		})
 		It("should return early if current node count is less than or equal to desired replicas", func() {
 			nodePool := test.StaticNodePool()
 			nodePool.Spec.Replicas = lo.ToPtr(int64(3))

pkg/controllers/static/provisioning/controller.go

@@ -73,7 +73,7 @@ func (c *Controller) Name() string {
 func (c *Controller) Reconcile(ctx context.Context, np *v1.NodePool) (reconcile.Result, error) {
 	ctx = injection.WithControllerName(ctx, c.Name())
 
-	if !nodepoolutils.IsManaged(np, c.cloudProvider) || !np.StatusConditions().Root().IsTrue() || np.Spec.Replicas == nil {
+	if !nodepoolutils.IsManaged(np, c.cloudProvider) || !np.StatusConditions().Root().IsTrue() || np.Spec.Replicas == nil || !np.DeletionTimestamp.IsZero() {
 		return reconcile.Result{}, nil
 	}
 

pkg/controllers/static/provisioning/suite_test.go

@@ -167,6 +167,20 @@ var _ = Describe("Static Provisioning Controller", func() {
 			Expect(env.Client.List(ctx, nodeClaims)).To(Succeed())
 			Expect(nodeClaims.Items).To(HaveLen(0))
 		})
+		It("should return early if nodepool is being deleted", func() {
+			nodePool := test.StaticNodePool()
+			nodePool.Spec.Replicas = lo.ToPtr(int64(1))
+			ExpectApplied(ctx, env.Client, nodePool)
+			ExpectDeletionTimestampSet(ctx, env.Client, nodePool)
+
+			result := ExpectObjectReconciled(ctx, env.Client, controller, nodePool)
+			Expect(result.RequeueAfter).To(BeZero())
+
+			// Should not create any NodeClaims
+			nodeClaims := &v1.NodeClaimList{}
+			Expect(env.Client.List(ctx, nodeClaims)).To(Succeed())
+			Expect(nodeClaims.Items).To(HaveLen(0))
+		})
 		It("should return early if nodepool replicas is nil", func() {
 			nodePool := test.StaticNodePool()
 			nodePool.Spec.Replicas = nil

test/suites/regression/staticcapacity_test.go

@@ -332,8 +332,6 @@ var _ = Describe("StaticCapacity", func() {
 		var dynamicNodePool *v1.NodePool
 		var label map[string]string
 		BeforeEach(func() {
-			// Create a static NodePool
-			nodePool.Spec.Replicas = lo.ToPtr(int64(2))
 			if env.IsDefaultNodeClassKWOK() {
 				nodePool.Spec.Template.Spec.Requirements = append(nodePool.Spec.Template.Spec.Requirements, v1.NodeSelectorRequirementWithMinValues{
 					Key:      corev1.LabelInstanceTypeStable,
@@ -344,26 +342,21 @@ var _ = Describe("StaticCapacity", func() {
 					},
 				})
 			}
+
+			// Copy nodePool before applying static-specific modifications
+			dynamicNodePool = test.NodePool(
+				lo.FromPtr(nodePool),
+				v1.NodePool{ObjectMeta: metav1.ObjectMeta{Name: "dynamic-nodepool"}},
+			)
+
+			// Apply static-specific modifications
+			nodePool.Spec.Replicas = lo.ToPtr(int64(2))
 			nodePool.Spec.Template.Spec.Taints = []corev1.Taint{
 				{
 					Key:    "static",
 					Effect: corev1.TaintEffectNoExecute,
 				},
 			}
-			// Create a dynamic NodePool
-			dynamicNodePool = test.NodePool(v1.NodePool{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "dynamic-nodepool",
-				},
-				Spec: v1.NodePoolSpec{
-					Template: v1.NodeClaimTemplate{
-						Spec: v1.NodeClaimTemplateSpec{
-							Requirements: nodePool.Spec.Template.Spec.Requirements,
-							NodeClassRef: nodePool.Spec.Template.Spec.NodeClassRef,
-						},
-					},
-				},
-			})
 			label = map[string]string{"app": "large-app"}
 		})
 
@@ -475,10 +468,12 @@ var _ = Describe("StaticCapacity", func() {
 			env.EventuallyExpectNodeClaimsReady(finalNodeClaims...)
 
 			// All final nodes should have the new annotation (either newly created or replaced due to drift)
-			nodes := env.EventuallyExpectInitializedNodeCount("==", 4)
-			for _, node := range nodes {
-				Expect(node.Annotations).To(HaveKeyWithValue("drift-trigger", "test-value"))
-			}
+			Eventually(func(g Gomega) {
+				nodes := env.EventuallyExpectInitializedNodeCount("==", 4)
+				for _, node := range nodes {
+					g.Expect(node.Annotations).To(HaveKeyWithValue("drift-trigger", "test-value"))
+				}
+			}).WithTimeout(10 * time.Minute).Should(Succeed())
 		})
 
 		It("should handle NodePool deletion gracefully", func() {
@@ -492,6 +487,9 @@ var _ = Describe("StaticCapacity", func() {
 			// Delete the NodePool
 			env.ExpectDeleted(nodePool)
 
+			// During deletion, no new NodeClaims should be created - count should only decrease
+			env.ConsistentlyExpectNodeClaimCountNotExceed(30*time.Second, 3)
+
 			// All nodes should eventually be cleaned up
 			env.EventuallyExpectInitializedNodeCount("==", 0)
 			env.EventuallyExpectCreatedNodeClaimCount("==", 0)