From fef189b5e8e99617e33e54bc9c7dc3cc017c295a Mon Sep 17 00:00:00 2001 From: Scot Schuchert-Wells Date: Fri, 6 Nov 2026 19:31:57 +0000 Subject: [PATCH 1/3] fix: Stop counting routine waits as reconcile errors Several controllers return errors for outcomes that resolve on retry: a write that lost to a newer version, a wait on an object's deletion, and a finalizer release on an object that is already gone. Each one counted as a reconcile failure and drew the error backoff. Key changes: - add a shared outcome helper that turns write conflicts into a 1s retry and removal waits into a 5s poll, and reports any real error combined with them - apply it to the workload, workload deployment, federator, instance, instance projector and internal DNS publisher reconcilers - report the projector's projection and DNS lifetime waits and the cell instance's write-back wait as removal pending - ignore NotFound when releasing a finalizer on an object already deleted --- internal/controller/instance_controller.go | 7 +- internal/controller/instance_projector.go | 7 +- .../controller/instance_projector_test.go | 11 ++- internal/controller/internaldns_publisher.go | 3 +- internal/controller/reconcile_outcome.go | 72 ++++++++++++++ internal/controller/reconcile_outcome_test.go | 95 +++++++++++++++++++ internal/controller/workload_controller.go | 5 +- .../workloaddeployment_controller.go | 5 +- .../workloaddeployment_federator.go | 25 ++--- .../workloaddeployment_federator_test.go | 4 +- 10 files changed, 196 insertions(+), 38 deletions(-) create mode 100644 internal/controller/reconcile_outcome.go create mode 100644 internal/controller/reconcile_outcome_test.go diff --git a/internal/controller/instance_controller.go b/internal/controller/instance_controller.go index 2c87d16f..5e6979c6 100644 --- a/internal/controller/instance_controller.go +++ b/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() { out, err = settleRoutineErrors(ctx, out, err) }() 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 diff --git a/internal/controller/instance_projector.go b/internal/controller/instance_projector.go index 6b45c17d..744abd20 100644 --- a/internal/controller/instance_projector.go +++ b/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, 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) } diff --git a/internal/controller/instance_projector_test.go b/internal/controller/instance_projector_test.go index da93422d..47c4b62f 100644 --- a/internal/controller/instance_projector_test.go +++ b/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) diff --git a/internal/controller/internaldns_publisher.go b/internal/controller/internaldns_publisher.go index 7c436f1d..b75178cd 100644 --- a/internal/controller/internaldns_publisher.go +++ b/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() { out, err = settleRoutineErrors(ctx, out, err) }() ctx, cancel := context.WithTimeout(ctx, internalDNSAPITimeout) defer cancel() diff --git a/internal/controller/reconcile_outcome.go b/internal/controller/reconcile_outcome.go new file mode 100644 index 00000000..29a33e51 --- /dev/null +++ b/internal/controller/reconcile_outcome.go @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controller + +import ( + "context" + "errors" + "time" + + 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" +) + +// 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 + +// 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. +func settleRoutineErrors(ctx context.Context, result ctrl.Result, err error) (ctrl.Result, error) { + if err == nil { + return result, nil + } + logger := log.FromContext(ctx) + switch { + case everyError(err, isRemovalPending): + logger.V(1).Info("waiting for removal; retrying", "reason", err.Error()) + return ctrl.Result{RequeueAfter: removalPollInterval}, nil + case everyError(err, apierrors.IsConflict): + logger.V(1).Info("write lost to a newer version; retrying", "reason", err.Error()) + return ctrl.Result{RequeueAfter: conflictRetryDelay}, nil + } + return result, err +} + +func isRemovalPending(err error) bool { + return errors.Is(err, errRemovalPending) +} + +// everyError reports whether match holds for every error err combines, so a +// routine error never hides a real one aggregated alongside it. +func everyError(err error, match func(error) bool) bool { + var errs []error + var agg kerrors.Aggregate + var joined interface{ Unwrap() []error } + switch { + case errors.As(err, &agg): + errs = agg.Errors() + case errors.As(err, &joined): + errs = joined.Unwrap() + default: + return match(err) + } + for _, e := range errs { + if !everyError(e, match) { + return false + } + } + return len(errs) > 0 +} diff --git a/internal/controller/reconcile_outcome_test.go b/internal/controller/reconcile_outcome_test.go new file mode 100644 index 00000000..2cafa3ea --- /dev/null +++ b/internal/controller/reconcile_outcome_test.go @@ -0,0 +1,95 @@ +// 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: "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(), 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) + }) + } +} diff --git a/internal/controller/workload_controller.go b/internal/controller/workload_controller.go index 5bc3590a..ab05e7bc 100644 --- a/internal/controller/workload_controller.go +++ b/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() { out, err = settleRoutineErrors(ctx, out, err) }() 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 diff --git a/internal/controller/workloaddeployment_controller.go b/internal/controller/workloaddeployment_controller.go index 56a9709f..3685cdb5 100644 --- a/internal/controller/workloaddeployment_controller.go +++ b/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() { out, err = settleRoutineErrors(ctx, out, err) }() 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 diff --git a/internal/controller/workloaddeployment_federator.go b/internal/controller/workloaddeployment_federator.go index b59777cf..6e049c55 100644 --- a/internal/controller/workloaddeployment_federator.go +++ b/internal/controller/workloaddeployment_federator.go @@ -8,7 +8,6 @@ import ( "fmt" "maps" "strings" - "time" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -108,7 +107,8 @@ type WorkloadDeploymentFederator struct { // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloaddeployments/finalizers,verbs=update // +kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch -func (r *WorkloadDeploymentFederator) Reconcile(ctx context.Context, req mcreconcile.Request) (ctrl.Result, error) { +func (r *WorkloadDeploymentFederator) Reconcile(ctx context.Context, req mcreconcile.Request) (out ctrl.Result, err error) { + defer func() { out, err = settleRoutineErrors(ctx, out, err) }() logger := log.FromContext(ctx) // An empty cluster name resolves to the local host management cluster, which @@ -136,17 +136,11 @@ func (r *WorkloadDeploymentFederator) Reconcile(ctx context.Context, req mcrecon } finalizationResult, err := r.finalizers.Finalize(ctx, &deployment) - if errors.Is(err, errDownstreamRemovalPending) { - // Not a failure: retry on a short timer rather than the error backoff, - // which grows to minutes and would hold project deletion that long. - logger.V(1).Info("waiting for downstream cleanup before releasing deployment", "reason", err.Error()) - return ctrl.Result{RequeueAfter: downstreamRemovalPollInterval}, nil - } if err != nil { 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) } return ctrl.Result{}, nil @@ -256,7 +250,7 @@ func (r *WorkloadDeploymentFederator) Finalize(ctx context.Context, obj client.O var pending error for _, downstreamNS := range hubNamespaces { err := r.finalizeInHubNamespace(ctx, deployment, downstreamNS) - if errors.Is(err, errDownstreamRemovalPending) { + if errors.Is(err, errRemovalPending) { pending = err continue } @@ -312,13 +306,6 @@ func (r *WorkloadDeploymentFederator) finalizeInHubNamespace(ctx context.Context return nil } -// errDownstreamRemovalPending marks a finalization that is waiting for hub -// objects to go away. -var errDownstreamRemovalPending = errors.New("downstream removal pending") - -// downstreamRemovalPollInterval is how often a held deployment rechecks the hub. -const downstreamRemovalPollInterval = 5 * time.Second - // waitForDownstreamRemoval holds the project deployment until its hub copy and // the Instances written back for it are gone. The project control plane is // only reclaimed once nothing is left in it, so holding the deployment keeps @@ -330,7 +317,7 @@ func (r *WorkloadDeploymentFederator) waitForDownstreamRemoval(ctx context.Conte var hub computev1alpha.WorkloadDeployment err := r.FederationClient.Get(ctx, client.ObjectKey{Namespace: downstreamNS, Name: name}, &hub) if err == nil { - return fmt.Errorf("%w: downstream deployment %s/%s still exists", errDownstreamRemovalPending, downstreamNS, name) + return fmt.Errorf("%w: downstream deployment %s/%s still exists", errRemovalPending, downstreamNS, name) } if !apierrors.IsNotFound(err) { return fmt.Errorf("failed to check downstream deployment %s/%s removal: %w", downstreamNS, name, err) @@ -352,7 +339,7 @@ func (r *WorkloadDeploymentFederator) waitForDownstreamRemoval(ctx context.Conte return fmt.Errorf("failed to delete downstream instance %s/%s: %w", downstreamNS, instance.Name, err) } } - return fmt.Errorf("%w: %d downstream instances of %s/%s still exist", errDownstreamRemovalPending, len(instances.Items), downstreamNS, name) + return fmt.Errorf("%w: %d downstream instances of %s/%s still exist", errRemovalPending, len(instances.Items), downstreamNS, name) } // cleanupLegacyPropagationPolicies removes the PropagationPolicies a diff --git a/internal/controller/workloaddeployment_federator_test.go b/internal/controller/workloaddeployment_federator_test.go index 54b870df..ed606def 100644 --- a/internal/controller/workloaddeployment_federator_test.go +++ b/internal/controller/workloaddeployment_federator_test.go @@ -818,7 +818,7 @@ func TestWorkloadDeploymentFederator_FinalizeWaitsForDownstreamRemoval(t *testin } _, err := r.Finalize(ctx, wd) - require.ErrorIs(t, err, errDownstreamRemovalPending) + require.ErrorIs(t, err, errRemovalPending) assert.False(t, exists(hubWD)) require.NoError(t, karmadaClient.Get(ctx, client.ObjectKeyFromObject(writeBack), writeBack)) assert.False(t, writeBack.DeletionTimestamp.IsZero(), "the finalizer deletes write-backs itself") @@ -828,7 +828,7 @@ func TestWorkloadDeploymentFederator_FinalizeWaitsForDownstreamRemoval(t *testin // Reconcile turns the hold into a short requeue, not an error. result, err := r.Reconcile(context.Background(), reconcileRequest()) require.NoError(t, err) - assert.Equal(t, downstreamRemovalPollInterval, result.RequeueAfter) + assert.Equal(t, removalPollInterval, result.RequeueAfter) writeBack.Finalizers = nil require.NoError(t, karmadaClient.Update(ctx, writeBack)) From d7442d3f237d2a8144b7ad77345df03a65abf4ef Mon Sep 17 00:00:00 2001 From: Scot Schuchert-Wells Date: Fri, 6 Nov 2026 19:31:57 +0000 Subject: [PATCH 2/3] fix: Count routine retries and tighten error splitting Review on the first pass noted that a wait which never ends polled silently, and that a real error nested beside an aggregate could be classified as routine. Key changes: - count retried reconciles by controller and reason, so a stuck wait or conflict loop shows on dashboards without entering the error rate - split combined errors at the first aggregate or join on the wrap chain, so a real error beside a nested aggregate is reported - poll on the removal interval when conflicts and removal waits mix - capture the internal DNS publisher's context before its timeout wraps it --- internal/controller/instance_controller.go | 2 +- internal/controller/instance_projector.go | 2 +- internal/controller/internaldns_publisher.go | 15 ++-- internal/controller/reconcile_outcome.go | 74 +++++++++++++------ internal/controller/reconcile_outcome_test.go | 5 +- internal/controller/workload_controller.go | 2 +- .../workloaddeployment_controller.go | 2 +- .../workloaddeployment_federator.go | 4 +- 8 files changed, 71 insertions(+), 35 deletions(-) diff --git a/internal/controller/instance_controller.go b/internal/controller/instance_controller.go index 5e6979c6..a01cf78a 100644 --- a/internal/controller/instance_controller.go +++ b/internal/controller/instance_controller.go @@ -239,7 +239,7 @@ type InstanceReconciler struct { //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) (out ctrl.Result, err error) { - defer func() { out, err = settleRoutineErrors(ctx, out, err) }() + 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) diff --git a/internal/controller/instance_projector.go b/internal/controller/instance_projector.go index 744abd20..19d87c0b 100644 --- a/internal/controller/instance_projector.go +++ b/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 settleRoutineErrors(ctx, ctrl.Result{}, err) + return settleRoutineErrors(ctx, "instance-projector", ctrl.Result{}, err) } func (r *InstanceProjector) reconcile(ctx context.Context, req ctrl.Request) error { diff --git a/internal/controller/internaldns_publisher.go b/internal/controller/internaldns_publisher.go index b75178cd..df48a630 100644 --- a/internal/controller/internaldns_publisher.go +++ b/internal/controller/internaldns_publisher.go @@ -124,7 +124,7 @@ type InternalDNSPublisherReconciler struct { } func (r *InternalDNSPublisherReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (out ctrl.Result, err error) { - defer func() { out, err = settleRoutineErrors(ctx, out, err) }() + defer func(ctx context.Context) { out, err = settleRoutineErrors(ctx, r.controllerName(), out, err) }(ctx) ctx, cancel := context.WithTimeout(ctx, internalDNSAPITimeout) defer cancel() @@ -712,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)). @@ -744,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" +} diff --git a/internal/controller/reconcile_outcome.go b/internal/controller/reconcile_outcome.go index 29a33e51..09252ac4 100644 --- a/internal/controller/reconcile_outcome.go +++ b/internal/controller/reconcile_outcome.go @@ -7,10 +7,12 @@ import ( "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 @@ -26,47 +28,73 @@ const removalPollInterval = 5 * time.Second // version of the object retries against that version. const conflictRetryDelay = time.Second +const ( + retryReasonConflict = "conflict" + retryReasonRemovalPending = "removal_pending" +) + +// 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{"controller", "reason"}, +) + +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. -func settleRoutineErrors(ctx context.Context, result ctrl.Result, err error) (ctrl.Result, error) { +// 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, isRemovalPending): - logger.V(1).Info("waiting for removal; retrying", "reason", err.Error()) - return ctrl.Result{RequeueAfter: removalPollInterval}, nil case everyError(err, apierrors.IsConflict): + routineRetries.WithLabelValues(controller, retryReasonConflict).Inc() logger.V(1).Info("write lost to a newer version; retrying", "reason", 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", "reason", err.Error()) + return ctrl.Result{RequeueAfter: removalPollInterval}, nil } return result, err } -func isRemovalPending(err error) bool { - return errors.Is(err, errRemovalPending) +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. +// 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 { - var errs []error - var agg kerrors.Aggregate - var joined interface{ Unwrap() []error } - switch { - case errors.As(err, &agg): - errs = agg.Errors() - case errors.As(err, &joined): - errs = joined.Unwrap() - default: - return match(err) - } - for _, e := range errs { - if !everyError(e, match) { - return false + 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 len(errs) > 0 + return match(err) } diff --git a/internal/controller/reconcile_outcome_test.go b/internal/controller/reconcile_outcome_test.go index 2cafa3ea..c260cbff 100644 --- a/internal/controller/reconcile_outcome_test.go +++ b/internal/controller/reconcile_outcome_test.go @@ -42,12 +42,15 @@ func TestSettleRoutineErrors(t *testing.T) { {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(), tt.in, tt.err) + result, err := settleRoutineErrors(context.Background(), "test", tt.in, tt.err) if tt.wantErr == nil { require.NoError(t, err) assert.Equal(t, tt.wantResult, result) diff --git a/internal/controller/workload_controller.go b/internal/controller/workload_controller.go index ab05e7bc..7eff89dc 100644 --- a/internal/controller/workload_controller.go +++ b/internal/controller/workload_controller.go @@ -77,7 +77,7 @@ type WorkloadReconciler struct { // +kubebuilder:rbac:groups=locations.miloapis.com,resources=locations,verbs=get;list;watch func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (out ctrl.Result, err error) { - defer func() { out, err = settleRoutineErrors(ctx, out, err) }() + 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) diff --git a/internal/controller/workloaddeployment_controller.go b/internal/controller/workloaddeployment_controller.go index 3685cdb5..2b1aec80 100644 --- a/internal/controller/workloaddeployment_controller.go +++ b/internal/controller/workloaddeployment_controller.go @@ -119,7 +119,7 @@ func workloadDeploymentPodSelector(deployment *computev1alpha.WorkloadDeployment // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networks,verbs=get;list;watch func (r *WorkloadDeploymentReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (out ctrl.Result, err error) { - defer func() { out, err = settleRoutineErrors(ctx, out, err) }() + 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) diff --git a/internal/controller/workloaddeployment_federator.go b/internal/controller/workloaddeployment_federator.go index 6e049c55..f25958fd 100644 --- a/internal/controller/workloaddeployment_federator.go +++ b/internal/controller/workloaddeployment_federator.go @@ -108,7 +108,9 @@ type WorkloadDeploymentFederator struct { // +kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch func (r *WorkloadDeploymentFederator) Reconcile(ctx context.Context, req mcreconcile.Request) (out ctrl.Result, err error) { - defer func() { out, err = settleRoutineErrors(ctx, out, err) }() + defer func(ctx context.Context) { + out, err = settleRoutineErrors(ctx, "workload-deployment-federator", out, err) + }(ctx) logger := log.FromContext(ctx) // An empty cluster name resolves to the local host management cluster, which From 92a14d74f6523ef7e5dcfa610015d31bc3679003 Mon Sep 17 00:00:00 2001 From: Scot Schuchert-Wells Date: Fri, 6 Nov 2026 19:31:58 +0000 Subject: [PATCH 3/3] style: Name the routine retry metric labels Satisfy goconst by naming the controller and reason labels the routine retry counter and its logs share. --- internal/controller/reconcile_outcome.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/controller/reconcile_outcome.go b/internal/controller/reconcile_outcome.go index 09252ac4..ffc4139b 100644 --- a/internal/controller/reconcile_outcome.go +++ b/internal/controller/reconcile_outcome.go @@ -31,6 +31,9 @@ const conflictRetryDelay = time.Second const ( retryReasonConflict = "conflict" retryReasonRemovalPending = "removal_pending" + + labelController = "controller" + labelReason = "reason" ) // routineRetries counts the reconciles settleRoutineErrors retried instead of @@ -41,7 +44,7 @@ var routineRetries = prometheus.NewCounterVec( Name: "reconcile_routine_retries_total", Help: "Reconciles retried on a timer instead of failing, by controller and reason (conflict, removal_pending).", }, - []string{"controller", "reason"}, + []string{labelController, labelReason}, ) func init() { @@ -60,11 +63,11 @@ func settleRoutineErrors(ctx context.Context, controller string, result ctrl.Res switch { case everyError(err, apierrors.IsConflict): routineRetries.WithLabelValues(controller, retryReasonConflict).Inc() - logger.V(1).Info("write lost to a newer version; retrying", "reason", err.Error()) + 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", "reason", err.Error()) + logger.V(1).Info("waiting for removal; retrying", labelReason, err.Error()) return ctrl.Result{RequeueAfter: removalPollInterval}, nil } return result, err