diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 5b8a664c..3086f3de 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -248,7 +248,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl case err != nil: log.Error(err, "Failed to get ephemeral runner") return ctrl.Result{}, err - case ephemeralRunnerSet.Status.Phase == v1alpha1.EphemeralRunnerSetPhaseOutdated && autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseRunning: + case ephemeralRunnerSetOutdatedForAppliedRevision(&ephemeralRunnerSet) && autoscalingRunnerSet.Status.Phase == v1alpha1.AutoscalingRunnerSetPhaseRunning: // Runners are outdated. We need to stop the listener so it stops getting new jobs. log.Info("Ephemeral runner set is outdated. Cleaning up resources for the outdated runner set") done, err := r.cleanupListener(ctx, &autoscalingRunnerSet, log) diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index 48c78e4b..af05eaf7 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -50,6 +50,12 @@ const ( AnnotationKeyGitHubRunnerGroupName = "actions.github.com/runner-group-name" AnnotationKeyGitHubRunnerScaleSetName = "actions.github.com/runner-scale-set-name" AnnotationKeyPatchID = "actions.github.com/patch-id" + // AnnotationKeyActionableRevision records the EphemeralRunnerSet + // Spec.ActionableRevision that was in effect when the runner was created. It + // lets the set tell apart a runner that reported Outdated against the current + // runner spec from one that reported it against a spec that has since been + // updated. + AnnotationKeyActionableRevision = "actions.github.com/actionable-revision" // 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 diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 2fc505f8..63779e29 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -196,11 +196,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), @@ -208,6 +209,23 @@ 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 { // Spec.Replicas is the count the listener asked for when it published @@ -316,13 +334,32 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx return err } - if latest.Status.AppliedActionableRevision >= targetAppliedRevision { - return nil - } - 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 + } + // The marker records a patch ID from the sequence that was current before // this spec change. Applying a new revision deletes the idle and pending // runners, so the shortfall that follows belongs to the new spec and must @@ -333,6 +370,12 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx // pool. latest.Status.FinishedRunnerCleanupPatchID = 0 + // Checked after every field above has been set, so that clearing the + // marker alone is still enough to issue the patch. + if original.Status == latest.Status { + return nil + } + return r.Status().Patch(ctx, &latest, client.MergeFrom(original)) }) } @@ -496,7 +539,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", @@ -871,12 +914,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 { @@ -898,7 +956,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. // @@ -910,8 +972,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 df3254e7..9e664925 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -2696,10 +2696,16 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { 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, @@ -2756,4 +2762,105 @@ var _ = Describe("Test EphemeralRunnerSet actionable revision cleanup", func() { g.Expect(updatedSet.Status.AppliedActionableRevision).To(Equal(int64(5)), "AppliedActionableRevision should be preserved") }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) }) + + // A runner that reported Outdated against a runner spec that has since been + // replaced must not drag the whole set back into the Outdated phase, because + // that switches the scale set off and discards the update the user just made. + // The runner is deleted instead, so the scaling logic replaces it with one + // built from the current spec. + It("replaces outdated runners from a superseded revision instead of going Outdated", func() { + controller := &EphemeralRunnerSetReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( + fake.WithClient(fake.NewClient()), + )), + }, + } + + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Name: "test-stale-outdated", Namespace: autoscalingNS.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 2, + EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:new"}}}}, + }, + }, + } + Expect(k8sClient.Create(ctx, ephemeralRunnerSet)).To(Succeed()) + + request := ctrl.Request{NamespacedName: types.NamespacedName{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}} + _, err := controller.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + // The set is already running revision 2. + current := new(v1alpha1.EphemeralRunnerSet) + Expect(k8sClient.Get(ctx, request.NamespacedName, current)).To(Succeed()) + statusUpdated := current.DeepCopy() + statusUpdated.Status.AppliedActionableRevision = 2 + statusUpdated.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + Expect(k8sClient.Status().Patch(ctx, statusUpdated, client.MergeFrom(current))).To(Succeed()) + + // A runner left over from revision 1 reports Outdated. This happens when a + // runner was busy with a job while the spec was updated, so it survived the + // revision cleanup and only exited (with the outdated exit code) afterwards. + staleRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner-from-old-revision", + Namespace: autoscalingNS.Name, + Annotations: map[string]string{AnnotationKeyActionableRevision: "1"}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: ephemeralRunnerSet.Name, + UID: ephemeralRunnerSet.UID, + Controller: func(b bool) *bool { return &b }(true), + BlockOwnerDeletion: func(b bool) *bool { return &b }(true), + }, + }, + }, + Spec: v1alpha1.EphemeralRunnerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 100, + PodTemplateSpec: corev1.PodTemplateSpec{Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "runner", Image: "ghcr.io/actions/runner:old"}}}}, + }, + } + Expect(k8sClient.Create(ctx, staleRunner)).To(Succeed()) + + runnerStatusUpdated := staleRunner.DeepCopy() + runnerStatusUpdated.Status.Phase = v1alpha1.EphemeralRunnerPhaseOutdated + Expect(k8sClient.Status().Patch(ctx, runnerStatusUpdated, client.MergeFrom(staleRunner))).To(Succeed()) + + Eventually(func(g Gomega) { + cachedRunner := new(v1alpha1.EphemeralRunner) + g.Expect(controller.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: staleRunner.Name}, cachedRunner)).To(Succeed()) + g.Expect(cachedRunner.Status.Phase).To(Equal(v1alpha1.EphemeralRunnerPhaseOutdated)) + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // The stale runner is removed rather than being treated as a verdict on the + // current spec. + Eventually(func(g Gomega) { + _, err := controller.Reconcile(ctx, request) + g.Expect(err).NotTo(HaveOccurred()) + + runner := new(v1alpha1.EphemeralRunner) + err = k8sClient.Get(ctx, types.NamespacedName{Namespace: autoscalingNS.Name, Name: staleRunner.Name}, runner) + g.Expect(kerrors.IsNotFound(err) || !runner.DeletionTimestamp.IsZero()).To(BeTrue(), "stale outdated runner should be deleted") + }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval).Should(Succeed()) + + // And the set never reports Outdated because of it. + Consistently(func(g Gomega) { + updatedSet := new(v1alpha1.EphemeralRunnerSet) + g.Expect(k8sClient.Get(ctx, request.NamespacedName, updatedSet)).To(Succeed()) + g.Expect(updatedSet.Status.Phase).NotTo(Equal(v1alpha1.EphemeralRunnerSetPhaseOutdated)) + }, "2s", ephemeralRunnerSetTestInterval).Should(Succeed()) + }) }) diff --git a/controllers/actions.github.com/helpers.go b/controllers/actions.github.com/helpers.go index 7b29446e..d0830111 100644 --- a/controllers/actions.github.com/helpers.go +++ b/controllers/actions.github.com/helpers.go @@ -39,6 +39,30 @@ func nextActionableRevision(current *v1alpha1.EphemeralRunnerSet) int64 { return current.Status.AppliedActionableRevision + 1 } +// ephemeralRunnerSetOutdatedForAppliedRevision reports whether the set is +// Outdated *because of the runner spec it is currently running*, which is the +// only situation in which the AutoscalingRunnerSet should tear the scale set +// down. +// +// The phase alone is not enough. Outdated is deliberately sticky: it survives in +// status while the outdated runners are collected, and it is only cleared once a +// new revision is applied. So between the moment the AutoscalingRunnerSet patches +// a new runner spec onto the set and the moment the EphemeralRunnerSet controller +// processes that patch, the set still reports Outdated for a spec that no longer +// exists. Tearing down there would discard the fix the user just applied, and the +// scale set would stay switched off until something else nudged it. +// +// Requiring the applied revision to have caught up with the spec revision closes +// that window: the verdict counts only once the set is running the current spec. +func ephemeralRunnerSetOutdatedForAppliedRevision(ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet) bool { + if ephemeralRunnerSet == nil { + return false + } + + return ephemeralRunnerSet.Status.Phase == v1alpha1.EphemeralRunnerSetPhaseOutdated && + ephemeralRunnerSet.Status.AppliedActionableRevision >= ephemeralRunnerSet.Spec.ActionableRevision +} + // listenerPodSpecRequiresRecreation reports whether the live listener pod must be // deleted and rebuilt to match the desired spec. // diff --git a/controllers/actions.github.com/helpers_outdated_test.go b/controllers/actions.github.com/helpers_outdated_test.go new file mode 100644 index 00000000..19e7f52d --- /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 7cbcd9aa..ea092aab 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -827,9 +827,10 @@ func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.Epheme maps.Copy(labels, ephemeralRunnerSet.Labels) labels[LabelKeyKubernetesComponent] = "runner" - annotations := make(map[string]string, len(ephemeralRunnerSet.Annotations)+1) + annotations := make(map[string]string, len(ephemeralRunnerSet.Annotations)+2) maps.Copy(annotations, ephemeralRunnerSet.Annotations) annotations[AnnotationKeyPatchID] = strconv.Itoa(ephemeralRunnerSet.Spec.PatchID) + annotations[AnnotationKeyActionableRevision] = strconv.FormatInt(ephemeralRunnerSet.Spec.ActionableRevision, 10) if ephemeralRunnerSet.Spec.EphemeralRunnerMetadata != nil { labels = b.filterAndMergeLabels(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Labels, labels)