New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 24, 2018

Conversation

@MikeSpreitzer
Member

MikeSpreitzer commented Feb 19, 2018

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

This comment has been minimized.

Member

MikeSpreitzer commented Feb 19, 2018

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

This comment has been minimized.

Member

MikeSpreitzer commented Feb 19, 2018

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

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

@MikeSpreitzer

This comment has been minimized.

Member

MikeSpreitzer commented Feb 20, 2018

/assign thockin

@deads2k

This comment has been minimized.

Contributor

deads2k commented Feb 20, 2018

/approve

lgtm

/assign sttts

@sttts merge at your discretion.

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

This comment has been minimized.

@sttts

sttts Feb 22, 2018

Contributor

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

This comment has been minimized.

@sttts

sttts Feb 22, 2018

Contributor

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?

This comment has been minimized.

@MikeSpreitzer

MikeSpreitzer Feb 22, 2018

Member

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?

This comment has been minimized.

@sttts

sttts Feb 22, 2018

Contributor

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

This comment has been minimized.

@MikeSpreitzer

MikeSpreitzer Feb 22, 2018

Member

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

This comment has been minimized.

Contributor

sttts commented Feb 22, 2018

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

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Feb 22, 2018

@@ -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 {

This comment has been minimized.

@sttts

sttts Feb 22, 2018

Contributor

no need to check for any of the nils here.

Fixes for HTTP/2 max streams per connection setting
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 #60042

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

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

@thockin

This comment has been minimized.

Member

thockin commented Feb 23, 2018

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

Assign back to me if approval is needed.

@sttts

This comment has been minimized.

Contributor

sttts commented Feb 23, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 23, 2018

@sttts

This comment has been minimized.

Contributor

sttts commented Feb 23, 2018

/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

This comment has been minimized.

Member

thockin commented Feb 23, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 23, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 24, 2018

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-merge-robot k8s-merge-robot merged commit 3c2a0c8 into kubernetes:master Feb 24, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation MikeSpreitzer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

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

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

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 16, 2018

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.

k8s-merge-robot added a commit that referenced this pull request Mar 28, 2018

Merge pull request #60802 from MikeSpreitzer/automated-cherry-pick-of-#…
…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

k8s-merge-robot added a commit that referenced this pull request Apr 12, 2018

Merge pull request #60685 from MikeSpreitzer/automated-cherry-pick-of-#…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment