From e481f69cffcf672215090726075d7b9bf7f20eaf Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Mon, 7 Sep 2026 14:23:48 +0200 Subject: [PATCH 1/5] Replace integrity-hash annotations with explicit API state fields 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. --- .../v1alpha1/autoscalingrunnerset_types.go | 4 + .../v1alpha1/ephemeralrunnerset_types.go | 16 + ...ions.github.com_autoscalingrunnersets.yaml | 6 + ...ctions.github.com_ephemeralrunnersets.yaml | 21 + ...ions.github.com_autoscalingrunnersets.yaml | 6 + ...ctions.github.com_ephemeralrunnersets.yaml | 21 + ...ions.github.com_autoscalingrunnersets.yaml | 6 + ...ctions.github.com_ephemeralrunnersets.yaml | 21 + .../autoscalinglistener_controller.go | 2 +- .../autoscalingrunnerset_controller.go | 57 +- .../autoscalingrunnerset_controller_test.go | 221 +++++- .../ephemeralrunnerset_controller.go | 99 ++- .../ephemeralrunnerset_controller_test.go | 649 +++++++++++++++++- controllers/actions.github.com/helpers.go | 106 +++ .../actions.github.com/helpers_bench_test.go | 122 ++++ .../actions.github.com/helpers_drift_test.go | 147 ++++ .../helpers_listener_test.go | 277 ++++++++ .../actions.github.com/resourcebuilder.go | 156 +---- .../resourcebuilder_test.go | 19 +- .../actions.github.com/resourcecache.go | 3 - .../actions.github.com/resourcecache_test.go | 15 - 21 files changed, 1724 insertions(+), 250 deletions(-) create mode 100644 controllers/actions.github.com/helpers.go create mode 100644 controllers/actions.github.com/helpers_bench_test.go create mode 100644 controllers/actions.github.com/helpers_drift_test.go create mode 100644 controllers/actions.github.com/helpers_listener_test.go diff --git a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go index dd1bce7fa1..b383c2c49d 100644 --- a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go @@ -319,6 +319,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 diff --git a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go index 641ec02f88..22ed2dd9fe 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go @@ -36,12 +36,28 @@ type EphemeralRunnerSetSpec struct { // but does not apply to existing ephemeral runners. // +optional EphemeralRunnerMetadata *ResourceMeta `json:"ephemeralRunnerMetadata,omitempty"` + // ActionableRevision is a restart-safe applied marker that increments whenever + // Spec.EphemeralRunnerSpec changes, enabling detection of spec updates. + // Unset defaults to 0. + // +optional + ActionableRevision int64 `json:"actionableRevision,omitempty"` } // EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet type EphemeralRunnerSetStatus struct { // +optional Phase EphemeralRunnerSetPhase `json:"phase"` + // AppliedActionableRevision is a restart-safe applied marker tracking the last successfully + // applied ActionableRevision value. Advances only after spec cleanup succeeds. + // Unset defaults to 0. + // +optional + AppliedActionableRevision int64 `json:"appliedActionableRevision,omitempty"` + // FinishedRunnerCleanupPatchID records the listener patch ID for which finished + // ephemeral runners were cleaned up. Scale-up is suppressed for the same patch ID + // until the listener publishes a fresh desired-state patch. + // Unset defaults to 0. + // +optional + FinishedRunnerCleanupPatchID int `json:"finishedRunnerCleanupPatchID,omitempty"` } // EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource 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 a2dfd46d9f..27c74aabee 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 @@ -16541,6 +16541,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-experimental/crds/actions.github.com_ephemeralrunnersets.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml index a6f9758bfa..f696ff054d 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml @@ -46,6 +46,13 @@ spec: spec: description: EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet properties: + actionableRevision: + description: |- + ActionableRevision is a restart-safe applied marker that increments whenever + Spec.EphemeralRunnerSpec changes, enabling detection of spec updates. + Unset defaults to 0. + format: int64 + type: integer ephemeralRunnerMetadata: description: |- EphemeralRunnerMetadata is the metadata to be applied to all ephemeral runners created by this set. @@ -8295,6 +8302,20 @@ spec: status: description: EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet properties: + appliedActionableRevision: + description: |- + AppliedActionableRevision is a restart-safe applied marker tracking the last successfully + applied ActionableRevision value. Advances only after spec cleanup succeeds. + Unset defaults to 0. + format: int64 + type: integer + finishedRunnerCleanupPatchID: + description: |- + FinishedRunnerCleanupPatchID records the listener patch ID for which finished + ephemeral runners were cleaned up. Scale-up is suppressed for the same patch ID + until the listener publishes a fresh desired-state patch. + Unset defaults to 0. + type: integer phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string 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 a2dfd46d9f..27c74aabee 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 @@ -16541,6 +16541,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_ephemeralrunnersets.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml index a6f9758bfa..f696ff054d 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml @@ -46,6 +46,13 @@ spec: spec: description: EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet properties: + actionableRevision: + description: |- + ActionableRevision is a restart-safe applied marker that increments whenever + Spec.EphemeralRunnerSpec changes, enabling detection of spec updates. + Unset defaults to 0. + format: int64 + type: integer ephemeralRunnerMetadata: description: |- EphemeralRunnerMetadata is the metadata to be applied to all ephemeral runners created by this set. @@ -8295,6 +8302,20 @@ spec: status: description: EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet properties: + appliedActionableRevision: + description: |- + AppliedActionableRevision is a restart-safe applied marker tracking the last successfully + applied ActionableRevision value. Advances only after spec cleanup succeeds. + Unset defaults to 0. + format: int64 + type: integer + finishedRunnerCleanupPatchID: + description: |- + FinishedRunnerCleanupPatchID records the listener patch ID for which finished + ephemeral runners were cleaned up. Scale-up is suppressed for the same patch ID + until the listener publishes a fresh desired-state patch. + Unset defaults to 0. + type: integer phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string diff --git a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml index a2dfd46d9f..27c74aabee 100644 --- a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml +++ b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml @@ -16541,6 +16541,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_ephemeralrunnersets.yaml b/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml index a6f9758bfa..f696ff054d 100644 --- a/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml +++ b/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml @@ -46,6 +46,13 @@ spec: spec: description: EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet properties: + actionableRevision: + description: |- + ActionableRevision is a restart-safe applied marker that increments whenever + Spec.EphemeralRunnerSpec changes, enabling detection of spec updates. + Unset defaults to 0. + format: int64 + type: integer ephemeralRunnerMetadata: description: |- EphemeralRunnerMetadata is the metadata to be applied to all ephemeral runners created by this set. @@ -8295,6 +8302,20 @@ spec: status: description: EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet properties: + appliedActionableRevision: + description: |- + AppliedActionableRevision is a restart-safe applied marker tracking the last successfully + applied ActionableRevision value. Advances only after spec cleanup succeeds. + Unset defaults to 0. + format: int64 + type: integer + finishedRunnerCleanupPatchID: + description: |- + FinishedRunnerCleanupPatchID records the listener patch ID for which finished + ephemeral runners were cleaned up. Scale-up is suppressed for the same patch ID + until the listener publishes a fresh desired-state patch. + Unset defaults to 0. + type: integer phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 38e8d64fdb..266d9d60f3 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -463,7 +463,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - shouldReCreate := desiredPod.Annotations[annotationKeyIntegrityHash] != listenerPod.Annotations[annotationKeyIntegrityHash] + shouldReCreate := listenerPodSpecRequiresRecreation(&listenerPod, desiredPod) if shouldReCreate { log.Info("Listener pod dependency changed, recreating listener pod") if err := r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log); err != nil { diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index b32696a954..d0af088b7f 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -143,27 +143,17 @@ 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 { + 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 @@ -293,10 +283,11 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] != desired.Annotations[annotationKeyIntegrityHash] { + if ephemeralRunnerSetActionableSpecChanged(&ephemeralRunnerSet, desired) { original := ephemeralRunnerSet.DeepCopy() ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec + ephemeralRunnerSet.Spec.ActionableRevision = nextActionableRevision(&ephemeralRunnerSet) ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) ephemeralRunnerSet.Annotations = r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) @@ -310,14 +301,20 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } + // Merge rather than overwrite so annotations/labels applied by other + // controllers or users are preserved. Compare against the merge result so + // foreign keys do not make this permanently report "modified". + desiredLabels := r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) + desiredAnnotations := r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) + ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata) - ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desired.Annotations) + ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desiredLabels) + ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desiredAnnotations) if ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerSet.Annotations = r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) + ephemeralRunnerSet.Labels = desiredLabels + ephemeralRunnerSet.Annotations = desiredAnnotations ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata log.Info("Updating ephemeral runner set metadata to match desired labels and annotations") if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { @@ -387,6 +384,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") @@ -431,14 +429,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") @@ -770,7 +776,6 @@ func (r *AutoscalingRunnerSetReconciler) createEphemeralRunnerSet(ctx context.Co log.Error(err, "Could not create EphemeralRunnerSet") return ctrl.Result{}, err } - log.Info("Creating a new EphemeralRunnerSet resource") if err := r.Create(ctx, desiredRunnerSet); err != nil { log.Error(err, "Failed to create EphemeralRunnerSet resource") diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index 30a30c46df..028f64acc5 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -513,7 +513,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") originalRunnerSetUID := runnerSet.UID - originalRunnerSetHash := runnerSet.Annotations[annotationKeyIntegrityHash] + originalActionableRevision := runnerSet.Spec.ActionableRevision patched := autoscalingRunnerSet.DeepCopy() patched.Spec.Template.Spec.Containers[0].Image = "ghcr.io/actions/runner:updated" @@ -526,8 +526,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should be updated in place") + g.Expect(current.Spec.ActionableRevision).To(BeNumerically(">", originalActionableRevision), "ActionableRevision should increment for actionable spec changes") g.Expect(current.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Image).To(Equal("ghcr.io/actions/runner:updated")) - g.Expect(current.Annotations[annotationKeyIntegrityHash]).NotTo(Equal(originalRunnerSetHash), "EphemeralRunnerSet spec hash should change") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -566,7 +566,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") originalRunnerSetUID := runnerSet.UID - originalRunnerSetHash := runnerSet.Annotations[annotationKeyIntegrityHash] + originalActionableRevision := runnerSet.Spec.ActionableRevision patched := autoscalingRunnerSet.DeepCopy() max := 20 @@ -592,7 +592,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should not be recreated") - g.Expect(current.Annotations[annotationKeyIntegrityHash]).To(Equal(originalRunnerSetHash), "EphemeralRunnerSet spec should not change") + g.Expect(current.Spec.ActionableRevision).To(Equal(originalActionableRevision), "ActionableRevision should not change for non-actionable updates") }, time.Second*5, autoscalingRunnerSetTestInterval, @@ -665,6 +665,65 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { ).Should(Succeed(), "EphemeralRunnerSet should be patched with annotation-only metadata drift") }) + It("preserves foreign annotations and labels on the EphemeralRunnerSet", func() { + runnerSet := new(v1alpha1.EphemeralRunnerSet) + Eventually( + func() (string, error) { + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + if err != nil { + return "", err + } + return runnerSet.Annotations["arc.test/metadata-annotation"], nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Equal("initial"), "EphemeralRunnerSet should start with the predefined annotation") + + // Simulate a third party (admission webhook, another controller, a user) + // adding metadata the AutoscalingRunnerSet knows nothing about. + foreign := runnerSet.DeepCopy() + foreign.Annotations["thirdparty.example.com/injected"] = "keep-me" + foreign.Labels["thirdparty.example.com/injected"] = "keep-me" + err := k8sClient.Patch(ctx, foreign, client.MergeFrom(runnerSet)) + Expect(err).NotTo(HaveOccurred(), "failed to inject foreign metadata on EphemeralRunnerSet") + + // Force the controller through the metadata reconciliation path. + patched := autoscalingRunnerSet.DeepCopy() + patched.Spec.EphemeralRunnerSetMetadata.Annotations["arc.test/metadata-annotation"] = "updated" + err = k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet)) + Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet EphemeralRunnerSet metadata") + + Eventually( + func(g Gomega) { + current := new(v1alpha1.EphemeralRunnerSet) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) + g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + g.Expect(current.Annotations["arc.test/metadata-annotation"]).To(Equal("updated")) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "desired annotation should still propagate") + + // The foreign keys must survive, and the resourceVersion must settle + // instead of the controller fighting over the annotation set forever. + settled := new(v1alpha1.EphemeralRunnerSet) + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, settled)).To(Succeed()) + lastResourceVersion := settled.ResourceVersion + + Consistently( + func(g Gomega) { + current := new(v1alpha1.EphemeralRunnerSet) + err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) + g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + g.Expect(current.Annotations).To(HaveKeyWithValue("thirdparty.example.com/injected", "keep-me"), "foreign annotation must not be stripped") + g.Expect(current.Labels).To(HaveKeyWithValue("thirdparty.example.com/injected", "keep-me"), "foreign label must not be stripped") + g.Expect(current.ResourceVersion).To(Equal(lastResourceVersion), "EphemeralRunnerSet should stop being patched once metadata has converged") + }, + 3*time.Second, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + }) + It("updates EphemeralRunnerSet runner metadata when only EphemeralRunner metadata changes", func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( @@ -910,10 +969,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") @@ -928,7 +983,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") }) }) @@ -2196,6 +2254,7 @@ var _ = Describe("Test AutoscalingRunnerSet with a stale runner scale set", Orde ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -2246,6 +2305,148 @@ var _ = Describe("Test AutoscalingRunnerSet with a stale runner scale set", Orde autoscalingRunnerSetTestInterval, ).Should(BeEquivalentTo(freshRunnerScaleSetID), "the listener should never be created with the stale runner scale set ID") }) + + // The listener is not the only thing that has to follow a re-registration: + // the EphemeralRunnerSet carries the scale set ID down to every runner, so + // assert the propagation here rather than only on the listener. + It("propagates the fresh runner scale set ID to the EphemeralRunnerSet", func() { + runnerSet := new(v1alpha1.EphemeralRunnerSet) + Eventually( + func() (int, error) { + if err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet); err != nil { + return 0, err + } + return runnerSet.Spec.EphemeralRunnerSpec.RunnerScaleSetID, nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeEquivalentTo(freshRunnerScaleSetID), + "the EphemeralRunnerSet must be re-pointed at the newly registered runner scale set") + + // The runners themselves are still registered against the dead scale + // set, so the revision has to advance for them to be cleaned up. + Expect(runnerSet.Spec.ActionableRevision).To(BeNumerically(">", 0), + "re-registration must bump ActionableRevision so existing runners are replaced") + }) + }) + + // A scale set can lose its Actions service counterpart long after it has + // settled, and re-registration then changes the runner scale set ID without + // anything in the AutoscalingRunnerSet spec changing. This exercises that + // full transition. + // + // It also pins down why drift detection cannot be keyed on + // metadata.generation: re-registration writes the ID as an annotation, and + // metadata changes do not bump generation. A generation-based shortcut would + // leave the EphemeralRunnerSet pointing at the dead scale set forever. + Context("When a settled runner scale set disappears from the Actions service", func() { + const originalRunnerScaleSetID = 77 + const replacementRunnerScaleSetID = 78 + + var ctx context.Context + var mgr ctrl.Manager + var autoscalingNS *corev1.Namespace + var autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet + var scaleSetDeleted atomic.Bool + + BeforeEach(func() { + ctx = context.Background() + autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) + configSecret := createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + scaleSetDeleted.Store(false) + + controller := &AutoscalingRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ControllerNamespace: autoscalingNS.Name, + DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( + scalefake.WithClient( + scalefake.NewClient( + scalefake.WithGetRunnerGroupByName(&scaleset.RunnerGroup{ID: 1, Name: "testgroup"}, nil), + scalefake.WithGetRunnerScaleSetByIDFunc(func(_ context.Context, runnerScaleSetID int) (*scaleset.RunnerScaleSet, error) { + if runnerScaleSetID == originalRunnerScaleSetID && scaleSetDeleted.Load() { + return nil, scaleset.NotFoundError + } + return &scaleset.RunnerScaleSet{ID: runnerScaleSetID, Name: "test-asrs", RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil + }), + scalefake.WithGetRunnerScaleSet(nil, nil), + scalefake.WithCreateRunnerScaleSet(&scaleset.RunnerScaleSet{ID: replacementRunnerScaleSetID, Name: "test-asrs", RunnerGroupID: 1, RunnerGroupName: "testgroup"}, nil), + scalefake.WithDeleteRunnerScaleSet(nil), + ), + ), + )), + }, + } + Expect(controller.SetupWithManager(mgr)).To(Succeed(), "failed to setup controller") + startManagers(GinkgoT(), mgr) + + autoscalingRunnerSet = newAutoscalingRunnerSet(autoscalingNS.Name, configSecret.Name, registeredAnnotations(originalRunnerScaleSetID)) + // Set the scale set name explicitly. createRunnerScaleSet defaults an + // empty Spec.RunnerScaleSetName to the object name, and that spec write + // bumps metadata.generation, which would let a generation-based + // shortcut pass this test for the wrong reason. + autoscalingRunnerSet.Spec.RunnerScaleSetName = "test-asrs" + Expect(k8sClient.Create(ctx, autoscalingRunnerSet)).To(Succeed(), "failed to create AutoScalingRunnerSet") + }) + + It("re-points the EphemeralRunnerSet without any spec change on the AutoscalingRunnerSet", func() { + // Let the scale set settle first, so observedGeneration catches up with + // generation and the re-registration below is the only thing in flight. + var settledGeneration int64 + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) + g.Expect(current.Status.ObservedGeneration).To(Equal(current.Generation), + "AutoscalingRunnerSet should reach a settled state") + settledGeneration = current.Generation + + runnerSet := new(v1alpha1.EphemeralRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet)).To(Succeed()) + g.Expect(runnerSet.Spec.EphemeralRunnerSpec.RunnerScaleSetID).To(Equal(originalRunnerScaleSetID)) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + // The scale set is deleted on the Actions service side. Nothing about + // the AutoscalingRunnerSet spec changes as a result. + scaleSetDeleted.Store(true) + + // Re-registration is only considered when the listener has to be + // created, so drop the listener the way an operator or an eviction + // would. This deliberately does not touch the AutoscalingRunnerSet, so + // its generation stays put. + listener := new(v1alpha1.AutoscalingListener) + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingNS.Name}, listener)).To(Succeed()) + Expect(k8sClient.Delete(ctx, listener)).To(Succeed()) + + Eventually( + func(g Gomega) { + current := new(v1alpha1.EphemeralRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current)).To(Succeed()) + g.Expect(current.Spec.EphemeralRunnerSpec.RunnerScaleSetID).To(Equal(replacementRunnerScaleSetID), + "the EphemeralRunnerSet must be re-pointed at the newly registered runner scale set even though the AutoscalingRunnerSet spec never changed") + g.Expect(current.Spec.ActionableRevision).To(BeNumerically(">", 0), + "re-registration must bump ActionableRevision so runners registered against the dead scale set are replaced") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + // Guard the premise of the test: re-registration must reach the + // EphemeralRunnerSet purely through a spec content change. If it ever + // starts writing to the AutoscalingRunnerSet spec, generation would + // bump and this would stop demonstrating that. + settled := new(v1alpha1.AutoscalingRunnerSet) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), settled)).To(Succeed()) + Expect(settled.Generation).To(Equal(settledGeneration), + "re-registration must not change the AutoscalingRunnerSet spec, otherwise this test proves nothing") + }) }) Context("When the Actions service cannot confirm whether the runner scale set exists", func() { @@ -2268,6 +2469,7 @@ var _ = Describe("Test AutoscalingRunnerSet with a stale runner scale set", Orde ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -2328,6 +2530,7 @@ var _ = Describe("Test AutoscalingRunnerSet with a stale runner scale set", Orde ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index c4f6d8c025..d979813732 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -35,6 +35,7 @@ import ( kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -133,11 +134,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, nil } - // If hash spec has changed, delete idle ephemeral runners - // in order to apply the change to the runners that did not yet receive a job. - ephemeralRunnerIntegrityHash := ephemeralRunnerSetIntegrityHash(&ephemeralRunnerSet) - if ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] != ephemeralRunnerIntegrityHash { - log.Info("EphemeralRunnerSpec has changed, deleting idle ephemeral runners to apply the new spec") + if ephemeralRunnerSet.Spec.ActionableRevision > ephemeralRunnerSet.Status.AppliedActionableRevision { + log.Info( + "EphemeralRunnerSpec revision has changed, deleting idle or pending ephemeral runners to apply the new spec", + "specActionableRevision", ephemeralRunnerSet.Spec.ActionableRevision, + "statusAppliedActionableRevision", ephemeralRunnerSet.Status.AppliedActionableRevision, + ) if _, err := r.cleanUpEphemeralRunners(ctx, &ephemeralRunnerSet, log); err != nil { log.Error(err, "Failed to clean up EphemeralRunners") return ctrl.Result{}, err @@ -148,18 +150,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } - log.Info("Updating EphemeralRunnerSet with new spec hash") - original := ephemeralRunnerSet.DeepCopy() - if ephemeralRunnerSet.Annotations == nil { - ephemeralRunnerSet.Annotations = make(map[string]string) - } - ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerIntegrityHash - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update ephemeral runner set with new spec hash") + if err := r.patchAppliedActionableRevisionStatus(ctx, req.NamespacedName, ephemeralRunnerSet.Spec.ActionableRevision); err != nil { + log.Error(err, "Failed to update EphemeralRunnerSet applied actionable revision status") return ctrl.Result{}, err } - log.Info("Updated ephemeral runner set with new spec hash") + log.Info("Updated EphemeralRunnerSet applied actionable revision status", "appliedActionableRevision", ephemeralRunnerSet.Spec.ActionableRevision) return ctrl.Result{}, nil } @@ -205,15 +201,31 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R total := ephemeralRunnersByState.scaleTotal() if ephemeralRunnerSet.Spec.PatchID == 0 || ephemeralRunnerSet.Spec.PatchID != ephemeralRunnersByState.latestPatchID { - defer func() { + if len(ephemeralRunnersByState.finished) > 0 { if err := r.cleanupFinishedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil { log.Error(err, "failed to cleanup finished ephemeral runners") + return ctrl.Result{}, err } - }() - log.Info("Scaling comparison", "current", total, "desired", ephemeralRunnerSet.Spec.Replicas) + if err := r.patchFinishedRunnerCleanupPatchIDStatus(ctx, req.NamespacedName, ephemeralRunnerSet.Spec.PatchID); err != nil { + log.Error(err, "failed to update finished runner cleanup patch ID status") + return ctrl.Result{}, err + } + ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID = ephemeralRunnerSet.Spec.PatchID + + log.Info("Finished ephemeral runners were cleaned up, deferring scaling decision") + return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) + } + + scaleUpTotal := total + len(ephemeralRunnersByState.deleting) + log.Info("Scaling comparison", "current", total, "deleting", len(ephemeralRunnersByState.deleting), "desired", ephemeralRunnerSet.Spec.Replicas) switch { - case total < ephemeralRunnerSet.Spec.Replicas: // Handle scale up - count := ephemeralRunnerSet.Spec.Replicas - total + case scaleUpTotal < ephemeralRunnerSet.Spec.Replicas: // Handle scale up + if ephemeralRunnerSet.Spec.PatchID > 0 && ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID == ephemeralRunnerSet.Spec.PatchID { + log.Info("Skipping scale up until listener publishes a fresh desired state after finished runner cleanup", "patchID", ephemeralRunnerSet.Spec.PatchID) + return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) + } + + count := ephemeralRunnerSet.Spec.Replicas - scaleUpTotal log.Info("Creating new ephemeral runners (scale up)", "count", count) if err := r.createEphemeralRunners(ctx, &ephemeralRunnerSet, count, log); err != nil { log.Error(err, "failed to make ephemeral runner") @@ -245,6 +257,51 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) } +func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error { + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + var latest v1alpha1.EphemeralRunnerSet + if err := r.Get(ctx, key, &latest); err != nil { + return err + } + + original := latest.DeepCopy() + latest.Status.AppliedActionableRevision = targetAppliedRevision + + ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList) + if err := r.List(ctx, ephemeralRunnerList, client.InNamespace(latest.Namespace), client.MatchingFields{resourceOwnerKey: latest.Name}); err != nil { + return fmt.Errorf("failed to list child ephemeral runners: %w", err) + } + + if len(newEphemeralRunnersByStates(ephemeralRunnerList).outdated) == 0 { + latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + } + + if original.Status == latest.Status { + return nil + } + + return r.Status().Patch(ctx, &latest, client.MergeFrom(original)) + }) +} + +func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(ctx context.Context, key types.NamespacedName, patchID int) error { + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + var latest v1alpha1.EphemeralRunnerSet + if err := r.Get(ctx, key, &latest); err != nil { + return err + } + + original := latest.DeepCopy() + latest.Status.FinishedRunnerCleanupPatchID = patchID + + if original.Status == latest.Status { + return nil + } + + return r.Status().Patch(ctx, &latest, client.MergeFrom(original)) + }) +} + func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, state *ephemeralRunnersByState, log logr.Logger) error { original := ephemeralRunnerSet.DeepCopy() var phase v1alpha1.EphemeralRunnerSetPhase @@ -257,7 +314,9 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer phase = ephemeralRunnerSet.Status.Phase } desiredStatus := v1alpha1.EphemeralRunnerSetStatus{ - Phase: phase, + Phase: phase, + AppliedActionableRevision: ephemeralRunnerSet.Status.AppliedActionableRevision, + FinishedRunnerCleanupPatchID: ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID, } // Update the status if needed. diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index 331e8e596c..a563016f41 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -627,7 +627,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { // confirm they are not deleted runnerList = new(v1alpha1.EphemeralRunnerList) - Consistently( + Eventually( func() (int, error) { err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) if err != nil { @@ -690,16 +690,37 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") runnerList = new(v1alpha1.EphemeralRunnerList) - // We should have 3 runners, and have no Succeeded ones Eventually( - func() error { + func() (int, error) { err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) if err != nil { - return err + return -1, err } - if len(runnerList.Items) != 3 { - return fmt.Errorf("Expected 3 runners, got %d", len(runnerList.Items)) + return len(runnerList.Items), nil + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(1), "only the running EphemeralRunner should remain before listener confirms the larger desired count") + + ers = new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated = ers.DeepCopy() + updated.Spec.Replicas = 3 + updated.Spec.PatchID = 3 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + + runnerList = new(v1alpha1.EphemeralRunnerList) + // We should have 3 runners, and have no Succeeded ones after listener confirms. + Eventually( + func() error { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return err } for _, runner := range runnerList.Items { @@ -708,6 +729,10 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { } } + if len(runnerList.Items) != 3 { + return fmt.Errorf("Expected 3 runners, got %d", len(runnerList.Items)) + } + return nil }, ephemeralRunnerSetTestTimeout, @@ -1017,7 +1042,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { } } - if succeeded != 1 && running != 1 { + if succeeded != 1 || running != 1 { return fmt.Errorf("Expected 1 runner in Succeeded and 1 in Running, got %d in Succeeded and %d in Running", succeeded, running) } @@ -1027,8 +1052,9 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ephemeralRunnerSetTestInterval, ).Should(BeNil(), "1 EphemeralRunner should be in Succeeded and 1 in Running phase") - // Now, let's simulate replacement. The desired count is still 2. - // This simulates that we got 1 job assigned, and 1 job completed. + // Now, let's simulate the listener publishing a stale patch before it has + // accounted for the completed job. The controller should clean up the + // finished runner but not create a replacement for this patch. ers = new(v1alpha1.EphemeralRunnerSet) err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) @@ -1041,6 +1067,46 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + runnerList = new(v1alpha1.EphemeralRunnerList) + Eventually( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(1), "the finished EphemeralRunner should be cleaned up") + + Consistently( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + 2*time.Second, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(1), "only the running EphemeralRunner should remain before listener confirms replacement") + + // A fresh listener decision with the same desired count confirms that a + // replacement is still needed. + ers = new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated = ers.DeepCopy() + updated.Spec.Replicas = 2 + updated.Spec.PatchID = 3 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + runnerList = new(v1alpha1.EphemeralRunnerList) Eventually( func() error { @@ -1066,6 +1132,107 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ).Should(BeNil(), "2 EphemeralRunner should be created and none should be in Succeeded phase") }) + It("Should not create a replacement when a runner finishes ahead of the listener decrement patch", func() { + ers := new(v1alpha1.EphemeralRunnerSet) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated := ers.DeepCopy() + updated.Spec.Replicas = 4 + updated.Spec.PatchID = 1 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + + runnerList := new(v1alpha1.EphemeralRunnerList) + Eventually( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(4), "4 EphemeralRunner should be created") + + for i := range 3 { + updatedRunner := runnerList.Items[i].DeepCopy() + updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[i])) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") + } + + updatedRunner := runnerList.Items[3].DeepCopy() + updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded + err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[3])) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") + + ers = new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated = ers.DeepCopy() + updated.Spec.Replicas = 4 + updated.Spec.PatchID = 2 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + + Eventually( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(3), "only the running EphemeralRunners should remain after stale-patch cleanup") + + Consistently( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + 12*time.Second, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(3), "EphemeralRunnerSet should not create a replacement before listener decrements") + + ers = new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) + Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + + updated = ers.DeepCopy() + updated.Spec.Replicas = 3 + updated.Spec.PatchID = 3 + + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) + Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") + + runnerList = new(v1alpha1.EphemeralRunnerList) + Eventually( + func() (int, error) { + err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) + if err != nil { + return -1, err + } + + return len(runnerList.Items), nil + }, + ephemeralRunnerSetTestTimeout, + ephemeralRunnerSetTestInterval, + ).Should(BeEquivalentTo(3), "EphemeralRunnerSet should converge after listener decrements") + }) + It("Should delete idle runners, keep busy runners, and create new runners when the spec changes", func() { ers := new(v1alpha1.EphemeralRunnerSet) err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) @@ -1092,7 +1259,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ).Should(BeEquivalentTo(3), "3 EphemeralRunner should be created") idleRunnerNames := map[string]struct{}{} - for i := 0; i < 2; i++ { + for i := range 2 { idleRunner := runnerList.Items[i].DeepCopy() idleRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning idleRunner.Status.RunnerID = i + 101 @@ -1117,6 +1284,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { updated = ers.DeepCopy() updated.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Image = "ghcr.io/actions/runner:new" + updated.Spec.ActionableRevision = ers.Spec.ActionableRevision + 1 err = k8sClient.Patch(ctx, updated, client.MergeFrom(ers)) Expect(err).NotTo(HaveOccurred(), "failed to patch EphemeralRunnerSet with new spec") @@ -1491,6 +1659,467 @@ var _ = Describe("EphemeralRunner phase metrics", func() { }) }) +var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { + var ctx context.Context + var mgr ctrl.Manager + var autoscalingNS *corev1.Namespace + var configSecret *corev1.Secret + + newRunner := func(name string, ers *v1alpha1.EphemeralRunnerSet) *v1alpha1.EphemeralRunner { + controllerRef := true + return &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ers.Namespace, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: ers.Name, + UID: ers.UID, + Controller: &controllerRef, + }}, + }, + Spec: ers.Spec.EphemeralRunnerSpec, + } + } + + BeforeEach(func() { + ctx = context.Background() + autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) + configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + startManagers(GinkgoT(), mgr) + }) + + It("does not clean up runners on initial creation without an actionable revision", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient(fake.WithRemoveRunner(nil))), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-actionable-revision-initial", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + pendingRunner := newRunner("runner-pending-initial", ephemeralRunnerSet) + err = k8sClient.Create(ctx, pendingRunner) + Expect(err).NotTo(HaveOccurred()) + + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Consistently(func() error { + runner := new(v1alpha1.EphemeralRunner) + return k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: pendingRunner.Name}, runner) + }, time.Second, ephemeralRunnerSetTestInterval).Should(Succeed()) + + Consistently(func() int64 { + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return -1 + } + return updatedSet.Status.AppliedActionableRevision + }, time.Second, ephemeralRunnerSetTestInterval).Should(Equal(int64(0))) + }) + + It("deletes runner-a-idle, keeps runner-b-busy, and advances applied actionable revision 3 to 4", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient(fake.WithRemoveRunner(nil))), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-actionable-revision-success", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 3, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 3 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + idleRunner := newRunner("runner-a-idle", statusUpdated) + err = k8sClient.Create(ctx, idleRunner) + Expect(err).NotTo(HaveOccurred()) + + idleCurrent := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(idleRunner), idleCurrent) + Expect(err).NotTo(HaveOccurred()) + idleUpdated := idleCurrent.DeepCopy() + idleUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + idleUpdated.Status.RunnerID = 101 + err = k8sClient.Status().Patch(ctx, idleUpdated, client.MergeFrom(idleCurrent)) + Expect(err).NotTo(HaveOccurred()) + + busyRunner := newRunner("runner-b-busy", statusUpdated) + err = k8sClient.Create(ctx, busyRunner) + Expect(err).NotTo(HaveOccurred()) + + busyCurrent := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(busyRunner), busyCurrent) + Expect(err).NotTo(HaveOccurred()) + busyUpdated := busyCurrent.DeepCopy() + busyUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + busyUpdated.Status.RunnerID = 102 + busyUpdated.Status.JobID = "job-1" + busyUpdated.Status.WorkflowRunID = 9001 + err = k8sClient.Status().Patch(ctx, busyUpdated, client.MergeFrom(busyCurrent)) + Expect(err).NotTo(HaveOccurred()) + + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + specUpdated := current.DeepCopy() + specUpdated.Spec.ActionableRevision = 4 + err = k8sClient.Patch(ctx, specUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func() bool { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + runner := new(v1alpha1.EphemeralRunner) + return kerrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-a-idle"}, runner)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(BeTrue()) + + Consistently(func() error { + runner := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-b-busy"}, runner); err != nil { + return err + } + if runner.Status.RunnerID != 102 { + return fmt.Errorf("expected busy runner ID 102, got %d", runner.Status.RunnerID) + } + if !runner.HasJob() { + return fmt.Errorf("expected runner-b-busy to keep its assigned job") + } + return nil + }, time.Second, ephemeralRunnerSetTestInterval).Should(Succeed()) + + Eventually(func() int64 { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return 0 + } + return updatedSet.Status.AppliedActionableRevision + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Equal(int64(4))) + }) + + It("keeps applied actionable revision at 3 when cleanup fails", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient(fake.WithRemoveRunner(fmt.Errorf("remove failed")))), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-actionable-revision-error", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 3, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 3 + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + idleRunner := newRunner("runner-a-idle", statusUpdated) + err = k8sClient.Create(ctx, idleRunner) + Expect(err).NotTo(HaveOccurred()) + + idleCurrent := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(idleRunner), idleCurrent) + Expect(err).NotTo(HaveOccurred()) + idleUpdated := idleCurrent.DeepCopy() + idleUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + idleUpdated.Status.RunnerID = 101 + err = k8sClient.Status().Patch(ctx, idleUpdated, client.MergeFrom(idleCurrent)) + Expect(err).NotTo(HaveOccurred()) + + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + specUpdated := current.DeepCopy() + specUpdated.Spec.ActionableRevision = 4 + err = k8sClient.Patch(ctx, specUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + // The reconciler reads through the manager's cache, so retry until the + // bumped actionable revision is observed and cleanup is attempted. + Eventually(func() error { + _, err := controller.Reconcile(ctx, request) + return err + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(MatchError(ContainSubstring("remove failed"))) + + Consistently(func() int64 { + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return 0 + } + return updatedSet.Status.AppliedActionableRevision + }, time.Second, ephemeralRunnerSetTestInterval).Should(Equal(int64(3))) + }) + + It("deletes unregistered pending runner during actionable revision cleanup after restart with no cache", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), // fresh empty cache simulating restart + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient()), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-restart-no-cache", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 4, // spec has been bumped + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:updated"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 3 // status is behind + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + pendingRunner := newRunner("runner-restart-pending", statusUpdated) + err = k8sClient.Create(ctx, pendingRunner) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func(g Gomega) { + cachedSet := new(v1alpha1.EphemeralRunnerSet) + err := controller.Get(ctx, request.NamespacedName, cachedSet) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedSet.Status.AppliedActionableRevision).To(Equal(int64(3))) + + cachedRunner := new(v1alpha1.EphemeralRunner) + err = controller.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-restart-pending"}, cachedRunner) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedRunner.Status.RunnerID).To(BeZero()) + g.Expect(cachedRunner.Status.Phase).To(BeEmpty()) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // Reconcile with fresh cache (simulating restart). Actionable revision cleanup deletes pending runners. + Eventually(func() bool { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + runner := new(v1alpha1.EphemeralRunner) + return kerrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-restart-pending"}, runner)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(BeTrue()) + + // AppliedActionableRevision should advance after cleanup completes. + Eventually(func() int64 { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return 0 + } + return updatedSet.Status.AppliedActionableRevision + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Equal(int64(4))) + }) + + It("preserves AppliedActionableRevision during status-only phase updates", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient()), + )), + }, + } + + // Setup: Create ERS with an actionable revision + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-preserve-applied-revision", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 5, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + // Set AppliedActionableRevision to 5 + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 5 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + // Create a runner that will cause phase change (outdated runner) + ephemeralRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-runner-outdated", + Namespace: autoscalingNS.Name, + Labels: map[string]string{ + LabelKeyGitHubScaleSetName: ephemeralRunnerSet.Name, + LabelKeyGitHubScaleSetNamespace: ephemeralRunnerSet.Namespace, + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: ephemeralRunnerSet.Name, + UID: ephemeralRunnerSet.UID, + Controller: func(b bool) *bool { return &b }(true), + BlockOwnerDeletion: func(b bool) *bool { return &b }(true), + }, + }, + }, + Spec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:old"}}}}, + }, + } + err = k8sClient.Create(ctx, ephemeralRunner) + Expect(err).NotTo(HaveOccurred()) + + runnerStatusUpdated := ephemeralRunner.DeepCopy() + runnerStatusUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseOutdated + runnerStatusUpdated.Status.RunnerID = 123 + runnerStatusUpdated.Status.JobRequestID = 456 + err = k8sClient.Status().Patch(ctx, runnerStatusUpdated, client.MergeFrom(ephemeralRunner)) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func(g Gomega) { + cachedSet := new(v1alpha1.EphemeralRunnerSet) + err := controller.Get(ctx, request.NamespacedName, cachedSet) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedSet.Status.AppliedActionableRevision).To(Equal(int64(5))) + + cachedRunner := new(v1alpha1.EphemeralRunner) + err = controller.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "test-runner-outdated"}, cachedRunner) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedRunner.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerPhaseOutdated)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // Verify: Phase changed to Outdated, but AppliedActionableRevision preserved + Eventually(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, updatedSet) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(updatedSet.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerSetPhaseOutdated), "phase should change to Outdated") + g.Expect(updatedSet.Status.AppliedActionableRevision).To(Equal(int64(5)), "AppliedActionableRevision should be preserved") + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + }) +}) + var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func() { var ctx context.Context var mgr ctrl.Manager diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go new file mode 100644 index 0000000000..5d0274d3d5 --- /dev/null +++ b/controllers/actions.github.com/helpers.go @@ -0,0 +1,106 @@ +package actionsgithubcom + +import ( + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" +) + +var ( + _ = ephemeralRunnerSetActionableSpecChanged + _ = nextActionableRevision +) + +// ephemeralRunnerSetActionableSpecChanged reports whether the runner spec the +// EphemeralRunnerSet is running differs from the one derived from the +// AutoscalingRunnerSet, in a way that requires re-applying it to the runners. +// +// Semantic.DeepEqual is used rather than cmp.Equal or reflect.DeepEqual because +// it treats a nil slice/map as equal to an empty one. That matters here: 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 would report drift on every +// single reconcile, bumping ActionableRevision each time and making the +// EphemeralRunnerSet controller delete every idle and pending runner, forever. +// Semantic also knows how to compare resource.Quantity, and unlike cmp.Equal it +// cannot panic on types with unexported fields. +func ephemeralRunnerSetActionableSpecChanged(current, desired *v1alpha1.EphemeralRunnerSet) bool { + if current == nil || desired == nil { + return current != desired + } + + return !apiequality.Semantic.DeepEqual(current.Spec.EphemeralRunnerSpec, desired.Spec.EphemeralRunnerSpec) +} + +func nextActionableRevision(current *v1alpha1.EphemeralRunnerSet) int64 { + if current == nil { + return 1 + } + + if current.Spec.ActionableRevision > current.Status.AppliedActionableRevision { + return current.Spec.ActionableRevision + 1 + } + + return current.Status.AppliedActionableRevision + 1 +} + +// listenerPodSpecRequiresRecreation reports whether the live listener pod must be +// deleted and rebuilt to match the desired spec. +// +// DeepDerivative, not DeepEqual: the live pod carries a large number of fields +// the desired pod never sets, written by the API server and by admission +// (nodeName, dnsPolicy, schedulerName, securityContext, enableServiceLinks, +// the default tolerations, the kube-api-access-* projected volume and its mount, +// terminationMessagePath, imagePullPolicy, secret defaultMode, ...). DeepEqual +// would therefore report drift on every reconcile of every healthy listener and +// spin in a delete/create loop. It cannot be made to work by pre-populating the +// defaults either, since nodeName is scheduler-assigned and the access-token +// volume has a generated name. See TestListenerPodSpecRequiresRecreation. +// +// The cost of DeepDerivative is that it ignores empty values on the desired side, +// so a field being *removed* is invisible to it. For everything sourced from the +// user-facing template that is harmless: the AutoscalingRunnerSet controller +// compares the whole AutoscalingListener spec with cmp.Equal and deletes the +// listener outright, which takes the pod with it. Container ports are the +// exception, because they come from the --listener-metrics-addr controller flag +// rather than from any resource, so disabling metrics would otherwise leave the +// port on the pod forever. Comparing port length is enough: additions and value +// changes are already caught by DeepDerivative, only removal is blind. The +// contents are deliberately not compared, because the API server defaults +// protocol to TCP and that would reintroduce the delete/create loop. +func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool { + if current == nil || desired == nil { + return current != desired + } + + if listenerContainerPortsRemoved(current, desired) { + return true + } + + return !apiequality.Semantic.DeepDerivative(desired.Spec, current.Spec) +} + +func listenerContainerPortsRemoved(current, desired *corev1.Pod) bool { + for i := range desired.Spec.Containers { + desiredContainer := &desired.Spec.Containers[i] + currentContainer := findContainerByName(current.Spec.Containers, desiredContainer.Name) + if currentContainer == nil { + // A container the live pod does not have at all is drift that + // DeepDerivative already reports; nothing to decide here. + continue + } + if len(desiredContainer.Ports) < len(currentContainer.Ports) { + return true + } + } + return false +} + +func findContainerByName(containers []corev1.Container, name string) *corev1.Container { + for i := range containers { + if containers[i].Name == name { + return &containers[i] + } + } + return nil +} diff --git a/controllers/actions.github.com/helpers_bench_test.go b/controllers/actions.github.com/helpers_bench_test.go new file mode 100644 index 0000000000..50149257ef --- /dev/null +++ b/controllers/actions.github.com/helpers_bench_test.go @@ -0,0 +1,122 @@ +package actionsgithubcom + +import ( + "fmt" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +// benchmarkEphemeralRunnerSet builds a runner set roughly the size of a real +// ARC deployment: a runner container, a dind sidecar, an init container, +// resource limits, volumes and a dozen environment variables. +func benchmarkEphemeralRunnerSet() *v1alpha1.EphemeralRunnerSet { + env := make([]corev1.EnvVar, 0, 12) + for i := range 12 { + env = append(env, corev1.EnvVar{Name: fmt.Sprintf("VAR_%d", i), Value: fmt.Sprintf("value-%d", i)}) + } + q := resource.MustParse + + return &v1alpha1.EphemeralRunnerSet{ + Spec: v1alpha1.EphemeralRunnerSetSpec{ + Replicas: 10, + PatchID: 7, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/octo-org", + GitHubConfigSecret: "gh-config", + RunnerScaleSetID: 42, + PodTemplateSpec: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + ServiceAccountName: "runner-sa", + RestartPolicy: corev1.RestartPolicyNever, + NodeSelector: map[string]string{"kubernetes.io/os": "linux", "node.kubernetes.io/pool": "runners"}, + Tolerations: []corev1.Toleration{{ + Key: "dedicated", Operator: corev1.TolerationOpEqual, + Value: "runners", Effect: corev1.TaintEffectNoSchedule, + }}, + Volumes: []corev1.Volume{ + {Name: "work", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, + {Name: "dind-sock", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, + }, + InitContainers: []corev1.Container{{ + Name: "init-dind", Image: "docker:dind", + Command: []string{"cp"}, + Args: []string{"-r", "/usr/local/bin/.", "/dind"}, + VolumeMounts: []corev1.VolumeMount{{Name: "dind-sock", MountPath: "/dind"}}, + }}, + Containers: []corev1.Container{ + { + Name: "runner", + Image: "ghcr.io/actions/actions-runner:2.337.0", + Command: []string{"/home/runner/run.sh"}, + Env: env, + VolumeMounts: []corev1.VolumeMount{ + {Name: "work", MountPath: "/home/runner/_work"}, + {Name: "dind-sock", MountPath: "/var/run"}, + }, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceCPU: q("500m"), corev1.ResourceMemory: q("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceCPU: q("2"), corev1.ResourceMemory: q("4Gi")}, + }, + }, + { + Name: "dind", Image: "docker:dind", Env: env[:6], + VolumeMounts: []corev1.VolumeMount{{Name: "dind-sock", MountPath: "/var/run"}}, + }, + }, + }, + }, + }, + }, + } +} + +// BenchmarkEphemeralRunnerSetActionableSpecChanged measures the drift check that +// runs on every AutoscalingRunnerSet reconcile. Reconciles are driven by +// EphemeralRunnerSet status churn via Owns(), so this executes constantly and +// its allocation count feeds directly into controller GC pressure. +// +// For reference on the machine this was written on: Semantic.DeepEqual is around +// 65us/241 allocs, versus 332us/405 allocs for cmp.Equal. If this regresses by an +// order of magnitude, something switched the comparison back to a reflection +// heavy implementation. +func BenchmarkEphemeralRunnerSetActionableSpecChanged(b *testing.B) { + b.Run("no drift", func(b *testing.B) { + current, desired := benchmarkEphemeralRunnerSet(), benchmarkEphemeralRunnerSet() + b.ReportAllocs() + b.ResetTimer() + for range b.N { + if ephemeralRunnerSetActionableSpecChanged(current, desired) { + b.Fatal("expected no drift") + } + } + }) + + b.Run("drift", func(b *testing.B) { + current, desired := benchmarkEphemeralRunnerSet(), benchmarkEphemeralRunnerSet() + desired.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Image = "ghcr.io/actions/actions-runner:2.338.0" + b.ReportAllocs() + b.ResetTimer() + for range b.N { + if !ephemeralRunnerSetActionableSpecChanged(current, desired) { + b.Fatal("expected drift") + } + } + }) +} + +// BenchmarkListenerPodSpecRequiresRecreation measures the listener pod drift +// check, which also runs on every AutoscalingListener reconcile. +func BenchmarkListenerPodSpecRequiresRecreation(b *testing.B) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + b.ReportAllocs() + b.ResetTimer() + for range b.N { + if listenerPodSpecRequiresRecreation(live, desired) { + b.Fatal("expected no recreation") + } + } +} diff --git a/controllers/actions.github.com/helpers_drift_test.go b/controllers/actions.github.com/helpers_drift_test.go new file mode 100644 index 0000000000..71a9d91df1 --- /dev/null +++ b/controllers/actions.github.com/helpers_drift_test.go @@ -0,0 +1,147 @@ +package actionsgithubcom + +import ( + "encoding/json" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" +) + +// roundTripThroughAPIServer simulates what happens when the controller writes an +// EphemeralRunnerSet and then reads it back: the Go value is marshalled to JSON +// (dropping fields tagged omitempty, including explicitly-empty slices and maps) +// and decoded again. Fields that were empty-but-non-nil come back nil. +func roundTripThroughAPIServer(t *testing.T, spec v1alpha1.EphemeralRunnerSpec) v1alpha1.EphemeralRunnerSpec { + t.Helper() + raw, err := json.Marshal(spec) + require.NoError(t, err) + var out v1alpha1.EphemeralRunnerSpec + require.NoError(t, json.Unmarshal(raw, &out)) + return out +} + +// TestEphemeralRunnerSetActionableSpecChanged_EmptySliceRoundTrip is a +// regression test for a permanent drift loop. +// +// A user may legitimately write `env: []` (or an empty nodeSelector, tolerations, +// volumes, ...) in the AutoscalingRunnerSet template. Those fields carry +// `omitempty`, so when the controller writes the derived EphemeralRunnerSet the +// empty slice is dropped entirely and reads back as nil. If the drift check +// treats nil and empty as different it reports drift forever: every reconcile +// bumps ActionableRevision, which makes the EphemeralRunnerSet controller delete +// every idle and pending runner, permanently. +func TestEphemeralRunnerSetActionableSpecChanged_EmptySliceRoundTrip(t *testing.T) { + desiredSpec := v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: "gh-config", + RunnerScaleSetID: 42, + PodTemplateSpec: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "runner", + Image: "ghcr.io/actions/actions-runner:2.337.0", + // Explicitly empty, exactly as a user writing `env: []` would produce. + Env: []corev1.EnvVar{}, + }}, + NodeSelector: map[string]string{}, + Tolerations: []corev1.Toleration{}, + }, + }, + } + + desired := &v1alpha1.EphemeralRunnerSet{ + Spec: v1alpha1.EphemeralRunnerSetSpec{EphemeralRunnerSpec: desiredSpec}, + } + + // The live object is what the API server hands back after the controller + // persisted exactly this desired spec. + current := &v1alpha1.EphemeralRunnerSet{ + Spec: v1alpha1.EphemeralRunnerSetSpec{ + EphemeralRunnerSpec: roundTripThroughAPIServer(t, desiredSpec), + }, + } + + // Sanity check that the round trip really does produce the nil/empty split, + // otherwise this test would pass vacuously. + require.NotNil(t, desired.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Env) + require.Nil(t, current.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Env) + + assert.False(t, + ephemeralRunnerSetActionableSpecChanged(current, desired), + "an empty slice that was dropped by omitempty on write must not be reported as drift; "+ + "reporting drift here bumps ActionableRevision on every reconcile and deletes idle runners forever", + ) +} + +// TestEphemeralRunnerSetActionableSpecChanged_RealChangeStillDetected guards the +// opposite direction: relaxing nil-vs-empty must not blind us to genuine drift. +func TestEphemeralRunnerSetActionableSpecChanged_RealChangeStillDetected(t *testing.T) { + base := func() *v1alpha1.EphemeralRunnerSet { + return &v1alpha1.EphemeralRunnerSet{ + Spec: v1alpha1.EphemeralRunnerSetSpec{ + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + RunnerScaleSetID: 42, + PodTemplateSpec: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "runner", + Image: "ghcr.io/actions/actions-runner:2.337.0", + Env: []corev1.EnvVar{{Name: "A", Value: "1"}}, + }}, + }, + }, + }, + }, + } + } + + tests := map[string]func(*v1alpha1.EphemeralRunnerSet){ + "image changed": func(e *v1alpha1.EphemeralRunnerSet) { + e.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Image = "ghcr.io/actions/actions-runner:2.338.0" + }, + "env value changed": func(e *v1alpha1.EphemeralRunnerSet) { + e.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Env[0].Value = "2" + }, + "env var removed": func(e *v1alpha1.EphemeralRunnerSet) { + e.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Env = nil + }, + "env var added": func(e *v1alpha1.EphemeralRunnerSet) { + e.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Env = append( + e.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Env, + corev1.EnvVar{Name: "B", Value: "2"}, + ) + }, + "scale set id changed": func(e *v1alpha1.EphemeralRunnerSet) { + e.Spec.EphemeralRunnerSpec.RunnerScaleSetID = 43 + }, + "config url changed": func(e *v1alpha1.EphemeralRunnerSet) { + e.Spec.EphemeralRunnerSpec.GitHubConfigURL = "https://github.com/owner/other" + }, + "container removed": func(e *v1alpha1.EphemeralRunnerSet) { + e.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers = nil + }, + } + + for name, mutate := range tests { + t.Run(name, func(t *testing.T) { + current, desired := base(), base() + mutate(desired) + assert.True(t, ephemeralRunnerSetActionableSpecChanged(current, desired), + "genuine drift must still be detected") + }) + } + + t.Run("identical specs report no drift", func(t *testing.T) { + assert.False(t, ephemeralRunnerSetActionableSpecChanged(base(), base())) + }) + + t.Run("nil handling", func(t *testing.T) { + assert.False(t, ephemeralRunnerSetActionableSpecChanged(nil, nil)) + assert.True(t, ephemeralRunnerSetActionableSpecChanged(nil, base())) + assert.True(t, ephemeralRunnerSetActionableSpecChanged(base(), nil)) + }) +} diff --git a/controllers/actions.github.com/helpers_listener_test.go b/controllers/actions.github.com/helpers_listener_test.go new file mode 100644 index 0000000000..89384a03a1 --- /dev/null +++ b/controllers/actions.github.com/helpers_listener_test.go @@ -0,0 +1,277 @@ +package actionsgithubcom + +import ( + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" +) + +// desiredListenerPod mirrors the shape produced by newScaleSetListenerPod: only +// the handful of fields the builder actually sets. +func desiredListenerPod() *corev1.Pod { + grace := int64(60) + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "listener", Namespace: "arc-systems"}, + Spec: corev1.PodSpec{ + ServiceAccountName: "listener", + NodeSelector: map[string]string{"kubernetes.io/os": "linux"}, + Containers: []corev1.Container{{ + Name: autoscalingListenerContainerName, + Image: "ghcr.io/actions/arc:0.1.0", + Command: []string{"/ghalistener"}, + Env: []corev1.EnvVar{ + {Name: "LISTENER_CONFIG_PATH", Value: "/etc/gha-listener/config.json"}, + }, + Ports: []corev1.ContainerPort{{ContainerPort: 8080}}, + VolumeMounts: []corev1.VolumeMount{ + {Name: "listener-config", MountPath: "/etc/gha-listener", ReadOnly: true}, + }, + }}, + Volumes: []corev1.Volume{{ + Name: "listener-config", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{SecretName: "listener-config"}, + }, + }}, + RestartPolicy: corev1.RestartPolicyNever, + TerminationGracePeriodSeconds: &grace, + }, + } +} + +// livePodFromDesired approximates what the API server and admission return after +// the desired pod is created: every field the controller set, plus the defaults +// and injections it did not. +func livePodFromDesired(desired *corev1.Pod) *corev1.Pod { + live := desired.DeepCopy() + defaultMode := int32(420) + enableServiceLinks := true + tolerationSeconds := int64(300) + + live.Spec.NodeName = "node-1" + live.Spec.DNSPolicy = corev1.DNSClusterFirst + live.Spec.SchedulerName = "default-scheduler" + live.Spec.SecurityContext = &corev1.PodSecurityContext{} + live.Spec.DeprecatedServiceAccount = desired.Spec.ServiceAccountName + live.Spec.EnableServiceLinks = &enableServiceLinks + live.Spec.PreemptionPolicy = ptr.To(corev1.PreemptLowerPriority) + live.Spec.Priority = ptr.To(int32(0)) + live.Spec.Tolerations = []corev1.Toleration{ + {Key: "node.kubernetes.io/not-ready", Operator: corev1.TolerationOpExists, + Effect: corev1.TaintEffectNoExecute, TolerationSeconds: &tolerationSeconds}, + {Key: "node.kubernetes.io/unreachable", Operator: corev1.TolerationOpExists, + Effect: corev1.TaintEffectNoExecute, TolerationSeconds: &tolerationSeconds}, + } + live.Spec.Volumes[0].Secret.DefaultMode = &defaultMode + live.Spec.Volumes = append(live.Spec.Volumes, corev1.Volume{ + Name: "kube-api-access-x7f2k", + VolumeSource: corev1.VolumeSource{ + Projected: &corev1.ProjectedVolumeSource{DefaultMode: &defaultMode}, + }, + }) + live.Spec.Containers[0].TerminationMessagePath = corev1.TerminationMessagePathDefault + live.Spec.Containers[0].TerminationMessagePolicy = corev1.TerminationMessageReadFile + live.Spec.Containers[0].ImagePullPolicy = corev1.PullIfNotPresent + live.Spec.Containers[0].Ports[0].Protocol = corev1.ProtocolTCP + live.Spec.Containers[0].VolumeMounts = append(live.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{ + Name: "kube-api-access-x7f2k", MountPath: "/var/run/secrets/kubernetes.io/serviceaccount", ReadOnly: true, + }) + return live +} + +// TestListenerPodSpecRequiresRecreation_SteadyState is the guard rail against +// swapping DeepDerivative for DeepEqual. +// +// A healthy listener pod carries many API-server defaults and admission +// injections that the desired pod never sets. If this comparison ever becomes +// strict, the controller deletes and recreates the pod on every reconcile, the +// listener never lives long enough to poll jobs, and every scale set in the +// cluster stops functioning. This test must keep passing. +func TestListenerPodSpecRequiresRecreation_SteadyState(t *testing.T) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + + assert.False(t, listenerPodSpecRequiresRecreation(live, desired), + "a healthy pod carrying only API-server defaults must never be recreated; "+ + "if this fails, the comparison became too strict and will spin in a delete/create loop") +} + +func TestListenerPodSpecRequiresRecreation(t *testing.T) { + tests := map[string]struct { + mutateDesired func(*corev1.Pod) + want bool + why string + }{ + "identical": { + mutateDesired: func(*corev1.Pod) {}, + want: false, + why: "no drift", + }, + "image changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Image = "ghcr.io/actions/arc:0.2.0" }, + want: true, + why: "an upgraded listener image must roll the pod", + }, + "metrics port removed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Ports = nil }, + want: true, + why: "disabling --listener-metrics-addr must remove the port from the running pod", + }, + "metrics port value changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Ports[0].ContainerPort = 9090 }, + want: true, + why: "changing the metrics port must roll the pod", + }, + "metrics port added": { + mutateDesired: func(p *corev1.Pod) { + p.Spec.Containers[0].Ports = append(p.Spec.Containers[0].Ports, + corev1.ContainerPort{ContainerPort: 9090}) + }, + want: true, + why: "an added port must roll the pod", + }, + "env var added": { + mutateDesired: func(p *corev1.Pod) { + p.Spec.Containers[0].Env = append(p.Spec.Containers[0].Env, + corev1.EnvVar{Name: "HTTP_PROXY", Value: "http://proxy:8080"}) + }, + want: true, + why: "proxy configuration must roll the pod", + }, + "env value changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Containers[0].Env[0].Value = "/etc/other/config.json" }, + want: true, + why: "a changed env value must roll the pod", + }, + "config secret name changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.Volumes[0].Secret.SecretName = "other-config" }, + want: true, + why: "pointing at a different config secret must roll the pod", + }, + "service account changed": { + mutateDesired: func(p *corev1.Pod) { p.Spec.ServiceAccountName = "other-sa" }, + want: true, + why: "a changed service account must roll the pod", + }, + "nodeSelector key added": { + mutateDesired: func(p *corev1.Pod) { p.Spec.NodeSelector["pool"] = "listeners" }, + want: true, + why: "an added nodeSelector key must roll the pod", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + tc.mutateDesired(desired) + + assert.Equal(t, tc.want, listenerPodSpecRequiresRecreation(live, desired), tc.why) + }) + } + + t.Run("nil handling", func(t *testing.T) { + desired := desiredListenerPod() + assert.False(t, listenerPodSpecRequiresRecreation(nil, nil)) + assert.True(t, listenerPodSpecRequiresRecreation(nil, desired)) + assert.True(t, listenerPodSpecRequiresRecreation(desired, nil)) + }) +} + +// TestListenerPodSpecRequiresRecreation_KnownDeepDerivativeLimits documents, +// rather than asserts away, the removals DeepDerivative cannot see. These are +// all sourced from the user-facing listener template, so they are handled +// upstream: the AutoscalingRunnerSet controller compares the whole +// AutoscalingListener spec with cmp.Equal and deletes the listener, which +// deletes the pod. If that upstream behaviour ever changes to a derivative +// comparison, these become real bugs. +func TestListenerPodSpecRequiresRecreation_KnownDeepDerivativeLimits(t *testing.T) { + tests := map[string]func(*corev1.Pod){ + "all tolerations removed": func(p *corev1.Pod) { p.Spec.Tolerations = nil }, + "trailing volume removed": func(p *corev1.Pod) { + p.Spec.Volumes = p.Spec.Volumes[:len(p.Spec.Volumes)-1] + }, + "nodeSelector emptied": func(p *corev1.Pod) { p.Spec.NodeSelector = nil }, + "grace period removed": func(p *corev1.Pod) { p.Spec.TerminationGracePeriodSeconds = nil }, + } + + for name, mutate := range tests { + t.Run(name, func(t *testing.T) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + mutate(desired) + + assert.False(t, listenerPodSpecRequiresRecreation(live, desired), + "documented DeepDerivative limitation: removal is invisible here and is "+ + "instead handled by AutoscalingListener re-creation upstream") + }) + } +} + +// TestListenerPodSpecRequiresRecreation_MetricsToggleUsesRealBuilder exercises +// the drift check against genuine newScaleSetListenerPod output rather than a +// hand-written fixture, closing the gap between the fixture above and reality. +// +// metricsConfig is the one pod-spec input that does not come from any resource: +// it is derived from the --listener-metrics-addr controller flag. Because +// nothing in the AutoscalingListener spec changes when an operator disables +// metrics, the AutoscalingListener is not re-created and this comparison is the +// only thing that can remove the stale port from the running pod. +func TestListenerPodSpecRequiresRecreation_MetricsToggleUsesRealBuilder(t *testing.T) { + autoscalingRunnerSet := v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-scale-set", + Namespace: "test-ns", + Labels: map[string]string{ + LabelKeyKubernetesPartOf: labelValueKubernetesPartOf, + LabelKeyKubernetesVersion: "0.2.0", + }, + Annotations: map[string]string{ + runnerScaleSetIDAnnotationKey: "1", + AnnotationKeyGitHubRunnerGroupName: "test-group", + AnnotationKeyGitHubRunnerScaleSetName: "test-scale-set", + }, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{GitHubConfigUrl: "https://github.com/org/repo"}, + } + + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} + ephemeralRunnerSet, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet) + require.NoError(t, err) + listener, err := b.newAutoscalingListener(&autoscalingRunnerSet, ephemeralRunnerSet, autoscalingRunnerSet.Namespace, "test:latest", nil) + require.NoError(t, err) + sa, err := b.newScaleSetListenerServiceAccount(listener) + require.NoError(t, err) + role := b.newScaleSetListenerRole(listener) + roleBinding := b.newScaleSetListenerRoleBinding(listener, role, sa) + + build := func(metrics *listenerMetricsServerConfig) *corev1.Pod { + // The cache keys on the listener object, not on metricsConfig, so it must + // be bypassed to build both variants. + b.ResourceCache.listenerPod.Delete(listener) + pod, err := b.newScaleSetListenerPod(listener, &corev1.Secret{}, sa, role, roleBinding, metrics) + require.NoError(t, err) + return pod + } + + withMetrics := build(&listenerMetricsServerConfig{addr: ":8080", endpoint: "/metrics"}) + withoutMetrics := build(nil) + + require.NotEmpty(t, withMetrics.Spec.Containers[0].Ports, + "builder should expose a container port when metrics are enabled") + require.Empty(t, withoutMetrics.Spec.Containers[0].Ports, + "builder should expose no container port when metrics are disabled") + + assert.True(t, listenerPodSpecRequiresRecreation(withMetrics, withoutMetrics), + "disabling --listener-metrics-addr must recreate a pod that still has the metrics port") + assert.True(t, listenerPodSpecRequiresRecreation(withoutMetrics, withMetrics), + "enabling --listener-metrics-addr must recreate a pod that has no metrics port") + assert.False(t, listenerPodSpecRequiresRecreation(withMetrics, withMetrics), + "an unchanged metrics configuration must not recreate the pod") +} diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index a75d414c57..c660a94f4f 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -46,15 +46,6 @@ var commonLabelKeys = [...]string{ LabelKeyGitHubRepository, } -// annotationKeyIntegrityHash is used as a hash of the important fields -// of each resource to determine if more drastic action should be taken. -// -// For example, annotations/labels are not something that should modify -// the behavior of a resource, while the change in spec is. Therefore, -// the spec hash should contain the spec fields in order to determine -// modifications. -const annotationKeyIntegrityHash = "actions.github.com/integrity-hash" - const labelValueKubernetesPartOf = "gha-runner-scale-set" var ( @@ -184,9 +175,7 @@ func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1. return nil, fmt.Errorf("failed to apply GitHub URL labels: %v", err) } - annotations := map[string]string{ - annotationKeyIntegrityHash: spec.Hash(), - } + var annotations map[string]string if autoscalingRunnerSet.Spec.AutoscalingListenerMetadata != nil { labels = b.filterAndMergeLabels(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Labels, labels) @@ -320,8 +309,6 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha }, } - desiredSecret.Annotations[annotationKeyIntegrityHash] = scaleSetListenerConfigIntegrityHash(desiredSecret) - if err := b.setControllerReference(autoscalingListener, desiredSecret); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener config secret: %w", err) } @@ -329,18 +316,6 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha return desiredSecret, nil } -func scaleSetListenerConfigIntegrityHash(secret *corev1.Secret) string { - type data struct { - Data map[string][]byte `json:"data,omitempty"` - } - - d := data{ - Data: secret.Data, - } - - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newScaleSetListenerPod( autoscalingListener *v1alpha1.AutoscalingListener, podConfig *corev1.Secret, @@ -478,16 +453,6 @@ func (b *ResourceBuilder) newScaleSetListenerPod( Spec: podSpec, } - newRunnerScaleSetListenerPod.Annotations[annotationKeyIntegrityHash] = scaleSetListenerPodIntegrity( - newRunnerScaleSetListenerPod, - autoscalingListener, - podConfig, - serviceAccount, - role, - roleBinding, - metricsConfig, - ) - if err := b.setControllerReference(autoscalingListener, newRunnerScaleSetListenerPod); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener pod: %w", err) } @@ -500,38 +465,6 @@ func (b *ResourceBuilder) newScaleSetListenerPod( return newRunnerScaleSetListenerPod, nil } -func scaleSetListenerPodIntegrity( - pod *corev1.Pod, - autoscalingListener *v1alpha1.AutoscalingListener, - podConfig *corev1.Secret, - serviceAccount *corev1.ServiceAccount, - role *rbacv1.Role, - roleBinding *rbacv1.RoleBinding, - metricsConfig *listenerMetricsServerConfig, -) string { - type data struct { - ListenerPodSpec *corev1.PodSpec `json:"listenerPodSpec,omitempty"` - AutoscalingListenerIntegrityHash string `json:"autoscalingListenerIntegrityHash"` - ConfigSecretIntegrityHash string `json:"configSecretIntegrityHash"` - ServiceAccountIntegrityHash string `json:"serviceAccountIntegrityHash"` - RoleIntegrityHash string `json:"roleIntegrityHash"` - RoleBindingIntegrityHash string `json:"roleBindingIntegrityHash"` - MetricsConfig *listenerMetricsServerConfig `json:"metricsConfig,omitempty"` - } - - d := data{ - ListenerPodSpec: &pod.Spec, - AutoscalingListenerIntegrityHash: autoscalingListener.Annotations[annotationKeyIntegrityHash], - ConfigSecretIntegrityHash: podConfig.Annotations[annotationKeyIntegrityHash], - ServiceAccountIntegrityHash: serviceAccount.Annotations[annotationKeyIntegrityHash], - RoleIntegrityHash: role.Annotations[annotationKeyIntegrityHash], - RoleBindingIntegrityHash: roleBinding.Annotations[annotationKeyIntegrityHash], - MetricsConfig: metricsConfig, - } - - return hash.ComputeTemplateHash(&d) -} - func mergeListenerPodWithTemplate(pod *corev1.Pod, tmpl *corev1.PodTemplateSpec) { if pod.Annotations == nil { pod.Annotations = make(map[string]string) @@ -681,8 +614,6 @@ func (b *ResourceBuilder) newScaleSetListenerServiceAccount(autoscalingListener base.Annotations = b.mergeAnnotations(autoscalingListener.Spec.ServiceAccountMetadata.Annotations, base.Annotations) } - base.Annotations[annotationKeyIntegrityHash] = scaleSetListenerServiceAccountIntegrityHash(base) - if err := b.setControllerReference(autoscalingListener, base); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener service account: %w", err) } @@ -691,22 +622,6 @@ func (b *ResourceBuilder) newScaleSetListenerServiceAccount(autoscalingListener return base, nil } -func scaleSetListenerServiceAccountIntegrityHash(sa *corev1.ServiceAccount) string { - type data struct { - Secrets []corev1.ObjectReference `json:"secrets"` - ImagePullSecrets []corev1.LocalObjectReference `json:"imagePullSecrets"` - AutomountServiceAccountToken *bool `json:"automountServiceAccountToken"` - } - - d := data{ - Secrets: sa.Secrets, - ImagePullSecrets: sa.ImagePullSecrets, - AutomountServiceAccountToken: sa.AutomountServiceAccountToken, - } - - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1.AutoscalingListener) *rbacv1.Role { cacheKeyObject := &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ @@ -745,24 +660,11 @@ func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1. Rules: rulesForListenerRole([]string{autoscalingListener.Spec.EphemeralRunnerSetName}), } - newRole.Annotations[annotationKeyIntegrityHash] = scaleSetRoleIntegrityHash(newRole) b.ResourceCache.listenerRole.Upsert(autoscalingListener, newRole) return newRole } -func scaleSetRoleIntegrityHash(role *rbacv1.Role) string { - type data struct { - Rules []rbacv1.PolicyRule `json:"rules"` - } - - d := data{ - Rules: role.Rules, - } - - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1alpha1.AutoscalingListener, listenerRole *rbacv1.Role, serviceAccount *corev1.ServiceAccount) *rbacv1.RoleBinding { cacheKeyObject := &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ @@ -815,26 +717,11 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 Subjects: subjects, } - newRoleBinding.Annotations[annotationKeyIntegrityHash] = scaleSetListenerRoleBindingIntegrityHash(newRoleBinding) b.ResourceCache.listenerRoleBinding.Upsert(autoscalingListener, newRoleBinding, listenerRole, serviceAccount) return newRoleBinding } -func scaleSetListenerRoleBindingIntegrityHash(rb *rbacv1.RoleBinding) string { - type data struct { - RoleRef rbacv1.RoleRef `json:"roleRef"` - Subjects []rbacv1.Subject `json:"subjects"` - } - - d := data{ - RoleRef: rb.RoleRef, - Subjects: rb.Subjects, - } - - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet) (*v1alpha1.EphemeralRunnerSet, error) { runnerScaleSetID, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey]) if err != nil { @@ -902,8 +789,6 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A Spec: spec, } - newEphemeralRunnerSet.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerSetIntegrityHash(newEphemeralRunnerSet) - if err := b.setControllerReference(autoscalingRunnerSet, newEphemeralRunnerSet); err != nil { return nil, fmt.Errorf("failed to set controller reference for ephemeral runner set: %w", err) } @@ -912,17 +797,6 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A return newEphemeralRunnerSet, nil } -func ephemeralRunnerSetIntegrityHash(ers *v1alpha1.EphemeralRunnerSet) string { - type data struct { - EphemeralRunnerSpec v1alpha1.EphemeralRunnerSpec `json:"ephemeralRunnerSpec"` - } - - d := data{ - EphemeralRunnerSpec: ers.Spec.EphemeralRunnerSpec, - } - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newAutoscalingListenerProxySecret(autoscalingListener *v1alpha1.AutoscalingListener, data map[string][]byte) (*corev1.Secret, error) { newProxySecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -937,8 +811,6 @@ func (b *ResourceBuilder) newAutoscalingListenerProxySecret(autoscalingListener Data: data, } - newProxySecret.Annotations[annotationKeyIntegrityHash] = autoscalingListenerProxySecretIntegrityHash(newProxySecret) - if err := b.setControllerReference(autoscalingListener, newProxySecret); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener proxy secret: %w", err) } @@ -946,18 +818,6 @@ func (b *ResourceBuilder) newAutoscalingListenerProxySecret(autoscalingListener return newProxySecret, nil } -func autoscalingListenerProxySecretIntegrityHash(secret *corev1.Secret) string { - type data struct { - Data map[string][]byte `json:"data"` - } - - d := data{ - Data: secret.Data, - } - - return hash.ComputeTemplateHash(&d) -} - func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet) (*v1alpha1.EphemeralRunner, error) { labels := make(map[string]string, len(ephemeralRunnerSet.Labels)) maps.Copy(labels, ephemeralRunnerSet.Labels) @@ -1104,8 +964,6 @@ func (b *ResourceBuilder) newEphemeralRunnerSetProxySecret(ephemeralRunnerSet *v Data: data, } - runnerPodProxySecret.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerSetProxySecretZIdentityHash(runnerPodProxySecret) - if err := b.setControllerReference(ephemeralRunnerSet, runnerPodProxySecret); err != nil { return nil, fmt.Errorf("failed to set controller reference for ephemeral runner set proxy secret: %w", err) } @@ -1113,18 +971,6 @@ func (b *ResourceBuilder) newEphemeralRunnerSetProxySecret(ephemeralRunnerSet *v return runnerPodProxySecret, nil } -func ephemeralRunnerSetProxySecretZIdentityHash(secret *corev1.Secret) string { - type data struct { - Data map[string][]byte `json:"data"` - } - - d := data{ - Data: secret.Data, - } - - return hash.ComputeTemplateHash(&d) -} - func scaleSetListenerConfigName(autoscalingListener *v1alpha1.AutoscalingListener) string { return autoscalingListener.Name + "-config" } diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index 6097308d71..9e91790d6c 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -115,7 +115,7 @@ func TestMetadataPropagation(t *testing.T) { assert.Equal(t, labelValueKubernetesPartOf, ephemeralRunnerSet.Labels[LabelKeyKubernetesPartOf]) assert.Equal(t, "runner-set", ephemeralRunnerSet.Labels[LabelKeyKubernetesComponent]) assert.Equal(t, autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], ephemeralRunnerSet.Labels[LabelKeyKubernetesVersion]) - assert.NotEmpty(t, ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash]) + assert.NotContains(t, ephemeralRunnerSet.Annotations, "actions.github.com/integrity-hash") assert.Equal(t, autoscalingRunnerSet.Name, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetName]) assert.Equal(t, autoscalingRunnerSet.Namespace, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetNamespace]) assert.Equal(t, "", ephemeralRunnerSet.Labels[LabelKeyGitHubEnterprise]) @@ -132,7 +132,7 @@ func TestMetadataPropagation(t *testing.T) { assert.Equal(t, labelValueKubernetesPartOf, listener.Labels[LabelKeyKubernetesPartOf]) assert.Equal(t, "runner-scale-set-listener", listener.Labels[LabelKeyKubernetesComponent]) assert.Equal(t, autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], listener.Labels[LabelKeyKubernetesVersion]) - assert.NotEmpty(t, ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash]) + assert.NotContains(t, listener.Annotations, "actions.github.com/integrity-hash") assert.Equal(t, autoscalingRunnerSet.Name, listener.Labels[LabelKeyGitHubScaleSetName]) assert.Equal(t, autoscalingRunnerSet.Namespace, listener.Labels[LabelKeyGitHubScaleSetNamespace]) assert.Equal(t, "", listener.Labels[LabelKeyGitHubEnterprise]) @@ -206,7 +206,7 @@ func TestMetadataPropagation(t *testing.T) { } } -func TestEphemeralRunnerSetProxySecretZIdentityHash(t *testing.T) { +func TestEphemeralRunnerSetProxySecretMetadata(t *testing.T) { ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ ObjectMeta: metav1.ObjectMeta{ Name: "test-scale-set", @@ -224,13 +224,11 @@ func TestEphemeralRunnerSetProxySecretZIdentityHash(t *testing.T) { }) require.NoError(t, err) - actualHash := proxySecret.Annotations[annotationKeyIntegrityHash] - assert.NotEmpty(t, actualHash) - assert.Equal(t, ephemeralRunnerSetProxySecretZIdentityHash(proxySecret), actualHash) - - changedProxySecret := proxySecret.DeepCopy() - changedProxySecret.Data["http_proxy"] = []byte("http://updated-proxy.example.com") - assert.NotEqual(t, actualHash, ephemeralRunnerSetProxySecretZIdentityHash(changedProxySecret)) + assert.Equal(t, proxyEphemeralRunnerSetSecretName(ephemeralRunnerSet), proxySecret.Name) + assert.Equal(t, ephemeralRunnerSet.Namespace, proxySecret.Namespace) + assert.Equal(t, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetName], proxySecret.Labels[LabelKeyGitHubScaleSetName]) + assert.Equal(t, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetNamespace], proxySecret.Labels[LabelKeyGitHubScaleSetNamespace]) + assert.NotContains(t, proxySecret.Annotations, "actions.github.com/integrity-hash") } func TestGitHubURLTrimLabelValues(t *testing.T) { @@ -318,7 +316,6 @@ func TestOwnershipRelationships(t *testing.T) { runnerScaleSetIDAnnotationKey: "1", AnnotationKeyGitHubRunnerGroupName: "test-group", AnnotationKeyGitHubRunnerScaleSetName: "test-scale-set", - annotationKeyIntegrityHash: "test-hash", }, }, Spec: v1alpha1.AutoscalingRunnerSetSpec{ diff --git a/controllers/actions.github.com/resourcecache.go b/controllers/actions.github.com/resourcecache.go index 2a4b1c1035..d6122914e0 100644 --- a/controllers/actions.github.com/resourcecache.go +++ b/controllers/actions.github.com/resourcecache.go @@ -246,9 +246,6 @@ func (k resourceCacheDependencyKey) Equal(other resourceCacheDependencyKey) bool func newResourceCacheObjectRef(object client.Object) ResourceCacheObjectRef { resourceVersion := object.GetResourceVersion() - if resourceVersion == "" { - resourceVersion = object.GetAnnotations()[annotationKeyIntegrityHash] - } if resourceVersion == "" { resourceVersion = hash.ComputeTemplateHash(object) } diff --git a/controllers/actions.github.com/resourcecache_test.go b/controllers/actions.github.com/resourcecache_test.go index 608ed9d95d..e187ac5e0d 100644 --- a/controllers/actions.github.com/resourcecache_test.go +++ b/controllers/actions.github.com/resourcecache_test.go @@ -187,9 +187,6 @@ func TestResourceBuilderCachesListenerPodDependencies(t *testing.T) { Name: "listener", Namespace: "controller-ns", UID: "listener-uid", - Annotations: map[string]string{ - annotationKeyIntegrityHash: "listener-hash", - }, }, Spec: v1alpha1.AutoscalingListenerSpec{ Image: "listener:latest", @@ -204,9 +201,6 @@ func TestResourceBuilderCachesListenerPodDependencies(t *testing.T) { Namespace: "controller-ns", UID: "config-secret-uid", ResourceVersion: "11", - Annotations: map[string]string{ - annotationKeyIntegrityHash: "config-hash", - }, }, } serviceAccount := &corev1.ServiceAccount{ @@ -215,9 +209,6 @@ func TestResourceBuilderCachesListenerPodDependencies(t *testing.T) { Namespace: "controller-ns", UID: "service-account-uid", ResourceVersion: "12", - Annotations: map[string]string{ - annotationKeyIntegrityHash: "service-account-hash", - }, }, } role := &rbacv1.Role{ @@ -226,9 +217,6 @@ func TestResourceBuilderCachesListenerPodDependencies(t *testing.T) { Namespace: "scale-set-ns", UID: "role-uid", ResourceVersion: "13", - Annotations: map[string]string{ - annotationKeyIntegrityHash: "role-hash", - }, }, } roleBinding := &rbacv1.RoleBinding{ @@ -237,9 +225,6 @@ func TestResourceBuilderCachesListenerPodDependencies(t *testing.T) { Namespace: "scale-set-ns", UID: "role-binding-uid", ResourceVersion: "14", - Annotations: map[string]string{ - annotationKeyIntegrityHash: "role-binding-hash", - }, }, } From 95179fbf54a3543bff9a92086f6d3fe1678026f9 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Tue, 8 Sep 2026 13:17:19 +0200 Subject: [PATCH 2/5] Remove dead blank-identifier block in helpers.go 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> --- controllers/actions.github.com/helpers.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go index 5d0274d3d5..a8c06fffc0 100644 --- a/controllers/actions.github.com/helpers.go +++ b/controllers/actions.github.com/helpers.go @@ -6,11 +6,6 @@ import ( apiequality "k8s.io/apimachinery/pkg/api/equality" ) -var ( - _ = ephemeralRunnerSetActionableSpecChanged - _ = nextActionableRevision -) - // ephemeralRunnerSetActionableSpecChanged reports whether the runner spec the // EphemeralRunnerSet is running differs from the one derived from the // AutoscalingRunnerSet, in a way that requires re-applying it to the runners. From 2aef0aeedd9c091337c67e4436c6c38c544026fd Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Tue, 8 Sep 2026 13:25:48 +0200 Subject: [PATCH 3/5] Remove dead spec hash helpers 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> --- .../v1alpha1/autoscalinglistener_types.go | 5 --- .../v1alpha1/autoscalingrunnerset_types.go | 43 ------------------- .../v1alpha1/ephemeralrunner_types.go | 5 --- .../v1alpha1/ephemeralrunnerset_types.go | 5 --- 4 files changed, 58 deletions(-) diff --git a/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go b/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go index 4556c45567..4122bb2121 100644 --- a/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go @@ -17,7 +17,6 @@ limitations under the License. package v1alpha1 import ( - "github.com/actions/actions-runner-controller/hash" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -84,10 +83,6 @@ type AutoscalingListenerSpec struct { RoleBindingMetadata *ResourceMeta `json:"roleBindingMetadata,omitempty"` } -func (s *AutoscalingListenerSpec) Hash() string { - return hash.ComputeTemplateHash(s) -} - // AutoscalingListenerStatus defines the observed state of AutoscalingListener type AutoscalingListenerStatus struct{} diff --git a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go index b383c2c49d..244d9b555e 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" @@ -335,26 +334,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 } @@ -382,28 +361,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/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go index c97b5595db..4c4a2acefd 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go @@ -17,7 +17,6 @@ limitations under the License. package v1alpha1 import ( - "github.com/actions/actions-runner-controller/hash" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -133,10 +132,6 @@ type EphemeralRunnerSpec struct { corev1.PodTemplateSpec `json:",inline"` } -func (s *EphemeralRunnerSpec) Hash() string { - return hash.ComputeTemplateHash(s) -} - // EphemeralRunnerStatus defines the observed state of EphemeralRunner type EphemeralRunnerStatus struct { // Turns true only if the runner is online. diff --git a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go index 22ed2dd9fe..0e9a398b59 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go @@ -87,11 +87,6 @@ type EphemeralRunnerSet struct { Status EphemeralRunnerSetStatus `json:"status,omitempty"` } -// EphemeralRunnerSpecHash computes the hash value of the EphemeralRunnerSpec and returns it as a string. -func (ers *EphemeralRunnerSet) EphemeralRunnerSpecHash() string { - return ers.Spec.EphemeralRunnerSpec.Hash() -} - func (ers *EphemeralRunnerSet) GitHubConfigSecret() string { return ers.Spec.EphemeralRunnerSpec.GitHubConfigSecret } From 41d05325c2fad12b3e24be595eb05ef1cb6397bf Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Tue, 8 Sep 2026 17:27:39 +0200 Subject: [PATCH 4/5] Make the Outdated phase revision-aware 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> --- .../autoscalingrunnerset_controller.go | 2 +- controllers/actions.github.com/constants.go | 6 + .../ephemeralrunnerset_controller.go | 93 +++++++- .../ephemeralrunnerset_controller_test.go | 109 ++++++++- controllers/actions.github.com/helpers.go | 24 ++ .../helpers_outdated_test.go | 217 ++++++++++++++++++ .../actions.github.com/resourcebuilder.go | 3 +- 7 files changed, 439 insertions(+), 15 deletions(-) create mode 100644 controllers/actions.github.com/helpers_outdated_test.go diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index d0af088b7f..402da77b12 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -243,7 +243,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl case err != nil: log.Error(err, "Failed to get ephemeral runner") return ctrl.Result{}, err - case ephemeralRunnerSet.Status.Phase == v1alpha1.EphemeralRunnerSetPhaseOutdated && autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseRunning: + case ephemeralRunnerSetOutdatedForAppliedRevision(&ephemeralRunnerSet) && autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseRunning: // Runners are outdated. We need to stop the listener so it stops getting new jobs. log.Info("Ephemeral runner set is outdated. Cleaning up resources for the outdated runner set") done, err := r.cleanupListener(ctx, &autoscalingRunnerSet, log) diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index 588c95beb7..3c523e9d0e 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -50,6 +50,12 @@ const ( AnnotationKeyGitHubRunnerGroupName = "actions.github.com/runner-group-name" AnnotationKeyGitHubRunnerScaleSetName = "actions.github.com/runner-scale-set-name" AnnotationKeyPatchID = "actions.github.com/patch-id" + // AnnotationKeyActionableRevision records the EphemeralRunnerSet + // Spec.ActionableRevision that was in effect when the runner was created. It + // lets the set tell apart a runner that reported Outdated against the current + // runner spec from one that reported it against a spec that has since been + // updated. + AnnotationKeyActionableRevision = "actions.github.com/actionable-revision" ) // Labels applied to listener roles diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index d979813732..45303138f5 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -187,11 +187,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } - ephemeralRunnersByState := newEphemeralRunnersByStates(&ephemeralRunnerList) + ephemeralRunnersByState := newEphemeralRunnersByStates(&ephemeralRunnerList, ephemeralRunnerSet.Status.AppliedActionableRevision) log.Info( "Ephemeral runner counts", "outdated", len(ephemeralRunnersByState.outdated), + "staleOutdated", len(ephemeralRunnersByState.staleOutdated), "pending", len(ephemeralRunnersByState.pending), "running", len(ephemeralRunnersByState.running), "finished", len(ephemeralRunnersByState.finished), @@ -199,10 +200,27 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R "deleting", len(ephemeralRunnersByState.deleting), ) + // Runners that reported Outdated against a runner spec that has since been + // replaced are not evidence about the current spec. Drop them so the scaling + // logic below replaces them with runners built from the current spec, instead + // of letting them hold the set in the Outdated phase forever. + if len(ephemeralRunnersByState.staleOutdated) > 0 { + log.Info( + "Deleting outdated ephemeral runners created before the last spec update so they can be replaced", + "count", len(ephemeralRunnersByState.staleOutdated), + "appliedActionableRevision", ephemeralRunnerSet.Status.AppliedActionableRevision, + ) + if err := r.deleteTerminatedEphemeralRunners(ctx, ephemeralRunnersByState.staleOutdated, log); err != nil { + log.Error(err, "failed to delete stale outdated ephemeral runners") + return ctrl.Result{}, err + } + return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) + } + total := ephemeralRunnersByState.scaleTotal() if ephemeralRunnerSet.Spec.PatchID == 0 || ephemeralRunnerSet.Spec.PatchID != ephemeralRunnersByState.latestPatchID { if len(ephemeralRunnersByState.finished) > 0 { - if err := r.cleanupFinishedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil { + if err := r.deleteTerminatedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil { log.Error(err, "failed to cleanup finished ephemeral runners") return ctrl.Result{}, err } @@ -272,7 +290,21 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx return fmt.Errorf("failed to list child ephemeral runners: %w", err) } - if len(newEphemeralRunnersByStates(ephemeralRunnerList).outdated) == 0 { + // Judge the runners against the revision being applied, not the one + // recorded in status: every runner created before this update is stale by + // definition, so its Outdated report says nothing about the new spec. This + // is what lets a spec update clear the Outdated phase immediately rather + // than waiting for the pre-update runners to be collected. + state := newEphemeralRunnersByStates(ephemeralRunnerList, targetAppliedRevision) + + // Set the phase in both directions. This function returns early from + // Reconcile without reaching updateStatus, so leaving the phase untouched + // would let a stale value survive: a stale Running would hide genuinely + // outdated runners from the cleanup path, and a stale Outdated would keep + // the set switched off after the spec that caused it was replaced. + if len(state.outdated) > 0 { + latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseOutdated + } else { latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning } @@ -332,12 +364,13 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer return nil } -func (r *EphemeralRunnerSetReconciler) cleanupFinishedEphemeralRunners(ctx context.Context, finishedEphemeralRunners []*v1alpha1.EphemeralRunner, log logr.Logger) error { - // cleanup finished runners and proceed +// deleteTerminatedEphemeralRunners deletes runners that have reached a terminal +// state and are no longer useful, so that the scaling logic can replace them. +func (r *EphemeralRunnerSetReconciler) deleteTerminatedEphemeralRunners(ctx context.Context, ephemeralRunners []*v1alpha1.EphemeralRunner, log logr.Logger) error { var errs []error - for i := range finishedEphemeralRunners { - log.Info("Deleting finished ephemeral runner", "name", finishedEphemeralRunners[i].Name) - if err := r.Delete(ctx, finishedEphemeralRunners[i]); err != nil { + for i := range ephemeralRunners { + log.Info("Deleting terminated ephemeral runner", "name", ephemeralRunners[i].Name, "phase", ephemeralRunners[i].Status.Phase) + if err := r.Delete(ctx, ephemeralRunners[i]); err != nil { if !kerrors.IsNotFound(err) { errs = append(errs, err) } @@ -383,7 +416,7 @@ func (r *EphemeralRunnerSetReconciler) cleanUpEphemeralRunners(ctx context.Conte return true, nil } - ephemeralRunnerState := newEphemeralRunnersByStates(ephemeralRunnerList) + ephemeralRunnerState := newEphemeralRunnersByStates(ephemeralRunnerList, ephemeralRunnerSet.Status.AppliedActionableRevision) log.Info( "Clean up runner counts", @@ -754,12 +787,27 @@ type ephemeralRunnersByState struct { finished []*v1alpha1.EphemeralRunner failed []*v1alpha1.EphemeralRunner deleting []*v1alpha1.EphemeralRunner + // outdated holds runners that reported Outdated against the runner spec that + // is currently applied. They are evidence that the current spec is still + // rejected by the service, so they drive the set into the Outdated phase. outdated []*v1alpha1.EphemeralRunner + // staleOutdated holds runners that reported Outdated against a runner spec + // that has since been replaced. They say nothing about the current spec, so + // they must not drive the set into the Outdated phase; they are deleted and + // replaced by runners built from the current spec instead. + staleOutdated []*v1alpha1.EphemeralRunner latestPatchID int } -func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerList) *ephemeralRunnersByState { +// newEphemeralRunnersByStates groups the child runners by state. +// +// appliedActionableRevision is the EphemeralRunnerSet revision the runners are +// being judged against. A runner that reported Outdated before that revision was +// applied is classified as stale rather than outdated, so that updating the +// runner spec clears the Outdated phase immediately instead of waiting for the +// pre-update runners to disappear. +func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerList, appliedActionableRevision int64) *ephemeralRunnersByState { var ephemeralRunnerState ephemeralRunnersByState for i := range ephemeralRunnerList.Items { @@ -781,7 +829,11 @@ func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerLi case v1alpha1.EphemeralRunnerPhaseFailed: ephemeralRunnerState.failed = append(ephemeralRunnerState.failed, r) case v1alpha1.EphemeralRunnerPhaseOutdated: - ephemeralRunnerState.outdated = append(ephemeralRunnerState.outdated, r) + if ephemeralRunnerActionableRevision(r) < appliedActionableRevision { + ephemeralRunnerState.staleOutdated = append(ephemeralRunnerState.staleOutdated, r) + } else { + ephemeralRunnerState.outdated = append(ephemeralRunnerState.outdated, r) + } default: // Pending or no phase should be considered as pending. // @@ -793,8 +845,25 @@ func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerLi return &ephemeralRunnerState } +// ephemeralRunnerActionableRevision reports the EphemeralRunnerSet revision the +// runner was created from. Runners created before this annotation existed report +// 0, which matches the zero value of Status.AppliedActionableRevision, so they +// are treated as current until the spec is updated for the first time. +func ephemeralRunnerActionableRevision(ephemeralRunner *v1alpha1.EphemeralRunner) int64 { + revision, err := strconv.ParseInt(ephemeralRunner.Annotations[AnnotationKeyActionableRevision], 10, 64) + if err != nil { + return 0 + } + return revision +} + func (s *ephemeralRunnersByState) terminated() []*v1alpha1.EphemeralRunner { - return append(s.finished, append(s.failed, s.outdated...)...) + terminated := make([]*v1alpha1.EphemeralRunner, 0, len(s.finished)+len(s.failed)+len(s.outdated)+len(s.staleOutdated)) + terminated = append(terminated, s.finished...) + terminated = append(terminated, s.failed...) + terminated = append(terminated, s.outdated...) + terminated = append(terminated, s.staleOutdated...) + return terminated } func (s *ephemeralRunnersByState) scaleTotal() int { diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index a563016f41..7a020a57b8 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -2057,11 +2057,17 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) Expect(err).NotTo(HaveOccurred()) - // Create a runner that will cause phase change (outdated runner) + // Create a runner that will cause phase change (outdated runner). + // It must carry the revision the set has applied, otherwise it is an + // outdated report about a runner spec that has already been replaced and + // the set deliberately ignores it. ephemeralRunner := &v1alpha1.EphemeralRunner{ ObjectMeta: metav1.ObjectMeta{ Name: "test-runner-outdated", Namespace: autoscalingNS.Name, + Annotations: map[string]string{ + AnnotationKeyActionableRevision: "5", + }, Labels: map[string]string{ LabelKeyGitHubScaleSetName: ephemeralRunnerSet.Name, LabelKeyGitHubScaleSetNamespace: ephemeralRunnerSet.Namespace, @@ -2118,6 +2124,107 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { g.Expect(updatedSet.Status.AppliedActionableRevision).To(Equal(int64(5)), "AppliedActionableRevision should be preserved") }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) }) + + // A runner that reported Outdated against a runner spec that has since been + // replaced must not drag the whole set back into the Outdated phase, because + // that switches the scale set off and discards the update the user just made. + // The runner is deleted instead, so the scaling logic replaces it with one + // built from the current spec. + It("replaces outdated runners from a superseded revision instead of going Outdated", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient()), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-stale-outdated", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 2, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:new"}}}}, + }, + }, + } + Expect(k8sClient.Create(ctx, ephemeralRunnerSet)).To(Succeed()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + // The set is already running revision 2. + current := new(v1alpha1.EphemeralRunnerSet) + Expect(k8sClient.Get(ctx, request.NamespacedName, current)).To(Succeed()) + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 2 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + Expect(k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current))).To(Succeed()) + + // A runner left over from revision 1 reports Outdated. This happens when a + // runner was busy with a job while the spec was updated, so it survived the + // revision cleanup and only exited (with the outdated exit code) afterwards. + staleRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner-from-old-revision", + Namespace: autoscalingNS.Name, + Annotations: map[string]string{AnnotationKeyActionableRevision: "1"}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: ephemeralRunnerSet.Name, + UID: ephemeralRunnerSet.UID, + Controller: func(b bool) *bool { return &b }(true), + BlockOwnerDeletion: func(b bool) *bool { return &b }(true), + }, + }, + }, + Spec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:old"}}}}, + }, + } + Expect(k8sClient.Create(ctx, staleRunner)).To(Succeed()) + + runnerStatusUpdated := staleRunner.DeepCopy() + runnerStatusUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseOutdated + Expect(k8sClient.Status().Patch(ctx, runnerStatusUpdated, client.MergeFrom(staleRunner))).To(Succeed()) + + Eventually(func(g Gomega) { + cachedRunner := new(v1alpha1.EphemeralRunner) + g.Expect(controller.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: staleRunner.Name}, cachedRunner)).To(Succeed()) + g.Expect(cachedRunner.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerPhaseOutdated)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // The stale runner is removed rather than being treated as a verdict on the + // current spec. + Eventually(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + runner := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: staleRunner.Name}, runner) + g.Expect(kerrors.IsNotFound(err) || !runner.DeletionTimestamp.IsZero()).To(BeTrue(), "stale outdated runner should be deleted") + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // And the set never reports Outdated because of it. + Consistently(func(g Gomega) { + updatedSet := new(v1alpha1.EphemeralRunnerSet) + g.Expect(k8sClient.Get(ctx, request.NamespacedName, updatedSet)).To(Succeed()) + g.Expect(updatedSet.Status.Phase).NotTo(Equal(v1alpha1.EphemeralRunnerSetPhaseOutdated)) + }, "2s", ephemeralRunnerSetTestInterval).Should(Succeed()) + }) }) var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func() { diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go index a8c06fffc0..e96b062149 100644 --- a/controllers/actions.github.com/helpers.go +++ b/controllers/actions.github.com/helpers.go @@ -39,6 +39,30 @@ func nextActionableRevision(current *v1alpha1.EphemeralRunnerSet) int64 { return current.Status.AppliedActionableRevision + 1 } +// ephemeralRunnerSetOutdatedForAppliedRevision reports whether the set is +// Outdated *because of the runner spec it is currently running*, which is the +// only situation in which the AutoscalingRunnerSet should tear the scale set +// down. +// +// The phase alone is not enough. Outdated is deliberately sticky: it survives in +// status while the outdated runners are collected, and it is only cleared once a +// new revision is applied. So between the moment the AutoscalingRunnerSet patches +// a new runner spec onto the set and the moment the EphemeralRunnerSet controller +// processes that patch, the set still reports Outdated for a spec that no longer +// exists. Tearing down there would discard the fix the user just applied, and the +// scale set would stay switched off until something else nudged it. +// +// Requiring the applied revision to have caught up with the spec revision closes +// that window: the verdict counts only once the set is running the current spec. +func ephemeralRunnerSetOutdatedForAppliedRevision(ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet) bool { + if ephemeralRunnerSet == nil { + return false + } + + return ephemeralRunnerSet.Status.Phase == v1alpha1.EphemeralRunnerSetPhaseOutdated && + ephemeralRunnerSet.Status.AppliedActionableRevision >= ephemeralRunnerSet.Spec.ActionableRevision +} + // listenerPodSpecRequiresRecreation reports whether the live listener pod must be // deleted and rebuilt to match the desired spec. // diff --git a/controllers/actions.github.com/helpers_outdated_test.go b/controllers/actions.github.com/helpers_outdated_test.go new file mode 100644 index 0000000000..19e7f52d20 --- /dev/null +++ b/controllers/actions.github.com/helpers_outdated_test.go @@ -0,0 +1,217 @@ +package actionsgithubcom + +import ( + "strconv" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func outdatedRunnerAtRevision(name string, revision int64) v1alpha1.EphemeralRunner { + return v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Annotations: map[string]string{ + AnnotationKeyActionableRevision: strconv.FormatInt(revision, 10), + }, + }, + Status: v1alpha1.EphemeralRunnerStatus{ + Phase: v1alpha1.EphemeralRunnerPhaseOutdated, + }, + } +} + +// TestNewEphemeralRunnersByStates_OutdatedIsRevisionScoped covers the core of the +// outdated-recovery behaviour: a runner that reported Outdated against a runner +// spec that has since been replaced must not be treated as evidence about the +// current spec. +func TestNewEphemeralRunnersByStates_OutdatedIsRevisionScoped(t *testing.T) { + tests := []struct { + name string + runners []v1alpha1.EphemeralRunner + appliedRevision int64 + wantOutdatedNames []string + wantStaleOutdatedName []string + }{ + { + name: "runner at the applied revision is genuinely outdated", + runners: []v1alpha1.EphemeralRunner{outdatedRunnerAtRevision("current", 3)}, + appliedRevision: 3, + wantOutdatedNames: []string{"current"}, + }, + { + name: "runner from before the last spec update is stale", + runners: []v1alpha1.EphemeralRunner{outdatedRunnerAtRevision("old", 2)}, + appliedRevision: 3, + wantStaleOutdatedName: []string{"old"}, + }, + { + name: "mixed revisions are split", + runners: []v1alpha1.EphemeralRunner{ + outdatedRunnerAtRevision("old", 1), + outdatedRunnerAtRevision("current", 4), + }, + appliedRevision: 4, + wantOutdatedNames: []string{"current"}, + wantStaleOutdatedName: []string{"old"}, + }, + { + name: "runner without the annotation is treated as revision 0", + runners: []v1alpha1.EphemeralRunner{{ + ObjectMeta: metav1.ObjectMeta{Name: "legacy"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseOutdated}, + }}, + appliedRevision: 0, + wantOutdatedNames: []string{"legacy"}, + }, + { + name: "legacy runner becomes stale once a revision is applied", + runners: []v1alpha1.EphemeralRunner{{ + ObjectMeta: metav1.ObjectMeta{Name: "legacy"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseOutdated}, + }}, + appliedRevision: 1, + wantStaleOutdatedName: []string{"legacy"}, + }, + { + name: "a runner being deleted is never classified as outdated", + runners: []v1alpha1.EphemeralRunner{func() v1alpha1.EphemeralRunner { + runner := outdatedRunnerAtRevision("terminating", 3) + now := metav1.Now() + runner.DeletionTimestamp = &now + runner.Finalizers = []string{ephemeralRunnerFinalizerName} + return runner + }()}, + appliedRevision: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + list := &v1alpha1.EphemeralRunnerList{Items: tt.runners} + state := newEphemeralRunnersByStates(list, tt.appliedRevision) + + assert.Equal(t, tt.wantOutdatedNames, runnerNames(state.outdated)) + assert.Equal(t, tt.wantStaleOutdatedName, runnerNames(state.staleOutdated)) + }) + } +} + +// TestEphemeralRunnersByState_TerminatedIncludesStaleOutdated ensures the cleanup +// paths still collect stale outdated runners; they are excluded from the phase +// decision, not from garbage collection. +func TestEphemeralRunnersByState_TerminatedIncludesStaleOutdated(t *testing.T) { + list := &v1alpha1.EphemeralRunnerList{Items: []v1alpha1.EphemeralRunner{ + outdatedRunnerAtRevision("stale", 1), + outdatedRunnerAtRevision("current", 5), + { + ObjectMeta: metav1.ObjectMeta{Name: "succeeded"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseSucceeded}, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "failed"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseFailed}, + }, + }} + + state := newEphemeralRunnersByStates(list, 5) + + assert.ElementsMatch(t, + []string{"succeeded", "failed", "current", "stale"}, + runnerNames(state.terminated()), + ) +} + +// TestEphemeralRunnersByState_TerminatedDoesNotAliasBackingArrays guards against +// terminated() corrupting the slices it concatenates, which would silently +// reclassify runners. +func TestEphemeralRunnersByState_TerminatedDoesNotAliasBackingArrays(t *testing.T) { + list := &v1alpha1.EphemeralRunnerList{Items: []v1alpha1.EphemeralRunner{ + { + ObjectMeta: metav1.ObjectMeta{Name: "succeeded"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseSucceeded}, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "failed"}, + Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseFailed}, + }, + }} + + state := newEphemeralRunnersByStates(list, 0) + _ = state.terminated() + + assert.Equal(t, []string{"succeeded"}, runnerNames(state.finished)) + assert.Equal(t, []string{"failed"}, runnerNames(state.failed)) +} + +// TestEphemeralRunnerSetOutdatedForAppliedRevision covers the guard that stops the +// AutoscalingRunnerSet from tearing the scale set down on an Outdated verdict that +// predates the runner spec it has just published. +func TestEphemeralRunnerSetOutdatedForAppliedRevision(t *testing.T) { + tests := []struct { + name string + phase v1alpha1.EphemeralRunnerSetPhase + specRevision int64 + appliedRevision int64 + want bool + }{ + { + name: "nil-safe: running set is not outdated", + phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + want: false, + }, + { + name: "outdated against the spec it is running", + phase: v1alpha1.EphemeralRunnerSetPhaseOutdated, + specRevision: 4, + appliedRevision: 4, + want: true, + }, + { + name: "outdated verdict predates a spec update that is still propagating", + phase: v1alpha1.EphemeralRunnerSetPhaseOutdated, + specRevision: 5, + appliedRevision: 4, + want: false, + }, + { + name: "legacy set with no revisions recorded still tears down", + phase: v1alpha1.EphemeralRunnerSetPhaseOutdated, + specRevision: 0, + appliedRevision: 0, + want: true, + }, + { + name: "running set with a pending revision is not outdated", + phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + specRevision: 5, + appliedRevision: 4, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + Spec: v1alpha1.EphemeralRunnerSetSpec{ActionableRevision: tt.specRevision}, + Status: v1alpha1.EphemeralRunnerSetStatus{Phase: tt.phase, AppliedActionableRevision: tt.appliedRevision}, + } + assert.Equal(t, tt.want, ephemeralRunnerSetOutdatedForAppliedRevision(ephemeralRunnerSet)) + }) + } + + assert.False(t, ephemeralRunnerSetOutdatedForAppliedRevision(nil)) +} + +func runnerNames(runners []*v1alpha1.EphemeralRunner) []string { + if len(runners) == 0 { + return nil + } + names := make([]string, 0, len(runners)) + for _, runner := range runners { + names = append(names, runner.Name) + } + return names +} diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index c660a94f4f..2ec35a1da5 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -823,9 +823,10 @@ func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.Epheme maps.Copy(labels, ephemeralRunnerSet.Labels) labels[LabelKeyKubernetesComponent] = "runner" - annotations := make(map[string]string, len(ephemeralRunnerSet.Annotations)+1) + annotations := make(map[string]string, len(ephemeralRunnerSet.Annotations)+2) maps.Copy(annotations, ephemeralRunnerSet.Annotations) annotations[AnnotationKeyPatchID] = strconv.Itoa(ephemeralRunnerSet.Spec.PatchID) + annotations[AnnotationKeyActionableRevision] = strconv.FormatInt(ephemeralRunnerSet.Spec.ActionableRevision, 10) if ephemeralRunnerSet.Spec.EphemeralRunnerMetadata != nil { labels = b.filterAndMergeLabels(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Labels, labels) From f22def86e0404c353b7fc29788491a5d90d10185 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 9 Sep 2026 10:41:31 +0200 Subject: [PATCH 5/5] Recreate the listener pod when its config secret changes 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> --- controllers/actions.github.com/constants.go | 7 +++ controllers/actions.github.com/helpers.go | 30 +++++++++++ .../helpers_listener_test.go | 54 +++++++++++++++++++ .../actions.github.com/resourcebuilder.go | 10 ++-- 4 files changed, 97 insertions(+), 4 deletions(-) diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index 3c523e9d0e..af05eaf703 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -56,6 +56,13 @@ const ( // runner spec from one that reported it against a spec that has since been // updated. AnnotationKeyActionableRevision = "actions.github.com/actionable-revision" + // AnnotationKeyListenerConfigResourceVersion records the resource version of + // the listener config secret the listener pod was created from. The pod + // mounts that secret and parses it once at startup, so a change to its + // contents only takes effect after a restart. Nothing about the change is + // visible in the pod spec, which references the secret by name, so the + // resource version is carried on the pod to make the drift observable. + AnnotationKeyListenerConfigResourceVersion = "actions.github.com/listener-config-resource-version" ) // Labels applied to listener roles diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go index e96b062149..27c26613b1 100644 --- a/controllers/actions.github.com/helpers.go +++ b/controllers/actions.github.com/helpers.go @@ -66,6 +66,23 @@ func ephemeralRunnerSetOutdatedForAppliedRevision(ephemeralRunnerSet *v1alpha1.E // listenerPodSpecRequiresRecreation reports whether the live listener pod must be // deleted and rebuilt to match the desired spec. // +// The config secret is checked first. The pod mounts it as a volume and the +// listener 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. None of that is visible in the pod spec, which +// references the secret by name and is byte-identical before and after, so the +// desired pod carries the secret's resource version as an annotation and drift +// is detected by comparing it. An empty annotation on the live pod is ignored: +// pods created before this annotation existed would otherwise all be recreated +// on controller upgrade, and the next legitimate config change recreates them +// anyway. +// +// The resource version also moves when only the secret's labels or annotations +// change, which does not affect the listener. That is accepted rather than +// worked around by hashing the secret data: those fields come from the +// AutoscalingListener spec, and a change to that spec already makes the +// AutoscalingRunnerSet controller replace the listener wholesale. +// // DeepDerivative, not DeepEqual: the live pod carries a large number of fields // the desired pod never sets, written by the API server and by admission // (nodeName, dnsPolicy, schedulerName, securityContext, enableServiceLinks, @@ -92,6 +109,10 @@ func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool { return current != desired } + if listenerConfigChanged(current, desired) { + return true + } + if listenerContainerPortsRemoved(current, desired) { return true } @@ -99,6 +120,15 @@ func listenerPodSpecRequiresRecreation(current, desired *corev1.Pod) bool { return !apiequality.Semantic.DeepDerivative(desired.Spec, current.Spec) } +func listenerConfigChanged(current, desired *corev1.Pod) bool { + currentVersion := current.Annotations[AnnotationKeyListenerConfigResourceVersion] + desiredVersion := desired.Annotations[AnnotationKeyListenerConfigResourceVersion] + if currentVersion == "" || desiredVersion == "" { + return false + } + return currentVersion != desiredVersion +} + func listenerContainerPortsRemoved(current, desired *corev1.Pod) bool { for i := range desired.Spec.Containers { desiredContainer := &desired.Spec.Containers[i] diff --git a/controllers/actions.github.com/helpers_listener_test.go b/controllers/actions.github.com/helpers_listener_test.go index 89384a03a1..ecd4044680 100644 --- a/controllers/actions.github.com/helpers_listener_test.go +++ b/controllers/actions.github.com/helpers_listener_test.go @@ -275,3 +275,57 @@ func TestListenerPodSpecRequiresRecreation_MetricsToggleUsesRealBuilder(t *testi assert.False(t, listenerPodSpecRequiresRecreation(withMetrics, withMetrics), "an unchanged metrics configuration must not recreate the pod") } + +// The listener pod mounts its config as a secret volume and parses it once at +// startup, so a change to the secret contents is invisible in the pod spec but +// still requires a restart to take effect. +func TestListenerPodSpecRequiresRecreation_ConfigSecretChanged(t *testing.T) { + tests := map[string]struct { + liveVersion string + desiredVersion string + want bool + why string + }{ + "unchanged": { + liveVersion: "100", + desiredVersion: "100", + want: false, + why: "the config the listener is running is still the desired one", + }, + "changed": { + liveVersion: "100", + desiredVersion: "101", + want: true, + why: "the listener only reads its config at startup, so it must be restarted", + }, + "missing on live pod": { + liveVersion: "", + desiredVersion: "101", + want: false, + why: "pods predating the annotation must not all be recreated on controller upgrade", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + desired := desiredListenerPod() + live := livePodFromDesired(desired) + + setListenerConfigVersion(live, tt.liveVersion) + setListenerConfigVersion(desired, tt.desiredVersion) + + assert.Equal(t, tt.want, listenerPodSpecRequiresRecreation(live, desired), tt.why) + }) + } +} + +func setListenerConfigVersion(pod *corev1.Pod, version string) { + if version == "" { + delete(pod.Annotations, AnnotationKeyListenerConfigResourceVersion) + return + } + if pod.Annotations == nil { + pod.Annotations = map[string]string{} + } + pod.Annotations[AnnotationKeyListenerConfigResourceVersion] = version +} diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index 9a6776d3a5..ea092aab09 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -447,10 +447,12 @@ func (b *ResourceBuilder) newScaleSetListenerPod( Kind: "Pod", }, ObjectMeta: metav1.ObjectMeta{ - Name: autoscalingListener.Name, - Namespace: autoscalingListener.Namespace, - Labels: labels, - Annotations: make(map[string]string), + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Namespace, + Labels: labels, + Annotations: map[string]string{ + AnnotationKeyListenerConfigResourceVersion: podConfig.ResourceVersion, + }, }, Spec: podSpec, }