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

Changes k8s watcher to use informers. #11161

Merged
merged 1 commit into from
Feb 11, 2020
Merged

Conversation

tlm
Copy link
Member

@tlm tlm commented Jan 30, 2020

Checklist

  • Checked if it requires a pylibjuju change?
  • Added integration tests for the PR?
  • Added or updated doc.go related to packages changed?
  • Do comments answer the question of why design decisions were made?

Description of change

Moves k8swatcher to use shared informers over raw k8s watchers. Benefits here are listing, resuming and connection handling. Also will use a shared cache for fetching objects instead of putting extra load on the API server.

QA steps

Bootstrap juju controller to microk8s. The following steps will then kill the kube-apiserver to mimic connection loss.

sudo kill $(pgrep kube-apiserver); microk8s.start

Check the logs to make sure we are not seeing any error messages with k8s event watcher closed, restarting

Next step is to manually destroy some workload pods and test the status is still being reported correctly in juju.

Bug reference

https://bugs.launchpad.net/juju/+bug/1859894

caas/kubernetes/provider/k8s.go Show resolved Hide resolved
caas/kubernetes/provider/k8swatcher.go Show resolved Hide resolved
caas/kubernetes/provider/k8swatcher.go Outdated Show resolved Hide resolved
caas/kubernetes/provider/namespaces.go Show resolved Hide resolved
caas/kubernetes/provider/bootstrap_test.go Outdated Show resolved Hide resolved
@tlm tlm force-pushed the watcher-disconnect branch 2 times, most recently from c43ba91 to 08ebe82 Compare January 30, 2020 03:17
Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

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

Looking nice

caas/kubernetes/provider/k8swatcher.go Show resolved Hide resolved
caas/kubernetes/provider/k8swatcher.go Show resolved Hide resolved
caas/kubernetes/provider/k8swatcher.go Show resolved Hide resolved
@tlm tlm force-pushed the watcher-disconnect branch 2 times, most recently from a2bb3ed to 4c76897 Compare February 3, 2020 07:03
Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

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

a couple more comments to look at while i continue to review

caas/kubernetes/provider/base_test.go Outdated Show resolved Hide resolved
caas/kubernetes/provider/base_test.go Outdated Show resolved Hide resolved
caas/kubernetes/provider/k8swatcher_test.go Outdated Show resolved Hide resolved
Copy link
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

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

LGTM, minor changes.

caas/kubernetes/provider/bootstrap_test.go Outdated Show resolved Hide resolved
caas/kubernetes/provider/k8swatcher.go Outdated Show resolved Hide resolved
caas/kubernetes/provider/k8swatcher.go Outdated Show resolved Hide resolved
@tlm tlm self-assigned this Feb 5, 2020
@tlm tlm force-pushed the watcher-disconnect branch 2 times, most recently from 6e54235 to f670240 Compare February 5, 2020 05:42
- This changes introduces the more robust shared index informer for
  juju. Underneath watchers are still used but with a higher safer level
  of abstraction
@tlm
Copy link
Member Author

tlm commented Feb 6, 2020

@tlm
Copy link
Member Author

tlm commented Feb 11, 2020

$$merge$$

@jujubot jujubot merged commit 472fb56 into juju:2.7 Feb 11, 2020
@tlm tlm mentioned this pull request Feb 11, 2020
jujubot added a commit that referenced this pull request Feb 11, 2020
#11203

# Description of change

Merge from 2.7 to bring forward:

#11161 from tlm/watcher-disconnect
#11202 from babbageclunk/2.7-dummy-provider-test-fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants