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

*: configurable gRPC message size limits #9047

Merged
merged 11 commits into from Dec 20, 2017

Conversation

4 participants

@gyuho gyuho requested review from xiang90 and jpbetz Dec 19, 2017

grpc.FailFast(true),

// client-side request send limit, gRPC default is math.MaxInt32
grpc.MaxCallSendMsgSize(math.MaxInt32),

This comment has been minimized.

Copy link
@xiang90

xiang90 Dec 19, 2017

Contributor

what is the sever side recv default limit? the send limit should be smaller than server side recv default limit.

This comment has been minimized.

Copy link
@gyuho

gyuho Dec 19, 2017

Author Member

@xiang90

Server-side default send maxSendBytes is math.MaxInt32.
Server-side default receive embed.DefaultMaxRequestBytes is 1.5MB.

For now, it seems easier to configure client-side parameters unlimited.

This is very easy to mis-configure.

For example, user A configures 5MB server-side request limit and 4.5MB client-side send limit, but forgot to configure client-side receive limit (thus defaults to 4MB). Then, 4.2MB write requests would still succeed, while receive requests fail.

This comment has been minimized.

Copy link
@xiang90

xiang90 Dec 19, 2017

Contributor

the goal of the default value is to prevent users to shoot in foot. I do not think we should expect users to send out a request that is > 4MB.

This comment has been minimized.

Copy link
@gyuho

gyuho Dec 19, 2017

Author Member

@xiang90 Agree. But, kubernetes/kubernetes#51099 is sending 26MB :0

This comment has been minimized.

Copy link
@jpbetz

jpbetz Dec 20, 2017

Contributor

FWIW, The current rule of thumb for kubernetes is to limit all objects stored in etcd to under 1MB. So I'd expect sends to be in that range. Receives can be range reads, which can get pretty huge for things like event-exporter (kubernetes/kubernetes#51099) that literally dumps out the entire etcd events keyspace.

grpc.MaxCallSendMsgSize(math.MaxInt32),

// client-side request receive limit, gRPC default is 4MB
// (must be consistent with server-side max request limit)

This comment has been minimized.

Copy link
@xiang90

xiang90 Dec 19, 2017

Contributor

client side recv limit should >= server side send max request limit

@xiang90

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

defer to @jpbetz

@xiang90

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

Is it sending 26MB or receiving? If it is sending 26MB, the request will fail regardless. etcd server wont accept put that is larger than 1.5MB.

@gyuho gyuho force-pushed the gyuho:client-grpc-call-options branch from d9951e3 to 6994373 Dec 19, 2017

@gyuho

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

@xiang90 The error in kubernetes/kubernetes#51099 is in the receiving. And since it happened with etcd client Get request, I assumed that 26MB writes were done via etcd client Puts as well. But not sure how such large value was written in the first place. /cc @jpbetz

client-side send limit < server-side recv default limit

client-side recv limit >= server-side send limit

Expose both client-side recv and send limits to clientv3.Config?

@xiang90

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

@gyuho

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

@xiang90 You are right. Let me address that shortly.

@gyuho gyuho added the WIP label Dec 20, 2017

@gyuho gyuho changed the title clientv3: make response receive size unlimited clientv3: configurable gRPC message size limits Dec 20, 2017

@gyuho gyuho changed the title clientv3: configurable gRPC message size limits *: configurable gRPC message size limits Dec 20, 2017

@gyuho gyuho force-pushed the gyuho:client-grpc-call-options branch 2 times, most recently from 393ad56 to e19e863 Dec 20, 2017

@gyuho gyuho added this to the v3.3.0 milestone Dec 20, 2017

@gyuho gyuho force-pushed the gyuho:client-grpc-call-options branch 5 times, most recently from 2cf2808 to 31242cd Dec 20, 2017

@gyuho gyuho removed the WIP label Dec 20, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Dec 20, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@2212789). Click here to learn what that means.
The diff coverage is 96.92%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #9047   +/-   ##
=========================================
  Coverage          ?   76.11%           
=========================================
  Files             ?      359           
  Lines             ?    29943           
  Branches          ?        0           
=========================================
  Hits              ?    22792           
  Misses            ?     5575           
  Partials          ?     1576
Impacted Files Coverage Δ
etcdserver/api/v3client/v3client.go 93.75% <100%> (ø)
clientv3/auth.go 95.94% <100%> (ø)
clientv3/watch.go 95.58% <100%> (ø)
clientv3/maintenance.go 71.42% <100%> (ø)
clientv3/txn.go 100% <100%> (ø)
integration/cluster.go 84.88% <100%> (ø)
clientv3/cluster.go 90.9% <100%> (ø)
clientv3/lease.go 92.83% <100%> (ø)
integration/cluster_proxy.go 100% <100%> (ø)
clientv3/kv.go 97.1% <100%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2212789...3c5eb4f. Read the comment docs.

@gyuho gyuho force-pushed the gyuho:client-grpc-call-options branch from 31242cd to cfcf65f Dec 20, 2017

// Make sure that "client-side send limit < server-side default send/recv limit"
// This is the same value as "embed.DefaultMaxRequestBytes" but not including
// gRPC overhead bytes; a bit smaller than server-side default send/recv limit.
defaultMaxCallSendMsgSize = grpc.MaxCallSendMsgSize(1.5 * 1024 * 1024)

This comment has been minimized.

Copy link
@xiang90

xiang90 Dec 20, 2017

Contributor

probably bump to 2 to leave some space?

This comment has been minimized.

Copy link
@gyuho

gyuho Dec 20, 2017

Author Member

Sounds good. Will change it to 2 MiB.

@gyuho gyuho force-pushed the gyuho:client-grpc-call-options branch from cfcf65f to 1aef249 Dec 20, 2017

@gyuho

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2017

@jpbetz @xiang90 PTAL.

The last commit addresses some flakiness in 32-bits--wait leader was taking longer than it should with 100ms context timeout. Increased the timeout to >1-second. Turns out this reduces test runtime by 1~2 minutes (semaphore 32- and 64-bit test suites).

For kubernetes/kubernetes#51099, etcd clientv3 must be upgraded to v3.2.12 (to be released soon), and MaxCallRecvMsgSize should be configured with minimum 27MiB, or just leave it empty and let it default to math.MaxInt32.

v3.2.11 without this patch defaults its receive limit to 4MiB in client-side. So, range response with 27MiB fails.

Will only backport clientv3 changes to v3.2.12 (hopefully today).

CHANGELOG is already updated to highlight this change (#8979).

@jpbetz

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

Thanks for the comprehensive docs, I just read through them and they made sense to me. Reviewing code now.

gyuho added some commits Dec 19, 2017

clientv3: configure gRPC message limits in Config
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
clientv3: call KV/Txn APIs with default gRPC call options
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
clientv3: call other APIs with default gRPC call options
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
clientv3/integration: test large KV requests
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
words: whitelist more
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Documentation/upgrades: clean up 3.2, 3.3 guides
Make headers consistent.

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
clientv3/integration: fix TestKVPutError
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
integration: bump up wait leader timeout for slow CIs
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

@gyuho gyuho force-pushed the gyuho:client-grpc-call-options branch from 1aef249 to ac9d22e Dec 20, 2017

Documentation/upgrades: highlight raw gRPC client wrapper changes
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

@gyuho gyuho force-pushed the gyuho:client-grpc-call-options branch from ac9d22e to 3c5eb4f Dec 20, 2017

@gyuho

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2017

@jpbetz Feedback all addressed. PTAL.

Will release v3.2.12 and v3.3.0-rc.0 with this by today afternoon.

Thanks!

@jpbetz

jpbetz approved these changes Dec 20, 2017

Copy link
Contributor

left a comment

lgtm

Thanks @gyuho!!

@gyuho gyuho merged commit 94e50a1 into etcd-io:master Dec 20, 2017

5 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-cov Build finished.
Details
jenkins-ppc64le Build finished.
Details
jenkins-proxy-ci Build finished.
Details
semaphoreci The build passed on Semaphore.
Details

@gyuho gyuho deleted the gyuho:client-grpc-call-options branch Dec 20, 2017

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Jan 7, 2018

Kubernetes Submit Queue
Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply #57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix #51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

k8s-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Jan 7, 2018

Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply kubernetes/kubernetes#57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix kubernetes/kubernetes#51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

Kubernetes-commit: 531b97ba93e46cde4b1fcd3f170224dbecbe2c1d

k8s-publishing-bot added a commit to kubernetes/kube-aggregator that referenced this pull request Jan 7, 2018

Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply kubernetes/kubernetes#57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix kubernetes/kubernetes#51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

Kubernetes-commit: 531b97ba93e46cde4b1fcd3f170224dbecbe2c1d

k8s-publishing-bot added a commit to kubernetes/sample-apiserver that referenced this pull request Jan 7, 2018

Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply kubernetes/kubernetes#57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix kubernetes/kubernetes#51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

Kubernetes-commit: 531b97ba93e46cde4b1fcd3f170224dbecbe2c1d

k8s-publishing-bot added a commit to kubernetes/apiextensions-apiserver that referenced this pull request Jan 7, 2018

Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply kubernetes/kubernetes#57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix kubernetes/kubernetes#51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

Kubernetes-commit: 531b97ba93e46cde4b1fcd3f170224dbecbe2c1d

sttts pushed a commit to sttts/apiserver that referenced this pull request Jan 9, 2018

Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply kubernetes/kubernetes#57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix kubernetes/kubernetes#51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

Kubernetes-commit: 531b97ba93e46cde4b1fcd3f170224dbecbe2c1d

sttts pushed a commit to sttts/kube-aggregator that referenced this pull request Jan 9, 2018

Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply kubernetes/kubernetes#57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix kubernetes/kubernetes#51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

Kubernetes-commit: 531b97ba93e46cde4b1fcd3f170224dbecbe2c1d

sttts pushed a commit to sttts/sample-apiserver that referenced this pull request Jan 9, 2018

Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply kubernetes/kubernetes#57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix kubernetes/kubernetes#51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

Kubernetes-commit: 531b97ba93e46cde4b1fcd3f170224dbecbe2c1d

sttts pushed a commit to sttts/apiextensions-apiserver that referenced this pull request Jan 9, 2018

Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply kubernetes/kubernetes#57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix kubernetes/kubernetes#51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

Kubernetes-commit: 531b97ba93e46cde4b1fcd3f170224dbecbe2c1d

openshift-publish-robot pushed a commit to openshift/kubernetes-sample-apiserver that referenced this pull request Jan 14, 2019

Merge pull request #57480 from jpbetz/etcd-client-3.2.12
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Version bump to etcd v3.2.13, grpc v1.7.5

Reapply kubernetes/kubernetes#57160 but with etcd 3.2.13, which includes etcd-io/etcd#9047 to fix kubernetes/kubernetes#51099.

We need to scalability test this PR before merging it since the previous attempt to version bump to grpc v1.7+ resulted in a scalability test failure after the PR was merged to master, and we don't want to repeat that. No, no we don't.

Thanks @gyuho for fixing the etcd grpc issue and releasing etcd-3.2.13 on short notice.

**Release note**:

```release-note
Upgrade to etcd client 3.2.13 and grpc 1.7.5 to improve HA etcd cluster stability.
```

Kubernetes-commit: 531b97ba93e46cde4b1fcd3f170224dbecbe2c1d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.