Skip to content
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

Bump grpc max message size for docker service #63894

Conversation

dims
Copy link
Member

@dims dims commented May 15, 2018

What this PR does / why we need it:
When we have a lot of containers, we run into the limit in grpc ( https://github.com/grpc/grpc-go/blob/master/clientconn.go#L118 )

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 #63858

Special notes for your reviewer:
In #63977 we fixed the send and receive sizes on the client side. we should fix the docker service too

Release note:

Increase dockershim grpc service default request and response size. This reduces chances of failures when there are a large number of pods.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 15, 2018
@k8s-ci-robot k8s-ci-robot requested review from feiskyer and yifan-gu May 15, 2018 22:49
@dims
Copy link
Member Author

dims commented May 15, 2018

/assign @Random-Liu
/assign @yujuhong

@dims dims changed the title Bump grpc max message size for docker service [WIP] Bump grpc max message size for docker service May 15, 2018
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2018
@ceshihao
Copy link
Contributor

ceshihao commented May 16, 2018

I think it is OK as a temporary solution.
However, it can not resolve the problem finally if more and more message (size > 16M), and we have to increase it again.

Perhaps, it can have a final solution, like stream output message, result pagination or something else.

@dims
Copy link
Member Author

dims commented May 16, 2018

@ceshihao it's a WIP to see if this helps this specific scenario that fails, if it does, then we can try to figure out how to turn it into something acceptable

@ceshihao
Copy link
Contributor

ceshihao commented May 16, 2018 via email

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2018
@dims dims force-pushed the bump-grpc-max-message-size-for-docker-service branch from 525eb04 to eb6bd67 Compare May 19, 2018 20:52
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 19, 2018
@dims dims changed the title [WIP] Bump grpc max message size for docker service Bump grpc max message size for docker service May 19, 2018
@k8s-ci-robot k8s-ci-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 19, 2018
@dims
Copy link
Member Author

dims commented May 19, 2018

/assign @derekwaynecarr

@dims
Copy link
Member Author

dims commented May 19, 2018

cc @runcom @sjenning @feiskyer

Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2018
@feiskyer
Copy link
Member

/retest

@dims
Copy link
Member Author

dims commented May 20, 2018

/test pull-kubernetes-e2e-gce

@yujuhong
Copy link
Contributor

I think bumping the limit is reasonable (although maybe we should have a more concrete example to reason able the size).

As for a long-term solution such as pagination, I am not against the idea, but do feel like we need a better justification (e.g., expected #containers and the size of each container) for adding the complexity.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, feiskyer, yujuhong

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@mkumatag
Copy link
Member

mkumatag commented Jul 5, 2018

@dims can this be cherry-picked for 1.10 release?

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 5, 2018
@dims
Copy link
Member Author

dims commented Jul 5, 2018

@mkumatag Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: ResourceExhausted desc = grpc: received message larger than max (4195017 vs. 4194304)
9 participants