Remove finalizers in one pass to speed up cleanups AutoscalingRunnerSet (#3536)

controllers/actions.github.com/autoscalingrunnerset_controller.go

@@ -134,17 +134,11 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl
 			return ctrl.Result{}, err
 		}
 
-		requeue, err := r.removeFinalizersFromDependentResources(ctx, autoscalingRunnerSet, log)
+		if err := r.removeFinalizersFromDependentResources(ctx, autoscalingRunnerSet, log); err != nil {
-		if err != nil {
 			log.Error(err, "Failed to remove finalizers on dependent resources")
 			return ctrl.Result{}, err
 		}
 
-		if requeue {
-			log.Info("Waiting for dependent resources to be deleted")
-			return ctrl.Result{Requeue: true}, nil
-		}
-
 		log.Info("Removing finalizer")
 		err = patch(ctx, r.Client, autoscalingRunnerSet, func(obj *v1alpha1.AutoscalingRunnerSet) {
 			controllerutil.RemoveFinalizer(obj, autoscalingRunnerSetFinalizerName)
@@ -389,7 +383,7 @@ func (r *AutoscalingRunnerSetReconciler) deleteEphemeralRunnerSets(ctx context.C
 	return nil
 }
 
-func (r *AutoscalingRunnerSetReconciler) removeFinalizersFromDependentResources(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) (requeue bool, err error) {
+func (r *AutoscalingRunnerSetReconciler) removeFinalizersFromDependentResources(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) error {
 	c := autoscalingRunnerSetFinalizerDependencyCleaner{
 		client:               r.Client,
 		autoscalingRunnerSet: autoscalingRunnerSet,
@@ -404,12 +398,7 @@ func (r *AutoscalingRunnerSetReconciler) removeFinalizersFromDependentResources(
 	c.removeManagerRoleBindingFinalizer(ctx)
 	c.removeManagerRoleFinalizer(ctx)
 
-	requeue, err = c.result()
+	return c.Err()
-	if err != nil {
-		logger.Error(err, "Failed to cleanup finalizer from dependent resource")
-		return true, err
-	}
-	return requeue, nil
 }
 
 func (r *AutoscalingRunnerSetReconciler) createRunnerScaleSet(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, logger logr.Logger) (ctrl.Result, error) {
@@ -784,17 +773,16 @@ type autoscalingRunnerSetFinalizerDependencyCleaner struct {
 	autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet
 	logger               logr.Logger
 
-	// fields to operate on
+	err error
-	requeue bool
-	err     error
 }
 
-func (c *autoscalingRunnerSetFinalizerDependencyCleaner) result() (requeue bool, err error) {
+func (c *autoscalingRunnerSetFinalizerDependencyCleaner) Err() error {
-	return c.requeue, c.err
+	return c.err
 }
 
 func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeKubernetesModeRoleBindingFinalizer(ctx context.Context) {
-	if c.requeue || c.err != nil {
+	if c.err != nil {
+		c.logger.Info("Skipping cleaning up kubernetes mode service account")
 		return
 	}
 
@@ -825,7 +813,6 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeKubernetesModeRol
 			c.err = fmt.Errorf("failed to patch kubernetes mode role binding without finalizer: %w", err)
 			return
 		}
-		c.requeue = true
 		c.logger.Info("Removed finalizer from container mode kubernetes role binding", "name", roleBindingName)
 		return
 	case err != nil && !kerrors.IsNotFound(err):
@@ -838,7 +825,7 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeKubernetesModeRol
 }
 
 func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeKubernetesModeRoleFinalizer(ctx context.Context) {
-	if c.requeue || c.err != nil {
+	if c.err != nil {
 		return
 	}
 
@@ -868,7 +855,6 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeKubernetesModeRol
 			c.err = fmt.Errorf("failed to patch kubernetes mode role without finalizer: %w", err)
 			return
 		}
-		c.requeue = true
 		c.logger.Info("Removed finalizer from container mode kubernetes role")
 		return
 	case err != nil && !kerrors.IsNotFound(err):
@@ -881,7 +867,7 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeKubernetesModeRol
 }
 
 func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeKubernetesModeServiceAccountFinalizer(ctx context.Context) {
-	if c.requeue || c.err != nil {
+	if c.err != nil {
 		return
 	}
 
@@ -912,7 +898,6 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeKubernetesModeSer
 			c.err = fmt.Errorf("failed to patch kubernetes mode service account without finalizer: %w", err)
 			return
 		}
-		c.requeue = true
 		c.logger.Info("Removed finalizer from container mode kubernetes service account")
 		return
 	case err != nil && !kerrors.IsNotFound(err):
@@ -925,7 +910,7 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeKubernetesModeSer
 }
 
 func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeNoPermissionServiceAccountFinalizer(ctx context.Context) {
-	if c.requeue || c.err != nil {
+	if c.err != nil {
 		return
 	}
 
@@ -956,7 +941,6 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeNoPermissionServi
 			c.err = fmt.Errorf("failed to patch service account without finalizer: %w", err)
 			return
 		}
-		c.requeue = true
 		c.logger.Info("Removed finalizer from no permission service account", "name", serviceAccountName)
 		return
 	case err != nil && !kerrors.IsNotFound(err):
@@ -969,7 +953,7 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeNoPermissionServi
 }
 
 func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeGitHubSecretFinalizer(ctx context.Context) {
-	if c.requeue || c.err != nil {
+	if c.err != nil {
 		return
 	}
 
@@ -1000,7 +984,6 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeGitHubSecretFinal
 			c.err = fmt.Errorf("failed to patch GitHub secret without finalizer: %w", err)
 			return
 		}
-		c.requeue = true
 		c.logger.Info("Removed finalizer from GitHub secret", "name", githubSecretName)
 		return
 	case err != nil && !kerrors.IsNotFound(err) && !kerrors.IsForbidden(err):
@@ -1013,7 +996,7 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeGitHubSecretFinal
 }
 
 func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleBindingFinalizer(ctx context.Context) {
-	if c.requeue || c.err != nil {
+	if c.err != nil {
 		return
 	}
 
@@ -1044,7 +1027,6 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleBindin
 			c.err = fmt.Errorf("failed to patch manager role binding without finalizer: %w", err)
 			return
 		}
-		c.requeue = true
 		c.logger.Info("Removed finalizer from manager role binding", "name", managerRoleBindingName)
 		return
 	case err != nil && !kerrors.IsNotFound(err):
@@ -1057,7 +1039,7 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleBindin
 }
 
 func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleFinalizer(ctx context.Context) {
-	if c.requeue || c.err != nil {
+	if c.err != nil {
 		return
 	}
 
@@ -1088,7 +1070,6 @@ func (c *autoscalingRunnerSetFinalizerDependencyCleaner) removeManagerRoleFinali
 			c.err = fmt.Errorf("failed to patch manager role without finalizer: %w", err)
 			return
 		}
-		c.requeue = true
 		c.logger.Info("Removed finalizer from manager role", "name", managerRoleName)
 		return
 	case err != nil && !kerrors.IsNotFound(err):