From b20df164d6ee3c2b0ddfdd647bb9189462799dac Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 9 Sep 2026 11:57:05 +0200 Subject: [PATCH] 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 afcb4c93..018f71c9 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 8b78ce24..38628427 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 8b78ce24..38628427 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 8b78ce24..38628427 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 ab44125f..8d28b5a2 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 e122cb73..bae467de 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") }) })