Merge remote-tracking branch 'origin/nikola-jokic-ers-scale-up-after-cleanup' into nikola-jokic-remove-integrity-hash-annotation

controllers/actions.github.com/ephemeralrunnerset_cleanup_patch_test.go

@@ -0,0 +1,145 @@
+package actionsgithubcom
+
+import (
+	"context"
+	"encoding/json"
+	"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"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
+)
+
+// TestPatchFinishedRunnerCleanupPatchIDStatusUsesOptimisticLock pins the
+// resourceVersion precondition on the cleanup marker patch.
+//
+// The marker decides whether a shortfall below Spec.Replicas is suppressed, so a
+// write that lands on the wrong patch ID re-enables the spurious scale up this
+// layer exists to prevent. The helper sets the marker to whatever patch ID the
+// reconcile is carrying rather than only ever advancing it, so without the lock
+// the API server cannot reject a stale write, retry.RetryOnConflict never fires,
+// and a reconcile serving an older patch ID can overwrite a marker recorded for
+// a newer one.
+//
+// Asserted on the bytes the production code emits rather than on an
+// independently built patch, so it cannot pass while the reconciler constructs
+// its patch some other way. Reverting the option to a plain client.MergeFrom
+// must fail this test.
+func TestPatchFinishedRunnerCleanupPatchIDStatusUsesOptimisticLock(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Spec: v1alpha1.EphemeralRunnerSetSpec{
+			PatchID: 4,
+		},
+	}
+
+	var capturedPatch []byte
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		WithInterceptorFuncs(interceptor.Funcs{
+			SubResourcePatch: func(ctx context.Context, clt client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
+				data, err := patch.Data(obj)
+				if err != nil {
+					return err
+				}
+				capturedPatch = data
+				return clt.Status().Patch(ctx, obj, patch, opts...)
+			},
+		}).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client:    c,
+		APIReader: c,
+		Log:       logr.Discard(),
+		Scheme:    scheme,
+	}
+
+	key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name}
+	require.NoError(t, reconciler.patchFinishedRunnerCleanupPatchIDStatus(context.Background(), key, 4))
+
+	require.NotEmpty(t, capturedPatch, "expected the reconciler to emit a status patch")
+
+	var emitted struct {
+		Metadata struct {
+			ResourceVersion string `json:"resourceVersion"`
+		} `json:"metadata"`
+		Status struct {
+			FinishedRunnerCleanupPatchID int `json:"finishedRunnerCleanupPatchID"`
+		} `json:"status"`
+	}
+	require.NoError(t, json.Unmarshal(capturedPatch, &emitted))
+
+	assert.NotEmpty(
+		t,
+		emitted.Metadata.ResourceVersion,
+		"status patch must carry a resourceVersion precondition so a stale write is rejected instead of recording the wrong patch ID, got %s",
+		string(capturedPatch),
+	)
+	assert.Equal(t, 4, emitted.Status.FinishedRunnerCleanupPatchID)
+
+	var updated v1alpha1.EphemeralRunnerSet
+	require.NoError(t, c.Get(context.Background(), key, &updated))
+	assert.Equal(t, 4, updated.Status.FinishedRunnerCleanupPatchID)
+}
+
+// TestPatchFinishedRunnerCleanupPatchIDStatusIsIdempotent covers the early
+// return: a marker already recording this patch ID must not be rewritten, so
+// repeated reconciles for one patch do not churn the status.
+func TestPatchFinishedRunnerCleanupPatchIDStatusIsIdempotent(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Status: v1alpha1.EphemeralRunnerSetStatus{
+			FinishedRunnerCleanupPatchID: 4,
+		},
+	}
+
+	patched := false
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		WithInterceptorFuncs(interceptor.Funcs{
+			SubResourcePatch: func(ctx context.Context, clt client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
+				patched = true
+				return clt.Status().Patch(ctx, obj, patch, opts...)
+			},
+		}).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client:    c,
+		APIReader: c,
+		Log:       logr.Discard(),
+		Scheme:    scheme,
+	}
+
+	key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name}
+	require.NoError(t, reconciler.patchFinishedRunnerCleanupPatchIDStatus(context.Background(), key, 4))
+
+	assert.False(t, patched, "recording a patch ID the marker already holds must not emit a write")
+}

controllers/actions.github.com/ephemeralrunnerset_controller.go

@@ -217,14 +217,14 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 		// patch ID the cleanup belongs to and return, leaving the scaling decision
 		// to the next reconcile, which sees the post-cleanup state.
 		if len(ephemeralRunnersByState.finished) > 0 {
-			if err := r.deleteTerminatedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil {
-				log.Error(err, "failed to delete terminated ephemeral runners")
-				return ctrl.Result{}, err
-			}
 			if err := r.patchFinishedRunnerCleanupPatchIDStatus(ctx, req.NamespacedName, ephemeralRunnerSet.Spec.PatchID); err != nil {
 				log.Error(err, "failed to update finished runner cleanup patch ID status")
 				return ctrl.Result{}, err
 			}
+			if err := r.deleteTerminatedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil {
+				log.Error(err, "failed to delete terminated ephemeral runners")
+				return ctrl.Result{}, err
+			}
 			ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID = ephemeralRunnerSet.Spec.PatchID
 
 			log.Info("Finished ephemeral runners were cleaned up, deferring scaling decision")
@@ -305,6 +305,16 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 // object that was read. A cached read that still showed the field as 0 while the
 // API server held a recorded marker would produce a patch with no entry for the
 // field, silently leaving the stale marker in place.
+//
+// The patch carries an optimistic lock so that the re-fetch actually means
+// something. A plain merge patch has no resourceVersion precondition, so the API
+// server can never reject it as conflicting: RetryOnConflict would never fire,
+// and a patch computed from a stale read could move the applied revision
+// backwards, re-satisfying the spec > applied comparison above and deleting the
+// idle runners all over again. With the lock, the server accepts the write only
+// if the re-fetched object is still the live one, so a successful patch proves
+// the monotonicity check above was evaluated against live data. A stale attempt
+// conflicts and is retried or requeued instead of regressing the marker.
 func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error {
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		var latest v1alpha1.EphemeralRunnerSet
@@ -333,7 +343,7 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx
 		// pool.
 		latest.Status.FinishedRunnerCleanupPatchID = 0
 
-		return r.Status().Patch(ctx, &latest, client.MergeFrom(original))
+		return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{}))
 	})
 }
 
@@ -392,6 +402,21 @@ func (r *EphemeralRunnerSetReconciler) scaleUpServicedByFinishedRunnerCleanup(ct
 // Like the applied revision above, this is written after the deletions succeed
 // and re-fetches the object inside the retry, so a conflicting write is never
 // resolved by replaying a status that predates the cleanup.
+//
+// The patch carries an optimistic lock for the same reason, and the exposure
+// here is if anything worse: the check below is an equality test rather than a
+// monotonicity test, so this helper is willing to move the marker to whatever
+// patch ID the reconcile is carrying, including backwards. Without a
+// resourceVersion precondition the API server cannot reject the write, so
+// RetryOnConflict can never fire and a reconcile serving an older patch ID can
+// overwrite a marker recorded for a newer one. The guard would then stop
+// suppressing for the patch ID that was actually serviced, and the controller
+// would create the replacement runners this layer exists to prevent.
+//
+// Re-fetching through the API reader narrows that window to the gap between the
+// read and the patch rather than closing it, because the decision is only as
+// fresh as the moment it was taken. The lock is what makes the write conditional
+// on that decision still holding.
 func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(ctx context.Context, key types.NamespacedName, patchID int) error {
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		var latest v1alpha1.EphemeralRunnerSet
@@ -410,7 +435,7 @@ func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(c
 		original := latest.DeepCopy()
 		latest.Status.FinishedRunnerCleanupPatchID = patchID
 
-		return r.Status().Patch(ctx, &latest, client.MergeFrom(original))
+		return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{}))
 	})
 }
 

controllers/actions.github.com/ephemeralrunnerset_controller_test.go

@@ -627,7 +627,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() {
 
 			// confirm they are not deleted
 			runnerList = new(v1alpha1.EphemeralRunnerList)
-			Eventually(
+			Consistently(
 				func() (int, error) {
 					err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace)
 					if err != nil {

controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go

@@ -0,0 +1,142 @@
+package actionsgithubcom
+
+import (
+	"context"
+	"encoding/json"
+	"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"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
+)
+
+// TestPatchAppliedActionableRevisionStatusUsesOptimisticLock pins the property
+// that makes the applied revision marker monotonic: the status patch must carry
+// a resourceVersion precondition.
+//
+// Without it the API server cannot reject the write as conflicting, so the
+// surrounding retry.RetryOnConflict never fires and the freshness check inside
+// it is unsound. Both the target revision and the re-fetched object come from
+// the cache-backed client, so a stale reconcile can compare a stale target
+// against an equally stale read, pass the check, and patch the applied revision
+// backwards. That re-satisfies the spec > applied comparison in Reconcile and
+// sends the controller through the idle and pending runner cleanup again.
+//
+// The property is asserted on the bytes the production code actually emits
+// rather than on an independently constructed patch, so the test cannot pass
+// while the reconciler builds its patch some other way. Reverting the patch
+// option to a plain client.MergeFrom must fail this test.
+func TestPatchAppliedActionableRevisionStatusUsesOptimisticLock(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Spec: v1alpha1.EphemeralRunnerSetSpec{
+			ActionableRevision: 7,
+		},
+		Status: v1alpha1.EphemeralRunnerSetStatus{
+			AppliedActionableRevision: 2,
+		},
+	}
+
+	var capturedPatch []byte
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		WithInterceptorFuncs(interceptor.Funcs{
+			SubResourcePatch: func(ctx context.Context, clt client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
+				data, err := patch.Data(obj)
+				if err != nil {
+					return err
+				}
+				capturedPatch = data
+				return clt.Status().Patch(ctx, obj, patch, opts...)
+			},
+		}).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client: c,
+		Log:    logr.Discard(),
+		Scheme: scheme,
+	}
+
+	key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name}
+	require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 7))
+
+	require.NotEmpty(t, capturedPatch, "expected the reconciler to emit a status patch")
+
+	var emitted struct {
+		Metadata struct {
+			ResourceVersion string `json:"resourceVersion"`
+		} `json:"metadata"`
+		Status struct {
+			AppliedActionableRevision int64 `json:"appliedActionableRevision"`
+		} `json:"status"`
+	}
+	require.NoError(t, json.Unmarshal(capturedPatch, &emitted))
+
+	assert.NotEmpty(
+		t,
+		emitted.Metadata.ResourceVersion,
+		"status patch must carry a resourceVersion precondition so a stale write is rejected instead of moving the applied revision backwards, got %s",
+		string(capturedPatch),
+	)
+	assert.Equal(t, int64(7), emitted.Status.AppliedActionableRevision)
+
+	var updated v1alpha1.EphemeralRunnerSet
+	require.NoError(t, c.Get(context.Background(), key, &updated))
+	assert.Equal(t, int64(7), updated.Status.AppliedActionableRevision)
+}
+
+// TestPatchAppliedActionableRevisionStatusDoesNotMoveBackwards covers the guard
+// inside the retry: a reconcile carrying an older target revision must leave a
+// marker that has already advanced further alone.
+func TestPatchAppliedActionableRevisionStatusDoesNotMoveBackwards(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, v1alpha1.AddToScheme(scheme))
+
+	ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ers",
+			Namespace: "default",
+		},
+		Status: v1alpha1.EphemeralRunnerSetStatus{
+			AppliedActionableRevision: 5,
+		},
+	}
+
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(ephemeralRunnerSet).
+		WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}).
+		Build()
+
+	reconciler := &EphemeralRunnerSetReconciler{
+		Client: c,
+		Log:    logr.Discard(),
+		Scheme: scheme,
+	}
+
+	key := types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: ephemeralRunnerSet.Name}
+	require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 3))
+
+	var updated v1alpha1.EphemeralRunnerSet
+	require.NoError(t, c.Get(context.Background(), key, &updated))
+	assert.Equal(t, int64(5), updated.Status.AppliedActionableRevision)
+}