Stop paying a write to release a registration a runner already released (#4669)

controllers/actions.github.com/ephemeralrunner_controller.go

@@ -110,6 +110,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 			return ctrl.Result{}, nil
 		}
 
+		deferredActionsFinalizer := false
 		if controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) {
 			// This finalizer exists to release the runner's registration with the
 			// Actions service. There are two ways that happens.
@@ -156,16 +157,22 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 				"phase", ephemeralRunner.Status.Phase,
 			)
 
-			if controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerActionsFinalizerName) {
+			removedActionsFinalizer := controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerActionsFinalizerName)
+
+			// The patch has to land before the runner is queued: queueing first
+			// would ask the service to remove the same runner twice when the patch
+			// fails and the reconcile comes back through this branch. A runner with
+			// nothing to queue has nothing to order the patch against, so its
+			// removal rides along with the finalizer patch made after cleanup
+			// below rather than paying for a round trip of its own.
+			deferredActionsFinalizer = removedActionsFinalizer && runnerID == 0
+			if removedActionsFinalizer && !deferredActionsFinalizer {
 				if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); err != nil {
 					log.Error(err, "Failed to update ephemeral runner after removing finalizer")
 					return ctrl.Result{}, err
 				}
 			}
 
-			// Queued only once the finalizer is actually gone. A failed patch
-			// above sends the reconcile back through this branch, and queueing
-			// first would ask the service to remove the same runner twice.
 			if runnerID != 0 {
 				r.UnregistrationQueue.Push(&ephemeralRunner, runnerID)
 			}
@@ -189,7 +196,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 
 		log.Info("Removing finalizer")
-		if controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerFinalizerName) {
+		if controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerFinalizerName) || deferredActionsFinalizer {
 			log.Info("Removed finalizer from ephemeral runner")
 			if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); client.IgnoreNotFound(err) != nil {
 				log.Error(err, "Failed to update ephemeral runner after removing finalizer")
@@ -212,9 +219,17 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		// Repeated here so that error costs a reconcile instead of leaving the
 		// registration held until the set gets around to deleting the runner.
 		// Does nothing once the registration is released.
-		if err := r.queueUnregistration(ctx, &ephemeralRunner, log); err != nil {
-			log.Error(err, "Failed to release the registration of a terminated ephemeral runner")
-			return ctrl.Result{}, err
+		//
+		// A runner that deregistered itself holds nothing to release, so there is
+		// no error to recover from and nothing to queue. Releasing it here would
+		// only drop the finalizer, which the deletion below does anyway in a patch
+		// it already makes, at the cost of an extra write and the reconcile that
+		// write wakes.
+		if !runnerSelfDeregistered(&ephemeralRunner) {
+			if err := r.queueUnregistration(ctx, &ephemeralRunner, log); err != nil {
+				log.Error(err, "Failed to release the registration of a terminated ephemeral runner")
+				return ctrl.Result{}, err
+			}
 		}
 
 		err := r.cleanupResources(ctx, &ephemeralRunner, log)

controllers/actions.github.com/ephemeralrunner_controller_test.go

@@ -1911,7 +1911,9 @@ var _ = Describe("EphemeralRunner", func() {
 
 		It("leaves a succeeded runner alone on the terminated path", func() {
 			// Same path, but the runner exited cleanly, so there is nothing to
-			// hand over and only the finalizer goes.
+			// hand over and nothing to release. The finalizer stays until the
+			// deletion that removes it anyway, in a patch that deletion already
+			// makes, rather than costing a write and a wake-up here.
 			name := "terminated-succeeded-runner"
 			ephemeralRunner := newExampleRunner(name, autoscalingNS.Name, configSecret.Name)
 			ephemeralRunner.Finalizers = []string{ephemeralRunnerFinalizerName, ephemeralRunnerActionsFinalizerName}
@@ -1930,7 +1932,18 @@ var _ = Describe("EphemeralRunner", func() {
 
 			updated := new(v1alpha1.EphemeralRunner)
 			Expect(k8sClient.Get(ctx, request.NamespacedName, updated)).To(Succeed())
-			Expect(updated.Finalizers).NotTo(ContainElement(ephemeralRunnerActionsFinalizerName))
+			Expect(updated.Finalizers).To(ContainElement(ephemeralRunnerActionsFinalizerName))
+
+			// Deleting it clears both finalizers, so nothing is left behind and
+			// the service is still never asked to remove the registration.
+			Expect(k8sClient.Delete(ctx, updated)).To(Succeed())
+			_, err = controller.Reconcile(ctx, request)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(queue.queued()).To(BeEmpty())
+			Eventually(func() bool {
+				return kerrors.IsNotFound(k8sClient.Get(ctx, request.NamespacedName, new(v1alpha1.EphemeralRunner)))
+			}, ephemeralRunnerTimeout, ephemeralRunnerInterval).Should(BeTrue())
 		})
 	})
 })