Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delay informer start after controller. #1757

Closed
wants to merge 1 commit into from
Closed

Delay informer start after controller. #1757

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 18 additions & 33 deletions injection/sharedmain/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,8 @@ func GetLeaderElectionConfig(ctx context.Context) (*leaderelection.Config, error
return leaderelection.NewConfigFromConfigMap(leaderElectionConfigMap)
}

// EnableInjectionOrDie enables Knative Injection and starts the informers.
// Both Context and Config are optional.
func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) context.Context {
// InjectionSetup sets up the context and informers.
func InjectionSetup(ctx context.Context, cfg *rest.Config) (context.Context, []controller.Informer) {
if ctx == nil {
ctx = signals.NewContext()
}
Expand All @@ -146,17 +145,23 @@ func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) context.Context
}
ctx = injection.WithConfig(ctx, cfg)

ctx, informers := injection.Default.SetupInformers(ctx, cfg)
return injection.Default.SetupInformers(ctx, cfg)
}

// StartInformers starts the clients and informers.
func StartInformers(ctx context.Context, informers ...controller.Informer) {
// Start the injection clients and informers.
logging.FromContext(ctx).Info("Starting informers...")
go func(ctx context.Context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@n3wscott n3wscott Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err := controller.StartInformers(ctx.Done(), informers...); err != nil {
logging.FromContext(ctx).Fatalw("Failed to start informers", zap.Error(err))
}
<-ctx.Done()
}(ctx)
if err := controller.StartInformers(ctx.Done(), informers...); err != nil {
logging.FromContext(ctx).Fatalw("Failed to start informers", zap.Error(err))
}
}

// EnableInjectionOrDie enables Knative Injection and starts the informers.
// Both Context and Config are optional.
func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) context.Context {
ctx, informers := InjectionSetup(ctx, cfg)
StartInformers(ctx, informers...)
return ctx
}

Expand Down Expand Up @@ -216,25 +221,7 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto

MemStatsOrDie(ctx)

// Respect user provided settings, but if omitted customize the default behavior.
if cfg.QPS == 0 {
// Adjust our client's rate limits based on the number of controllers we are running.
cfg.QPS = float32(len(ctors)) * rest.DefaultQPS
}
if cfg.Burst == 0 {
cfg.Burst = len(ctors) * rest.DefaultBurst
}

// Respect user provided settings, but if omitted customize the default behavior.
if cfg.QPS == 0 {
cfg.QPS = rest.DefaultQPS
}
if cfg.Burst == 0 {
cfg.Burst = rest.DefaultBurst
}
ctx = injection.WithConfig(ctx, cfg)

ctx, informers := injection.Default.SetupInformers(ctx, cfg)
ctx, informers := InjectionSetup(ctx, cfg)

logger, atomicLevel := SetupLoggerOrDie(ctx, component)
defer flush(logger)
Expand Down Expand Up @@ -288,10 +275,8 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto
})
}
// Start the injection clients and informers.
logging.FromContext(ctx).Info("Starting informers...")
if err := controller.StartInformers(ctx.Done(), informers...); err != nil {
logging.FromContext(ctx).Fatalw("Failed to start informers", zap.Error(err))
}
StartInformers(ctx, informers...)

// Wait for webhook informers to sync.
if wh != nil {
wh.InformersHaveSynced()
Expand Down