Add a limit to the number of in-flight requests that a server processes. #6207

brendandburns · 2015-03-31T04:41:49Z

@smarterclayton @alex-mohr @fabioy @quinton-hoole

smarterclayton · 2015-03-31T04:43:25Z

What happens when all the inflight requests are consumed by watches? Starvation?

brendandburns · 2015-03-31T04:49:07Z

yes. I suppose we might want to have a separate counter for watches?

brendandburns · 2015-03-31T04:53:33Z

Or we could just ignore watches for now. We managed to DOS an apiserver out of file descriptors by spamming it. That's bad :P hence this PR.

smarterclayton · 2015-03-31T05:33:43Z

We are already talking about pushing some operations deeper into apiserver, so if it's high level for now when those other steps complete it will be easier to move this down.

ghost · 2015-03-31T14:43:23Z

pkg/apiserver/handlers.go

@@ -66,6 +66,22 @@ func ReadOnly(handler http.Handler) http.Handler {
 	})
 }

+// MaxInFlight limits the number of in flight requests to the number of


ghost · 2015-03-31T15:05:26Z

How do we monitor the number of inflight requests over time? I'm guessing that the current answer is "we don't", in which case please add an issue requesting this, at least.

vmarmol · 2015-03-31T15:11:40Z

Assigning to @quinton-hoole, feel free to reassign.

brendandburns · 2015-03-31T17:55:18Z

Comments addressed, and #6227 filed for the issue Quinton asked for.

ghost · 2015-03-31T18:20:23Z

Getting back to smarterclayton@ 's comment above, surely with the low default that you've set to 20 inflight requests in this PR, watch requests will pretty much always use up all of those, and we'll just serve errors for everything else? Am I missing something?

smarterclayton · 2015-03-31T18:42:41Z

Also clients can open watches pretty easily, so suddenly our watch could could be 50-100

----- Original Message -----

Getting back to smarterclayton@ 's comment above, surely with the low default
that you've set to 20 inflight requests in this PR, watch requests will
pretty much always use up all of those, and we'll just serve errors for
everything else? Am I missing something?

Reply to this email directly or view it on GitHub:
#6207 (comment)

ghost · 2015-03-31T18:56:35Z

One option would be to set the default limit closer to the default per-process file handle limit of 1024. A better solution, as you mentioned, would be to split the limits for watches and other stuff into separate counters.

alex-mohr · 2015-03-31T21:20:04Z

+1 to separate buckets for long-running requests vs. short ones. May also be worth having separate buckets for different operation types that have different dependencies -- e.g. reading from etcd vs. writing?

Is there a separate issue to call go's equivalent of setrlimit(RLIMIT_NOFILE) to something higher? filedescs are relatively cheap and 1024 is tiny. See golang/go#5949 for sample code?

brendandburns · 2015-03-31T23:25:32Z

Ok, added a regexp to not count watch and proxy, expanded tests to cover that functionality.

brendandburns · 2015-03-31T23:25:57Z

Please take another look.

smarterclayton · 2015-03-31T23:38:35Z

pkg/apiserver/handlers_test.go

+	block.Add(1)
+	sem := make(chan bool, Iterations)
+
+	re := regexp.MustCompile("[.*\\/watch][^\\/proxy.*]")


In v1beta3 we are not going to be using /watch - we'll be using a query param on the list response. I don't know where that leaves us.

To fix the implementation so the test passes when that goes live?

its there now, it's just no one is using it yet. It doesn't have to block this, but it's not clear to me how we tell that.

brendandburns · 2015-04-01T22:13:43Z

Fixed gofmt issues (and installed the git hooks on my new machine... ;)

ghost · 2015-04-02T04:28:24Z

Travis is still whining about a build failure. I don't know enough about Travis yet to know whether or not we can trust it's judgement. Other than that, I think that this can be merged.

zmerlynn · 2015-04-02T04:49:46Z

Travis kicked.

ghost · 2015-04-02T15:28:28Z

Travis still complaining :-(

ghost · 2015-04-02T18:26:24Z

PS: zmerlynn@ when you say "Travis kicked", what exactly do you kick?

smarterclayton · 2015-04-02T18:32:06Z

There's a restart build button in the travis UI if you have commit writes on the underlying repo

On Apr 2, 2015, at 2:26 PM, Quinton Hoole notifications@github.com wrote:

PS: zmerlynn@ when you say "Travis kicked", what exactly do you kick?

—
Reply to this email directly or view it on GitHub.

brendandburns · 2015-04-02T22:38:01Z

Travis is now happy. I can haz mergez?

brendandburns · 2015-04-02T22:38:20Z

(and you need to login to travis via github)

Add a limit to the number of in-flight requests that a server processes.

timothysc · 2015-04-06T17:11:56Z

Can folks please label this as perf for tracking.

roberthbailey · 2015-04-06T17:21:04Z

@timothysc done.

timothysc · 2015-04-06T18:35:06Z

@wojtek-t have you assessed the impact of this and #6203 yet?

wojtek-t · 2015-04-06T18:41:44Z

Sorry - we have a holiday in Poland today - will take a look tomorrow.

fgrzadkowski · 2015-04-07T13:28:50Z

Side effect of this PR is that density test is creating pods much slower than earlier (~6 pods per second). For 50 node cluster and rc with 1500 replicas (30 pods per node) this gives 300 seconds just to create rc.

fgrzadkowski · 2015-04-07T13:45:47Z

Actually my previous comment should be in #6203

timothysc · 2015-04-07T15:16:34Z

@fgrzadkowski thx, I figured this would be the case, we're currently working through issues with running load-balanced apiservers as a result.

@abhgupta, @smarterclayton, @rrati ^ FYI.

jayunit100 · 2015-04-07T19:40:28Z

related ???

I'm seeing very slow proxy returns in networking.go in my cluster.
However, i don't see this when i run locally (i.e. hack/local-up-cluster.sh), so only in a real cluster do these 6 second latencies for requests happen.

INFO: Proxy status call returned in 6.015694382s
INFO: About to make a proxy status call
INFO: Proxy status call returned in 6.013796121s 
...

timothysc · 2015-04-07T20:54:14Z

The cluster response curve from *this & #6203 are bad, imho this is not the correct route to take, or at least default off until good defaults can be found. Time to fill a pool has regressed sharply.

~2.3 pods per second which is a fairly drastic decrease from about a week ago.

wojtek-t · 2015-04-08T06:13:36Z

@timothysc - I agree. I observed a very similar scheduling rate and indeed it's much worse than before it.

googlebot added the cla: yes label Mar 31, 2015

ghost reviewed Mar 31, 2015
View reviewed changes

vmarmol assigned ghost Mar 31, 2015

brendandburns force-pushed the server branch from 895d70a to afc9d3c Compare March 31, 2015 17:55

brendandburns force-pushed the server branch from afc9d3c to eba4b0a Compare March 31, 2015 23:22

smarterclayton reviewed Mar 31, 2015
View reviewed changes

Add a limit to the number of in-flight requests that a server processes.

f327e97

brendandburns force-pushed the server branch from eba4b0a to f327e97 Compare April 1, 2015 22:08

ghost pushed a commit that referenced this pull request Apr 2, 2015

Merge pull request #6207 from brendandburns/server

4a2000c

Add a limit to the number of in-flight requests that a server processes.

ghost merged commit 4a2000c into kubernetes:master Apr 2, 2015

roberthbailey added the sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. label Apr 6, 2015

wojtek-t mentioned this pull request Apr 7, 2015

Watch shouldn't break every 5 minutes #6513

Closed

timothysc mentioned this pull request Apr 9, 2015

Update to option enable client.QPS and client.Burst #6635

Merged

smarterclayton mentioned this pull request Jun 8, 2015

Support for /watch in api paths should be removed #8337

Closed

brendandburns unassigned ghost Aug 12, 2015

This pull request was closed.

Add a limit to the number of in-flight requests that a server processes. #6207

Add a limit to the number of in-flight requests that a server processes. #6207

Conversation

brendandburns commented Mar 31, 2015

smarterclayton commented Mar 31, 2015

brendandburns commented Mar 31, 2015

brendandburns commented Mar 31, 2015

smarterclayton commented Mar 31, 2015

ghost Mar 31, 2015

Choose a reason for hiding this comment

brendandburns Mar 31, 2015

Choose a reason for hiding this comment

ghost commented Mar 31, 2015

vmarmol commented Mar 31, 2015

brendandburns commented Mar 31, 2015

ghost commented Mar 31, 2015

smarterclayton commented Mar 31, 2015

ghost commented Mar 31, 2015

alex-mohr commented Mar 31, 2015

brendandburns commented Mar 31, 2015

brendandburns commented Mar 31, 2015

smarterclayton Mar 31, 2015

Choose a reason for hiding this comment

brendandburns Apr 1, 2015

Choose a reason for hiding this comment

smarterclayton Apr 1, 2015

Choose a reason for hiding this comment

brendandburns commented Apr 1, 2015

ghost commented Apr 2, 2015

zmerlynn commented Apr 2, 2015

ghost commented Apr 2, 2015

ghost commented Apr 2, 2015

smarterclayton commented Apr 2, 2015

brendandburns commented Apr 2, 2015

brendandburns commented Apr 2, 2015

timothysc commented Apr 6, 2015

roberthbailey commented Apr 6, 2015

timothysc commented Apr 6, 2015

wojtek-t commented Apr 6, 2015

fgrzadkowski commented Apr 7, 2015

fgrzadkowski commented Apr 7, 2015

timothysc commented Apr 7, 2015

jayunit100 commented Apr 7, 2015

timothysc commented Apr 7, 2015

wojtek-t commented Apr 8, 2015