fix: reducing log level of failure to patch in handleErrorStatusHandler#2983
fix: reducing log level of failure to patch in handleErrorStatusHandler#2983csviri merged 1 commit intooperator-framework:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR reduces log verbosity for status patching failures in the reconciliation error handler by changing the log level from ERROR to DEBUG in specific scenarios where the failure is expected or temporary.
- Updates log level logic to use DEBUG instead of ERROR when next reconciliation is imminent or when encountering HTTP 409 conflicts
- Adds conditional logging based on reconciliation timing and HTTP status codes
- Imports additional classes to support the new logging logic
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Level level = Level.ERROR; | ||
| if (context.isNextReconciliationImminent() | ||
| || ((ex instanceof KubernetesClientException kcex | ||
| && kcex.getCode() == HttpURLConnection.HTTP_CONFLICT))) { | ||
| level = Level.DEBUG; // we'll be reconciling again soon, so don't over log | ||
| } | ||
| log.atLevel(level) | ||
| .log( |
There was a problem hiding this comment.
[nitpick] The nested instanceof check with pattern matching creates a complex boolean expression. Consider extracting the HTTP conflict check into a separate method or variable for better readability: boolean isHttpConflict = ex instanceof KubernetesClientException kcex && kcex.getCode() == HttpURLConnection.HTTP_CONFLICT;
| Level level = Level.ERROR; | ||
| if (context.isNextReconciliationImminent() | ||
| || ((ex instanceof KubernetesClientException kcex | ||
| && kcex.getCode() == HttpURLConnection.HTTP_CONFLICT))) { |
There was a problem hiding this comment.
[nitpick] Using the raw HTTP status code constant HttpURLConnection.HTTP_CONFLICT (409) may not be the most appropriate choice for Kubernetes API interactions. Consider using a Kubernetes-specific constant if available in the fabric8 client library, as HTTP 409 in Kubernetes context typically indicates resource version conflicts.
| && kcex.getCode() == HttpURLConnection.HTTP_CONFLICT))) { | |
| && kcex.getCode() == KubernetesClientException.CONFLICT))) { |
|
Thank you for the PR.
log.info("Conflict while updating custom resource, but next reconiliation is imminent").Would this help in your case?
private static final Logger conflictLogger = LoggerFactory.getLogger(ReconciliationDispatcher.class.getCanonicalName()+".conflict");
// omittes code
// modified log in this PR:
conflictLogger.info(
"updateErrorStatus failed for resource: {} with version: {} for error {}",
getUID(resource),
getVersion(resource),
e.getMessage(),
ex);Now you can change the log level of this logger to "warn" in your config you won't see these log messages, however we will be backwards compatible. (but maybe this approach is overkill) Please let me know what do you think about these. |
|
closes: operator-framework#2981 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
|
See if this looks right. There will be three different logging behaviors, which is very similar to the other processing. If it looks like this will be retried and is based upon a conflict, then it's a INFO message and DEBUG log of the exception. |
| failedMessage = " due to conflict"; | ||
| log.info( | ||
| "ErrorStatusUpdateControl.patchStatus of {} failed due to a conflict, but the next" | ||
| + " reconiliation is imminent.", |
There was a problem hiding this comment.
| + " reconiliation is imminent.", | |
| + " reconciliation is imminent.", |
closes: #2981