diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 5b08edaf..3b43a3a7 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -299,10 +299,20 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R // reconciler already has, because the cleanup can take long enough for that copy // to go stale, and a conflicting write must not be resolved by replaying an old // status. +// +// The read bypasses the cache because this also clears +// FinishedRunnerCleanupPatchID, and the patch is computed as a diff against the +// 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. func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx context.Context, key types.NamespacedName, targetAppliedRevision int64) error { return retry.RetryOnConflict(retry.DefaultBackoff, func() error { var latest v1alpha1.EphemeralRunnerSet - if err := r.Get(ctx, key, &latest); err != nil { + reader := r.APIReader + if reader == nil { + reader = r.Client + } + if err := reader.Get(ctx, key, &latest); err != nil { return err } @@ -313,6 +323,16 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx original := latest.DeepCopy() latest.Status.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 pending + // runners, so the shortfall that follows belongs to the new spec and must + // be filled. Worse, a spec change restarts the listener, and a restarted + // listener numbers its patches from 0 upwards, counting 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 + return r.Status().Patch(ctx, &latest, client.MergeFrom(original)) }) } @@ -323,25 +343,36 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx // listener. // // The marker is written by an earlier reconcile and then read back here, so the -// cached copy handed to Reconcile cannot be trusted on its own: deleting the -// finished runners triggers watch events that schedule the next reconcile, and -// that reconcile can be served from an informer cache that has not yet observed -// the controller's own status write. Reading a zero marker there would skip the -// suppression and create exactly the unwanted runner this logic exists to -// prevent. +// cached copy handed to Reconcile cannot be trusted: deleting the finished +// runners triggers watch events that schedule the next reconcile, and that +// reconcile can be served from an informer cache that has not yet observed the +// controller's own status write. The decision is therefore always made against +// an uncached read. That confines the extra API call to scale-up decisions, +// where the controller is about to issue creates anyway. // -// A cached hit is always safe, because nothing clears the marker, so only a miss -// falls through to an uncached read. That confines the extra API call to -// scale-up decisions, where the controller is about to issue creates anyway. +// An earlier version short-circuited on a cached hit, on the reasoning that the +// marker was only ever set and so a hit could never be a false positive. That +// reasoning no longer holds: applying a new actionable revision clears the +// marker, so a lagging cache can show a recorded marker that the API server has +// already cleared, and trusting it would suppress exactly the scale up that +// rebuilds the pool after a spec change. +// +// One window remains. A listener that restarts without a spec change keeps the +// marker but starts its patch sequence again from 0 and counts up through every +// integer, so it passes through the recorded value with near-certainty rather +// than by coincidence. If that collision lands on a reconcile that needs to +// scale up, that reconcile is suppressed. +// +// That is a hiccup rather than an outage. The listener calls back into scaling +// on every long-poll timeout, not only when something changes, and once the set +// is idle at its minimum with no job completed it publishes the collapsed patch +// ID 0, which is never suppressed. So the shortfall is filled on the next +// long-poll cycle. func (r *EphemeralRunnerSetReconciler) scaleUpServicedByFinishedRunnerCleanup(ctx context.Context, key types.NamespacedName, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet) (bool, error) { if ephemeralRunnerSet.Spec.PatchID == 0 { return false, nil } - if ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID == ephemeralRunnerSet.Spec.PatchID { - return true, nil - } - if r.APIReader == nil { return false, errors.New("APIReader is not configured, cannot confirm the finished runner cleanup patch ID without reading through the cache") } diff --git a/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go b/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go index 175f2c4d..55c7b09d 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go @@ -47,14 +47,15 @@ func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) { return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() } - t.Run("suppresses when the cached marker already matches the spec patch ID", func(t *testing.T) { - // A cached hit needs no confirmation, so the reconciler is deliberately - // left without an APIReader: reaching for one here would be a bug. - r := &EphemeralRunnerSetReconciler{} + t.Run("suppresses when the API server records the spec patch ID", func(t *testing.T) { + r := &EphemeralRunnerSetReconciler{ + Client: newReader(newSet(2, 2)), + APIReader: newReader(newSet(2, 2)), + } suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, newSet(2, 2)) require.NoError(t, err) - assert.True(t, suppressed, "the cleanup for this patch ID is already recorded, so the shortfall is not new demand") + assert.True(t, suppressed, "the cleanup for this patch ID is recorded, so the shortfall is not new demand") }) t.Run("does not suppress when neither the cache nor the API server records the patch ID", func(t *testing.T) { @@ -69,10 +70,10 @@ func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) { }) t.Run("suppresses when the cache is stale but the API server records the patch ID", func(t *testing.T) { - // This is the race: the cleanup reconcile wrote the marker and returned, - // and the reconcile triggered by its own deletions is still reading a - // pre-write cache. Client stands in for that lagging cache, APIReader for - // the API server that already has the write. + // The cleanup reconcile wrote the marker and returned, and the reconcile + // triggered by its own deletions is still reading a pre-write cache. + // Client stands in for that lagging cache, APIReader for the API server + // that already has the write. stale := newSet(2, 0) r := &EphemeralRunnerSetReconciler{ Client: newReader(newSet(2, 0)), @@ -84,10 +85,27 @@ func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) { assert.True(t, suppressed, "the decision must come from the API server, not from a cache that has not caught up") }) + t.Run("does not suppress when the cache still shows a marker the API server has cleared", func(t *testing.T) { + // The mirror image of the case above, and the one that opens up once + // applying a new revision clears the marker. A cached hit is no longer + // self-evidently safe: here the cache still carries the marker from + // before the spec change while the API server has already cleared it, and + // trusting the cache would suppress the scale up that rebuilds the pool. + stale := newSet(2, 2) + r := &EphemeralRunnerSetReconciler{ + Client: newReader(newSet(2, 2)), + APIReader: newReader(newSet(2, 0)), + } + + suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, stale) + require.NoError(t, err) + assert.False(t, suppressed, "a cleared marker on the API server must win over a stale cached one") + }) + t.Run("never suppresses when the listener published patch ID zero", func(t *testing.T) { - // Patch ID 0 is the idle-at-minimum state the listener republishes - // without incrementing. Suppressing on it would let a scale set sit - // below its minimum indefinitely. + // Patch ID 0 is the collapsed state the listener republishes on every + // long-poll timeout once the set is idle at its minimum. Suppressing on + // it would let a scale set sit below its minimum indefinitely. r := &EphemeralRunnerSetReconciler{ Client: newReader(newSet(0, 0)), APIReader: newReader(newSet(0, 0)), @@ -105,3 +123,79 @@ func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) { assert.Error(t, err, "a missing APIReader must surface rather than silently fall back to the cached marker") }) } + +// TestPatchAppliedActionableRevisionStatusClearsFinishedRunnerCleanupPatchID +// covers the marker's lifetime across a spec change. +// +// The marker is a patch ID, and patch IDs are only meaningful within one +// listener incarnation. A spec change restarts the listener, which numbers its +// patches from 0 upwards and so passes through any leftover value. Carrying the +// marker across the revision boundary therefore suppresses the scale up that is +// supposed to rebuild the pool the revision cleanup just deleted. +func TestPatchAppliedActionableRevisionStatusClearsFinishedRunnerCleanupPatchID(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + key := types.NamespacedName{Namespace: "test-ns", Name: "test-ers"} + + newSet := func(appliedRevision int64, cleanupPatchID int) *v1alpha1.EphemeralRunnerSet { + return &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Namespace: key.Namespace, Name: key.Name}, + Status: v1alpha1.EphemeralRunnerSetStatus{ + AppliedActionableRevision: appliedRevision, + FinishedRunnerCleanupPatchID: cleanupPatchID, + }, + } + } + + newClient := func(object client.Object) client.Client { + return fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(object). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + Build() + } + + t.Run("clears the marker when the applied revision advances", func(t *testing.T) { + c := newClient(newSet(3, 7)) + r := &EphemeralRunnerSetReconciler{Client: c, APIReader: c} + + require.NoError(t, r.patchAppliedActionableRevisionStatus(context.Background(), key, 4)) + + var got v1alpha1.EphemeralRunnerSet + require.NoError(t, c.Get(context.Background(), key, &got)) + assert.EqualValues(t, 4, got.Status.AppliedActionableRevision, "the applied revision should advance") + assert.Zero(t, got.Status.FinishedRunnerCleanupPatchID, "a marker from the previous patch sequence must not survive the spec change") + }) + + t.Run("decides against authoritative state rather than the cache", func(t *testing.T) { + // The helper both decides and diffs against the object it reads, so that + // read has to bypass the cache. Here the cache has already caught up to + // revision 4 while the API server has not, which is the shape a cache + // takes when it has observed a write the reconcile is about to redo: a + // cached read would conclude there is nothing to do and leave the stale + // marker in place. + authoritative := newClient(newSet(3, 7)) + lagging := newClient(newSet(4, 7)) + r := &EphemeralRunnerSetReconciler{Client: lagging, APIReader: authoritative} + + require.NoError(t, r.patchAppliedActionableRevisionStatus(context.Background(), key, 4)) + + var got v1alpha1.EphemeralRunnerSet + require.NoError(t, lagging.Get(context.Background(), key, &got)) + assert.Zero(t, got.Status.FinishedRunnerCleanupPatchID, "the clear must be computed against authoritative state") + }) + + t.Run("leaves the marker alone when the revision has already been applied", func(t *testing.T) { + // Nothing was cleaned up here, so there is no reason to disturb a marker + // that is still describing the current patch sequence. + c := newClient(newSet(4, 7)) + r := &EphemeralRunnerSetReconciler{Client: c, APIReader: c} + + require.NoError(t, r.patchAppliedActionableRevisionStatus(context.Background(), key, 4)) + + var got v1alpha1.EphemeralRunnerSet + require.NoError(t, c.Get(context.Background(), key, &got)) + assert.EqualValues(t, 7, got.Status.FinishedRunnerCleanupPatchID, "an unchanged revision must not clear the marker") + }) +}