From a7edbd25d09bf4fc15bb4f2f3204f159779ec6f7 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 9 Sep 2026 11:43:28 +0200 Subject: [PATCH 1/2] Fix EphemeralRunnerSet metadata drift check comparing against the wrong value The AutoscalingRunnerSet controller decides whether the EphemeralRunnerSet's labels and annotations need updating by comparing the live object against desired.Labels and desired.Annotations, but when it acts on that decision it assigns the result of filterAndMergeLabels and mergeAnnotations instead. Those two values only agree when the EphemeralRunnerSet carries nothing beyond what this controller puts there. As soon as anything else writes to the object -- a workload identity injector, a service mesh, an admission webhook, or a person with kubectl -- the merged result keeps the foreign key while the desired map does not, so the comparison reports "modified" forever. Every reconcile then takes that branch, issues a patch that changes nothing, and returns early. The early return is the damaging part: everything after the block, including listener reconciliation, stops running entirely. A listener that is deleted or evicted at that point is never recreated, because the controller can no longer get past the metadata check. Compute the merged labels and annotations first and compare against those, then assign the same values. Foreign metadata is still preserved, and the check reports "modified" only when the merge would actually change something. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalingrunnerset_controller.go | 14 ++-- .../autoscalingrunnerset_controller_test.go | 64 +++++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index b32696a954..ab44125f65 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -310,14 +310,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 { diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index efdd2df14c..e4a14b65af 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -665,6 +665,70 @@ 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 controller must be able to get + // past the metadata block. Before the fix the comparison was made + // against the unmerged desired metadata, so a foreign key made the + // block report "modified" on every reconcile and return early, which + // meant nothing after it — including listener reconciliation — ever + // ran again. Deleting the listener makes that stall observable. + listener := new(v1alpha1.AutoscalingListener) + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener)).To(Succeed()) + Expect(k8sClient.Delete(ctx, listener)).To(Succeed()) + + Eventually( + func(g Gomega) { + recreated := new(v1alpha1.AutoscalingListener) + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, recreated)).To(Succeed()) + + current := new(v1alpha1.EphemeralRunnerSet) + g.Expect(k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current)).To(Succeed()) + 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") + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "reconciliation must converge past the metadata block instead of looping on it") + }) + It("updates EphemeralRunnerSet runner metadata when only EphemeralRunner metadata changes", func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( From 86c974abe8d50d29bdb3ecd85d52882d0262d588 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 9 Sep 2026 18:21:57 +0200 Subject: [PATCH 2/2] Update listener retrieval to use Eventually Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../autoscalingrunnerset_controller_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index e4a14b65af..e122cb737e 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -711,9 +711,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { // meant nothing after it — including listener reconciliation — ever // ran again. Deleting the listener makes that stall observable. listener := new(v1alpha1.AutoscalingListener) - Expect(k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener)).To(Succeed()) + Eventually( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "listener should exist before deletion") Expect(k8sClient.Delete(ctx, listener)).To(Succeed()) - Eventually( func(g Gomega) { recreated := new(v1alpha1.AutoscalingListener)