Only remove finalizer after there are no instances to termiante (#265)

Author: fiunchinho

CHANGELOG.md

@@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Change `renovate` configuration to only get upgrades for CAPA compatible versions of k8s dependencies.
 - Don't reconcile the `ShareReconciler` if only the status field has changed.
 
+### Fixed
+
+- Only remove `finalizer` after there are no more karpenter instances to terminate.
+
 ## [0.19.0] - 2025-06-17
 
 ### Added

controllers/karpentermachinepool_controller.go

@@ -213,10 +213,18 @@ func (r *KarpenterMachinePoolReconciler) reconcileDelete(ctx context.Context, lo
 
 		// Terminate EC2 instances with the karpenter.sh/nodepool tag matching the KarpenterMachinePool name
 		logger.Info("Terminating EC2 instances for KarpenterMachinePool", "karpenterMachinePoolName", karpenterMachinePool.Name)
-		err = ec2Client.TerminateInstancesByTag(ctx, logger, "karpenter.sh/nodepool", karpenterMachinePool.Name)
+		instanceIDs, err := ec2Client.TerminateInstancesByTag(ctx, logger, "karpenter.sh/nodepool", karpenterMachinePool.Name)
 		if err != nil {
 			return reconcile.Result{}, fmt.Errorf("failed to terminate EC2 instances: %w", err)
 		}
+
+		logger.Info("Found instances", "instanceIDs", instanceIDs)
+
+		// Requeue if we find instances to terminate. Once there are no instances to terminate, we proceed to remove the finalizer.
+		// We do this when the cluster is being deleted, to avoid removing the finalizer before karpenter launches a new instance that would be left over.
+		if len(instanceIDs) > 0 {
+			return reconcile.Result{RequeueAfter: 30 * time.Second}, nil
+		}
 	}
 
 	// Create deep copy of the reconciled object so we can change it

controllers/karpentermachinepool_controller_test.go

@@ -261,15 +261,42 @@ var _ = Describe("KarpenterMachinePool reconciler", func() {
 				err = fakeCtrlClient.Delete(ctx, cluster)
 				Expect(err).NotTo(HaveOccurred())
 			})
+			// This test is a bit cumbersome because we are deleting CRs, so we can't use different `It` blocks or the CRs would be gone.
+			// We first mock the call to `TerminateInstancesByTag` to return some instances so that we can test
+			// the behavior when there are pending instances to remove.
+			// Then we manually/explicitly call the reconciler inside the test again, to be able to test the behavior
+			// when there are no instances to remove.
+			When("there are ec2 instances from karpenter", func() {
+				BeforeEach(func() {
+					ec2Client.TerminateInstancesByTagReturnsOnCall(0, []string{"i-abc123", "i-def456"}, nil)
+				})
 
-			It("deletes KarpenterMachinePool ec2 instances and finalizer", func() {
-				Expect(reconcileErr).NotTo(HaveOccurred())
-				Expect(ec2Client.TerminateInstancesByTagCallCount()).To(Equal(1))
+				It("deletes KarpenterMachinePool ec2 instances and finalizer", func() {
+					Expect(reconcileErr).NotTo(HaveOccurred())
+					Expect(ec2Client.TerminateInstancesByTagCallCount()).To(Equal(1))
+					Expect(reconcileResult.RequeueAfter).To(Equal(30 * time.Second))
 
-				karpenterMachinePoolList := &karpenterinfra.KarpenterMachinePoolList{}
-				err := fakeCtrlClient.List(ctx, karpenterMachinePoolList)
-				Expect(err).NotTo(HaveOccurred())
-				Expect(karpenterMachinePoolList.Items).To(HaveLen(0))
+					karpenterMachinePoolList := &karpenterinfra.KarpenterMachinePoolList{}
+					err := fakeCtrlClient.List(ctx, karpenterMachinePoolList)
+					Expect(err).NotTo(HaveOccurred())
+					// Finalizer should be there blocking the deletion of the CR
+					Expect(karpenterMachinePoolList.Items).To(HaveLen(1))
+
+					ec2Client.TerminateInstancesByTagReturnsOnCall(0, nil, nil)
+
+					reconcileResult, reconcileErr = reconciler.Reconcile(ctx, ctrl.Request{
+						NamespacedName: types.NamespacedName{
+							Namespace: KarpenterMachinePoolNamespace,
+							Name:      KarpenterMachinePoolName,
+						},
+					})
+
+					karpenterMachinePoolList = &karpenterinfra.KarpenterMachinePoolList{}
+					err = fakeCtrlClient.List(ctx, karpenterMachinePoolList)
+					Expect(err).NotTo(HaveOccurred())
+					// Finalizer should've been removed and the CR should be gone
+					Expect(karpenterMachinePoolList.Items).To(HaveLen(0))
+				})
 			})
 		})
 	})

pkg/aws/ec2client.go

@@ -172,7 +172,9 @@ func getEc2Tags(t map[string]string) []*ec2.Tag {
 }
 
 // TerminateInstancesByTag terminates all EC2 instances that have the specified tag key and value.
-func (a *AWSEC2) TerminateInstancesByTag(ctx context.Context, logger logr.Logger, tagKey, tagValue string) error {
+func (a *AWSEC2) TerminateInstancesByTag(ctx context.Context, logger logr.Logger, tagKey, tagValue string) ([]string, error) {
+	ids := []string{}
+
 	logger.Info("Finding EC2 instances with tag", "tagKey", tagKey, "tagValue", tagValue)
 
 	// Create filter for the tag
@@ -188,7 +190,7 @@ func (a *AWSEC2) TerminateInstancesByTag(ctx context.Context, logger logr.Logger
 		Filters: filter,
 	})
 	if err != nil {
-		return errors.WithStack(err)
+		return ids, errors.WithStack(err)
 	}
 
 	// Collect instance IDs
@@ -206,7 +208,7 @@ func (a *AWSEC2) TerminateInstancesByTag(ctx context.Context, logger logr.Logger
 	// If no instances found, return
 	if len(instanceIds) == 0 {
 		logger.Info("No instances found with the specified tag")
-		return nil
+		return ids, nil
 	}
 
 	// Terminate the instances
@@ -215,9 +217,13 @@ func (a *AWSEC2) TerminateInstancesByTag(ctx context.Context, logger logr.Logger
 		InstanceIds: instanceIds,
 	})
 	if err != nil {
-		return errors.WithStack(err)
+		return nil, errors.WithStack(err)
 	}
 
-	logger.Info("Successfully requested termination of instances", "count", len(instanceIds), "tagKey", tagKey, "tagValue", tagValue)
-	return nil
+	for _, id := range instanceIds {
+		ids = append(ids, *id)
+	}
+
+	logger.Info("Successfully requested termination of instances", "count", len(ids), "instances", ids, "tagKey", tagKey, "tagValue", tagValue)
+	return ids, nil
 }

pkg/resolver/package.go

@@ -26,7 +26,7 @@ type AWSClients interface {
 type EC2Client interface {
 	CreateSecurityGroupForResolverEndpoints(ctx context.Context, vpcId, groupName string, tags map[string]string) (string, error)
 	DeleteSecurityGroupForResolverEndpoints(ctx context.Context, logger logr.Logger, vpcId, groupName string) error
-	TerminateInstancesByTag(ctx context.Context, logger logr.Logger, tagKey, tagValue string) error
+	TerminateInstancesByTag(ctx context.Context, logger logr.Logger, tagKey, tagValue string) ([]string, error)
 }
 
 type ResourceShare struct {