From b9f0f74bab4db867420a1bcad56e79bafa893bfb Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 9 Sep 2026 11:57:05 +0200 Subject: [PATCH 1/3] Track AutoscalingRunnerSet updates with metadata.generation The AutoscalingRunnerSet controller detected changes by hashing the spec and the labels on every reconcile and comparing the result against the actions.github.com/integrity-hash annotation it had written on a previous pass. That is a hand-rolled version of something the API server already does for us: it bumps metadata.generation on every spec write, and nothing else. Recording that value in the status as ObservedGeneration gives the same "has this changed since I last applied it" signal without recomputing a hash, without an extra non-status write to the object, and with a value a user can read and reason about. updateStatus now takes the observed generation and patches it next to the phase, returning early only when both are already what we want. When the generation is ahead of the observed generation we move to Pending but deliberately leave the observed generation where it is; it only catches up at the end of a reconcile that actually applied the change, so a reconcile that fails part way through is retried as pending rather than being mistaken for settled. The old code returned immediately after stamping the hash. That was needed because stamping the annotation was itself a write to the object, so continuing would have worked from a stale copy. Recording the generation only touches the status subresource, which does not bump generation, so the rest of the reconcile can run on the same object and apply the change in the same pass instead of waiting for the next event. One user-visible difference: the old hash covered Labels as well as Spec, and metadata.generation does not move when only labels change. Editing just a label on an AutoscalingRunnerSet no longer forces it through Pending. Labels still propagate to the EphemeralRunnerSet; they simply no longer count as an update that has to be applied. Hash, ListenerSpecHash and RunnerSetSpecHash are removed. The latter two already had no callers, and the first has none now. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../v1alpha1/autoscalingrunnerset_types.go | 47 +---------- ...ions.github.com_autoscalingrunnersets.yaml | 6 ++ ...ions.github.com_autoscalingrunnersets.yaml | 6 ++ ...ions.github.com_autoscalingrunnersets.yaml | 6 ++ .../autoscalingrunnerset_controller.go | 44 +++++----- .../autoscalingrunnerset_controller_test.go | 84 +++++++++++++++++-- 6 files changed, 125 insertions(+), 68 deletions(-) diff --git a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go index afcb4c93ed..018f71c914 100644 --- a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go @@ -23,7 +23,6 @@ import ( "net/url" "strings" - "github.com/actions/actions-runner-controller/hash" "github.com/actions/actions-runner-controller/vault" "golang.org/x/net/http/httpproxy" corev1 "k8s.io/api/core/v1" @@ -322,6 +321,10 @@ type HistogramMetric struct { type AutoscalingRunnerSetStatus struct { // +optional Phase AutoscalingRunnerSetPhase `json:"phase"` + // ObservedGeneration tracks the metadata.generation of this ARS at observation time, + // enabling detection of Pending phase when generation differs. Unset defaults to 0. + // +optional + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } type AutoscalingRunnerSetPhase string @@ -334,26 +337,6 @@ const ( AutoscalingRunnerSetPhaseOutdated AutoscalingRunnerSetPhase = "Outdated" ) -func (ars *AutoscalingRunnerSet) Hash() string { - type data struct { - Spec *AutoscalingRunnerSetSpec - Labels map[string]string - } - - d := &data{ - Spec: ars.Spec.DeepCopy(), - Labels: ars.Labels, - } - - return hash.ComputeTemplateHash(d) -} - -func (ars *AutoscalingRunnerSet) ListenerSpecHash() string { - arsSpec := ars.Spec.DeepCopy() - spec := arsSpec - return hash.ComputeTemplateHash(&spec) -} - func (ars *AutoscalingRunnerSet) GitHubConfigSecret() string { return ars.Spec.GitHubConfigSecret } @@ -381,28 +364,6 @@ func (ars *AutoscalingRunnerSet) VaultProxy() *ProxyConfig { return nil } -func (ars *AutoscalingRunnerSet) RunnerSetSpecHash() string { - type runnerSetSpec struct { - GitHubConfigUrl string - GitHubConfigSecret string - RunnerGroup string - RunnerScaleSetName string - Proxy *ProxyConfig - GitHubServerTLS *TLSConfig - Template corev1.PodTemplateSpec - } - spec := &runnerSetSpec{ - GitHubConfigUrl: ars.Spec.GitHubConfigUrl, - GitHubConfigSecret: ars.Spec.GitHubConfigSecret, - RunnerGroup: ars.Spec.RunnerGroup, - RunnerScaleSetName: ars.Spec.RunnerScaleSetName, - Proxy: ars.Spec.Proxy, - GitHubServerTLS: ars.Spec.GitHubServerTLS, - Template: ars.Spec.Template, - } - return hash.ComputeTemplateHash(&spec) -} - // +kubebuilder:object:root=true // AutoscalingRunnerSetList contains a list of AutoscalingRunnerSet diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml index 8b78ce2430..386284273f 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml @@ -16555,6 +16555,12 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + observedGeneration: + description: |- + ObservedGeneration tracks the metadata.generation of this ARS at observation time, + enabling detection of Pending phase when generation differs. Unset defaults to 0. + format: int64 + type: integer phase: type: string type: object diff --git a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml index 8b78ce2430..386284273f 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml @@ -16555,6 +16555,12 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + observedGeneration: + description: |- + ObservedGeneration tracks the metadata.generation of this ARS at observation time, + enabling detection of Pending phase when generation differs. Unset defaults to 0. + format: int64 + type: integer phase: type: string type: object diff --git a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml index 8b78ce2430..386284273f 100644 --- a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml +++ b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml @@ -16555,6 +16555,12 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + observedGeneration: + description: |- + ObservedGeneration tracks the metadata.generation of this ARS at observation time, + enabling detection of Pending phase when generation differs. Unset defaults to 0. + format: int64 + type: integer phase: type: string type: object diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index ab44125f65..8d28b5a293 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -143,27 +143,22 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - // Something has changed, we need to re-apply the pending phase and change hash annotation to trigger the update of runner scale set and listener. - if targetHash := autoscalingRunnerSet.Hash(); autoscalingRunnerSet.Annotations[annotationKeyIntegrityHash] != targetHash { - // TODO: apply the version label - original := autoscalingRunnerSet.DeepCopy() - if autoscalingRunnerSet.Annotations == nil { - autoscalingRunnerSet.Annotations = map[string]string{} - } - autoscalingRunnerSet.Annotations[annotationKeyIntegrityHash] = targetHash - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update autoscaling runner set with new change hash and pending phase") - return ctrl.Result{}, err - } - - original = autoscalingRunnerSet.DeepCopy() - autoscalingRunnerSet.Status.Phase = v1alpha1.AutoscalingRunnerSetPhasePending - if err := r.Status().Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { + // The spec changed since we last observed it, so move back to the pending + // phase. The observed generation is deliberately left at its old value here: + // it only catches up at the end of a successful reconcile, so a reconcile + // that fails half way through is retried as pending rather than being + // mistaken for settled. + if autoscalingRunnerSet.Generation > autoscalingRunnerSet.Status.ObservedGeneration { + if err := r.updateStatus( + ctx, + &autoscalingRunnerSet, + v1alpha1.AutoscalingRunnerSetPhasePending, + autoscalingRunnerSet.Status.ObservedGeneration, + log, + ); err != nil { log.Error(err, "Failed to update autoscaling runner set status with pending phase") return ctrl.Result{}, err } - - return ctrl.Result{}, nil } outdated := autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseOutdated @@ -393,6 +388,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl ctx, &autoscalingRunnerSet, v1alpha1.AutoscalingRunnerSetPhaseRunning, + autoscalingRunnerSet.Generation, log, ); err != nil { log.Error(err, "Failed to update autoscaling runner set status to running") @@ -437,14 +433,22 @@ func (r *AutoscalingRunnerSetReconciler) cleanUpResources(ctx context.Context, a } // Update the status of autoscaling runner set if necessary -func (r *AutoscalingRunnerSetReconciler) updateStatus(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, phase v1alpha1.AutoscalingRunnerSetPhase, log logr.Logger) error { +func (r *AutoscalingRunnerSetReconciler) updateStatus( + ctx context.Context, + autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, + phase v1alpha1.AutoscalingRunnerSetPhase, + observedGeneration int64, + log logr.Logger, +) error { phaseDiff := phase != autoscalingRunnerSet.Status.Phase - if !phaseDiff { + observedGenerationDiff := observedGeneration != autoscalingRunnerSet.Status.ObservedGeneration + if !phaseDiff && !observedGenerationDiff { return nil } original := autoscalingRunnerSet.DeepCopy() autoscalingRunnerSet.Status.Phase = phase + autoscalingRunnerSet.Status.ObservedGeneration = observedGeneration if err := r.Status().Patch(ctx, autoscalingRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to patch autoscaling runner set status") diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index e122cb737e..bae467de44 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -492,6 +492,81 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { }) Context("When updating a new AutoScalingRunnerSet", func() { + It("advances the observed generation once a spec change has been applied", func() { + 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.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)) + g.Expect(current.Status.ObservedGeneration).To(Equal(current.Generation)) + settledGeneration = current.Generation + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "AutoscalingRunnerSet should settle with its generation observed") + + patched := autoscalingRunnerSet.DeepCopy() + patched.Spec.Template.Spec.Containers[0].Image = "ghcr.io/actions/runner:updated" + Expect(k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet))).To(Succeed(), "failed to patch AutoScalingRunnerSet") + + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) + g.Expect(current.Generation).To(BeNumerically(">", settledGeneration), "a spec write must bump metadata.generation") + g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)) + g.Expect(current.Status.ObservedGeneration).To(Equal(current.Generation), "observed generation must catch up once the change is applied") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + }) + + // metadata.generation only tracks spec writes, so a label-only edit no + // longer drags the scale set through the Pending phase the way the old + // label-inclusive hash did. Labels still propagate to the + // EphemeralRunnerSet, they just do not count as an update to apply. + It("does not re-observe a generation when only labels change", func() { + 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)) + settledGeneration = current.Generation + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "AutoscalingRunnerSet should settle with its generation observed") + + patched := autoscalingRunnerSet.DeepCopy() + patched.Labels["arc.test/label-drift"] = "updated" + Expect(k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet))).To(Succeed(), "failed to patch AutoScalingRunnerSet labels") + + 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.Labels).To(HaveKeyWithValue("arc.test/label-drift", "updated"), "labels should still propagate to the EphemeralRunnerSet") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + Consistently( + func(g Gomega) { + current := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) + g.Expect(current.Generation).To(Equal(settledGeneration), "a label-only edit must not bump metadata.generation") + g.Expect(current.Status.ObservedGeneration).To(Equal(settledGeneration)) + g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning), "a label-only edit must not push the scale set back to Pending") + }, + 3*time.Second, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + }) + It("updates EphemeralRunnerSet when the runner image changes without touching the Listener", func() { listener := new(v1alpha1.AutoscalingListener) Eventually( @@ -979,10 +1054,6 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { statusUpdate := runnerSet.DeepCopy() statusUpdate.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning - desiredStatus := v1alpha1.AutoscalingRunnerSetStatus{ - Phase: v1alpha1.AutoscalingRunnerSetPhaseRunning, - } - err := k8sClient.Status().Patch(ctx, statusUpdate, client.MergeFrom(&runnerSet)) Expect(err).NotTo(HaveOccurred(), "Failed to patch runner set status") @@ -997,7 +1068,10 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, - ).Should(BeEquivalentTo(desiredStatus), "AutoScalingRunnerSet status should be updated") + ).Should(SatisfyAll( + WithTransform(func(s v1alpha1.AutoscalingRunnerSetStatus) v1alpha1.AutoscalingRunnerSetPhase { return s.Phase }, Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)), + WithTransform(func(s v1alpha1.AutoscalingRunnerSetStatus) int64 { return s.ObservedGeneration }, BeNumerically(">=", ars.Generation)), + ), "AutoScalingRunnerSet status should be updated") }) }) From f2e44107511df5d5ce6745d7ea1cb8dc3d09db97 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Thu, 10 Sep 2026 10:33:24 +0200 Subject: [PATCH 2/3] Report the pending phase when the listener is rebuilt Moving update detection to metadata.generation lost a signal. The desired listener is built from the AutoscalingRunnerSet's labels and annotations as well as its spec, and any difference there deletes the listener so it can be re-created. metadata.generation only moves on spec writes, so a label-only edit still tore the listener down while the scale set kept reporting Running. If the rebuild then failed, it reported Running with no listener indefinitely. Under the old label-inclusive hash the phase did move, so this was a regression. Mark the resource pending at the point the listener is deleted rather than trying to infer it from the generation. That is what the phase already means: its own doc comment says pending is when the listener is not yet started. It also covers the case properly, because it keys off the actual decision to rebuild instead of guessing from the trigger. This does not change when the listener is rebuilt. A label-only edit recreates it today and still does; only the reported phase changes. The earlier claim that labels no longer restart anything was wrong: labels are propagated to the listener, so editing one is a restart. What metadata.generation changes is narrower than that, and the test now covers the listener's identity across a label edit rather than implying it is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalingrunnerset_controller.go | 19 ++++++++ .../autoscalingrunnerset_controller_test.go | 46 +++++++++++++++++-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 8d28b5a293..d4404243f5 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -373,6 +373,25 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if !cmp.Equal(listener.Spec, desired.Spec) || !cmp.Equal(listener.Labels, desired.Labels) || !cmp.Equal(listener.Annotations, desired.Annotations) { + // The listener is about to be torn down and rebuilt, which is what + // the pending phase means. Report it here rather than relying on the + // generation check above: the desired listener is derived from the + // AutoscalingRunnerSet's labels and annotations as well as its spec, + // and metadata writes do not bump metadata.generation. Without this, + // a label-only edit would leave the scale set claiming to be running + // while it has no listener at all, and it would keep claiming that + // if the rebuild never succeeded. + if err := r.updateStatus( + ctx, + &autoscalingRunnerSet, + v1alpha1.AutoscalingRunnerSetPhasePending, + autoscalingRunnerSet.Status.ObservedGeneration, + log, + ); err != nil { + log.Error(err, "Failed to update autoscaling runner set status before re-creating the listener") + return ctrl.Result{}, err + } + log.Info("Deleting AutoscalingListener to re-create with updated spec") if err := r.Delete(ctx, &listener); err != nil { log.Error(err, "Failed to delete AutoscalingListener for re-creation") diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index bae467de44..eea65b8ea0 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -527,12 +527,27 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { // longer drags the scale set through the Pending phase the way the old // label-inclusive hash did. Labels still propagate to the // EphemeralRunnerSet, they just do not count as an update to apply. - It("does not re-observe a generation when only labels change", func() { + // A label-only edit does not bump metadata.generation, so it is not an + // update to apply. It does still rebuild the listener, because the + // desired listener labels are derived from the AutoscalingRunnerSet's, + // and the phase has to report that. + It("does not re-observe a generation when only labels change, but still reports the listener rebuild", func() { + listener := new(v1alpha1.AutoscalingListener) + Eventually( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "Listener should be created") + originalListenerUID := listener.UID + 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.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)) g.Expect(current.Status.ObservedGeneration).To(Equal(current.Generation)) settledGeneration = current.Generation }, @@ -554,13 +569,38 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed()) + // The listener is rebuilt to pick the new label up, so the scale set + // must not keep claiming to be running while that happens. + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingListener) + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current)).To(Succeed()) + g.Expect(current.UID).NotTo(Equal(originalListenerUID), "listener should be re-created to pick up the new label") + g.Expect(current.Labels).To(HaveKeyWithValue("arc.test/label-drift", "updated"), "listener should carry the new label") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + // Once the listener is back, the scale set settles on Running again + // without the observed generation ever moving, because no spec write + // happened. + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) + g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "AutoscalingRunnerSet should return to Running once the listener is rebuilt") + Consistently( func(g Gomega) { current := new(v1alpha1.AutoscalingRunnerSet) g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) g.Expect(current.Generation).To(Equal(settledGeneration), "a label-only edit must not bump metadata.generation") - g.Expect(current.Status.ObservedGeneration).To(Equal(settledGeneration)) - g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning), "a label-only edit must not push the scale set back to Pending") + g.Expect(current.Status.ObservedGeneration).To(Equal(settledGeneration), "a label-only edit must not move the observed generation") }, 3*time.Second, autoscalingRunnerSetTestInterval, From fbbeeeca439c350a5cea851d69aa74645feaf10a Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Thu, 10 Sep 2026 12:24:19 +0200 Subject: [PATCH 3/3] Make the generation tests observe the states they claim to cover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The specs added with the observed-generation work asserted on end states only. Both the listener rebuild and a stalled reconcile pass through a transient phase, and nothing looked at it: the tests waited for the rebuilt listener and then checked for Running. Removing the pending update that guards the rebuild left them green, which was pointed out in review and is true — I checked by deleting that update and re-running, and they passed. The window was invisible because it closes on its own between two polls. Hold it open instead: the AutoscalingListener controller does not run in this suite, so a finalizer added by the test keeps the deleted listener around until the test removes it. That turns a race into a state the test can sit on and assert against. The label spec now waits for the listener to carry a deletion timestamp while the scale set reports Pending, holds that to show it is not a blip, then releases the finalizer and checks the listener comes back with a new UID and the scale set returns to Running. With the pending update removed it now fails on the phase, which is the point. The same trick gives the failure path its first coverage. A max runners change bumps the generation and forces a listener rebuild, so blocking the rebuild stalls the reconcile part way through. The new spec pins what the contract claims: the observed generation stays behind the live generation and the phase stays Pending for as long as the change cannot be applied, and the marker only catches up once it can. Also drops a stale comment that survived a merge and contradicted the code next to it, still claiming label edits no longer reach the Pending phase. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalingrunnerset_controller_test.go | 171 +++++++++++++++++- 1 file changed, 161 insertions(+), 10 deletions(-) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index eea65b8ea0..10cf20eed7 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -523,14 +523,12 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { ).Should(Succeed()) }) - // metadata.generation only tracks spec writes, so a label-only edit no - // longer drags the scale set through the Pending phase the way the old - // label-inclusive hash did. Labels still propagate to the - // EphemeralRunnerSet, they just do not count as an update to apply. - // A label-only edit does not bump metadata.generation, so it is not an - // update to apply. It does still rebuild the listener, because the - // desired listener labels are derived from the AutoscalingRunnerSet's, - // and the phase has to report that. + // A label-only edit does not bump metadata.generation, so it never + // advances the observed generation. It does still rebuild the listener, + // because the desired listener labels are derived from the + // AutoscalingRunnerSet's, and the phase has to report that rebuild. So + // the scale set passes through Pending transiently and comes back to + // Running, while the observed generation stays exactly where it was. It("does not re-observe a generation when only labels change, but still reports the listener rebuild", func() { listener := new(v1alpha1.AutoscalingListener) Eventually( @@ -555,6 +553,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed(), "AutoscalingRunnerSet should settle with its generation observed") + // Hold the deletion window open. Without this the listener is deleted + // and re-created between two polls, so an implementation that never + // reported Pending would look identical to one that did. The + // AutoscalingListener controller does not run in this suite, so + // nothing else is going to clear this finalizer. + blockDeletion(listener) + defer unblockDeletion(listener) + patched := autoscalingRunnerSet.DeepCopy() patched.Labels["arc.test/label-drift"] = "updated" Expect(k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet))).To(Succeed(), "failed to patch AutoScalingRunnerSet labels") @@ -569,8 +575,39 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed()) - // The listener is rebuilt to pick the new label up, so the scale set - // must not keep claiming to be running while that happens. + // The listener is on its way out, so the scale set must not claim to + // be running. This is the assertion that fails if the Pending update + // before the listener delete is removed. + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingListener) + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current)).To(Succeed()) + g.Expect(current.DeletionTimestamp).NotTo(BeNil(), "listener should be marked for deletion to pick up the new label") + + ars := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), ars)).To(Succeed()) + g.Expect(ars.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhasePending), "scale set must report Pending while its listener is being rebuilt") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + // It stays Pending for as long as the rebuild has not happened, and + // the observed generation does not drift in the meantime. + Consistently( + func(g Gomega) { + ars := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), ars)).To(Succeed()) + g.Expect(ars.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhasePending), "scale set must stay Pending until the listener is back") + g.Expect(ars.Status.ObservedGeneration).To(Equal(settledGeneration)) + }, + 3*time.Second, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + // Let the rebuild finish. + unblockDeletion(listener) + Eventually( func(g Gomega) { current := new(v1alpha1.AutoscalingListener) @@ -607,6 +644,84 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { ).Should(Succeed()) }) + // The observed generation is only meant to advance once a change has + // actually been applied. A reconcile that cannot finish must leave it + // behind the live generation, so the work is retried rather than being + // mistaken for settled. + It("leaves the observed generation behind until the reconcile completes", func() { + listener := new(v1alpha1.AutoscalingListener) + Eventually( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "Listener should be created") + + 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.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)) + g.Expect(current.Status.ObservedGeneration).To(Equal(current.Generation)) + settledGeneration = current.Generation + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "AutoscalingRunnerSet should settle with its generation observed") + + // Stall the reconcile part way through: the max runner change below + // forces the listener to be rebuilt, and the rebuild cannot complete + // while the listener is held in deletion. + blockDeletion(listener) + defer unblockDeletion(listener) + + patched := autoscalingRunnerSet.DeepCopy() + updatedMax := 20 + patched.Spec.MaxRunners = &updatedMax + Expect(k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet))).To(Succeed(), "failed to patch AutoScalingRunnerSet max runners") + + var liveGeneration int64 + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) + g.Expect(current.Generation).To(BeNumerically(">", settledGeneration), "a spec write must bump metadata.generation") + g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhasePending), "the scale set should go Pending while the change is being applied") + liveGeneration = current.Generation + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + Consistently( + 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(settledGeneration), "observed generation must not advance while the change is still being applied") + g.Expect(current.Status.ObservedGeneration).To(BeNumerically("<", liveGeneration)) + g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhasePending), "the scale set must stay Pending until the change is applied") + }, + 3*time.Second, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + // Let the reconcile complete, and only now should the marker catch up. + unblockDeletion(listener) + + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) + g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)) + g.Expect(current.Status.ObservedGeneration).To(Equal(liveGeneration), "observed generation must catch up once the change has been applied") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + }) + It("updates EphemeralRunnerSet when the runner image changes without touching the Listener", func() { listener := new(v1alpha1.AutoscalingListener) Eventually( @@ -2561,3 +2676,39 @@ var _ = Describe("Test AutoscalingRunnerSet with a stale runner scale set", Orde }) }) }) + +// testHoldFinalizer keeps an AutoscalingListener around after it has been +// deleted, so a test can observe the window during which the scale set has no +// usable listener. The AutoscalingListener controller is not running in this +// suite, so nothing else adds or removes finalizers on these objects. +const testHoldFinalizer = "arc.test/hold-deletion" + +func blockDeletion(listener *v1alpha1.AutoscalingListener) { + GinkgoHelper() + + current := new(v1alpha1.AutoscalingListener) + Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(listener), current)).To(Succeed(), "failed to get listener to block its deletion") + + original := current.DeepCopy() + Expect(controllerutil.AddFinalizer(current, testHoldFinalizer)).To(BeTrue(), "listener should not already hold the test finalizer") + Expect(k8sClient.Patch(context.Background(), current, client.MergeFrom(original))).To(Succeed(), "failed to add the test finalizer to the listener") +} + +// unblockDeletion is idempotent so it can be deferred as a safety net and still +// be called explicitly at the point a test wants the rebuild to proceed. +func unblockDeletion(listener *v1alpha1.AutoscalingListener) { + GinkgoHelper() + + current := new(v1alpha1.AutoscalingListener) + err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(listener), current) + if errors.IsNotFound(err) { + return + } + Expect(err).NotTo(HaveOccurred(), "failed to get listener to unblock its deletion") + + original := current.DeepCopy() + if !controllerutil.RemoveFinalizer(current, testHoldFinalizer) { + return + } + Expect(k8sClient.Patch(context.Background(), current, client.MergeFrom(original))).To(Succeed(), "failed to remove the test finalizer from the listener") +}