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()) +}