High default `config.read_timeout` delays client recovery

[clux]

Current and expected behavior

Under network errors, using our default config.read_timeout (which we explicitly set using our defaults) can make certain error cases slow to recover due to them having to wait the full long-poll recovery period before erroring out. This full 290s time was never intended as recovery period for normal GET requests and confuses users + causes suboptimal recovery behavior.

Examples of confused users (closed those in favor of this):

• Network error takes too much time to recover #1796
• watcher missed events on VPN disconnect #1791

Maybe there's something better we can do with timeouts that doesn't by default, propagate this timeout to all calls.

In particular we do populate default watch parameters to 290s separately, even though this is by default unset.

Possible solution

Maybe we can rely on the server side default timeout in WatchParams::default and maybe we can shift our config.read_timeout to something that does not apply to watches.

This would be confusing unless we also changed the name though (because we cannot change the functionality this radically while not making the user think about the explicit choice they have made).

Affected crates

kube-client, kube-core

Would you like to work on fixing this bug?

maybe

[clux]

...although, the parameter in kube-core is actually only for the Kubernetes server wide timeout, which is different from the client-side timeout that hyper provides.

[paologallinaharbur]

Thanks for unifying the two issues!

What I find a bit confusing now (but I could be wrong) is that there are two timeouts with defaults applied to "watches" calls:

• one is config.read_timeout being applied to the client itself
• and the other WatchParams.timeout being passed as config URL.

Therefore, you could assume to modify one of the two, however this action could lead to issues if only one of the two is modified, moreover it is not immediate to understand the difference 🤔

---

Checking what the go client is doing, the approach is slightly different. The client has no default timeout, while the watcher has a random one that AFAIK is not directly configurable:

	timeoutSeconds := int64(minWatchTimeout.Seconds() * (rand.Float64() + 1.0))
	options := metav1.ListOptions{
		ResourceVersion: r.LastSyncResourceVersion(),
		// We want to avoid situations of hanging watchers. Stop any watchers that do not
		// receive any events within the timeout window.
		TimeoutSeconds: &timeoutSeconds,

If I am not mistaken, the watch timeout is used for the request and passed via URL, but not for the client:

func (c *nodes) Watch(ctx context.Context, opts metav1.ListOptions) (watch.Interface, error) {
	var timeout time.Duration
	if opts.TimeoutSeconds != nil {
		timeout = time.Duration(*opts.TimeoutSeconds) * time.Second
	}
	opts.Watch = true
	return c.client.Get().
		Resource("nodes").
		VersionedParams(&opts, scheme.ParameterCodec).
		Timeout(timeout).
		Watch(ctx)
}

Notice that the client by default has no timeout, while the watcher has 481:

The approach overall seems similar, what seems different is the likeness to hang 🤔 (we used for several years the go client without setting a timeout for all calls and never experienced any hangs)

[Ramilito]

Just adding some more context, setting timeout on the client caused problems when using exec and debug where the lowering the client timeout would cause disconnects.
I solved it by creating two different clients, one with a lower timeout and one with the default. Would be a lot simpler to just have a way to set lower Watcher timeout instead.

[clux]

Would be a lot simpler to just have a way to set lower Watcher timeout instead.

If that's the desire, maybe you can try to lower watcher::Config::timeout manually and see if that does the right thing?

[Ramilito]

I tried that but it didn't trigger the same recovery that I'm getting from using the client.read_timeout.