dynamic informer factory #69308

p0lyn0mial · 2018-10-01T20:10:48Z

What this PR does / why we need it: adds dynamic informer factory

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): #64319

Special notes for your reviewer:

Release note:

adds dynamic shared informers to write generic, non-generated controllers

p0lyn0mial · 2018-10-01T20:11:02Z

/assign @deads2k

p0lyn0mial · 2018-10-01T20:11:32Z

staging/src/k8s.io/client-go/dynamic/dynamicinformer/informer.go

+}
+
+// Start initializes all requested informers.
+// TODO: accept context ?


No context. The start should be consistent with the normal SharedInformerFactory. It may be helpful to make an interface that contains both. It would have

start

waitforcachesync

forresource

We would have to create a new interface that would contain methods that accept the context. Golang doesn't support method overloading :)

p0lyn0mial · 2018-10-01T20:13:31Z

staging/src/k8s.io/client-go/dynamic/dynamicinformer/informer.go

+	f.lock.Lock()
+	defer f.lock.Unlock()
+
+	// TODO: do we want to format the keys ?


to make debugging more informative, especially when using WaitForCacheSync rather than printing something like /v1, Resources=pod

to make debugging more informative, especially when using WaitForCacheSync rather than printing something like /v1, Resources=pod

I would key on the natural gvr and then coerce the display however we like

oh, good idea.

p0lyn0mial · 2018-10-01T20:14:12Z

staging/src/k8s.io/client-go/dynamic/dynamicinformer/informer.go

+}
+
+// NewFilteredDynamicInformer constructs a new informer for a dynamic type.
+func NewFilteredDynamicInformer(client dynamic.Interface, gvr schema.GroupVersionResource, namespace string, resyncPeriod time.Duration, indexers cache.Indexers, tweakListOptions TweakListOptionsFunc) informers.GenericInformer {


should this method be public ?
should this method return GenericInformer ?

should this method be public ?
should this method return GenericInformer ?

this looks reasonable.

p0lyn0mial · 2018-10-01T20:16:31Z

staging/src/k8s.io/client-go/dynamic/dynamicinformer/interface.go

+	WaitForCacheSync(stopCh <-chan struct{}) map[string]bool
+}
+
+type TweakListOptionsFunc func(*metav1.ListOptions)


are you okay with defining this func here ?
I think it is already defined somewhere here k8s.io/client-go/informers/internalinterfaces

are you okay with defining this func here ?
I think it is already defined somewhere here k8s.io/client-go/informers/internalinterfaces

Yes, we want to keep this here. Eventually we are likely to need to snip the links to the typed clients.

p0lyn0mial · 2018-10-01T20:18:10Z

@deads2k waiting for your feedback before adding some tests.

deads2k · 2018-10-02T11:55:41Z

staging/src/k8s.io/client-go/dynamic/dynamicinformer/informer.go

+	namespace     string
+
+	lock      sync.Mutex
+	informers map[string]informers.GenericInformer


I expected a GVR

deads2k · 2018-10-02T12:01:05Z

staging/src/k8s.io/client-go/dynamic/dynamiclister/shim.go

+
+// New returns a new shim for Lister.
+// It wraps Lister so that it implements cache.GenericLister interface
+func NewShim(lister Lister) cache.GenericLister {


NewRuntimeObjectShim?

deads2k · 2018-10-02T12:03:24Z

This looks really good. Have you been able to think of a good way to test it?

p0lyn0mial · 2018-10-02T21:39:49Z

Have you been able to think of a good way to test it?

not much, but I expect it will be messy, I think the flow will be:

create a fake client
create a factory and give it the fake client
create an informer
start and wait for the informer
use the fake client to add a resource
get the lister and see if the resource is there ?

jennybuckley · 2018-10-04T20:50:25Z

/cc @yliaog

deads2k · 2018-10-08T12:29:16Z

not much, but I expect it will be messy, I think the flow will be:

create a fake client

create a factory and give it the fake client

create an informer

start and wait for the informer

use the fake client to add a resource

get the lister and see if the resource is there ?

/lgtm
pending test

/hold

holding for the test.

p0lyn0mial · 2018-10-08T20:10:45Z

added missing test, PTAL

yliaog · 2018-10-09T18:25:54Z

the commit "adds dynamic lister" 5c9261294cbcf1e2daab5719bbc8b4cd4dcd0a6c could be removed

deads2k · 2018-10-10T18:21:34Z

/lgtm
/hold cancel

@p0lyn0mial great job.

deads2k · 2018-10-10T18:28:44Z

/approve

k8s-ci-robot · 2018-10-10T18:28:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, p0lyn0mial

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:

~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yue9944882 · 2018-10-11T02:15:38Z

/kind feature

p0lyn0mial · 2018-10-11T08:27:50Z

/test pull-kubernetes-e2e-kops-aws

k8s-ci-robot requested review from caesarxuchao and liggitt October 1, 2018 20:10

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 1, 2018

k8s-ci-robot assigned deads2k Oct 1, 2018

p0lyn0mial commented Oct 1, 2018

View reviewed changes

deads2k reviewed Oct 2, 2018

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 2, 2018

nikhita mentioned this pull request Oct 3, 2018

Support object count quota for custom resources #64201

Closed

k8s-ci-robot requested a review from yliaog October 4, 2018 20:50

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 8, 2018

p0lyn0mial force-pushed the dynamic_informer branch from b544571 to 0b63464 Compare October 8, 2018 19:28

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2018

p0lyn0mial force-pushed the dynamic_informer branch from 0b63464 to 48a8d29 Compare October 8, 2018 19:41

p0lyn0mial force-pushed the dynamic_informer branch from 48a8d29 to d3dc9c5 Compare October 8, 2018 20:40

adds dynamic informer factory

4d479af

p0lyn0mial force-pushed the dynamic_informer branch from d3dc9c5 to 4d479af Compare October 10, 2018 08:06

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 10, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2018

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 11, 2018

k8s-ci-robot merged commit effd009 into kubernetes:master Oct 11, 2018

p0lyn0mial mentioned this pull request Oct 11, 2018

Create dynamic informers and listers #64319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dynamic informer factory #69308

dynamic informer factory #69308

p0lyn0mial commented Oct 1, 2018 •

edited

p0lyn0mial commented Oct 1, 2018

p0lyn0mial Oct 1, 2018

deads2k Oct 2, 2018

p0lyn0mial Oct 2, 2018

p0lyn0mial Oct 1, 2018

deads2k Oct 2, 2018

p0lyn0mial Oct 2, 2018

p0lyn0mial Oct 1, 2018

deads2k Oct 2, 2018

p0lyn0mial Oct 1, 2018

deads2k Oct 2, 2018

p0lyn0mial commented Oct 1, 2018

deads2k Oct 2, 2018

deads2k Oct 2, 2018

p0lyn0mial Oct 2, 2018

deads2k commented Oct 2, 2018

p0lyn0mial commented Oct 2, 2018

jennybuckley commented Oct 4, 2018

deads2k commented Oct 8, 2018

p0lyn0mial commented Oct 8, 2018

yliaog commented Oct 9, 2018

deads2k commented Oct 10, 2018

deads2k commented Oct 10, 2018

k8s-ci-robot commented Oct 10, 2018

yue9944882 commented Oct 11, 2018

p0lyn0mial commented Oct 11, 2018

dynamic informer factory #69308

dynamic informer factory #69308

Conversation

p0lyn0mial commented Oct 1, 2018 • edited

p0lyn0mial commented Oct 1, 2018

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

p0lyn0mial commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 2, 2018

p0lyn0mial commented Oct 2, 2018

jennybuckley commented Oct 4, 2018

deads2k commented Oct 8, 2018

p0lyn0mial commented Oct 8, 2018

yliaog commented Oct 9, 2018

deads2k commented Oct 10, 2018

deads2k commented Oct 10, 2018

k8s-ci-robot commented Oct 10, 2018

yue9944882 commented Oct 11, 2018

p0lyn0mial commented Oct 11, 2018

p0lyn0mial commented Oct 1, 2018 •

edited