Track AutoscalingRunnerSet updates with metadata.generation by nikola-jokic · Pull Request #4636 · actions/actions-runner-controller

nikola-jokic · 2026-09-09T09:57:24Z

Layer 3 of a stack that splits #4575 into reviewable pieces. Its base is nikola-jokic-l2-listener-pod-recreation (layer 2), not master.

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.

Why the early return is gone

The old code did return ctrl.Result{}, nil right after stamping the hash. That was necessary because stamping the annotation was itself a write to the object, so carrying on would have meant working from a stale copy and racing the update it had just made. Recording the generation only touches the status subresource, which does not bump metadata.generation, so nothing the rest of the reconcile reads is invalidated. Falling through lets the same pass apply the change instead of dropping it and waiting for the next watch event.

What changes for label-only edits

An earlier version of this description claimed that editing a label no longer restarts anything. That was wrong, and the correction is worth stating plainly because it is the one place where swapping the hash for the generation is not a straight substitution.

The desired AutoscalingListener is built from the AutoscalingRunnerSet's labels and annotations as well as its spec, and the reconcile deletes the listener whenever any of the three differ. That has been true since #4516 and is unchanged here. So a label-only edit does rebuild the listener, both before and after this change.

What the old label-inclusive hash gave us was a Pending phase while that happened. metadata.generation does not move for a metadata-only write, so keying the phase solely on it left the scale set reporting Running while its listener was deleted, and reporting Running indefinitely if the rebuild never succeeded. That was a real regression, caught in review.

The fix is to mark the resource Pending at the point the listener is deleted, rather than inferring it from the generation. That matches what the phase is documented to mean — "the listener is not yet started" — and it keys off the actual decision to rebuild instead of guessing from what triggered it. It also covers rebuilds that the generation could never have predicted.

So the accurate summary is narrower than the original claim: ObservedGeneration tracks whether the AutoscalingRunnerSet's own spec has been applied, and the phase separately reports dependent-resource rebuilds. A label edit does not advance the observed generation, because no spec write happened, but it does still rebuild the listener and is still reported as such.

The spec covering this asserts all of it: the label reaches the EphemeralRunnerSet, the listener is re-created with a new UID and carries the new label, the scale set returns to Running afterwards, and the generation and observed generation never move.

Removals

Hash(), ListenerSpecHash() and RunnerSetSpecHash() are removed. The latter two already had no callers, and the first has none once the hash block is gone. AutoscalingListenerSpec.Hash() and EphemeralRunnerSpec.Hash() are untouched; later layers deal with those, as does removing the annotationKeyIntegrityHash constant itself.

CRDs were regenerated with make manifests; only actions.github.com_autoscalingrunnersets.yaml changes, and the chart copies are byte-identical to config/crd/bases.

go build ./..., go vet, gofmt, make manifests and the envtest suite for controllers/actions.github.com/... and apis/... all pass.

Copilot

Copilot review overview

🟡 Changes recommended

Label-only updates can leave the AutoscalingRunnerSet marked Running while its listener is unavailable.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	`controllers/actions.github.com/autoscalingrunnerset_controller.go` — This generation check suppresses `Pending` for label-only edits, but such an edit still recreates…

What changed in this PR

This PR replaces AutoscalingRunnerSet hash tracking with metadata.generation and status.observedGeneration, updating reconciliation logic, tests, and CRD schemas.

Changes:

Adds generation-based status tracking.
Removes obsolete hash helpers.
Updates controller tests and API types.
Regenerates base and chart CRDs.

File	Summary
`controllers/actions.github.com/autoscalingrunnerset_controller.go`	Implements generation tracking; label-only edits may leave the listener unavailable while status remains `Running`.
`controllers/actions.github.com/autoscalingrunnerset_controller_test.go`	Tests generation and label-update behavior.
`config/crd/bases/actions.github.com_autoscalingrunnersets.yaml`	Updates the generated CRD schema.
`charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml`	Synchronizes the stable chart CRD.
`charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml`	Synchronizes the experimental chart CRD.
`apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go`	Adds `ObservedGeneration` and removes obsolete hash helpers.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

humh25 · 2026-09-09T23:51:01Z

536975948763 humbertohlgl El sept 9, 2026, 03:57 -0600, Nikola Jokic ***@***.***>, escribió:

Copilot

Copilot review overview

🟡 Changes recommended

Moderate test-coverage gaps and a stale test comment remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	`controllers/actions.github.com/autoscalingrunnerset_controller_test.go` — The behavior added in the controller at lines 384-393 is the `Pending` status while the listener is…

Issues resolved since last review (1)

Severity	Finding
	`controllers/actions.github.com/autoscalingrunnerset_controller.go` — This generation check suppresses `Pending` for label-only edits, but such an edit still recreates… View resolved comment

Suppressed comments (3)

controllers/actions.github.com/autoscalingrunnerset_controller.go:155

Please add a failure-path assertion for this new contract. The envtest only waits for eventual Running with ObservedGeneration == Generation, so it would also pass if a downstream update failed after this status write and the controller incorrectly advanced ObservedGeneration before applying the spec. Inject a child/listener operation error and assert the old observed generation remains until a retry succeeds.

	if autoscalingRunnerSet.Generation > autoscalingRunnerSet.Status.ObservedGeneration {
		if err := r.updateStatus(
			ctx,
			&autoscalingRunnerSet,
			v1alpha1.AutoscalingRunnerSetPhasePending,

controllers/actions.github.com/autoscalingrunnerset_controller.go:388

The new Pending transition is not asserted by the label-only test: it waits for the recreated listener and then checks Running, so the test would still pass if the listener were deleted while the ARS incorrectly remained Running. Please observe Pending after the listener rebuild starts, before asserting the final Running state.

			if err := r.updateStatus(
				ctx,
				&autoscalingRunnerSet,
				v1alpha1.AutoscalingRunnerSetPhasePending,
				autoscalingRunnerSet.Status.ObservedGeneration,

controllers/actions.github.com/autoscalingrunnerset_controller_test.go:529

These lines still describe the corrected-but-false behavior: a label-only edit does enter Pending when the listener is rebuilt, because the controller now sets that phase before deleting the listener. The generation and ObservedGeneration remain unchanged, but the phase does not; please update this comment so it does not claim label edits no longer go through Pending.

		// 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.

Copilot

Copilot review overview

🔵 Needs a closer look

Moderate test coverage gaps remain for failed reconciles and the listener-deletion Pending transition.

Review tier: Lite
Findings: 1

Pre-existing issues (1)

Severity	Finding
	`controllers/actions.github.com/autoscalingrunnerset_controller_test.go` — The behavior added in the controller at lines 384-393 is the `Pending` status while the listener is… View comment

Suppressed comments (3)

controllers/actions.github.com/autoscalingrunnerset_controller.go:155

The new generation-tracking contract also says a failed reconcile must leave ObservedGeneration at the previous value and keep the phase Pending, but the added tests only wait for eventual Running after successful application. Please add a failure-path test that prevents a dependent resource from being applied and verifies the status remains behind the live generation until a later successful reconcile.

	if autoscalingRunnerSet.Generation > autoscalingRunnerSet.Status.ObservedGeneration {
		if err := r.updateStatus(
			ctx,
			&autoscalingRunnerSet,
			v1alpha1.AutoscalingRunnerSetPhasePending,

controllers/actions.github.com/autoscalingrunnerset_controller_test.go:578

This assertion only observes the listener after it has been recreated, so it cannot catch the regression this change is meant to prevent. If the Pending update in the controller were removed, the listener would still be deleted and recreated and the final phase would still be Running, so this test would pass. Synchronize on the deletion window (accept the listener's NotFound result) and assert the ARS phase is Pending before waiting for the replacement.

			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")

controllers/actions.github.com/autoscalingrunnerset_controller_test.go:529

The opening comment says a label-only edit no longer takes the scale set through Pending, but the new controller code explicitly sets Pending when the resulting listener rebuild is deleted. Only the observed generation remains unchanged; please make this comment describe the transient Pending phase so it does not contradict the test's behavior.

		// 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.

Copilot

Copilot review overview

🔵 Needs a closer look

Add deterministic Pending-state assertions before listener recreation and correct the stale test comment.

Review tier: Lite
Findings: 1

Pre-existing issues (1)

Severity	Finding
	`controllers/actions.github.com/autoscalingrunnerset_controller_test.go` — The behavior added in the controller at lines 384-393 is the `Pending` status while the listener is… View comment

Suppressed comments (3)

controllers/actions.github.com/autoscalingrunnerset_controller.go:390

The new Pending transition is not actually asserted here: the test only waits for the replacement listener and then verifies the final Running state. A regression that removes the updateStatus(...Pending...) call before deletion would still pass, so please add a deterministic failure/delay around listener recreation and assert Pending with the prior ObservedGeneration while the listener is unavailable.

			if err := r.updateStatus(
				ctx,
				&autoscalingRunnerSet,
				v1alpha1.AutoscalingRunnerSetPhasePending,
				autoscalingRunnerSet.Status.ObservedGeneration,
				log,
			); err != nil {

controllers/actions.github.com/autoscalingrunnerset_controller_test.go:578

This spec only waits for the listener to reappear and then for Running; it never observes the ARS while the old listener is being deleted. The new pending-before-delete behavior could regress to the previous Running state and this test would still pass, especially because the deletion/recreation window is asynchronous. Add a deterministic synchronization point (for example, hold or intercept listener deletion) and assert Status.Phase == Pending before allowing recreation.

			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")

controllers/actions.github.com/autoscalingrunnerset_controller_test.go:529

This test comment says a label-only edit no longer moves the scale set through Pending, but the implementation now deliberately sets Pending when the listener is deleted at lines 373-401, and the test below waits for that listener rebuild. Please update the comment so it says the generation/observed-generation do not change while the dependent listener rebuild is still reported as Pending.

		// 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.

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved issues block approval.

Review tier: Lite
Findings: None

Issues resolved since last review (1)

Severity	Finding
	`controllers/actions.github.com/autoscalingrunnerset_controller_test.go` — The behavior added in the controller at lines 384-393 is the `Pending` status while the listener is… View resolved comment

Copilot

Copilot review overview

🟢 Approval recommended

The changes and tests are complete; the remaining feedback is only a non-blocking wording nit.

Review tier: Lite
Findings: None

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>

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>

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>

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review tier: Lite
Findings: None

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues remain in the reviewed changes.

Review tier: Lite
Findings: None

Layers 3 and 4 landed on master as squash merges (#4636, #4638), so the commits this branch built on no longer exist by patch ID. Git fell back to the dependabot bump both sides share and re-reported all of both layers as divergent, conflicting in the five files this layer also touches. Took this side for all five. That is only sound if this branch already contains everything the squashes added, so I checked that from the inputs rather than assuming it. For the types file and the three CRDs the resolution is a pure addition over master -- seven added lines, zero deleted -- which is a stronger statement than patch-ID equivalence because it also rules out a base change having been partly reverted here later. The controller was the one file needing real justification, since it drops fifteen lines of master. Each is a line this layer deliberately replaces: the deferred cleanup, cleanupFinishedEphemeralRunners, the scale-up comparing against total rather than scaleUpTotal, and the cached read in patchAppliedActionableRevisionStatus. Of the fifty lines master added, three are absent, and all three are accounted for: the cached Get, and the Phase and AppliedActionableRevision fields, which are present but realigned by gofmt because the struct literal gained a longer field name. Checked by function set rather than by diff alone, since alignment can swallow a whole function without leaving a mark. The only one of master's functions missing is cleanupFinishedEphemeralRunners, which this layer renames to deleteTerminatedEphemeralRunners. Layer 4's optimistic lock and its >= guard survive; the lock count goes from one to two because this layer adds its own. No Ginkgo spec was lost from either side: every It() name on master and on this branch is present in the result. make manifests reports no drift and the chart CRDs stay byte-identical to config/crd/bases. rerere staged all five resolutions automatically. That is agreement with a cached resolution, not evidence, so none of the above rests on it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

nikola-jokic requested review from a team, Steve-Glass, mumoshu, rentziass and toast-gear as code owners September 9, 2026 09:57

nikola-jokic added this pull request to stack #4645 September 9, 2026 10:40

nikola-jokic mentioned this pull request Sep 9, 2026

Use ActionableRevision and observed generation to track update propagation #4575

Draft

nikola-jokic requested a lite review from Copilot September 9, 2026 21:45

Copilot started reviewing on behalf of nikola-jokic September 9, 2026 21:45 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread controllers/actions.github.com/autoscalingrunnerset_controller.go

nikola-jokic requested a lite review from Copilot September 10, 2026 09:08

Copilot started reviewing on behalf of nikola-jokic September 10, 2026 09:08 View session

nikola-jokic force-pushed the nikola-jokic-l3-ars-observed-generation branch from 8d55cc5 to 989be41 Compare September 10, 2026 09:12

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread controllers/actions.github.com/autoscalingrunnerset_controller_test.go

nikola-jokic force-pushed the nikola-jokic-l3-ars-observed-generation branch from 989be41 to 2866162 Compare September 10, 2026 09:20

nikola-jokic force-pushed the nikola-jokic-l3-ars-observed-generation branch from 2866162 to 398bf7c Compare September 10, 2026 09:52

nikola-jokic requested a lite review from Copilot September 10, 2026 09:52

Copilot started reviewing on behalf of nikola-jokic September 10, 2026 09:53 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

nikola-jokic requested a lite review from Copilot September 10, 2026 10:11

Copilot started reviewing on behalf of nikola-jokic September 10, 2026 10:11 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

nikola-jokic requested a lite review from Copilot September 10, 2026 10:30

Copilot started reviewing on behalf of nikola-jokic September 10, 2026 10:31 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Base automatically changed from nikola-jokic-l2-listener-pod-recreation to master September 10, 2026 20:56

nikola-jokic force-pushed the nikola-jokic-l3-ars-observed-generation branch from ecf6359 to b830b65 Compare September 10, 2026 20:56

nikola-jokic requested a lite review from Copilot September 11, 2026 07:08

Copilot started reviewing on behalf of nikola-jokic September 11, 2026 07:09 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

nikola-jokic and others added 3 commits September 11, 2026 12:03

nikola-jokic force-pushed the nikola-jokic-l3-ars-observed-generation branch from b830b65 to fbbeeec Compare September 11, 2026 10:03

nikola-jokic requested a lite review from Copilot September 11, 2026 10:49

Copilot started reviewing on behalf of nikola-jokic September 11, 2026 10:49 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

nikola-jokic requested a lite review from Copilot September 11, 2026 11:16

Copilot started reviewing on behalf of nikola-jokic September 11, 2026 11:17 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

rentziass approved these changes Sep 11, 2026

View reviewed changes

nikola-jokic merged commit 5581cd0 into master Sep 11, 2026
33 checks passed

nikola-jokic deleted the nikola-jokic-l3-ars-observed-generation branch September 11, 2026 12:38

nikola-jokic mentioned this pull request Sep 11, 2026

Defer scale up until the listener publishes a state that accounts for finished runners #4642

Merged

Navigation Menu

Uh oh!

Track AutoscalingRunnerSet updates with metadata.generation - #4636

Track AutoscalingRunnerSet updates with metadata.generation#4636
nikola-jokic merged 3 commits into
masterfrom
nikola-jokic-l3-ars-observed-generation

nikola-jokic commented Sep 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

humh25 commented Sep 9, 2026 via email

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

nikola-jokic commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why the early return is gone

What changes for label-only edits

Removals

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

humh25 commented Sep 9, 2026 via email

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nikola-jokic commented Sep 9, 2026 •

edited

Loading