Use lazy copy to patch resources to ensure multiple modifications are applied to the base resource

.mockery.yaml

@@ -14,4 +14,8 @@ packages:
       all: true
   github.com/actions/actions-runner-controller/controllers/actions.github.com:
     config:
-      all: true
+      all: false
+      include-interface-regex: ".*"
+      # deepCopyObject is a generic type constraint, not a collaborator, so it
+      # must not be mocked.
+      exclude-interface-regex: "^deepCopyObject$"

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
 		}

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
 		}

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")
 	}

controllers/actions.github.com/ephemeralrunnerset_advancing_revision_phase_test.go

@@ -0,0 +1,183 @@
+package actionsgithubcom
+
+import (
+	"context"
+	"testing"
+
+	"github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1"
+	"github.com/go-logr/logr"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+// TestPatchAppliedActionableRevisionStatusClassifiesAgainstTheAdvancedRevision
+// pins the other half of the classifier contract: which revision is used when
+// the monotonicity guard actually fires.
+//
+// TestPatchAppliedActionableRevisionStatusIgnoresATargetBehindTheLiveRevision
+// covers the case where the target has fallen behind the live marker, so the
+// guard does not fire and the applied revision is unchanged. That case cannot
+// observe this one. The guard writes the advance to a copy of the status, so
+// only when it fires do the copy and the stored status hold different
+// revisions, and only then can the classifier be pointed at the wrong one.
+//
+// Reading the pre-advance revision rates a runner left over from the superseded
+// revision as current instead of staleOutdated, which sets the Outdated phase.
+// That phase is then persisted next to the freshly advanced revision, which is
+// the state this function exists to avoid: Reconcile's Outdated branch returns
+// before updateStatus, and this function only runs while spec > applied, so
+// nothing recomputes the phase and the set stays switched off until the next
+// spec change.
+func TestPatchAppliedActionableRevisionStatusClassifiesAgainstTheAdvancedRevision(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	// The spec has moved to revision 2 while the status still records 1, so the
+	// guard below advances the marker and the two revisions differ.
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Spec: v1alpha1.EphemeralRunnerSetSpec{
+			ActionableRevision: 2,
+		},
+		Status: v1alpha1.EphemeralRunnerSetStatus{
+			AppliedActionableRevision: 1,
+			Phase:                     v1alpha1.EphemeralRunnerSetPhaseRunning,
+		},
+	}
+
+	// A runner built from revision 1 that the service rejected. Applying
+	// revision 2 deletes the idle and pending runners, but this one can survive
+	// the cleanup: the cleanup list is read through the cache and can be stale,
+	// which is the case the reviewer of this code identified.
+	//
+	// Its revision equals the pre-advance marker, so classifying against that
+	// value puts it in outdated rather than staleOutdated. Against the advanced
+	// revision it is correctly stale.
+	controllerRef := true
+	supersededRunner := &v1alpha1.EphemeralRunner{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "runner-from-revision-1",
+			Namespace: "default",
+			Annotations: map[string]string{
+				AnnotationKeyActionableRevision: "1",
+			},
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: v1alpha1.GroupVersion.String(),
+					Kind:       "EphemeralRunnerSet",
+					Name:       "test-ers",
+					UID:        "test-uid",
+					Controller: &controllerRef,
+				},
+			},
+		},
+		Status: v1alpha1.EphemeralRunnerStatus{
+			Phase: v1alpha1.EphemeralRunnerPhaseOutdated,
+		},
+	}
+
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet, supersededRunner).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client:    fakeClient,
+		APIReader: fakeClient,
+		Log:       logr.Discard(),
+		Scheme:    scheme,
+	}
+
+	key := types.NamespacedName{Namespace: "default", Name: "test-ers"}
+
+	require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 2))
+
+	var patched v1alpha1.EphemeralRunnerSet
+	require.NoError(t, fakeClient.Get(context.Background(), key, &patched))
+
+	assert.Equal(
+		t,
+		int64(2),
+		patched.Status.AppliedActionableRevision,
+		"the guard must advance the marker to the target revision",
+	)
+
+	assert.Equal(
+		t,
+		v1alpha1.EphemeralRunnerSetPhaseRunning,
+		patched.Status.Phase,
+		"the leftover runner reported Outdated against revision 1, which this call has just superseded, so it must be classified against the advanced revision and ignored: counting it against the pre-advance revision saves the Outdated phase alongside revision 2, and nothing recomputes the phase from there",
+	)
+}
+
+// TestPatchAppliedActionableRevisionStatusDoesNotPatchWhenNothingChanges pins
+// the other half of that contract: the call must issue no patch at all when the
+// status already says what it would set.
+//
+// This is the reason the object is not deep copied up front, and it is decided
+// by whether any mutation was actually made rather than by comparing a
+// separately built status value. Losing it would send an empty patch on every
+// reconcile that reaches this function, and each write bumps the resourceVersion
+// and wakes every watcher, so the regression is invisible in behaviour and
+// visible only in load.
+func TestPatchAppliedActionableRevisionStatusDoesNotPatchWhenNothingChanges(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	// Already applied revision 3, already Running, marker already clear, and no
+	// child runners to move the phase. Every write below is therefore a no-op.
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Status: v1alpha1.EphemeralRunnerSetStatus{
+			AppliedActionableRevision: 3,
+			Phase:                     v1alpha1.EphemeralRunnerSetPhaseRunning,
+		},
+	}
+
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client:    fakeClient,
+		APIReader: fakeClient,
+		Log:       logr.Discard(),
+		Scheme:    scheme,
+	}
+
+	key := types.NamespacedName{Namespace: "default", Name: "test-ers"}
+
+	var before v1alpha1.EphemeralRunnerSet
+	require.NoError(t, fakeClient.Get(context.Background(), key, &before))
+
+	require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 3))
+
+	var after v1alpha1.EphemeralRunnerSet
+	require.NoError(t, fakeClient.Get(context.Background(), key, &after))
+
+	assert.Equal(
+		t,
+		before.ResourceVersion,
+		after.ResourceVersion,
+		"the status already matched, so no patch should have been sent: the resourceVersion moving means an empty patch was written anyway",
+	)
+	assert.Equal(t, before.Status, after.Status)
+}