From 9990243520b4a774a72ab1239c8973a5c81495d6 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 8 Feb 2023 15:21:13 +0100 Subject: [PATCH] Early return if finalizer does not exist to make it more readable (#2262) --- .../autoscalinglistener_controller.go | 47 ++++++----- .../autoscalingrunnerset_controller.go | 83 ++++++++++--------- .../ephemeralrunner_controller.go | 65 ++++++++------- .../ephemeralrunnerset_controller.go | 43 +++++----- 4 files changed, 121 insertions(+), 117 deletions(-) diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 4078278f..faf2e4e6 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -73,30 +73,31 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } if !autoscalingListener.ObjectMeta.DeletionTimestamp.IsZero() { - if controllerutil.ContainsFinalizer(autoscalingListener, autoscalingListenerFinalizerName) { - log.Info("Deleting resources") - done, err := r.cleanupResources(ctx, autoscalingListener, log) - if err != nil { - log.Error(err, "Failed to cleanup resources after deletion") - return ctrl.Result{}, err - } - if !done { - log.Info("Waiting for resources to be deleted before removing finalizer") - return ctrl.Result{}, nil - } - - log.Info("Removing finalizer") - err = patch(ctx, r.Client, autoscalingListener, func(obj *v1alpha1.AutoscalingListener) { - controllerutil.RemoveFinalizer(obj, autoscalingListenerFinalizerName) - }) - if err != nil && !kerrors.IsNotFound(err) { - log.Error(err, "Failed to remove finalizer") - return ctrl.Result{}, err - } - - log.Info("Successfully removed finalizer after cleanup") + if !controllerutil.ContainsFinalizer(autoscalingListener, autoscalingListenerFinalizerName) { + return ctrl.Result{}, nil } - return ctrl.Result{}, nil + + log.Info("Deleting resources") + done, err := r.cleanupResources(ctx, autoscalingListener, log) + if err != nil { + log.Error(err, "Failed to cleanup resources after deletion") + return ctrl.Result{}, err + } + if !done { + log.Info("Waiting for resources to be deleted before removing finalizer") + return ctrl.Result{}, nil + } + + log.Info("Removing finalizer") + err = patch(ctx, r.Client, autoscalingListener, func(obj *v1alpha1.AutoscalingListener) { + controllerutil.RemoveFinalizer(obj, autoscalingListenerFinalizerName) + }) + if err != nil && !kerrors.IsNotFound(err) { + log.Error(err, "Failed to remove finalizer") + return ctrl.Result{}, err + } + + log.Info("Successfully removed finalizer after cleanup") } if !controllerutil.ContainsFinalizer(autoscalingListener, autoscalingListenerFinalizerName) { diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 9b14b515..b956d281 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -86,48 +86,49 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl } if !autoscalingRunnerSet.ObjectMeta.DeletionTimestamp.IsZero() { - if controllerutil.ContainsFinalizer(autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { - log.Info("Deleting resources") - done, err := r.cleanupListener(ctx, autoscalingRunnerSet, log) - if err != nil { - log.Error(err, "Failed to clean up listener") - return ctrl.Result{}, err - } - if !done { - // we are going to get notified anyway to proceed with rest of the - // cleanup. No need to re-queue - log.Info("Waiting for listener to be deleted") - return ctrl.Result{}, nil - } - - done, err = r.cleanupEphemeralRunnerSets(ctx, autoscalingRunnerSet, log) - if err != nil { - log.Error(err, "Failed to clean up ephemeral runner sets") - return ctrl.Result{}, err - } - if !done { - log.Info("Waiting for ephemeral runner sets to be deleted") - return ctrl.Result{}, nil - } - - err = r.deleteRunnerScaleSet(ctx, autoscalingRunnerSet, log) - if err != nil { - log.Error(err, "Failed to delete runner scale set") - return ctrl.Result{}, err - } - - log.Info("Removing finalizer") - err = patch(ctx, r.Client, autoscalingRunnerSet, func(obj *v1alpha1.AutoscalingRunnerSet) { - controllerutil.RemoveFinalizer(obj, autoscalingRunnerSetFinalizerName) - }) - if err != nil && !kerrors.IsNotFound(err) { - log.Error(err, "Failed to update autoscaling runner set without finalizer") - return ctrl.Result{}, err - } - - log.Info("Successfully removed finalizer after cleanup") + if !controllerutil.ContainsFinalizer(autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + return ctrl.Result{}, nil } - return ctrl.Result{}, nil + + log.Info("Deleting resources") + done, err := r.cleanupListener(ctx, autoscalingRunnerSet, log) + if err != nil { + log.Error(err, "Failed to clean up listener") + return ctrl.Result{}, err + } + if !done { + // we are going to get notified anyway to proceed with rest of the + // cleanup. No need to re-queue + log.Info("Waiting for listener to be deleted") + return ctrl.Result{}, nil + } + + done, err = r.cleanupEphemeralRunnerSets(ctx, autoscalingRunnerSet, log) + if err != nil { + log.Error(err, "Failed to clean up ephemeral runner sets") + return ctrl.Result{}, err + } + if !done { + log.Info("Waiting for ephemeral runner sets to be deleted") + return ctrl.Result{}, nil + } + + err = r.deleteRunnerScaleSet(ctx, autoscalingRunnerSet, log) + if err != nil { + log.Error(err, "Failed to delete runner scale set") + return ctrl.Result{}, err + } + + log.Info("Removing finalizer") + err = patch(ctx, r.Client, autoscalingRunnerSet, func(obj *v1alpha1.AutoscalingRunnerSet) { + controllerutil.RemoveFinalizer(obj, autoscalingRunnerSetFinalizerName) + }) + if err != nil && !kerrors.IsNotFound(err) { + log.Error(err, "Failed to update autoscaling runner set without finalizer") + return ctrl.Result{}, err + } + + log.Info("Successfully removed finalizer after cleanup") } if !controllerutil.ContainsFinalizer(autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index dd12a50f..f2aca7b1 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -76,40 +76,41 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if !ephemeralRunner.ObjectMeta.DeletionTimestamp.IsZero() { - if controllerutil.ContainsFinalizer(ephemeralRunner, ephemeralRunnerFinalizerName) { - log.Info("Finalizing ephemeral runner") - done, err := r.cleanupResources(ctx, ephemeralRunner, log) - if err != nil { - log.Error(err, "Failed to clean up ephemeral runner owned resources") - return ctrl.Result{}, err - } - if !done { - log.Info("Waiting for ephemeral runner owned resources to be deleted") - return ctrl.Result{}, nil - } - - done, err = r.cleanupContainerHooksResources(ctx, ephemeralRunner, log) - if err != nil { - log.Error(err, "Failed to clean up container hooks resources") - return ctrl.Result{}, err - } - if !done { - log.Info("Waiting for container hooks resources to be deleted") - return ctrl.Result{RequeueAfter: 5 * time.Second}, nil - } - - log.Info("Removing finalizer") - err = patch(ctx, r.Client, ephemeralRunner, func(obj *v1alpha1.EphemeralRunner) { - controllerutil.RemoveFinalizer(obj, ephemeralRunnerFinalizerName) - }) - if err != nil && !kerrors.IsNotFound(err) { - log.Error(err, "Failed to update ephemeral runner without the finalizer") - return ctrl.Result{}, err - } - - log.Info("Successfully removed finalizer after cleanup") + if !controllerutil.ContainsFinalizer(ephemeralRunner, ephemeralRunnerFinalizerName) { return ctrl.Result{}, nil } + + log.Info("Finalizing ephemeral runner") + done, err := r.cleanupResources(ctx, ephemeralRunner, log) + if err != nil { + log.Error(err, "Failed to clean up ephemeral runner owned resources") + return ctrl.Result{}, err + } + if !done { + log.Info("Waiting for ephemeral runner owned resources to be deleted") + return ctrl.Result{}, nil + } + + done, err = r.cleanupContainerHooksResources(ctx, ephemeralRunner, log) + if err != nil { + log.Error(err, "Failed to clean up container hooks resources") + return ctrl.Result{}, err + } + if !done { + log.Info("Waiting for container hooks resources to be deleted") + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + + log.Info("Removing finalizer") + err = patch(ctx, r.Client, ephemeralRunner, func(obj *v1alpha1.EphemeralRunner) { + controllerutil.RemoveFinalizer(obj, ephemeralRunnerFinalizerName) + }) + if err != nil && !kerrors.IsNotFound(err) { + log.Error(err, "Failed to update ephemeral runner without the finalizer") + return ctrl.Result{}, err + } + + log.Info("Successfully removed finalizer after cleanup") return ctrl.Result{}, nil } diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 08ecc2da..e1840a4e 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -81,29 +81,30 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R // Requested deletion does not need reconciled. if !ephemeralRunnerSet.ObjectMeta.DeletionTimestamp.IsZero() { - if controllerutil.ContainsFinalizer(ephemeralRunnerSet, ephemeralRunnerSetFinalizerName) { - log.Info("Deleting resources") - done, err := r.cleanUpEphemeralRunners(ctx, ephemeralRunnerSet, log) - if err != nil { - log.Error(err, "Failed to clean up EphemeralRunners") - return ctrl.Result{}, err - } - if !done { - log.Info("Waiting for resources to be deleted") - return ctrl.Result{}, nil - } - - log.Info("Removing finalizer") - if err := patch(ctx, r.Client, ephemeralRunnerSet, func(obj *v1alpha1.EphemeralRunnerSet) { - controllerutil.RemoveFinalizer(obj, ephemeralRunnerSetFinalizerName) - }); err != nil && !kerrors.IsNotFound(err) { - log.Error(err, "Failed to update ephemeral runner set with removed finalizer") - return ctrl.Result{}, err - } - - log.Info("Successfully removed finalizer after cleanup") + if !controllerutil.ContainsFinalizer(ephemeralRunnerSet, ephemeralRunnerSetFinalizerName) { return ctrl.Result{}, nil } + + log.Info("Deleting resources") + done, err := r.cleanUpEphemeralRunners(ctx, ephemeralRunnerSet, log) + if err != nil { + log.Error(err, "Failed to clean up EphemeralRunners") + return ctrl.Result{}, err + } + if !done { + log.Info("Waiting for resources to be deleted") + return ctrl.Result{}, nil + } + + log.Info("Removing finalizer") + if err := patch(ctx, r.Client, ephemeralRunnerSet, func(obj *v1alpha1.EphemeralRunnerSet) { + controllerutil.RemoveFinalizer(obj, ephemeralRunnerSetFinalizerName) + }); err != nil && !kerrors.IsNotFound(err) { + log.Error(err, "Failed to update ephemeral runner set with removed finalizer") + return ctrl.Result{}, err + } + + log.Info("Successfully removed finalizer after cleanup") return ctrl.Result{}, nil }