fix: Stop counting routine waits as reconcile errors

internal/controller/instance_controller.go

@@ -238,7 +238,8 @@ type InstanceReconciler struct {
 // +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch
 
 //nolint:gocyclo // conditions are reconciled, persisted, then returned as errors; the ordered pipeline is inherently branchy
-func (r *InstanceReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (_ ctrl.Result, err error) {
+func (r *InstanceReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (out ctrl.Result, err error) {
+	defer func(ctx context.Context) { out, err = settleRoutineErrors(ctx, "instance", out, err) }(ctx)
 	logger := log.FromContext(ctx)
 
 	cl, err := r.mgr.GetCluster(ctx, req.ClusterName)
@@ -262,7 +263,7 @@ func (r *InstanceReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ
 		return ctrl.Result{}, fmt.Errorf("failed to finalize: %w", err)
 	}
 	if finalizationResult.Updated {
-		if err = cl.GetClient().Update(ctx, &instance); err != nil {
+		if err = cl.GetClient().Update(ctx, &instance); client.IgnoreNotFound(err) != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to update based on finalization result: %w", err)
 		}
 		return ctrl.Result{}, nil
@@ -1233,7 +1234,7 @@ func (r *InstanceReconciler) Finalize(ctx context.Context, obj client.Object) (f
 		}
 		return finalizer.Result{}, fmt.Errorf("failed checking downstream write-back deletion: %w", err)
 	}
-	return finalizer.Result{}, fmt.Errorf("waiting for downstream write-back instance %s/%s to be deleted", instance.Namespace, instance.Name)
+	return finalizer.Result{}, fmt.Errorf("%w: downstream write-back instance %s/%s", errRemovalPending, instance.Namespace, instance.Name)
 }
 
 // writeBackToUpstream copies the Instance spec and status to the upstream

internal/controller/instance_projector.go

@@ -81,7 +81,7 @@ func (r *InstanceProjector) Reconcile(ctx context.Context, req ctrl.Request) (ct
 			"instance", req.NamespacedName, "error", err)
 		return ctrl.Result{RequeueAfter: projectClusterEngageWait}, nil
 	}
-	return ctrl.Result{}, err
+	return settleRoutineErrors(ctx, "instance-projector", ctrl.Result{}, err)
 }
 
 func (r *InstanceProjector) reconcile(ctx context.Context, req ctrl.Request) error {
@@ -287,7 +287,7 @@ func (r *InstanceProjector) deleteProjection(ctx context.Context, key types.Name
 		}
 		return fmt.Errorf("failed checking instance projection %s deletion in project cluster %q: %w", projectionKey, clusterName, err)
 	}
-	return fmt.Errorf("waiting for instance projection %s to be deleted in project cluster %q", projectionKey, clusterName)
+	return fmt.Errorf("%w: instance projection %s in project cluster %q", errRemovalPending, projectionKey, clusterName)
 }
 
 // SetupWithManager registers the InstanceProjector on mgr, which must be the
@@ -319,6 +319,5 @@ func reconcileDNSProjectionLifetime(ctx context.Context, reader client.Reader, w
 			return err
 		}
 	}
-	// Returning an error uses the controller's exponential retry backoff.
-	return fmt.Errorf("waiting for DNS projection lifetime replacement")
+	return fmt.Errorf("%w: DNS projection lifetime replacement", errRemovalPending)
 }

internal/controller/instance_projector_test.go

@@ -542,7 +542,7 @@ func TestInstanceProjectionDeletedBeforeCellInstanceFinalizes(t *testing.T) {
 	cellReconciler := &InstanceReconciler{FederationClient: hubClient}
 
 	_, err := cellReconciler.Finalize(ctx, writeBack)
-	require.ErrorContains(t, err, "waiting for downstream write-back")
+	require.ErrorIs(t, err, errRemovalPending)
 	var terminating computev1alpha.Instance
 	require.NoError(t, hubClient.Get(ctx, projectorRequest().NamespacedName, &terminating))
 	require.NotNil(t, terminating.DeletionTimestamp)
@@ -579,8 +579,9 @@ func TestInstanceProjectorWaitsForProjectDeletion(t *testing.T) {
 		Build()
 	projector := newTestProjector(hubClient, projectClient)
 
-	_, err := projector.Reconcile(ctx, projectorRequest())
-	require.ErrorContains(t, err, "waiting for instance projection")
+	result, err := projector.Reconcile(ctx, projectorRequest())
+	require.NoError(t, err, "waiting on project deletion is not a reconcile error")
+	require.Equal(t, removalPollInterval, result.RequeueAfter)
 	var pending computev1alpha.Instance
 	require.NoError(t, projectClient.Get(ctx, types.NamespacedName{
 		Name: projTestInstanceName, Namespace: projTestProjNS,
@@ -682,8 +683,8 @@ func TestInstanceProjectorReplacesDifferentSourceLifetime(t *testing.T) {
 	r := newTestProjector(newKarmadaFakeClient(upstream), cl)
 	r.InternalDNSEnabled = true
 	result, err := r.Reconcile(context.Background(), projectorRequest())
-	require.ErrorContains(t, err, "waiting for DNS projection lifetime replacement")
-	require.Zero(t, result.RequeueAfter)
+	require.NoError(t, err)
+	require.Equal(t, removalPollInterval, result.RequeueAfter)
 	require.True(t, apierrors.IsNotFound(cl.Get(context.Background(), client.ObjectKeyFromObject(projection), projection)))
 	_, err = r.Reconcile(context.Background(), projectorRequest())
 	require.NoError(t, err)

internal/controller/internaldns_publisher.go

@@ -123,7 +123,8 @@ type InternalDNSPublisherReconciler struct {
 	SkipLocations map[string]bool
 }
 
-func (r *InternalDNSPublisherReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (ctrl.Result, error) {
+func (r *InternalDNSPublisherReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (out ctrl.Result, err error) {
+	defer func(ctx context.Context) { out, err = settleRoutineErrors(ctx, r.controllerName(), out, err) }(ctx)
 	ctx, cancel := context.WithTimeout(ctx, internalDNSAPITimeout)
 	defer cancel()
 
@@ -711,12 +712,8 @@ func (r *InternalDNSPublisherReconciler) SetupWithManager(mgr mcmanager.Manager)
 	if concurrency <= 0 {
 		concurrency = defaultDNSConcurrency
 	}
-	name := "internal-dns-instance-publisher"
-	if r.LocalRegion != "" {
-		name = "internal-dns-local-instance-publisher"
-	}
 	return mcbuilder.ControllerManagedBy(mgr).
-		Named(name).
+		Named(r.controllerName()).
 		For(&computev1alpha.Instance{}, mcbuilder.WithEngageWithLocalCluster(false)).
 		Owns(newDNSObject(dnsRegistrationGVK, "", ""), mcbuilder.WithEngageWithLocalCluster(false)).
 		Owns(newDNSObject(dnsContributionGVK, "", ""), mcbuilder.WithEngageWithLocalCluster(false)).
@@ -743,3 +740,10 @@ func (r *InternalDNSPublisherReconciler) SetupWithManager(mgr mcmanager.Manager)
 func internalDNSPollInterval(base time.Duration) time.Duration {
 	return base + time.Duration(rand.Int64N(int64(base/2)))
 }
+
+func (r *InternalDNSPublisherReconciler) controllerName() string {
+	if r.LocalRegion != "" {
+		return "internal-dns-local-instance-publisher"
+	}
+	return "internal-dns-instance-publisher"
+}

internal/controller/reconcile_outcome.go

@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+
+package controller
+
+import (
+	"context"
+	"errors"
+	"time"
+
+	"github.com/prometheus/client_golang/prometheus"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/log"
+	ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics"
+)
+
+// errRemovalPending marks a reconcile that is waiting for an object to finish
+// deleting where no watch reports that it has gone.
+var errRemovalPending = errors.New("removal pending")
+
+// removalPollInterval is how often a reconcile held by errRemovalPending
+// rechecks. The error backoff grows to minutes and would hold deletion that
+// long.
+const removalPollInterval = 5 * time.Second
+
+// conflictRetryDelay is how soon a reconcile that lost a write to a newer
+// version of the object retries against that version.
+const conflictRetryDelay = time.Second
+
+const (
+	retryReasonConflict       = "conflict"
+	retryReasonRemovalPending = "removal_pending"
+
+	labelController = "controller"
+	labelReason     = "reason"
+)
+
+// routineRetries counts the reconciles settleRoutineErrors retried instead of
+// failing, so a wait that never ends stays visible outside the error rate.
+var routineRetries = prometheus.NewCounterVec(
+	prometheus.CounterOpts{
+		Namespace: metricsNamespace,
+		Name:      "reconcile_routine_retries_total",
+		Help:      "Reconciles retried on a timer instead of failing, by controller and reason (conflict, removal_pending).",
+	},
+	[]string{labelController, labelReason},
+)
+
+func init() {
+	ctrlmetrics.Registry.MustRegister(routineRetries)
+}
+
+// settleRoutineErrors turns outcomes that are routine on a busy control plane
+// into a timed retry, so the reconcile error count reports only failures.
+// A write conflict and a wait on removal both resolve by trying again. An
+// error that mixes the two is retried on the slower removal poll.
+func settleRoutineErrors(ctx context.Context, controller string, result ctrl.Result, err error) (ctrl.Result, error) {
+	if err == nil {
+		return result, nil
+	}
+	logger := log.FromContext(ctx)
+	switch {
+	case everyError(err, apierrors.IsConflict):
+		routineRetries.WithLabelValues(controller, retryReasonConflict).Inc()
+		logger.V(1).Info("write lost to a newer version; retrying", labelReason, err.Error())
+		return ctrl.Result{RequeueAfter: conflictRetryDelay}, nil
+	case everyError(err, isRoutine):
+		routineRetries.WithLabelValues(controller, retryReasonRemovalPending).Inc()
+		logger.V(1).Info("waiting for removal; retrying", labelReason, err.Error())
+		return ctrl.Result{RequeueAfter: removalPollInterval}, nil
+	}
+	return result, err
+}
+
+func isRoutine(err error) bool {
+	return errors.Is(err, errRemovalPending) || apierrors.IsConflict(err)
+}
+
+// everyError reports whether match holds for every error err combines, so a
+// routine error never hides a real one aggregated alongside it. Wrapping is
+// followed only down a single chain, and the first combining error found
+// splits the walk, so a real error beside a nested aggregate is still seen.
+func everyError(err error, match func(error) bool) bool {
+	for e := err; e != nil; e = errors.Unwrap(e) {
+		var errs []error
+		switch combined := e.(type) {
+		case kerrors.Aggregate:
+			errs = combined.Errors()
+		case interface{ Unwrap() []error }:
+			errs = combined.Unwrap()
+		default:
+			continue
+		}
+		for _, member := range errs {
+			if !everyError(member, match) {
+				return false
+			}
+		}
+		return len(errs) > 0
+	}
+	return match(err)
+}

internal/controller/reconcile_outcome_test.go

@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+
+package controller
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
+
+	computev1alpha "go.datum.net/compute/api/v1alpha"
+)
+
+func TestSettleRoutineErrors(t *testing.T) {
+	t.Parallel()
+
+	gr := schema.GroupResource{Group: computev1alpha.GroupVersion.Group, Resource: "workloads"}
+	conflict := apierrors.NewConflict(gr, "w", errors.New("the object has been modified"))
+	pending := fmt.Errorf("%w: hub copy", errRemovalPending)
+	failure := errors.New("boom")
+
+	tests := []struct {
+		name       string
+		in         ctrl.Result
+		err        error
+		wantResult ctrl.Result
+		wantErr    error
+	}{
+		{name: "success keeps its result", in: ctrl.Result{RequeueAfter: removalPollInterval * 3}, wantResult: ctrl.Result{RequeueAfter: removalPollInterval * 3}},
+		{name: "wrapped conflict retries soon", err: fmt.Errorf("failed updating workload status: %w", conflict), wantResult: ctrl.Result{RequeueAfter: conflictRetryDelay}},
+		{name: "removal pending polls", err: fmt.Errorf("failed to finalize: %w", kerrors.NewAggregate([]error{pending})), wantResult: ctrl.Result{RequeueAfter: removalPollInterval}},
+		{name: "aggregated conflicts retry soon", err: kerrors.NewAggregate([]error{conflict, conflict}), wantResult: ctrl.Result{RequeueAfter: conflictRetryDelay}},
+		{name: "real failure alongside a conflict is reported", err: kerrors.NewAggregate([]error{conflict, failure}), wantErr: failure},
+		{name: "real failure joined with a pending removal is reported", err: errors.Join(pending, failure), wantErr: failure},
+		{name: "conflict and pending removal poll for removal", err: kerrors.NewAggregate([]error{conflict, pending}), wantResult: ctrl.Result{RequeueAfter: removalPollInterval}},
+		{name: "real failure beside a nested aggregate is reported", err: errors.Join(kerrors.NewAggregate([]error{conflict}), failure), wantErr: failure},
+		{name: "real failure wrapped beside an aggregate is reported", err: fmt.Errorf("%w; %w", kerrors.NewAggregate([]error{pending}), failure), wantErr: failure},
+		{name: "not found is not swallowed", err: apierrors.NewNotFound(gr, "w"), wantErr: apierrors.NewNotFound(gr, "w")},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			result, err := settleRoutineErrors(context.Background(), "test", tt.in, tt.err)
+			if tt.wantErr == nil {
+				require.NoError(t, err)
+				assert.Equal(t, tt.wantResult, result)
+				return
+			}
+			require.Error(t, err)
+			assert.ErrorContains(t, err, tt.wantErr.Error())
+		})
+	}
+}
+
+func TestWorkloadDeploymentFederator_FinalizerReleaseRaces(t *testing.T) {
+	t.Parallel()
+
+	gr := computev1alpha.GroupVersion.WithResource("workloaddeployments").GroupResource()
+	tests := []struct {
+		name       string
+		updateErr  error
+		wantResult ctrl.Result
+	}{
+		{name: "already deleted", updateErr: apierrors.NewNotFound(gr, testWDName)},
+		{name: "stale copy", updateErr: apierrors.NewConflict(gr, testWDName, errors.New("the object has been modified")), wantResult: ctrl.Result{RequeueAfter: conflictRetryDelay}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			wd := testWorkloadDeployment(withFinalizer, withDeletionTimestamp)
+			wd.Annotations = map[string]string{computev1alpha.FederationNamespaceAnnotation: testKarmadaNSStr}
+			projectClient := fake.NewClientBuilder().
+				WithScheme(newProjectScheme()).
+				WithObjects(wd).
+				WithInterceptorFuncs(interceptor.Funcs{
+					Update: func(context.Context, client.WithWatch, client.Object, ...client.UpdateOption) error {
+						return tt.updateErr
+					},
+				}).
+				Build()
+			r := newTestFederator(projectClient, newKarmadaFakeClient(testHubNamespace()))
+
+			result, err := r.Reconcile(context.Background(), reconcileRequest())
+			require.NoError(t, err)
+			assert.Equal(t, tt.wantResult, result)
+		})
+	}
+}

internal/controller/workload_controller.go

@@ -76,7 +76,8 @@ type WorkloadReconciler struct {
 // +kubebuilder:rbac:groups=services.miloapis.com,resources=serviceavailabilities,verbs=get;list;watch
 // +kubebuilder:rbac:groups=locations.miloapis.com,resources=locations,verbs=get;list;watch
 
-func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (ctrl.Result, error) {
+func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (out ctrl.Result, err error) {
+	defer func(ctx context.Context) { out, err = settleRoutineErrors(ctx, "workload", out, err) }(ctx)
 	logger := log.FromContext(ctx)
 
 	cl, err := r.mgr.GetCluster(ctx, req.ClusterName)
@@ -106,7 +107,7 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ
 		}
 	}
 	if finalizationResult.Updated {
-		if err = cl.GetClient().Update(ctx, &workload); err != nil {
+		if err = cl.GetClient().Update(ctx, &workload); client.IgnoreNotFound(err) != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to update based on finalization result: %w", err)
 		}
 		return ctrl.Result{}, nil

internal/controller/workloaddeployment_controller.go

@@ -118,7 +118,8 @@ func workloadDeploymentPodSelector(deployment *computev1alpha.WorkloadDeployment
 // a marker so regenerating the role keeps it.
 // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networks,verbs=get;list;watch
 
-func (r *WorkloadDeploymentReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (ctrl.Result, error) {
+func (r *WorkloadDeploymentReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (out ctrl.Result, err error) {
+	defer func(ctx context.Context) { out, err = settleRoutineErrors(ctx, "workloaddeployment", out, err) }(ctx)
 	logger := log.FromContext(ctx)
 
 	cl, err := r.mgr.GetCluster(ctx, req.ClusterName)
@@ -141,7 +142,7 @@ func (r *WorkloadDeploymentReconciler) Reconcile(ctx context.Context, req mcreco
 		return ctrl.Result{}, fmt.Errorf("failed to finalize: %w", err)
 	}
 	if finalizationResult.Updated {
-		if err = cl.GetClient().Update(ctx, &deployment); err != nil {
+		if err = cl.GetClient().Update(ctx, &deployment); client.IgnoreNotFound(err) != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to update based on finalization result: %w", err)
 		}
 		// The finalizer-add Update is metadata-only and may be filtered by event