From b9eaf560f8dcbe83da108b262955651e367bc99c Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Tue, 15 Sep 2026 16:23:37 +0200 Subject: [PATCH] Switch the scale set off instead of rebuilding it when runners are outdated (#4652) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalingrunnerset_controller.go | 181 +++++----- .../autoscalingrunnerset_controller_test.go | 323 ++++++++++++++++++ ...scalingrunnerset_outdated_recovery_test.go | 130 +++++++ controllers/actions.github.com/constants.go | 5 + controllers/actions.github.com/helpers.go | 20 ++ .../actions.github.com/resourcebuilder.go | 5 +- .../resourcebuilder_test.go | 6 +- 7 files changed, 586 insertions(+), 84 deletions(-) create mode 100644 controllers/actions.github.com/autoscalingrunnerset_outdated_recovery_test.go diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 6877dc4a..01ceb24a 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) { @@ -250,38 +202,28 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl case err != nil: 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") + case ephemeralRunnerSetOutdatedForAppliedRevision(&ephemeralRunnerSet) && + !ephemeralRunnerSetNeedsOutdatedRecovery(&ephemeralRunnerSet, &autoscalingRunnerSet): + // 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. This also covers Pending during a + // metadata-only listener rebuild; only an unobserved spec generation is a + // recovery signal. 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 +232,18 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if ephemeralRunnerSetActionableSpecChanged(&ephemeralRunnerSet, desired) { + recoveringFromOutdated := ephemeralRunnerSetOutdatedForAppliedRevision(&ephemeralRunnerSet) && + ephemeralRunnerSetNeedsOutdatedRecovery(&ephemeralRunnerSet, &autoscalingRunnerSet) + if ephemeralRunnerSetActionableSpecChanged(&ephemeralRunnerSet, desired) || recoveringFromOutdated { + // A real AutoscalingRunnerSet spec update leaves its observed generation + // behind until reconciliation succeeds. Require that signal before + // recovering an outdated set: Pending can also mean a metadata-only + // listener rebuild, which must not retry the same rejected runner spec. + // + // The revision has to advance even when the runner spec itself is + // unchanged. It tells the EphemeralRunnerSet to stop judging itself by + // the runners that failed, clearing its outdated phase and allowing it + // to scale up again. original := ephemeralRunnerSet.DeepCopy() ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec @@ -420,6 +373,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..06007964 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/watch" "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" "github.com/actions/actions-runner-controller/build" @@ -2854,3 +2855,325 @@ 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.Spec.Replicas = 3 + runnerSet.Spec.PatchID = 7 + Expect(k8sClient.Patch(ctx, runnerSet, client.MergeFrom(original))).To(Succeed(), "failed to seed a nonzero ephemeral runner set") + + 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) + }) + + It("does not retry an outdated runner spec during a metadata-only listener rebuild", func() { + listener := new(v1alpha1.AutoscalingListener) + Expect(k8sClient.Get(ctx, listenerKey(), listener)).To(Succeed()) + blockDeletion(listener) + defer unblockDeletion(listener) + + runnerSet := getEphemeralRunnerSet() + rejectedRevision := runnerSet.Spec.ActionableRevision + + updated := new(v1alpha1.AutoscalingRunnerSet) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), updated)).To(Succeed()) + original := updated.DeepCopy() + if updated.Labels == nil { + updated.Labels = map[string]string{} + } + updated.Labels["arc.test/listener-rebuild"] = "true" + Expect(k8sClient.Patch(ctx, updated, client.MergeFrom(original))).To(Succeed(), "failed to trigger a metadata-only listener rebuild") + + Eventually( + func(g Gomega) { + currentListener := new(v1alpha1.AutoscalingListener) + g.Expect(k8sClient.Get(ctx, listenerKey(), currentListener)).To(Succeed()) + g.Expect(currentListener.DeletionTimestamp).NotTo(BeNil(), "listener deletion should remain blocked") + + current := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) + g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhasePending)) + g.Expect(current.Generation).To(Equal(current.Status.ObservedGeneration), "metadata-only updates must not look like spec recovery") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + watchClient, err := client.NewWithWatch(cfg, client.Options{Scheme: mgr.GetScheme()}) + Expect(err).NotTo(HaveOccurred()) + listenerWatch, err := watchClient.Watch(ctx, new(v1alpha1.AutoscalingListenerList), client.InNamespace(autoscalingRunnerSet.Namespace)) + Expect(err).NotTo(HaveOccurred()) + defer listenerWatch.Stop() + + var listenerRecreated atomic.Bool + go func() { + for event := range listenerWatch.ResultChan() { + if event.Type != watch.Added { + continue + } + added, ok := event.Object.(*v1alpha1.AutoscalingListener) + if ok && added.Name == listenerKey().Name && added.UID != listener.UID { + listenerRecreated.Store(true) + } + } + }() + + markRunnersOutdated() + + Consistently( + func(g Gomega) { + current := getEphemeralRunnerSet() + g.Expect(current.Spec.ActionableRevision).To(Equal(rejectedRevision), "the rejected runner spec must not be retried without an AutoscalingRunnerSet spec update") + + currentListener := new(v1alpha1.AutoscalingListener) + g.Expect(k8sClient.Get(ctx, listenerKey(), currentListener)).To(Succeed()) + g.Expect(currentListener.DeletionTimestamp).NotTo(BeNil(), "the metadata-only listener rebuild should remain blocked") + }, + 3*time.Second, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + unblockDeletion(listener) + + Eventually(autoscalingRunnerSetPhase, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval). + Should(BeEquivalentTo(v1alpha1.AutoscalingRunnerSetPhaseOutdated), "the metadata-only rebuild should transition directly to outdated") + + Eventually( + func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, listenerKey(), new(v1alpha1.AutoscalingListener))) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeTrue(), "the outdated scale set should stay switched off") + + Consistently(listenerRecreated.Load, time.Second, autoscalingRunnerSetTestInterval). + Should(BeFalse(), "the listener must not be recreated for a rejected runner spec") + }) + }) +}) diff --git a/controllers/actions.github.com/autoscalingrunnerset_outdated_recovery_test.go b/controllers/actions.github.com/autoscalingrunnerset_outdated_recovery_test.go new file mode 100644 index 00000000..59fb327a --- /dev/null +++ b/controllers/actions.github.com/autoscalingrunnerset_outdated_recovery_test.go @@ -0,0 +1,130 @@ +/* +Copyright 2026 The actions-runner-controller authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package actionsgithubcom + +import ( + "context" + "strconv" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/actions-runner-controller/build" +) + +func TestAutoscalingRunnerSetParksFirstRejectionOfPublishedGeneration(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + const ( + name = "test" + namespace = "test" + generation = int64(2) + revision = int64(1) + ) + template := corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "runner", Image: "runner:new"}}, + }, + } + autoscalingRunnerSet := &v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Generation: generation, + Finalizers: []string{autoscalingRunnerSetFinalizerName}, + Labels: map[string]string{LabelKeyKubernetesVersion: build.Version}, + Annotations: map[string]string{ + runnerScaleSetIDAnnotationKey: "1", + AnnotationKeyGitHubRunnerGroupName: "group", + AnnotationKeyGitHubRunnerScaleSetName: name, + }, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{ + GitHubConfigUrl: "https://github.com/owner/repo", + Template: template, + }, + Status: v1alpha1.AutoscalingRunnerSetStatus{ + Phase: v1alpha1.AutoscalingRunnerSetPhasePending, + ObservedGeneration: generation - 1, + }, + } + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Annotations: map[string]string{ + AnnotationKeyAutoscalingRunnerSetGeneration: strconv.FormatInt(generation, 10), + }, + }, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + Replicas: 3, + PatchID: 7, + ActionableRevision: revision, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + RunnerScaleSetID: 1, + GitHubConfigURL: autoscalingRunnerSet.Spec.GitHubConfigUrl, + PodTemplateSpec: template, + }, + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + Phase: v1alpha1.EphemeralRunnerSetPhaseOutdated, + AppliedActionableRevision: revision, + }, + } + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(autoscalingRunnerSet, ephemeralRunnerSet). + WithObjects(autoscalingRunnerSet, ephemeralRunnerSet). + Build() + resourceCache := NewResourceCache() + reconciler := &AutoscalingRunnerSetReconciler{ + Client: c, + Scheme: scheme, + Log: logr.Discard(), + ControllerNamespace: namespace, + ResourceBuilder: ResourceBuilder{ + ResourceCache: &resourceCache, + Scheme: scheme, + }, + } + + _, err := reconciler.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: types.NamespacedName{Name: name, Namespace: namespace}, + }) + require.NoError(t, err) + + gotARS := new(v1alpha1.AutoscalingRunnerSet) + require.NoError(t, c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: namespace}, gotARS)) + require.Equal(t, v1alpha1.AutoscalingRunnerSetPhaseOutdated, gotARS.Status.Phase) + + gotERS := new(v1alpha1.EphemeralRunnerSet) + require.NoError(t, c.Get(context.Background(), types.NamespacedName{Name: name, Namespace: namespace}, gotERS)) + require.Equal(t, revision, gotERS.Spec.ActionableRevision) + require.Zero(t, gotERS.Spec.Replicas) + require.Zero(t, gotERS.Spec.PatchID) +} diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index af05eaf7..36ded027 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -50,6 +50,11 @@ const ( AnnotationKeyGitHubRunnerGroupName = "actions.github.com/runner-group-name" AnnotationKeyGitHubRunnerScaleSetName = "actions.github.com/runner-scale-set-name" AnnotationKeyPatchID = "actions.github.com/patch-id" + // AnnotationKeyAutoscalingRunnerSetGeneration records the AutoscalingRunnerSet + // generation that published the current EphemeralRunnerSet actionable + // revision. It prevents a rejected revision from being retried more than once + // for the same AutoscalingRunnerSet spec update. + AnnotationKeyAutoscalingRunnerSetGeneration = "actions.github.com/autoscaling-runner-set-generation" // AnnotationKeyActionableRevision records the EphemeralRunnerSet // Spec.ActionableRevision that was in effect when the runner was created. It // lets the set tell apart a runner that reported Outdated against the current diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go index d0830111..c47f8bd3 100644 --- a/controllers/actions.github.com/helpers.go +++ b/controllers/actions.github.com/helpers.go @@ -1,11 +1,31 @@ package actionsgithubcom import ( + "strconv" + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" corev1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" ) +func ephemeralRunnerSetNeedsOutdatedRecovery(ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) bool { + if ephemeralRunnerSet == nil || autoscalingRunnerSet == nil || + autoscalingRunnerSet.Generation <= autoscalingRunnerSet.Status.ObservedGeneration { + return false + } + + publishedGeneration, err := strconv.ParseInt( + ephemeralRunnerSet.Annotations[AnnotationKeyAutoscalingRunnerSetGeneration], + 10, + 64, + ) + if err != nil { + return true + } + + return publishedGeneration < autoscalingRunnerSet.Generation +} + // ephemeralRunnerSetActionableSpecChanged reports whether the runner spec the // EphemeralRunnerSet is running differs from the one derived from the // AutoscalingRunnerSet, in a way that requires re-applying it to the runners. diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index 4dc13f5b..043035c2 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -770,8 +770,9 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A } annotations := map[string]string{ - AnnotationKeyGitHubRunnerGroupName: autoscalingRunnerSet.Annotations[AnnotationKeyGitHubRunnerGroupName], - AnnotationKeyGitHubRunnerScaleSetName: autoscalingRunnerSet.Annotations[AnnotationKeyGitHubRunnerScaleSetName], + AnnotationKeyGitHubRunnerGroupName: autoscalingRunnerSet.Annotations[AnnotationKeyGitHubRunnerGroupName], + AnnotationKeyGitHubRunnerScaleSetName: autoscalingRunnerSet.Annotations[AnnotationKeyGitHubRunnerScaleSetName], + AnnotationKeyAutoscalingRunnerSetGeneration: strconv.FormatInt(autoscalingRunnerSet.Generation, 10), } if autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata != nil { diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index b472143b..71febed0 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -16,8 +16,9 @@ import ( func TestMetadataPropagation(t *testing.T) { autoscalingRunnerSet := v1alpha1.AutoscalingRunnerSet{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-scale-set", - Namespace: "test-ns", + Name: "test-scale-set", + Namespace: "test-ns", + Generation: 7, Labels: map[string]string{ LabelKeyKubernetesPartOf: labelValueKubernetesPartOf, LabelKeyKubernetesVersion: "0.2.0", @@ -123,6 +124,7 @@ func TestMetadataPropagation(t *testing.T) { assert.Equal(t, "repo", ephemeralRunnerSet.Labels[LabelKeyGitHubRepository]) assert.Equal(t, autoscalingRunnerSet.Annotations[AnnotationKeyGitHubRunnerGroupName], ephemeralRunnerSet.Annotations[AnnotationKeyGitHubRunnerGroupName]) assert.Equal(t, autoscalingRunnerSet.Annotations[AnnotationKeyGitHubRunnerScaleSetName], ephemeralRunnerSet.Annotations[AnnotationKeyGitHubRunnerScaleSetName]) + assert.Equal(t, "7", ephemeralRunnerSet.Annotations[AnnotationKeyAutoscalingRunnerSetGeneration]) assert.Equal(t, autoscalingRunnerSet.Labels["arbitrary-label"], ephemeralRunnerSet.Labels["arbitrary-label"]) assert.Equal(t, "ephemeral-runner-set-label", ephemeralRunnerSet.Labels["test.com/ephemeral-runner-set-label"]) assert.Equal(t, "ephemeral-runner-set-annotation", ephemeralRunnerSet.Annotations["test.com/ephemeral-runner-set-annotation"])