From 6e4d85f28f211ab01bf8b0cbdcc4fee2bd3169c8 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Mon, 14 Sep 2026 14:20:33 +0200 Subject: [PATCH] Make the Outdated phase revision-aware (#4644) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalingrunnerset_controller.go | 2 +- .../autoscalingrunnerset_controller_test.go | 142 +++++++++ controllers/actions.github.com/constants.go | 6 + .../ephemeralrunnerset_controller.go | 169 +++++++++-- .../ephemeralrunnerset_controller_test.go | 270 +++++++++++++++++- .../ephemeralrunnerset_phase_read_test.go | 117 ++++++++ .../ephemeralrunnerset_revision_patch_test.go | 2 + ...meralrunnerset_scaleup_suppression_test.go | 4 + .../ephemeralrunnerset_stale_phase_test.go | 114 ++++++++ .../ephemeralrunnerset_stale_target_test.go | 118 ++++++++ controllers/actions.github.com/helpers.go | 24 ++ .../helpers_outdated_test.go | 251 ++++++++++++++++ controllers/actions.github.com/indexer.go | 14 + .../actions.github.com/resourcebuilder.go | 3 +- .../resourcebuilder_test.go | 49 ++++ 15 files changed, 1248 insertions(+), 37 deletions(-) create mode 100644 controllers/actions.github.com/ephemeralrunnerset_phase_read_test.go create mode 100644 controllers/actions.github.com/ephemeralrunnerset_stale_phase_test.go create mode 100644 controllers/actions.github.com/ephemeralrunnerset_stale_target_test.go create mode 100644 controllers/actions.github.com/helpers_outdated_test.go diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 5b8a664c..3086f3de 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -248,7 +248,7 @@ 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 ephemeralRunnerSet.Status.Phase == v1alpha1.EphemeralRunnerSetPhaseOutdated && autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseRunning: + 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) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index 2965683a..96f50010 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -2545,6 +2545,148 @@ var _ = Describe("Test AutoscalingRunnerSet with a stale runner scale set", Orde autoscalingRunnerSetTestInterval, ).Should(BeEquivalentTo(freshRunnerScaleSetID), "the listener should never be created with the stale runner scale set ID") }) + + // The listener is not the only thing that has to follow a re-registration: + // the EphemeralRunnerSet carries the scale set ID down to every runner, so + // assert the propagation here rather than only on the listener. + It("propagates the fresh runner scale set ID to the EphemeralRunnerSet", func() { + runnerSet := new(v1alpha1.EphemeralRunnerSet) + Eventually( + func() (int, error) { + if err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet); err != nil { + return 0, err + } + return runnerSet.Spec.EphemeralRunnerSpec.RunnerScaleSetID, nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeEquivalentTo(freshRunnerScaleSetID), + "the EphemeralRunnerSet must be re-pointed at the newly registered runner scale set") + + // The runners themselves are still registered against the dead scale + // set, so the revision has to advance for them to be cleaned up. + Expect(runnerSet.Spec.ActionableRevision).To(BeNumerically(">", 0), + "re-registration must bump ActionableRevision so existing runners are replaced") + }) + }) + + // A scale set can lose its Actions service counterpart long after it has + // settled, and re-registration then changes the runner scale set ID without + // anything in the AutoscalingRunnerSet spec changing. This exercises that + // full transition. + // + // It also pins down why drift detection cannot be keyed on + // metadata.generation: re-registration writes the ID as an annotation, and + // metadata changes do not bump generation. A generation-based shortcut would + // leave the EphemeralRunnerSet pointing at the dead scale set forever. + Context("When a settled runner scale set disappears from the Actions service", func() { + const originalRunnerScaleSetID = 77 + const replacementRunnerScaleSetID = 78 + + var ctx context.Context + var mgr ctrl.Manager + var autoscalingNS *corev1.Namespace + var autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet + var scaleSetDeleted atomic.Bool + + BeforeEach(func() { + ctx = context.Background() + autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) + configSecret := createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + scaleSetDeleted.Store(false) + + 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.WithGetRunnerScaleSetByIDFunc(func(_ context.Context, runnerScaleSetID int) (*scaleset.RunnerScaleSet, error) { + if runnerScaleSetID == originalRunnerScaleSetID && scaleSetDeleted.Load() { + return nil, scaleset.NotFoundError + } + return &scaleset.RunnerScaleSet{ID: runnerScaleSetID, Name: "test-asrs", RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil + }), + scalefake.WithGetRunnerScaleSet(nil, nil), + scalefake.WithCreateRunnerScaleSet(&scaleset.RunnerScaleSet{ID: replacementRunnerScaleSetID, Name: "test-asrs", RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil), + scalefake.WithDeleteRunnerScaleSet(nil), + ), + ), + )), + }, + } + Expect(controller.SetupWithManager(mgr)).To(Succeed(), "failed to setup controller") + startManagers(GinkgoT(), mgr) + + autoscalingRunnerSet = newAutoscalingRunnerSet(autoscalingNS.Name, configSecret.Name, registeredAnnotations(originalRunnerScaleSetID)) + // Set the scale set name explicitly. createRunnerScaleSet defaults an + // empty Spec.RunnerScaleSetName to the object name, and that spec write + // bumps metadata.generation, which would let a generation-based + // shortcut pass this test for the wrong reason. + autoscalingRunnerSet.Spec.RunnerScaleSetName = "test-asrs" + Expect(k8sClient.Create(ctx, autoscalingRunnerSet)).To(Succeed(), "failed to create AutoScalingRunnerSet") + }) + + It("re-points the EphemeralRunnerSet without any spec change on the AutoscalingRunnerSet", func() { + // Let the scale set settle first, so observedGeneration catches up with + // generation and the re-registration below is the only thing in flight. + var settledGeneration int64 + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) + g.Expect(current.Status.ObservedGeneration).To(Equal(current.Generation), + "AutoscalingRunnerSet should reach a settled state") + settledGeneration = current.Generation + + runnerSet := new(v1alpha1.EphemeralRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet)).To(Succeed()) + g.Expect(runnerSet.Spec.EphemeralRunnerSpec.RunnerScaleSetID).To(Equal(originalRunnerScaleSetID)) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + // The scale set is deleted on the Actions service side. Nothing about + // the AutoscalingRunnerSet spec changes as a result. + scaleSetDeleted.Store(true) + + // Re-registration is only considered when the listener has to be + // created, so drop the listener the way an operator or an eviction + // would. This deliberately does not touch the AutoscalingRunnerSet, so + // its generation stays put. + listener := new(v1alpha1.AutoscalingListener) + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingNS.Name}, listener)).To(Succeed()) + Expect(k8sClient.Delete(ctx, listener)).To(Succeed()) + + Eventually( + func(g Gomega) { + current := new(v1alpha1.EphemeralRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current)).To(Succeed()) + g.Expect(current.Spec.EphemeralRunnerSpec.RunnerScaleSetID).To(Equal(replacementRunnerScaleSetID), + "the EphemeralRunnerSet must be re-pointed at the newly registered runner scale set even though the AutoscalingRunnerSet spec never changed") + g.Expect(current.Spec.ActionableRevision).To(BeNumerically(">", 0), + "re-registration must bump ActionableRevision so runners registered against the dead scale set are replaced") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + // Guard the premise of the test: re-registration must reach the + // EphemeralRunnerSet purely through a spec content change. If it ever + // starts writing to the AutoscalingRunnerSet spec, generation would + // bump and this would stop demonstrating that. + settled := new(v1alpha1.AutoscalingRunnerSet) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), settled)).To(Succeed()) + Expect(settled.Generation).To(Equal(settledGeneration), + "re-registration must not change the AutoscalingRunnerSet spec, otherwise this test proves nothing") + }) }) Context("When the Actions service cannot confirm whether the runner scale set exists", func() { diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index 48c78e4b..af05eaf7 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -50,6 +50,12 @@ const ( AnnotationKeyGitHubRunnerGroupName = "actions.github.com/runner-group-name" AnnotationKeyGitHubRunnerScaleSetName = "actions.github.com/runner-scale-set-name" AnnotationKeyPatchID = "actions.github.com/patch-id" + // 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 + // runner spec from one that reported it against a spec that has since been + // updated. + AnnotationKeyActionableRevision = "actions.github.com/actionable-revision" // AnnotationKeyListenerConfigResourceVersion records the resource version of // the listener config secret the listener pod was created from. The pod // mounts that secret and parses it once at startup, so a change to its diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index dbb248a1..cf73bd51 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -22,6 +22,7 @@ import ( "errors" "fmt" "maps" + "slices" "sort" "strconv" "time" @@ -196,11 +197,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } - ephemeralRunnersByState := newEphemeralRunnersByStates(&ephemeralRunnerList) + ephemeralRunnersByState := newEphemeralRunnersByStates(&ephemeralRunnerList, ephemeralRunnerSet.Status.AppliedActionableRevision) log.Info( "Ephemeral runner counts", "outdated", len(ephemeralRunnersByState.outdated), + "staleOutdated", len(ephemeralRunnersByState.staleOutdated), "pending", len(ephemeralRunnersByState.pending), "running", len(ephemeralRunnersByState.running), "finished", len(ephemeralRunnersByState.finished), @@ -208,6 +210,23 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R "deleting", len(ephemeralRunnersByState.deleting), ) + // Runners that reported Outdated against a runner spec that has since been + // replaced are not evidence about the current spec. Drop them so the scaling + // logic below replaces them with runners built from the current spec, instead + // of letting them hold the set in the Outdated phase forever. + if len(ephemeralRunnersByState.staleOutdated) > 0 { + log.Info( + "Deleting outdated ephemeral runners created before the last spec update so they can be replaced", + "count", len(ephemeralRunnersByState.staleOutdated), + "appliedActionableRevision", ephemeralRunnerSet.Status.AppliedActionableRevision, + ) + if err := r.deleteTerminatedEphemeralRunners(ctx, ephemeralRunnersByState.staleOutdated, log); err != nil { + log.Error(err, "failed to delete stale outdated ephemeral runners") + return ctrl.Result{}, err + } + return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) + } + total := ephemeralRunnersByState.scaleTotal() if ephemeralRunnerSet.Spec.PatchID == 0 || ephemeralRunnerSet.Spec.PatchID != ephemeralRunnersByState.latestPatchID { // Spec.Replicas is the count the listener asked for when it published @@ -284,8 +303,11 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) } -// patchAppliedActionableRevisionStatus records that the runner spec carried by -// targetAppliedRevision has been fully applied. +// patchAppliedActionableRevisionStatus brings status into line with the runner +// spec carried by targetAppliedRevision once that spec has been fully applied. +// It records the applied revision, clears the scale-up suppression marker when +// the revision actually advances, and re-derives Status.Phase from the child +// runners. // // The marker lives in status rather than in an annotation on the spec, and it is // written only once the cleanup above has actually succeeded. If the controller @@ -315,6 +337,11 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R // if the re-fetched object is still the live one, so a successful patch proves // the monotonicity check above was evaluated against live data. A stale attempt // conflicts and is retried or requeued instead of regressing the marker. +// +// The lock covers the EphemeralRunnerSet object and nothing else. The phase is +// derived from a separate list of the child runners, which no precondition on +// this patch can vouch for, so that list is read through the same authoritative +// reader rather than the cache. func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error { return retry.RetryOnConflict(retry.DefaultBackoff, func() error { var latest v1alpha1.EphemeralRunnerSet @@ -326,22 +353,88 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx return err } - if latest.Status.AppliedActionableRevision >= targetAppliedRevision { - return nil + original := latest.DeepCopy() + + // Only an advance means the idle and pending runners were just deleted and + // the listener restarted. Guarding both writes on it keeps this callable + // as a plain "make sure status reflects revision N" without disturbing a + // marker that still describes the live patch sequence. + if latest.Status.AppliedActionableRevision < targetAppliedRevision { + latest.Status.AppliedActionableRevision = targetAppliedRevision + + // The marker records a patch ID from the sequence that was current + // before this spec change. Applying a new revision deletes the idle and + // pending runners, so the shortfall that follows belongs to the new spec + // and must be filled. Worse, a spec change restarts the listener, and a + // restarted listener numbers its patches from 0 upwards, counting + // through every integer. It therefore passes through a leftover marker + // value with near-certainty, and would suppress the very scale up that + // rebuilds the pool. + latest.Status.FinishedRunnerCleanupPatchID = 0 } - original := latest.DeepCopy() - latest.Status.AppliedActionableRevision = targetAppliedRevision + ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList) + // Listed through the same authoritative reader as the Get above. The + // optimistic lock on the patch below covers the EphemeralRunnerSet object + // only, so it cannot vouch for a separately-read list: deriving the phase + // from the cache would let a successful, lock-protected write carry a + // value the lock says nothing about. The list also sits inside + // RetryOnConflict, and a cached list can return the same stale data on + // every attempt, spending the whole backoff re-deriving one wrong phase. + // + // resourceOwnerKey cannot be used here. It is a client-side index + // registered on the manager's cache, and the API server rejects it as an + // unsupported field label, so the ownership filter has to be applied in + // this process instead. + // + // Narrowing server-side by label is not a safe alternative either. Label + // propagation is operator-configurable through + // --exclude-label-propagation-prefix, so the scale set labels are not + // guaranteed to reach the runners, and a selector that silently matched + // none of them would derive the phase from an empty list rather than + // fail. A namespace can hold more than one scale set, so this does read + // runners that are not ours, but it only runs when a revision actually + // advances rather than on every reconcile. + if err := reader.List(ctx, ephemeralRunnerList, client.InNamespace(latest.Namespace)); err != nil { + return fmt.Errorf("failed to list child ephemeral runners: %w", err) + } + ephemeralRunnerList.Items = slices.DeleteFunc(ephemeralRunnerList.Items, func(runner v1alpha1.EphemeralRunner) bool { + return !isControlledBy(&runner, "EphemeralRunnerSet", latest.Name) + }) - // The marker records a patch ID from the sequence that was current before - // this spec change. Applying a new revision deletes the idle and pending - // runners, so the shortfall that follows belongs to the new spec and must - // be filled. Worse, a spec change restarts the listener, and a restarted - // listener numbers its patches from 0 upwards, counting through every - // integer. It therefore passes through a leftover marker value with - // near-certainty, and would suppress the very scale up that rebuilds the - // pool. - latest.Status.FinishedRunnerCleanupPatchID = 0 + // Judge the runners against the revision the set has now applied, rather + // than the one this call was asked to apply: every runner created before + // that revision is stale by definition, so its Outdated report says + // nothing about the current spec. This is what lets a spec update clear + // the Outdated phase immediately rather than waiting for the pre-update + // runners to be collected. + // + // After the guard above, this field is max(live, target), and the two + // differ in a case that matters. The caller reads the spec from the + // cache while this function re-reads the status from the API server, so + // a lagging reconcile can arrive with a target behind the live marker. + // Judging against that lower target would rate a runner left over from + // the superseded revision as current and flip a set that has already + // moved on back to Outdated. That phase is deliberately absorbing, so + // the set would then stay switched off until the next spec change. + state := newEphemeralRunnersByStates(ephemeralRunnerList, latest.Status.AppliedActionableRevision) + + // Set the phase in both directions. This function returns early from + // Reconcile without reaching updateStatus, so leaving the phase untouched + // would let a stale value survive: a stale Running would hide genuinely + // outdated runners from the cleanup path, and a stale Outdated would keep + // the set switched off after the spec that caused it was replaced. + if len(state.outdated) > 0 { + latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseOutdated + } else { + latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + } + + // Checked after every field above has been set, so that clearing the + // marker alone is still enough to issue the patch. + if original.Status == latest.Status { + return nil + } return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})) }) @@ -539,7 +632,7 @@ func (r *EphemeralRunnerSetReconciler) cleanUpEphemeralRunners(ctx context.Conte return true, nil } - ephemeralRunnerState := newEphemeralRunnersByStates(ephemeralRunnerList) + ephemeralRunnerState := newEphemeralRunnersByStates(ephemeralRunnerList, ephemeralRunnerSet.Status.AppliedActionableRevision) log.Info( "Clean up runner counts", @@ -914,12 +1007,27 @@ type ephemeralRunnersByState struct { finished []*v1alpha1.EphemeralRunner failed []*v1alpha1.EphemeralRunner deleting []*v1alpha1.EphemeralRunner + // outdated holds runners that reported Outdated against the runner spec that + // is currently applied. They are evidence that the current spec is still + // rejected by the service, so they drive the set into the Outdated phase. outdated []*v1alpha1.EphemeralRunner + // staleOutdated holds runners that reported Outdated against a runner spec + // that has since been replaced. They say nothing about the current spec, so + // they must not drive the set into the Outdated phase; they are deleted and + // replaced by runners built from the current spec instead. + staleOutdated []*v1alpha1.EphemeralRunner latestPatchID int } -func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerList) *ephemeralRunnersByState { +// newEphemeralRunnersByStates groups the child runners by state. +// +// appliedActionableRevision is the EphemeralRunnerSet revision the runners are +// being judged against. A runner that reported Outdated before that revision was +// applied is classified as stale rather than outdated, so that updating the +// runner spec clears the Outdated phase immediately instead of waiting for the +// pre-update runners to disappear. +func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerList, appliedActionableRevision int64) *ephemeralRunnersByState { var ephemeralRunnerState ephemeralRunnersByState for i := range ephemeralRunnerList.Items { @@ -941,7 +1049,11 @@ func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerLi case v1alpha1.EphemeralRunnerPhaseFailed: ephemeralRunnerState.failed = append(ephemeralRunnerState.failed, r) case v1alpha1.EphemeralRunnerPhaseOutdated: - ephemeralRunnerState.outdated = append(ephemeralRunnerState.outdated, r) + if ephemeralRunnerActionableRevision(r) < appliedActionableRevision { + ephemeralRunnerState.staleOutdated = append(ephemeralRunnerState.staleOutdated, r) + } else { + ephemeralRunnerState.outdated = append(ephemeralRunnerState.outdated, r) + } default: // Pending or no phase should be considered as pending. // @@ -953,8 +1065,25 @@ func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerLi return &ephemeralRunnerState } +// ephemeralRunnerActionableRevision reports the EphemeralRunnerSet revision the +// runner was created from. Runners created before this annotation existed report +// 0, which matches the zero value of Status.AppliedActionableRevision, so they +// are treated as current until the spec is updated for the first time. +func ephemeralRunnerActionableRevision(ephemeralRunner *v1alpha1.EphemeralRunner) int64 { + revision, err := strconv.ParseInt(ephemeralRunner.Annotations[AnnotationKeyActionableRevision], 10, 64) + if err != nil { + return 0 + } + return revision +} + func (s *ephemeralRunnersByState) terminated() []*v1alpha1.EphemeralRunner { - return append(s.finished, append(s.failed, s.outdated...)...) + terminated := make([]*v1alpha1.EphemeralRunner, 0, len(s.finished)+len(s.failed)+len(s.outdated)+len(s.staleOutdated)) + terminated = append(terminated, s.finished...) + terminated = append(terminated, s.failed...) + terminated = append(terminated, s.outdated...) + terminated = append(terminated, s.staleOutdated...) + return terminated } func (s *ephemeralRunnersByState) scaleTotal() int { diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index a2d4ccd1..993c62e3 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -2330,9 +2330,10 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { It("does not clean up runners on initial creation without an actionable revision", func() { controller := &EphemeralRunnerSetReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Log: logf.Log, + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Scheme: mgr.GetScheme(), + Log: logf.Log, ResourceBuilder: ResourceBuilder{ ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( @@ -2383,9 +2384,10 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { It("deletes runner-a-idle, keeps runner-b-busy, and advances applied actionable revision 3 to 4", func() { controller := &EphemeralRunnerSetReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Log: logf.Log, + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Scheme: mgr.GetScheme(), + Log: logf.Log, ResourceBuilder: ResourceBuilder{ ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( @@ -2495,9 +2497,10 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { It("keeps applied actionable revision at 3 when cleanup fails", func() { controller := &EphemeralRunnerSetReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Log: logf.Log, + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Scheme: mgr.GetScheme(), + Log: logf.Log, ResourceBuilder: ResourceBuilder{ ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( @@ -2573,9 +2576,10 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { It("deletes unregistered pending runner during actionable revision cleanup after restart with no cache", func() { controller := &EphemeralRunnerSetReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Log: logf.Log, + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Scheme: mgr.GetScheme(), + Log: logf.Log, ResourceBuilder: ResourceBuilder{ ResourceCache: newTestResourceCache(), // fresh empty cache simulating restart SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient()), @@ -2652,9 +2656,10 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { It("preserves AppliedActionableRevision during status-only phase updates", func() { controller := &EphemeralRunnerSetReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Log: logf.Log, + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Scheme: mgr.GetScheme(), + Log: logf.Log, ResourceBuilder: ResourceBuilder{ ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( @@ -2696,10 +2701,16 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { Expect(err).NotTo(HaveOccurred()) // Create a runner that will cause phase change (outdated runner). + // It must carry the revision the set has applied, otherwise it is an + // outdated report about a runner spec that has already been replaced and + // the set deliberately ignores it. ephemeralRunner := &v1alpha1.EphemeralRunner{ ObjectMeta: metav1.ObjectMeta{ Name: "test-runner-outdated", Namespace: autoscalingNS.Name, + Annotations: map[string]string{ + AnnotationKeyActionableRevision: "5", + }, Labels: map[string]string{ LabelKeyGitHubScaleSetName: ephemeralRunnerSet.Name, LabelKeyGitHubScaleSetNamespace: ephemeralRunnerSet.Namespace, @@ -2756,4 +2767,233 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { g.Expect(updatedSet.Status.AppliedActionableRevision).To(Equal(int64(5)), "AppliedActionableRevision should be preserved") }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) }) + + // A runner that reported Outdated against a runner spec that has since been + // replaced must not drag the whole set back into the Outdated phase, because + // that switches the scale set off and discards the update the user just made. + // The runner is deleted instead, so the scaling logic replaces it with one + // built from the current spec. + It("replaces outdated runners from a superseded revision instead of going Outdated", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient()), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-stale-outdated", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 2, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:new"}}}}, + }, + }, + } + Expect(k8sClient.Create(ctx, ephemeralRunnerSet)).To(Succeed()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + // The set is already running revision 2. + current := new(v1alpha1.EphemeralRunnerSet) + Expect(k8sClient.Get(ctx, request.NamespacedName, current)).To(Succeed()) + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 2 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + Expect(k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current))).To(Succeed()) + + // A runner left over from revision 1 reports Outdated. This happens when a + // runner was busy with a job while the spec was updated, so it survived the + // revision cleanup and only exited (with the outdated exit code) afterwards. + staleRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner-from-old-revision", + Namespace: autoscalingNS.Name, + Annotations: map[string]string{AnnotationKeyActionableRevision: "1"}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: ephemeralRunnerSet.Name, + UID: ephemeralRunnerSet.UID, + Controller: func(b bool) *bool { return &b }(true), + BlockOwnerDeletion: func(b bool) *bool { return &b }(true), + }, + }, + }, + Spec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:old"}}}}, + }, + } + Expect(k8sClient.Create(ctx, staleRunner)).To(Succeed()) + + runnerStatusUpdated := staleRunner.DeepCopy() + runnerStatusUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseOutdated + Expect(k8sClient.Status().Patch(ctx, runnerStatusUpdated, client.MergeFrom(staleRunner))).To(Succeed()) + + Eventually(func(g Gomega) { + cachedRunner := new(v1alpha1.EphemeralRunner) + g.Expect(controller.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: staleRunner.Name}, cachedRunner)).To(Succeed()) + g.Expect(cachedRunner.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerPhaseOutdated)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // The stale runner is removed rather than being treated as a verdict on the + // current spec. + Eventually(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + runner := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: staleRunner.Name}, runner) + g.Expect(kerrors.IsNotFound(err) || !runner.DeletionTimestamp.IsZero()).To(BeTrue(), "stale outdated runner should be deleted") + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // And the set never reports Outdated because of it. + Consistently(func(g Gomega) { + updatedSet := new(v1alpha1.EphemeralRunnerSet) + g.Expect(k8sClient.Get(ctx, request.NamespacedName, updatedSet)).To(Succeed()) + g.Expect(updatedSet.Status.Phase).NotTo(Equal(v1alpha1.EphemeralRunnerSetPhaseOutdated)) + }, "2s", ephemeralRunnerSetTestInterval).Should(Succeed()) + }) + + // Once a runner reports Outdated against the applied revision, the set is + // switched off: it reports Outdated and drains. Every runner that is not + // executing a job is removed, a runner holding a job is kept until that job + // finishes, and the set ends up at zero runners. It must not refill, because + // any replacement would be built from the same spec and report Outdated again. + It("drains every runner that is not running a job and scales to zero", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient()), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-outdated-drain", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + Replicas: 3, + ActionableRevision: 1, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:current"}}}}, + }, + }, + } + Expect(k8sClient.Create(ctx, ephemeralRunnerSet)).To(Succeed()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + current := new(v1alpha1.EphemeralRunnerSet) + Expect(k8sClient.Get(ctx, request.NamespacedName, current)).To(Succeed()) + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 1 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + Expect(k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current))).To(Succeed()) + + // Every runner below carries the revision the set has applied, so none of + // them is a stale report about a superseded spec. + createRunner := func(name string, phase v1alpha1.EphemeralRunnerPhase, jobID string) *v1alpha1.EphemeralRunner { + runner := newRunner(name, ephemeralRunnerSet) + runner.Annotations = map[string]string{AnnotationKeyActionableRevision: "1"} + Expect(k8sClient.Create(ctx, runner)).To(Succeed()) + + if phase == "" && jobID == "" { + return runner + } + withStatus := runner.DeepCopy() + withStatus.Status.Phase = phase + withStatus.Status.RunnerID = 1 + withStatus.Status.JobID = jobID + Expect(k8sClient.Status().Patch(ctx, withStatus, client.MergeFrom(runner))).To(Succeed()) + return runner + } + + outdatedRunner := createRunner("drain-outdated", v1alpha1.EphemeralRunnerPhaseOutdated, "") + pendingRunner := createRunner("drain-pending", "", "") + idleRunner := createRunner("drain-idle", v1alpha1.EphemeralRunnerPhaseRunning, "") + busyRunner := createRunner("drain-busy", v1alpha1.EphemeralRunnerPhaseRunning, "job-1") + + // The outdated report is about the current spec, so the set switches off. + Eventually(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + g.Expect(k8sClient.Get(ctx, request.NamespacedName, updatedSet)).To(Succeed()) + g.Expect(updatedSet.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerSetPhaseOutdated)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + gone := func(g Gomega, name string) { + runner := new(v1alpha1.EphemeralRunner) + err := k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: name}, runner) + g.Expect(kerrors.IsNotFound(err) || !runner.DeletionTimestamp.IsZero()).To(BeTrue(), "runner %q should be deleted", name) + } + + // Draining removes everything that is not executing a job, and the runner + // holding a job survives it. + Eventually(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + gone(g, outdatedRunner.Name) + gone(g, pendingRunner.Name) + gone(g, idleRunner.Name) + + stillThere := new(v1alpha1.EphemeralRunner) + g.Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: busyRunner.Name}, stillThere)).To(Succeed()) + g.Expect(stillThere.DeletionTimestamp.IsZero()).To(BeTrue(), "a runner executing a job must not be deleted") + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // The set must not refill the drained capacity while it is Outdated. + Consistently(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + runnerList := new(v1alpha1.EphemeralRunnerList) + g.Expect(k8sClient.List(ctx, runnerList, client.InNamespace(autoscalingNS.Name))).To(Succeed()) + g.Expect(runnerList.Items).To(HaveLen(1), "no replacement runners while Outdated") + }, "2s", ephemeralRunnerSetTestInterval).Should(Succeed()) + + // When the job finishes, the last runner goes too and the set reaches zero. + finished := new(v1alpha1.EphemeralRunner) + Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: busyRunner.Name}, finished)).To(Succeed()) + done := finished.DeepCopy() + done.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded + done.Status.JobID = "" + Expect(k8sClient.Status().Patch(ctx, done, client.MergeFrom(finished))).To(Succeed()) + + Eventually(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + runnerList := new(v1alpha1.EphemeralRunnerList) + g.Expect(k8sClient.List(ctx, runnerList, client.InNamespace(autoscalingNS.Name))).To(Succeed()) + g.Expect(runnerList.Items).To(BeEmpty(), "the set must scale back to zero") + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + }) }) diff --git a/controllers/actions.github.com/ephemeralrunnerset_phase_read_test.go b/controllers/actions.github.com/ephemeralrunnerset_phase_read_test.go new file mode 100644 index 00000000..23f7247c --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_phase_read_test.go @@ -0,0 +1,117 @@ +package actionsgithubcom + +import ( + "context" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// TestPatchAppliedActionableRevisionStatusDerivesPhaseFromAuthoritativeRead +// pins the read that the derived phase is computed from. +// +// patchAppliedActionableRevisionStatus reads the EphemeralRunnerSet through +// APIReader precisely because a cached read cannot be trusted here, then +// derives Status.Phase from a list of the child runners. If that list goes +// through the cache-backed client instead, the phase is derived from data the +// surrounding optimistic lock does not cover: the lock proves only that the +// EphemeralRunnerSet was live at write time, never that the list was. +// +// A wrong phase does not merely flap. Reconcile's Outdated branch cleans up and +// returns before reaching updateStatus, and this function only runs while +// spec > applied, which it then makes false by advancing the marker. So a phase +// wrongly set to Outdated is never recomputed, and the set stays switched off. +// +// The two clients below disagree on purpose. The authoritative one holds no +// runners, so the correct phase is Running. The cached one holds an Outdated +// runner at the revision being applied, which is the phase a cached list would +// produce. Listing through the cache-backed client must fail this test. +func TestPatchAppliedActionableRevisionStatusDerivesPhaseFromAuthoritativeRead(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + newEphemeralRunnerSet := func() *v1alpha1.EphemeralRunnerSet { + return &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ers", + Namespace: "default", + }, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 7, + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + AppliedActionableRevision: 2, + }, + } + } + + // Phase Outdated at the revision being applied, so the classifier counts it + // as outdated rather than staleOutdated. + controllerRef := true + staleView := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner-from-cache", + Namespace: "default", + Annotations: map[string]string{ + AnnotationKeyActionableRevision: "7", + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: "test-ers", + UID: "test-uid", + Controller: &controllerRef, + }, + }, + }, + Status: v1alpha1.EphemeralRunnerStatus{ + Phase: v1alpha1.EphemeralRunnerPhaseOutdated, + }, + } + + builder := func(objs ...client.Object) client.Client { + return fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objs...). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). + Build() + } + + // The cache-backed client still sees a runner the API server no longer has. + cached := builder(newEphemeralRunnerSet(), staleView) + // The authoritative view: the runner is gone, so nothing is outdated. + authoritative := builder(newEphemeralRunnerSet()) + + reconciler := &EphemeralRunnerSetReconciler{ + Client: cached, + APIReader: authoritative, + Log: logr.Discard(), + Scheme: scheme, + } + + key := types.NamespacedName{Namespace: "default", Name: "test-ers"} + require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 7)) + + var patched v1alpha1.EphemeralRunnerSet + require.NoError(t, cached.Get(context.Background(), key, &patched)) + + assert.Equal( + t, + v1alpha1.EphemeralRunnerSetPhaseRunning, + patched.Status.Phase, + "phase must be derived from the authoritative read, not the cache: the cached client holds an outdated runner the API server no longer has, and a phase of Outdated is never recomputed because Reconcile's Outdated branch returns before updateStatus", + ) + assert.Equal(t, int64(7), patched.Status.AppliedActionableRevision) +} diff --git a/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go b/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go index df3f6309..61901c3c 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go @@ -57,6 +57,7 @@ func TestPatchAppliedActionableRevisionStatusUsesOptimisticLock(t *testing.T) { WithScheme(scheme). WithObjects(ephemeralRunnerSet). WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). WithInterceptorFuncs(interceptor.Funcs{ SubResourcePatch: func(ctx context.Context, clt client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { data, err := patch.Data(obj) @@ -125,6 +126,7 @@ func TestPatchAppliedActionableRevisionStatusDoesNotMoveBackwards(t *testing.T) WithScheme(scheme). WithObjects(ephemeralRunnerSet). WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). Build() reconciler := &EphemeralRunnerSetReconciler{ diff --git a/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go b/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go index 55c7b09d..84e7a70b 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go @@ -153,6 +153,10 @@ func TestPatchAppliedActionableRevisionStatusClearsFinishedRunnerCleanupPatchID( WithScheme(scheme). WithObjects(object). WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + // patchAppliedActionableRevisionStatus lists the child runners to + // recompute the phase, so the fake client needs the same index + // SetupIndexers registers on the manager. + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). Build() } diff --git a/controllers/actions.github.com/ephemeralrunnerset_stale_phase_test.go b/controllers/actions.github.com/ephemeralrunnerset_stale_phase_test.go new file mode 100644 index 00000000..a4f87055 --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_stale_phase_test.go @@ -0,0 +1,114 @@ +package actionsgithubcom + +import ( + "context" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// TestUpdateStatusStaleOutdatedRunnersNeverSetTheOutdatedPhase pins the +// invariant that makes the Outdated early return in Reconcile safe. +// +// The Outdated branch returns through cleanUpEphemeralRunners without +// recomputing the phase, so the stale-runner recovery block further down is +// unreachable while the set is already Outdated. That is only harmless because +// a stale runner cannot put the set into the Outdated phase in the first place: +// updateStatus switches to Outdated on len(outdated) alone, and a runner whose +// revision predates the applied one is classified staleOutdated instead. +// +// The case that matters for upgrades is a runner carrying no revision +// annotation at all. It parses to revision 0, so on a set that has applied any +// revision above 0 it is stale, not outdated, and cannot strand the set in a +// phase that stops it scaling. Stale runners are still collected -- terminated() +// includes them -- they simply are not evidence about the current runner spec. +// +// Recomputing the phase inside the Outdated branch instead, as review has twice +// proposed, would not fix anything here and would break the phase outright: +// cleanUpEphemeralRunners deletes terminated(), which includes outdated, so +// "no outdated runners remain" is trivially true immediately after it runs. The +// set would return to Running against an unchanged spec, scale a replacement +// runner from that same spec, and be told Outdated again on the next pass. +func TestUpdateStatusStaleOutdatedRunnersNeverSetTheOutdatedPhase(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + legacyRunner := func() v1alpha1.EphemeralRunner { + return v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{Name: "legacy-runner", Namespace: "default"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseOutdated}, + } + } + + tests := []struct { + name string + appliedRevision int64 + startingPhase v1alpha1.EphemeralRunnerSetPhase + wantPhase v1alpha1.EphemeralRunnerSetPhase + }{ + { + // The upgrade case from review: an annotation-less runner on a set + // that has already applied a revision must not stop it scaling. + name: "legacy runner on an upgraded set leaves the phase Running", + appliedRevision: 4, + startingPhase: v1alpha1.EphemeralRunnerSetPhaseRunning, + wantPhase: v1alpha1.EphemeralRunnerSetPhaseRunning, + }, + { + // Before any revision is applied the same runner is genuinely + // outdated, which is what keeps upgrades behaving as they do today. + name: "legacy runner before any revision is applied still reports Outdated", + appliedRevision: 0, + startingPhase: v1alpha1.EphemeralRunnerSetPhaseRunning, + wantPhase: v1alpha1.EphemeralRunnerSetPhaseOutdated, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ers", Namespace: "default"}, + Status: v1alpha1.EphemeralRunnerSetStatus{ + AppliedActionableRevision: tt.appliedRevision, + Phase: tt.startingPhase, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ephemeralRunnerSet). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + Build() + + reconciler := &EphemeralRunnerSetReconciler{ + Client: fakeClient, + Log: logr.Discard(), + Scheme: scheme, + } + + list := &v1alpha1.EphemeralRunnerList{Items: []v1alpha1.EphemeralRunner{legacyRunner()}} + state := newEphemeralRunnersByStates(list, tt.appliedRevision) + + require.NoError(t, reconciler.updateStatus(context.Background(), ephemeralRunnerSet, state, logr.Discard())) + + var patched v1alpha1.EphemeralRunnerSet + require.NoError(t, fakeClient.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "test-ers"}, &patched)) + + assert.Equal( + t, + tt.wantPhase, + patched.Status.Phase, + "a stale runner must not drive the set into a phase that stops it scaling, because the Outdated branch returns without recomputing the phase", + ) + }) + } +} diff --git a/controllers/actions.github.com/ephemeralrunnerset_stale_target_test.go b/controllers/actions.github.com/ephemeralrunnerset_stale_target_test.go new file mode 100644 index 00000000..c2746928 --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_stale_target_test.go @@ -0,0 +1,118 @@ +package actionsgithubcom + +import ( + "context" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// TestPatchAppliedActionableRevisionStatusIgnoresATargetBehindTheLiveRevision +// pins which revision the runners are classified against. +// +// Reconcile reads the EphemeralRunnerSet through the cache and passes +// Spec.ActionableRevision as the target, while this function re-reads the +// status through APIReader. The two can disagree: if another reconcile has +// already advanced the live marker, a lagging reconcile arrives with a target +// behind it. The monotonicity guard stops the marker regressing, but the phase +// is derived separately and has no such protection. +// +// Classifying against the stale target rates a runner from the superseded +// revision as current rather than staleOutdated, which flips a set that has +// already moved on back to Outdated. That is not self-correcting: Reconcile's +// Outdated branch returns before updateStatus, and this function only runs +// while spec > applied, so the set stays switched off until the next spec +// change. +// +// A single client is enough here, because the property under test is which +// integer reaches the classifier, not how the data was read. The revision gap +// is created in the stored object rather than by disagreeing views. +func TestPatchAppliedActionableRevisionStatusIgnoresATargetBehindTheLiveRevision(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + // The live set has already applied revision 5. + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ers", + Namespace: "default", + }, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 5, + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + AppliedActionableRevision: 5, + Phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + }, + } + + // A runner left over from revision 4 reporting Outdated. Against the live + // revision 5 it is staleOutdated and must not hold the set Outdated. + controllerRef := true + supersededRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner-from-revision-4", + Namespace: "default", + Annotations: map[string]string{ + AnnotationKeyActionableRevision: "4", + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: "test-ers", + UID: "test-uid", + Controller: &controllerRef, + }, + }, + }, + Status: v1alpha1.EphemeralRunnerStatus{ + Phase: v1alpha1.EphemeralRunnerPhaseOutdated, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ephemeralRunnerSet, supersededRunner). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). + Build() + + reconciler := &EphemeralRunnerSetReconciler{ + Client: fakeClient, + APIReader: fakeClient, + Log: logr.Discard(), + Scheme: scheme, + } + + key := types.NamespacedName{Namespace: "default", Name: "test-ers"} + + // The lagging reconcile still believes revision 4 is the one to apply. + require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 4)) + + var patched v1alpha1.EphemeralRunnerSet + require.NoError(t, fakeClient.Get(context.Background(), key, &patched)) + + assert.Equal( + t, + v1alpha1.EphemeralRunnerSetPhaseRunning, + patched.Status.Phase, + "a runner from a superseded revision must be judged against the live applied revision, not against a target that has fallen behind it: rating it as current flips the set to Outdated, and the Outdated phase is absorbing, so the set never scales again until the spec changes", + ) + + assert.Equal( + t, + int64(5), + patched.Status.AppliedActionableRevision, + "the monotonicity guard must keep the marker from regressing to the stale target", + ) +} diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go index 7b29446e..d0830111 100644 --- a/controllers/actions.github.com/helpers.go +++ b/controllers/actions.github.com/helpers.go @@ -39,6 +39,30 @@ func nextActionableRevision(current *v1alpha1.EphemeralRunnerSet) int64 { return current.Status.AppliedActionableRevision + 1 } +// ephemeralRunnerSetOutdatedForAppliedRevision reports whether the set is +// Outdated *because of the runner spec it is currently running*, which is the +// only situation in which the AutoscalingRunnerSet should tear the scale set +// down. +// +// The phase alone is not enough. Outdated is deliberately sticky: it survives in +// status while the outdated runners are collected, and it is only cleared once a +// new revision is applied. So between the moment the AutoscalingRunnerSet patches +// a new runner spec onto the set and the moment the EphemeralRunnerSet controller +// processes that patch, the set still reports Outdated for a spec that no longer +// exists. Tearing down there would discard the fix the user just applied, and the +// scale set would stay switched off until something else nudged it. +// +// Requiring the applied revision to have caught up with the spec revision closes +// that window: the verdict counts only once the set is running the current spec. +func ephemeralRunnerSetOutdatedForAppliedRevision(ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet) bool { + if ephemeralRunnerSet == nil { + return false + } + + return ephemeralRunnerSet.Status.Phase == v1alpha1.EphemeralRunnerSetPhaseOutdated && + ephemeralRunnerSet.Status.AppliedActionableRevision >= ephemeralRunnerSet.Spec.ActionableRevision +} + // listenerPodSpecRequiresRecreation reports whether the live listener pod must be // deleted and rebuilt to match the desired spec. // diff --git a/controllers/actions.github.com/helpers_outdated_test.go b/controllers/actions.github.com/helpers_outdated_test.go new file mode 100644 index 00000000..72ea5b7d --- /dev/null +++ b/controllers/actions.github.com/helpers_outdated_test.go @@ -0,0 +1,251 @@ +package actionsgithubcom + +import ( + "strconv" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func outdatedRunnerAtRevision(name string, revision int64) v1alpha1.EphemeralRunner { + return v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Annotations: map[string]string{ + AnnotationKeyActionableRevision: strconv.FormatInt(revision, 10), + }, + }, + Status: v1alpha1.EphemeralRunnerStatus{ + Phase: v1alpha1.EphemeralRunnerPhaseOutdated, + }, + } +} + +// TestNewEphemeralRunnersByStates_OutdatedIsRevisionScoped covers the core of the +// outdated-recovery behaviour: a runner that reported Outdated against a runner +// spec that has since been replaced must not be treated as evidence about the +// current spec. +func TestNewEphemeralRunnersByStates_OutdatedIsRevisionScoped(t *testing.T) { + tests := []struct { + name string + runners []v1alpha1.EphemeralRunner + appliedRevision int64 + wantOutdatedNames []string + wantStaleOutdatedName []string + }{ + { + name: "runner at the applied revision is genuinely outdated", + runners: []v1alpha1.EphemeralRunner{outdatedRunnerAtRevision("current", 3)}, + appliedRevision: 3, + wantOutdatedNames: []string{"current"}, + }, + { + name: "runner from before the last spec update is stale", + runners: []v1alpha1.EphemeralRunner{outdatedRunnerAtRevision("old", 2)}, + appliedRevision: 3, + wantStaleOutdatedName: []string{"old"}, + }, + { + name: "mixed revisions are split", + runners: []v1alpha1.EphemeralRunner{ + outdatedRunnerAtRevision("old", 1), + outdatedRunnerAtRevision("current", 4), + }, + appliedRevision: 4, + wantOutdatedNames: []string{"current"}, + wantStaleOutdatedName: []string{"old"}, + }, + { + name: "runner without the annotation is treated as revision 0", + runners: []v1alpha1.EphemeralRunner{{ + ObjectMeta: metav1.ObjectMeta{Name: "legacy"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseOutdated}, + }}, + appliedRevision: 0, + wantOutdatedNames: []string{"legacy"}, + }, + { + name: "legacy runner becomes stale once a revision is applied", + runners: []v1alpha1.EphemeralRunner{{ + ObjectMeta: metav1.ObjectMeta{Name: "legacy"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseOutdated}, + }}, + appliedRevision: 1, + wantStaleOutdatedName: []string{"legacy"}, + }, + { + name: "a runner being deleted is never classified as outdated", + runners: []v1alpha1.EphemeralRunner{func() v1alpha1.EphemeralRunner { + runner := outdatedRunnerAtRevision("terminating", 3) + now := metav1.Now() + runner.DeletionTimestamp = &now + runner.Finalizers = []string{ephemeralRunnerFinalizerName} + return runner + }()}, + appliedRevision: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + list := &v1alpha1.EphemeralRunnerList{Items: tt.runners} + state := newEphemeralRunnersByStates(list, tt.appliedRevision) + + assert.Equal(t, tt.wantOutdatedNames, runnerNames(state.outdated)) + assert.Equal(t, tt.wantStaleOutdatedName, runnerNames(state.staleOutdated)) + }) + } +} + +// TestEphemeralRunnersByState_TerminatedIncludesStaleOutdated ensures the cleanup +// paths still collect stale outdated runners; they are excluded from the phase +// decision, not from garbage collection. +func TestEphemeralRunnersByState_TerminatedIncludesStaleOutdated(t *testing.T) { + list := &v1alpha1.EphemeralRunnerList{Items: []v1alpha1.EphemeralRunner{ + outdatedRunnerAtRevision("stale", 1), + outdatedRunnerAtRevision("current", 5), + { + ObjectMeta: metav1.ObjectMeta{Name: "succeeded"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseSucceeded}, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "failed"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseFailed}, + }, + }} + + state := newEphemeralRunnersByStates(list, 5) + + assert.ElementsMatch(t, + []string{"succeeded", "failed", "current", "stale"}, + runnerNames(state.terminated()), + ) +} + +// TestEphemeralRunnersByState_TerminatedDoesNotAliasBackingArrays guards against +// terminated() corrupting the slices it concatenates, which would silently +// reclassify runners. +// +// A concatenation written as append(s.finished, others...) does not copy when +// s.finished has spare capacity: it writes the other runners into that spare +// room and hands back a slice sharing the caller's backing array. The corruption +// only becomes visible once something appends to s.finished again, which reuses +// the same slots and overwrites entries in the already-returned slice. So the +// test needs a state whose finished slice has room to spare, a retained result, +// and a subsequent append. +func TestEphemeralRunnersByState_TerminatedDoesNotAliasBackingArrays(t *testing.T) { + finished := func(name string) v1alpha1.EphemeralRunner { + return v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseSucceeded}, + } + } + + // Three finished runners, because the classifier builds the slice with + // append and the backing array grows 1, 2, 4: the result is length 3 with + // room for a fourth. + list := &v1alpha1.EphemeralRunnerList{Items: []v1alpha1.EphemeralRunner{ + finished("succeeded-a"), + finished("succeeded-b"), + finished("succeeded-c"), + { + ObjectMeta: metav1.ObjectMeta{Name: "failed"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseFailed}, + }, + }} + + state := newEphemeralRunnersByStates(list, 0) + + // Asserted rather than assumed: if the classifier ever stops leaving spare + // capacity, the concatenation is forced to allocate, no aliasing is possible + // and the rest of this test would quietly stop proving anything. + require.Greater(t, cap(state.finished), len(state.finished), + "precondition: finished needs spare capacity for aliasing to be reproducible") + + terminated := state.terminated() + require.Contains(t, runnerNames(terminated), "failed") + + // Reuses the spare slot that an aliasing concatenation would have written + // the failed runner into. + state.finished = append(state.finished, &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{Name: "succeeded-d"}, + }) + + assert.Contains(t, runnerNames(terminated), "failed", + "terminated() must own its backing array: appending to state.finished overwrote a runner in the slice already returned to the caller") + assert.Equal(t, []string{"succeeded-a", "succeeded-b", "succeeded-c"}, runnerNames(state.finished[:3])) + assert.Equal(t, []string{"failed"}, runnerNames(state.failed)) +} + +// TestEphemeralRunnerSetOutdatedForAppliedRevision covers the guard that stops the +// AutoscalingRunnerSet from tearing the scale set down on an Outdated verdict that +// predates the runner spec it has just published. +func TestEphemeralRunnerSetOutdatedForAppliedRevision(t *testing.T) { + tests := []struct { + name string + phase v1alpha1.EphemeralRunnerSetPhase + specRevision int64 + appliedRevision int64 + want bool + }{ + { + name: "nil-safe: running set is not outdated", + phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + want: false, + }, + { + name: "outdated against the spec it is running", + phase: v1alpha1.EphemeralRunnerSetPhaseOutdated, + specRevision: 4, + appliedRevision: 4, + want: true, + }, + { + name: "outdated verdict predates a spec update that is still propagating", + phase: v1alpha1.EphemeralRunnerSetPhaseOutdated, + specRevision: 5, + appliedRevision: 4, + want: false, + }, + { + name: "legacy set with no revisions recorded still tears down", + phase: v1alpha1.EphemeralRunnerSetPhaseOutdated, + specRevision: 0, + appliedRevision: 0, + want: true, + }, + { + name: "running set with a pending revision is not outdated", + phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + specRevision: 5, + appliedRevision: 4, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + Spec: v1alpha1.EphemeralRunnerSetSpec{ActionableRevision: tt.specRevision}, + Status: v1alpha1.EphemeralRunnerSetStatus{Phase: tt.phase, AppliedActionableRevision: tt.appliedRevision}, + } + assert.Equal(t, tt.want, ephemeralRunnerSetOutdatedForAppliedRevision(ephemeralRunnerSet)) + }) + } + + assert.False(t, ephemeralRunnerSetOutdatedForAppliedRevision(nil)) +} + +func runnerNames(runners []*v1alpha1.EphemeralRunner) []string { + if len(runners) == 0 { + return nil + } + names := make([]string, 0, len(runners)) + for _, runner := range runners { + names = append(names, runner.Name) + } + return names +} diff --git a/controllers/actions.github.com/indexer.go b/controllers/actions.github.com/indexer.go index 466c9f14..0c47f409 100644 --- a/controllers/actions.github.com/indexer.go +++ b/controllers/actions.github.com/indexer.go @@ -69,3 +69,17 @@ func newGroupVersionOwnerKindIndexer(ownerKind string, otherOwnerKinds ...string return []string{owner.Name} } } + +// isControlledBy applies the same ownership test as the resourceOwnerKey index, +// for callers that cannot use that index. It is registered on the manager's +// cache, so a read that deliberately bypasses the cache has to filter here +// instead: the API server rejects .metadata.controller as an unsupported field +// label. Keeping the predicate alongside the indexer is what stops the two +// drifting apart. +func isControlledBy(o client.Object, ownerKind, ownerName string) bool { + owner := metav1.GetControllerOfNoCopy(o) + return owner != nil && + owner.APIVersion == v1alpha1.GroupVersion.String() && + owner.Kind == ownerKind && + owner.Name == ownerName +} diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index 7cbcd9aa..ea092aab 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -827,9 +827,10 @@ func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.Epheme maps.Copy(labels, ephemeralRunnerSet.Labels) labels[LabelKeyKubernetesComponent] = "runner" - annotations := make(map[string]string, len(ephemeralRunnerSet.Annotations)+1) + annotations := make(map[string]string, len(ephemeralRunnerSet.Annotations)+2) maps.Copy(annotations, ephemeralRunnerSet.Annotations) annotations[AnnotationKeyPatchID] = strconv.Itoa(ephemeralRunnerSet.Spec.PatchID) + annotations[AnnotationKeyActionableRevision] = strconv.FormatInt(ephemeralRunnerSet.Spec.ActionableRevision, 10) if ephemeralRunnerSet.Spec.EphemeralRunnerMetadata != nil { labels = b.filterAndMergeLabels(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Labels, labels) diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index 9e91790d..b472143b 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -546,3 +546,52 @@ func TestListenerPodNodeSelector(t *testing.T) { "explicitly empty nodeSelector should override the linux default") }) } + +// TestNewEphemeralRunnerStampsActionableRevision pins the annotation the +// Outdated lifecycle is built on. The controller compares a runner's actionable +// revision against the set's applied revision to decide whether an Outdated +// report concerns the current runner spec or one that has since been replaced. +// A runner that lost this annotation would parse as revision 0 and be treated as +// stale, so it would be deleted and replaced instead of holding the set +// Outdated, and the set would never stop scaling. +func TestNewEphemeralRunnerStampsActionableRevision(t *testing.T) { + newSet := func(revision int64, metadata *v1alpha1.ResourceMeta) *v1alpha1.EphemeralRunnerSet { + return &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ers", Namespace: "test-ns"}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: revision, + EphemeralRunnerMetadata: metadata, + }, + } + } + + var b ResourceBuilder + + t.Run("stamps the set's actionable revision", func(t *testing.T) { + runner, err := b.newEphemeralRunner(newSet(7, nil)) + require.NoError(t, err) + assert.Equal(t, "7", runner.Annotations[AnnotationKeyActionableRevision]) + }) + + // The zero value is what an unupgraded set carries, and it has to round-trip + // as "0" rather than being omitted: the classifier parses a missing + // annotation as 0 too, so an absent stamp would be indistinguishable from a + // genuine revision 0 and upgrades would silently rely on that coincidence. + t.Run("stamps the zero revision explicitly", func(t *testing.T) { + runner, err := b.newEphemeralRunner(newSet(0, nil)) + require.NoError(t, err) + assert.Equal(t, "0", runner.Annotations[AnnotationKeyActionableRevision]) + }) + + // User-supplied runner annotations are merged underneath the controller's + // own, so they cannot forge a revision. If this inverted, a user annotation + // could make every runner look stale and the set would delete and recreate + // runners forever. + t.Run("user metadata cannot override it", func(t *testing.T) { + runner, err := b.newEphemeralRunner(newSet(7, &v1alpha1.ResourceMeta{ + Annotations: map[string]string{AnnotationKeyActionableRevision: "1"}, + })) + require.NoError(t, err) + assert.Equal(t, "7", runner.Annotations[AnnotationKeyActionableRevision]) + }) +}