Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Reaper watchers across multiple clouds and config updates #1170

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

cronik
Copy link
Contributor

@cronik cronik commented May 3, 2022

Update Reaper to keep track of watchers across multiple clouds and add/remove
them when clouds are updated. To better track when watchers need to be
recreated the KubernetesClientProvider#getValidity method was made public.
This allows the Reaper to validate the watch client connection is still valid if
the cloud connection properties are updated.

If a watcher is closed due to "410 Gone" responses the watcher will deregister
itself and reregister next time a node comes online.

Watcher implementations will now filter out Bookmark and Error action events
which don't represent Pod state changes. Future enhancement might be to use
bookmarks on reconnect to catchup on events.

Deprecated KubernetesLauncher watcher field which does not appear to be
used by the plugin anywhere and is a potential for threading issues when lots of
agents are launched at once.

Changes to KubernetesLauncher may conflict with #1167
Changes to Reaper may conflict with #1083

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@Vlatombe Vlatombe added the enhancement Improvements label May 30, 2022
@Vlatombe Vlatombe requested review from Vlatombe and jglick June 1, 2022 15:48
Copy link
Member

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

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

I don't get everything because of the PR size, but all tests are passing and the code looks fine so LGTM

@cronik
Copy link
Contributor Author

cronik commented Jun 1, 2022

Thanks for reviewing! The only thing I was unsure about and was hoping to get a little guidance on was the deprecation of the watcher field. https://github.com/jenkinsci/kubernetes-plugin/pull/1170/files#diff-8e43be7dacff0fa974a7614fda2f4103bf4b820be52d747d9498e6f7efbd688fL66

Since that is no longer reference anywhere in the class, should I just remove the field and leave the deprecated accessor method to alway return null?

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

I would advise getting #1167 released first and resolve conflicts with that here.

Thanks for the heads-up about #1083. Looks like it will take some work to resolve the merge conflicts but I think they should be mostly superficial.

Did not try to do any sort of detailed code review here—leaving approval to @Vlatombe.

return;
}

ExtensionList.lookup(Listener.class).forEach(listener -> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ExtensionList.lookup(Listener.class).forEach(listener -> {
ExtensionList.lookup(Listener.class).forEach(listener -> { // TODO 2.324+ jenkins.util.Listeners

…dates

Update Reaper to keep track of watchers across multiple clouds and add/remove
them when clouds are updated. To better track when watchers need to be
recreated the KubernetesClientProvider#getValidity method was made public.
This allows the Reaper to validate the watch client connection is still valid if
the cloud connection properties are updated.

If a watcher is closed due to "410 Gone" responses the watcher will deregister
itself and reregister next time a node comes online.

Watcher implementations will now filter out Bookmark and Error action events
which don't represent Pod state changes. Future enhancement might be to use
bookmarks on reconnect to catchup on events.

Deprecated KubernetesLauncher `watcher` field which does not appear to be
used by the plugin anywhere and is a potential for threading issues when lots of
agents are launched at once.
@cronik cronik force-pushed the feature/reaper-watchers branch from 398486b to 9f76251 Compare June 8, 2022 21:23
@Vlatombe Vlatombe merged commit fbb4493 into jenkinsci:master Jun 10, 2022
jglick added a commit to jglick/kubernetes-plugin that referenced this pull request Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants