Simplify the informer resync logic

[shawkins]

In reviewing the informer resync logic it seems a little complicated and it's also imprecise.

For example try starting an informer with a resync of 1 second and adding a handler with the default resync and another with a value of 1.1 seconds. You'll notice that the latter handler is called much less often than expected. That's because the logic is driven by the shortest interval, rather than by a least common denominator. Unless you only add handlers with resyncs shorter or longer than the lowest interval by an even multiple you won't actual get the behavior you expect.

Also if the informer is already started, then you cannot successfully add a handler with a smaller interval than what is already in use.

As an alternative I propose the following behavior - let there be only one resync interval, which is the one defined when the informer is created. Using addEventHandlerWithResyncPeriod should be deprecated and the meaning changed to allow a 0 value to indicate resync is disabled for a given handler. The replacement method should be addEventHandlerWithoutResync.

With that change almost all of the resync code will go away and have a much precise behavior.

cc @rohanKanojia @manusa

[manusa]

That's because the logic is driven by the shortest interval, rather than by a least common denominator

I'm a little confused by this statement. I'll try to figure out on your PR.

[shawkins]

I'm a little confused by this statement. I'll try to figure out on your PR.

Suppose you have a resync of 1 second for handler 1 and and 1.2 seconds for handler 2. The resync interval will be the min value of 1 second, and at each interval the resync runner will check which handlers are syncing for that period. At the first interval (1 second) handler 1 will report that it is syncing, while handler 2 will report that it is not (we have not reached 1.2 seconds). At the second interval (2 seconds) both handlers will report as syncing, but handler 2 will compute the next time as (2 + 1.2) seconds. At the third interval (3 seconds) handler 1 will report syncing, while handler 2 will not - and so on and so forth.
This means that handler 2 will actually run at approximately half the rate of handler 1.

I'm proposing to just get rid of all of the temporal reasoning, so that we can just schedule a simple job.

[shawkins]

Here's my answer from the go client source: https://github.com/kubernetes/client-go/blob/da281643cfad72bc8c4565b197ec23b557e30446/tools/cache/shared_informer.go#L203 - this is the expected behavior.

You have to know the relationship between the informer resyncPeriodInMillis, addEventHandler (with and without resync period) and the notion of period rounding, whether already started, or that the min is 1 second. It is not really clear why things are this way without reading the go source.

I converted my repos pr into #3165 to aid in this discussion. I'll understand if a breaking change does not seem warranted.

[shawkins]

I went ahead and closed that pr. It's probably not worth changes in this regard.