diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index e78ede68ff..f0d11ca569 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -46,6 +46,10 @@ const ( ephemeralRunnerActionsFinalizerName = "ephemeralrunner.actions.github.com/runner-registration-finalizer" ) +// how long a deleting EphemeralRunner may stay blocked on JobStillRunning +// before the registration finalizer is force-removed +const defaultRegistrationFinalizerForceTimeout = 10 * time.Minute + // EphemeralRunnerReconciler reconciles a EphemeralRunner object type EphemeralRunnerReconciler struct { client.Client @@ -53,6 +57,16 @@ type EphemeralRunnerReconciler struct { Scheme *runtime.Scheme PublishMetrics bool ResourceBuilder + + // RegistrationFinalizerForceTimeout overrides defaultRegistrationFinalizerForceTimeout when set + RegistrationFinalizerForceTimeout time.Duration +} + +func (r *EphemeralRunnerReconciler) registrationFinalizerForceTimeout() time.Duration { + if r.RegistrationFinalizerForceTimeout > 0 { + return r.RegistrationFinalizerForceTimeout + } + return defaultRegistrationFinalizerForceTimeout } var ephemeralRunnerPhaseMetrics = struct { @@ -110,12 +124,28 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ log.Error(err, "Failed to clean up runner from service") return ctrl.Result{}, err } + forced := false if !ok { - log.Info("Runner is not finished yet, retrying in 30s") - return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + force, forceErr := r.shouldForceRegistrationFinalizerRemoval(ctx, &ephemeralRunner, log) + if forceErr != nil { + log.Error(forceErr, "Failed to evaluate registration finalizer force-removal") + return ctrl.Result{}, forceErr + } + if !force { + log.Info("Runner is not finished yet, retrying in 30s") + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + } + forced = true + log.Info( + "Actions service still reports the job as running, but the runner pod is gone or terminal and deletion exceeded the timeout; force-removing registration finalizer", + "timeout", r.registrationFinalizerForceTimeout(), + "deletionTimestamp", ephemeralRunner.DeletionTimestamp, + ) } - log.Info("Runner is cleaned up from the service, removing finalizer") + if !forced { + log.Info("Runner is cleaned up from the service, removing finalizer") + } if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { log.Info("Removed finalizer from ephemeral runner") if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { @@ -469,6 +499,31 @@ func (r *EphemeralRunnerReconciler) cleanupRunnerFromService(ctx context.Context return true, nil } +// shouldForceRegistrationFinalizerRemoval returns true when the registration finalizer can be removed without a successful RemoveRunner call: +// the runner has been deleting for longer than the timeout and its pod is gone or in a terminal phase, so it can never unregister on its own. +func (r *EphemeralRunnerReconciler) shouldForceRegistrationFinalizerRemoval(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) (bool, error) { + if ephemeralRunner.DeletionTimestamp.IsZero() { + return false, nil + } + if time.Since(ephemeralRunner.DeletionTimestamp.Time) < r.registrationFinalizerForceTimeout() { + return false, nil + } + + pod := new(corev1.Pod) + err := r.Get(ctx, types.NamespacedName{Namespace: ephemeralRunner.Namespace, Name: ephemeralRunner.Name}, pod) + switch { + case kerrors.IsNotFound(err): + return true, nil + case err != nil: + return false, fmt.Errorf("failed to get runner pod while evaluating finalizer force-removal: %w", err) + case pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed: + log.Info("Runner pod is in a terminal phase and cannot unregister on its own", "phase", pod.Status.Phase) + return true, nil + default: + return false, nil + } +} + func (r *EphemeralRunnerReconciler) cleanupResources(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) error { log.Info("Cleaning up the runner pod") pod := new(corev1.Pod) diff --git a/controllers/actions.github.com/ephemeralrunner_controller_test.go b/controllers/actions.github.com/ephemeralrunner_controller_test.go index 80c27134a7..97e7687065 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunner_controller_test.go @@ -1285,6 +1285,193 @@ var _ = Describe("EphemeralRunner", func() { }) }) + Describe("Registration finalizer force removal", func() { + var ctx context.Context + var mgr ctrl.Manager + var autoscalingNS *corev1.Namespace + var configSecret *corev1.Secret + var controller *EphemeralRunnerReconciler + var ephemeralRunner *v1alpha1.EphemeralRunner + var forceTimeout time.Duration + + BeforeEach(func() { + ctx = context.Background() + autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) + configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + }) + + JustBeforeEach(func() { + controller = &EphemeralRunnerReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + RegistrationFinalizerForceTimeout: forceTimeout, + ResourceBuilder: ResourceBuilder{ + SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( + scalefake.WithClient( + scalefake.NewClient( + scalefake.WithGenerateJitRunnerConfig( + &scaleset.RunnerScaleSetJitRunnerConfig{ + Runner: &scaleset.RunnerReference{ID: 1, Name: "test-runner"}, + EncodedJITConfig: "fake-jit-config", + }, + nil, + ), + scalefake.WithRemoveRunner(scaleset.JobStillRunningError), + ), + ), + )), + }, + } + + err := controller.SetupWithManager(mgr) + Expect(err).To(BeNil(), "failed to setup controller") + + ephemeralRunner = newExampleRunner("test-runner", autoscalingNS.Name, configSecret.Name) + err = k8sClient.Create(ctx, ephemeralRunner) + Expect(err).To(BeNil(), "failed to create ephemeral runner") + + startManagers(GinkgoT(), mgr) + + Eventually( + func() ([]string, error) { + er := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil { + return nil, err + } + n := len(er.Finalizers) + return er.Finalizers[:n:n], nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(ContainElements(ephemeralRunnerFinalizerName, ephemeralRunnerActionsFinalizerName), "both finalizers should be added") + + Eventually( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(corev1.Pod)) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(Succeed(), "runner pod should be created") + }) + + deleteEphemeralRunnerAndWaitForTerminating := func() { + err := k8sClient.Delete(ctx, ephemeralRunner) + Expect(err).To(BeNil(), "failed to delete ephemeral runner") + + Eventually( + func() (bool, error) { + er := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil { + return false, err + } + return !er.DeletionTimestamp.IsZero(), nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "ephemeral runner should be terminating but held by finalizers") + } + + Context("with a short force timeout", func() { + BeforeEach(func() { + forceTimeout = time.Second + }) + + It("force-removes the registration finalizer when the pod is gone and deletion exceeded the timeout", func() { + deleteEphemeralRunnerAndWaitForTerminating() + + Consistently( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner)) + }, + 2*time.Second, + ephemeralRunnerInterval, + ).Should(Succeed(), "ephemeral runner should stay terminating while the pod exists") + + pod := new(corev1.Pod) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod) + Expect(err).To(BeNil(), "failed to get runner pod") + err = k8sClient.Delete(ctx, pod) + Expect(err).To(BeNil(), "failed to delete runner pod") + + Eventually( + func() bool { + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner)) + return kerrors.IsNotFound(err) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "ephemeral runner should be force-finalized and deleted") + }) + + It("force-removes the registration finalizer when the pod is in a terminal phase and deletion exceeded the timeout", func() { + deleteEphemeralRunnerAndWaitForTerminating() + + Consistently( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner)) + }, + 2*time.Second, + ephemeralRunnerInterval, + ).Should(Succeed(), "ephemeral runner should stay terminating while the pod is not terminal") + + pod := new(corev1.Pod) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod) + Expect(err).To(BeNil(), "failed to get runner pod") + pod.Status.Phase = corev1.PodFailed + err = k8sClient.Status().Update(ctx, pod) + Expect(err).To(BeNil(), "failed to update pod status") + + Eventually( + func() bool { + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner)) + return kerrors.IsNotFound(err) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "ephemeral runner should be force-finalized and deleted") + }) + }) + + Context("with a long force timeout", func() { + BeforeEach(func() { + forceTimeout = 5 * time.Minute + }) + + It("does not force-remove the registration finalizer before the timeout even when the pod is gone", func() { + deleteEphemeralRunnerAndWaitForTerminating() + + pod := new(corev1.Pod) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod) + Expect(err).To(BeNil(), "failed to get runner pod") + err = k8sClient.Delete(ctx, pod) + Expect(err).To(BeNil(), "failed to delete runner pod") + + Eventually( + func() bool { + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(corev1.Pod)) + return kerrors.IsNotFound(err) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "runner pod should be gone") + + Consistently( + func() ([]string, error) { + er := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil { + return nil, err + } + n := len(er.Finalizers) + return er.Finalizers[:n:n], nil + }, + 3*time.Second, + ephemeralRunnerInterval, + ).Should(ContainElement(ephemeralRunnerActionsFinalizerName), "registration finalizer should be kept before the timeout") + }) + }) + }) + Describe("Pod proxy config", func() { var ctx context.Context var mgr ctrl.Manager