Add backoff to watcher reconnection

[nightkr]

Currently watcher will always keep a reconnection attempt in-flight when polled (and there is no active connection already). That's a bit excessive, and pretty unkind to the apiserver.

This isn't too bad for watcher itself since you can add a delay on the consumer side instead, but we should at least add something for Controller...

[nightkr]

https://canary.discord.com/channels/500028886025895936/685161813675212899/861276713295675424

[clux]

Pasting comments here for people off discord:

pavelp: Out of pure curiosity, I noticed kube calls K8S API with high frequency when it's not available (e.g. local cluster down). Is this the usual frequency excercised by kuibe-rs to call REST API watches ? Is there any recommended ? I was fiddling with K8S API this weekend and did some throttling - I only ask twice a second maximum.
https://kubernetes.io/docs/reference/using-api/api-concepts/#efficient-detection-of-changes

teozkr: that said.. there are still some missing pieces (for example: exponential backoff, backoff for kube watchers, and so on)

Do we think the solution here is to try to bake an exponential backoff watcher or the the step_trampolined (inner state machinery of watcher) fn?

(btw one way I have gotten around on this in the controller-rs example is to try to get the CRD first before trying to watch it - as that causes the same error - but that only works for controllers who forgot to add the CRD)

[nightkr]

I think it would make sense to just have watcher (and applier/Controller in turn) take an arbitrary Backoff from the user.

[clux]

If that's the case then I think we should consider creating builders for watcher to pass it in (#276), it's already a bit strange to have to disambiguate the import paths runtime::watcher (module) vs runtime::watcher() (fn) or runtime::watcher::watcher (fn)

[clux]

rewording a bit; the watcher import problem is a separate issue, could be fixed by renaming the watcher module to watch,.

I do still think there's room for a Watcher<K>, there are a couple of other naming problems here that i would like to improve separately (module name + flatten helpers/naming (maybe something akin to #296)). I'll open an issue separately.

[nightkr]

To be honest, I don't really see the problem with the import, Rust has been smart enough to do the right thing in all the cases I've tried.

[clux]

Rustdoc needs explicit disambiguation when linking to either module or the fn.
And although that's more of a problem for us, people still have to do a double-take here on the name being re-used in imports:

Generally i just think we should organise it in the way that is less confusing:

-use runtime::{reflector, reflector::Store};
+use runtime::cache::{reflector, Store};

relatively minor point though.

[nightkr]

That makes sense to me.

[clux]

Coming back to this after thinking more about it in #698

If we put a backoff into watcher, propagated it into step and slowed down progression on errors, then this would fix this particular issue (being kind to the api server when it's having trouble), but there are other issues that are hard errors here which cannot be fixed by retrying:

if RBAC is wrong, then no amount of retrying should occur - we need to bubble this up from the watcher.

I can't think of any other non-ephemeral issues that can cause a watcher retry loop to happen, but if there are we should also catch them.

So, what we should probably try to do:

• catch the RBAC permission issue (that will cause a retry loop within watcher)
• have watcher take a mandatory backoff that controls non-critical error retries
• create a builder for watcher that creates a sensible default backoff builders for kube-runtime reflector #276
• document the error cases that are actually bubbled up from watcher to the user

The last point here because most users thing watcher/reflector always returns true and just uses ? without thinking (and i've personally never seen it crash the app). So if we suddenly add a common case here, we should document it.

[nightkr]

I can't think of any other non-ephemeral issues that can cause a watcher retry loop to happen, but if there are we should also catch them.

I'm not so sure that we can make a hard distinction here. For example:

• RBAC errors may be caused by misconfiguration (permanent) or a redeploy that reorganizes RBAC roles but ultimately gives the same permissions in the end (ephemeral)
• 404 errors may be caused by a K8s version mismatch or missing CRD (permanent) or a new CRD that hasn't been picked up by the API server yet (ephemeral)
• Connection errors may be caused by firewall misconfiguration (permanent) or the load balancer not having picked up on the new cluster state yet (ephemeral)

And so on, and so forth. Ultimately, we need to percolate the problem up to the administrator and let them handle the problem.

And even if it is a human-caused misconfiguration error, it'd still be useful to retry since we probably won't get notified once the issue is resolved.

[clux]

True. These configuration errors can happen, and it's nice if we can just keep retrying through it. But it's also important for us to provide that first signal that something has gone wrong upstream (like in the load balancer policy example - if we silently try to retry for that, then we'll never get correct percolation).

I checked watcher with some custom rbac to limit my watch and list permissions inside to verify and the following errors are not retried and are bubbled up to the stream consumer:

• ErrorResponse 403 (missing/insufficient rbac)
• ErrorResponse 404 (missing/invalid api kind)

So these would be up to users to handle atm.

Pre-backoff; I think this is good. If the user want's to be resilient towards upstream configuration errors (rather than crash and slowly go into CrashLoopBackOff), then they should be explicit about that and percolate signals in other ways (most clusters have notification systems when pods go into crashloop after all so it's a pretty safe default).

But this means that means the RBAC retry we get from configmapgen_controller (if we delete the configmapgenerators.nullable.se crd) is because the controller discards the watcher's errors. So we are currently inconsistent in our strategy.

If we inject backoffs into watcher then it probably also needs some sort of default threshold for how long the watcher (and ultimately the using Controller) is allowed to exist in a non-functioning state, because otherwise we run the risk of the default behaviour not percolating signals upstream at all.