Merge changes for OWLS-930732 to include retry stage and fatal condition in domain status message #2622

ankedia · 2021-11-10T19:35:31Z

Merge changes for OWLS-930732 from PR 2571 for following domain conditions -

When retries exceed the max limit, add "introspect failure retry count exceeded error" to the domain status message.
In case of a fatal error, include the fatal status in the domain status message.
For non-fatal retries, indicate the current retry stage i.e. "Introspection failed on try 1 of x".

rjeberhard · 2021-11-10T23:58:40Z

operator/src/main/java/oracle/kubernetes/operator/ProcessingConstants.java

+
+  String INTROSPECTION_ERROR = "Introspection Error: ";
+
+  String FATAL_ERROR_DOMAIN_STATUS_MESSAGE = "Introspection encountered a fatal error and will not retry automatically."


Can we internationalize this by pulling from the resource bundle?

Fixed in ea644db .

doxiao · 2021-11-12T15:28:50Z

operator/src/main/java/oracle/kubernetes/operator/DomainStatusUpdater.java

+
+    private String getNonFatalRetryStatusMessage(DomainStatus domainStatus) {
+      return "Introspection failed on try " + domainStatus.getIntrospectJobFailureCount()
+              + " of " + DomainPresence.getDomainPresenceFailureRetryMaxCount() + ".";


Shall we also provide the information about when the next retry will happen?

@rjeberhard @tbarnes-us @jshum2479 - Any thoughts about this and where do we get this information from? Thanks.

Would DomainPresence.getDomainPresenceFailureRetrySeconds() work for this?

Do we need exact timing? or can we just tell the user it will retry approximately in next make right interval (2 mins or the configured time).

In some cases such as when an introspector job times out, we retry after DomainPresence.getDomainPresenceFailureRetrySeconds but in other cases we retry during next make right interval.

We will track this new requirement with a separate JIRA and address in a new pull request.

please let me know the retry JIRA# when filed

note there's an existing umbrella retry JIRA OWLS-84438 that's meant for brain-storming purposes.

IMO, it's confusing to have two different retry settings - as this makes it hard for customers to figure out which setting they need to tune

@tbarnes-us new JIRA is https://jira.oraclecorp.com/jira/browse/OWLS-93899 .

…e bundle.

russgold

Overall looks very good; I have requested some minor changes.

operator/src/main/java/oracle/kubernetes/operator/DomainStatusUpdater.java

operator/src/main/java/oracle/kubernetes/utils/OperatorUtils.java

ankedia · 2021-11-12T20:40:31Z

Overall looks very good; I have requested some minor changes.

Thanks, I have made the suggested changes in ec9d534.

doxiao · 2021-11-12T23:27:37Z

operator/src/main/java/oracle/kubernetes/operator/helpers/JobHelper.java


+        nextSteps = Step.chain(
+              createFailureRelatedSteps(Introspection, onSeparateLines(severeStatuses)),
+              getNextStep(packet, domainIntrospectorJob), nextSteps);


I believe getNextSteop() method can return null. When it is null, will we have a null step in the middle of the chain here?

My understanding was that Step.chain can handle null elements in the middle of the chain. The reason I made the above change is because we want the FailureCountStep to be executed last (i.e. after the FailedStep) to retain the correct message in domain status. If FailedStep is executed last, it overwrites the domain status message created by the FailureCount step. I didn't see any issues in the unit or the integration test with this approach. Please let me know if my understanding is incorrect, I can try to come up with some other approach.

From the Javadoc of Step.chain, the parameter stepGroups is multiple groups of steps. Any ideas if the group of steps have to be non-null?

/** * Chain the specified step groups into a single chain of steps. * * @param stepGroups multiple groups of steps * @return the first step of the resultant chain */

Look at StepChainTest.ignoreNullMiddleSteps. It actually verifies that this case works - null steps are ignored.

Thanks, Russ. We are good then.

Ideally, the FailureCount step should be after the FailedStep and before createEventStep so that the event can be populated with the domain status message. This will help to match the event message with the domain status message. If this requires too much changes, we can consider to make this change in a future PR.

Changed in ea2d103.

doxiao · 2021-11-15T14:46:44Z

operator/src/main/resources/Operator.properties

+  Please resolve the error and then update 'domain.spec.introspectVersion' to force a retry.
+WLSKO-0196=Stop introspection retry - exceeded configured domainPresenceFailureRetryMaxCount: {0}. The \
+  domainPresenceFailureRetryMaxCount is an operator tuning parameter and can be controlled by adding it to the \
+  weblogic-operator-cm configmap. To force the introspector to start retrying again, update 'domain.spec.introspectVersion.


Missing a "'" at the end.

Fixed in ea2d103 .

doxiao · 2021-11-15T14:55:25Z

operator/src/main/resources/Operator.properties

+WLSKO-0195=Introspection encountered a fatal error and will not retry automatically. \
+  Please resolve the error and then update 'domain.spec.introspectVersion' to force a retry.
+WLSKO-0196=Stop introspection retry - exceeded configured domainPresenceFailureRetryMaxCount: {0}. The \
+  domainPresenceFailureRetryMaxCount is an operator tuning parameter and can be controlled by adding it to the \


Can domainPresenceFailureRetryMaxCount also be controlled via helm upgrade command?

AFAIK, this can't be done using helm upgrade command.

doxiao

LGTM.

Merge changes from PR2571 for OWLS-930732 to main.

4b4a0e9

ankedia requested review from jshum2479, rjeberhard, russgold and tbarnes-us November 10, 2021 19:35

rjeberhard reviewed Nov 10, 2021

View reviewed changes

doxiao reviewed Nov 12, 2021

View reviewed changes

Changes to internationalize the messages by pulling them from resourc…

ea644db

…e bundle.

russgold approved these changes Nov 12, 2021

View reviewed changes

ankedia added 2 commits November 12, 2021 20:26

Changes to address PR review comments.

ec9d534

Fix checkstyple errors.

7d370a5

doxiao reviewed Nov 12, 2021

View reviewed changes

doxiao reviewed Nov 15, 2021

View reviewed changes

Address Dongbo's PR review comments.

ea2d103

doxiao approved these changes Nov 15, 2021

View reviewed changes

rjeberhard merged commit ce39d31 into main Nov 16, 2021

ankedia deleted the merge_pr2571_owls930732 branch November 16, 2021 14:49


		String INTROSPECTION_ERROR = "Introspection Error: ";

		String FATAL_ERROR_DOMAIN_STATUS_MESSAGE = "Introspection encountered a fatal error and will not retry automatically."

Merge changes for OWLS-930732 to include retry stage and fatal condition in domain status message #2622

Merge changes for OWLS-930732 to include retry stage and fatal condition in domain status message #2622

Uh oh!

Conversation

ankedia commented Nov 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

russgold left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankedia commented Nov 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doxiao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants