feat: downgrade uncaught error log when reconciler handles the error#3494
Open
csviri wants to merge 6 commits into
Open
feat: downgrade uncaught error log when reconciler handles the error#3494csviri wants to merge 6 commits into
csviri wants to merge 6 commits into
Conversation
csviri
force-pushed
the
loggin-improve-warn-retry
branch
2 times, most recently
from
00e465d to
35504ad
Compare
Contributor
There was a problem hiding this comment.
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
errorHandledByReconcilerflag toPostExecutionControland propagate it fromReconciliationDispatcher. - Adjust
ReconciliationDispatcherto return an exceptionPostExecutionControl(marked handled) instead of rethrowing in the “handled-but-still-retry” scenario. - Downgrade relevant
EventProcessorerror logs toDEBUGwhen 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. |
csviri
force-pushed
the
loggin-improve-warn-retry
branch
from
591f99c to
fc4be6c
Compare
csviri
force-pushed
the
loggin-improve-warn-retry
branch
from
28bf1d6 to
d6004cf
Compare
Comment on lines
+432
to
+436
| } else if (errorHandledByReconciler && !retry.isLastAttempt()) { | ||
| // The reconciler already handled the error in updateErrorStatus (and had the chance to log it | ||
| // as needed), so while there are retries remaining the framework only logs it on debug level. | ||
| // On the last attempt we still fall through to the higher-severity logging below, so a final, | ||
| // non-recoverable failure is not hidden from operators. |
### 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 (`Options` → `Mode`) 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`. Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
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>
Previously errorHandledByReconciler downgraded the error log to DEBUG unconditionally, which also hid final, non-recoverable failures on the last retry attempt. Now the downgrade only applies while retry attempts remain (errorHandledByReconciler && !retry.isLastAttempt()). On the last attempt the existing higher-severity WARN/ERROR logging is preserved. Likewise, when no retry is configured every failure is final, so it keeps the ERROR log regardless of whether the reconciler handled the error. Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
The error-handling docs claimed native retry is still performed for any non-default ErrorStatusUpdateControl, which is inaccurate for controls that disable retry (withNoRetry() / rescheduleAfter()). Qualify the paragraph so it only claims retry/backoff is preserved when a retry is configured and not disabled by the returned control. Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
retryAwareErrorLogging checked errorHandledByReconciler before the HTTP_CONFLICT special-case, so handled errors that were also KubernetesClientException conflicts no longer emitted the conflict-specific INFO message. The conflict branch is already low-noise (DEBUG + INFO) and provides actionable info, so it now keeps precedence over the handled-error downgrade.
csviri
force-pushed
the
loggin-improve-warn-retry
branch
from
15fa00d to
8329130
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a
Reconcilerthrows an exception that it deliberately handles inupdateErrorStatusand classifies as expected & retryable (e.g. "waiting for a dependency that isn't present yet"), there was no way to keep JOSDK'snative retry (exponential backoff via
@GradualRetry) while suppressing or downgrading theEventProcessor"Uncaught error during event processing" WARN. Retry and that log were the same code path.In
ReconciliationDispatcher.handleErrorStatusHandler, theupdateErrorStatusresult only avoided re-throwing whenisNoRetry()was set; any retry-enabled control ended withthrow e, which becameexceptionDuringExecutionand triggeredEventProcessor.handleRetryOnException— the single method that both schedules the retry and emits the WARN viaretryAwareErrorLogging.The only built-in escape was
withNoRetry()(also implied byrescheduleAfter()), which suppresses the log but disables native retry: you lose the@GradualRetryexponential backoff and the retry counter. With@GradualRetry(maxAttempts = -1)this was especially bad:isLastAttempt()is never true, so every attempt hit the WARN branch — a continuous stream of WARN + stack trace for an entirely expected condition, on top ofduplicate logging since the exception was already logged inside
updateErrorStatus.Change
When
updateErrorStatusreturns anyErrorStatusUpdateControlother thanErrorStatusUpdateControl.defaultErrorProcessing(), the error is now considered handled by the reconciler. Native retry (including@GradualRetrybackoff and the retry counter) is kept fully intact, but while retry attempts remain the framework logs the error on DEBUG level instead of emitting the "Uncaught error during event processing" WARN.Crucially, this only downgrades transient, still-retrying failures. On the last retry attempt — or when no retry is configured — the failure is final, so the framework preserves its existing higher-severity WARN/ERROR
logging. This avoids hiding a non-recoverable error from operators once retries are exhausted.
Returning
ErrorStatusUpdateControl.defaultErrorProcessing()preserves the previous behavior in all cases, including the warning. This follows the precedent already in this area, where the status-patch failure path andretryAwareErrorLoggingalready do selective, level-aware downgrading.Behavior matrix
updateErrorStatusreturnsdefaultErrorProcessing()patchStatus()/noStatusUpdate()(retry enabled)withNoRetry()/rescheduleAfter()Implementation
PostExecutionControlcarries a newerrorHandledByReconcilerflag.ReconciliationDispatchermarks the exception control as handled instead of rethrowing when a non-default control still wants retry.EventProcessor.retryAwareErrorLoggingdowngrades to DEBUG only whenerrorHandledByReconciler && !retry.isLastAttempt(); otherwise it falls through to the existing WARN/ERROR logic.EventProcessor.logErrorIfNoRetryConfiguredkeeps logging at ERROR regardless of the flag, since without retry every failure is final.Notes for reviewers
This makes the downgrade implicit for any non-default control, rather than an explicit opt-in (e.g.
withoutDefaultErrorLogging()). It's the simplest approach and matches the proposal in the issue, but it does change thelog level for existing users who return
patchStatus()with retry enabled (while retries remain). If we'd prefer an explicit opt-in method onErrorStatusUpdateControl, that's a small addition on top of this.Docs
Updated
error-handling-retries.mdto describe the new behavior, including the last-attempt / no-retry exception.