fix: ensure node leases aren't leaked (#1807)

pkg/controllers/nodeclaim/lifecycle/controller.go

@@ -173,28 +173,33 @@ func (c *Controller) finalize(ctx context.Context, nodeClaim *v1.NodeClaim) (rec
 		return reconcile.Result{}, fmt.Errorf("adding nodeclaim terminationGracePeriod annotation, %w", err)
 	}
 
-	nodes, err := nodeclaimutil.AllNodesForNodeClaim(ctx, c.kubeClient, nodeClaim)
-	if err != nil {
-		return reconcile.Result{}, err
-	}
-	for _, node := range nodes {
-		// If we still get the Node, but it's already marked as terminating, we don't need to call Delete again
-		if !node.DeletionTimestamp.IsZero() {
-			continue
-		}
-		// We delete nodes to trigger the node finalization and deletion flow
-		if err = c.kubeClient.Delete(ctx, node); client.IgnoreNotFound(err) != nil {
+	// Only delete Nodes if the NodeClaim has not been registered. Deleting Node's without the termination finalizer
+	// may result in leaked leases due to a kubelet bug until k8s 1.29. The Node should be garbage collected after the
+	// instance is terminated by CCM.
+	// Upstream Kubelet Fix: https://github.com/kubernetes/kubernetes/pull/119661
+	if nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue() {
+		nodes, err := nodeclaimutil.AllNodesForNodeClaim(ctx, c.kubeClient, nodeClaim)
+		if err != nil {
 			return reconcile.Result{}, err
 		}
+		for _, node := range nodes {
+			// If we still get the Node, but it's already marked as terminating, we don't need to call Delete again
+			if !node.DeletionTimestamp.IsZero() {
+				continue
+			}
+			// We delete nodes to trigger the node finalization and deletion flow
+			if err = c.kubeClient.Delete(ctx, node); client.IgnoreNotFound(err) != nil {
+				return reconcile.Result{}, err
+			}
+		}
+		// We wait until all the nodes associated with this nodeClaim have completed their deletion before triggering the finalization of the nodeClaim
+		if len(nodes) > 0 {
+			return reconcile.Result{}, nil
+		}
 	}
-	// We wait until all the nodes associated with this nodeClaim have completed their deletion before triggering the finalization of the nodeClaim
-	if len(nodes) > 0 {
-		return reconcile.Result{}, nil
-	}
-	var isInstanceTerminated bool
 	// We can expect ProviderID to be empty when there is a failure while launching the nodeClaim
 	if nodeClaim.Status.ProviderID != "" {
-		isInstanceTerminated, err = terminationutil.EnsureTerminated(ctx, c.kubeClient, nodeClaim, c.cloudProvider)
+		isInstanceTerminated, err := terminationutil.EnsureTerminated(ctx, c.kubeClient, nodeClaim, c.cloudProvider)
 		if err != nil {
 			// 404 = the nodeClaim no longer exists
 			if errors.IsNotFound(err) {
@@ -220,7 +225,7 @@ func (c *Controller) finalize(ctx context.Context, nodeClaim *v1.NodeClaim) (rec
 		// can cause races due to the fact that it fully replaces the list on a change
 		// Here, we are updating the finalizer list
 		// https://github.com/kubernetes/kubernetes/issues/111643#issuecomment-2016489732
-		if err = c.kubeClient.Patch(ctx, nodeClaim, client.MergeFromWithOptions(stored, client.MergeFromWithOptimisticLock{})); err != nil {
+		if err := c.kubeClient.Patch(ctx, nodeClaim, client.MergeFromWithOptions(stored, client.MergeFromWithOptimisticLock{})); err != nil {
 			if errors.IsConflict(err) {
 				return reconcile.Result{Requeue: true}, nil
 			}

pkg/controllers/nodeclaim/lifecycle/initialization.go

@@ -48,6 +48,9 @@ func (i *Initialization) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim)
 	if !nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized).IsUnknown() {
 		return reconcile.Result{}, nil
 	}
+	if !nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue() {
+		return reconcile.Result{}, nil
+	}
 	ctx = log.IntoContext(ctx, log.FromContext(ctx).WithValues("provider-id", nodeClaim.Status.ProviderID))
 	node, err := nodeclaimutil.NodeForNodeClaim(ctx, i.kubeClient, nodeClaim)
 	if err != nil {

pkg/controllers/nodeclaim/lifecycle/initialization_test.go

@@ -88,6 +88,57 @@ var _ = Describe("Initialization", func() {
 		Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeRegistered).Status).To(Equal(metav1.ConditionTrue))
 		Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeInitialized).Status).To(Equal(metav1.ConditionTrue))
 	})
+	It("shouldn't consider the nodeClaim initialized when it has not registered", func() {
+		nodeClaim := test.NodeClaim(v1.NodeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					v1.NodePoolLabelKey: nodePool.Name,
+				},
+			},
+			Spec: v1.NodeClaimSpec{
+				Resources: v1.ResourceRequirements{
+					Requests: corev1.ResourceList{
+						corev1.ResourceCPU:    resource.MustParse("2"),
+						corev1.ResourceMemory: resource.MustParse("50Mi"),
+						corev1.ResourcePods:   resource.MustParse("5"),
+					},
+				},
+			},
+		})
+		ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+
+		node := test.Node(test.NodeOptions{
+			ProviderID: nodeClaim.Status.ProviderID,
+		})
+		ExpectApplied(ctx, env.Client, node)
+
+		_ = ExpectObjectReconcileFailed(ctx, env.Client, nodeClaimController, nodeClaim)
+		ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint
+
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeRegistered).Status).To(Equal(metav1.ConditionFalse))
+		Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeInitialized).Status).To(Equal(metav1.ConditionUnknown))
+
+		node = ExpectExists(ctx, env.Client, node)
+		node.Status.Capacity = corev1.ResourceList{
+			corev1.ResourceCPU:    resource.MustParse("10"),
+			corev1.ResourceMemory: resource.MustParse("100Mi"),
+			corev1.ResourcePods:   resource.MustParse("110"),
+		}
+		node.Status.Allocatable = corev1.ResourceList{
+			corev1.ResourceCPU:    resource.MustParse("8"),
+			corev1.ResourceMemory: resource.MustParse("80Mi"),
+			corev1.ResourcePods:   resource.MustParse("110"),
+		}
+		ExpectApplied(ctx, env.Client, node)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeRegistered).Status).To(Equal(metav1.ConditionFalse))
+		Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeInitialized).Status).To(Equal(metav1.ConditionUnknown))
+	})
 	It("should add the initialization label to the node when the nodeClaim is initialized", func() {
 		nodeClaim := test.NodeClaim(v1.NodeClaim{
 			ObjectMeta: metav1.ObjectMeta{

pkg/controllers/nodeclaim/lifecycle/termination_test.go

@@ -77,6 +77,9 @@ var _ = Describe("Termination", func() {
 
 		node := test.NodeClaimLinkedNode(nodeClaim)
 		ExpectApplied(ctx, env.Client, node)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue())
 
 		// Expect the node and the nodeClaim to both be gone
 		Expect(env.Client.Delete(ctx, nodeClaim)).To(Succeed())
@@ -166,10 +169,16 @@ var _ = Describe("Termination", func() {
 		_, err := cloudProvider.Get(ctx, nodeClaim.Status.ProviderID)
 		Expect(err).ToNot(HaveOccurred())
 
+		// First register a single Node to ensure the NodeClaim can successfully register, then apply the remaining nodes.
 		node1 := test.NodeClaimLinkedNode(nodeClaim)
+		ExpectApplied(ctx, env.Client, node1)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue())
+
 		node2 := test.NodeClaimLinkedNode(nodeClaim)
 		node3 := test.NodeClaimLinkedNode(nodeClaim)
-		ExpectApplied(ctx, env.Client, node1, node2, node3)
+		ExpectApplied(ctx, env.Client, node2, node3)
 
 		// Expect the node and the nodeClaim to both be gone
 		Expect(env.Client.Delete(ctx, nodeClaim)).To(Succeed())
@@ -199,6 +208,9 @@ var _ = Describe("Termination", func() {
 
 		node := test.NodeClaimLinkedNode(nodeClaim)
 		ExpectApplied(ctx, env.Client, node)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue())
 
 		Expect(env.Client.Delete(ctx, nodeClaim)).To(Succeed())
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // triggers the node deletion
@@ -255,6 +267,9 @@ var _ = Describe("Termination", func() {
 
 		node := test.NodeClaimLinkedNode(nodeClaim)
 		ExpectApplied(ctx, env.Client, node)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue())
 
 		Expect(env.Client.Delete(ctx, nodeClaim)).To(Succeed())
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // triggers the node deletion
@@ -274,6 +289,9 @@ var _ = Describe("Termination", func() {
 
 		node := test.NodeClaimLinkedNode(nodeClaim)
 		ExpectApplied(ctx, env.Client, node)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue())
 
 		Expect(env.Client.Delete(ctx, nodeClaim)).To(Succeed())
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // triggers the node deletion
@@ -297,6 +315,9 @@ var _ = Describe("Termination", func() {
 
 		node := test.NodeClaimLinkedNode(nodeClaim)
 		ExpectApplied(ctx, env.Client, node, nodeClaim)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue())
 
 		Expect(env.Client.Delete(ctx, nodeClaim)).To(Succeed())
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) // triggers the node deletion
@@ -307,4 +328,26 @@ var _ = Describe("Termination", func() {
 			v1.NodeClaimTerminationTimestampAnnotationKey: "2024-04-01T12:00:00-05:00",
 		}))
 	})
+	It("should not delete Nodes if the NodeClaim is not registered", func() {
+		ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		_, err := cloudProvider.Get(ctx, nodeClaim.Status.ProviderID)
+		Expect(err).ToNot(HaveOccurred())
+
+		node := test.NodeClaimLinkedNode(nodeClaim)
+		// Remove the unregistered taint to ensure the NodeClaim can't be marked as registered
+		node.Spec.Taints = nil
+		ExpectApplied(ctx, env.Client, node)
+		_ = ExpectObjectReconcileFailed(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsFalse()).To(BeTrue())
+
+		Expect(env.Client.Delete(ctx, nodeClaim)).To(Succeed())
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		ExpectExists(ctx, env.Client, node)
+		ExpectNotFound(ctx, env.Client, nodeClaim)
+	})
 })