diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 368505fc..a1bc316d 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -52,6 +52,12 @@ type EphemeralRunnerSetReconciler struct { client.Client Log logr.Logger Scheme *runtime.Scheme + // APIReader reads straight from the API server, bypassing the manager's + // cache. It is needed where the controller has to observe a status field it + // wrote itself in an earlier reconcile, because the informer cache is not + // guaranteed to have caught up by the time the next reconcile runs. + // SetupWithManager fills this in from the manager when it is left unset. + APIReader client.Reader ResourceBuilder } @@ -235,7 +241,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R // The gap below Spec.Replicas is the one the cleanup above opened for // this patch ID, not new demand. Wait for the listener to publish a // fresh desired state before acting on it. - if ephemeralRunnerSet.Spec.PatchID > 0 && ephemeralRunnerSet.Status.FinishedRunnerCleanupPatchID == ephemeralRunnerSet.Spec.PatchID { + suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(ctx, req.NamespacedName, &ephemeralRunnerSet) + if err != nil { + log.Error(err, "failed to determine whether scale up was already serviced by finished runner cleanup") + return ctrl.Result{}, err + } + if suppressed { log.Info("Skipping scale up until listener publishes a fresh desired state after finished runner cleanup", "patchID", ephemeralRunnerSet.Spec.PatchID) return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) } @@ -305,6 +316,43 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx }) } +// scaleUpServicedByFinishedRunnerCleanup reports whether the shortfall against +// Spec.Replicas was created by this controller cleaning up finished runners for +// the patch ID currently in the spec, rather than by new demand from the +// 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. +// +// 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. +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") + } + + var latest v1alpha1.EphemeralRunnerSet + if err := r.APIReader.Get(ctx, key, &latest); err != nil { + return false, fmt.Errorf("failed to read EphemeralRunnerSet without the cache: %w", err) + } + + return latest.Status.FinishedRunnerCleanupPatchID == ephemeralRunnerSet.Spec.PatchID, nil +} + // patchFinishedRunnerCleanupPatchIDStatus records that finished runners were // deleted while serving patchID, so a later reconcile can tell the resulting gap // below Spec.Replicas apart from genuine new demand. @@ -724,6 +772,10 @@ func (r *EphemeralRunnerSetReconciler) deleteEphemeralRunnerWithActionsClient(ct func (r *EphemeralRunnerSetReconciler) SetupWithManager(mgr ctrl.Manager, opts ...Option) error { r.setSchemeIfUnset(r.Scheme) + if r.APIReader == nil { + r.APIReader = mgr.GetAPIReader() + } + return builderWithOptions( ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.EphemeralRunnerSet{}). diff --git a/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go b/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go new file mode 100644 index 00000000..175f2c4d --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_scaleup_suppression_test.go @@ -0,0 +1,107 @@ +package actionsgithubcom + +import ( + "context" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "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" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// TestScaleUpServicedByFinishedRunnerCleanup pins down the decision that +// suppresses scale up after finished runners were cleaned up. +// +// The interesting case is the third one. The marker is written by one reconcile +// and read back by the next, and the deletions performed by the first reconcile +// are themselves what triggers the second. That next reconcile is regularly +// served from an informer cache that has not yet observed the controller's own +// status write, so the decision must not be made from the cached copy alone. An +// envtest spec only hits that window under load, which is why this is asserted +// directly instead. +func TestScaleUpServicedByFinishedRunnerCleanup(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + key := types.NamespacedName{Namespace: "test-ns", Name: "test-ers"} + + newSet := func(specPatchID, statusCleanupPatchID int) *v1alpha1.EphemeralRunnerSet { + return &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{Namespace: key.Namespace, Name: key.Name}, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + Replicas: 2, + PatchID: specPatchID, + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + FinishedRunnerCleanupPatchID: statusCleanupPatchID, + }, + } + } + + newReader := func(objects ...client.Object) client.Client { + 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{} + + 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") + }) + + t.Run("does not suppress when neither the cache nor the API server records the patch ID", func(t *testing.T) { + r := &EphemeralRunnerSetReconciler{ + Client: newReader(newSet(2, 0)), + APIReader: newReader(newSet(2, 0)), + } + + suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, newSet(2, 0)) + require.NoError(t, err) + assert.False(t, suppressed, "no cleanup was performed for this patch ID, so the shortfall is genuine demand") + }) + + 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. + stale := newSet(2, 0) + r := &EphemeralRunnerSetReconciler{ + Client: newReader(newSet(2, 0)), + APIReader: newReader(newSet(2, 2)), + } + + suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, stale) + require.NoError(t, err) + assert.True(t, suppressed, "the decision must come from the API server, not from a cache that has not caught up") + }) + + 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. + r := &EphemeralRunnerSetReconciler{ + Client: newReader(newSet(0, 0)), + APIReader: newReader(newSet(0, 0)), + } + + suppressed, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, newSet(0, 0)) + require.NoError(t, err) + assert.False(t, suppressed, "patch ID 0 must always be free to scale up") + }) + + t.Run("fails loudly when it cannot read past the cache", func(t *testing.T) { + r := &EphemeralRunnerSetReconciler{Client: newReader(newSet(2, 2))} + + _, err := r.scaleUpServicedByFinishedRunnerCleanup(context.Background(), key, newSet(2, 0)) + assert.Error(t, err, "a missing APIReader must surface rather than silently fall back to the cached marker") + }) +}