From 80a0a64f3f47c6f4a5de93c26908d7dadda253cc Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Thu, 10 Sep 2026 10:33:24 +0200 Subject: [PATCH] Report the pending phase when the listener is rebuilt Moving update detection to metadata.generation lost a signal. The desired listener is built from the AutoscalingRunnerSet's labels and annotations as well as its spec, and any difference there deletes the listener so it can be re-created. metadata.generation only moves on spec writes, so a label-only edit still tore the listener down while the scale set kept reporting Running. If the rebuild then failed, it reported Running with no listener indefinitely. Under the old label-inclusive hash the phase did move, so this was a regression. Mark the resource pending at the point the listener is deleted rather than trying to infer it from the generation. That is what the phase already means: its own doc comment says pending is when the listener is not yet started. It also covers the case properly, because it keys off the actual decision to rebuild instead of guessing from the trigger. This does not change when the listener is rebuilt. A label-only edit recreates it today and still does; only the reported phase changes. The earlier claim that labels no longer restart anything was wrong: labels are propagated to the listener, so editing one is a restart. What metadata.generation changes is narrower than that, and the test now covers the listener's identity across a label edit rather than implying it is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalingrunnerset_controller.go | 19 ++++++++ .../autoscalingrunnerset_controller_test.go | 46 +++++++++++++++++-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 8d28b5a2..d4404243 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -373,6 +373,25 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if !cmp.Equal(listener.Spec, desired.Spec) || !cmp.Equal(listener.Labels, desired.Labels) || !cmp.Equal(listener.Annotations, desired.Annotations) { + // The listener is about to be torn down and rebuilt, which is what + // the pending phase means. Report it here rather than relying on the + // generation check above: the desired listener is derived from the + // AutoscalingRunnerSet's labels and annotations as well as its spec, + // and metadata writes do not bump metadata.generation. Without this, + // a label-only edit would leave the scale set claiming to be running + // while it has no listener at all, and it would keep claiming that + // if the rebuild never succeeded. + if err := r.updateStatus( + ctx, + &autoscalingRunnerSet, + v1alpha1.AutoscalingRunnerSetPhasePending, + autoscalingRunnerSet.Status.ObservedGeneration, + log, + ); err != nil { + log.Error(err, "Failed to update autoscaling runner set status before re-creating the listener") + return ctrl.Result{}, err + } + log.Info("Deleting AutoscalingListener to re-create with updated spec") if err := r.Delete(ctx, &listener); err != nil { log.Error(err, "Failed to delete AutoscalingListener for re-creation") diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index bae467de..eea65b8e 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -527,12 +527,27 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { // longer drags the scale set through the Pending phase the way the old // label-inclusive hash did. Labels still propagate to the // EphemeralRunnerSet, they just do not count as an update to apply. - It("does not re-observe a generation when only labels change", func() { + // A label-only edit does not bump metadata.generation, so it is not an + // update to apply. It does still rebuild the listener, because the + // desired listener labels are derived from the AutoscalingRunnerSet's, + // and the phase has to report that. + It("does not re-observe a generation when only labels change, but still reports the listener rebuild", func() { + listener := new(v1alpha1.AutoscalingListener) + Eventually( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "Listener should be created") + originalListenerUID := listener.UID + 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.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)) g.Expect(current.Status.ObservedGeneration).To(Equal(current.Generation)) settledGeneration = current.Generation }, @@ -554,13 +569,38 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(Succeed()) + // The listener is rebuilt to pick the new label up, so the scale set + // must not keep claiming to be running while that happens. + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingListener) + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current)).To(Succeed()) + g.Expect(current.UID).NotTo(Equal(originalListenerUID), "listener should be re-created to pick up the new label") + g.Expect(current.Labels).To(HaveKeyWithValue("arc.test/label-drift", "updated"), "listener should carry the new label") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) + + // Once the listener is back, the scale set settles on Running again + // without the observed generation ever moving, because no spec write + // happened. + Eventually( + func(g Gomega) { + current := new(v1alpha1.AutoscalingRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) + g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning)) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "AutoscalingRunnerSet should return to Running once the listener is rebuilt") + Consistently( func(g Gomega) { current := new(v1alpha1.AutoscalingRunnerSet) g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(autoscalingRunnerSet), current)).To(Succeed()) g.Expect(current.Generation).To(Equal(settledGeneration), "a label-only edit must not bump metadata.generation") - g.Expect(current.Status.ObservedGeneration).To(Equal(settledGeneration)) - g.Expect(current.Status.Phase).To(Equal(v1alpha1.AutoscalingRunnerSetPhaseRunning), "a label-only edit must not push the scale set back to Pending") + g.Expect(current.Status.ObservedGeneration).To(Equal(settledGeneration), "a label-only edit must not move the observed generation") }, 3*time.Second, autoscalingRunnerSetTestInterval,