shared controller informers by deads2k · Pull Request #23575 · kubernetes/kubernetes

deads2k · 2016-03-29T13:18:46Z

Related to #14978

This demonstrates how controllers which use an Informer, would be able to share the same watch and store. A similar "setup and run" approach could be done for an IndexInformer to share that cache. I found adding listeners here to be easier than intercepting at the watch interface (problems with resourceVersion) or the reflector (same plumbing, but you have to fan out to multiple stores).

We could also use the cache we build here to back several of the admission plugins that currently run their own lookup caches today.

If there's interest, I can finish out the SharedInformer and switch the low hanging fruit over.

@kubernetes/rh-cluster-infra @smarterclayton @liggitt @wojtek-t

k8s-bot · 2016-03-29T13:49:32Z

GCE e2e build/test passed for commit 97be6688850149e4b09f0976b5715be9acbabeac.

ncdc · 2016-03-30T14:22:40Z

In this case:

@k8s-bot unit test this issue: #23533

k8s-bot · 2016-03-30T14:24:18Z

GCE e2e build/test passed for commit afed8824c9e93d739a1769b35b23f1e81de4dd25.

wojtek-t · 2016-03-31T18:17:00Z

@deads2k - sorry for late response - I'm in US now and have a calendar full of meetings.
I will try to look into it later today or tomorrow. Anyway - thanks for looking into it.

derekwaynecarr · 2016-03-31T18:56:04Z

pkg/controller/framework/shared_informer.go

maybe you floss really well the next morning ;-)

deads2k · 2016-04-05T19:39:31Z

@deads2k - sorry for late response - I'm in US now and have a calendar full of meetings.
I will try to look into it later today or tomorrow. Anyway - thanks for looking into it.

@wojtek-t bump

wojtek-t · 2016-04-11T08:49:45Z

@deads2k - sorry for a huge delay in reviewing this; I'm back from US now; looking into it now.

wojtek-t · 2016-04-11T08:51:41Z

pkg/controller/endpoint/endpoints_controller.go

If we want to proceed with this PR, we should avoid this code duplication. But I guess this is just POC now, so never mind :)

If we want to proceed with this PR, we should avoid this code duplication. But I guess this is just POC now, so never mind :)

My plan would be to switch over a couple controllers, prove that they work, then start phasing in the new approach and removing the old paths. Probably once we have a standard way of tracking the shared informers for a given resource.

wojtek-t · 2016-04-11T09:25:43Z

@deads2k - I added few comments to it.

After some deeper thinking about it, my biggest concern is that we may want to unregister/register a handler function when the informer is already running - see this comment:
#23575 (comment)
for more details.
I know it complicates things, but maybe it's necessary?

WDYT?

wojtek-t · 2016-04-11T09:25:59Z

@lavalamp @kubernetes/sig-api-machinery - FYI

deads2k · 2016-04-13T22:53:39Z

I know it complicates things, but maybe it's necessary?

Looking at the thread, I think we closed it off with me implementing an infinite slice feeding an unbuffered channel on a go func and that would satisfy the primary case of register/unregister. If you agree, I'll work on fixing this up tomorrow, switching over the two sample controllers I saw and building unit tests to finish up the concept.

wojtek-t · 2016-04-14T06:10:40Z

@deads2k - I would like this to be fixed in the future, but I'm fine with what you implemented short term. So yeah - your plan SGTM. Please let me know when it will be ready for review.

timothysc · 2016-04-14T12:59:09Z

/cc @rrati

wojtek-t · 2016-04-18T09:31:39Z

pkg/controller/framework/shared_informer.go

nit: s/coordinatino/coordination/

wojtek-t · 2016-04-18T09:40:06Z

@deads2k - thanks a lot for working on it
I went through the code one more time and added just few very minor comments. Once applied, this PR LGTM.

@derekwaynecarr - should we wait for comments from you?

deads2k · 2016-04-18T12:52:12Z

Comments addressed and squashed.

wojtek-t · 2016-04-18T12:56:05Z

LGTM - thanks!

wojtek-t · 2016-04-18T12:57:03Z

I will wait for @derekwaynecarr for comments and if no comments for a day, I will apply lgtm label (or feel free to reassign it to me, if you know that Derek is fine with that).

k8s-bot · 2016-04-18T13:30:46Z

GCE e2e build/test passed for commit f0c33d6.

deads2k · 2016-04-18T13:32:09Z

@wojtek-t yeah, he'll be fine with it.

wojtek-t · 2016-04-18T13:34:19Z

LGTM - thanks!

k8s-github-robot · 2016-04-18T14:05:46Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-04-18T14:48:14Z

GCE e2e build/test passed for commit f0c33d6.

k8s-github-robot · 2016-04-18T14:48:29Z

Automatic merge from submit-queue

saad-ali · 2016-04-28T01:04:49Z

cmd/kube-controller-manager/app/controllermanager.go


+	// run the shared informers
+	for _, informer := range informers {
+		go informer.Run(wait.NeverStop)


@deads2k I'm creating a new controller so I was looking into using a shared informer. Some questions:

Are shared informers ready for wider use, or are they still a work in progress?

All shared informers (including the podInformer) are started here (Run(...) is called) in the controllermanager, but I see that the individual controllers also start the pod informer passed into them manually (internalPodInformer.Run(...)), won't this result in the podInformer's Run(...) getting called twice?

@saad-ali the internalPodInformer field of the controllers is only set when calling the New_Resource_ControllerFromClient() functions. All of the Run() functions have a check to see if the internalPodInformer != nil before trying to start it.

Said another way, tests use New_Resource_ControllerFromClient() so that the Run() will start the informer. The regular New_Resource_Controller() function, don't set internalPodInformer, and the shared informer is started here.

Also, FYI I have PR #24841 open for converting some of the remaining controllers to using the shared pod informer.

@sjenning that makes perfect sense. Thanks for the clarification.

Are shared informers ready for wider use, or are they still a work in progress?

They are ready for wider use. The initial transitions of controllers to a shared pod informer went very smoothly, so I don't expect any structural problems. Remember not to mess with the object in the cache, its shared.

We should have called the informer method that your register on
"AddMeAsLongAsIPromiseNotToMutateTheSharedCache"

On Thu, Apr 28, 2016 at 8:54 AM, David Eads notifications@github.com
wrote:

In cmd/kube-controller-manager/app/controllermanager.go
#23575 (comment)
:

@@ -410,6 +418,11 @@ func StartControllers(s *options.CMServer, kubeClient *client.Client, kubeconfig
).Run()
time.Sleep(wait.Jitter(s.ControllerStartInterval.Duration, ControllerStartJitter))

// run the shared informers

for _, informer := range informers {

go informer.Run(wait.NeverStop)

Are shared informers ready for wider use, or are they still a work in
progress?

They are ready for wider use. The initial transitions of controllers to a
shared pod informer went very smoothly, so I don't expect any structural
problems. Remember not to mess with the object in the cache, its shared.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/23575/files/f0c33d65b6fea84d791059d96c93a15e4b3de4ec#r61421456

:-)
but I completely agree

They are ready for wider use. The initial transitions of controllers to a shared pod informer went very smoothly, so I don't expect any structural problems. Remember not to mess with the object in the cache, its shared.

Sounds good, thanks!

Automatic merge from submit-queue update node controller to use shared pod informer continuing work from #24470 and #23575

googlebot added the cla: yes label Mar 29, 2016

k8s-github-robot assigned derekwaynecarr Mar 29, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 29, 2016

deads2k force-pushed the shared-cache branch from 97be668 to afed882 Compare March 30, 2016 13:52

derekwaynecarr reviewed Mar 31, 2016
View reviewed changes

pkg/controller/framework/shared_informer.go Outdated

Copy link
Copy Markdown

Member

derekwaynecarr Mar 31, 2016

Choose a reason for hiding this comment

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

maybe you floss really well the next morning ;-)

derekwaynecarr mentioned this pull request Mar 31, 2016

[GarbageCollector] Adding a proposal for server-side cascading deletion #23656

Merged

k8s-github-robot added the release-note-label-needed label Mar 31, 2016

wojtek-t reviewed Apr 11, 2016
View reviewed changes

deads2k force-pushed the shared-cache branch from afed882 to 0e9d126 Compare April 14, 2016 18:16

deads2k changed the title ~~[POC] shared controller informers~~ shared controller informers Apr 14, 2016

k8s-github-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2016

wojtek-t reviewed Apr 18, 2016
View reviewed changes

pkg/controller/framework/shared_informer.go Outdated

Copy link
Copy Markdown

Member

wojtek-t Apr 18, 2016

Choose a reason for hiding this comment

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

nit: s/coordinatino/coordination/

wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 18, 2016

deads2k added 2 commits April 18, 2016 08:51

add SharedInformer

96d1f48

start sharing the pod cache and list/watch

f0c33d6

deads2k force-pushed the shared-cache branch from d7b4ae3 to f0c33d6 Compare April 18, 2016 12:51

deads2k assigned wojtek-t and unassigned derekwaynecarr Apr 18, 2016

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2016

k8s-github-robot merged commit 5ad27f2 into kubernetes:master Apr 18, 2016

deads2k mentioned this pull request Apr 19, 2016

update controllers watching all pods to share an informer #24470

Merged

mqliang mentioned this pull request Apr 19, 2016

add namespace index for cache #23795

Merged

sjenning mentioned this pull request Apr 27, 2016

update node controller to use shared pod informer #24841

Merged

saad-ali reviewed Apr 28, 2016
View reviewed changes

nikhiljindal mentioned this pull request May 9, 2016

Implement the cluster-controller of kubernetes federation #23430

Merged

k8s-github-robot pushed a commit that referenced this pull request Aug 2, 2016

Merge pull request #24841 from sjenning/shared-informer

59836d6

Automatic merge from submit-queue update node controller to use shared pod informer continuing work from #24470 and #23575

deads2k deleted the shared-cache branch September 6, 2016 17:22

Conversation

deads2k commented Mar 29, 2016

Uh oh!

k8s-bot commented Mar 29, 2016

Uh oh!

ncdc commented Mar 30, 2016

Uh oh!

k8s-bot commented Mar 30, 2016

Uh oh!

wojtek-t commented Mar 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Apr 5, 2016

Uh oh!

wojtek-t commented Apr 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtek-t commented Apr 11, 2016

Uh oh!

wojtek-t commented Apr 11, 2016

Uh oh!

deads2k commented Apr 13, 2016

Uh oh!

wojtek-t commented Apr 14, 2016

Uh oh!

timothysc commented Apr 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtek-t commented Apr 18, 2016

Uh oh!

deads2k commented Apr 18, 2016

Uh oh!

wojtek-t commented Apr 18, 2016

Uh oh!

wojtek-t commented Apr 18, 2016

Uh oh!

k8s-bot commented Apr 18, 2016

Uh oh!

deads2k commented Apr 18, 2016

Uh oh!

wojtek-t commented Apr 18, 2016

Uh oh!

k8s-github-robot commented Apr 18, 2016

Uh oh!

k8s-bot commented Apr 18, 2016

Uh oh!

k8s-github-robot commented Apr 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjenning Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

sjenning Apr 28, 2016 •

edited

Loading