Put EnableInjectionOrDie back on the main path #1772

n3wscott · 2020-10-05T17:22:32Z

Supersedes #1757
Reverts #1767

codecov · 2020-10-05T17:27:43Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2478414). Click here to learn what that means.
The diff coverage is 14.70%.

@@            Coverage Diff            @@
##             master    #1772   +/-   ##
=========================================
  Coverage          ?   68.23%           
=========================================
  Files             ?      218           
  Lines             ?     9295           
  Branches          ?        0           
=========================================
  Hits              ?     6342           
  Misses            ?     2637           
  Partials          ?      316

Impacted Files	Coverage Δ
codegen/cmd/injection-gen/generators/packages.go	`0.00% <0.00%> (ø)`
injection/config.go	`0.00% <0.00%> (ø)`
injection/injection.go	`0.00% <0.00%> (ø)`
test/helpers/dryrun.go	`0.00% <0.00%> (ø)`
test/helpers/name.go	`94.28% <ø> (ø)`
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/duck/v1/binding_types.go	`6.66% <6.66%> (ø)`
apis/duck/v1/register.go	`100.00% <100.00%> (ø)`
... and 5 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...ff42410. Read the comment docs.

n3wscott · 2020-10-05T17:41:44Z

/retest

vagababov · 2020-10-05T17:30:44Z

injection/sharedmain/main.go

+	}
+
+	untyped := ctx.Value(informerStartChanKey{})
+	if untyped == nil {


given return is interface{} this might not always work.
https://medium.com/@mangatmodi/go-check-nil-interface-the-right-way-d142776edef1

zhongduo · 2020-10-06T03:58:14Z

Hi @n3wscott,

I think this PR needs some discussion. I understand you are trying to avoid changing the function signature to maintain compatibility, but I believe API compatibility also includes underlying logic too and this PR changes the logic considerably IMO. If I understand correctly, with this PR:

For users other than shared_main, the informer start with be delayed for 5 seconds.
For shared_main, it chooses to start the informer after 5 seconds or after controller setup, whichever comes first.

For 1, though I know the timeout is configurable, I do believe most users won't notice this change until they encounter a problem. I am afraid that we are making the issue harder to reproduce to most users and the magic 5 seconds can cause some unexpected problems. Eg, in some case if the controller setup takes about 5 seconds for whatever reason, they will experience some flakiness depending on the actual runtime > 5s or < 5s. Another possible problem is that some logic might be assuming informer start at that time, and this delay start will be quite difficult to debug. So to me it is not much different from the original logic, which starts the informer immediately.

For 2 or any other users that are aware of this problem and decide to start it manually, they can easily just use existing code without this PR. If we want to keep API and at the same time avoid duplicate code, here is what I would suggest:
a) Break the existing EnableInjectionOrDie to two parts: BeforeStartInformer and StartInfomer
b) Rewrite EnableInjectionOrDie to BeforeStartInformer + go StartInformer, which is same logic as the original EnableInjectionOrDie.
c) For shared_main or any users that want to start manually, we can do: BeforeStartInformer + do sth + StartInformer

Thanks,
Jimmy

zhongduo · 2020-10-06T13:43:44Z

I have updated #1757 with this idea, which will keep API compatibility, could you please see if that can be an alternative solution too?

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

n3wscott · 2020-10-06T16:25:02Z

Hi @n3wscott,

I think this PR needs some discussion. I understand you are trying to avoid changing the function signature to maintain compatibility, but I believe API compatibility also includes underlying logic too and this PR changes the logic considerably IMO. If I understand correctly, with this PR:

For users other than shared_main, the informer start with be delayed for 5 seconds.

For shared_main, it chooses to start the informer after 5 seconds or after controller setup, whichever comes first.

For 1, though I know the timeout is configurable, I do believe most users won't notice this change until they encounter a problem. I am afraid that we are making the issue harder to reproduce to most users and the magic 5 seconds can cause some unexpected problems. Eg, in some case if the controller setup takes about 5 seconds for whatever reason, they will experience some flakiness depending on the actual runtime > 5s or < 5s. Another possible problem is that some logic might be assuming informer start at that time, and this delay start will be quite difficult to debug. So to me it is not much different from the original logic, which starts the informer immediately.

For 2 or any other users that are aware of this problem and decide to start it manually, they can easily just use existing code without this PR. If we want to keep API and at the same time avoid duplicate code, here is what I would suggest:
a) Break the existing EnableInjectionOrDie to two parts: BeforeStartInformer and StartInfomer
b) Rewrite EnableInjectionOrDie to BeforeStartInformer + go StartInformer, which is same logic as the original EnableInjectionOrDie.
c) For shared_main or any users that want to start manually, we can do: BeforeStartInformer + do sth + StartInformer

Thanks,
Jimmy

Thanks for the thoughtful response. I agree that the timeout was strange. I have moved the EnableInjectionOrDie, and changed the signature, it now returns back a callback the caller should call if it wants to start the informers. It is basically BeforeStartInformer + do sth + StartInformer except wrapped up in one call with a callback. I think the naming of BeforeStartInformer and StartInformer is strange and calling StartInformer alone will cause issues.

Using a callback means the caller can not have access to start informers out of order.

n3wscott · 2020-10-06T16:34:22Z

I have updated #1757 with this idea, which will keep API compatibility, could you please see if that can be an alternative solution too?

How do you like this way?

n3wscott · 2020-10-06T16:34:39Z

/retest

zhongduo · 2020-10-06T16:32:00Z

injection/sharedmain/main.go

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


Do we really need this go function? I think it is safer to let it finishes.

This will be deleted asap after knative rolls to the new one, shared main now calls down to the new version. this method is not changed and is legacy.

My original question is about using go func or not in the old function, but I guess the whole function will be removed anyway.

zhongduo · 2020-10-06T16:33:30Z

injection/injection.go

+// EnableInjectionOrDie enables Knative Injection and starts the informers.
+// Both Context and Config are optional. Returns context with rest config set
+// and a function to start the informers after watches have been set up.
+func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) (context.Context, func()) {


This makes sense to me, though the name is a little bit confusing as the original EnableInjectionOrDie is exported too.

yeah, that is intended, the original one is in a different package and marked deprecated.

zhongduo · 2020-10-06T16:35:46Z

leaderelection/chaosduck/main.go

@@ -116,8 +117,7 @@ func quack(ctx context.Context, kc kubernetes.Interface, component string, leade
 }

 func main() {
-	ctx := signals.NewContext()
-	ctx = sharedmain.EnableInjectionOrDie(ctx, nil)
+	ctx, _ := injection.EnableInjectionOrDie(signals.NewContext(), nil)


If we have to do this, that means we are really trying to avoid calling the original function. I would recommend to have a roadmap to deprecate the original function.

It is marked deprecated and documented redirected to the new one version.

That's awesome. Is this on purpose not to call the start function here? Note that the original logic will have informers started here.

I think this might skip your attention. Otherwise LGTM.

I checked and the duck does not use informers so it is not needed.

Thanks for checking.

zhongduo · 2020-10-06T17:18:51Z

injection/injection.go

+	"k8s.io/client-go/rest"
+)
+
+// EnableInjectionOrDie enables Knative Injection and starts the informers.


Remove starts the informers.

zhongduo · 2020-10-06T17:20:30Z

injection/sharedmain/main.go

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


My original question is about using go func or not in the old function, but I guess the whole function will be removed anyway.

zhongduo · 2020-10-06T17:23:53Z

Hi @n3wscott,
I think this PR needs some discussion. I understand you are trying to avoid changing the function signature to maintain compatibility, but I believe API compatibility also includes underlying logic too and this PR changes the logic considerably IMO. If I understand correctly, with this PR:

For users other than shared_main, the informer start with be delayed for 5 seconds.

For shared_main, it chooses to start the informer after 5 seconds or after controller setup, whichever comes first.

For 1, though I know the timeout is configurable, I do believe most users won't notice this change until they encounter a problem. I am afraid that we are making the issue harder to reproduce to most users and the magic 5 seconds can cause some unexpected problems. Eg, in some case if the controller setup takes about 5 seconds for whatever reason, they will experience some flakiness depending on the actual runtime > 5s or < 5s. Another possible problem is that some logic might be assuming informer start at that time, and this delay start will be quite difficult to debug. So to me it is not much different from the original logic, which starts the informer immediately.
For 2 or any other users that are aware of this problem and decide to start it manually, they can easily just use existing code without this PR. If we want to keep API and at the same time avoid duplicate code, here is what I would suggest:
a) Break the existing EnableInjectionOrDie to two parts: BeforeStartInformer and StartInfomer
b) Rewrite EnableInjectionOrDie to BeforeStartInformer + go StartInformer, which is same logic as the original EnableInjectionOrDie.
c) For shared_main or any users that want to start manually, we can do: BeforeStartInformer + do sth + StartInformer
Thanks,
Jimmy

Thanks for the thoughtful response. I agree that the timeout was strange. I have moved the EnableInjectionOrDie, and changed the signature, it now returns back a callback the caller should call if it wants to start the informers. It is basically BeforeStartInformer + do sth + StartInformer except wrapped up in one call with a callback. I think the naming of BeforeStartInformer and StartInformer is strange and calling StartInformer alone will cause issues.

Using a callback means the caller can not have access to start informers out of order.

The naming is just for illustration purpose only. :)

n3wscott · 2020-10-06T18:00:12Z

/retest

n3wscott · 2020-10-06T18:02:38Z

flakes: #1779

n3wscott · 2020-10-06T18:10:14Z

g2g?

/cc @mattmoor

n3wscott · 2020-10-06T18:10:59Z

/retest

n3wscott · 2020-10-06T18:24:14Z

/retest

injection/config.go

mattmoor · 2020-10-06T20:10:19Z

injection/config.go

+//   2. Fallback to the KUBECONFIG environment variable.
+//   3. Fallback to in-cluster config.
+//   4. Fallback to the ~/.kube/config.
+func GetRestConfig(serverURL, kubeconfig string) (*rest.Config, error) {


ick, how about GetKubeconfig

it is getting a rest.Config...

injection/config.go

mattmoor · 2020-10-06T23:18:35Z

injection/sharedmain/main.go

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

 // EnableInjectionOrDie enables Knative Injection and starts the informers.
 // Both Context and Config are optional.
+// Deprecated: use injection.EnableInjectionOrDie


Should we change this method to:

ctx, si := injection.EnableInjectionOrDie(ctx, cfg) go si() return ctx

yes, good idea!

mattmoor · 2020-10-06T23:19:14Z

injection/sharedmain/main.go

@@ -63,6 +63,7 @@ import (
 //   2. Fallback to the KUBECONFIG environment variable.
 //   3. Fallback to in-cluster config.
 //   4. Fallback to the ~/.kube/config.
+// Deprecated: use injection.GetRESTConfig


Should this call the other method?

mattmoor

/lgtm

mattmoor · 2020-10-06T23:54:59Z

/approve

knative-prow-robot · 2020-10-06T23:55:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, n3wscott

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:

~~OWNERS~~ [mattmoor]

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

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

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 5, 2020

knative-prow-robot requested review from evankanderson and vaikas October 5, 2020 17:22

vagababov reviewed Oct 5, 2020

View reviewed changes

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 6, 2020

mattmoor reviewed Oct 6, 2020

View reviewed changes

Scott Nichols added 3 commits October 6, 2020 09:15

put EnableInjectionOrDie back on the main path

d9ea126

nil check pointer type

e9ae4cc

move enable injeciton out of sharedmain

c0b722b

n3wscott force-pushed the no-globals branch from 7dfb20f to c0b722b Compare October 6, 2020 16:15

mattmoor reviewed Oct 6, 2020

View reviewed changes

n3wscott mentioned this pull request Oct 6, 2020

Delay informer start after controller. #1757

Closed

lint

54e14de

nit picking fmt....

67f9161

zhongduo reviewed Oct 6, 2020

View reviewed changes

add documentation

a8fb062

n3wscott mentioned this pull request Oct 6, 2020

Allow override QPS and Burst via flag #1778

Merged

n3wscott closed this Oct 6, 2020

n3wscott reopened this Oct 6, 2020

knative-prow-robot requested a review from mattmoor October 6, 2020 18:10

mattmoor reviewed Oct 6, 2020

View reviewed changes

injection/config.go Outdated Show resolved Hide resolved

mattmoor reviewed Oct 6, 2020

View reviewed changes

injection/config.go Outdated Show resolved Hide resolved

Scott Nichols added 2 commits October 6, 2020 13:25

feedback cleanup

6fb2c29

injection.GetRESTConfig

24f3a3d

mattmoor reviewed Oct 6, 2020

View reviewed changes

redirect code that moved:

ff42410

n3wscott force-pushed the no-globals branch from 574d024 to ff42410 Compare October 6, 2020 23:46

mattmoor reviewed Oct 6, 2020

View reviewed changes

knative-prow-robot assigned mattmoor Oct 6, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2020

knative-prow-robot merged commit 46761ba into knative:master Oct 6, 2020

Put EnableInjectionOrDie back on the main path #1772

Put EnableInjectionOrDie back on the main path #1772

Conversation

n3wscott commented Oct 5, 2020

codecov bot commented Oct 5, 2020 • edited Loading

Codecov Report

n3wscott commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongduo commented Oct 6, 2020 • edited Loading

zhongduo commented Oct 6, 2020

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

n3wscott commented Oct 6, 2020

n3wscott commented Oct 6, 2020

n3wscott commented Oct 6, 2020

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

zhongduo commented Oct 6, 2020

n3wscott commented Oct 6, 2020

n3wscott commented Oct 6, 2020

n3wscott commented Oct 6, 2020

n3wscott commented Oct 6, 2020

n3wscott commented Oct 6, 2020

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

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor commented Oct 6, 2020

knative-prow-robot commented Oct 6, 2020

codecov bot commented Oct 5, 2020 •

edited

Loading

zhongduo commented Oct 6, 2020 •

edited

Loading