diff --git a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go index 641ec02f..2f2be1d9 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go @@ -36,12 +36,23 @@ type EphemeralRunnerSetSpec struct { // but does not apply to existing ephemeral runners. // +optional EphemeralRunnerMetadata *ResourceMeta `json:"ephemeralRunnerMetadata,omitempty"` + // ActionableRevision is the desired runner-spec revision. It increments whenever + // Spec.EphemeralRunnerSpec changes, enabling the EphemeralRunnerSet controller + // to detect 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"` } // EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource @@ -71,11 +82,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_ephemeralrunnersets.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml index a6f9758b..eb8a605e 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,14 @@ spec: spec: description: EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet properties: + actionableRevision: + description: |- + ActionableRevision is the desired runner-spec revision. It increments whenever + Spec.EphemeralRunnerSpec changes, enabling the EphemeralRunnerSet controller + to detect 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 +8303,13 @@ 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 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_ephemeralrunnersets.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml index a6f9758b..eb8a605e 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,14 @@ spec: spec: description: EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet properties: + actionableRevision: + description: |- + ActionableRevision is the desired runner-spec revision. It increments whenever + Spec.EphemeralRunnerSpec changes, enabling the EphemeralRunnerSet controller + to detect 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 +8303,13 @@ 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 phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string diff --git a/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml b/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml index a6f9758b..eb8a605e 100644 --- a/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml +++ b/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml @@ -46,6 +46,14 @@ spec: spec: description: EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet properties: + actionableRevision: + description: |- + ActionableRevision is the desired runner-spec revision. It increments whenever + Spec.EphemeralRunnerSpec changes, enabling the EphemeralRunnerSet controller + to detect 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 +8303,13 @@ 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 phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index d4404243..5b8a664c 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -288,10 +288,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) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index 10cf20ee..2965683a 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -743,7 +743,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" @@ -757,7 +757,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { 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.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") + g.Expect(current.Spec.ActionableRevision).To(BeNumerically(">", originalActionableRevision), "ActionableRevision should increment for actionable spec changes") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -796,7 +796,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 @@ -822,7 +822,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, diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index c4f6d8c0..31a6069f 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,15 @@ 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 the runner spec revision has advanced past the one that was last + // successfully applied, delete idle and pending ephemeral runners so they are + // rebuilt from 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 +153,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 } @@ -245,6 +244,49 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) } +// patchAppliedActionableRevisionStatus records that the runner spec carried by +// targetAppliedRevision has been fully applied. +// +// The marker lives in status rather than in an annotation on the spec, and it is +// written only once the cleanup above has actually succeeded. If the controller +// dies part-way through deleting the idle and pending runners, the applied +// revision is still behind the spec revision when it comes back, so the work is +// redone rather than skipped. Writing the marker first, or writing it together +// with the spec, would let a crash leave runners alive that are running a spec +// nobody will ever revisit. +// +// The object is re-fetched inside the retry rather than reusing the copy the +// reconciler already has, because the cleanup can take long enough for that copy +// to go stale, and a conflicting write must not be resolved by replaying an old +// status. +// +// The patch carries an optimistic lock so that the re-fetch actually means +// something. A plain merge patch has no resourceVersion precondition, so the API +// server can never reject it as conflicting: RetryOnConflict would never fire, +// and a patch computed from a stale read could move the applied revision +// backwards, re-satisfying the spec > applied comparison above and deleting the +// idle runners all over again. With the lock, the server accepts the write only +// if the re-fetched object is still the live one, so a successful patch proves +// the monotonicity check above was evaluated against live data. A stale attempt +// conflicts and is retried or requeued instead of regressing the marker. +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 + } + + if latest.Status.AppliedActionableRevision >= targetAppliedRevision { + return nil + } + + original := latest.DeepCopy() + latest.Status.AppliedActionableRevision = targetAppliedRevision + + return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})) + }) +} + 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 +299,8 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer phase = ephemeralRunnerSet.Status.Phase } desiredStatus := v1alpha1.EphemeralRunnerSetStatus{ - Phase: phase, + Phase: phase, + AppliedActionableRevision: ephemeralRunnerSet.Status.AppliedActionableRevision, } // Update the status if needed. diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index 331e8e59..cbdbae35 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -1117,6 +1117,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") @@ -1920,3 +1921,464 @@ func listEphemeralRunnersAndRemoveFinalizers(ctx context.Context, k8sClient clie list.Items = liveItems return nil } + +var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { + var ctx context.Context + var mgr ctrl.Manager + var autoscalingNS *corev1.Namespace + var configSecret *corev1.Secret + + newRunner := func(name string, ers *v1alpha1.EphemeralRunnerSet) *v1alpha1.EphemeralRunner { + controllerRef := true + return &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ers.Namespace, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: ers.Name, + UID: ers.UID, + Controller: &controllerRef, + }}, + }, + Spec: ers.Spec.EphemeralRunnerSpec, + } + } + + BeforeEach(func() { + ctx = context.Background() + autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) + configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + startManagers(GinkgoT(), mgr) + }) + + It("does not clean up runners on initial creation without an actionable revision", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient(fake.WithRemoveRunner(nil))), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-actionable-revision-initial", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + pendingRunner := newRunner("runner-pending-initial", ephemeralRunnerSet) + err = k8sClient.Create(ctx, pendingRunner) + Expect(err).NotTo(HaveOccurred()) + + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Consistently(func() error { + runner := new(v1alpha1.EphemeralRunner) + return k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: pendingRunner.Name}, runner) + }, time.Second, ephemeralRunnerSetTestInterval).Should(Succeed()) + + Consistently(func() int64 { + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return -1 + } + return updatedSet.Status.AppliedActionableRevision + }, time.Second, ephemeralRunnerSetTestInterval).Should(Equal(int64(0))) + }) + + It("deletes runner-a-idle, keeps runner-b-busy, and advances applied actionable revision 3 to 4", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient(fake.WithRemoveRunner(nil))), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-actionable-revision-success", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 3, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 3 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + idleRunner := newRunner("runner-a-idle", statusUpdated) + err = k8sClient.Create(ctx, idleRunner) + Expect(err).NotTo(HaveOccurred()) + + idleCurrent := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(idleRunner), idleCurrent) + Expect(err).NotTo(HaveOccurred()) + idleUpdated := idleCurrent.DeepCopy() + idleUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + idleUpdated.Status.RunnerID = 101 + err = k8sClient.Status().Patch(ctx, idleUpdated, client.MergeFrom(idleCurrent)) + Expect(err).NotTo(HaveOccurred()) + + busyRunner := newRunner("runner-b-busy", statusUpdated) + err = k8sClient.Create(ctx, busyRunner) + Expect(err).NotTo(HaveOccurred()) + + busyCurrent := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(busyRunner), busyCurrent) + Expect(err).NotTo(HaveOccurred()) + busyUpdated := busyCurrent.DeepCopy() + busyUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + busyUpdated.Status.RunnerID = 102 + busyUpdated.Status.JobID = "job-1" + busyUpdated.Status.WorkflowRunID = 9001 + err = k8sClient.Status().Patch(ctx, busyUpdated, client.MergeFrom(busyCurrent)) + Expect(err).NotTo(HaveOccurred()) + + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + specUpdated := current.DeepCopy() + specUpdated.Spec.ActionableRevision = 4 + err = k8sClient.Patch(ctx, specUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func() bool { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + runner := new(v1alpha1.EphemeralRunner) + return kerrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-a-idle"}, runner)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(BeTrue()) + + Consistently(func() error { + runner := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-b-busy"}, runner); err != nil { + return err + } + if runner.Status.RunnerID != 102 { + return fmt.Errorf("expected busy runner ID 102, got %d", runner.Status.RunnerID) + } + if !runner.HasJob() { + return fmt.Errorf("expected runner-b-busy to keep its assigned job") + } + return nil + }, time.Second, ephemeralRunnerSetTestInterval).Should(Succeed()) + + Eventually(func() int64 { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return 0 + } + return updatedSet.Status.AppliedActionableRevision + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Equal(int64(4))) + }) + + It("keeps applied actionable revision at 3 when cleanup fails", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient(fake.WithRemoveRunner(fmt.Errorf("remove failed")))), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-actionable-revision-error", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 3, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 3 + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + idleRunner := newRunner("runner-a-idle", statusUpdated) + err = k8sClient.Create(ctx, idleRunner) + Expect(err).NotTo(HaveOccurred()) + + idleCurrent := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, client.ObjectKeyFromObject(idleRunner), idleCurrent) + Expect(err).NotTo(HaveOccurred()) + idleUpdated := idleCurrent.DeepCopy() + idleUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning + idleUpdated.Status.RunnerID = 101 + err = k8sClient.Status().Patch(ctx, idleUpdated, client.MergeFrom(idleCurrent)) + Expect(err).NotTo(HaveOccurred()) + + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + specUpdated := current.DeepCopy() + specUpdated.Spec.ActionableRevision = 4 + err = k8sClient.Patch(ctx, specUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + // The reconciler reads through the manager's cache, so retry until the + // bumped actionable revision is observed and cleanup is attempted. + Eventually(func() error { + _, err := controller.Reconcile(ctx, request) + return err + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(MatchError(ContainSubstring("remove failed"))) + + Consistently(func() int64 { + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return 0 + } + return updatedSet.Status.AppliedActionableRevision + }, time.Second, ephemeralRunnerSetTestInterval).Should(Equal(int64(3))) + }) + + It("deletes unregistered pending runner during actionable revision cleanup after restart with no cache", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), // fresh empty cache simulating restart + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient()), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-restart-no-cache", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 4, // spec has been bumped + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:updated"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 3 // status is behind + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + pendingRunner := newRunner("runner-restart-pending", statusUpdated) + err = k8sClient.Create(ctx, pendingRunner) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func(g Gomega) { + cachedSet := new(v1alpha1.EphemeralRunnerSet) + err := controller.Get(ctx, request.NamespacedName, cachedSet) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedSet.Status.AppliedActionableRevision).To(Equal(int64(3))) + + cachedRunner := new(v1alpha1.EphemeralRunner) + err = controller.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-restart-pending"}, cachedRunner) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedRunner.Status.RunnerID).To(BeZero()) + g.Expect(cachedRunner.Status.Phase).To(BeEmpty()) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // Reconcile with fresh cache (simulating restart). Actionable revision cleanup deletes pending runners. + Eventually(func() bool { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + runner := new(v1alpha1.EphemeralRunner) + return kerrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "runner-restart-pending"}, runner)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(BeTrue()) + + // AppliedActionableRevision should advance after cleanup completes. + Eventually(func() int64 { + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + if err := k8sClient.Get(ctx, request.NamespacedName, updatedSet); err != nil { + return 0 + } + return updatedSet.Status.AppliedActionableRevision + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Equal(int64(4))) + }) + + It("preserves AppliedActionableRevision during status-only phase updates", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient()), + )), + }, + } + + // Setup: Create ERS with an actionable revision + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-preserve-applied-revision", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 5, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner"}}}}, + }, + }, + } + + err := k8sClient.Create(ctx, ephemeralRunnerSet) + Expect(err).NotTo(HaveOccurred()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err = controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + // Set AppliedActionableRevision to 5 + current := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, current) + Expect(err).NotTo(HaveOccurred()) + + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 5 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + err = k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current)) + Expect(err).NotTo(HaveOccurred()) + + // Create a runner that will cause phase change (outdated runner). + ephemeralRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-runner-outdated", + Namespace: autoscalingNS.Name, + Labels: map[string]string{ + LabelKeyGitHubScaleSetName: ephemeralRunnerSet.Name, + LabelKeyGitHubScaleSetNamespace: ephemeralRunnerSet.Namespace, + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: ephemeralRunnerSet.Name, + UID: ephemeralRunnerSet.UID, + Controller: func(b bool) *bool { return &b }(true), + BlockOwnerDeletion: func(b bool) *bool { return &b }(true), + }, + }, + }, + Spec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:old"}}}}, + }, + } + err = k8sClient.Create(ctx, ephemeralRunner) + Expect(err).NotTo(HaveOccurred()) + + runnerStatusUpdated := ephemeralRunner.DeepCopy() + runnerStatusUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseOutdated + runnerStatusUpdated.Status.RunnerID = 123 + runnerStatusUpdated.Status.JobRequestID = 456 + err = k8sClient.Status().Patch(ctx, runnerStatusUpdated, client.MergeFrom(ephemeralRunner)) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func(g Gomega) { + cachedSet := new(v1alpha1.EphemeralRunnerSet) + err := controller.Get(ctx, request.NamespacedName, cachedSet) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedSet.Status.AppliedActionableRevision).To(Equal(int64(5))) + + cachedRunner := new(v1alpha1.EphemeralRunner) + err = controller.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: "test-runner-outdated"}, cachedRunner) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cachedRunner.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerPhaseOutdated)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // Verify: Phase changed to Outdated, but AppliedActionableRevision preserved + Eventually(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + updatedSet := new(v1alpha1.EphemeralRunnerSet) + err = k8sClient.Get(ctx, request.NamespacedName, updatedSet) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(updatedSet.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerSetPhaseOutdated), "phase should change to Outdated") + g.Expect(updatedSet.Status.AppliedActionableRevision).To(Equal(int64(5)), "AppliedActionableRevision should be preserved") + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + }) +}) diff --git a/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go b/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go new file mode 100644 index 00000000..df3f6309 --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go @@ -0,0 +1,142 @@ +package actionsgithubcom + +import ( + "context" + "encoding/json" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +// TestPatchAppliedActionableRevisionStatusUsesOptimisticLock pins the property +// that makes the applied revision marker monotonic: the status patch must carry +// a resourceVersion precondition. +// +// Without it the API server cannot reject the write as conflicting, so the +// surrounding retry.RetryOnConflict never fires and the freshness check inside +// it is unsound. Both the target revision and the re-fetched object come from +// the cache-backed client, so a stale reconcile can compare a stale target +// against an equally stale read, pass the check, and patch the applied revision +// backwards. That re-satisfies the spec > applied comparison in Reconcile and +// sends the controller through the idle and pending runner cleanup again. +// +// The property is asserted on the bytes the production code actually emits +// rather than on an independently constructed patch, so the test cannot pass +// while the reconciler builds its patch some other way. Reverting the patch +// option to a plain client.MergeFrom must fail this test. +func TestPatchAppliedActionableRevisionStatusUsesOptimisticLock(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ers", + Namespace: "default", + }, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 7, + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + AppliedActionableRevision: 2, + }, + } + + var capturedPatch []byte + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ephemeralRunnerSet). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourcePatch: func(ctx context.Context, clt client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + data, err := patch.Data(obj) + if err != nil { + return err + } + capturedPatch = data + return clt.Status().Patch(ctx, obj, patch, opts...) + }, + }). + Build() + + reconciler := &EphemeralRunnerSetReconciler{ + Client: c, + Log: logr.Discard(), + Scheme: scheme, + } + + key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name} + require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 7)) + + require.NotEmpty(t, capturedPatch, "expected the reconciler to emit a status patch") + + var emitted struct { + Metadata struct { + ResourceVersion string `json:"resourceVersion"` + } `json:"metadata"` + Status struct { + AppliedActionableRevision int64 `json:"appliedActionableRevision"` + } `json:"status"` + } + require.NoError(t, json.Unmarshal(capturedPatch, &emitted)) + + assert.NotEmpty( + t, + emitted.Metadata.ResourceVersion, + "status patch must carry a resourceVersion precondition so a stale write is rejected instead of moving the applied revision backwards, got %s", + string(capturedPatch), + ) + assert.Equal(t, int64(7), emitted.Status.AppliedActionableRevision) + + var updated v1alpha1.EphemeralRunnerSet + require.NoError(t, c.Get(context.Background(), key, &updated)) + assert.Equal(t, int64(7), updated.Status.AppliedActionableRevision) +} + +// TestPatchAppliedActionableRevisionStatusDoesNotMoveBackwards covers the guard +// inside the retry: a reconcile carrying an older target revision must leave a +// marker that has already advanced further alone. +func TestPatchAppliedActionableRevisionStatusDoesNotMoveBackwards(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ers", + Namespace: "default", + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + AppliedActionableRevision: 5, + }, + } + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ephemeralRunnerSet). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + Build() + + reconciler := &EphemeralRunnerSetReconciler{ + Client: c, + Log: logr.Discard(), + Scheme: scheme, + } + + key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name} + require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 3)) + + var updated v1alpha1.EphemeralRunnerSet + require.NoError(t, c.Get(context.Background(), key, &updated)) + assert.Equal(t, int64(5), updated.Status.AppliedActionableRevision) +} diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go index 9f2a22c2..7b29446e 100644 --- a/controllers/actions.github.com/helpers.go +++ b/controllers/actions.github.com/helpers.go @@ -1,10 +1,44 @@ 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 +} + // listenerPodSpecRequiresRecreation reports whether the live listener pod must be // deleted and rebuilt to match the desired spec. // diff --git a/controllers/actions.github.com/helpers_bench_test.go b/controllers/actions.github.com/helpers_bench_test.go index 95cc9ca8..50149257 100644 --- a/controllers/actions.github.com/helpers_bench_test.go +++ b/controllers/actions.github.com/helpers_bench_test.go @@ -1,9 +1,112 @@ 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) { 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 00000000..71a9d91d --- /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/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index c78bc200..e5349670 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -864,8 +864,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) } @@ -874,17 +872,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{ diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index 6097308d..e865dd81 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, annotationKeyIntegrityHash) 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.NotEmpty(t, listener.Annotations[annotationKeyIntegrityHash]) assert.Equal(t, autoscalingRunnerSet.Name, listener.Labels[LabelKeyGitHubScaleSetName]) assert.Equal(t, autoscalingRunnerSet.Namespace, listener.Labels[LabelKeyGitHubScaleSetNamespace]) assert.Equal(t, "", listener.Labels[LabelKeyGitHubEnterprise])