Use ActionableRevision and observed generation to track update propagation by nikola-jokic · Pull Request #4575 · actions/actions-runner-controller

nikola-jokic · 2026-07-20T12:09:28Z

Previously the controllers knew about the update when the spec hash contained a mismatch. It caused unnecessary hash calculations on each reconciliation loop.

This change aims to use 2 fields, 1 is the spec field containing the actual desired actionable revision, while the status field helps us know if the state has been reached.

It reduces the unnecessary allocations as well as hash computation.

Based on #4568

Copilot

Pull request overview

This PR replaces the previous “integrity-hash mismatch” update-detection approach with (a) a new in-memory ResourceCache intended to reduce allocations during desired-object construction and (b) restart-safe status markers (ActionableRevision / AppliedActionableRevision and ObservedGeneration) to track update propagation without recomputing hashes every reconcile.

Changes:

Introduces a typed ResourceCache and wires it into controller ResourceBuilder usage.
Replaces spec-hash-based propagation with EphemeralRunnerSet.Spec.ActionableRevision + Status.AppliedActionableRevision, and adds AutoscalingRunnerSet.Status.ObservedGeneration.
Updates CRDs/charts and expands controller/unit tests to cover the new propagation semantics and cache lifecycle.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
main.go	Instantiates a shared `ResourceCache` and injects it into the `ResourceBuilder` used by reconcilers.
controllers/actions.github.com/resourcecache.go	Adds the new in-memory cache implementation for desired resource objects.
controllers/actions.github.com/resourcecache_test.go	Adds unit tests for cache keying/invalidation and deletion behavior.
controllers/actions.github.com/resourcebuilder.go	Removes integrity-hash annotations and attempts to use `ResourceCache` to reuse built objects.
controllers/actions.github.com/resourcebuilder_test.go	Updates tests to initialize `ResourceCache` and removes integrity-hash assertions.
controllers/actions.github.com/helpers.go	Adds helper logic for actionable-spec detection, actionable revision incrementing, and pod canonical/spec comparison.
controllers/actions.github.com/helpers_test.go	Adds unit tests for the new helper functions.
controllers/actions.github.com/ephemeralrunnerset_controller.go	Switches spec-change detection to actionable revision and patches applied revision status after cleanup.
controllers/actions.github.com/ephemeralrunnerset_controller_test.go	Adds scenarios validating cleanup semantics and applied-revision advancement (including failure/restart cases).
controllers/actions.github.com/ephemeralrunner_controller.go	Adjusts cleanup flow and ensures cache entries are deleted on finalization paths.
controllers/actions.github.com/ephemeralrunner_controller_test.go	Updates tests to validate cache cleanup on runner deletion.
controllers/actions.github.com/autoscalingrunnerset_controller.go	Uses `ObservedGeneration` for pending detection and uses `ActionableRevision` when updating the ERS spec.
controllers/actions.github.com/autoscalingrunnerset_controller_test.go	Updates tests to validate observed generation and actionable revision increments and cache behavior.
controllers/actions.github.com/autoscalinglistener_controller.go	Removes integrity-hash-based pod recreation and shifts to spec derivative checks + annotation/label patching.
controllers/actions.github.com/autoscalinglistener_controller_test.go	Updates/extends tests around caching, pod recreation criteria, and metadata propagation expectations.
config/crd/bases/actions.github.com_ephemeralrunnersets.yaml	Adds `actionableRevision` (spec) and `appliedActionableRevision` (status) to the ERS CRD schema.
config/crd/bases/actions.github.com_autoscalingrunnersets.yaml	Adds `observedGeneration` to the ARS CRD schema.
charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml	Mirrors ERS CRD schema changes in the Helm chart CRDs.
charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml	Mirrors ARS CRD schema changes in the Helm chart CRDs.
charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml	Mirrors ERS CRD schema changes in the experimental chart CRDs.
charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml	Mirrors ARS CRD schema changes in the experimental chart CRDs.
apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go	Adds `ActionableRevision` to spec and `AppliedActionableRevision` to status.
apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go	Adds `ObservedGeneration` to status.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The `actions.github.com/integrity-hash` annotation was used as an opaque fingerprint to detect spec drift across AutoscalingRunnerSet, EphemeralRunnerSet and the listener resources. Hashes are brittle: they change whenever unrelated serialization details change, they are invisible to users, and they are not restart-safe. FNV-32a also carries a real collision risk, where the consequence is an update silently never applied. Replace it with explicit, typed state: - `AutoscalingRunnerSetStatus.ObservedGeneration` drives the Pending phase transition via `metadata.generation` instead of an annotation hash. - `EphemeralRunnerSetSpec.ActionableRevision` and `EphemeralRunnerSetStatus.AppliedActionableRevision` form a restart-safe applied marker. The revision is bumped by the AutoscalingRunnerSet controller when `EphemeralRunnerSpec` changes, and only advanced in status after idle/pending runner cleanup succeeds. - `EphemeralRunnerSetStatus.FinishedRunnerCleanupPatchID` records the listener patch ID for which finished runners were reaped, so scale-up is suppressed until the listener publishes a fresh desired state. This prevents creating a replacement runner for a job that already completed. - Listener pod recreation compares pod specs semantically instead of comparing hash annotations. Drift detection uses `apiequality.Semantic`, not `cmp` or `reflect`: - `Semantic.DeepEqual` for the EphemeralRunnerSpec. Most PodSpec collection fields carry `omitempty`, so a template containing an explicitly empty value (`env: []`) is dropped when the EphemeralRunnerSet is written and reads back as nil. A strict comparison reports drift on every reconcile, bumping ActionableRevision each time and deleting every idle and pending runner, forever. Semantic treats nil and empty as equal, understands resource.Quantity, and cannot panic on unexported fields the way cmp can. It is also roughly six times cheaper than cmp.Equal on a realistic spec. - `Semantic.DeepDerivative` for the listener pod, because the live pod carries many fields the desired pod never sets (nodeName, dnsPolicy, default tolerations, the kube-api-access volume, ...). DeepEqual there would spin in a delete/create loop. Container port length is checked separately, since ports come from the --listener-metrics-addr flag rather than from a resource, so disabling metrics would otherwise leave the port on the pod forever. Drift detection is deliberately not short-circuited on metadata.generation. Re-registration changes the runner scale set ID through an annotation, and metadata changes do not bump generation, so a generation-based shortcut would leave the EphemeralRunnerSet pointing at a scale set that no longer exists. The measured saving did not justify the risk. Additionally: - Count deleting runners toward the scale-up total so terminating runners are not double-replaced. - Cleanup of finished runners is no longer deferred; failures now surface as reconcile errors instead of being logged and swallowed. - Status patches for the new fields use `RetryOnConflict` against a freshly read object. - Keep merging EphemeralRunnerSet annotations and labels rather than overwriting them, so metadata applied by admission webhooks or other controllers is preserved. Drift detection compares against the merge result so foreign keys cannot cause a permanent patch loop. - Add unit tests and benchmarks for both drift checks, including a guard that fails if the listener comparison is ever tightened to DeepEqual. - Cover the re-registration path, which previously had no assertion that the new runner scale set ID reaches the EphemeralRunnerSet at all.

Both ephemeralRunnerSetActionableSpecChanged and nextActionableRevision are referenced from autoscalingrunnerset_controller.go, so the blank assignments that suppressed unused-function warnings are no longer needed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Now that update propagation is tracked via ActionableRevision and observedGeneration, the integrity-hash helpers have no callers: - AutoscalingRunnerSet.Hash and AutoscalingListenerSpec.Hash lost their last callers in this change. - ListenerSpecHash, RunnerSetSpecHash, EphemeralRunnerSet.EphemeralRunnerSpecHash and EphemeralRunnerSpec.Hash were already unreferenced. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

A status-phase update bug in the new actionable-revision status patching can leave EphemeralRunnerSet phase incorrectly reporting Running when outdated runners still exist.

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/ephemeralrunnerset_controller.go` — `patchAppliedActionableRevisionStatus` only forces Phase=Running when there are no outdated…

Issues resolved since last review (6)

Severity	Finding
	`controllers/actions.github.com/autoscalinglistener_controller.go` — Using `desiredAnnotations := desiredPod.Annotations` makes the reconciler enforce an exact… View resolved comment
	`controllers/actions.github.com/autoscalinglistener_controller.go` — Setting `desiredAnnotations` to exactly `desiredServiceAccount.Annotations` means any annotations… View resolved comment
	`controllers/actions.github.com/resourcecache.go` — `newResourceCacheDependencyObjectRef` always hashes the full dependency object to produce… View resolved comment
	`controllers/actions.github.com/resourcebuilder.go` — This `Get` uses a minimal `cacheKeyObject` (name/namespace only), but `Upsert` stores an entry… View resolved comment
	`controllers/actions.github.com/resourcebuilder.go` — Same issue as the listener/ERS cache lookup: `Get` is called with a minimal `cacheKeyObject` (no… View resolved comment
	`controllers/actions.github.com/resourcebuilder.go` — This cache lookup will never hit as written: `ResourceCacheState.Get` derives the cache entry… View resolved comment

An outdated runner used to flip the whole scale set into the Outdated phase permanently, which tears down the listener and switches the scale set off. That verdict outlived the runner spec it was about: a runner busy with a job survives the revision cleanup that follows a spec update, and only reports Outdated once the job finishes. The result was that a freshly applied fix could be discarded by a runner that never ran it. Runners are now stamped with the actionable revision they were built from. An Outdated runner whose revision is behind the applied revision is considered stale: it is deleted so the scaling logic replaces it with one built from the current spec, and it no longer contributes to the set's phase. A runner at the current revision still marks the set Outdated, so a genuinely bad spec is still surfaced. Two supporting fixes: - patchAppliedActionableRevisionStatus now recomputes the phase in both directions. It only ever forced Running, so the early-return path could leave a stale Outdated behind. - The AutoscalingRunnerSet only tears down on an Outdated set once that set has applied its current actionable revision. Otherwise a spec update races the EphemeralRunnerSet controller and the teardown fires against a phase that predates the update. Runners created before this change parse to revision 0, which matches the zero value of AppliedActionableRevision, so they are treated as current until a revision is actually applied. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🔵 Needs a closer look

Listener configuration changes can fail to restart the pod, leaving it with stale configuration.

Review tier: Balanced
Findings: None

Issues resolved since last review (1)

Severity	Finding
	`controllers/actions.github.com/ephemeralrunnerset_controller.go` — `patchAppliedActionableRevisionStatus` only forces Phase=Running when there are no outdated… View resolved comment

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

controllers/actions.github.com/autoscalinglistener_controller.go:466

This comparison drops dependency-driven restarts. A listener reads config.json only once during startup (cmd/ghalistener/main.go:24-30), but changing the config Secret's contents/resource version does not change the PodSpec, so this now returns false and the running listener keeps its old configuration. Preserve a restart token derived from the config dependency (or otherwise compare the dependency revision) when deciding whether to recreate the pod.
controllers/actions.github.com/ephemeralrunnerset_controller_test.go:630
This no longer verifies the stated behavior. Eventually succeeds as soon as it observes the initial two runners, so the test still passes if the reconciler deletes one immediately afterward. Keep this as Consistently so the assertion proves finished runners remain present while the patch ID is unchanged.

…-annotation-fingerprint

Replacing the integrity hash with a pod spec comparison lost the one signal the spec cannot carry. The listener mounts its config as a secret volume and parses it once at startup, so a change to the scale set URL, the TLS certificate, the metrics configuration or the scaler tuning only reaches the listener after a restart. The pod references the secret by name, so the spec is byte-identical before and after and the pod was never recreated. The desired pod now carries the config secret's resource version as an annotation, which is free to read and moves exactly when the secret is written. An empty annotation on the live pod is ignored so that pods created by an older controller are not all recreated on upgrade. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

nikola-jokic · 2026-09-09T10:41:11Z

Superseded by stack #4645

This PR has been split into a 7-PR stack so each change can be reviewed on its own. The top of the stack is functionally identical to this branch (verified by diffing refs/pull/4575/head against the stack top — the only remaining delta is explanatory comments and one equivalent early-return refactor).

Stack: https://github.com/actions/actions-runner-controller/pull/4645 (base master), bottom to top:

Fix EphemeralRunnerSet metadata drift check comparing against the wrong value #4634 — Fix EphemeralRunnerSet metadata drift check comparing against the wrong value
Detect listener pod drift by comparing the pod, not a hash #4635 — Detect listener pod drift by comparing the pod, not a hash
Track AutoscalingRunnerSet updates with metadata.generation #4636 — Track AutoscalingRunnerSet updates with metadata.generation
Track runner spec updates with an actionable revision #4638 — Track runner spec updates with an actionable revision
Defer scale up until the listener publishes a state that accounts for finished runners #4642 — Defer scale up until the listener publishes a state that accounts for finished runners
Remove the integrity hash annotation #4643 — Remove the integrity hash annotation
Make the Outdated phase revision-aware #4644 — Make the Outdated phase revision-aware

Each layer builds, vets and passes the full envtest suite on its own, and each regenerates CRDs so both chart copies stay byte-identical to config/crd/bases.

Marking this as a draft; it is kept for history and should not be reviewed. Review the stack instead.

nikola-jokic requested review from Steve-Glass, mumoshu, rentziass and toast-gear as code owners July 20, 2026 12:09

nikola-jokic changed the base branch from nikola-jokic/improve-perf-foundation to master July 20, 2026 12:11

Copilot AI review requested due to automatic review settings July 21, 2026 09:13

Copilot started reviewing on behalf of nikola-jokic July 21, 2026 09:13 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

nikola-jokic force-pushed the nikola-jokic/remove-annotation-fingerprint branch from 0ce130d to 36d8aae Compare July 21, 2026 14:25

nikola-jokic mentioned this pull request Jul 23, 2026

Use lazy copy to patch resources to ensure multiple modifications are applied to the base resource #4580

Open

rentziass previously approved these changes Sep 7, 2026

View reviewed changes

nikola-jokic dismissed rentziass’s stale review via 3ba17c7 September 7, 2026 11:58

nikola-jokic force-pushed the nikola-jokic/remove-annotation-fingerprint branch from 3ba17c7 to 92a1fd6 Compare September 7, 2026 13:31

nikola-jokic force-pushed the nikola-jokic/remove-annotation-fingerprint branch from 92a1fd6 to e481f69 Compare September 7, 2026 20:13

nikola-jokic and others added 2 commits September 8, 2026 13:17

nikola-jokic requested a lite review from Copilot September 8, 2026 14:56

Copilot started reviewing on behalf of nikola-jokic September 8, 2026 14:57 View session

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Comment thread controllers/actions.github.com/ephemeralrunnerset_controller.go Outdated

nikola-jokic requested a review from a team as a code owner September 8, 2026 15:27

nikola-jokic requested a balanced review from Copilot September 9, 2026 07:36

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

Copilot AI reviewed Sep 9, 2026

View reviewed changes

nikola-jokic and others added 2 commits September 9, 2026 10:28

Merge remote-tracking branch 'origin/master' into nikola-jokic/remove…

2558bfa

…-annotation-fingerprint

This was referenced Sep 9, 2026

Fix EphemeralRunnerSet metadata drift check comparing against the wrong value #4634

Merged

Detect listener pod drift by comparing the pod, not a hash #4635

Merged

Track AutoscalingRunnerSet updates with metadata.generation #4636

Merged

nikola-jokic marked this pull request as draft September 9, 2026 10:41

Navigation Menu

Uh oh!

Use ActionableRevision and observed generation to track update propagation - #4575

Use ActionableRevision and observed generation to track update propagation#4575
nikola-jokic wants to merge 6 commits into
masterfrom
nikola-jokic/remove-annotation-fingerprint

nikola-jokic commented Jul 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

nikola-jokic commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

nikola-jokic commented Jul 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

nikola-jokic commented Sep 9, 2026

Superseded by stack #4645

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants