Fixes for HTTP/2 max streams per connection setting #60054

MikeSpreitzer · 2018-02-19T19:23:15Z

What this PR does / why we need it:
This PR makes two changes. One is to introduce a parameter
for the HTTP/2 setting that an api-server sends to its clients
telling them how many streams they may have concurrently open in
an HTTP/2 connection. If left at its default value of zero,
this means to use the default in golang's HTTP/2 code (which
is currently 250; see https://github.com/golang/net/blob/master/http2/server.go).

The other change is to make the recommended options for an aggregated
api-server set this limit to 1000. The limit of 250 is annoyingly low
for the use case of many controllers watching objects of Kinds served
by an aggregated api-server reached through the main api-server (in
its mode as a proxy for the aggregated api-server, in which it uses a
single HTTP/2 connection for all calls proxied to that aggregated
api-server).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #60042

Special notes for your reviewer:

Release note:

Introduced `--http2-max-streams-per-connection` command line flag on api-servers and set default to 1000 for aggregated API servers.

MikeSpreitzer · 2018-02-19T19:52:07Z

All the tests are failing with the following complaint:

W0219 19:28:43.861] 2018/02/19 19:28:43 missing strict dependencies:
W0219 19:28:43.862] 	vendor/k8s.io/apiserver/pkg/server/serve.go: import of golang.org/x/net/http2, which is not a direct dependency

But golang.org/x/net/http2 is in vendor, Godeps/Godeps.json, and staging/src/k8s.io/apiserver/Godeps/Godeps.json. What am I missing?

MikeSpreitzer · 2018-02-19T23:04:03Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

MikeSpreitzer · 2018-02-20T11:18:47Z

/assign thockin

deads2k · 2018-02-20T12:59:15Z

/approve

lgtm

/assign sttts

@sttts merge at your discretion.

sttts · 2018-02-22T07:20:42Z

staging/src/k8s.io/apiserver/pkg/server/options/serving.go

@@ -83,6 +86,7 @@ func NewSecureServingOptions() *SecureServingOptions {
 			PairName:      "apiserver",
 			CertDirectory: "apiserver.local.config/certificates",
 		},
+		HTTP2MaxStreamsPerConnection: 1000,


are there any side-effects of this? Memory, cpu, ...?

This default here is for every apiserver, and even controller-manager and soon'ish the scheduler. Maybe we should only change Golang's default for kube-apiserver?

Of course this affects memory, CPU, and network load. I agree it would be better for each program to have its own default. It would be even better if a server could tailor this limit to each individual client. What would you suggest as the API for either of these?

We can leave the default at 0 (go default) and override it in kube-aggregator and kube-apiserver.

Actually, the use case that concerns me is an aggregated api-server. One of those today tells the main api-server to limit itself to 250 open streams in its HTTP/2 connection to the aggregated api-server. The pain point was having more than 250 watches open from controllers that connect to the main api-server. Those connections work just fine, it is where the proxy function multiplexes them all into one HTTP/2 connection to the aggregated api-server that the current limit bites. So I will update just the sample-apiserver to override the limit.

sttts · 2018-02-22T16:10:05Z

This is probably also worth a cherry-pick to 1.9 after it is merged.

sttts · 2018-02-22T20:36:34Z

staging/src/k8s.io/sample-apiserver/pkg/cmd/server/start.go

@@ -52,7 +52,11 @@ func NewWardleServerOptions(out, errOut io.Writer) *WardleServerOptions {
 		StdOut: out,
 		StdErr: errOut,
 	}
-
+	if o.RecommendedOptions != nil && o.RecommendedOptions.SecureServing != nil && o.RecommendedOptions.SecureServing.SecureServingOptions != nil {


no need to check for any of the nils here.

This PR makes two changes. One is to introduce a parameter for the HTTP/2 setting that an api-server sends to its clients telling them how many streams they may have concurrently open in an HTTP/2 connection. If left at its default value of zero, this means to use the default in golang's HTTP/2 code (which is currently 250). The other change is to make the recommended options for an aggregated api-server set this limit to 1000. The limit of 250 is annoyingly low for the use case of many controllers watching objects of Kinds served by an aggregated api-server reached through the main api-server (in its mode as a proxy for the aggregated api-server, in which it uses a single HTTP/2 connection for all calls proxied to that aggregated api-server). Fixes kubernetes#60042

thockin · 2018-02-23T06:39:45Z

I know too little about this area, so punting to others who know it better.

Assign back to me if approval is needed.

sttts · 2018-02-23T15:47:55Z

/lgtm
/approve

sttts · 2018-02-23T15:50:56Z

/assign @thockin

This has no influence on any kubernetes binary we ship, but only on aggregated API servers building on our libraries. But for the later and any non-trivial use this is crucial.

thockin · 2018-02-23T16:14:40Z

/approve

k8s-ci-robot · 2018-02-23T16:14:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, MikeSpreitzer, sttts, thockin

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:

~~cmd/cloud-controller-manager/OWNERS~~ [thockin]
~~cmd/kube-apiserver/OWNERS~~ [deads2k,sttts,thockin]
~~cmd/kube-controller-manager/OWNERS~~ [deads2k,thockin]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,sttts,thockin]

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

k8s-github-robot · 2018-02-24T07:15:33Z

Automatic merge from submit-queue (batch tested with PRs 60054, 60202, 60219, 58090, 60275). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot · 2018-03-16T22:34:26Z

Commit found in the "release-1.9" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

…-#60054-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #60054 Cherry pick of #60054 on release-1.8. #60054: Fixes for HTTP/2 max streams per connection setting

…-#60054-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #60054 Cherry pick of #60054 on release-1.9. #60054: Fixes for HTTP/2 max streams per connection setting

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 19, 2018

k8s-ci-robot requested review from deads2k and smarterclayton February 19, 2018 19:23

MikeSpreitzer mentioned this pull request Feb 20, 2018

Release-1.8 edition of #60054 #60065

Closed

MikeSpreitzer force-pushed the issue-60042-field branch from 84231b5 to 930fd6d Compare February 20, 2018 05:39

k8s-ci-robot assigned thockin Feb 20, 2018

k8s-ci-robot assigned sttts Feb 20, 2018

sttts reviewed Feb 22, 2018

View reviewed changes

MikeSpreitzer force-pushed the issue-60042-field branch from 930fd6d to 2f2ff19 Compare February 22, 2018 20:30

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 22, 2018

sttts reviewed Feb 22, 2018

View reviewed changes

MikeSpreitzer force-pushed the issue-60042-field branch 2 times, most recently from 377f893 to f89946f Compare February 22, 2018 21:30

MikeSpreitzer force-pushed the issue-60042-field branch from f89946f to 201c11f Compare February 22, 2018 21:31

MikeSpreitzer added a commit to MikeSpreitzer/kubernetes that referenced this pull request Feb 23, 2018

Release-1.8 edition of kubernetes#60054

cd124cf

thockin assigned lavalamp and cheftako and unassigned thockin Feb 23, 2018

sttts added the kind/bug Categorizes issue or PR as related to a bug. label Feb 23, 2018

sttts added this to the v1.10 milestone Feb 23, 2018

sttts added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Feb 23, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2018

k8s-ci-robot assigned thockin Feb 23, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2018

k8s-github-robot merged commit 3c2a0c8 into kubernetes:master Feb 24, 2018

MikeSpreitzer added a commit to MikeSpreitzer/kubernetes that referenced this pull request Feb 26, 2018

Cherry pick kubernetes#60054 to release-1.9

e8b04c3

MikeSpreitzer mentioned this pull request Feb 26, 2018

Cherry-Pick #60054 to release-1.9 #60449

Closed

lavalamp added the cherrypick-candidate label Mar 2, 2018

lavalamp modified the milestones: v1.10, v1.9 Mar 2, 2018

This was referenced Mar 2, 2018

Automated cherry pick of #60054 #60685

Merged

Automated cherry pick of #60054 #60802

Merged

k8s-github-robot removed the cherrypick-candidate label Mar 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for HTTP/2 max streams per connection setting #60054

Fixes for HTTP/2 max streams per connection setting #60054

MikeSpreitzer commented Feb 19, 2018 •

edited by sttts

Loading

MikeSpreitzer commented Feb 19, 2018

MikeSpreitzer commented Feb 19, 2018

MikeSpreitzer commented Feb 20, 2018

deads2k commented Feb 20, 2018

sttts Feb 22, 2018

sttts Feb 22, 2018

MikeSpreitzer Feb 22, 2018

sttts Feb 22, 2018

MikeSpreitzer Feb 22, 2018 •

edited

Loading

sttts commented Feb 22, 2018

sttts Feb 22, 2018

thockin commented Feb 23, 2018

sttts commented Feb 23, 2018

sttts commented Feb 23, 2018

thockin commented Feb 23, 2018

k8s-ci-robot commented Feb 23, 2018

k8s-github-robot commented Feb 24, 2018

k8s-github-robot commented Mar 16, 2018

Fixes for HTTP/2 max streams per connection setting #60054

Fixes for HTTP/2 max streams per connection setting #60054

Conversation

MikeSpreitzer commented Feb 19, 2018 • edited by sttts Loading

MikeSpreitzer commented Feb 19, 2018

MikeSpreitzer commented Feb 19, 2018

MikeSpreitzer commented Feb 20, 2018

deads2k commented Feb 20, 2018

sttts Feb 22, 2018

Choose a reason for hiding this comment

sttts Feb 22, 2018

Choose a reason for hiding this comment

MikeSpreitzer Feb 22, 2018

Choose a reason for hiding this comment

sttts Feb 22, 2018

Choose a reason for hiding this comment

MikeSpreitzer Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

sttts commented Feb 22, 2018

sttts Feb 22, 2018

Choose a reason for hiding this comment

thockin commented Feb 23, 2018

sttts commented Feb 23, 2018

sttts commented Feb 23, 2018

thockin commented Feb 23, 2018

k8s-ci-robot commented Feb 23, 2018

k8s-github-robot commented Feb 24, 2018

k8s-github-robot commented Mar 16, 2018

MikeSpreitzer commented Feb 19, 2018 •

edited by sttts

Loading

MikeSpreitzer Feb 22, 2018 •

edited

Loading