KEP-4460: Enable per-request Read/Write deadline

[tkashem]

• One-line PR description: adding new KEP

• Issue link: Enable per-request Read/Write Deadline #4460

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[tkashem]

/cc @sttts

[benluddy]

How similar is the Kubernetes timeout filter to https://pkg.go.dev/net/http#TimeoutHandler? Are there examples of attempts outside of Kubernetes to address similar concerns?

[benluddy]

Is handling these errors more expensive than 504 timeouts? At least for HTTP1, I guess the client will typically connect() again (and perform another TLS handshake, etc.)?

I'm curious about pathological situations that could be systemically worsened if instead of receiving a 504 and retrying on an existing socket, clients instead need to reconnect.

[tkashem]

for HTTP/1x, the connection will be closed, so the client needs to reconnect. For http/2.0, the stream will be reset, and the tcp connection will still be intact.
I will add a test to document this behavior.

[enj]

Something to consider: we could wrap context.Value of the request's context to panic if the request has been canceled. Functions like RequestInfoFrom constantly fetch values off the context, so it can serve as a good preemption point in addition to the response writer.

[tkashem]

that's a good idea, we could wrap the context with

type PanicAfterTimeoutContext struct {
	context.Context
}

func (c PanicAfterTimeoutContext) Value(key any) any {
	if c.Context.Err() != nil {
		panic(...)
	}
	return c.Context.Value(key)
}

i think it should not have any ripple effect, and any new context derived from the wrapped PanicAfterTimeoutContext (using WithValue, WithCancel etc.) still works as expected, i wrote a quick test to verify this:

type keyType1 int

const key1 keyType1 = iota

type keyType2 int

const key2 keyType2 = iota

type keyType3 int

const key3 keyType3 = iota

type PanicAfterTimeoutContext struct {
	context.Context
}

func (c PanicAfterTimeoutContext) Value(key any) any {
	if c.Context.Err() != nil {
		panic("timeout occurred")
	}
	return c.Context.Value(key)
}

func TestContext(t *testing.T) {
	parent, cancel1 := context.WithCancel(context.Background())
	defer cancel1()
	parent = context.WithValue(parent, key1, "value1")

	child, cancel2 := context.WithTimeout(parent, time.Hour)
	child = context.WithValue(child, key2, "value2")

	wrapped := &PanicAfterTimeoutContext{Context: child}
	// derive a context from wrapped
	ctx, cancel := context.WithCancel(wrapped)
	defer cancel()
	ctx = context.WithValue(ctx, key3, "value3")

	if got := ctx.Value(key1).(string); got != "value1" {
		t.Errorf("expected value: %q, got: %q", "value1", got)
	}
	if got := ctx.Value(key2).(string); got != "value2" {
		t.Errorf("expected value: %q, got: %q", "value2", got)
	}
	if got := ctx.Value(key3).(string); got != "value3" {
		t.Errorf("expected value: %q, got: %q", "value3", got)
	}

	cancel2()
	func() {
		defer func() {
			r := recover()
			if r == nil {
				t.Errorf("expected a panic")
			}
			t.Logf("panic: %v", r)
		}()
		ctx.Value(key1)
	}()
}

[enj]

Unfortunately, when I experimented with this approach in the past, I discovered that the context package would call the Value method under certain circumstances from other go routines, so a panic in there would kill the whole process. The way I mitigated that was checking that the current caller was in a http handler stack before panic-ing. There might be other approaches as well.

[tkashem]

On he other hand, if we want to do post-timeout logging/reporting metric, we would need to access the request scoped attributes using the context, and the panic from Value is not desirable there.

[enj]

I think you could always special case whatever case you needed to do preserve.

Probably it would be best to experiment and measure how much benefit we see in terms of resource usage by pre-empting earlier via the Value method vs just the request writer. Then you can determine if it is worth the effort to implement.

[enj]

A lot of my initial reservation on this KEP was because I thought it was going to use net/http.TimeoutHandler which is strictly worse than our handler. Setting deadlines will close the http2 stream, which does cover "half" of the resource usage (basically it does not cover the handler logic which runs in its own go routine).

[tkashem]

Thanks for the clarification, yes, we are not going to use net/http.TimeoutHandler.

Setting deadlines will close the http2 stream, which does cover "half" of the resource usage (basically it does not cover the handler logic which runs in its own go routine)

today the other half (the inner handler running in its own goroutine) is not directly managed by the timeout filter since there is no way to kill a goroutine externally. The timeout handler delegates the Write method of the ResponseWriter object:

https://github.com/kubernetes/kubernetes/blob/57b406a18afc54c84725488e0ca3d4b4cabd61db/staging/src/k8s.io/apiserver/pkg/server/filters/timeout.go#L200-L202

So when the inner handler writes to the ResponseWriter object after timeout occurs, it returns an http.ErrHandlerTimeout error, this provides a way for the inner handler to return and allow the goroutine to finish.
The KEP proposes a similar approach, either return an error or panic from the Write method after the timeout occurs

Note: This PR kubernetes/kubernetes#124730 documents the request timeout behavior with the timeout handler as it is today.

[enj]

I am struggling to parse what this is saying.

[tkashem]

today, our timeout handler does not handle long-running requests including WATCH, it only handles the short (non long-running) requests. The scope of this proposal is the same, short (non long-running) requests only.

An orthogonal effort to allocate a deadline to the Context of WATCH request(s) is a prerequisite, and such effort is outside the scope of this proposal.

today for any WATCH request, if we inspect the context of the request it would not have any deadline. This work in progress PR kubernetes/kubernetes#122976 makes an attempt to wire context with a proper deadline to a WATCH request. This effort is orthogonal and is outside the scope of this KEP.

[enj]

Is there some way for us to mimic the http2 behavior in http1 without using the timeout handler that we have today?

[tkashem]

when we don't have the timeout handler the entire handler chain executes in a single goroutine, and there is no mechanism to preempt a goroutine, i think, a reasonable option is to panic from within the Write and Flush method of the ResponseWriter object.

I will investigate if there is a way to achieve this without using the timeout handler, I opened an issue to track this: golang/go#65526

[deads2k]

test plan looks thorough, the expected equivalence looks good. @enj has an outstanding question on it. With a featuregate, I think we can try removing our special cased timeout handler. I won't have time to personally review implementation.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, tkashem

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS [deads2k]
• keps/sig-api-machinery/OWNERS [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[enj]

/lgtm

[wojtek-t]

For posterity - it merged before my PRR review, but this looks fine for Alpha.