Stop all informers is calling start watcher #3029

akram · 2021-04-21T13:48:49Z

Description

The onClose method reference was pointing to startWatcher method which was causing a new watcher to be started when the stopAllRegisteredInformers was called. The startWacher was containing a sleep of 5 seconds causing the stopAllRegisteredInformers to last that long.
This PR changes this method to stopWatcher which does only close the watcher.
Fixes #3024

It is may be required to update the signature of the class by adding an onStart method but, that's beyond the scope of this PR.

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

centos-ci · 2021-04-21T13:48:51Z

Can one of the admins verify this patch?

shawkins · 2021-04-26T20:29:56Z

@rohanKanojia @akram my understanding of where things stand now is that the initially created Watch should not be terminated unless explicitly closed. If this is correct, then stopping the watch is appropriate only on the non-exceptional onClose. You could also just reuse the existing stop method here.

I've made some related cleanups in #3048

shawkins · 2021-04-27T17:00:12Z

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/informers/cache/Reflector.java

@@ -111,6 +111,15 @@ private void reListAndSync() {
    }
  }

+  private void stopWatcher() {


Let's reuse the existing stop method instead of introducing a new method.

Then in the ReflectorWatcher.onClose(WatcherException exception) method, remove the call to onClose.run. Based upon #3018 the watch should just be left in place to keep retrying.

Thinking about it more though, after the relist you do need to restart the watch at the new latest resource version. So the changes I'm describing here a bit more pervasive. Let me apply your pr and layer some changes on top of that for clarity.

ok, so, I will update my PR to call the stop method instead.

@akram sorry, I didn't go back and update all of my comments - we don't want to call stop as written.

this incorporates the reflector changes from fabric8io#3029 It goes further in refining responsibilities by mostly relying on the watch for tracking the lastResourceVersion and removing onClose behavior that is no longer necessary due to fabric8io#3018

shawkins · 2021-04-27T21:06:23Z

@akram shawkins#1 builds upon #3048 and adds your Reflector changes (the utils changes are not there). This tries to further clean up the responsibilities in the code - as a lot of what the Reflector was assuming is already handled.

…watcher and not re-open it

manusa

LGTM, thx!

shawkins · 2021-04-28T13:25:46Z

@manusa @akram based upon what my understanding with with working shawkins#1 - the most consistent solution here is to only have the current watcher perform a list, sync, and watch on an exceptional onClose.

No action is needed from the Watcher non-exceptional onClose method - that will only be called when the Watch is explicitly closed, which can only be called via the the Reflector. There is no need for the Watcher to call back into the Reflector in that case.

sonarcloud · 2021-04-28T13:38:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

60.0% Coverage
0.0% Duplication

manusa · 2021-04-28T13:49:52Z

Hi @shawkins, just to be clear, your #3048 + the additional PR supersedes this one (#3029)?

Should this one be closed in favor of yours?

(Also #3048 is marked as ready, but has your internal outstanding PR --> shawkins#1. Is it ready?)

shawkins · 2021-04-28T14:06:57Z

@manusa #3048 by itself is ready. shawkins#1 supersedes the reflector changes from here.

Can we have this pr narrowed to just the util changes?

On shawkins#1 - that may need some refinement. I'm also trying to reuse the resource version from the Watch itself - but that isn't required for just the change to the onClose behavior.

shawkins · 2021-04-28T14:19:07Z

Also I know that we've started down the path of having the Watcher indicate that it's reconnecting. FWIW in looking at the go client they do have a more formal concept of a RetryWatcher https://github.com/kubernetes/client-go/blob/f6ce18ae578c8cca64d14ab9687824d9e1305a67/tools/watch/retrywatcher.go that this logic may have been originally based upon. If preferred we can try to take things in that direction instead.

shawkins · 2021-04-28T16:58:51Z

#3055 addresses the same issue as the Reflector changes here.

manusa · 2021-04-29T08:59:29Z

#3048 was merged yesterday.

I think that the next one to be merged in the Shared Informer stack is this one. Could you please resolve the conflicts or maybe create a superseding PR which includes @akram changes + shawkins#1 (@shawkins) ---> Whatever works best

akram · 2021-05-11T09:02:44Z

@shawkins what it is the status of this? Should I rebase ? or does #3048 fit the long stop time and recreation on startup?

shawkins · 2021-05-11T11:42:12Z

@akram you should remove the reflector changes from this pr. The util changes are still applicable if you want them.

manusa · 2021-05-13T05:12:25Z

Relates to: #2010

shawkins · 2021-05-25T21:09:05Z

This pr will be completely out-of-date if #3169 is accepted.

shawkins · 2021-06-17T11:53:36Z

All of the changes in this pr have now been addressed.

akram force-pushed the stop-all-informers-was-calling-start-watcher branch from cc10a4c to d62b193 Compare April 21, 2021 13:52

akram mentioned this pull request Apr 21, 2021

Concurrency issues when using SharedInformers and closing the KubernetesClient #2939

Closed

akram changed the title ~~Stop all informers was calling start watcher~~ Stop all informers is calling start watcher Apr 22, 2021

shawkins mentioned this pull request Apr 26, 2021

refactoring informer logic for type safety and to refine the role of stores #3008

Closed

8 tasks

akram mentioned this pull request Apr 27, 2021

Enforce informer lifecycle #3050

Closed

shawkins mentioned this pull request Apr 27, 2021

Informer watch status #3048

Merged

8 tasks

shawkins reviewed Apr 27, 2021

View reviewed changes

shawkins mentioned this pull request Apr 27, 2021

Informer OnClose Cleanup shawkins/kubernetes-client#1

Closed

11 tasks

akram force-pushed the stop-all-informers-was-calling-start-watcher branch from d62b193 to 2fc4ead Compare April 28, 2021 09:03

Change the onClose method of ReflectorWatcher instance to only close …

d7ced42

…watcher and not re-open it

manusa force-pushed the stop-all-informers-was-calling-start-watcher branch from 2fc4ead to d7ced42 Compare April 28, 2021 13:09

manusa assigned manusa, oscerd and rohanKanojia and unassigned manusa, oscerd and rohanKanojia Apr 28, 2021

manusa requested review from rohanKanojia, manusa and oscerd April 28, 2021 13:10

manusa approved these changes Apr 28, 2021

View reviewed changes

shawkins mentioned this pull request Apr 28, 2021

Fix #3024 removing any onClose action #3055

Merged

11 tasks

shawkins mentioned this pull request May 11, 2021

Review if patch(context, string) should handle the rolling case #3098

Closed

oscerd approved these changes May 13, 2021

View reviewed changes

shawkins closed this Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop all informers is calling start watcher #3029

Stop all informers is calling start watcher #3029

akram commented Apr 21, 2021

centos-ci commented Apr 21, 2021

shawkins commented Apr 26, 2021

shawkins Apr 27, 2021 •

edited

Loading

akram Apr 28, 2021

shawkins Apr 28, 2021

shawkins commented Apr 27, 2021

manusa left a comment

shawkins commented Apr 28, 2021

sonarcloud bot commented Apr 28, 2021

manusa commented Apr 28, 2021

shawkins commented Apr 28, 2021

shawkins commented Apr 28, 2021

shawkins commented Apr 28, 2021

manusa commented Apr 29, 2021

akram commented May 11, 2021

shawkins commented May 11, 2021

manusa commented May 13, 2021

shawkins commented May 25, 2021

shawkins commented Jun 17, 2021

Stop all informers is calling start watcher #3029

Stop all informers is calling start watcher #3029

Conversation

akram commented Apr 21, 2021

Description

Type of change

Checklist

centos-ci commented Apr 21, 2021

shawkins commented Apr 26, 2021

shawkins Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

akram Apr 28, 2021

Choose a reason for hiding this comment

shawkins Apr 28, 2021

Choose a reason for hiding this comment

shawkins commented Apr 27, 2021

manusa left a comment

Choose a reason for hiding this comment

shawkins commented Apr 28, 2021

sonarcloud bot commented Apr 28, 2021

manusa commented Apr 28, 2021

shawkins commented Apr 28, 2021

shawkins commented Apr 28, 2021

shawkins commented Apr 28, 2021

manusa commented Apr 29, 2021

akram commented May 11, 2021

shawkins commented May 11, 2021

manusa commented May 13, 2021

shawkins commented May 25, 2021

shawkins commented Jun 17, 2021

shawkins Apr 27, 2021 •

edited

Loading