diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 236496a5..6877dc4a 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 ce903b1a..337653a3 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) phase := ephemeralRunner.Status.Phase if pod.Status.Phase == corev1.PodPending && phase == "" { @@ -961,7 +955,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 ea31a8f5..f58ab0ae 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" @@ -940,7 +941,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 00000000..bd660a96 --- /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 00000000..99d9ecd4 --- /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{}, + })) + }) +}