Adding proper timeouts. #10656

krousey · 2015-07-02T00:11:55Z

An attempt to properly fix #9013 and #9180.

krousey · 2015-07-02T00:16:17Z

Ideally we should push these timeouts down to the specific handlers and not have it at the top level.

I also may be unnecessary supporting http.Hijacker and http.CloseNotifier. We may only need those on connections that aren't subject to timeouts any way. To support that, there was a combinatoric explosion of types creates (4 in total).

If we can eventually push timeouts down to individual handlers that need them, and those specific ones don't need close notification or hijacking, this can greatly simplify.

krousey · 2015-07-02T00:19:53Z

cc @dchen1107

k8s-bot · 2015-07-02T00:54:15Z

GCE e2e build/test passed for commit 8fa775c9fe62d070c097159a7c6279b7426417e6.

zmerlynn · 2015-07-02T04:01:25Z

Assigning back to yourself. Assign the oncall when you take "WIP" off.

thockin · 2015-07-04T20:53:27Z

Thanks for digging into this one

bgrant0607 · 2015-07-06T17:07:52Z

Since still WIP, this has missed the 1.0 window. Can go into the next release.

krousey · 2015-07-07T23:25:26Z

I have added tests.

@dchen1107 Do you think some timeout handling should be done in kubelet too?

k8s-bot · 2015-07-08T00:48:12Z

GCE e2e build/test passed for commit 2f788c6bdd809881a1ca7f5ac509d9069cce926d.

k8s-bot · 2015-07-10T22:51:40Z

GCE e2e build/test passed for commit 7e5919f26d964dacc4d699439b550a1efdf485ca.

k8s-bot · 2015-07-10T22:54:21Z

GCE e2e build/test passed for commit 1d033b9.

brendandburns · 2015-07-22T20:07:07Z

pkg/apiserver/handlers.go

+
+type timeoutHandler struct {
+	handler http.Handler
+	timeout func(*http.Request) (<-chan time.Time, string)


Document the expectations of this function. (in particular that returning nil is allowed and cauases it to early terminate.

Actually returning nil will not cause it to terminate early. A nil return will in effect be no timeout. And this is documented in that last sentence of the public function 7 lines above.

brendandburns · 2015-07-22T20:12:24Z

one small nit. otherwise LGTM.

Adding proper timeouts.

liggitt · 2015-08-10T14:12:36Z

pkg/kubelet/server.go

@@ -65,8 +65,6 @@ func ListenAndServeKubeletServer(host HostInterface, address net.IP, port uint,
 	s := &http.Server{
 		Addr:           net.JoinHostPort(address.String(), strconv.FormatUint(uint64(port), 10)),
 		Handler:        &handler,
-		ReadTimeout:    5 * time.Minute,
-		WriteTimeout:   5 * time.Minute,


this drops the timeouts on the kubelet server, but I don't see how the new TimeoutHandler is getting set up for the kubelet, only the API server... am I missing something?

No. It is not set up. I was under the impression that the API calls were proxied through the API server, so they would handle the timeouts.

liggitt · 2015-08-10T21:15:19Z

cmd/kube-apiserver/app/server.go

+	longRunningTimeout := func(req *http.Request) (<-chan time.Time, string) {
+		// TODO unify this with apiserver.MaxInFlightLimit
+		if longRunningRE.MatchString(req.URL.Path) || req.URL.Query().Get("watch") == "true" {
+			return nil, ""


does this make long-running requests of unlimited duration?

As long as the client is still connected.

I would also note that Read and Write timeouts on the server object don't actually close connections. They only cause errors on read and write. It's up to the error handling code to close it in that case. The bugs mentioned in this PR were two that actually held connections open for a long time, but didn't close until there was IO interaction to trigger it.

xiang90 · 2016-07-25T00:14:19Z

@krousey You mentioned that we might have a few advanced use case. None of them actually happened over the last year. Can I assume that we actually do not need them? This implementation is too racy. Can we switch to use something simpler like https://golang.org/pkg/net/http/#TimeoutHandler or a slightly modified version of that?

liggitt · 2016-07-25T02:01:51Z

This implementation is too racy

@xiang90 do you have specific issues you can reference?

Can we switch to use something simpler like https://golang.org/pkg/net/http/#TimeoutHandler

That impl is simpler, but does not actually work with the mix of handlers we have (long running, chunk writing, streaming, etc). From the godoc:

TimeoutHandler buffers all Handler writes to memory and does not support the Hijacker or Flusher interfaces.

I think any timeout handler that does work with those interfaces is likely to be more complex

xiang90 · 2016-07-25T02:13:03Z

@liggitt

do you have specific issues you can reference?

#29001 (comment)
#29001 (comment)

That impl is simpler, but does not actually work with the mix of handlers

No. The timeout handler can wrap any handlers. You can have a timeout handler per handler.

liggitt · 2016-07-25T02:28:21Z

Did you read the comment about TimeoutHandler buffering all writes to memory, and not working with Hijacker interfaces?

xiang90 · 2016-07-25T02:41:41Z

@liggitt

Why you assume I did not?

Can you please read #10656 (comment) and my comment at #10656 (comment)?

I am actually not sure what you are trying to explain. If you know there is a case that we need hijacker or flush, probably you should just post it here. Or if you think the current implement is not racy, you can just tell me why. If you want to fix the race, go ahead and do it. Thanks.

ncdc · 2016-07-25T09:57:29Z

We use hijack for exec, attach/run, and port forwarding currently.
Eventually I'd like to move to an HTTP/2 implementation so hijacking
wouldn't be required any more.

On Sunday, July 24, 2016, Xiang Li notifications@github.com wrote:

@liggitt https://github.com/liggitt

Why you assume I did not?

Can you please read #10656 (comment)
#10656 (comment)
and my comment at #10656 (comment)
#10656 (comment)?

I am actually not sure what you are trying to explain. If you know there
is a case that we need hijacker or flush, probably you should just post it
here. Or if you think the current implement is not racy, you can just tell
me why. If you want to fix the race, go ahead and do it. Thanks.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#10656 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYuAio9NKZ72LkCYWj9uZt_m9X6FNks5qZCKUgaJpZM4FQR6U
.

xiang90 · 2016-07-25T14:08:13Z

@ncdc Hijacking and timeout cannot work well together I think. Once we hijack it, we have to cancel the timeout handler and set TCP timeout on the conn. The timeout handler cannot assume the inner hijacked handler speaks http anymore. Writing HTTP timeout headers once it timeouts makes no sense after the hijacking. That is part of the reason standard lib does not support timeout + hijacking i believe.

I did grep Hijacker -r . and failed to find a case that we really need hijacker. Maybe I was wrong.

ncdc · 2016-07-25T14:14:13Z

@xiang90 see

kubernetes/pkg/util/httpstream/spdy/upgrade.go

Lines 54 to 65 in ef0c9f0

    
           hijacker, ok := w.(http.Hijacker) 
        
           if !ok { 
        
           	w.WriteHeader(http.StatusInternalServerError) 
        
           	fmt.Fprintf(w, "unable to upgrade: unable to hijack response") 
        
           	return nil 
        
           } 
        
           w.Header().Add(httpstream.HeaderConnection, httpstream.HeaderUpgrade) 
        
           w.Header().Add(httpstream.HeaderUpgrade, HeaderSpdy31) 
        
           w.WriteHeader(http.StatusSwitchingProtocols) 
        
           conn, _, err := hijacker.Hijack()

I agree we don't want to timeout a hijacked connection (the current spdy implementation for exec/attach/port-forward has its own idle timeout functionality).

xiang90 · 2016-07-25T14:32:08Z

@ncdc Oh. I missed that one... Was assuming it is a dependency :(. OK. Then that makes sense. The spdy thing will go away once we use HTTP2 by default which does multiplexing on TCP? What is your suggestion for moving forward for now? also /cc @smarterclayton

ncdc · 2016-07-25T14:36:01Z

We will need time to engineer the HTTP2 replacement, plus we'll have to go through a deprecation period for the spdy implementation.

The go HTTP2 implementation allows you to read from/write to the request and response bodies without having to go through any sort of hijacking. We'll have to write our own multiplexing to support multiple "streams" (stdin, stdout, stderr, error, resize, etc), however.

xiang90 · 2016-07-25T14:38:43Z

@ncdc OK. So we still need this. How do you think about cancelling http timeout once we hijacked the connection from timeout writer?

ncdc · 2016-07-25T14:47:48Z

@xiang90 there are a few routes that are currently excluded from the timeout handler, including logs, exec, attach, and portforward:

kubernetes/pkg/genericapiserver/options/server_run_options.go

Line 46 in 411c32b

    
           defaultLongRunningRequestRE = "(/|^)((watch|proxy)(/|$)|(logs?|portforward|exec|attach)/?$)"

kubernetes/pkg/genericapiserver/genericapiserver.go

Line 675 in 411c32b

if longRunningRequestCheck(req) {

kubernetes/pkg/apiserver/handlers.go

Line 186 in 411c32b

if after == nil {

Can we continue to do this?

xiang90 · 2016-07-25T14:50:11Z

@ncdc OK... So that means that the timeout handler does not need to support hijacking at all, since the one use hijacking does not under timeout handler?

ncdc · 2016-07-25T14:54:15Z

@xiang90 I think that's correct, but let's make sure @liggitt and @smarterclayton agree too 😄

liggitt · 2016-07-25T14:56:25Z

possible. I also wonder about the watch handling endpoints, which write chunked data and expect it to be flushed to the client, but also be able to timeout.

smarterclayton · 2016-07-25T17:23:11Z

Watch uses hijack for web sockets, but only needs to flush for chunked. I
thought for watch we set timeouts on the connection ourselves.

On Mon, Jul 25, 2016 at 10:56 AM, Jordan Liggitt notifications@github.com
wrote:

possible. I also wonder about the watch handling endpoints, which write
chunked data and expect it to be flushed to the client, but also be able to
timeout.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10656 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p8dbdvNNTjSFG12Y0kj0aXTLDypXks5qZM6ngaJpZM4FQR6U
.

xiang90 · 2016-07-25T17:34:14Z

@liggitt @smarterclayton

OK. So we do not need timeout for spdy upgrade as @ncdc mentioned. We do not need timeout for watch, which requires flush, since it should set timeout on connection.

The we probably should just use the standard timeout handler (which does not support hijacking and flush for good reasons) instead of the customized one.

I can make the change if @smarterclayton @liggitt @ncdc all agree.

liggitt · 2016-07-25T17:51:26Z

@ncdc do you know whether our current timeout is exercised for proxy paths?

liggitt · 2016-07-25T17:52:16Z

I just want to be careful that we don't regress spdy connections (exec/attach), websocket connections (exec/attach/log/watch), or streaming connections (log, watch, proxy). I also wonder about the perf impact of buffering all responses in memory... some responses can get quite large, especially for controllers that list across all namespaces.

ncdc · 2016-07-25T17:53:37Z

do you know whether our current timeout is exercised for proxy paths?

@liggitt which timeout in particular? for exec/attach/pf?

xiang90 · 2016-07-25T17:53:51Z

@liggitt First, we need to make things correct. Then we can start taking care about performance, optimization. Now it is broken.

liggitt · 2016-07-25T17:59:59Z

@ncdc if the current timeout handler is being used in proxy paths, or if proxy gets exempted from timeouts

googlebot added the cla: yes label Jul 2, 2015

zmerlynn assigned krousey Jul 2, 2015

bgrant0607 added this to the v1.0-post milestone Jul 6, 2015

krousey force-pushed the timeouts branch from 8fa775c to 2f788c6 Compare July 7, 2015 23:23

krousey force-pushed the timeouts branch from 2f788c6 to 7e5919f Compare July 10, 2015 21:40

krousey changed the title ~~WIP: Adding proper timeouts.~~ Adding proper timeouts. Jul 10, 2015

krousey removed their assignment Jul 10, 2015

Adding proper timeouts.

1d033b9

krousey force-pushed the timeouts branch from 7e5919f to 1d033b9 Compare July 10, 2015 21:43

krousey mentioned this pull request Jul 15, 2015

Updating timeouts to match master. #10647

Closed

brendandburns reviewed Jul 22, 2015
View reviewed changes

brendandburns self-assigned this Jul 22, 2015

bgrant0607 removed this from the v1.0-post milestone Jul 24, 2015

brendandburns added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2015

gmarek added a commit that referenced this pull request Jul 27, 2015

Merge pull request #10656 from krousey/timeouts

00cd52d

Adding proper timeouts.

gmarek merged commit 00cd52d into kubernetes:master Jul 27, 2015

liggitt reviewed Aug 10, 2015
View reviewed changes

pweil- mentioned this pull request Aug 10, 2015

re-add kubelet server timeouts #12498

Closed

liggitt reviewed Aug 10, 2015
View reviewed changes

krousey unassigned brendandburns Aug 12, 2015

krousey deleted the timeouts branch August 21, 2015 02:12

mmilata mentioned this pull request Aug 27, 2015

Default regular expression for long running requests does not match .../pods/.../log #13266

Closed

krousey added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 11, 2015

krousey mentioned this pull request Dec 8, 2015

Set timeout for clients created in e2e tests #18340

Merged

xiang90 mentioned this pull request Jul 25, 2016

API server panics when writing response #29001

Closed

Adding proper timeouts. #10656

Adding proper timeouts. #10656

Conversation

krousey commented Jul 2, 2015

krousey commented Jul 2, 2015

krousey commented Jul 2, 2015

k8s-bot commented Jul 2, 2015

zmerlynn commented Jul 2, 2015

thockin commented Jul 4, 2015

bgrant0607 commented Jul 6, 2015

krousey commented Jul 7, 2015

k8s-bot commented Jul 8, 2015

k8s-bot commented Jul 10, 2015

k8s-bot commented Jul 10, 2015

brendandburns Jul 22, 2015

Choose a reason for hiding this comment

krousey Jul 22, 2015

Choose a reason for hiding this comment

brendandburns commented Jul 22, 2015

liggitt Aug 10, 2015

Choose a reason for hiding this comment

krousey Aug 10, 2015

Choose a reason for hiding this comment

liggitt Aug 10, 2015

Choose a reason for hiding this comment

krousey Aug 10, 2015

Choose a reason for hiding this comment

krousey Aug 10, 2015

Choose a reason for hiding this comment

xiang90 commented Jul 25, 2016

liggitt commented Jul 25, 2016

xiang90 commented Jul 25, 2016

liggitt commented Jul 25, 2016

xiang90 commented Jul 25, 2016

ncdc commented Jul 25, 2016

xiang90 commented Jul 25, 2016

ncdc commented Jul 25, 2016

xiang90 commented Jul 25, 2016

ncdc commented Jul 25, 2016

xiang90 commented Jul 25, 2016

ncdc commented Jul 25, 2016

xiang90 commented Jul 25, 2016

ncdc commented Jul 25, 2016

liggitt commented Jul 25, 2016

smarterclayton commented Jul 25, 2016

xiang90 commented Jul 25, 2016

liggitt commented Jul 25, 2016

liggitt commented Jul 25, 2016

ncdc commented Jul 25, 2016

xiang90 commented Jul 25, 2016

liggitt commented Jul 25, 2016