From 85e367b3c96254b8b43b9d457fa3cbadf3c118fb Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Thu, 10 Sep 2026 13:41:43 +0200 Subject: [PATCH 1/3] Filter owned-resource events in the workqueue Every controller in this package woke up on every update of the resources it owns, including the ones that only touch fields it never reads. An EphemeralRunner status carries readiness, failure bookkeeping and the job details written by the listener, a pod reports IPs, its node, a start time and several conditions, and an EphemeralRunnerSet rewrites Status.FinishedRunnerCleanupPatchID for every listener patch id. None of that is input to the owner, yet all of it enqueued a reconcile. Add an update predicate to each owned watch. A predicate is only allowed to be an optimisation, so each one is written as the projection of the fields its reconciler actually reads and drops an update only when all of them are equal: - AutoscalingRunnerSet -> EphemeralRunnerSet: object metadata, the whole spec, and the Status.Phase and Status.AppliedActionableRevision pair that ephemeralRunnerSetOutdatedForAppliedRevision consults. - EphemeralRunnerSet -> EphemeralRunner: object metadata, spec, Status.Phase and Status.RunnerID. - EphemeralRunner -> Pod: UID, deletion timestamp, pod phase, reason and message, the container and init container statuses, and the Ready condition. An event of an unexpected type is always delivered, and create, delete and generic events are untouched. The primary watches keep seeing every update, so the status patches a reconciler makes to hand work to its own next pass, such as marking a runner Failed or Outdated before cleaning up its resources, still re-enqueue. The Ready condition lookup is extracted into podReady so that the predicate and updateRunStatusFromPod cannot disagree about what readiness means. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalingrunnerset_controller.go | 3 +- .../ephemeralrunner_controller.go | 12 +- .../ephemeralrunnerset_controller.go | 3 +- controllers/actions.github.com/predicates.go | 162 ++++++++++++ .../actions.github.com/predicates_test.go | 243 ++++++++++++++++++ 5 files changed, 412 insertions(+), 11 deletions(-) create mode 100644 controllers/actions.github.com/predicates.go create mode 100644 controllers/actions.github.com/predicates_test.go diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 236496a510..6877dc4aa7 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -36,6 +36,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -888,7 +889,7 @@ func (r *AutoscalingRunnerSetReconciler) SetupWithManager(mgr ctrl.Manager, opts return builderWithOptions( ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.AutoscalingRunnerSet{}). - Owns(&v1alpha1.EphemeralRunnerSet{}). + Owns(&v1alpha1.EphemeralRunnerSet{}, builder.WithPredicates(autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate())). Watches(&v1alpha1.AutoscalingListener{}, handler.EnqueueRequestsFromMapFunc( func(_ context.Context, o client.Object) []reconcile.Request { autoscalingListener := o.(*v1alpha1.AutoscalingListener) diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index 9998793c37..6d519bcf7a 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -36,6 +36,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -843,14 +844,7 @@ func (r *EphemeralRunnerReconciler) updateRunStatusFromPod(ctx context.Context, return nil } - var ready bool - var lastTransitionTime time.Time - for _, condition := range pod.Status.Conditions { - if condition.Type == corev1.PodReady && condition.LastTransitionTime.After(lastTransitionTime) { - ready = condition.Status == corev1.ConditionTrue - lastTransitionTime = condition.LastTransitionTime.Time - } - } + ready := podReady(pod) // Publish Pending as soon as the runner is observed non-terminal, regardless of // the pod phase. The controller only reaches this point once the runner @@ -971,7 +965,7 @@ func (r *EphemeralRunnerReconciler) SetupWithManager(mgr ctrl.Manager, opts ...O return builderWithOptions( ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.EphemeralRunner{}). - Owns(&corev1.Pod{}). + Owns(&corev1.Pod{}, builder.WithPredicates(ephemeralRunnerOwnedPodPredicate())). WithEventFilter(predicate.ResourceVersionChangedPredicate{}), opts, ).Complete(r) diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 26b50d295b..6a7f847d58 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -38,6 +38,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -981,7 +982,7 @@ func (r *EphemeralRunnerSetReconciler) SetupWithManager(mgr ctrl.Manager, opts . return builderWithOptions( ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.EphemeralRunnerSet{}). - Owns(&v1alpha1.EphemeralRunner{}). + Owns(&v1alpha1.EphemeralRunner{}, builder.WithPredicates(ephemeralRunnerSetOwnedEphemeralRunnerPredicate())). WithEventFilter(predicate.ResourceVersionChangedPredicate{}), opts, ).Complete(r) diff --git a/controllers/actions.github.com/predicates.go b/controllers/actions.github.com/predicates.go new file mode 100644 index 0000000000..bd660a96ef --- /dev/null +++ b/controllers/actions.github.com/predicates.go @@ -0,0 +1,162 @@ +package actionsgithubcom + +import ( + "slices" + "time" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +// The predicates in this file exist purely to keep work out of the workqueue. +// They must never change what a reconciler does, so each of them is written as +// the projection of the fields its reconciler actually reads: an update is +// dropped only when every one of those fields is unchanged. Whenever a +// reconciler starts reading a new field, the matching projection below has to +// grow with it. +// +// Create, delete and generic events are always delivered. Only updates are +// considered here. + +// autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate filters updates of the +// EphemeralRunnerSets owned by an AutoscalingRunnerSet. +// +// The AutoscalingRunnerSet reconciler reads the runner set's object metadata +// (labels, annotations, finalizers and deletion timestamp), its whole spec, and, +// through ephemeralRunnerSetOutdatedForAppliedRevision, Status.Phase together +// with Status.AppliedActionableRevision. It never reads +// Status.FinishedRunnerCleanupPatchID, which the EphemeralRunnerSet rewrites for +// every listener patch id and which is the noisiest part of the object. +func autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + oldRunnerSet, oldOk := e.ObjectOld.(*v1alpha1.EphemeralRunnerSet) + newRunnerSet, newOk := e.ObjectNew.(*v1alpha1.EphemeralRunnerSet) + if !oldOk || !newOk { + // Not the type we reasoned about, so we cannot claim the event + // is irrelevant. Let it through. + return true + } + + if !equalReconciledObjectMeta(&oldRunnerSet.ObjectMeta, &newRunnerSet.ObjectMeta) || + !equality.Semantic.DeepEqual(&oldRunnerSet.Spec, &newRunnerSet.Spec) { + return true + } + + return oldRunnerSet.Status.Phase != newRunnerSet.Status.Phase || + oldRunnerSet.Status.AppliedActionableRevision != newRunnerSet.Status.AppliedActionableRevision + }, + } +} + +// ephemeralRunnerSetOwnedEphemeralRunnerPredicate filters updates of the +// EphemeralRunners owned by an EphemeralRunnerSet. +// +// Besides object metadata and spec, the EphemeralRunnerSet reconciler reads +// Status.Phase, to group runners by state, and Status.RunnerID, to decide +// whether a runner still has to be removed from the service. The rest of the +// runner status (readiness, failure bookkeeping, reason, message and the job +// details written by the listener) is never read, and it is by far the noisiest +// part of the object. +func ephemeralRunnerSetOwnedEphemeralRunnerPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + oldRunner, oldOk := e.ObjectOld.(*v1alpha1.EphemeralRunner) + newRunner, newOk := e.ObjectNew.(*v1alpha1.EphemeralRunner) + if !oldOk || !newOk { + return true + } + + if !equalReconciledObjectMeta(&oldRunner.ObjectMeta, &newRunner.ObjectMeta) || + !equality.Semantic.DeepEqual(&oldRunner.Spec, &newRunner.Spec) { + return true + } + + return oldRunner.Status.Phase != newRunner.Status.Phase || + oldRunner.Status.RunnerID != newRunner.Status.RunnerID + }, + } +} + +// ephemeralRunnerOwnedPodPredicate filters updates of the pod owned by an +// EphemeralRunner. +// +// The EphemeralRunner reconciler reads the pod UID, its deletion timestamp, the +// pod phase, reason and message, the container and init container statuses, and +// the Ready condition. It never reads the pod spec or the remaining status +// fields, which is where most pod updates land: assigned IPs, the node the pod +// was scheduled on, start time and the conditions other than Ready. +func ephemeralRunnerOwnedPodPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + oldPod, oldOk := e.ObjectOld.(*corev1.Pod) + newPod, newOk := e.ObjectNew.(*corev1.Pod) + if !oldOk || !newOk { + return true + } + + if oldPod.UID != newPod.UID || + !equalTime(oldPod.DeletionTimestamp, newPod.DeletionTimestamp) { + return true + } + + oldStatus, newStatus := &oldPod.Status, &newPod.Status + if oldStatus.Phase != newStatus.Phase || + oldStatus.Reason != newStatus.Reason || + oldStatus.Message != newStatus.Message { + return true + } + + if !equality.Semantic.DeepEqual(oldStatus.ContainerStatuses, newStatus.ContainerStatuses) || + !equality.Semantic.DeepEqual(oldStatus.InitContainerStatuses, newStatus.InitContainerStatuses) { + return true + } + + return podReady(oldPod) != podReady(newPod) + }, + } +} + +// equalReconciledObjectMeta compares the metadata fields the controllers in this +// package branch on. Bookkeeping the API server owns, such as the resource +// version, the managed fields and the timestamps outside of deletion, is +// deliberately left out. +// podReady reports whether the pod advertises the Ready condition. It is the +// single source of truth for both the reconciler, which mirrors the result into +// EphemeralRunner.Status.Ready, and the pod predicate, which has to wake the +// reconciler whenever the result changes. +func podReady(pod *corev1.Pod) bool { + var ready bool + var lastTransitionTime time.Time + for _, condition := range pod.Status.Conditions { + if condition.Type == corev1.PodReady && condition.LastTransitionTime.After(lastTransitionTime) { + ready = condition.Status == corev1.ConditionTrue + lastTransitionTime = condition.LastTransitionTime.Time + } + } + return ready +} + +func equalReconciledObjectMeta(old, new *metav1.ObjectMeta) bool { + return old.Generation == new.Generation && + equalTime(old.DeletionTimestamp, new.DeletionTimestamp) && + slices.Equal(old.Finalizers, new.Finalizers) && + equality.Semantic.DeepEqual(old.Labels, new.Labels) && + equality.Semantic.DeepEqual(old.Annotations, new.Annotations) && + equality.Semantic.DeepEqual(old.OwnerReferences, new.OwnerReferences) +} + +func equalTime(old, new *metav1.Time) bool { + switch { + case old == nil && new == nil: + return true + case old == nil || new == nil: + return false + default: + return old.Equal(new) + } +} diff --git a/controllers/actions.github.com/predicates_test.go b/controllers/actions.github.com/predicates_test.go new file mode 100644 index 0000000000..99d9ecd49e --- /dev/null +++ b/controllers/actions.github.com/predicates_test.go @@ -0,0 +1,243 @@ +package actionsgithubcom + +import ( + "testing" + "time" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/event" +) + +func TestAutoscalingRunnerSetOwnedEphemeralRunnerSetPredicate(t *testing.T) { + base := func() *v1alpha1.EphemeralRunnerSet { + return &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner-set", + Namespace: "default", + Generation: 1, + Labels: map[string]string{"app": "runner"}, + Annotations: map[string]string{AnnotationKeyPatchID: "1"}, + Finalizers: []string{"finalizer"}, + }, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + Replicas: 2, + PatchID: 3, + ActionableRevision: 4, + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + Phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + }, + } + } + + // Every field the AutoscalingRunnerSet reconciler reads off an owned + // EphemeralRunnerSet has to wake it up. + for name, mutate := range map[string]func(*v1alpha1.EphemeralRunnerSet){ + "replicas": func(s *v1alpha1.EphemeralRunnerSet) { s.Spec.Replicas = 5 }, + "patch id": func(s *v1alpha1.EphemeralRunnerSet) { s.Spec.PatchID = 9 }, + "actionable revision": func(s *v1alpha1.EphemeralRunnerSet) { s.Spec.ActionableRevision = 7 }, + "ephemeral runner spec": func(s *v1alpha1.EphemeralRunnerSet) { + s.Spec.EphemeralRunnerSpec.GitHubConfigURL = "https://github.com/org" + }, + "ephemeral runner meta": func(s *v1alpha1.EphemeralRunnerSet) { + s.Spec.EphemeralRunnerMetadata = &v1alpha1.ResourceMeta{Labels: map[string]string{"a": "b"}} + }, + "labels": func(s *v1alpha1.EphemeralRunnerSet) { s.Labels["app"] = "changed" }, + "annotations": func(s *v1alpha1.EphemeralRunnerSet) { s.Annotations[AnnotationKeyPatchID] = "2" }, + "finalizers": func(s *v1alpha1.EphemeralRunnerSet) { s.Finalizers = nil }, + "deletion timestamp": func(s *v1alpha1.EphemeralRunnerSet) { s.DeletionTimestamp = &metav1.Time{Time: time.Now()} }, + "generation": func(s *v1alpha1.EphemeralRunnerSet) { s.Generation = 2 }, + "owner references": func(s *v1alpha1.EphemeralRunnerSet) { s.OwnerReferences = []metav1.OwnerReference{{Name: "owner"}} }, + } { + t.Run("reconciles on "+name, func(t *testing.T) { + old, updated := base(), base() + mutate(updated) + assert.True(t, autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + } + + t.Run("ignores the finished runner cleanup patch id", func(t *testing.T) { + old, updated := base(), base() + updated.Status.FinishedRunnerCleanupPatchID = 3 + updated.ResourceVersion = "2" + assert.False(t, autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + + t.Run("reconciles on unexpected types", func(t *testing.T) { + assert.True(t, autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate().Update(event.UpdateEvent{ + ObjectOld: &corev1.Pod{}, + ObjectNew: &corev1.Pod{}, + })) + }) + + t.Run("does not filter create, delete or generic events", func(t *testing.T) { + p := autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate() + assert.True(t, p.Create(event.CreateEvent{Object: base()})) + assert.True(t, p.Delete(event.DeleteEvent{Object: base()})) + assert.True(t, p.Generic(event.GenericEvent{Object: base()})) + }) +} + +func TestEphemeralRunnerSetOwnedEphemeralRunnerPredicate(t *testing.T) { + base := func() *v1alpha1.EphemeralRunner { + return &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner", + Namespace: "default", + Generation: 1, + Annotations: map[string]string{ + AnnotationKeyPatchID: "1", + AnnotationKeyActionableRevision: "1", + }, + Finalizers: []string{"finalizer"}, + }, + Status: v1alpha1.EphemeralRunnerStatus{ + Phase: v1alpha1.EphemeralRunnerPhaseRunning, + RunnerID: 42, + }, + } + } + + // The EphemeralRunnerSet reconciler groups runners by phase, reads the patch + // id and actionable revision annotations, and needs the runner id to remove + // the runner from the service. + for name, mutate := range map[string]func(*v1alpha1.EphemeralRunner){ + "phase": func(r *v1alpha1.EphemeralRunner) { r.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded }, + "runner id": func(r *v1alpha1.EphemeralRunner) { r.Status.RunnerID = 43 }, + "patch id annotation": func(r *v1alpha1.EphemeralRunner) { r.Annotations[AnnotationKeyPatchID] = "2" }, + "actionable revision": func(r *v1alpha1.EphemeralRunner) { r.Annotations[AnnotationKeyActionableRevision] = "2" }, + "labels": func(r *v1alpha1.EphemeralRunner) { r.Labels = map[string]string{"a": "b"} }, + "finalizers": func(r *v1alpha1.EphemeralRunner) { r.Finalizers = nil }, + "deletion timestamp": func(r *v1alpha1.EphemeralRunner) { r.DeletionTimestamp = &metav1.Time{Time: time.Now()} }, + "generation": func(r *v1alpha1.EphemeralRunner) { r.Generation = 2 }, + "spec": func(r *v1alpha1.EphemeralRunner) { r.Spec.GitHubConfigURL = "https://github.com/org" }, + } { + t.Run("reconciles on "+name, func(t *testing.T) { + old, updated := base(), base() + mutate(updated) + assert.True(t, ephemeralRunnerSetOwnedEphemeralRunnerPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + } + + t.Run("ignores runner status the runner set never reads", func(t *testing.T) { + old, updated := base(), base() + updated.Status.Ready = true + updated.Status.Reason = "reason" + updated.Status.Message = "message" + updated.Status.RunnerName = "runner-name" + updated.Status.Failures = map[string]metav1.Time{"pod": metav1.Now()} + updated.Status.JobRequestID = 7 + updated.Status.JobID = "job" + updated.Status.JobDisplayName = "display" + updated.Status.JobRepositoryName = "org/repo" + updated.Status.JobWorkflowRef = "ref" + updated.ResourceVersion = "2" + assert.False(t, ephemeralRunnerSetOwnedEphemeralRunnerPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + + t.Run("reconciles on unexpected types", func(t *testing.T) { + assert.True(t, ephemeralRunnerSetOwnedEphemeralRunnerPredicate().Update(event.UpdateEvent{ + ObjectOld: &corev1.Pod{}, + ObjectNew: &corev1.Pod{}, + })) + }) +} + +func TestEphemeralRunnerOwnedPodPredicate(t *testing.T) { + base := func() *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner", + Namespace: "default", + UID: types.UID("uid"), + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Now()), + }, + }, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: v1alpha1.EphemeralRunnerContainerName, + Ready: true, + State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, + }, + }, + InitContainerStatuses: []corev1.ContainerStatus{ + { + Name: "init", + State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 0}}, + }, + }, + }, + } + } + + // Every pod field the EphemeralRunner reconciler branches on has to wake it up. + for name, mutate := range map[string]func(*corev1.Pod){ + "phase": func(p *corev1.Pod) { p.Status.Phase = corev1.PodFailed }, + "reason": func(p *corev1.Pod) { p.Status.Reason = "Evicted" }, + "message": func(p *corev1.Pod) { p.Status.Message = "evicted" }, + "uid": func(p *corev1.Pod) { p.UID = types.UID("other") }, + "deletion timestamp": func(p *corev1.Pod) { + p.DeletionTimestamp = &metav1.Time{Time: time.Now()} + }, + "runner container terminated": func(p *corev1.Pod) { + p.Status.ContainerStatuses[0].State = corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 1}} + }, + "runner container exit code": func(p *corev1.Pod) { + p.Status.ContainerStatuses[0].State = corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 7}} + }, + "runner container readiness": func(p *corev1.Pod) { + p.Status.ContainerStatuses[0].Ready = false + }, + "init container status": func(p *corev1.Pod) { + p.Status.InitContainerStatuses[0].State = corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 1}} + }, + "ready condition": func(p *corev1.Pod) { + p.Status.Conditions[0].Status = corev1.ConditionFalse + }, + } { + t.Run("reconciles on "+name, func(t *testing.T) { + old, updated := base(), base() + mutate(updated) + assert.True(t, ephemeralRunnerOwnedPodPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + } + + t.Run("ignores pod noise the runner never reads", func(t *testing.T) { + old, updated := base(), base() + updated.ResourceVersion = "2" + updated.Labels = map[string]string{"a": "b"} + updated.Status.PodIP = "10.0.0.1" + updated.Status.PodIPs = []corev1.PodIP{{IP: "10.0.0.1"}} + updated.Status.HostIP = "10.0.0.2" + updated.Status.StartTime = &metav1.Time{Time: time.Now()} + updated.Status.Conditions = append(updated.Status.Conditions, + corev1.PodCondition{Type: corev1.PodScheduled, Status: corev1.ConditionTrue}, + corev1.PodCondition{Type: corev1.ContainersReady, Status: corev1.ConditionTrue}, + ) + assert.False(t, ephemeralRunnerOwnedPodPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + + t.Run("ready condition transition time alone does not reconcile", func(t *testing.T) { + old, updated := base(), base() + updated.Status.Conditions[0].LastTransitionTime = metav1.NewTime(time.Now().Add(time.Hour)) + assert.False(t, ephemeralRunnerOwnedPodPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + + t.Run("reconciles on unexpected types", func(t *testing.T) { + assert.True(t, ephemeralRunnerOwnedPodPredicate().Update(event.UpdateEvent{ + ObjectOld: &v1alpha1.EphemeralRunner{}, + ObjectNew: &v1alpha1.EphemeralRunner{}, + })) + }) +} From 5c6b1de3c1ad28b3f283d46b8ae0948d8ff6622b Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Mon, 14 Sep 2026 14:19:24 +0200 Subject: [PATCH 2/3] Wake the runner set on EphemeralRunner job id changes The EphemeralRunnerSet reconciler skips runners that are busy serving a job, and it decides that through HasJob, which reads Status.JobID. The owned EphemeralRunner predicate projected only Status.Phase and Status.RunnerID, so a job id change that did not come with a phase change was dropped before it reached the workqueue and the runner set kept acting on a stale answer. Add the job id to the projection and correct the doc comment, which claimed the job details are never read. The predicate tables never mutated EphemeralRunnerSet Status.Phase or Status.AppliedActionableRevision, so a regression in either comparison passed them. Cover both, and pin the status field names so that a field added to either type fails until somebody classifies it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- controllers/actions.github.com/predicates.go | 10 ++-- .../actions.github.com/predicates_test.go | 57 ++++++++++++++++++- 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/controllers/actions.github.com/predicates.go b/controllers/actions.github.com/predicates.go index bd660a96ef..df31171da7 100644 --- a/controllers/actions.github.com/predicates.go +++ b/controllers/actions.github.com/predicates.go @@ -57,9 +57,10 @@ func autoscalingRunnerSetOwnedEphemeralRunnerSetPredicate() predicate.Predicate // EphemeralRunners owned by an EphemeralRunnerSet. // // Besides object metadata and spec, the EphemeralRunnerSet reconciler reads -// Status.Phase, to group runners by state, and Status.RunnerID, to decide -// whether a runner still has to be removed from the service. The rest of the -// runner status (readiness, failure bookkeeping, reason, message and the job +// Status.Phase, to group runners by state, Status.RunnerID, to decide whether a +// runner still has to be removed from the service, and Status.JobID, through +// HasJob, to skip runners that are busy serving a job. The rest of the runner +// status (readiness, failure bookkeeping, reason, message and the remaining job // details written by the listener) is never read, and it is by far the noisiest // part of the object. func ephemeralRunnerSetOwnedEphemeralRunnerPredicate() predicate.Predicate { @@ -77,7 +78,8 @@ func ephemeralRunnerSetOwnedEphemeralRunnerPredicate() predicate.Predicate { } return oldRunner.Status.Phase != newRunner.Status.Phase || - oldRunner.Status.RunnerID != newRunner.Status.RunnerID + oldRunner.Status.RunnerID != newRunner.Status.RunnerID || + oldRunner.Status.JobID != newRunner.Status.JobID }, } } diff --git a/controllers/actions.github.com/predicates_test.go b/controllers/actions.github.com/predicates_test.go index 99d9ecd49e..f5b2d76099 100644 --- a/controllers/actions.github.com/predicates_test.go +++ b/controllers/actions.github.com/predicates_test.go @@ -1,6 +1,8 @@ package actionsgithubcom import ( + "reflect" + "sort" "testing" "time" @@ -52,6 +54,12 @@ func TestAutoscalingRunnerSetOwnedEphemeralRunnerSetPredicate(t *testing.T) { "deletion timestamp": func(s *v1alpha1.EphemeralRunnerSet) { s.DeletionTimestamp = &metav1.Time{Time: time.Now()} }, "generation": func(s *v1alpha1.EphemeralRunnerSet) { s.Generation = 2 }, "owner references": func(s *v1alpha1.EphemeralRunnerSet) { s.OwnerReferences = []metav1.OwnerReference{{Name: "owner"}} }, + "phase": func(s *v1alpha1.EphemeralRunnerSet) { + s.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseOutdated + }, + "applied actionable revision": func(s *v1alpha1.EphemeralRunnerSet) { + s.Status.AppliedActionableRevision = 9 + }, } { t.Run("reconciles on "+name, func(t *testing.T) { old, updated := base(), base() @@ -108,6 +116,7 @@ func TestEphemeralRunnerSetOwnedEphemeralRunnerPredicate(t *testing.T) { for name, mutate := range map[string]func(*v1alpha1.EphemeralRunner){ "phase": func(r *v1alpha1.EphemeralRunner) { r.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded }, "runner id": func(r *v1alpha1.EphemeralRunner) { r.Status.RunnerID = 43 }, + "job id": func(r *v1alpha1.EphemeralRunner) { r.Status.JobID = "job" }, "patch id annotation": func(r *v1alpha1.EphemeralRunner) { r.Annotations[AnnotationKeyPatchID] = "2" }, "actionable revision": func(r *v1alpha1.EphemeralRunner) { r.Annotations[AnnotationKeyActionableRevision] = "2" }, "labels": func(r *v1alpha1.EphemeralRunner) { r.Labels = map[string]string{"a": "b"} }, @@ -131,7 +140,6 @@ func TestEphemeralRunnerSetOwnedEphemeralRunnerPredicate(t *testing.T) { updated.Status.RunnerName = "runner-name" updated.Status.Failures = map[string]metav1.Time{"pod": metav1.Now()} updated.Status.JobRequestID = 7 - updated.Status.JobID = "job" updated.Status.JobDisplayName = "display" updated.Status.JobRepositoryName = "org/repo" updated.Status.JobWorkflowRef = "ref" @@ -241,3 +249,50 @@ func TestEphemeralRunnerOwnedPodPredicate(t *testing.T) { })) }) } + +// The predicates above are projections of the status fields their reconcilers +// read, so a field added to either status has to be classified: either it wakes +// the reconciler up and belongs in the tables above, or it is deliberately +// ignored. Nothing else forces that decision, so the field names are pinned +// here and adding one fails until somebody updates this list and the tables. +// +// The pin is a tripwire on the type, not a proof of correspondence: it cannot +// tell whether a reconciler started branching on a field the matching predicate +// still drops. Only the tables above assert the behaviour. +func TestPredicateProjectionsCoverEveryStatusField(t *testing.T) { + fieldNames := func(v any) []string { + typ := reflect.TypeOf(v) + names := make([]string, 0, typ.NumField()) + for i := 0; i < typ.NumField(); i++ { + names = append(names, typ.Field(i).Name) + } + sort.Strings(names) + return names + } + + t.Run("ephemeral runner set status", func(t *testing.T) { + assert.Equal(t, []string{ + "AppliedActionableRevision", + "FinishedRunnerCleanupPatchID", + "Phase", + }, fieldNames(v1alpha1.EphemeralRunnerSetStatus{})) + }) + + t.Run("ephemeral runner status", func(t *testing.T) { + assert.Equal(t, []string{ + "Failures", + "JobDisplayName", + "JobID", + "JobRepositoryName", + "JobRequestID", + "JobWorkflowRef", + "Message", + "Phase", + "Ready", + "Reason", + "RunnerID", + "RunnerName", + "WorkflowRunID", + }, fieldNames(v1alpha1.EphemeralRunnerStatus{})) + }) +} From 19cf95e3e41b40f00c1382456f87e8eb636f8e1b Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Mon, 14 Sep 2026 21:02:11 +0200 Subject: [PATCH 3/3] Filter listener pod and service account events too The AutoscalingListener controller owns a pod and a service account, and both watches were still unfiltered, so the optimization stopped short of the one controller that watches the noisiest object of the three. Project what the reconciler actually reads. Off the listener pod that is the object metadata, which it merges labels and annotations back onto and where it compares the listener config resource version, the whole spec, which listenerPodSpecRequiresRecreation compares, and, off the status, only the phase, reason and message, plus the container statuses it finds the listener container in. Assigned IPs, the node, the start time, the conditions and the init container statuses are dropped. Off the service account only the metadata is read, so everything the token controller writes, the mounted secrets above all, is dropped. Also add WorkflowRunID to the EphemeralRunner noise case. It is pinned by TestPredicateProjectionsCoverEveryStatusField but was never mutated, so the suite would have stayed green if the predicate started comparing it. Every new assertion was verified to bite by disabling the matching comparison and confirming only the expected subtests fail. One of those mutations first reported zero failures because it stopped the package compiling and nothing ran at all, so the runs below assert the subtest count as well as the failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalinglistener_controller.go | 5 +- controllers/actions.github.com/predicates.go | 66 +++++++- .../actions.github.com/predicates_test.go | 147 ++++++++++++++++++ 3 files changed, 212 insertions(+), 6 deletions(-) diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index ebc0b7d841..4e9219d584 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -873,8 +874,8 @@ func (r *AutoscalingListenerReconciler) SetupWithManager(mgr ctrl.Manager, opts return builderWithOptions( ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.AutoscalingListener{}). - Owns(&corev1.Pod{}). - Owns(&corev1.ServiceAccount{}). + Owns(&corev1.Pod{}, builder.WithPredicates(autoscalingListenerOwnedPodPredicate())). + Owns(&corev1.ServiceAccount{}, builder.WithPredicates(autoscalingListenerOwnedServiceAccountPredicate())). Watches(&rbacv1.Role{}, handler.EnqueueRequestsFromMapFunc(labelBasedWatchFunc)). Watches(&rbacv1.RoleBinding{}, handler.EnqueueRequestsFromMapFunc(labelBasedWatchFunc)). WithEventFilter(predicate.ResourceVersionChangedPredicate{}), diff --git a/controllers/actions.github.com/predicates.go b/controllers/actions.github.com/predicates.go index df31171da7..4f00c828d8 100644 --- a/controllers/actions.github.com/predicates.go +++ b/controllers/actions.github.com/predicates.go @@ -123,10 +123,64 @@ func ephemeralRunnerOwnedPodPredicate() predicate.Predicate { } } -// equalReconciledObjectMeta compares the metadata fields the controllers in this -// package branch on. Bookkeeping the API server owns, such as the resource -// version, the managed fields and the timestamps outside of deletion, is -// deliberately left out. +// autoscalingListenerOwnedPodPredicate filters updates of the listener pod +// owned by an AutoscalingListener. +// +// The AutoscalingListener reconciler reads the pod's object metadata, because it +// merges labels and annotations back onto the pod and compares the listener +// config resource version annotation, and its whole spec, through +// listenerPodSpecRequiresRecreation. Off the status it reads only the phase, +// reason and message, to detect eviction, and the container statuses, to find +// the listener container and branch on whether it is running or terminated. The +// rest of the pod status is never read: assigned IPs, the node the pod landed +// on, start time, the conditions and the init container statuses. +func autoscalingListenerOwnedPodPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + oldPod, oldOk := e.ObjectOld.(*corev1.Pod) + newPod, newOk := e.ObjectNew.(*corev1.Pod) + if !oldOk || !newOk { + return true + } + + if !equalReconciledObjectMeta(&oldPod.ObjectMeta, &newPod.ObjectMeta) || + !equality.Semantic.DeepEqual(&oldPod.Spec, &newPod.Spec) { + return true + } + + oldStatus, newStatus := &oldPod.Status, &newPod.Status + if oldStatus.Phase != newStatus.Phase || + oldStatus.Reason != newStatus.Reason || + oldStatus.Message != newStatus.Message { + return true + } + + return !equality.Semantic.DeepEqual(oldStatus.ContainerStatuses, newStatus.ContainerStatuses) + }, + } +} + +// autoscalingListenerOwnedServiceAccountPredicate filters updates of the service +// account owned by an AutoscalingListener. +// +// The AutoscalingListener reconciler only ever reads the service account's +// labels and annotations, which it merges back onto the object. Everything else +// the API server and the token controller write to it, the mounted secrets +// above all, is never read. +func autoscalingListenerOwnedServiceAccountPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + oldServiceAccount, oldOk := e.ObjectOld.(*corev1.ServiceAccount) + newServiceAccount, newOk := e.ObjectNew.(*corev1.ServiceAccount) + if !oldOk || !newOk { + return true + } + + return !equalReconciledObjectMeta(&oldServiceAccount.ObjectMeta, &newServiceAccount.ObjectMeta) + }, + } +} + // podReady reports whether the pod advertises the Ready condition. It is the // single source of truth for both the reconciler, which mirrors the result into // EphemeralRunner.Status.Ready, and the pod predicate, which has to wake the @@ -143,6 +197,10 @@ func podReady(pod *corev1.Pod) bool { return ready } +// equalReconciledObjectMeta compares the metadata fields the controllers in this +// package branch on. Bookkeeping the API server owns, such as the resource +// version, the managed fields and the timestamps outside of deletion, is +// deliberately left out. func equalReconciledObjectMeta(old, new *metav1.ObjectMeta) bool { return old.Generation == new.Generation && equalTime(old.DeletionTimestamp, new.DeletionTimestamp) && diff --git a/controllers/actions.github.com/predicates_test.go b/controllers/actions.github.com/predicates_test.go index f5b2d76099..72aea8ef82 100644 --- a/controllers/actions.github.com/predicates_test.go +++ b/controllers/actions.github.com/predicates_test.go @@ -143,6 +143,7 @@ func TestEphemeralRunnerSetOwnedEphemeralRunnerPredicate(t *testing.T) { updated.Status.JobDisplayName = "display" updated.Status.JobRepositoryName = "org/repo" updated.Status.JobWorkflowRef = "ref" + updated.Status.WorkflowRunID = 12 updated.ResourceVersion = "2" assert.False(t, ephemeralRunnerSetOwnedEphemeralRunnerPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) }) @@ -250,6 +251,152 @@ func TestEphemeralRunnerOwnedPodPredicate(t *testing.T) { }) } +func TestAutoscalingListenerOwnedPodPredicate(t *testing.T) { + base := func() *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "default", + Generation: 1, + Labels: map[string]string{"app": "listener"}, + Annotations: map[string]string{AnnotationKeyListenerConfigResourceVersion: "1"}, + }, + Spec: corev1.PodSpec{ + ServiceAccountName: "listener", + Containers: []corev1.Container{ + { + Name: autoscalingListenerContainerName, + Image: "listener:1", + }, + }, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: autoscalingListenerContainerName, + State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, + }, + }, + }, + } + } + + // Every listener pod field the AutoscalingListener reconciler branches on has + // to wake it up. + for name, mutate := range map[string]func(*corev1.Pod){ + "phase": func(p *corev1.Pod) { p.Status.Phase = corev1.PodFailed }, + "reason": func(p *corev1.Pod) { p.Status.Reason = "Evicted" }, + "message": func(p *corev1.Pod) { p.Status.Message = "evicted" }, + "labels": func(p *corev1.Pod) { p.Labels["app"] = "changed" }, + "listener config resource version": func(p *corev1.Pod) { + p.Annotations[AnnotationKeyListenerConfigResourceVersion] = "2" + }, + "deletion timestamp": func(p *corev1.Pod) { + p.DeletionTimestamp = &metav1.Time{Time: time.Now()} + }, + "generation": func(p *corev1.Pod) { p.Generation = 2 }, + "owner references": func(p *corev1.Pod) { p.OwnerReferences = []metav1.OwnerReference{{Name: "owner"}} }, + "finalizers": func(p *corev1.Pod) { p.Finalizers = []string{"finalizer"} }, + "image": func(p *corev1.Pod) { p.Spec.Containers[0].Image = "listener:2" }, + "container ports": func(p *corev1.Pod) { + p.Spec.Containers[0].Ports = []corev1.ContainerPort{{ContainerPort: 8080}} + }, + "service account name": func(p *corev1.Pod) { p.Spec.ServiceAccountName = "other" }, + "listener container terminated": func(p *corev1.Pod) { + p.Status.ContainerStatuses[0].State = corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 1}} + }, + } { + t.Run("reconciles on "+name, func(t *testing.T) { + old, updated := base(), base() + mutate(updated) + assert.True(t, autoscalingListenerOwnedPodPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + } + + t.Run("ignores pod noise the listener never reads", func(t *testing.T) { + old, updated := base(), base() + updated.ResourceVersion = "2" + updated.Status.PodIP = "10.0.0.1" + updated.Status.PodIPs = []corev1.PodIP{{IP: "10.0.0.1"}} + updated.Status.HostIP = "10.0.0.2" + updated.Status.StartTime = &metav1.Time{Time: time.Now()} + updated.Status.NominatedNodeName = "node" + updated.Status.QOSClass = corev1.PodQOSBestEffort + updated.Status.Conditions = []corev1.PodCondition{ + {Type: corev1.PodReady, Status: corev1.ConditionTrue}, + {Type: corev1.PodScheduled, Status: corev1.ConditionTrue}, + } + updated.Status.InitContainerStatuses = []corev1.ContainerStatus{ + {Name: "init", State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 0}}}, + } + assert.False(t, autoscalingListenerOwnedPodPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + + t.Run("reconciles on unexpected types", func(t *testing.T) { + assert.True(t, autoscalingListenerOwnedPodPredicate().Update(event.UpdateEvent{ + ObjectOld: &v1alpha1.AutoscalingListener{}, + ObjectNew: &v1alpha1.AutoscalingListener{}, + })) + }) + + t.Run("does not filter create, delete or generic events", func(t *testing.T) { + p := autoscalingListenerOwnedPodPredicate() + assert.True(t, p.Create(event.CreateEvent{Object: base()})) + assert.True(t, p.Delete(event.DeleteEvent{Object: base()})) + assert.True(t, p.Generic(event.GenericEvent{Object: base()})) + }) +} + +func TestAutoscalingListenerOwnedServiceAccountPredicate(t *testing.T) { + base := func() *corev1.ServiceAccount { + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "default", + Generation: 1, + Labels: map[string]string{"app": "listener"}, + Annotations: map[string]string{"annotation": "1"}, + }, + } + } + + // The reconciler merges labels and annotations back onto the service account, + // so a change to either has to wake it up. + for name, mutate := range map[string]func(*corev1.ServiceAccount){ + "labels": func(sa *corev1.ServiceAccount) { sa.Labels["app"] = "changed" }, + "annotations": func(sa *corev1.ServiceAccount) { sa.Annotations["annotation"] = "2" }, + "deletion timestamp": func(sa *corev1.ServiceAccount) { + sa.DeletionTimestamp = &metav1.Time{Time: time.Now()} + }, + "finalizers": func(sa *corev1.ServiceAccount) { sa.Finalizers = []string{"finalizer"} }, + "owner references": func(sa *corev1.ServiceAccount) { sa.OwnerReferences = []metav1.OwnerReference{{Name: "owner"}} }, + } { + t.Run("reconciles on "+name, func(t *testing.T) { + old, updated := base(), base() + mutate(updated) + assert.True(t, autoscalingListenerOwnedServiceAccountPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + } + + t.Run("ignores service account noise the listener never reads", func(t *testing.T) { + old, updated := base(), base() + updated.ResourceVersion = "2" + updated.Secrets = []corev1.ObjectReference{{Name: "listener-token"}} + updated.ImagePullSecrets = []corev1.LocalObjectReference{{Name: "pull"}} + automount := true + updated.AutomountServiceAccountToken = &automount + assert.False(t, autoscalingListenerOwnedServiceAccountPredicate().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated})) + }) + + t.Run("reconciles on unexpected types", func(t *testing.T) { + assert.True(t, autoscalingListenerOwnedServiceAccountPredicate().Update(event.UpdateEvent{ + ObjectOld: &corev1.Pod{}, + ObjectNew: &corev1.Pod{}, + })) + }) +} + // The predicates above are projections of the status fields their reconcilers // read, so a field added to either status has to be classified: either it wakes // the reconciler up and belongs in the tables above, or it is deliberately