From a7ce96b76ce2a34f457156991288e2d11e1907d6 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Thu, 10 Sep 2026 13:35:51 +0200 Subject: [PATCH] Take the reconcile snapshot lazily, right before the first mutation Reconcilers deep copied the object they had just fetched on every single reconcile, purely so a merge patch could be computed on the rare pass that actually changes something. The copy is a full recursive walk and allocation of the object, and the overwhelming majority of reconciles throw it away untouched. Introduce lazyCopy, which takes the snapshot on the first call to Mutate and hands back the live object. Because Mutate is the only way to reach the object, the snapshot cannot be taken after the mutation it is supposed to be diffed against, which is the way this optimization is usually gotten wrong. Apply it to the four Reconcile entry points, and move the two EphemeralRunnerSet status copies inside the branch that patches, so they are only paid for when the status really changed. While here, drop the short circuit in the EphemeralRunner finalizer block: `addedFinalizers || AddFinalizer(...)` skipped adding the actions finalizer whenever the first finalizer was added. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../autoscalinglistener_controller.go | 11 +-- .../autoscalingrunnerset_controller.go | 11 +-- .../ephemeralrunner_controller.go | 28 ++++--- .../ephemeralrunnerset_controller.go | 30 +++++--- controllers/actions.github.com/lazycopy.go | 73 +++++++++++++++++++ .../actions.github.com/lazycopy_test.go | 63 ++++++++++++++++ 6 files changed, 179 insertions(+), 37 deletions(-) create mode 100644 controllers/actions.github.com/lazycopy.go create mode 100644 controllers/actions.github.com/lazycopy_test.go diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 662a9a1a..ebc0b7d8 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -78,7 +78,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. if err := r.Get(ctx, req.NamespacedName, &autoscalingListener); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingListener.DeepCopy() + listener := newLazyCopy(&autoscalingListener) if !autoscalingListener.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { @@ -97,8 +97,8 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { - if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { + if controllerutil.RemoveFinalizer(listener.Mutate(), autoscalingListenerFinalizerName) { + if err := r.Patch(ctx, &autoscalingListener, listener.MergeFrom()); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to remove finalizer") return ctrl.Result{}, err } @@ -109,8 +109,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { - if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil { + if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { + controllerutil.AddFinalizer(listener.Mutate(), autoscalingListenerFinalizerName) + if err := r.Patch(ctx, &autoscalingListener, listener.MergeFrom()); err != nil { log.Error(err, "Failed to add finalizer") return ctrl.Result{}, err } diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 3086f3de..236496a5 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -75,7 +75,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if err := r.Get(ctx, req.NamespacedName, &autoscalingRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingRunnerSet.DeepCopy() + runnerSet := newLazyCopy(&autoscalingRunnerSet) if !autoscalingRunnerSet.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { @@ -100,9 +100,9 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, err } - if controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + if controllerutil.RemoveFinalizer(runnerSet.Mutate(), autoscalingRunnerSetFinalizerName) { log.Info("Removing finalizer") - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { + if err := r.Patch(ctx, &autoscalingRunnerSet, runnerSet.MergeFrom()); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to update autoscaling runner set without finalizer") return ctrl.Result{}, err } @@ -131,10 +131,11 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + controllerutil.AddFinalizer(runnerSet.Mutate(), autoscalingRunnerSetFinalizerName) log.Info("Adding finalizer") - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &autoscalingRunnerSet, runnerSet.MergeFrom()); err != nil { log.Error(err, "Failed to update autoscaling runner set with finalizer") return ctrl.Result{}, err } diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index 256ed311..7608ef8b 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -94,7 +94,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err := r.Get(ctx, req.NamespacedName, &ephemeralRunner); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunner.DeepCopy() + runner := newLazyCopy(&ephemeralRunner) if !ephemeralRunner.DeletionTimestamp.IsZero() { r.publishEphemeralRunnerPhaseMetric(&ephemeralRunner, "", log) @@ -116,9 +116,9 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } log.Info("Runner is cleaned up from the service, removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { + if controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerActionsFinalizerName) { log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); err != nil { log.Error(err, "Failed to update ephemeral runner after removing finalizer") return ctrl.Result{}, err } @@ -143,9 +143,9 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) { + if controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerFinalizerName) { log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); client.IgnoreNotFound(err) != nil { + if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); client.IgnoreNotFound(err) != nil { log.Error(err, "Failed to update ephemeral runner after removing finalizer") return ctrl.Result{}, err } @@ -171,17 +171,15 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - addFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) || !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) - if addFinalizers { + missingFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) || + !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + if missingFinalizers { log.Info("Adding finalizers") - var addedFinalizers bool - addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) - addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) - if addedFinalizers { - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update with finalizer set") - return ctrl.Result{}, err - } + controllerutil.AddFinalizer(runner.Mutate(), ephemeralRunnerFinalizerName) + controllerutil.AddFinalizer(runner.Mutate(), ephemeralRunnerActionsFinalizerName) + if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); err != nil { + log.Error(err, "Failed to update with finalizer set") + return ctrl.Result{}, err } log.Info("Successfully added finalizers") } diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index b772f8d6..ea31a8f5 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -87,7 +87,7 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R if err := r.Get(ctx, req.NamespacedName, &ephemeralRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunnerSet.DeepCopy() + runnerSet := newLazyCopy(&ephemeralRunnerSet) // Requested deletion does not need reconciled. if !ephemeralRunnerSet.DeletionTimestamp.IsZero() { @@ -117,8 +117,8 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R } log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + if controllerutil.RemoveFinalizer(runnerSet.Mutate(), EphemeralRunnerSetFinalizerName) { + if err := r.Patch(ctx, &ephemeralRunnerSet, runnerSet.MergeFrom()); err != nil { log.Error(err, "Failed to update ephemeral runner set with removed finalizer") return ctrl.Result{}, err } @@ -130,9 +130,10 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R } // Add finalizer if not present - if controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { + if !controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { + controllerutil.AddFinalizer(runnerSet.Mutate(), EphemeralRunnerSetFinalizerName) log.Info("Adding finalizer") - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &ephemeralRunnerSet, runnerSet.MergeFrom()); err != nil { log.Error(err, "Failed to update ephemeral runner set with new finalizer") return ctrl.Result{}, err } @@ -353,14 +354,16 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx return err } - original := latest.DeepCopy() + // Build the desired status as a value so no copy of the object is taken + // on the common path where the status already matches. + desiredStatus := latest.Status // Only an advance means the idle and pending runners were just deleted and // the listener restarted. Guarding both writes on it keeps this callable // as a plain "make sure status reflects revision N" without disturbing a // marker that still describes the live patch sequence. if latest.Status.AppliedActionableRevision < targetAppliedRevision { - latest.Status.AppliedActionableRevision = targetAppliedRevision + desiredStatus.AppliedActionableRevision = targetAppliedRevision // The marker records a patch ID from the sequence that was current // before this spec change. Applying a new revision deletes the idle and @@ -370,7 +373,7 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx // through every integer. It therefore passes through a leftover marker // value with near-certainty, and would suppress the very scale up that // rebuilds the pool. - latest.Status.FinishedRunnerCleanupPatchID = 0 + desiredStatus.FinishedRunnerCleanupPatchID = 0 } ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList) @@ -408,17 +411,20 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx // 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 + desiredStatus.Phase = v1alpha1.EphemeralRunnerSetPhaseOutdated } else { - latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + desiredStatus.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning } // 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 { + if latest.Status == desiredStatus { return nil } + original := latest.DeepCopy() + latest.Status = desiredStatus + return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})) }) } @@ -534,7 +540,6 @@ func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(c } func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, state *ephemeralRunnersByState, log logr.Logger) error { - original := ephemeralRunnerSet.DeepCopy() var phase v1alpha1.EphemeralRunnerSetPhase switch { case len(state.outdated) > 0: @@ -552,6 +557,7 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer // Update the status if needed. if ephemeralRunnerSet.Status != desiredStatus { + original := ephemeralRunnerSet.DeepCopy() ephemeralRunnerSet.Status = desiredStatus if err := r.Status().Patch(ctx, ephemeralRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to update EphemeralRunnerSet status") diff --git a/controllers/actions.github.com/lazycopy.go b/controllers/actions.github.com/lazycopy.go new file mode 100644 index 00000000..973032fa --- /dev/null +++ b/controllers/actions.github.com/lazycopy.go @@ -0,0 +1,73 @@ +package actionsgithubcom + +import "sigs.k8s.io/controller-runtime/pkg/client" + +// deepCopyObject is satisfied by every generated API type pointer, as well as +// by the built-in Kubernetes types. +type deepCopyObject[T any] interface { + client.Object + DeepCopy() T +} + +// lazyCopy defers the DeepCopy of a fetched object until the moment it is +// actually about to be mutated. +// +// Reconcilers observe an object far more often than they change it, so taking +// the snapshot up front means paying for a full deep copy on every reconcile +// just to serve the rare patch. lazyCopy pays for it only on the reconciles +// that patch. +// +// The snapshot must be taken before the first mutation, otherwise the merge +// patch is computed against the already mutated object and comes out empty. +// Mutate is the only way to reach the object, which makes that ordering +// impossible to get wrong: +// +// runner := newLazyCopy(&ephemeralRunner) +// if !controllerutil.ContainsFinalizer(&ephemeralRunner, name) { +// controllerutil.AddFinalizer(runner.Mutate(), name) +// } +// if runner.Modified() { +// err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()) +// } +// +// A lazyCopy is not safe for concurrent use. +type lazyCopy[T deepCopyObject[T]] struct { + obj T + original T + copied bool +} + +// newLazyCopy returns a lazyCopy guarding obj. No copy is taken until the +// first call to Mutate. +func newLazyCopy[T deepCopyObject[T]](obj T) *lazyCopy[T] { + return &lazyCopy[T]{obj: obj} +} + +// Mutate snapshots the object on its first call and returns the live object so +// the caller can modify it. Every mutation that a later patch should carry must +// go through Mutate. +func (l *lazyCopy[T]) Mutate() T { + if !l.copied { + l.original = l.obj.DeepCopy() + l.copied = true + } + return l.obj +} + +// Modified reports whether Mutate has been called, and therefore whether there +// is anything to patch. +func (l *lazyCopy[T]) Modified() bool { + return l.copied +} + +// MergeFrom returns a merge patch against the snapshot taken by the first +// Mutate call. It panics when called on an unmodified lazyCopy, because there +// is no snapshot to diff against and the caller would otherwise silently issue +// a patch computed from the live object against itself. Guard it with +// Modified. +func (l *lazyCopy[T]) MergeFrom() client.Patch { + if !l.copied { + panic("lazyCopy: MergeFrom called before Mutate") + } + return client.MergeFrom(l.original) +} diff --git a/controllers/actions.github.com/lazycopy_test.go b/controllers/actions.github.com/lazycopy_test.go new file mode 100644 index 00000000..934ec29e --- /dev/null +++ b/controllers/actions.github.com/lazycopy_test.go @@ -0,0 +1,63 @@ +package actionsgithubcom + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestLazyCopyDoesNotCopyUntilMutated(t *testing.T) { + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod"}} + lazy := newLazyCopy(pod) + + assert.False(t, lazy.Modified()) + assert.Panics(t, func() { lazy.MergeFrom() }) +} + +func TestLazyCopySnapshotsBeforeTheFirstMutation(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Annotations: map[string]string{"key": "old"}, + }, + } + lazy := newLazyCopy(pod) + + lazy.Mutate().Annotations["key"] = "new" + require.True(t, lazy.Modified()) + + data, err := lazy.MergeFrom().Data(pod) + require.NoError(t, err) + assert.JSONEq(t, `{"metadata":{"annotations":{"key":"new"}}}`, string(data)) +} + +func TestLazyCopySnapshotsOnlyOnce(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Annotations: map[string]string{"first": "old", "second": "old"}, + }, + } + lazy := newLazyCopy(pod) + + lazy.Mutate().Annotations["first"] = "new" + // The second mutation must diff against the state before the first one, + // otherwise the earlier change is dropped from the patch. + lazy.Mutate().Annotations["second"] = "new" + + data, err := lazy.MergeFrom().Data(pod) + require.NoError(t, err) + assert.JSONEq(t, `{"metadata":{"annotations":{"first":"new","second":"new"}}}`, string(data)) +} + +func TestLazyCopyMergeFromIsAMergePatch(t *testing.T) { + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod"}} + lazy := newLazyCopy(pod) + lazy.Mutate().Labels = map[string]string{"key": "value"} + + assert.Equal(t, client.MergeFrom(pod).Type(), lazy.MergeFrom().Type()) +}