actions-runner-controller/controllers/actions.github.com/ephemeralrunnerset_status_patch_test.go

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

// TestUpdateStatusNeverRepublishesTheCleanupMarker pins the invariant that makes
// it safe for Reconcile to copy the cleanup marker onto the object it carries
// from the top of the reconcile.
//
// That object comes from the cache, so its marker can be older than the API
// server's by the time the final status patch runs. The reason this cannot
// resurrect a superseded value is that updateStatus copies both the marker and
// the applied revision verbatim into desiredStatus and computes its patch as a
// diff against a copy taken at entry. Both sides of the diff therefore hold the
// same value, and a JSON merge patch emits nothing for a field that did not
// change: the only key updateStatus can ever produce is the phase.
//
// The invariant is not obvious from reading the function, it is load-bearing,
// and it has now been read the wrong way round twice in review -- once as
// updateStatus clobbering the marker with a stale zero, once as it restoring a
// stale marker over a newer one. Neither is possible while the field is copied
// rather than computed, so this test asserts on the emitted bytes.
//
// Two independent changes would make both readings real, and the test is
// written to fail on each of them. Taking original before the assignment rather
// than after puts the field in the diff. Computing the marker inside
// updateStatus, for instance from Spec.PatchID since the marker means "cleanup
// ran for this patch ID", puts a value in the diff that the caller never
// approved. The second is only detectable if the fixture gives Spec.PatchID a
// value distinct from the marker, which is why run refuses to accept equal ones.
func TestUpdateStatusNeverRepublishesTheCleanupMarker(t *testing.T) {
	scheme := runtime.NewScheme()
	require.NoError(t, clientgoscheme.AddToScheme(scheme))
	require.NoError(t, v1alpha1.AddToScheme(scheme))

	key := types.NamespacedName{Namespace: "default", Name: "test-ers"}

	// specPatchID is the patch ID the listener has published; serverMarker is what
	// the API server holds by the time updateStatus runs; carriedMarker is what the
	// cached object in Reconcile carries, including the assignment made after the
	// authoritative write.
	//
	// All three have to be distinct. The marker is assigned from Spec.PatchID on
	// the cleanup path, so it is tempting to reuse one value for the spec and the
	// carried marker, but then copying the marker out of the status and computing
	// it from the spec produce the same number and no assertion can tell them
	// apart -- which is exactly the substitution this test exists to catch. The
	// require below keeps that from being reintroduced quietly.
	run := func(t *testing.T, specPatchID, serverMarker, carriedMarker int) ([]byte, int) {
		t.Helper()
		require.NotEqual(t, specPatchID, carriedMarker, "spec patch ID must differ from the carried marker or compute-from-spec is indistinguishable from copy-from-status")
		require.NotEqual(t, specPatchID, serverMarker, "spec patch ID must differ from the server marker or a republished value is indistinguishable from an untouched one")

		stored := &v1alpha1.EphemeralRunnerSet{
			ObjectMeta: metav1.ObjectMeta{Namespace: key.Namespace, Name: key.Name},
			Spec:       v1alpha1.EphemeralRunnerSetSpec{PatchID: specPatchID},
			Status: v1alpha1.EphemeralRunnerSetStatus{
				Phase:                        v1alpha1.EphemeralRunnerSetPhaseRunning,
				FinishedRunnerCleanupPatchID: serverMarker,
			},
		}

		var capturedPatch []byte
		c := fake.NewClientBuilder().
			WithScheme(scheme).
			WithObjects(stored).
			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,
		}

		// The object Reconcile carries: a cached read whose marker has since been
		// overtaken, plus the assignment, and an empty phase so that updateStatus
		// has a genuine change to patch.
		carried := stored.DeepCopy()
		carried.Status.Phase = ""
		carried.Status.FinishedRunnerCleanupPatchID = carriedMarker

		require.NoError(t, reconciler.updateStatus(context.Background(), carried, &ephemeralRunnersByState{}, logr.Discard()))
		require.NotEmpty(t, capturedPatch, "expected updateStatus to emit a status patch for the phase change")

		var updated v1alpha1.EphemeralRunnerSet
		require.NoError(t, c.Get(context.Background(), key, &updated))

		return capturedPatch, updated.Status.FinishedRunnerCleanupPatchID
	}

	assertMarkerAbsent := func(t *testing.T, patch []byte) {
		t.Helper()

		var emitted struct {
			Status map[string]json.RawMessage `json:"status"`
		}
		require.NoError(t, json.Unmarshal(patch, &emitted))

		_, present := emitted.Status["finishedRunnerCleanupPatchID"]
		assert.False(t, present,
			"updateStatus must not carry the cleanup marker, or a cached reconcile could overwrite an authoritative write, got %s",
			string(patch))
	}

	t.Run("does not restore a marker another reconcile has cleared", func(t *testing.T) {
		// An actionable revision advanced and cleared the marker between the
		// authoritative write and this patch. Restoring it here would suppress the
		// scale up that rebuilds the pool for the new spec.
		patch, marker := run(t, 7, 0, 4)

		assertMarkerAbsent(t, patch)
		assert.Zero(t, marker, "a cleared marker must stay cleared")
	})

	t.Run("does not lower a marker another reconcile has advanced", func(t *testing.T) {
		// A concurrent cleanup recorded a newer patch ID. Lowering it back would
		// stop suppression matching the patch that was actually serviced.
		patch, marker := run(t, 7, 9, 4)

		assertMarkerAbsent(t, patch)
		assert.Equal(t, 9, marker, "a newer marker must not be overwritten by a cached one")
	})
}