update controllers watching all pods to share an informer#24470
update controllers watching all pods to share an informer#24470k8s-github-robot merged 3 commits intokubernetes:masterfrom
Conversation
There was a problem hiding this comment.
@derekwaynecarr creating at point of use if one doesn't already exist. Is this factory supposed to be threadsafe, because clearly this is not.
There was a problem hiding this comment.
it is not intended to be thread-safe.
|
GCE e2e build/test passed for commit 1bf310124e5d41b602cef6110cf70942036c5140. |
There was a problem hiding this comment.
i assume the plan is to continue this for other object types?
at what point can we bite the bullet and have a informer registry tied to a particular type of resource so we can avoid passing in multiple informers in this operation?
There was a problem hiding this comment.
at what point can we bite the bullet and have a informer registry tied to a particular type of resource so we can avoid passing in multiple informers in this operation?
Probably two.
There was a problem hiding this comment.
Ok,will hold you to that :-)
On Tuesday, April 19, 2016, David Eads notifications@github.com wrote:
In pkg/controller/resourcequota/replenishment_controller.go
#24470 (comment)
:}
// NewReplenishmentControllerFactory returns a factory that knows how to build controllers
// to replenish resources when updated or deleted
-func NewReplenishmentControllerFactory(kubeClient clientset.Interface) ReplenishmentControllerFactory {
+func NewReplenishmentControllerFactory(podInformer framework.SharedInformer, kubeClient clientset.Interface) ReplenishmentControllerFactory {at what point can we bite the bullet and have a informer registry tied to
a particular type of resource so we can avoid passing in multiple informers
in this operation?Probably two.
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/24470/files/1bf310124e5d41b602cef6110cf70942036c5140#r60270619
|
This lgtm will let @wojtek-t take a pass as well. |
pkg/controller/job/controller.go
Outdated
There was a problem hiding this comment.
jm.podController = podInformer.GetController()
is missing here.
There was a problem hiding this comment.
jm.podController = podInformer.GetController()
is missing here.
There is no such field in this controller. Turns out it we don't need it for this one.
There was a problem hiding this comment.
OK - I missed that you removed it :)
|
I added some comments - once applied, this lgtm. |
pkg/controller/job/controller.go
Outdated
There was a problem hiding this comment.
s/NewReplicationManager/NewJobController
|
Some nits, but overall LGTM. |
1bf3101 to
3b93f7d
Compare
|
comments addressed. |
|
LGTM (it seems that everyone is fine with that, so applying label). |
|
GCE e2e build/test passed for commit 3b93f7d0f0316234c56e9551e7a3376838079f57. |
3b93f7d to
60fe17d
Compare
|
GCE e2e build/test passed for commit 60fe17d. |
|
e2e tests took 350s, whereas I've seen runs ~460s on average. Will be interested to see impact on load tests |
|
GCE e2e build/test passed for commit 60fe17d. |
|
GCE e2e build/test passed for commit 60fe17d. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 60fe17d. |
|
Automatic merge from submit-queue |
This plumbs the shared pod informer through the various controllers to avoid duplicated watches.