Refactor using single SharedInformer #244
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
|
/hold |
k8s-ci-robot
added
do-not-merge/hold
Update Update
davidvonthenen
force-pushed
the
feature/sharedinformer
branch
from
8b409c5 to
33cfc0f
Compare
|
/hold cancel This is ready for review... |
k8s-ci-robot
removed
the
do-not-merge/hold
| var signalHandler <-chan struct{} | ||
| var ( | ||
| signalHandler <-chan struct{} | ||
| informerFactory informers.SharedInformerFactory |
There was a problem hiding this comment.
Not a fan of global var for the informer factory, is there a way to build the informer factory in Initialize() and just pass down only the needed informers down to connection manager (i.e. informerFactory.Core().Secrets())?
There was a problem hiding this comment.
hmmm okay, I see now that would require a larger refactor on the InformerManager
There was a problem hiding this comment.
I think it's ok as a global. The global informerFactory isn't exported out of the package/process so it can't be re-used by other processes. Since this global is confined to this package, you are getting exactly what you want... a single informer factory.
There was a problem hiding this comment.
If you really do want to pass in the variable, it would require a second function for NewInformer(WithProvidedFactory) and also a second function to InitializeSecretLister(WithProvidedFactory), but it would still accomplish effectively the same thing with a private global.
There was a problem hiding this comment.
So I think my biggest concern with this is that you would call informerFactory.Start() multiple times for multiple InformerManagers using the same shared informer factory. Ideally you construct the informer factory in one place and pass that into various types that need resource-specific informers and you would call informerFactory.Start() once, ideally in the same place you constructed it. With that said, I think the real problem isn't the global var but the InformerManager type which makes it hard to easily pass down the informer factory but probably don't want to address that in this PR.
There was a problem hiding this comment.
Yea, I looked at the code behind Start() and also tested to make sure the behavior is correct. That's why I made the update to the function comment. You can call Start() multiple times as you add in new Listers and it only inits the new ones.
The downside of getting all of the Listers upfront, is it complicates the CPI init logic a lot.
|
|
||
| // NewInformer creates a newk8s client based on a service account | ||
| func NewInformer(client clientset.Interface) *InformerManager { | ||
| func NewInformer(client clientset.Interface, singleWatcher bool) *InformerManager { |
There was a problem hiding this comment.
Do we need the singleWatcher toggle? In what scenario would we not want a single informer for all lister/watchers?
There was a problem hiding this comment.
Yea, we don't have a need for it currently. Was just trying to preserve the option. Only reason for it would be having multiple client connections based on individual service accounts.
If you want to remove it and tackle it when we get there, that cool. Just let me know.
| var signalHandler <-chan struct{} | ||
| var ( | ||
| signalHandler <-chan struct{} | ||
| informerFactory informers.SharedInformerFactory |
There was a problem hiding this comment.
So I think my biggest concern with this is that you would call informerFactory.Start() multiple times for multiple InformerManagers using the same shared informer factory. Ideally you construct the informer factory in one place and pass that into various types that need resource-specific informers and you would call informerFactory.Start() once, ideally in the same place you constructed it. With that said, I think the real problem isn't the global var but the InformerManager type which makes it hard to easily pass down the informer factory but probably don't want to address that in this PR.
|
/approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
This was some fallout from creating listers for multiple secrets for VC creds. This refactors NewInformer so that only a single SharedInformer is created. In English, we only have a single watcher for all secrets.
Which issue this PR fixes : fixes #243
Special notes for your reviewer:
Tested on k8s 1.14.1 on vsphere 6.7u2 using multiple secrets.
Release note:
NA