Support global timeouts for reconcilers

[ekuefler]

I ran into an issue today where one of my reconcilers was unexpectedly deadlocking on a particular resource. This appeared to halt on processing of that reconciler for other resources, essentially freezing the system until I restarted the manager binary (where it would eventually freeze again when it got to the bad resource).

It would be nice if I could configure a timeout that would apply to all reconcilers in the manager such that they would automatically abort and back off it they appear to be deadlocked.

[vincepri]

The idea of a global timeout is definitely interesting and we can probably make it opt-in. I've seen these kinds of situations when the resources weren't enough, most common is deployments with less than 2-3 CPUs allocated.

[ekuefler]

Sounds good. My particular situation wasn't related to starving the CPU. I have a resource representing an external server that I need to do TCP health checks on. I'd failed to call SetDeadline on my connection, so when I got a server that responded to the connect request but failed to write back any data, the reconciler would block indefinitely on that resource and lock out processing for all the others. A global timeout would be a useful safety net here.

[vincepri]

/priority important-soon
/kind design
/help

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/priority important-soon
/kind design
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[GrigoriyMikhalkin]

they would automatically abort and back off it they appear to be deadlocked

I'm little bit confused, what this issue is about. You want some timeout mechanism that would terminate reconciler automatically(i.e. without user handling this termination in reconciliation logic)?

[ekuefler]

That's right. Many web frameworks have a built-in timeout that will terminate a request if it takes too long, ensuring request threads aren't occupied indefinitely. I would imagine something similar here. In kubebuilder's case, it would force the reconciler to return an error.

Here's the wrapper I wrote and am using in my project to do this (it also handles panics per #797):

func MakeSafe(r reconcile.Reconciler) reconcile.Reconciler {
	return safeReconciler{impl: r}
}

type safeReconciler struct {
	impl reconcile.Reconciler
}

func (r safeReconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) {
	type response struct {
		result reconcile.Result
		err    error
	}

	ch := make(chan response)
	go func() {
		defer func() {
			if p := recover(); p != nil {
				ch <- response{
					err: fmt.Errorf("panic: %v [recovered]\n\n%s", p, debug.Stack()),
				}
			}
		}()

		r, e := r.impl.Reconcile(request)
		ch <- response{
			result: r,
			err:    e,
		}
	}()

	select {
	case r := <-ch:
		return r.result, r.err
	case <-time.After(10 * time.Second):
		return reconcile.Result{}, fmt.Errorf("reconciler timed out")
	}
}

This is imperfect since it means we'll leak a goroutine if the wrapped reconciler deadlocks, but it will at least guarantee that kubebuilder's worker goroutines can't deadlock.

[GrigoriyMikhalkin]

IMO, it looks like solution to conceal some bugs in program, instead of fixing them.

[alvaroaleman]

It is not possible to terminate a goroutine from the outside, so I don't know how this could be implemented (leaking goroutines is IMHO not an option we should consider, it also means that we break the guarantee of "Only one routine will work at a given key at a given time").

Or did I misunderstand something about this?