Skip to content

Conversation

shawkins
Copy link
Collaborator

@shawkins shawkins commented Oct 9, 2025

closes: #2981

@Copilot Copilot AI review requested due to automatic review settings October 9, 2025 20:36
@openshift-ci openshift-ci bot requested review from metacosm and xstefank October 9, 2025 20:36
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 221 to 228
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(
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

[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;

Copilot uses AI. Check for mistakes.

Level level = Level.ERROR;
if (context.isNextReconciliationImminent()
|| ((ex instanceof KubernetesClientException kcex
&& kcex.getCode() == HttpURLConnection.HTTP_CONFLICT))) {
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

[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.

Suggested change
&& kcex.getCode() == HttpURLConnection.HTTP_CONFLICT))) {
&& kcex.getCode() == KubernetesClientException.CONFLICT))) {

Copilot uses AI. Check for mistakes.

@csviri
Copy link
Collaborator

csviri commented Oct 10, 2025

Thank you for the PR.
I see your point and I'm not against this, just some points to discuss / consider:

  1. if sombody (most of the time) will have logger for the framework on info level won't see any log message regarding what happened. It will just retry but there will be no info about that the conflict happened whatsoever. So the reconiliation will "silently fail". Maybe for this case just rather log a short log message without stack trace that this happened? thus:
    like:
log.info("Conflict while updating custom resource, but next reconiliation is imminent").

Would this help in your case?

  1. Note that usually retry is always in place (well it is considered a bad practice if turned off), so in most the cases it will retry this error right after the conflict anyways (regardless if reconciliation is imminent (other event received meanwhile) or not).

  2. another option I see is to have a dedicated logger, this where we log now the conflic we could have a dedicated logger:

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.

@shawkins
Copy link
Collaborator Author

  1. Fair enough, I'll follow the same pattern from https://github.com/operator-framework/java-operator-sdk/pull/2748/files here. At worst you'll see two highly related info logs - but it should be understandable what is happening.

  2. Agreed.

  3. That does seem like overkill.

@shawkins
Copy link
Collaborator Author

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.
If it looks like this will be retried and is any other reason, then it's a WARN log.
Otherwise it's an ERROR log.

@csviri
Copy link
Collaborator

csviri commented Oct 10, 2025

LGTM, let's wait also for a while for @xstefank @metacosm

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.

Patch status errors on conflict logged too highly

3 participants