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

Jump to bottom

Allows utilities easy mode for Knative Injection #1658

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Allows utilities easy mode for Knative Injection #1658

Show file tree
Hide file tree
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
41 changes: 35 additions & 6 deletions injection/sharedmain/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,39 @@ 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 {
if ctx == nil {
ctx = signals.NewContext()
}
if cfg == nil {
cfg = ParseAndGetConfigOrDie()
}

// 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)

// Start the injection clients and informers.
logging.FromContext(ctx).Info("Starting informers...")
go func(ctx context.Context) {
if err := controller.StartInformers(ctx.Done(), informers...); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change broke reconcilers that set non-default resyncPeriod values because it starts informers before the controllers are set up. Attempting to set the resyncPeriod is ignored and the informer code logs this warning message:

resyncPeriod 300000000000 is smaller than resyncCheckPeriod 36000000000000 and the informer has already started. Changing it to 36000000000000

Copy link
Member

Choose a reason for hiding this comment

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

I think that this is just generally broken. Without the controllers created the informer events won't be set up and the controllers won't properly resync things on startup. This is likely masked by the resync we get when we are promoted to leader, but generally informers should not be started until we're done setting up events.

@n3wscott @vagababov What was the motivation for this change? What will rolling it back in the release branch break?

Copy link
Member

Choose a reason for hiding this comment

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

@grantr we should probably make sure that we have test coverage of the scenario where you folks got broken so that we don't make a similar regression in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anyone working on fixing this or should I try?
@grantr

logging.FromContext(ctx).Fatalw("Failed to start informers", zap.Error(err))
}
<-ctx.Done()
}(ctx)

return ctx
}

// Main runs the generic main flow with a new context.
// If any of the contructed controllers are AdmissionControllers or Conversion webhooks,
// then a webhook is started to serve them.
Expand Down Expand Up @@ -190,9 +223,8 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto
if cfg.Burst == 0 {
cfg.Burst = len(ctors) * rest.DefaultBurst
}
ctx = injection.WithConfig(ctx, cfg)

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

logger, atomicLevel := SetupLoggerOrDie(ctx, component)
defer flush(logger)
Expand Down Expand Up @@ -246,10 +278,7 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto
})
}

logger.Info("Starting informers...")
if err := controller.StartInformers(ctx.Done(), informers...); err != nil {
logger.Fatalw("Failed to start informers", zap.Error(err))
}
// Wait for webhook informers to sync.
if wh != nil {
wh.InformersHaveSynced()
}
Expand Down
10 changes: 1 addition & 9 deletions leaderelection/chaosduck/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,8 @@ import (
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/kubernetes"
kubeclient "knative.dev/pkg/client/injection/kube/client"
"knative.dev/pkg/controller"
"knative.dev/pkg/injection"
"knative.dev/pkg/injection/sharedmain"
"knative.dev/pkg/kflag"
"knative.dev/pkg/signals"
"knative.dev/pkg/system"
)

Expand Down Expand Up @@ -118,13 +115,8 @@ func quack(ctx context.Context, kc kubernetes.Interface, component string, leade
}

func main() {
ctx := signals.NewContext()
ctx := sharedmain.EnableInjectionOrDie(nil, nil)

// We don't expect informers to be set up, but we do expect the client to get attached to ctx.
ctx, informers := injection.Default.SetupInformers(ctx, sharedmain.ParseAndGetConfigOrDie())
if err := controller.StartInformers(ctx.Done(), informers...); err != nil {
log.Fatalf("Failed to start informers %v", err)
}
kc := kubeclient.Get(ctx)

// Until we are shutdown, build up an index of components and kill
Expand Down