OWLS-89106 - Potential fix for pod startup issue in GBU CNE environment after node drain/repave operation #2398

ankedia · 2021-06-08T15:16:16Z

OWLS-89106 - In GBU CNE environment with a large k8s cluster, some pods are not started after a node drain operation. The event notifications are not generated for pod delete/start and fiber gets stuck in a suspended state due to this. This change will interrupt the fiber in the WaitForPodReady step after a timeout interval (2 min) and execute a make-right domain operation.

The WaitForJobReady step logic is not changed because the introspector job for the JRF domain is not fully idempotent. We also don't store the state of job pods in DomainPresenceInfo. Hence it doesn't need to be refreshed with an explicit relisting in case of a missed watch event.

This PR also contains a change to ItParameterizedDomain.testMultiClustersRollingRestart as that test performs extra validation using domain events. With this change, the DomainProcessingCompleted event may not always be generated in time during the test validation. We cover domain events testing in ItKubernetesEvents and this extra validation is not necessary.

The integration test run is clean with current set of changes - https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/5310/

…epave operation.

doxiao · 2021-06-08T18:38:34Z

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

+
+ @Override
+ public NextAction onSuccess(Packet packet, CallResponse callResponse) {
+ MakeRightDomainOperation makeRightDomainOperation =


I am not sure if it is always correct for all cases to use the existing MakeRightDomainOperation object. For example, if the original object has an EventData, we may not know if the event has been generated already or not in the previous attempt.

Thanks, Dongbo. That's a good point. In the latest integration test run, I'm seeing some new failures in ItKubernetesEvent and ItPodsRestart related to event generation. It looks like these failures are intermittent and timing-dependent. I'm not sure if I can create a new MakeRightDomainOperation object in this step. I'll continue looking into this tomorrow.

@doxiao, I'm not sure but I wouldn't be surprised if there are some bug in our flow. The intended design is that the operator could die at any code point and have a new operator start and have to recover. Therefore, it's a bug if any of our code paths depend on in-memory state. Since updating the Domain status and generated Events is not transactional then there may be some gaps that are impossible to close, but we should for instance be able to start a rolling, have the operator die, be restarted, and then have the new operator complete the roll and properly generate the rolling completed event. I see these timeouts as being analogous. If a wait timesout and the make-right loop goes back to the "top" then it ought to complete similarly.

Ryan, I agree with you that the operator's processing flow should not depend on in-memory states. We could review the flow to make sure that is the case. But the issue here is a little different, I think - we are reusing a DomainMakeRightOperation object to do a new execution. I don't think the operator does this prior to this PR. EventData is just one piece of the data in the MakeRightDomainOperation object. We are probably better off creating a new MakeRightDomainOperation; the new code here already overrides some of the data in the existing object prior to calling execute().

I'm not quite sure what you are proposing. What Anil and I originally discussed was to, on timeout, cancel the current fiber and start a new fiber for a new make-right operation. He found that he could instead take the current Fiber back to the top step using this pattern. That sounds reasonable. Is there just some other state in the current make right operation and/or Fiber that he needs to clear.

My suggestion is to create a new MakeRightDomainOperation passing in the new DomainPresenceInfo, instead of reusing the current make right domain operation. This suggestion does not change the logic/spirit of the current changes. I discussed with Anil offline, and found out that apparently he had problem in creating a new make right domain operation instance within PodWatcher's static context. We could add a static method somewhere for this. For example, we could refactor the Main class a little and add a static factory method there for PodWatcher to use to create a new make right domain operation instance. What I like about using a new instance is that the code here would have full control of what it needs to do. Alternatively, we could have a clear() method on MakeRightDomainOperation to start clean if we know for sure there is no other fiber running using this make right domain operation instance. Then we have to keep the clear() method up-to-date when we add new variables into it in the future. I personally prefers the first option.

I have implemented the clear() method on MakeRightDomainOperation to reset its state before executing the new make-right operation in MakeRightDomainStep. Thanks.

rjeberhard · 2021-06-09T14:28:41Z

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

+ getWatchBackstopRecheckDelaySeconds(), TimeUnit.SECONDS);
+ } else {
+ // Watch backstop recheck count is more than configured recheck count, proceed to make-right step.
+ return doNext(new CallBuilder().readDomainAsync(info.getDomainUid(),


Here you reread the domain, but don't we also need to reread the pod and/or other services?

I was following the approach in handleModifiedDomain where we call the make-right domain with just the domain object received in the watch event.

private void handleModifiedDomain(Domain domain) { LOGGER.fine(MessageKeys.WATCH_DOMAIN, domain.getDomainUid()); createMakeRightOperation(new DomainPresenceInfo(domain)) .interrupt() .withEventData(EventItem.DOMAIN_CHANGED, null) .execute(); }

…oid the intermittent test failures.

…on detection.

doxiao · 2021-06-11T13:09:44Z

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

+ MakeRightDomainOperation makeRightDomainOperation =
+ (MakeRightDomainOperation)packet.get(MAKE_RIGHT_DOMAIN_OPERATION);
+ makeRightDomainOperation.setLiveInfo(new DomainPresenceInfo((Domain)callResponse.getResult()));
+ callback.fiber.terminate(null, packet);


we probably need to call removeCallback here.

I made the change to remove callback here.

doxiao · 2021-06-11T13:21:25Z

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

 .ifPresent(i -> i.setServerPodFromEvent(getPodLabel(pod, LabelConstants.SERVERNAME_LABEL), pod));
 }
- if (isReady(callResponse.getResult())) {
+ if (isReady(callResponse.getResult()) || callback.didResume.get()) {


I don't understand why we need to check callback.didResume here. How would the operator get here again if the callback has been resumed already?

The check callback.didResume is to avoid the race condition. We do the periodic listing of the introspector job to check if it's ready and also dispatch the callback when the watch event is received. In this scenario, the watch event notifications are flowing. After the intro job completed watch notification is received, the callback is removed and the fiber is resumed. After this, the intro job gets deleted but the child fiber that's listing the job periodically to check for readiness doesn't see the job as ready since it got deleted. Hence the child fiber never finishes and times out. The above check will terminate the child fiber after intro job has been deleted.

If I understand it correctly, check for didResume (didResumeFiber now) is only needed for the job case. If so, we need to remove the check in the pod code path,

The didResumeFiber will return true only when the fiber is resumed and the resource should be ready at that time. I think there's no harm in keeping the check as that should avoid any potential race condition in pod case as well.

…make-right domain.

ankedia · 2021-06-14T14:28:11Z

The integration tests are passing with current set of changes - https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/5310/

…sing the one in the packet.

…ment a clear method instead.

doxiao · 2021-06-15T15:15:51Z

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

+ }
+ }
+
+ if (isReady(callResponse.getResult()) || callback.didResumeFiber()) {


Do we need to reset the recheckCount here?

I moved the recheckCount variable from DomainPresenceInfo to the Callback object since we create a separate callback instance for each resource. The WaitForReady step registers a new Callback instance in the resumeWhenReady method and the previous callback will be removed once the fiber is resumed. With this approach, there's no need to reset the recheckCount since previous callback instance will be garbage collected once the resource is ready.

doxiao

Looks good to me.

We need to add unit test cases to cover the timeout code path.

…ht domain step.

ankedia · 2021-06-16T13:15:02Z

Looks good to me.

We need to add unit test cases to cover the timeout code path.

Added new unit test whenPodCreatedAndNotReadyAfterTimeout_executeMakeRightDomain in PodWatcherTest to cover the timeout code path in 1c3b6a0. The integration test run is clean after this change - https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/5349/

doxiao · 2021-06-16T13:26:22Z

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

+ (callback, info, next) -> createMakeDomainRightStep(callback, info, next);
+
+ protected static Step createMakeDomainRightStep(WaitForReadyStep.Callback callback,
+ DomainPresenceInfo info, Step next) {


The "next" parameter is not used.

doxiao

LGTM

Potential fix for pod startup issue in GBU CNE env after node drain/r…

b33a4a3

…epave operation.

ankedia requested review from rjeberhard and doxiao June 8, 2021 15:16

doxiao reviewed Jun 8, 2021

View reviewed changes

rjeberhard reviewed Jun 9, 2021

View reviewed changes

ankedia added 6 commits June 9, 2021 15:44

Increase the pod ready wait timeout to 5 min (instead of 2 min) to av…

9c83cb0

…oid the intermittent test failures.

Undo refactoring related changes.

4d6749c

Fix to not increment count for introspector job.

291d6d2

Potential fix for the race condition in the introspector job completi…

6d106c7

…on detection.

Reduce timeout interval to 2 min.

3fb128f

Add previously removed event validation.

70eab49

doxiao reviewed Jun 11, 2021

View reviewed changes

ankedia added 6 commits June 12, 2021 02:34

Changes to check if the cached pod is not found on read then execute …

4dbbe1c

…make-right domain.

Fix checkstyle error.

240130d

Minor refactoring and increase the timeout value.

b035257

Fix comments and minor change.

e988050

Minor changes.

b009898

Fix checkstyle error.

9a913de

ankedia marked this pull request as ready for review June 14, 2021 14:26

ankedia added 7 commits June 14, 2021 15:53

PR review comments - create new MakeRightDomainOperation instead of u…

2093c8f

…sing the one in the packet.

Revert previous change to create a new make-right operation and imple…

b3f2362

…ment a clear method instead.

Refactoring changes.

b471058

Check for null call result.

7b2bc9f

More refactoring changes.

5a040cc

Added debug message.

b974ff8

Minor changes.

0616eb6

doxiao reviewed Jun 15, 2021

View reviewed changes

ankedia changed the title ~~Potential fix for pod startup issue in GBU CNE environment after node drain/repave operation~~ OWLS-89106 - Potential fix for pod startup issue in GBU CNE environment after node drain/repave operation Jun 15, 2021

ankedia added 3 commits June 16, 2021 03:27

added unit test to test that wait for ready timeout executes make rig…

1c3b6a0

…ht domain step.

Minor change.

a61ddea

Remove unused variable and potential fix for integration test failure.

23b60ae

doxiao reviewed Jun 16, 2021

View reviewed changes

doxiao approved these changes Jun 16, 2021

View reviewed changes

rjeberhard approved these changes Jun 16, 2021

View reviewed changes

rjeberhard merged commit a611541 into main Jun 16, 2021

ankedia deleted the gbu_node_drain_startup_fix branch September 22, 2021 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OWLS-89106 - Potential fix for pod startup issue in GBU CNE environment after node drain/repave operation #2398

OWLS-89106 - Potential fix for pod startup issue in GBU CNE environment after node drain/repave operation #2398

ankedia commented Jun 8, 2021 •

edited

Loading

doxiao Jun 8, 2021

ankedia Jun 9, 2021

rjeberhard Jun 9, 2021

doxiao Jun 9, 2021

rjeberhard Jun 9, 2021

doxiao Jun 9, 2021 •

edited

Loading

ankedia Jun 15, 2021

rjeberhard Jun 9, 2021

ankedia Jun 9, 2021

doxiao Jun 11, 2021

ankedia Jun 14, 2021

doxiao Jun 11, 2021

ankedia Jun 11, 2021

doxiao Jun 15, 2021

ankedia Jun 15, 2021

ankedia commented Jun 14, 2021

doxiao Jun 15, 2021

ankedia Jun 15, 2021

doxiao left a comment

ankedia commented Jun 16, 2021

doxiao Jun 16, 2021

doxiao left a comment

OWLS-89106 - Potential fix for pod startup issue in GBU CNE environment after node drain/repave operation #2398

OWLS-89106 - Potential fix for pod startup issue in GBU CNE environment after node drain/repave operation #2398

Conversation

ankedia commented Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doxiao Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankedia commented Jun 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doxiao left a comment

Choose a reason for hiding this comment

ankedia commented Jun 16, 2021

Choose a reason for hiding this comment

doxiao left a comment

Choose a reason for hiding this comment

ankedia commented Jun 8, 2021 •

edited

Loading

doxiao Jun 9, 2021 •

edited

Loading