code-gen: allow specifying custom resync periods for certain informer types #61400

natronq · 2018-03-20T11:16:26Z

What this PR does / why we need it:
This PR extends the informer code-generator to allow the consumer to specify a custom resync period for certain informer types and uses the default resync period if none is defined.

Special notes for your reviewer:
Example:

cs := clientset.NewForConfigOrDie(config)
resyncConfig := externalversions.ResyncConfiguration{
  &samplev1alpha1.Sample{}: 30 * time.Second,
}
informer := externalversions.NewSharedInformerFactory(cs, 2*time.Minute, externalversions.WithCustomResyncConfig(resyncConfig))

Release note:

NONE

natronq · 2018-03-20T11:18:58Z

/assign @wojtek-t

dims · 2018-03-23T19:59:38Z

/ok-to-test

natronq · 2018-03-29T08:29:01Z

/retest

natronq · 2018-03-30T17:16:10Z

/retest

natronq · 2018-03-31T15:04:00Z

/cc @sttts

sttts · 2018-04-03T12:29:36Z

.../apiextensions-apiserver/examples/client-go/pkg/client/informers/externalversions/factory.go

 }

 // NewFilteredSharedInformerFactory constructs a new instance of sharedInformerFactory.
 // Listers obtained via this SharedInformerFactory will be subject to the same filters
 // as specified here.
-func NewFilteredSharedInformerFactory(client versioned.Interface, defaultResync time.Duration, namespace string, tweakListOptions internalinterfaces.TweakListOptionsFunc) SharedInformerFactory {
+func NewFilteredSharedInformerFactory(client versioned.Interface, defaultResync time.Duration, namespace string, tweakListOptions internalinterfaces.TweakListOptionsFunc,


why does NewFilteredSharedInformerFactory get the resyncConfig param, but NewSharedInformerFactory doesn't?

Instead of adding more args, we should switch to another extension pattern, maybe informer.WithCustomResyncs.

Adding the parameter to NewSharedInformerFactory would cause a huge refactor accross the Kubernetes code-base, because all consumers of this package exclusivley use this method to instantiate a new factory.

Changing the arguments of NewFilteredSharedInformerFactory does not break any existing functionality as it is not used verbatim. So this seemed like a natural fit as it also already incorporates custom list options.

What I wanted to say is that adding another param is the wrong pattern.

Ok, so you would suggest adding a seperate method e.g. NewSharedInformerFactoryWithCustomResyncinstead?

No. Rather I would prefer some functional style pattern:

NewSharedInformerFactory(...).WithTweakListOptions(...).WithCustomResyncConfig(...).

Refactored accordingly.

k8s-ci-robot · 2018-04-05T11:11:26Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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.

sttts · 2018-04-05T11:35:01Z

pkg/client/informers/informers_generated/internalversion/factory.go

-func NewFilteredSharedInformerFactory(client internalclientset.Interface, defaultResync time.Duration, namespace string, tweakListOptions internalinterfaces.TweakListOptionsFunc) SharedInformerFactory {
-	return &sharedInformerFactory{
+// NewFilteredSharedInformerFactory constructs a new instance of sharedInformerFactory contained to the specified namespace with additional options.
+func NewFilteredSharedInformerFactory(client internalclientset.Interface, defaultResync time.Duration, namespace string, options ...SharedInformerOption) SharedInformerFactory {


~~we have to leave the old func and reimplement it with NewInformerFactoryWithOptions.~~

Disregard my previous comment. it's compatible. But still, I would not add the options here. Instead I would even deprecate it in favor of the generic other variant with options.

As NewFilteredSharedInformerFactory is not used anywhere I deleted it and introdued NewSharedInformerFactoryWithOptions.

It's used by 3rdparties. Would leave it and just deprecate it.

sttts · 2018-04-05T11:35:32Z

pkg/client/informers/informers_generated/internalversion/factory.go

@@ -44,36 +44,63 @@ import (
 	storage "k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion/storage"
 )

+type ResyncConfiguration map[v1.Object]time.Duration


do we really need a ResyncConfiguration type? Sounds "over-abstracted".

Good catch, removed it.

sttts · 2018-04-05T11:38:01Z

pkg/generated/bindata.go

@@ -13651,7 +13651,7 @@ msgstr "Imposta il current-context in un file kubeconfig"

 #: pkg/kubectl/cmd/describe.go:86
 msgid "Show details of a specific resource or group of resources"
-msgstr "Mostra i dettagli di una specifiche risorsa o un gruppo di risorse"
+msgstr "Mostra i dettagli di una specifica risorsa o un gruppo di risorse"


unexpected change. Make it another commit at least if this fix is right.

This was not intended. I think it was caused by running update-codegen.sh against the current master.

Do we have a spell checker included now? :)

Looks like it https://github.com/kubernetes/kubernetes/blob/3ea4e449c361a66eb3539433487cd22d7256b032/hack/verify-spelling.sh
:-)

Wow. Anyway, saw the same issue. The checked in bindata is broken it seems.

Separated it in another commit.

sttts · 2018-04-05T12:59:44Z

pkg/client/informers/informers_generated/internalversion/factory.go

+}
+
+// NewSharedInformerFactoryWithOptions constructs a new instance of a SharedInformerFactory with additional options.
+func NewSharedInformerFactoryWithOptions(client internalclientset.Interface, defaultResync time.Duration, options ...SharedInformerOption) SharedInformerFactory {


While not exactly the same type, this is compatible with NewSharedInformerFactory calls. It might be worth to merge those two again. I think the chance is high that nobody passes around constructors such that this change will make any trouble.

I agree that compatibility is probably not an issue here, but I actually prefer having a separate method for passing the options. It is more verbose and immediately clear to the caller that this method accepts additional options.

sttts · 2018-04-05T13:00:47Z

/cc @caesarxuchao @ncdc any opinion about this style? I like this much more than adding more and more constructors. This pattern is much more idiomatic.

ncdc · 2018-04-05T13:30:43Z

Style is great. Let's do this!

ncdc · 2018-04-05T13:30:54Z

staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go

@@ -98,36 +99,78 @@ func (g *factoryGenerator) GenerateType(c *generator.Context, t *types.Type, w i
 }

 var sharedInformerFactoryStruct = `
+type SharedInformerOption func(*sharedInformerFactory) *sharedInformerFactory


ncdc · 2018-04-05T13:31:41Z

staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go


 	informers map[{{.reflectType|raw}}]{{.cacheSharedIndexInformer|raw}}
 	// startedInformers is used for tracking which informers have been started.
 	// This allows Start() to be called multiple times safely.
 	startedInformers map[{{.reflectType|raw}}]bool
 }

-// NewSharedInformerFactory constructs a new instance of sharedInformerFactory
+// Sets a custom resync period for the specified informer types.


Please use the function name as the beginning of the comment, i.e. // WithCustomResyncConfig sets a ...

ncdc · 2018-04-05T13:32:01Z

staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go

+	}
+}
+
+// Sets a custom filter on all listers of the configured SharedInformerFactory.


Please use the function name as the beginning of the comment, i.e. // WithTweakListOptions sets a ...

ncdc · 2018-04-05T13:32:37Z

staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go

+	}
+}
+
+// Limits the SharedInformerFactory to the specified namespace.


Please use the function name as the beginning of the comment, i.e. // WithNamespace limits the ...

ncdc · 2018-04-05T13:35:08Z

@natronq thanks for this - I really like it.

… types and switch to functional option pattern for SharedInformerFactory

natronq · 2018-04-05T21:40:03Z

@ncdc Refactored according to your comments.

natronq · 2018-04-06T07:33:18Z

/retest

ncdc · 2018-04-06T13:47:26Z

/lgtm

sttts · 2018-04-06T13:53:46Z

/approve

ncdc · 2018-04-06T13:55:34Z

/assign @deads2k for approval

k8s-ci-robot · 2018-04-06T13:55:35Z

@ncdc: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubernetes members and repo collaborators can be assigned.

In response to this:

/assign @deads2k for approval

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.

deads2k · 2018-04-06T18:49:32Z

/cc @caesarxuchao @ncdc any opinion about this style? I like this much more than adding more and more constructors. This pattern is much more idiomatic.

I dislike the style, but as long as the types remain private to make it impossible to write "trust me, I'm doing good things" functions I won't fight it.

/approve

k8s-ci-robot · 2018-04-06T18:49:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, natronq, ncdc, sttts

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:

~~pkg/client/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/code-generator/OWNERS~~ [sttts]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/sample-controller/OWNERS~~ [sttts]

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

k8s-github-robot · 2018-04-06T21:06:24Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-04-06T21:47:05Z

Automatic merge from submit-queue (batch tested with PRs 61400, 61048). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot · 2018-04-06T22:25:56Z

@natronq: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	`a923acd`	link	`/test pull-kubernetes-verify`

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.

k8s-ci-robot requested review from lavalamp and wojtek-t March 20, 2018 11:16

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 Mar 20, 2018

k8s-ci-robot assigned wojtek-t Mar 20, 2018

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 23, 2018

natronq force-pushed the master branch from dc2d30d to c603fdc Compare March 30, 2018 14:44

natronq force-pushed the master branch from c603fdc to 8c71f12 Compare March 30, 2018 19:56

k8s-ci-robot requested a review from sttts March 31, 2018 15:04

sttts reviewed Apr 3, 2018

View reviewed changes

natronq force-pushed the master branch from 8c71f12 to 1660ce9 Compare April 5, 2018 11:11

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 5, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 5, 2018

natronq force-pushed the master branch from 1660ce9 to 51010da Compare April 5, 2018 11:17

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 5, 2018

sttts reviewed Apr 5, 2018

View reviewed changes

natronq force-pushed the master branch 2 times, most recently from 75e85e5 to 5025e4a Compare April 5, 2018 12:52

sttts reviewed Apr 5, 2018

View reviewed changes

ncdc suggested changes Apr 5, 2018

View reviewed changes

natronq added 2 commits April 5, 2018 23:35

code-gen: allow specifying custom resync periods for certain informer…

4f4798a

… types and switch to functional option pattern for SharedInformerFactory

Run hack/update-codegen.sh

a923acd

natronq force-pushed the master branch from 5025e4a to a923acd Compare April 5, 2018 21:39

ncdc approved these changes Apr 6, 2018

View reviewed changes

k8s-ci-robot assigned ncdc Apr 6, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2018

k8s-ci-robot assigned deads2k Apr 6, 2018

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

k8s-github-robot merged commit 7daaa82 into kubernetes:master Apr 6, 2018

code-gen: allow specifying custom resync periods for certain informer types #61400

code-gen: allow specifying custom resync periods for certain informer types #61400

Conversation

natronq commented Mar 20, 2018 • edited Loading

natronq commented Mar 20, 2018

dims commented Mar 23, 2018

natronq commented Mar 29, 2018

natronq commented Mar 30, 2018

natronq commented Mar 31, 2018

Choose a reason for hiding this comment

natronq Apr 3, 2018 • edited Loading

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

k8s-ci-robot commented Apr 5, 2018

sttts Apr 5, 2018 • edited Loading

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

sttts commented Apr 5, 2018

ncdc commented Apr 5, 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

ncdc commented Apr 5, 2018

natronq commented Apr 5, 2018

natronq commented Apr 6, 2018

ncdc commented Apr 6, 2018

sttts commented Apr 6, 2018

ncdc commented Apr 6, 2018

k8s-ci-robot commented Apr 6, 2018

deads2k commented Apr 6, 2018

k8s-ci-robot commented Apr 6, 2018

k8s-github-robot commented Apr 6, 2018

k8s-github-robot commented Apr 6, 2018

k8s-ci-robot commented Apr 6, 2018

natronq commented Mar 20, 2018 •

edited

Loading

natronq Apr 3, 2018 •

edited

Loading

sttts Apr 5, 2018 •

edited

Loading