Skip to content

feat: downgrade uncaught error log when reconciler handles the error#3494

Open
csviri wants to merge 2 commits into
mainfrom
loggin-improve-warn-retry
Open

feat: downgrade uncaught error log when reconciler handles the error#3494
csviri wants to merge 2 commits into
mainfrom
loggin-improve-warn-retry

Conversation

@csviri

@csviri csviri commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

This reworks ResourceOperations into the primary, idiomatic API for create/update/patch operations that keep the informer cache read-after-write consistent and control how the resulting own event is handled. Every operation comes in a
default flavor and an Options-taking flavor, so users can tune caching vs. own-event filtering per call.

Own-event filtering is only correct when the framework can distinguish an own write from a concurrent third-party write. This PR makes that requirement explicit through a small strategy model (OptionsMode) and a pluggable Matcher
framework with sensible per-operation defaults.

Options / Mode

  • matchAndFilter(matcher) / matchAndFilterWithDefaultMatcher(updateType) — compare desired vs. actual (cached) state; skip the write entirely if they already match, otherwise write and filter the own event. This is the default for the
    update/patch methods and the most efficient option (filters and avoids a needless API call).
  • filterWithOptimisticLocking() — filter the own event; the write must use optimistic locking (a resourceVersion set), otherwise an IllegalArgumentException is thrown. Guarantees a concurrent change is rejected server-side rather than
    silently filtered out.
  • cacheOnly() — only cache the response (read-after-write consistency), no own-event filtering.
  • forceFilterEvents() — always filter (mostly internal; safe only when correctness is otherwise guaranteed).

Matcher framework

  • New Matcher interface plus default matchers for every UpdateType (UPDATE, SSA, JSON_PATCH, JSON_MERGE_PATCH, and their *_STATUS variants), backed by MatcherUtils.
  • GenericKubernetesResourceMatcher.matchStatus(...) added to match only the /status subtree (tolerating server-added fields by default).

Framework integration

  • Finalizer handling (ReconciliationDispatcher): the finalizer is now added with cacheOnly (its own event is no longer filtered), replacing the previous filter + INSTANT_RESCHEDULE so the finalizer-add event itself drives the follow-up
    reconcile.
  • Primary UpdateControl writes now use cacheOnly, trading one extra self-triggered reconcile for correctness (a concurrent spec change during a status patch can no longer be absorbed by the own-event filter).
  • KubernetesDependentResource create/SSA now use forceFilterEvents().
  • AbstractExternalDependentResource persists explicit state through resourceOperations().create(...).

Tests & docs

  • New ITs covering the concurrency and read-after-write edge cases: ResourceOperationsIT, SecondaryResourceOperationsIT, SpecChangeDuringStatusPatchIT, OwnSsaStatusUpdateIT, plus expanded ResourceOperationsTest, PatchMatchersTest,
    StatusMatchersTest.
  • Adjusted existing tests to the new event cadence (e.g. SubResourceUpdateIT, WorkflowMultipleActivationIT now snapshot reconcile counts only after they stabilize).
  • Docs updated: reconciler.md, v5-5-migration.md, v5-5-release.md, and the read-after-write blog post.

Notes / follow-ups

  • The Options API is marked @Experimental (API may change).
  • Default matchers are heuristics — reconcilers relying on them should be tested against their concrete resources, or supply a custom Matcher.

Copilot AI review requested due to automatic review settings July 14, 2026 14:27
@openshift-ci openshift-ci Bot requested review from metacosm and xstefank July 14, 2026 14:27
@csviri csviri force-pushed the loggin-improve-warn-retry branch 2 times, most recently from 00e465d to 35504ad Compare July 14, 2026 14:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR changes error handling so that when updateErrorStatus returns a non-default ErrorStatusUpdateControl, the framework treats the error as “handled by the reconciler” and downgrades framework-level logging while keeping retry/backoff behavior intact.

Changes:

  • Add an errorHandledByReconciler flag to PostExecutionControl and propagate it from ReconciliationDispatcher.
  • Adjust ReconciliationDispatcher to return an exception PostExecutionControl (marked handled) instead of rethrowing in the “handled-but-still-retry” scenario.
  • Downgrade relevant EventProcessor error logs to DEBUG when the handled flag is set, and add tests + docs for the new behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java Adds test coverage for the new “handled by reconciler” flag behavior.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java Marks exception as handled (via PostExecutionControl) rather than rethrowing in the handled+retry path.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java Introduces errorHandledByReconciler flag with setter/getter to carry handling state downstream.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java Threads handled flag into retry + no-retry log paths and downgrades framework logs to DEBUG accordingly.
docs/content/en/docs/documentation/error-handling-retries.md Documents the new semantics and logging behavior when updateErrorStatus returns non-default control.

Comment on lines +425 to 434
if (errorHandledByReconciler) {
// The reconciler already handled the error in updateErrorStatus (and had the chance to log it
// as needed), so the framework only logs it on debug level while still retrying.
log.debug(
"Error during event processing {}, but was handled by the reconciler",
executionScope,
exception);
} else if (!retry.isLastAttempt()
&& exception instanceof KubernetesClientException ex
&& ex.getCode() == HttpURLConnection.HTTP_CONFLICT) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This we should probably not do (what copilot means). WDYT @xstefank @benkeil ?

csviri added 2 commits July 15, 2026 08:45
When a reconciler's updateErrorStatus returns any ErrorStatusUpdateControl
other than defaultErrorProcessing(), the error is now considered handled by
the reconciler. Native retry (including @GradualRetry exponential backoff)
is kept intact, but the framework logs the error on DEBUG level instead of
emitting the "Uncaught error during event processing" WARN.

This lets a reconciler keep retrying an expected, recoverable condition
without producing a continuous stream of WARN messages, while it remains
free to log the error at whatever level it wants inside updateErrorStatus.
Returning defaultErrorProcessing() preserves the previous behavior,
including the warning.

- PostExecutionControl carries an errorHandledByReconciler flag
- ReconciliationDispatcher marks the exception control as handled instead
  of rethrowing when a non-default control still wants retry
- EventProcessor downgrades the retry-aware and no-retry-configured error
  logs to DEBUG when the error was handled by the reconciler

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
@csviri csviri force-pushed the loggin-improve-warn-retry branch from 591f99c to fc4be6c Compare July 15, 2026 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow suppressing/downgrading the "Uncaught error during event processing" log without disabling retry (for exceptions handled in updateErrorStatus)

2 participants