diff --git a/.mockery.yaml b/.mockery.yaml index 4cffec6438..57b564a93f 100644 --- a/.mockery.yaml +++ b/.mockery.yaml @@ -14,4 +14,8 @@ packages: all: true github.com/actions/actions-runner-controller/controllers/actions.github.com: config: - all: true + all: false + include-interface-regex: ".*" + # deepCopyObject is a generic type constraint, not a collaborator, so it + # must not be mocked. + exclude-interface-regex: "^deepCopyObject$" diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 662a9a1adf..ebc0b7d841 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -78,7 +78,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. if err := r.Get(ctx, req.NamespacedName, &autoscalingListener); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingListener.DeepCopy() + listener := newLazyCopy(&autoscalingListener) if !autoscalingListener.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { @@ -97,8 +97,8 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { - if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { + if controllerutil.RemoveFinalizer(listener.Mutate(), autoscalingListenerFinalizerName) { + if err := r.Patch(ctx, &autoscalingListener, listener.MergeFrom()); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to remove finalizer") return ctrl.Result{}, err } @@ -109,8 +109,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { - if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil { + if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { + controllerutil.AddFinalizer(listener.Mutate(), autoscalingListenerFinalizerName) + if err := r.Patch(ctx, &autoscalingListener, listener.MergeFrom()); err != nil { log.Error(err, "Failed to add finalizer") return ctrl.Result{}, err } diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index 3086f3de2d..236496a510 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -75,7 +75,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if err := r.Get(ctx, req.NamespacedName, &autoscalingRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingRunnerSet.DeepCopy() + runnerSet := newLazyCopy(&autoscalingRunnerSet) if !autoscalingRunnerSet.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { @@ -100,9 +100,9 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, err } - if controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + if controllerutil.RemoveFinalizer(runnerSet.Mutate(), autoscalingRunnerSetFinalizerName) { log.Info("Removing finalizer") - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { + if err := r.Patch(ctx, &autoscalingRunnerSet, runnerSet.MergeFrom()); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to update autoscaling runner set without finalizer") return ctrl.Result{}, err } @@ -131,10 +131,11 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + controllerutil.AddFinalizer(runnerSet.Mutate(), autoscalingRunnerSetFinalizerName) log.Info("Adding finalizer") - if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &autoscalingRunnerSet, runnerSet.MergeFrom()); err != nil { log.Error(err, "Failed to update autoscaling runner set with finalizer") return ctrl.Result{}, err } diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index 256ed311c4..7608ef8bf2 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -94,7 +94,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err := r.Get(ctx, req.NamespacedName, &ephemeralRunner); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunner.DeepCopy() + runner := newLazyCopy(&ephemeralRunner) if !ephemeralRunner.DeletionTimestamp.IsZero() { r.publishEphemeralRunnerPhaseMetric(&ephemeralRunner, "", log) @@ -116,9 +116,9 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } log.Info("Runner is cleaned up from the service, removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { + if controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerActionsFinalizerName) { log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); err != nil { log.Error(err, "Failed to update ephemeral runner after removing finalizer") return ctrl.Result{}, err } @@ -143,9 +143,9 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) { + if controllerutil.RemoveFinalizer(runner.Mutate(), ephemeralRunnerFinalizerName) { log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); client.IgnoreNotFound(err) != nil { + if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); client.IgnoreNotFound(err) != nil { log.Error(err, "Failed to update ephemeral runner after removing finalizer") return ctrl.Result{}, err } @@ -171,17 +171,15 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - addFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) || !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) - if addFinalizers { + missingFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) || + !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + if missingFinalizers { log.Info("Adding finalizers") - var addedFinalizers bool - addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) - addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) - if addedFinalizers { - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update with finalizer set") - return ctrl.Result{}, err - } + controllerutil.AddFinalizer(runner.Mutate(), ephemeralRunnerFinalizerName) + controllerutil.AddFinalizer(runner.Mutate(), ephemeralRunnerActionsFinalizerName) + if err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()); err != nil { + log.Error(err, "Failed to update with finalizer set") + return ctrl.Result{}, err } log.Info("Successfully added finalizers") } diff --git a/controllers/actions.github.com/ephemeralrunnerset_advancing_revision_phase_test.go b/controllers/actions.github.com/ephemeralrunnerset_advancing_revision_phase_test.go new file mode 100644 index 0000000000..9b0c00388d --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_advancing_revision_phase_test.go @@ -0,0 +1,183 @@ +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/fake" +) + +// TestPatchAppliedActionableRevisionStatusClassifiesAgainstTheAdvancedRevision +// pins the other half of the classifier contract: which revision is used when +// the monotonicity guard actually fires. +// +// TestPatchAppliedActionableRevisionStatusIgnoresATargetBehindTheLiveRevision +// covers the case where the target has fallen behind the live marker, so the +// guard does not fire and the applied revision is unchanged. That case cannot +// observe this one. The guard writes the advance to a copy of the status, so +// only when it fires do the copy and the stored status hold different +// revisions, and only then can the classifier be pointed at the wrong one. +// +// Reading the pre-advance revision rates a runner left over from the superseded +// revision as current instead of staleOutdated, which sets the Outdated phase. +// That phase is then persisted next to the freshly advanced revision, which is +// the state this function exists to avoid: Reconcile's Outdated branch returns +// before updateStatus, and this function only runs while spec > applied, so +// nothing recomputes the phase and the set stays switched off until the next +// spec change. +func TestPatchAppliedActionableRevisionStatusClassifiesAgainstTheAdvancedRevision(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + // The spec has moved to revision 2 while the status still records 1, so the + // guard below advances the marker and the two revisions differ. + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ers", + Namespace: "default", + }, + Spec: v1alpha1.EphemeralRunnerSetSpec{ + ActionableRevision: 2, + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + AppliedActionableRevision: 1, + Phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + }, + } + + // A runner built from revision 1 that the service rejected. Applying + // revision 2 deletes the idle and pending runners, but this one can survive + // the cleanup: the cleanup list is read through the cache and can be stale, + // which is the case the reviewer of this code identified. + // + // Its revision equals the pre-advance marker, so classifying against that + // value puts it in outdated rather than staleOutdated. Against the advanced + // revision it is correctly stale. + controllerRef := true + supersededRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runner-from-revision-1", + Namespace: "default", + Annotations: map[string]string{ + AnnotationKeyActionableRevision: "1", + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "EphemeralRunnerSet", + Name: "test-ers", + UID: "test-uid", + Controller: &controllerRef, + }, + }, + }, + Status: v1alpha1.EphemeralRunnerStatus{ + Phase: v1alpha1.EphemeralRunnerPhaseOutdated, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ephemeralRunnerSet, supersededRunner). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). + Build() + + reconciler := &EphemeralRunnerSetReconciler{ + Client: fakeClient, + APIReader: fakeClient, + Log: logr.Discard(), + Scheme: scheme, + } + + key := types.NamespacedName{Namespace: "default", Name: "test-ers"} + + require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 2)) + + var patched v1alpha1.EphemeralRunnerSet + require.NoError(t, fakeClient.Get(context.Background(), key, &patched)) + + assert.Equal( + t, + int64(2), + patched.Status.AppliedActionableRevision, + "the guard must advance the marker to the target revision", + ) + + assert.Equal( + t, + v1alpha1.EphemeralRunnerSetPhaseRunning, + patched.Status.Phase, + "the leftover runner reported Outdated against revision 1, which this call has just superseded, so it must be classified against the advanced revision and ignored: counting it against the pre-advance revision saves the Outdated phase alongside revision 2, and nothing recomputes the phase from there", + ) +} + +// TestPatchAppliedActionableRevisionStatusDoesNotPatchWhenNothingChanges pins +// the other half of that contract: the call must issue no patch at all when the +// status already says what it would set. +// +// This is the reason the object is not deep copied up front, and it is decided +// by whether any mutation was actually made rather than by comparing a +// separately built status value. Losing it would send an empty patch on every +// reconcile that reaches this function, and each write bumps the resourceVersion +// and wakes every watcher, so the regression is invisible in behaviour and +// visible only in load. +func TestPatchAppliedActionableRevisionStatusDoesNotPatchWhenNothingChanges(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + // Already applied revision 3, already Running, marker already clear, and no + // child runners to move the phase. Every write below is therefore a no-op. + ephemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ers", + Namespace: "default", + }, + Status: v1alpha1.EphemeralRunnerSetStatus{ + AppliedActionableRevision: 3, + Phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ephemeralRunnerSet). + WithStatusSubresource(&v1alpha1.EphemeralRunnerSet{}). + WithIndex(&v1alpha1.EphemeralRunner{}, resourceOwnerKey, newGroupVersionOwnerKindIndexer("EphemeralRunnerSet")). + Build() + + reconciler := &EphemeralRunnerSetReconciler{ + Client: fakeClient, + APIReader: fakeClient, + Log: logr.Discard(), + Scheme: scheme, + } + + key := types.NamespacedName{Namespace: "default", Name: "test-ers"} + + var before v1alpha1.EphemeralRunnerSet + require.NoError(t, fakeClient.Get(context.Background(), key, &before)) + + require.NoError(t, reconciler.patchAppliedActionableRevisionStatus(context.Background(), key, 3)) + + var after v1alpha1.EphemeralRunnerSet + require.NoError(t, fakeClient.Get(context.Background(), key, &after)) + + assert.Equal( + t, + before.ResourceVersion, + after.ResourceVersion, + "the status already matched, so no patch should have been sent: the resourceVersion moving means an empty patch was written anyway", + ) + assert.Equal(t, before.Status, after.Status) +} diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index cf73bd51df..26b50d295b 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -87,7 +87,7 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R if err := r.Get(ctx, req.NamespacedName, &ephemeralRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunnerSet.DeepCopy() + runnerSet := newLazyCopy(&ephemeralRunnerSet) // Requested deletion does not need reconciled. if !ephemeralRunnerSet.DeletionTimestamp.IsZero() { @@ -117,8 +117,8 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R } log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + if controllerutil.RemoveFinalizer(runnerSet.Mutate(), EphemeralRunnerSetFinalizerName) { + if err := r.Patch(ctx, &ephemeralRunnerSet, runnerSet.MergeFrom()); err != nil { log.Error(err, "Failed to update ephemeral runner set with removed finalizer") return ctrl.Result{}, err } @@ -130,9 +130,10 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R } // Add finalizer if not present - if controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { + if !controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { + controllerutil.AddFinalizer(runnerSet.Mutate(), EphemeralRunnerSetFinalizerName) log.Info("Adding finalizer") - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + if err := r.Patch(ctx, &ephemeralRunnerSet, runnerSet.MergeFrom()); err != nil { log.Error(err, "Failed to update ephemeral runner set with new finalizer") return ctrl.Result{}, err } @@ -344,23 +345,34 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R // 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 + var ephemeralRunnerSet v1alpha1.EphemeralRunnerSet reader := r.APIReader if reader == nil { reader = r.Client } - if err := reader.Get(ctx, key, &latest); err != nil { + if err := reader.Get(ctx, key, &ephemeralRunnerSet); err != nil { return err } - original := latest.DeepCopy() + // Mutations go through this rather than into a separate desired-status + // value. Writing to a copy while reading from the original is how the + // revision below came to be judged against a superseded value: the write + // moved and the read stayed, and because the read was textually unchanged + // nothing in the diff pointed at it. Mutate returns the live object, so + // every read below observes the writes above it, and the applied revision + // only ever lives in one place. + // + // It also leaves the object uncopied on the common path where nothing + // changes, which is why a plain DeepCopy is not taken here. + runnerSet := newLazyCopy(&ephemeralRunnerSet) // Only an advance means the idle and pending runners were just deleted and // the listener restarted. Guarding both writes on it keeps this callable // as a plain "make sure status reflects revision N" without disturbing a // marker that still describes the live patch sequence. - if latest.Status.AppliedActionableRevision < targetAppliedRevision { - latest.Status.AppliedActionableRevision = targetAppliedRevision + if ephemeralRunnerSet.Status.AppliedActionableRevision < targetAppliedRevision { + status := &runnerSet.Mutate().Status + 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 @@ -370,7 +382,7 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx // 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 + status.FinishedRunnerCleanupPatchID = 0 } ephemeralRunnerList := new(v1alpha1.EphemeralRunnerList) @@ -395,11 +407,11 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx // fail. A namespace can hold more than one scale set, so this does read // runners that are not ours, but it only runs when a revision actually // advances rather than on every reconcile. - if err := reader.List(ctx, ephemeralRunnerList, client.InNamespace(latest.Namespace)); err != nil { + if err := reader.List(ctx, ephemeralRunnerList, client.InNamespace(ephemeralRunnerSet.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) + return !isControlledBy(&runner, "EphemeralRunnerSet", ephemeralRunnerSet.Name) }) // Judge the runners against the revision the set has now applied, rather @@ -409,34 +421,52 @@ func (r *EphemeralRunnerSetReconciler) patchAppliedActionableRevisionStatus(ctx // the Outdated phase immediately rather than waiting for the pre-update // runners to be collected. // - // After the guard above, this field is max(live, target), and the two - // differ in a case that matters. The caller reads the spec from the - // cache while this function re-reads the status from the API server, so - // a lagging reconcile can arrive with a target behind the live marker. - // Judging against that lower target would rate a runner left over from - // the superseded revision as current and flip a set that has already - // moved on back to Outdated. That phase is deliberately absorbing, so - // the set would then stay switched off until the next spec change. - state := newEphemeralRunnersByStates(ephemeralRunnerList, latest.Status.AppliedActionableRevision) + // This must stay below the guard, which is what makes the field read here + // max(live, target) rather than just the live value. The two differ in a + // case that matters in both directions. + // + // Reading a value behind the live marker rates a runner left over from a + // superseded revision as current and flips a set that has already moved on + // back to Outdated. The caller reads the spec from the cache while this + // function re-reads the status from the API server, so a lagging reconcile + // can arrive with a target behind the live marker; the guard is what stops + // that target being used. + // + // Reading a value behind the one being applied does the same thing to the + // advance itself. A runner missed by the cleanup, whose list is read + // through the cache, carries the pre-advance revision, so judging it + // against that revision counts it as current and saves Outdated alongside + // the freshly advanced marker. + // + // Neither is self-correcting. The Outdated phase is absorbing here: + // Reconcile returns on that path before reaching updateStatus, and this + // function only runs while spec is ahead of applied, so nothing recomputes + // the phase and the set stays switched off until the next spec change. + state := newEphemeralRunnersByStates(ephemeralRunnerList, ephemeralRunnerSet.Status.AppliedActionableRevision) // Set the phase in both directions. This function returns early from // Reconcile without reaching updateStatus, so leaving the phase untouched // would let a stale value survive: a stale Running would hide genuinely // outdated runners from the cleanup path, and a stale Outdated would keep // the set switched off after the spec that caused it was replaced. + phase := v1alpha1.EphemeralRunnerSetPhaseRunning if len(state.outdated) > 0 { - latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseOutdated - } else { - latest.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + phase = v1alpha1.EphemeralRunnerSetPhaseOutdated + } + if ephemeralRunnerSet.Status.Phase != phase { + runnerSet.Mutate().Status.Phase = phase } - // Checked after every field above has been set, so that clearing the - // marker alone is still enough to issue the patch. - if original.Status == latest.Status { + // Every write above is guarded on the value actually changing, so an + // unmodified lazyCopy means the status already says what this call wanted + // it to say. Clearing the marker alone still counts as a change. + if !runnerSet.Modified() { return nil } - return r.Status().Patch(ctx, &latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})) + // The lock covers the object this patch was computed against, so the + // snapshot has to be the one taken before the first mutation above. + return r.Status().Patch(ctx, &ephemeralRunnerSet, runnerSet.MergeFrom(client.MergeFromWithOptimisticLock{})) }) } @@ -551,7 +581,6 @@ func (r *EphemeralRunnerSetReconciler) patchFinishedRunnerCleanupPatchIDStatus(c } func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, state *ephemeralRunnersByState, log logr.Logger) error { - original := ephemeralRunnerSet.DeepCopy() var phase v1alpha1.EphemeralRunnerSetPhase switch { case len(state.outdated) > 0: @@ -569,6 +598,7 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer // Update the status if needed. if ephemeralRunnerSet.Status != desiredStatus { + original := ephemeralRunnerSet.DeepCopy() ephemeralRunnerSet.Status = desiredStatus if err := r.Status().Patch(ctx, ephemeralRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to update EphemeralRunnerSet status") diff --git a/controllers/actions.github.com/lazycopy.go b/controllers/actions.github.com/lazycopy.go new file mode 100644 index 0000000000..977297b701 --- /dev/null +++ b/controllers/actions.github.com/lazycopy.go @@ -0,0 +1,98 @@ +package actionsgithubcom + +import "sigs.k8s.io/controller-runtime/pkg/client" + +// deepCopyObject is satisfied by every generated API type pointer, as well as +// by the built-in Kubernetes types. +type deepCopyObject[T any] interface { + client.Object + DeepCopy() T +} + +// lazyCopy defers the DeepCopy of a fetched object until the moment it is +// actually about to be mutated. +// +// Reconcilers observe an object far more often than they change it, so taking +// the snapshot up front means paying for a full deep copy on every reconcile +// just to serve the rare patch. lazyCopy pays for it only on the reconciles +// that patch. +// +// The snapshot must be taken before the first mutation, otherwise the merge +// patch is computed against the already mutated object and comes out empty. +// Routing a mutation through Mutate is what guarantees that ordering: +// +// runner := newLazyCopy(&ephemeralRunner) +// if !controllerutil.ContainsFinalizer(&ephemeralRunner, name) { +// controllerutil.AddFinalizer(runner.Mutate(), name) +// } +// if runner.Modified() { +// err := r.Patch(ctx, &ephemeralRunner, runner.MergeFrom()) +// } +// +// Callers must uphold that ordering themselves, because lazyCopy cannot +// enforce it. The caller keeps the pointer it passed to newLazyCopy, and as the +// example shows it goes on using that pointer to read the object and to address +// the patch. Nothing stops it from writing through it as well. A write that +// lands before the first Mutate is already present in the snapshot, so the +// merge patch against that snapshot is empty and the write is silently dropped +// rather than sent to the API server. +// +// So the invariant is: read through the original as much as you like, but make +// every mutation that the patch should carry go through Mutate. +// +// "The patch" is the qualifier that matters for a type with a status +// subresource, because there the object is two independently patchable +// surfaces. A write to status cannot go missing from a patch that does not +// carry status: the API server ignores status in the body of a merge patch to +// the main resource, so a status write made outside Mutate is neither captured +// by nor dropped from that patch. EphemeralRunnerSetReconciler.updateStatus +// relies on this, writing Status directly while Reconcile holds a lazyCopy over +// the same object and persisting it through its own Status().Patch. That reads +// like a violation of the rule above and is not one. +// +// The reverse is not true. A lazyCopy guarding a status patch has the same +// exposure to metadata writes, so the rule holds surface by surface rather than +// object by object. +// +// A lazyCopy is not safe for concurrent use. +type lazyCopy[T deepCopyObject[T]] struct { + obj T + original T + copied bool +} + +// newLazyCopy returns a lazyCopy guarding obj. No copy is taken until the +// first call to Mutate. +func newLazyCopy[T deepCopyObject[T]](obj T) *lazyCopy[T] { + return &lazyCopy[T]{obj: obj} +} + +// Mutate snapshots the object on its first call and returns the live object so +// the caller can modify it. Every mutation that a later patch should carry must +// go through Mutate. +func (l *lazyCopy[T]) Mutate() T { + if !l.copied { + l.original = l.obj.DeepCopy() + l.copied = true + } + return l.obj +} + +// Modified reports whether Mutate has been called, and therefore whether there +// is anything to patch. +func (l *lazyCopy[T]) Modified() bool { + return l.copied +} + +// MergeFrom returns a merge patch against the snapshot taken by the first +// Mutate call, with any of controller-runtime's merge options applied, such as +// client.MergeFromWithOptimisticLock{}. It panics when called on an unmodified +// lazyCopy, because there is no snapshot to diff against and the caller would +// otherwise silently issue a patch computed from the live object against +// itself. Guard it with Modified. +func (l *lazyCopy[T]) MergeFrom(opts ...client.MergeFromOption) client.Patch { + if !l.copied { + panic("lazyCopy: MergeFrom called before Mutate") + } + return client.MergeFromWithOptions(l.original, opts...) +} diff --git a/controllers/actions.github.com/lazycopy_test.go b/controllers/actions.github.com/lazycopy_test.go new file mode 100644 index 0000000000..1ff277422f --- /dev/null +++ b/controllers/actions.github.com/lazycopy_test.go @@ -0,0 +1,113 @@ +package actionsgithubcom + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestLazyCopyDoesNotCopyUntilMutated(t *testing.T) { + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod"}} + lazy := newLazyCopy(pod) + + assert.False(t, lazy.Modified()) + assert.Panics(t, func() { lazy.MergeFrom() }) +} + +func TestLazyCopySnapshotsBeforeTheFirstMutation(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Annotations: map[string]string{"key": "old"}, + }, + } + lazy := newLazyCopy(pod) + + lazy.Mutate().Annotations["key"] = "new" + require.True(t, lazy.Modified()) + + data, err := lazy.MergeFrom().Data(pod) + require.NoError(t, err) + assert.JSONEq(t, `{"metadata":{"annotations":{"key":"new"}}}`, string(data)) +} + +func TestLazyCopySnapshotsOnlyOnce(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Annotations: map[string]string{"first": "old", "second": "old"}, + }, + } + lazy := newLazyCopy(pod) + + lazy.Mutate().Annotations["first"] = "new" + // The second mutation must diff against the state before the first one, + // otherwise the earlier change is dropped from the patch. + lazy.Mutate().Annotations["second"] = "new" + + data, err := lazy.MergeFrom().Data(pod) + require.NoError(t, err) + assert.JSONEq(t, `{"metadata":{"annotations":{"first":"new","second":"new"}}}`, string(data)) +} + +func TestLazyCopyMergeFromIsAMergePatch(t *testing.T) { + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod"}} + lazy := newLazyCopy(pod) + lazy.Mutate().Labels = map[string]string{"key": "value"} + + assert.Equal(t, client.MergeFrom(pod).Type(), lazy.MergeFrom().Type()) +} + +// Pins the hazard documented on lazyCopy: the caller keeps the pointer it +// handed to newLazyCopy, so it can write through it without going via Mutate. +// A write that lands before the first Mutate is already in the snapshot, so it +// is absent from the patch and never reaches the API server. The type cannot +// prevent this, which is why the ordering is a caller invariant rather than a +// guarantee. +func TestLazyCopyDropsWritesMadeBeforeTheFirstMutate(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Annotations: map[string]string{"smuggled": "old", "declared": "old"}, + }, + } + lazy := newLazyCopy(pod) + + // Bypasses Mutate, so it is captured by the snapshot taken below. + pod.Annotations["smuggled"] = "new" + + lazy.Mutate().Annotations["declared"] = "new" + + data, err := lazy.MergeFrom().Data(pod) + require.NoError(t, err) + assert.JSONEq(t, `{"metadata":{"annotations":{"declared":"new"}}}`, string(data)) +} + +// The status patch in patchAppliedActionableRevisionStatus relies on being able +// to attach an optimistic lock, so that a patch computed from a stale read is +// rejected by the API server rather than silently moving the applied revision +// backwards. Without the precondition the patch can never conflict, so the +// surrounding RetryOnConflict would never fire. +func TestLazyCopyMergeFromForwardsMergeOptions(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + ResourceVersion: "42", + }, + } + lazy := newLazyCopy(pod) + lazy.Mutate().Labels = map[string]string{"key": "value"} + + data, err := lazy.MergeFrom(client.MergeFromWithOptimisticLock{}).Data(pod) + require.NoError(t, err) + assert.JSONEq( + t, + `{"metadata":{"labels":{"key":"value"},"resourceVersion":"42"}}`, + string(data), + "the lock is carried as a resourceVersion precondition in the patch body, taken from the snapshot rather than the mutated object", + ) +}