Delay informer start after controller. #1757

zhongduo · 2020-09-29T22:32:33Z

Starting informer before controller will cause resync period set
in the informer not respected, this commit makes EnableInjectionOrDie
to return informers so that the callers can delay the start of the
informer.

Fix #1756

zhongduo · 2020-09-29T22:33:32Z

/assign @n3wscott

zhongduo · 2020-09-29T22:33:44Z

/assign @grantr

codecov · 2020-09-29T22:37:36Z

Codecov Report

Merging #1757 into master will not change coverage.
The diff coverage is 58.33%.

@@           Coverage Diff           @@
##           master    #1757   +/-   ##
=======================================
  Coverage   68.61%   68.61%           
=======================================
  Files         215      215           
  Lines        9239     9239           
=======================================
  Hits         6339     6339           
  Misses       2584     2584           
  Partials      316      316

Impacted Files	Coverage Δ
codegen/cmd/injection-gen/generators/packages.go	`0.00% <0.00%> (ø)`
test/helpers/dryrun.go	`0.00% <0.00%> (ø)`
test/interactive/docker.go	`42.42% <0.00%> (ø)`
test/mako/config/benchmark.go	`0.00% <0.00%> (ø)`
test/mako/config/environment.go	`0.00% <0.00%> (ø)`
apis/field_error.go	`98.29% <100.00%> (ø)`
metrics/prometheus_exporter.go	`89.28% <100.00%> (ø)`
profiling/server.go	`84.37% <100.00%> (ø)`
testutils/clustermanager/e2e-tests/util.go	`75.00% <100.00%> (ø)`
webhook/webhook.go	`77.38% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2478414...735f6a3. Read the comment docs.

n3wscott · 2020-09-30T20:37:12Z

injection/sharedmain/main.go

-
-	// Start the injection clients and informers.
-	logging.FromContext(ctx).Info("Starting informers...")
-	go func(ctx context.Context) {


The whole point of this method is to start the informers here and not need to duplicate that code in the caller.

Perhaps we can pass a delay config in the inbound context object instead? The goal is to reduce the amount of special code each injection enabled cmd needs to run. MainWithConfig is just one simple example.

Another example of integration: https://github.com/knative-sandbox/eventing-rabbitmq/blob/56208970df1d5db5526a12137d54ea16b37fe149/test/e2e/cmd/recorder/main.go#L32

I don't want to have to copy the informer startup code in all the places I have called enable injection.

Delaying the start of goroutine might be a little bit tricky. If we use a magic time duration, then it might cause lots of unit test to fail. If we use a signal instead, then we still need to change all the downstream codes to signal start, which is same as calling StartInformers directly.

Please check this PR #1772 again. I think I prefer a single method that returns a callback over two new methods.

n3wscott · 2020-09-30T20:37:41Z

injection/sharedmain/main.go

@@ -129,7 +129,7 @@ func GetLeaderElectionConfig(ctx context.Context) (*leaderelection.Config, error

 // EnableInjectionOrDie enables Knative Injection and starts the informers.
 // Both Context and Config are optional.
-func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) context.Context {
+func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) (context.Context, []controller.Informer) {


This is a breaking api change for all things that already use this and we can't do that for all the downstreams.

/hold

I guess this logic only works if StartInformers is called immediately after this function. At least sharedmain does not do that, so it needs to be reverted. Callers like chaosduck/main.go however seems less a problem, though I still think it should be fixed.

n3wscott · 2020-09-30T20:38:32Z

leaderelection/chaosduck/main.go

-	ctx = sharedmain.EnableInjectionOrDie(ctx, nil)
-
+	ctx, informers := sharedmain.EnableInjectionOrDie(ctx, nil)
+	if err := controller.StartInformers(ctx.Done(), informers...); err != nil {


yeah ^ exactly, I don't want to need to copy this chunk of code in the dozens of places EnableInjectionOrDie is used.

Same here, will it be better if the EnableInjectionOrDie function return a start function instead of []informers, so downstream just need to call a function instead of this chunk of code? I do know this will still break the API.

…r manually

knative-prow-robot · 2020-10-06T13:38:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhongduo
To complete the pull request process, please assign grantr after the PR has been reviewed.
You can assign the PR to them by writing /assign @grantr in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

injection/OWNERS

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

knative-prow-robot · 2020-10-06T13:46:14Z

@zhongduo: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-pkg-unit-tests	`735f6a3`	link	`/test pull-knative-pkg-unit-tests`

Full PR test history. Your PR dashboard.

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.

zhongduo · 2020-10-07T16:36:54Z

Closing this because the fix is done in #1772

knative-prow-robot · 2020-10-07T16:37:02Z

@zhongduo: PR needs rebase.

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.

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 29, 2020

knative-prow-robot requested review from evankanderson and n3wscott September 29, 2020 22:32

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 29, 2020

knative-prow-robot assigned n3wscott Sep 29, 2020

knative-prow-robot assigned grantr Sep 29, 2020

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 29, 2020

n3wscott reviewed Sep 30, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2020

n3wscott reviewed Sep 30, 2020

View reviewed changes

zhongduo mentioned this pull request Oct 2, 2020

Delay start of informers in sharedmain #1767

Merged

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2020

n3wscott mentioned this pull request Oct 5, 2020

Put EnableInjectionOrDie back on the main path #1772

Merged

Break down EnableInjectionOrDie to allow flexibility to start informe…

735f6a3

…r manually

zhongduo force-pushed the fix-resync-period branch from 89b56be to 735f6a3 Compare October 6, 2020 13:38

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 6, 2020

zhongduo requested a review from n3wscott October 6, 2020 13:40

zhongduo closed this Oct 7, 2020

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay informer start after controller. #1757

Delay informer start after controller. #1757

zhongduo commented Sep 29, 2020

zhongduo commented Sep 29, 2020

zhongduo commented Sep 29, 2020

codecov bot commented Sep 29, 2020 •

edited

Loading

n3wscott Sep 30, 2020

zhongduo Sep 30, 2020

n3wscott Oct 6, 2020 •

edited

Loading

n3wscott Sep 30, 2020

zhongduo Sep 30, 2020

n3wscott Sep 30, 2020

zhongduo Sep 30, 2020

knative-prow-robot commented Oct 6, 2020

knative-prow-robot commented Oct 6, 2020

zhongduo commented Oct 7, 2020

knative-prow-robot commented Oct 7, 2020

Delay informer start after controller. #1757

Delay informer start after controller. #1757

Conversation

zhongduo commented Sep 29, 2020

zhongduo commented Sep 29, 2020

zhongduo commented Sep 29, 2020

codecov bot commented Sep 29, 2020 • edited Loading

Codecov Report

n3wscott Sep 30, 2020

Choose a reason for hiding this comment

zhongduo Sep 30, 2020

Choose a reason for hiding this comment

n3wscott Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

n3wscott Sep 30, 2020

Choose a reason for hiding this comment

zhongduo Sep 30, 2020

Choose a reason for hiding this comment

n3wscott Sep 30, 2020

Choose a reason for hiding this comment

zhongduo Sep 30, 2020

Choose a reason for hiding this comment

knative-prow-robot commented Oct 6, 2020

knative-prow-robot commented Oct 6, 2020

zhongduo commented Oct 7, 2020

knative-prow-robot commented Oct 7, 2020

codecov bot commented Sep 29, 2020 •

edited

Loading

n3wscott Oct 6, 2020 •

edited

Loading