[Observability] Stop recording optimistic concurrency conflict errors that are retried in metrics

[jabellard]

What would you like to be added:

The work_sync_workload_duration_seconds metric should not count Kubernetes conflict errors (HTTP 409) as failures. These are expected retriable errors in distributed systems and should not be recorded with result="error".

Why is this needed:

Problem: Conflict Errors Cause False SLO Violations

In our environment, this metric shows 15% error rate (16.23x burn rate) causing critical alerts, but the system is actually healthy. Nearly all "errors" are Kubernetes conflict errors that the controller successfully retries.

Sample Logs

E0115 02:56:14.736986 Failed to update resource(kind=Deployment, default/nginx-deployment)
in cluster test-cluster-region1, err: Operation cannot be fulfilled on
deployments.apps "nginx-deployment": the object has been modified

E0115 02:56:19.000869 Failed to update resource(kind=Deployment, default/nginx-deployment)
in cluster test-cluster-region1, err: Operation cannot be fulfilled on
deployments.apps "nginx-deployment": the object has been modified

[Pattern repeats - controller retries and eventually succeeds]

These are HTTP 409 Conflict errors from Kubernetes optimistic concurrency control - normal, expected, and automatically retried.

Current Metrics

# Error rate: 15.8% (target is 99% success)
sum(rate(work_sync_workload_duration_seconds_count{result="error"}[5m])) /
sum(rate(work_sync_workload_duration_seconds_count[5m]))

# Burn rate: 16.23x (burning error budget 16x faster than allowed)
slo:current_burn_rate:ratio{sloth_slo="work-sync-workload-availability"}

Nearly all these "errors" are conflict errors that successfully retry.

Why This Matters

Conflict errors are not failures:

• Temporary and automatically retried (controller succeeds on next attempt)
• Expected in distributed systems (Kubernetes optimistic concurrency control)
• Not user-facing (workloads eventually sync successfully)

Impact:

• False critical alerts (teams paged for "healthy" system)
• Misleading dashboards (85% availability shown when actual is >99%)
• Alert fatigue and wasted engineering time

Current: Metric measures retry rate ("Did first attempt succeed?")
Expected: Metric should measure availability ("Did workload eventually sync?")

---

Proposed Change

File: pkg/controllers/execution/execution_controller.go

Skip recording the metric:

func (c *Controller) syncWork(...) (controllerruntime.Result, error) {
	start := time.Now()
	err := c.syncToClusters(ctx, clusterName, work)

	// Don't count conflict errors  - they are retriable
	...

Impact: Error rate 15% → <1%, Burn rate 16x → <1x

Iteration Tasks

• execution-controller (@RainbowMango , retry on conflict when syncing workload to member clusters #7106)
• binding-controller (@AnupamSingh2004, retry on conflict when syncing work for binding controllers #7121)
• cluster-binding-controller (@AnupamSingh2004, retry on conflict when syncing work for binding controllers #7121)

[jabellard]

Hey @RainbowMango . Would love to hear your thoughts.

[jabellard]

Hey @RainbowMango . Would love to hear your thoughts.

Also, this probably also effects some other metrics that I'm unaware of at this point.

[AnupamSingh2004]

Hi @jabellard,

Thank you for raising this issue! If possible, I'd like to work on this.

Based on my analysis of the codebase, I understand the problem: the work_sync_workload_duration_seconds metric currently records Kubernetes conflict errors (HTTP 409) as failures, even though these are expected retriable errors from optimistic concurrency control that the controller successfully handles on retry.

My proposed approach:

In pkg/controllers/execution/execution_controller.go, modify the syncWork function to skip recording the metric when the error is a conflict error:

func (c *Controller) syncWork(ctx context.Context, clusterName string, work *workv1alpha1.Work) (controllerruntime.Result, error) {
    start := time.Now()
    err := c.syncToClusters(ctx, clusterName, work)
    
    // Don't record conflict errors in metrics - they are retriable and expected
    if !apierrors.IsConflict(err) {
        metrics.ObserveSyncWorkloadLatency(err, start)
    }
    // ... rest of the function
}

I also noticed similar patterns in:

• pkg/controllers/binding/binding_controller.go (uses ObserveSyncWorkLatency)
• pkg/controllers/binding/cluster_resource_binding_controller.go (uses ObserveSyncWorkLatency)

Should I apply the same fix to these controllers as well, or would you prefer to keep the scope limited to the execution controller?

Please let me know if this approach looks good, or if you have any suggestions then i begin the implementation.

[RainbowMango]

In our environment, this metric shows 15% error rate (16.23x burn rate) causing critical alerts, but the system is actually healthy. Nearly all "errors" are Kubernetes conflict errors that the controller successfully retries.

Oh, I see. I agree with you that this error shouldn't be treated as an error.

I guess #7106 will mitigate this issue as well. With this patch, no conflict error will be counted. (Two birds with one stone :))

@jabellard @AnupamSingh2004 thank you both, but please look at the PR.

[RainbowMango]

@jabellard For the binding-controller and cluster-binding-controller, can you help point out the code or the name of the metrics?

[jabellard]

@jabellard For the binding-controller and cluster-binding-controller, can you help point out the code or the name of the metrics?

Yup. I get some similar errors from other controllers. Dashboard from one of my Karmada instances:

Will post more details in a few. Btw, I wanna do some stress/scalability testing. Will share results with the community once I'm done with that.

[jabellard]

We need to make a similar change for the resource binding and cluster resource binding controllers. They record the binding_sync_work_duration_seconds metric.

The code should be more like this:

start := time.Now()
  err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
      return ensureWork(ctx, c.Client, c.ResourceInterpreter, workload, c.OverrideManager, binding, apiextensionsv1.NamespaceScoped)
  })
  metrics.ObserveSyncWorkLatency(err, start)

@AnupamSingh2004 , would you like to send PRs for this?

[AnupamSingh2004]

Hi @jabellard,

Yes, I'd be happy to send PRs for this!

I'll implement the fix for both:

• pkg/controllers/binding/binding_controller.go
• pkg/controllers/binding/cluster_resource_binding_controller.go

Following your suggestion, I'll wrap the ensureWork call with retry.RetryOnConflict so that the binding_sync_work_duration_seconds metric only records the final outcome after retries, not the intermediate conflict errors.

Could you please assign this issue to me? I'll prepare and submit the PR shortly. Thanks for the guidance

[jabellard]

Hi @jabellard,

Yes, I'd be happy to send PRs for this!

I'll implement the fix for both:

• pkg/controllers/binding/binding_controller.go
• pkg/controllers/binding/cluster_resource_binding_controller.go

Following your suggestion, I'll wrap the ensureWork call with retry.RetryOnConflict so that the binding_sync_work_duration_seconds metric only records the final outcome after retries, not the intermediate conflict errors.

Could you please assign this issue to me? I'll prepare and submit the PR shortly. Thanks for the guidance

Awesome! Assigned to you.