Controller: Per-key increasing backoff

[alvaroaleman]

Would you like to work on this feature?

None

What problem are you trying to solve?

Hello,

Trying to leverage kube-rs to builder a Kubernetes controller, I noticed that there doesn't really seem to be a way to use per-key increasing backoff for reconciliation failures. Using such a backoff strategy (usually exponentially) is done in all go Kubernetes controllers that I've seen, both in-tree and out of tree. It ensures both that transient errors are retried quickly as well as avoids overloading external components by reducing the retry frequency if errors do not go away on retry.

The way this is handled in kube-rs is through the error_policy which gets called on errors but not on successes. Any sort of per-key increasing backoff is stateful in the sense that it needs to know how many failures for the given key occured previously and clear that if a key was reconciled successfully. Due to the error policy not being notified on successes, there is no straight forward way to achieve this from what I can tell. In theory it would probably be possible to implement this through an error_policy that wraps the reconciler to get this information but that seems overly complex to me.

Do you have ideas/thoughts/opinions on how to best achieve this and what is the compatibility policy of kube-rs? In the go ecosystem, all of this is done through a configurable ratelimiter that is part of a configurable workqueue.

Describe the solution you'd like

Something that just works out of the box :)

Describe alternatives you've considered

/

Documentation, Adoption, Migration Strategy

No response

Target crate for feature

No response

[nightkr]

Yeah.. I thought we already had an issue for this, but I can't seem to find it now.. I've been meaning for ages to either:

1. Redefine error_policy as a trait with a reset(&mut self, ObjectRef<K>) method (this could be non-breaking, since we can add a blanket impl that covers the current interface and no-ops on reset)
2. Redefine error_policy to take Result<Action, &Error> instead of &Error (this would be breaking, but relatively easy to fix by adding let err = match res { Err(err) => err, Ok(action) => return action, };..)
   • That said, it's also kind of surprising that error_policy has the ability to rewrite the specific Action returned by reconciler...
3. (Extending 2 a bit...) replacing the Action returned by reconciler with a generic user-defined type, ensuring that error_policy is fully in control of user-requested re-reconciles (this wouldn't necessarily change much in practice, since that user-defined type could still be Action, and then we're back to option 2)
4. Killing off error_policy entirely (you could just wrap reconciler yourself)

4 is appealing (smaller interfaces, yay!), but I'm mostly leaning towards option 1 since that'd also let you remove leftover state when objects are deleted from k8s. Otherwise, that'd still end up leaking memory unless you have some other expiration mechanism.

[nightkr]

(And to be clear, #1916 (comment) is meant as an RFC/commentary on what I'd lean towards merging, not "I'm reserving this issue now".)

[alvaroaleman]

For my understanding, how would it be possible to do 1) in a non-breaking way? It is a mandatory arg to run so any change to its signature will break users or not?

Also, from an UX perspective its odd that this must be configured. The error_policy/ratelimiter is something where a default will be the right thing for 98% of cases, so making it optional by for example adding an with_error_policy to the controller would simplify things IMHO

[nightkr]

For my understanding, how would it be possible to do 1) in a non-breaking way? It is a mandatory arg to run so any change to its signature will break users or not?

Currently, we take (simplified):

fn applier<Error>(
    // ... other args
    error_policy: impl FnMut(Error) -> Action,
);

The naíve way to replace it with a trait would indeed be breaking:

trait ErrorPolicy<Error> {
    fn on_error(&mut self, error: Error) -> Action;
}

fn applier<Error>(
    // ... other args
    error_policy: impl ErrorPolicy<Error>,
);

But, since we own the trait ourselves, we can add a blanket implementation, essentially saying "anything that satisfies this condition also satisfies our trait":

impl<Error, Policy> ErrorPolicy<Error> for Policy
where Policy: FnMut(Error) -> Action {
    fn on_error(&mut self, error: Error) -> Action {
        self(error)
    }
}

That way, any existing arguments using the old lambda (or function reference) API would still satisfy the new condition.

Anyone who takes the type would need to adapt, but that's just us. Any library that proxies the API (like Controller) would also still be able to use the old (stricter) type contract until they're ready to migrate.

We use a similar trick for wait::Condition, where we have our own combinator syntax on top of what's basically just lambdas.

Also, from an UX perspective its odd that this must be configured. The error_policy/ratelimiter is something where a default will be the right thing for 98% of cases, so making it optional by for example adding an with_error_policy to the controller would simplify things IMHO

Agreed. Once we have the rest in, it'd be good to provide at least some good starting constants. Ideally, we'd have a PerObjectBackoff<B: Backoff>, with a reasonable default variant (like https://docs.rs/kube/latest/kube/runtime/watcher/struct.DefaultBackoff.html for watchers).

I'm not sure about making it completely optional (there's also a bit of a type trickery tradeoff; historically we've been reluctant to add more generics to Controller), but ideally you should at least never have to write your own error_policy unless you really want to.