diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 6877dc4a..46973edd 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -163,56 +163,8 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl } } - outdated := autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseOutdated - if outdated { - log.Info("Autoscaling runner set is in outdated phase, removing the listener") - done, err := r.cleanupListener(ctx, &autoscalingRunnerSet, log) - if err != nil { - log.Error(err, "Failed to clean up listener") - return ctrl.Result{}, err - } - if !done { - log.Info("Waiting for listener to be cleaned up for the outdated runner set") - return ctrl.Result{RequeueAfter: 5 * time.Second}, nil - } - - var ephemeralRunnerSet v1alpha1.EphemeralRunnerSet - err = r.Get( - ctx, - types.NamespacedName{ - Namespace: autoscalingRunnerSet.Namespace, - Name: autoscalingRunnerSet.Name, - }, - &ephemeralRunnerSet, - ) - switch { - case kerrors.IsNotFound(err): - // If the ephemeral runner set is not found, something removed the ephemeral runner set. The ephemeral runner set should - // not be removed by the controller once it is outdated. However, if the ephemeral runner set is removed, it means no ephemeral - // runners should be running (or at least no ephemeral runners associated with the ephemeral runner set). - // Therefore, this state is acceptable, because the update to the autoscaling runner set will trigger the loop - // that will eventually create a new ephemeral runner set. - log.Info("Ephemeral runner set is not found. Ignoring the state until the autoscaling runner set is updated") - return ctrl.Result{}, nil - case err != nil: - log.Error(err, "Failed to get ephemeral runner set for the outdated runner set") - return ctrl.Result{}, err - default: - if !ephemeralRunnerSet.DeletionTimestamp.IsZero() { - // Same as NotFound case, ignore. - return ctrl.Result{}, nil - } - - original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Spec.Replicas = 0 - ephemeralRunnerSet.Spec.PatchID = 0 - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to patch ephemeral runner set with 0 replicas and reset patch ID for the outdated runner set") - return ctrl.Result{}, err - } - - return ctrl.Result{}, nil - } + if autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseOutdated { + return r.reconcileOutdated(ctx, &autoscalingRunnerSet, log) } if shouldCreateScaleSet(&autoscalingRunnerSet) { @@ -251,37 +203,24 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl log.Error(err, "Failed to get ephemeral runner") return ctrl.Result{}, err case ephemeralRunnerSetOutdatedForAppliedRevision(&ephemeralRunnerSet) && autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseRunning: - // Runners are outdated. We need to stop the listener so it stops getting new jobs. - log.Info("Ephemeral runner set is outdated. Cleaning up resources for the outdated runner set") - done, err := r.cleanupListener(ctx, &autoscalingRunnerSet, log) - if err != nil { - log.Error(err, "Failed to clean up listener for outdated ephemeral runner set") + // The runners rejected the spec they were given, so the scale set has to + // stop acquiring jobs it cannot run. Record that in the phase first: it is + // what keeps the listener switched off across reconciles, and what stops + // the branches below from rebuilding it. The observed generation is carried + // over unchanged, so a spec update still registers as new work. + log.Info("Ephemeral runner set is outdated. Moving the autoscaling runner set to the outdated phase") + if err := r.updateStatus( + ctx, + &autoscalingRunnerSet, + v1alpha1.AutoscalingRunnerSetPhaseOutdated, + autoscalingRunnerSet.Status.ObservedGeneration, + log, + ); err != nil { + log.Error(err, "Failed to update autoscaling runner set status with outdated phase") return ctrl.Result{}, err } - if !done { - log.Info("Waiting for listener to be cleaned up for the outdated ephemeral runner set") - return ctrl.Result{RequeueAfter: 5 * time.Second}, nil - } - // Then, we need to remove the ephemeral runner set to force scale-down. The ephemeral runner set - // will eventually remove all runners as soon as possible. - // - // The scale set should not be removed yet, since user did not explicitly remove the scale set (or the autoscaling runner set) - // Therefore, the autoscaling runner set should stay in outdated state until the spec is updated, - // or until the autoscaling runner set is removed. - done, err = r.cleanupEphemeralRunnerSet(ctx, &autoscalingRunnerSet, log) - if err != nil { - log.Error(err, "Failed to clean up ephemeral runner set for outdated runner set") - return ctrl.Result{}, err - } - if !done { - log.Info("Waiting for ephemeral runner set to be cleaned up for the outdated runner set") - return ctrl.Result{RequeueAfter: 5 * time.Second}, nil - } - - log.Info("Successfully cleaned up resources for the outdated runner set") - - return ctrl.Result{}, nil + return r.reconcileOutdated(ctx, &autoscalingRunnerSet, log) default: desired, err := r.newEphemeralRunnerSet(&autoscalingRunnerSet) @@ -290,7 +229,19 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if ephemeralRunnerSetActionableSpecChanged(&ephemeralRunnerSet, desired) { + if ephemeralRunnerSetActionableSpecChanged(&ephemeralRunnerSet, desired) || + ephemeralRunnerSetOutdatedForAppliedRevision(&ephemeralRunnerSet) { + // The second condition is the recovery path out of the outdated phase. + // Reaching here at all means the AutoscalingRunnerSet is not outdated, + // and the case above already claimed every running set that is, so the + // spec must have been updated since the runners rejected it. That update + // is the signal to try again, and the revision has to advance even when + // the runner spec itself is unchanged: the revision is what tells the + // EphemeralRunnerSet to stop judging itself by the runners that failed, + // which is what clears its outdated phase and lets it scale up again. + // Without it a scale set could only ever be recovered by editing the + // runner spec, and an edit to anything else would switch the listener + // back on against a set that stays parked at zero. original := ephemeralRunnerSet.DeepCopy() ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec @@ -420,6 +371,74 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } +// reconcileOutdated holds a scale set whose runners rejected the runner spec. +// +// The listener is removed so no new jobs are acquired, and the EphemeralRunnerSet +// is pinned to zero replicas so it releases every runner that is not currently +// executing a job. The set itself is deliberately kept: the user has not asked +// for the scale set to go away, and deleting it would make the controller +// immediately rebuild it from the same rejected spec, in a loop. Keeping it also +// preserves the revision bookkeeping that decides when the scale set may run +// again. +// +// This state is left only when the AutoscalingRunnerSet spec is updated, which +// moves the phase back to pending and lets the next reconcile publish the new +// spec to the set. +func (r *AutoscalingRunnerSetReconciler) reconcileOutdated(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, log logr.Logger) (ctrl.Result, error) { + log.Info("Autoscaling runner set is in outdated phase, removing the listener") + done, err := r.cleanupListener(ctx, autoscalingRunnerSet, log) + if err != nil { + log.Error(err, "Failed to clean up listener") + return ctrl.Result{}, err + } + if !done { + log.Info("Waiting for listener to be cleaned up for the outdated runner set") + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + + var ephemeralRunnerSet v1alpha1.EphemeralRunnerSet + err = r.Get( + ctx, + types.NamespacedName{ + Namespace: autoscalingRunnerSet.Namespace, + Name: autoscalingRunnerSet.Name, + }, + &ephemeralRunnerSet, + ) + switch { + case kerrors.IsNotFound(err): + // If the ephemeral runner set is not found, something removed the ephemeral runner set. The ephemeral runner set should + // not be removed by the controller once it is outdated. However, if the ephemeral runner set is removed, it means no ephemeral + // runners should be running (or at least no ephemeral runners associated with the ephemeral runner set). + // Therefore, this state is acceptable, because the update to the autoscaling runner set will trigger the loop + // that will eventually create a new ephemeral runner set. + log.Info("Ephemeral runner set is not found. Ignoring the state until the autoscaling runner set is updated") + return ctrl.Result{}, nil + case err != nil: + log.Error(err, "Failed to get ephemeral runner set for the outdated runner set") + return ctrl.Result{}, err + default: + if !ephemeralRunnerSet.DeletionTimestamp.IsZero() { + // Same as NotFound case, ignore. + return ctrl.Result{}, nil + } + + if ephemeralRunnerSet.Spec.Replicas == 0 && ephemeralRunnerSet.Spec.PatchID == 0 { + return ctrl.Result{}, nil + } + + original := ephemeralRunnerSet.DeepCopy() + ephemeralRunnerSet.Spec.Replicas = 0 + ephemeralRunnerSet.Spec.PatchID = 0 + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + log.Error(err, "Failed to patch ephemeral runner set with 0 replicas and reset patch ID for the outdated runner set") + return ctrl.Result{}, err + } + + return ctrl.Result{}, nil + } +} + func (r *AutoscalingRunnerSetReconciler) cleanUpResources(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, log logr.Logger) (bool, error) { log.Info("Deleting the listener") done, err := r.cleanupListener(ctx, autoscalingRunnerSet, log) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index 96f50010..191ab729 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -2854,3 +2854,235 @@ func unblockDeletion(listener *v1alpha1.AutoscalingListener) { } Expect(k8sClient.Patch(context.Background(), current, client.MergeFrom(original))).To(Succeed(), "failed to remove the test finalizer from the listener") } + +var _ = Describe("Test AutoscalingRunnerSet outdated lifecycle", Ordered, func() { + var originalBuildVersion string + buildVersion := "0.1.0" + + BeforeAll(func() { + originalBuildVersion = build.Version + build.Version = buildVersion + }) + + AfterAll(func() { + build.Version = originalBuildVersion + }) + + Context("When the runners reject the runner spec they were given", func() { + var ctx context.Context + var mgr ctrl.Manager + var autoscalingNS *corev1.Namespace + var autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet + + ephemeralRunnerSetKey := func() client.ObjectKey { + return client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace} + } + + listenerKey := func() client.ObjectKey { + return client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace} + } + + getEphemeralRunnerSet := func() *v1alpha1.EphemeralRunnerSet { + GinkgoHelper() + + runnerSet := new(v1alpha1.EphemeralRunnerSet) + Expect(k8sClient.Get(ctx, ephemeralRunnerSetKey(), runnerSet)).To(Succeed(), "failed to get the ephemeral runner set") + return runnerSet + } + + autoscalingRunnerSetPhase := func() (v1alpha1.AutoscalingRunnerSetPhase, error) { + updated := new(v1alpha1.AutoscalingRunnerSet) + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), updated); err != nil { + return "", err + } + return v1alpha1.AutoscalingRunnerSetPhase(updated.Status.Phase), nil + } + + // markRunnersOutdated stands in for the EphemeralRunnerSet controller + // reporting that the runners it created rejected the runner spec. The + // applied revision is moved up to the spec revision because that is the + // state the report is only meaningful in: the set is running the spec it + // is complaining about. + markRunnersOutdated := func() { + GinkgoHelper() + + runnerSet := getEphemeralRunnerSet() + original := runnerSet.DeepCopy() + runnerSet.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseOutdated + runnerSet.Status.AppliedActionableRevision = runnerSet.Spec.ActionableRevision + Expect(k8sClient.Status().Patch(ctx, runnerSet, client.MergeFrom(original))).To(Succeed(), "failed to mark the ephemeral runner set outdated") + } + + expectSwitchedOff := func() int64 { + GinkgoHelper() + + Eventually(autoscalingRunnerSetPhase, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval). + Should(BeEquivalentTo(v1alpha1.AutoscalingRunnerSetPhaseOutdated), "the autoscaling runner set should report the outdated phase") + + Eventually( + func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, listenerKey(), new(v1alpha1.AutoscalingListener))) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeTrue(), "the listener should be removed so no further jobs are acquired") + + // The set is kept, not deleted: deleting it would make the controller + // rebuild it from the same rejected spec on the very next reconcile. + var runnerSet *v1alpha1.EphemeralRunnerSet + Eventually( + func() (bool, error) { + runnerSet = new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, ephemeralRunnerSetKey(), runnerSet); err != nil { + return false, err + } + return runnerSet.Spec.Replicas == 0 && runnerSet.Spec.PatchID == 0, nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeTrue(), "the ephemeral runner set should be held at zero replicas") + + Consistently( + func() error { + return k8sClient.Get(ctx, ephemeralRunnerSetKey(), new(v1alpha1.EphemeralRunnerSet)) + }, + 2*time.Second, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "the ephemeral runner set should not be deleted while the scale set is outdated") + + return runnerSet.Spec.ActionableRevision + } + + expectRecovered := func(outdatedRevision int64) { + GinkgoHelper() + + Eventually( + func() (int64, error) { + runnerSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, ephemeralRunnerSetKey(), runnerSet); err != nil { + return 0, err + } + return runnerSet.Spec.ActionableRevision, nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeNumerically(">", outdatedRevision), "the runner spec revision should advance so the runner set stops judging itself by the rejected runners") + + Eventually( + func() error { + return k8sClient.Get(ctx, listenerKey(), new(v1alpha1.AutoscalingListener)) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "the listener should be created again so the scale set can acquire jobs") + + Eventually(autoscalingRunnerSetPhase, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval). + Should(BeEquivalentTo(v1alpha1.AutoscalingRunnerSetPhaseRunning), "the autoscaling runner set should leave the outdated phase") + } + + BeforeEach(func() { + ctx = context.Background() + autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) + configSecret := createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + + controller := &AutoscalingRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ControllerNamespace: autoscalingNS.Name, + DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( + scalefake.WithClient( + scalefake.NewClient( + scalefake.WithGetRunnerGroupByName(&scaleset.RunnerGroup{ID: 1, Name: "testgroup"}, nil), + scalefake.WithGetRunnerScaleSet(nil, nil), + scalefake.WithCreateRunnerScaleSet(&scaleset.RunnerScaleSet{ID: 1, Name: "test-asrs", RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil), + scalefake.WithDeleteRunnerScaleSet(nil), + ), + ), + )), + }, + } + Expect(controller.SetupWithManager(mgr)).To(Succeed(), "failed to setup controller") + startManagers(GinkgoT(), mgr) + + min := 1 + max := 10 + autoscalingRunnerSet = &v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-asrs", + Namespace: autoscalingNS.Name, + Labels: map[string]string{LabelKeyKubernetesVersion: buildVersion}, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{ + GitHubConfigUrl: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + MaxRunners: &max, + MinRunners: &min, + RunnerGroup: "testgroup", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "runner", + Image: "ghcr.io/actions/runner", + }, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, autoscalingRunnerSet)).To(Succeed(), "failed to create AutoScalingRunnerSet") + + Eventually( + func() error { + return k8sClient.Get(ctx, ephemeralRunnerSetKey(), new(v1alpha1.EphemeralRunnerSet)) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "the ephemeral runner set should be created") + + Eventually(autoscalingRunnerSetPhase, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval). + Should(BeEquivalentTo(v1alpha1.AutoscalingRunnerSetPhaseRunning), "the autoscaling runner set should settle before the runners reject the spec") + }) + + It("switches the scale set off and keeps the ephemeral runner set", func() { + markRunnersOutdated() + expectSwitchedOff() + }) + + It("recovers when the runner spec is corrected", func() { + markRunnersOutdated() + outdatedRevision := expectSwitchedOff() + + updated := new(v1alpha1.AutoscalingRunnerSet) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), updated)).To(Succeed()) + original := updated.DeepCopy() + updated.Spec.Template.Spec.Containers[0].Image = "ghcr.io/actions/runner:fixed" + Expect(k8sClient.Patch(ctx, updated, client.MergeFrom(original))).To(Succeed(), "failed to correct the runner spec") + + expectRecovered(outdatedRevision) + }) + + // The runner spec is not the only reason a scale set can be stuck: the + // runners may have been rejected because of the scale set registration + // rather than the pod template. Any spec edit therefore has to be enough + // to retry, otherwise the scale set can only be recovered by touching a + // field that has nothing to do with the failure. + It("recovers when a field outside the runner spec is updated", func() { + markRunnersOutdated() + outdatedRevision := expectSwitchedOff() + + updated := new(v1alpha1.AutoscalingRunnerSet) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), updated)).To(Succeed()) + original := updated.DeepCopy() + max := 20 + updated.Spec.MaxRunners = &max + Expect(k8sClient.Patch(ctx, updated, client.MergeFrom(original))).To(Succeed(), "failed to update the autoscaling runner set") + + expectRecovered(outdatedRevision) + }) + }) +})