diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index f6ef03cc..47103bfc 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/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) diff --git a/controllers/actions.github.com/ephemeralrunner_controller_test.go b/controllers/actions.github.com/ephemeralrunner_controller_test.go index 49bf54e4..cfc5a266 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller_test.go +++ b/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()) }) }) })