PR #1658 is preventing resync period of informer to be overwritten

[zhongduo]

/kind bug

PR #1658 changes the order of informer setup and controller setup, however the resync period of informer can only be overwritten before the informer is started. As a result, all the event handler with customized resync period will have the default of 10 hours resync period after this PR.

Expected Behavior

Event handlers in AddEventHandlerWithResyncPeriod with resync period set to 5 mins should get called every 5 mins.

Actual Behavior

The event handlers above will only get called every 10 hours or whatever the k8s default has.

Steps to Reproduce the Problem

In any controller code using Informer().AddEventHandlerWithResyncPeriod add resync period of 5 mins, following error will be found when the controller is started.

W0922 21:37:44.196866 1 shared_informer.go:461] resyncPeriod 300000000000 is smaller than resyncCheckPeriod 36000000000000 and the informer has already started. Changing it to 36000000000000

Additional Info

[n3wscott]

The order of controller setup and informer setup has not changed:

It was:

	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()
	}
	logger.Info("Starting controllers...")
	go controller.StartAll(ctx, controllers...)

Additionally, go controller.StartAll(ctx, controllers...) is non-deterministic in ordering.

[zhongduo]

The order of controller setup and informer setup has not changed:

It was:

	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()
	}
	logger.Info("Starting controllers...")
	go controller.StartAll(ctx, controllers...)

Additionally, go controller.StartAll(ctx, controllers...) is non-deterministic in ordering.

You are right, it is the setup of controller and start of informers. We might just need a way to call StartInformers after ControllersAndWebhooksFromCtors. I didn't do the experiment of the exact location, but I think to fix it we just need an option to delay starting informers somehow.

[n3wscott]

@mattmoor had some insight about calling start informers before the controllers get a chance to start, not clear I changed that. Might have been a bug before and still?

[zhongduo]

@mattmoor had some insight about calling start informers before the controllers get a chance to start, not clear I changed that. Might have been a bug before and still?

I think that's probably a different problem. Here the problem is that during controller setup, the controllers will add event handlers to informers with resync period set, which will be ignored now because the resync period cannot be changed. So the problem in this issue is "start of informer" vs "setup of controller". Your previous PR moved the "start of informer" before "setup of controller".

[mattmoor]

@n3wscott We used to call StartInformers below this line, now it is in a go routine above it: https://github.com/knative/pkg/pull/1658/files#diff-7a2878cf5c6550229d8ac30e0d5e74e2R250

[mattmoor]

I see @zhongduo pointed that out above 🤦

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[mattmoor]

I believe this was fixed.

/assign @n3wscott

[zhongduo]

Yes, it is already fixed.