Use shared informers in gc controller if possible #45427

ncdc · 2017-05-05T17:11:35Z

Modify the garbage collector controller to try to use shared informers for resources, if possible, to reduce the number of unique reflectors listing and watching the same thing.

cc @kubernetes/sig-api-machinery-pr-reviews @caesarxuchao @deads2k @liggitt @sttts @smarterclayton @timothysc @soltysh @Kargakis @kubernetes/rh-cluster-infra @derekwaynecarr @wojtek-t @gmarek

k8s-reviewable · 2017-05-05T17:11:42Z

This change is

ncdc · 2017-05-05T17:16:54Z

@k8s-bot bazel test this

0xmichalis · 2017-05-05T17:19:03Z

pkg/controller/garbagecollector/graph_builder.go

 			// add the event to the dependencyGraphBuilder's graphChanges.
 			AddFunc: func(obj interface{}) {
-				setObjectTypeMeta(obj)


Not sure why this was originally needed. Can runtime objects exist w/o a Kind?

Yes, if you don't have this, kind and apiVersion are empty

IIRC, we needed it because TypeMeta is reset when unmarshalled.

kubernetes/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/conversion.go

Lines 157 to 163 in db0b0bd

// +k8s:conversion-fn=drop

func Convert_v1_TypeMeta_To_v1_TypeMeta(in, out *TypeMeta, s conversion.Scope) error {

// These values are explicitly not copied

//out.APIVersion = in.APIVersion

//out.Kind = in.Kind

return nil

}

Why are we doing this? Does it mean that any controller that cares about TypeMeta needs to reset the fields?

@Kargakis I don't remember the history. @smarterclayton or @deads2k or @liggitt or @lavalamp probably do?

caesarxuchao · 2017-05-05T17:22:39Z

pkg/controller/garbagecollector/graph_builder.go

+			return nil, fmt.Errorf("expected runtime.Object, got %#v", obj)
+		}
+		if clone {
+			copy, err := api.Scheme.DeepCopy(runtimeObject)


How does this DeepCopy affect the performance?

It's presumably going to do memory allocations and take some cpu time that it wouldn't otherwise be doing. I haven't measured it yet. I wanted to get this up for review first.

I just looked a bit more, and it only uses the object so it can get access to the meta.Accessor and meta.TypeAccessor. Maybe we could be more efficient and not use the full object?

Or we could not clone and instead include the GVK as part of the event object.

Possible change that removes the need to clone: https://gist.github.com/ncdc/24b2716d448cd820eac897c4e5ba5521

Ok, the changes in my gist pass unit & integration tests locally.

Pushed this as the 2nd commit in this PR. PTAL

ncdc · 2017-05-05T18:06:09Z

@k8s-bot bazel test this

ncdc · 2017-05-05T18:10:31Z

@k8s-bot bazel test this

smarterclayton · 2017-05-05T18:17:32Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

-				rbac.NewRule("list", "watch").Groups(certificatesGroup).Resources("certificatesigningrequests").RuleOrDie(),
-				rbac.NewRule("list", "watch").Groups(storageGroup).Resources("storageclasses").RuleOrDie(),
+				// Needed for all shared informers
+				rbac.NewRule("list", "watch").Groups("*").Resources("*").RuleOrDie(),


This is kind of scary. It means the core controllers can see anything in the entire system, whereas before it had to be enumerated. Not terribly different from the loopback controller, but means we don't have a resource that you can't add to garbage collector (like can you use this to fish for token names by doing timing channel attacks on the gc controller?).

Loopback client

I debated doing this vs enumerating. The problem with enumerating is that as new types are added, people likely will forget to add them to the policy, and then you'll see RBAC denials in the logs that may or may not be meaningful (depending on whether or not the new types need to participate in GC).

Also the current GC controller rule is exactly this, but it's scoped to the GC controller and not the controller-manager's client. So this does expand what the controller-manager client can list/watch, but it doesn't broaden the GC controller's powers, as they're already this broad.

We don't need to rush this PR in, so let's discuss how we want to proceed and I'll make the changes once we reach a consensus.

@smarterclayton have you had any more time to think about this?

Game theory:

We add a new resource which is super powerful and the garbage collection controller shouldn't have.

We close the hole where garbage collector seeing secrets means it's root on the cluster anyway

Would everyone have to migrate from * to a maintained list because 1 now shouldn't be given to GC?

Is there anything more powerful than the GC controller already is (can delete everything in the cluster)? Should the GC controller not control PodSecurityPolicy by default? Should the GC controller control RBAC by default?

@smarterclayton I would expect 1. the encryption in the storage to be configurable so I can encrypt more than secrets and 2. I could see the etcd write key being stored in its own TPR that I would probably not want the GC to have access to.

Would everyone have to migrate from * to a maintained list because 1 now shouldn't be given to GC?

So we would expect anyone contributing an aggregated API server or a customresource will create another role and bind the garbage collector to it? It could be done, but we should set the expectation now so we can start working through examples.

We add a new resource which is super powerful and the garbage collection controller shouldn't have.

Add it where - to the core? As a TPR? Via an extension apiserver? Or maybe it doesn't matter and the answer is just "yes" 😄.

To recap where we are today, in the master branch, without my PR:

The GC controller client is allowed to get, list, watch, patch, update, and delete / (

kubernetes/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go

Line 129 in 7b43f92

rbac.NewRule("get", "list", "watch", "patch", "update", "delete").Groups("*").Resources("*").RuleOrDie(),

)

The GC controller only sets up monitoring resources when it is constructed, and it doesn't refresh (e.g. to pick up new TPRs) unless I'm missing seeing it (

kubernetes/pkg/controller/garbagecollector/garbagecollector.go

Line 98 in 7b43f92

if err := gb.monitorsForResources(deletableResources); err != nil {

)

My PR grants the controller-manager the ability to list/watch /, so all current and future shared informers can function without having to remember to modify the policy.

Maybe we need to consider looking into a way to restrict which portions of the code (controllers) are allowed to use specific shared informers? Otherwise I don't have a good idea for how to use shared informers in the GC controller without granting list/watch of / to the controller-manager client.

ncdc · 2017-05-05T22:28:06Z

@k8s-bot bazel test this

deads2k · 2017-05-08T15:12:38Z

pkg/controller/garbagecollector/graph_builder.go

+		},
+	}
+
+	shared, err := gb.sharedInformers.ForResource(resource)


I don't see where you're restarting the shared informers. This could suddenly start watching a previously unwatched resource, so you'll need to be sure that you start the informers again.

Yeah, the informer needs to restart when gc restarts.

The current flow with this PR looks like this:

Create GC controller

Create "monitors" aka controllers and set up event handlers

Run GC controller

Call Run() on all the monitors. If the monitor is a dummy controller from a shared informer, this will be a no-op. Otherwise, it will start the lister/watcher/reflector from the non-shared controller.

Instantiate and run various other controllers

Start the shared informers

Given that the GC controller doesn't currently reread discovery and create new monitors for newly seen resources (or does it???), can we punt on this for now?

Given that the GC controller doesn't currently reread discovery and create new monitors for newly seen resources (or does it???), can we punt on this for now?

It will happens in 1.7. I guess it doesn't have to be this pull, but definitely place a comment.

Do all controllers (gc, replicaset controller, etc.) always restart at the same time? I think it's true, so i think the current PR works.

@caesarxuchao what are you referring to re controllers restarting? AFAIK all controllers start when the kube-controller-manager starts, and they all stop when that process terminates.

Please ignore the "restarting" part. What i was concerned with is the following situation: the shared informer had started by other controllers, then GC starts, and it misses the earlier events. This case seems to be impossible since GC calls Run() on all monitors when it starts.

But will other controllers call Run() on a shared informer? Will that cause GC see duplicate events (though GC should operate correctly if there are duplicate events)?

There are 3 similar functions that can start these things:

shared informer factory Start() - this starts any initialized but unstarted shared informers in the factory. It's a no-op for ones that are already started.

shared informer Run() - this starts running the given shared informer, and is what (1) calls. This is never invoked anywhere in the GC controller code.

controller Run() - this runs a controller, and is what the GC controller calls when it's starting monitors. For shared informers, they return a dummy controller whose Run() implementation is a no-op, so we're safe here.

There is a potential danger if multiple callers invoke a shared informer's Run() function, and we should probably guard against that happening, but it's not in the code right now.

Does this answer your question?

Thanks for the explanation, @ncdc .

this runs a controller, and is what the GC controller calls when it's starting monitors. For shared informers, they return a dummy controller whose Run() implementation is a no-op, so we're safe here.

Does this mean the gc will miss the ADD events for the objects that are already in the sharedInformer before gc invokes the dummyController.Run()?

deads2k · 2017-05-08T15:14:39Z

pkg/controller/garbagecollector/graph_builder.go

 	// TODO: consider store in one storage.
 	glog.V(5).Infof("create storage for resource %s", resource)
 	client, err := gb.metaOnlyClientPool.ClientForGroupVersionKind(kind)
 	if err != nil {
 		return nil, err
 	}
 	gb.registeredRateLimiterForControllers.registerIfNotPresent(resource.GroupVersion(), client, "garbage_collector_monitoring")
-	setObjectTypeMeta := func(obj interface{}) {


Why did this mutation become unnecessary in the non-shared informer case?

Because I pass down the gvk, which has all the info we need, in the event

deads2k · 2017-05-08T15:16:47Z

pkg/controller/garbagecollector/graph_builder.go

 	// Check if the node already exsits
 	existingNode, found := gb.uidToNode.Read(accessor.GetUID())
 	switch {
 	case (event.eventType == addEvent || event.eventType == updateEvent) && !found:
 		newNode := &node{
 			identity: objectReference{
 				OwnerReference: metav1.OwnerReference{
-					APIVersion: typeAccessor.GetAPIVersion(),
-					Kind:       typeAccessor.GetKind(),
+					APIVersion: event.gvk.GroupVersion().String(),


build up the string, don't rely on the stringification for display purposes.

IIRC typeAccessor.GetAPIVersion() is doing exactly this, but I can build it up instead.

Chatted with @deads2k on irc. typeAccessor.GetAPIVersion() just does this under the covers:

obj.GetObjectKind().GroupVersionKind().GroupVersion().String()

which is a multi-line function that has some special casing for "v1" vs everything else. David said he won't block the PR on this (given that it's not a change in functionality).

ncdc · 2017-05-16T16:52:56Z

@k8s-bot unit test this #44846
@k8s-bot kops aws e2e test this

ncdc · 2017-05-16T17:47:59Z

All tests are green. Ready for final review, I think. We also still need to resolve @smarterclayton's question about granting the controller manager client permission to list/watch everything. I'll squash once we've resolved the outstanding policy question.

ncdc · 2017-05-17T00:34:56Z

Restart when? Controllers aren't restarted, are they?

…

On Tue, May 16, 2017 at 8:14 PM Chao Xu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/controller/garbagecollector/graph_builder.go <#45427 (comment)> : > + }, + DeleteFunc: func(obj interface{}) { + // delta fifo may wrap the object in a cache.DeletedFinalStateUnknown, unwrap it + if deletedFinalStateUnknown, ok := obj.(cache.DeletedFinalStateUnknown); ok { + obj = deletedFinalStateUnknown.Obj + } + event := &event{ + eventType: deleteEvent, + obj: obj, + gvk: kind, + } + gb.graphChanges.Add(event) + }, + } + + shared, err := gb.sharedInformers.ForResource(resource) Do all controllers (gc, replicaset controller, etc.) always restart at the same time? I think it's true, so i think the current PR works. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45427 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABYov72nKW-NmwlRzlimz7wpDIJJw_ks5r6jvpgaJpZM4NSLxa> .

ncdc · 2017-05-18T22:18:24Z

No, the dummy controller is just to satisfy the interface.

…

On Thu, May 18, 2017 at 6:11 PM Chao Xu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/controller/garbagecollector/graph_builder.go <#45427 (comment)> : > + }, + DeleteFunc: func(obj interface{}) { + // delta fifo may wrap the object in a cache.DeletedFinalStateUnknown, unwrap it + if deletedFinalStateUnknown, ok := obj.(cache.DeletedFinalStateUnknown); ok { + obj = deletedFinalStateUnknown.Obj + } + event := &event{ + eventType: deleteEvent, + obj: obj, + gvk: kind, + } + gb.graphChanges.Add(event) + }, + } + + shared, err := gb.sharedInformers.ForResource(resource) Thanks for the explanation, @ncdc <https://github.com/ncdc> . this runs a controller, and is what the GC controller calls when it's starting monitors. For shared informers, they return a dummy controller whose Run() implementation is a no-op, so we're safe here. Does this mean the gc will miss the ADD events for the objects that are already in the sharedInformer before gc invokes the dummyController.Run()? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45427 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABYh6Y1Z7gu0AkE8AywTShoUOwAhU9ks5r7MIlgaJpZM4NSLxa> .

caesarxuchao · 2017-05-18T22:39:12Z

@ncdc you are right. They are handled by the listener and processor logic. Sorry i should had read the sharedInformer code more patiently.

ncdc · 2017-05-22T16:49:27Z

Bump, anyone have any other comments?

smarterclayton · 2017-05-22T16:50:06Z

I'm fine with */*. If no other comments, this is lgtm as well

ncdc · 2017-05-22T16:51:57Z

Squashed

ncdc · 2017-05-22T17:26:37Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

k8s-ci-robot · 2017-05-22T17:54:33Z

@ncdc: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`2480f2c`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ncdc · 2017-05-22T17:56:38Z

Green tests (federation is non-blocking due to flakiness for now)

smarterclayton · 2017-05-22T18:02:26Z

I'd like one or two reviewers to chime in

ncdc · 2017-05-22T18:03:08Z

Definitely

deads2k · 2017-05-22T18:11:00Z

lgtm

caesarxuchao · 2017-05-22T18:29:22Z

The gc changes lgtm.

smarterclayton · 2017-05-22T20:25:26Z

/lgtm

as per reviewers

k8s-github-robot · 2017-05-22T20:25:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc, smarterclayton

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-controller-manager/OWNERS~~ [smarterclayton]
~~pkg/controller/OWNERS~~ [smarterclayton]
~~plugin/pkg/auth/OWNERS~~ [smarterclayton]
~~test/OWNERS~~ [ncdc,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-23T03:58:02Z

Automatic merge from submit-queue (batch tested with PRs 46201, 45952, 45427, 46247, 46062)

ncdc added release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels May 5, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 5, 2017

k8s-github-robot assigned liggitt and lavalamp May 5, 2017

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 5, 2017

0xmichalis reviewed May 5, 2017

View reviewed changes

caesarxuchao reviewed May 5, 2017

View reviewed changes

ncdc mentioned this pull request May 5, 2017

Duplicate caches in OpenShift master openshift/origin#8229

Closed

smarterclayton reviewed May 5, 2017

View reviewed changes

deads2k reviewed May 8, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2017

ncdc force-pushed the gc-shared-informers branch from da80ba2 to 61e2d4c Compare May 16, 2017 15:39

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 16, 2017

ncdc mentioned this pull request May 16, 2017

GC: update required verbs for deletable resources, allow list of ignored resources to be customized #45897

Merged

ncdc assigned caesarxuchao and deads2k May 16, 2017

Use shared informers in gc controller if possible

2480f2c

ncdc force-pushed the gc-shared-informers branch from 61e2d4c to 2480f2c Compare May 22, 2017 16:51

ncdc added this to the v1.7 milestone May 22, 2017

k8s-ci-robot assigned smarterclayton May 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2017

k8s-github-robot merged commit cc6e51c into kubernetes:master May 23, 2017

caesarxuchao mentioned this pull request May 25, 2017

Add an Informer example in client-go #44446

Closed

ncdc deleted the gc-shared-informers branch October 22, 2018 15:30

	// +k8s:conversion-fn=drop
	func Convert_v1_TypeMeta_To_v1_TypeMeta(in, out *TypeMeta, s conversion.Scope) error {
	// These values are explicitly not copied
	//out.APIVersion = in.APIVersion
	//out.Kind = in.Kind
	return nil
	}

Use shared informers in gc controller if possible #45427

Use shared informers in gc controller if possible #45427

Conversation

ncdc commented May 5, 2017

k8s-reviewable commented May 5, 2017

ncdc commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc May 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented May 5, 2017

ncdc commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc May 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented May 16, 2017

ncdc commented May 16, 2017

ncdc commented May 17, 2017 via email

ncdc commented May 18, 2017 via email

caesarxuchao commented May 18, 2017

ncdc commented May 22, 2017

smarterclayton commented May 22, 2017 • edited by ncdc

ncdc commented May 22, 2017

ncdc commented May 22, 2017

k8s-ci-robot commented May 22, 2017

ncdc commented May 22, 2017

smarterclayton commented May 22, 2017

ncdc commented May 22, 2017

deads2k commented May 22, 2017

caesarxuchao commented May 22, 2017

smarterclayton commented May 22, 2017

k8s-github-robot commented May 22, 2017

k8s-github-robot commented May 23, 2017

ncdc May 5, 2017 •

edited

ncdc May 5, 2017 •

edited

smarterclayton commented May 22, 2017 •

edited by ncdc