diff --git a/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go b/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go index b1bb01363a..3c4b3b3a00 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" ) @@ -87,10 +86,6 @@ type AutoscalingListenerSpec struct { ListenerConfig *ListenerConfig `json:"listenerConfig,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 afcb4c93ed..018f71c914 100644 --- a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go @@ -23,7 +23,6 @@ import ( "net/url" "strings" - "github.com/actions/actions-runner-controller/hash" "github.com/actions/actions-runner-controller/vault" "golang.org/x/net/http/httpproxy" corev1 "k8s.io/api/core/v1" @@ -322,6 +321,10 @@ type HistogramMetric struct { type AutoscalingRunnerSetStatus struct { // +optional Phase AutoscalingRunnerSetPhase `json:"phase"` + // ObservedGeneration tracks the metadata.generation of this ARS at observation time, + // enabling detection of Pending phase when generation differs. Unset defaults to 0. + // +optional + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } type AutoscalingRunnerSetPhase string @@ -334,26 +337,6 @@ const ( AutoscalingRunnerSetPhaseOutdated AutoscalingRunnerSetPhase = "Outdated" ) -func (ars *AutoscalingRunnerSet) Hash() string { - type data struct { - Spec *AutoscalingRunnerSetSpec - Labels map[string]string - } - - d := &data{ - Spec: ars.Spec.DeepCopy(), - Labels: ars.Labels, - } - - return hash.ComputeTemplateHash(d) -} - -func (ars *AutoscalingRunnerSet) ListenerSpecHash() string { - arsSpec := ars.Spec.DeepCopy() - spec := arsSpec - return hash.ComputeTemplateHash(&spec) -} - func (ars *AutoscalingRunnerSet) GitHubConfigSecret() string { return ars.Spec.GitHubConfigSecret } @@ -381,28 +364,6 @@ func (ars *AutoscalingRunnerSet) VaultProxy() *ProxyConfig { return nil } -func (ars *AutoscalingRunnerSet) RunnerSetSpecHash() string { - type runnerSetSpec struct { - GitHubConfigUrl string - GitHubConfigSecret string - RunnerGroup string - RunnerScaleSetName string - Proxy *ProxyConfig - GitHubServerTLS *TLSConfig - Template corev1.PodTemplateSpec - } - spec := &runnerSetSpec{ - GitHubConfigUrl: ars.Spec.GitHubConfigUrl, - GitHubConfigSecret: ars.Spec.GitHubConfigSecret, - RunnerGroup: ars.Spec.RunnerGroup, - RunnerScaleSetName: ars.Spec.RunnerScaleSetName, - Proxy: ars.Spec.Proxy, - GitHubServerTLS: ars.Spec.GitHubServerTLS, - Template: ars.Spec.Template, - } - return hash.ComputeTemplateHash(&spec) -} - // +kubebuilder:object:root=true // AutoscalingRunnerSetList contains a list of AutoscalingRunnerSet diff --git a/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 641ec02f88..0e9a398b59 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 @@ -71,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 } diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml index 8b78ce2430..386284273f 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml @@ -16555,6 +16555,12 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + observedGeneration: + description: |- + ObservedGeneration tracks the metadata.generation of this ARS at observation time, + enabling detection of Pending phase when generation differs. Unset defaults to 0. + format: int64 + type: integer phase: type: string type: object diff --git a/charts/gha-runner-scale-set-controller-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 8b78ce2430..386284273f 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml @@ -16555,6 +16555,12 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + observedGeneration: + description: |- + ObservedGeneration tracks the metadata.generation of this ARS at observation time, + enabling detection of Pending phase when generation differs. Unset defaults to 0. + format: int64 + type: integer phase: type: string type: object diff --git a/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 8b78ce2430..386284273f 100644 --- a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml +++ b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml @@ -16555,6 +16555,12 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + observedGeneration: + description: |- + ObservedGeneration tracks the metadata.generation of this ARS at observation time, + enabling detection of Pending phase when generation differs. Unset defaults to 0. + format: int64 + type: integer phase: type: string type: object diff --git a/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 497a294abb..662a9a1adf 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -467,7 +467,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..402da77b12 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 @@ -253,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) @@ -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 efdd2df14c..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") }) }) @@ -2247,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() { diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index 588c95beb7..af05eaf703 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -50,6 +50,19 @@ 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" + // 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/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index c4f6d8c025..45303138f5 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 } @@ -191,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), @@ -203,17 +200,50 @@ 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 { - defer func() { - if err := r.cleanupFinishedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil { + if len(ephemeralRunnersByState.finished) > 0 { + if err := r.deleteTerminatedEphemeralRunners(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 +275,65 @@ 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) + } + + // 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 + } + + 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 +346,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. @@ -273,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) } @@ -324,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", @@ -695,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 { @@ -722,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. // @@ -734,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 331e8e596c..7a020a57b8 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,574 @@ 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). + // 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, + }, + 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()) + }) + + // 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() { 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..27c26613b1 --- /dev/null +++ b/controllers/actions.github.com/helpers.go @@ -0,0 +1,155 @@ +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" +) + +// 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 +} + +// 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. +// +// 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, +// 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 listenerConfigChanged(current, desired) { + return true + } + + if listenerContainerPortsRemoved(current, desired) { + return true + } + + 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] + 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..ecd4044680 --- /dev/null +++ b/controllers/actions.github.com/helpers_listener_test.go @@ -0,0 +1,331 @@ +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") +} + +// 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/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 961f7b9821..ea092aab09 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 ( @@ -185,9 +176,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) @@ -322,8 +311,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) } @@ -331,18 +318,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, @@ -472,24 +447,16 @@ 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, } - 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) } @@ -502,38 +469,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) @@ -683,8 +618,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) } @@ -693,22 +626,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{ @@ -747,24 +664,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{ @@ -817,26 +721,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 { @@ -904,8 +793,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) } @@ -914,17 +801,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{ @@ -939,8 +815,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) } @@ -948,26 +822,15 @@ 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) 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) @@ -1106,8 +969,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) } @@ -1115,18 +976,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", - }, }, }