diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index c7fea9bb..27efd043 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -22,6 +22,7 @@ import ( "errors" "fmt" "maps" + "slices" "sort" "strconv" "time" @@ -302,8 +303,11 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) } -// patchAppliedActionableRevisionStatus records that the runner spec carried by -// targetAppliedRevision has been fully applied. +// patchAppliedActionableRevisionStatus brings status into line with the runner +// spec carried by targetAppliedRevision once that spec has been fully applied. +// It records the applied revision, clears the scale-up suppression marker when +// the revision actually advances, and re-derives Status.Phase from the child +// runners. // // The marker lives in status rather than in an annotation on the spec, and it is // written only once the cleanup above has actually succeeded. If the controller @@ -333,6 +337,11 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R // 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. +// +// The lock covers the EphemeralRunnerSet object and nothing else. The phase is +// derived from a separate list of the child runners, which no precondition on +// this patch can vouch for, so that list is read through the same authoritative +// reader rather than the cache. func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error { return retry.RetryOnConflict(retry.DefaultBackoff, func() error { var latest v1alpha1.EphemeralRunnerSet @@ -365,9 +374,26 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx } ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList) - if err := r.List(ctx, ephemeralRunnerList, client.InNamespace(latest.Namespace), client.MatchingFields{resourceOwnerKey: latest.Name}); err != nil { + // Listed through the same authoritative reader as the Get above. The + // optimistic lock on the patch below covers the EphemeralRunnerSet object + // only, so it cannot vouch for a separately-read list: deriving the phase + // from the cache would let a successful, lock-protected write carry a + // value the lock says nothing about. The list also sits inside + // RetryOnConflict, and a cached list can return the same stale data on + // every attempt, spending the whole backoff re-deriving one wrong phase. + // + // resourceOwnerKey cannot be used here. It is a client-side index + // registered on the manager's cache, and the API server rejects it as an + // unsupported field label, so the ownership filter has to be applied in + // this process instead. The list is namespace-scoped, and a namespace + // holds the runners of one scale set, so this reads little more than the + // selector would have. + if err := reader.List(ctx, ephemeralRunnerList, client.InNamespace(latest.Namespace)); err != nil { return fmt.Errorf("failed to list child ephemeral runners: %w", err) } + ephemeralRunnerList.Items = slices.DeleteFunc(ephemeralRunnerList.Items, func(runner v1alpha1.EphemeralRunner) bool { + return !isControlledBy(&runner, "EphemeralRunnerSet", latest.Name) + }) // Judge the runners against the revision being applied, not the one // recorded in status: every runner created before this update is stale by diff --git a/controllers/actions.github.com/ephemeralrunnerset_phase_read_test.go b/controllers/actions.github.com/ephemeralrunnerset_phase_read_test.go new file mode 100644 index 00000000..23f7247c --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_phase_read_test.go @@ -0,0 +1,117 @@ +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" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// TestPatchAppliedActionableRevisionStatusDerivesPhaseFromAuthoritativeRead +// pins the read that the derived phase is computed from. +// +// patchAppliedActionableRevisionStatus reads the EphemeralRunnerSet through +// APIReader precisely because a cached read cannot be trusted here, then +// derives Status.Phase from a list of the child runners. If that list goes +// through the cache-backed client instead, the phase is derived from data the +// surrounding optimistic lock does not cover: the lock proves only that the +// EphemeralRunnerSet was live at write time, never that the list was. +// +// A wrong phase does not merely flap. Reconcile's Outdated branch cleans up and +// returns before reaching updateStatus, and this function only runs while +// spec > applied, which it then makes false by advancing the marker. So a phase +// wrongly set to Outdated is never recomputed, and the set stays switched off. +// +// The two clients below disagree on purpose. The authoritative one holds no +// runners, so the correct phase is Running. The cached one holds an Outdated +// runner at the revision being applied, which is the phase a cached list would +// produce. Listing through the cache-backed client must fail this test. +func TestPatchAppliedActionableRevisionStatusDerivesPhaseFromAuthoritativeRead(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + newEphemeralRunnerSet := func() *v1alpha1.EphemeralRunnerSet { + return &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ers", + Namespace: "default", + }, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 7, + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + AppliedActionableRevision: 2, + }, + } + } + + // Phase Outdated at the revision being applied, so the classifier counts it + // as outdated rather than staleOutdated. + controllerRef := true + staleView := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner-from-cache", + Namespace: "default", + Annotations: map[string]string{ + AnnotationKeyActionableRevision: "7", + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: "test-ers", + UID: "test-uid", + Controller: &controllerRef, + }, + }, + }, + Status: v1alpha1.EphemeralRunnerStatus{ + Phase: v1alpha1.EphemeralRunnerPhaseOutdated, + }, + } + + builder := func(objs ...client.Object) client.Client { + return fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objs...). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). + Build() + } + + // The cache-backed client still sees a runner the API server no longer has. + cached := builder(newEphemeralRunnerSet(), staleView) + // The authoritative view: the runner is gone, so nothing is outdated. + authoritative := builder(newEphemeralRunnerSet()) + + reconciler := &EphemeralRunnerSetReconciler{ + Client: cached, + APIReader: authoritative, + Log: logr.Discard(), + Scheme: scheme, + } + + key := types.NamespacedName{Namespace: "default", Name: "test-ers"} + require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 7)) + + var patched v1alpha1.EphemeralRunnerSet + require.NoError(t, cached.Get(context.Background(), key, &patched)) + + assert.Equal( + t, + v1alpha1.EphemeralRunnerSetPhaseRunning, + patched.Status.Phase, + "phase must be derived from the authoritative read, not the cache: the cached client holds an outdated runner the API server no longer has, and a phase of Outdated is never recomputed because Reconcile's Outdated branch returns before updateStatus", + ) + assert.Equal(t, int64(7), patched.Status.AppliedActionableRevision) +} diff --git a/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go b/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go index df3f6309..61901c3c 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_revision_patch_test.go @@ -57,6 +57,7 @@ func TestPatchAppliedActionableRevisionStatusUsesOptimisticLock(t *testing.T) { WithScheme(scheme). WithObjects(ephemeralRunnerSet). WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("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) @@ -125,6 +126,7 @@ func TestPatchAppliedActionableRevisionStatusDoesNotMoveBackwards(t *testing.T) WithScheme(scheme). WithObjects(ephemeralRunnerSet). WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). Build() reconciler := &EphemeralRunnerSetReconciler{ diff --git a/controllers/actions.github.com/indexer.go b/controllers/actions.github.com/indexer.go index 466c9f14..0c47f409 100644 --- a/controllers/actions.github.com/indexer.go +++ b/controllers/actions.github.com/indexer.go @@ -69,3 +69,17 @@ func newGroupVersionOwnerKindIndexer(ownerKind string, otherOwnerKinds ...string return []string{owner.Name} } } + +// isControlledBy applies the same ownership test as the resourceOwnerKey index, +// for callers that cannot use that index. It is registered on the manager's +// cache, so a read that deliberately bypasses the cache has to filter here +// instead: the API server rejects .metadata.controller as an unsupported field +// label. Keeping the predicate alongside the indexer is what stops the two +// drifting apart. +func isControlledBy(o client.Object, ownerKind, ownerName string) bool { + owner := metav1.GetControllerOfNoCopy(o) + return owner != nil && + owner.APIVersion == v1alpha1.GroupVersion.String() && + owner.Kind == ownerKind && + owner.Name == ownerName +}