*: configurable gRPC message size limits #9047

gyuho · 2017-12-19T23:23:07Z

gRPC update causing failure of API calls with large responses kubernetes/kubernetes#51099
Version bump to etcd v3.2.11, grpc v1.7.5 kubernetes/kubernetes#57160 (comment)

xiang90 · 2017-12-19T23:26:17Z

clientv3/grpc_options.go

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


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

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

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.

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

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.

xiang90 · 2017-12-19T23:26:46Z

clientv3/grpc_options.go

+	grpc.MaxCallSendMsgSize(math.MaxInt32),
+
+	// client-side request receive limit, gRPC default is 4MB
+	// (must be consistent with server-side max request limit)


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

xiang90 · 2017-12-19T23:27:53Z

defer to @jpbetz

xiang90 · 2017-12-19T23:44:38Z

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 · 2017-12-19T23:51:57Z

@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 · 2017-12-19T23:54:11Z

Not really. Users can write 1000 small values and do a range to get them all. That is why we should limit the send value from client not the receive value.

…

On Tue, Dec 19, 2017 at 3:52 PM Gyuho Lee ***@***.***> wrote: @xiang90 <https://github.com/xiang90> The error in kubernetes/kubernetes#51099 <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 <https://github.com/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? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9047 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AERby40bXikj8QSp-AbHIXpyTAWr2MKTks5tCEwxgaJpZM4RHsZi> .

gyuho · 2017-12-19T23:55:50Z

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

codecov-io · 2017-12-20T13:38:46Z

Codecov Report

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

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

xiang90 · 2017-12-20T16:48:38Z

clientv3/grpc_options.go

+	// 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)


probably bump to 2 to leave some space?

Sounds good. Will change it to 2 MiB.

gyuho · 2017-12-20T18:14:58Z

@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 · 2017-12-20T18:17:38Z

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

Signed-off-by: Gyuho Lee <[email protected]>

Make headers consistent. Signed-off-by: Gyuho Lee <[email protected]>

Signed-off-by: Gyuho Lee <[email protected]>

gyuho · 2017-12-20T19:12:39Z

@jpbetz Feedback all addressed. PTAL.

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

Thanks!

jpbetz

lgtm

Thanks @gyuho!!

@gyuho

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. ```

@gyuho

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

@gyuho

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

@gyuho

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

@gyuho

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

@gyuho

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

@gyuho

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

@gyuho

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

@gyuho

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

@gyuho

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

gyuho requested review from xiang90 and jpbetz December 19, 2017 23:23

xiang90 reviewed Dec 19, 2017

View reviewed changes

gyuho force-pushed the client-grpc-call-options branch from d9951e3 to 6994373 Compare December 19, 2017 23:45

gyuho added the WIP label Dec 20, 2017

gyuho mentioned this pull request Dec 20, 2017

clientv3: configure grpc.MaxCallSendMsgSize, grpc.MaxCallRecvMsgSize #9043

Closed

2 tasks

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

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

gyuho force-pushed the client-grpc-call-options branch 2 times, most recently from 393ad56 to e19e863 Compare December 20, 2017 08:18

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

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

gyuho removed the WIP label Dec 20, 2017

gyuho force-pushed the client-grpc-call-options branch from 31242cd to cfcf65f Compare December 20, 2017 13:47

xiang90 reviewed Dec 20, 2017

View reviewed changes

gyuho force-pushed the client-grpc-call-options branch from cfcf65f to 1aef249 Compare December 20, 2017 17:39

gyuho mentioned this pull request Dec 20, 2017

etcdctl/v3, grpc-proxy: configure --max-send/recv-bytes #9055

Closed

gyuho added 9 commits December 20, 2017 10:58

clientv3: configure gRPC message limits in Config

63d66b1

Signed-off-by: Gyuho Lee <[email protected]>

clientv3: call KV/Txn APIs with default gRPC call options

f877609

Signed-off-by: Gyuho Lee <[email protected]>

clientv3: call other APIs with default gRPC call options

497412c

Signed-off-by: Gyuho Lee <[email protected]>

clientv3/integration: test large KV requests

f38593b

Signed-off-by: Gyuho Lee <[email protected]>

words: whitelist more

1b3ed91

Signed-off-by: Gyuho Lee <[email protected]>

Documentation/upgrades: clean up 3.2, 3.3 guides

3d924ae

Make headers consistent. Signed-off-by: Gyuho Lee <[email protected]>

Documentation/upgrades: highlight request limit changes in v3.2, v3.3

6bfde98

Signed-off-by: Gyuho Lee <[email protected]>

clientv3/integration: fix TestKVPutError

88fe8de

Signed-off-by: Gyuho Lee <[email protected]>

integration: bump up wait leader timeout for slow CIs

3d56045

Signed-off-by: Gyuho Lee <[email protected]>

gyuho force-pushed the client-grpc-call-options branch from 1aef249 to ac9d22e Compare December 20, 2017 19:05

Documentation/upgrades: highlight raw gRPC client wrapper changes

3c5eb4f

Signed-off-by: Gyuho Lee <[email protected]>

gyuho force-pushed the client-grpc-call-options branch from ac9d22e to 3c5eb4f Compare December 20, 2017 19:09

jpbetz approved these changes Dec 20, 2017

View reviewed changes

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

gyuho deleted the client-grpc-call-options branch December 20, 2017 20:25

This was referenced Dec 20, 2017

Version bump to etcd v3.2.13, grpc v1.7.5 kubernetes/kubernetes#57480

Merged

gRPC update causing failure of API calls with large responses kubernetes/kubernetes#51099

Closed

benpaxton-hf mentioned this pull request Jan 15, 2018

Vault Fails to Start with Large Number of Tokens hashicorp/vault#3772

Closed

shyamjvs mentioned this pull request Sep 12, 2022

Apiserver should map etcd errors to proper response status kubernetes/kubernetes#112152

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: configurable gRPC message size limits #9047

*: configurable gRPC message size limits #9047

gyuho commented Dec 19, 2017 •

edited

Loading

xiang90 Dec 19, 2017

gyuho Dec 19, 2017 •

edited

Loading

xiang90 Dec 19, 2017

gyuho Dec 19, 2017

jpbetz Dec 20, 2017 •

edited

Loading

xiang90 Dec 19, 2017

xiang90 commented Dec 19, 2017

xiang90 commented Dec 19, 2017

gyuho commented Dec 19, 2017

xiang90 commented Dec 19, 2017 via email

gyuho commented Dec 19, 2017

codecov-io commented Dec 20, 2017 •

edited

Loading

xiang90 Dec 20, 2017

gyuho Dec 20, 2017

gyuho commented Dec 20, 2017 •

edited

Loading

jpbetz commented Dec 20, 2017

gyuho commented Dec 20, 2017

jpbetz left a comment

*: configurable gRPC message size limits #9047

*: configurable gRPC message size limits #9047

Conversation

gyuho commented Dec 19, 2017 • edited Loading

xiang90 Dec 19, 2017

Choose a reason for hiding this comment

gyuho Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

xiang90 Dec 19, 2017

Choose a reason for hiding this comment

gyuho Dec 19, 2017

Choose a reason for hiding this comment

jpbetz Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

xiang90 Dec 19, 2017

Choose a reason for hiding this comment

xiang90 commented Dec 19, 2017

xiang90 commented Dec 19, 2017

gyuho commented Dec 19, 2017

xiang90 commented Dec 19, 2017 via email

gyuho commented Dec 19, 2017

codecov-io commented Dec 20, 2017 • edited Loading

Codecov Report

xiang90 Dec 20, 2017

Choose a reason for hiding this comment

gyuho Dec 20, 2017

Choose a reason for hiding this comment

gyuho commented Dec 20, 2017 • edited Loading

jpbetz commented Dec 20, 2017

gyuho commented Dec 20, 2017

jpbetz left a comment

Choose a reason for hiding this comment

gyuho commented Dec 19, 2017 •

edited

Loading

gyuho Dec 19, 2017 •

edited

Loading

jpbetz Dec 20, 2017 •

edited

Loading

codecov-io commented Dec 20, 2017 •

edited

Loading

gyuho commented Dec 20, 2017 •

edited

Loading