etcd: make max request bytes configurable #7968

fanminshi · 2017-05-23T00:00:26Z

heyitsanthony · 2017-05-23T00:02:25Z

etcdserver/server.go

@@ -83,6 +83,9 @@ const (

 	// maxPendingRevokes is the maximum number of outstanding expired lease revocations.
 	maxPendingRevokes = 16
+
+	defaultMaxRequestBytes     = 1.5 * 1024 * 1024
+	RecommendedMaxRequestBytes = 10 * 1024 * 1024


why export this?

no need to export. it was a typo.

heyitsanthony · 2017-05-23T00:03:53Z

etcdserver/server.go

@@ -83,6 +83,9 @@ const (

 	// maxPendingRevokes is the maximum number of outstanding expired lease revocations.
 	maxPendingRevokes = 16
+
+	defaultMaxRequestBytes     = 1.5 * 1024 * 1024


probably set this using embed.NewConfig(...) and lose the cfg.MaxRequestBytes <= 0 check

heyitsanthony · 2017-05-23T00:04:45Z

etcdserver/config.go

@@ -54,6 +54,8 @@ type ServerConfig struct {

 	AutoCompactionRetention int
 	QuotaBackendBytes       int64
+	// the max request size that raft accepts.


this should conform to godoc style if it's going to be commented

heyitsanthony · 2017-05-23T00:05:26Z

etcdserver/api/v3rpc/grpc.go

@@ -36,7 +36,10 @@ func Server(s *etcdserver.EtcdServer, tls *tls.Config) *grpc.Server {
 	}
 	opts = append(opts, grpc.UnaryInterceptor(newUnaryInterceptor(s)))
 	opts = append(opts, grpc.StreamInterceptor(newStreamInterceptor(s)))
-
+	// grpc defaults to 4MB for received message length.
+	if s.Cfg.MaxRequestBytes > 4*1024*1014 {


set this unconditionally instead of hardcoding 4MB?

By default grpc set it at 4MB, I was thinking about if the MaxRequestBytes is > than 4MB, then increase it or else leave it as it is.

OK, that's what the code does, but that doesn't explain the reasoning behind leaving it as-is in some cases but not in other cases. Also, what happens if the grpc default changes?

if grpc default change to less than 1.5 MB, then we will have an issue since the default msg size is 1.5 MB. I'll set it unconditionally.

heyitsanthony · 2017-05-23T00:06:20Z

etcdmain/help.go

@@ -66,6 +66,8 @@ member flags:
 		comma-separated whitelist of origins for CORS (cross-origin resource sharing).
 	--quota-backend-bytes '0'
 		raise alarms when backend size exceeds the given quota (0 defaults to low space quota).
+	--max-request-bytes '0'
+		maximum request size that raft accepts (size <=0 defaults to 1.5 MB). be aware that large request size can incur performance penalty.


default setting the var to 1.5MB instead of having a special case for <= 0?

so if I don't specify --max-request-bytes flag, then max-request-bytes defaults to 1.5 MB. If I specify --max-request-bytes, then it should be anything that's passed in even including 0?. I guess that's fine because etcdserver will return an error on reaching over the max request size.

heyitsanthony · 2017-05-23T00:06:30Z

embed/config.go

@@ -85,6 +85,7 @@ type Config struct {
 	TickMs            uint  `json:"heartbeat-interval"`
 	ElectionMs        uint  `json:"election-timeout"`
 	QuotaBackendBytes int64 `json:"quota-backend-bytes"`
+	MaxRequestBytes   int   `json:"max-request-bytes"`


uint to avoid < 0 cases

heyitsanthony · 2017-05-23T00:07:13Z

etcdmain/config.go

@@ -138,6 +138,7 @@ func newConfig() *config {
 	fs.UintVar(&cfg.TickMs, "heartbeat-interval", cfg.TickMs, "Time (in milliseconds) of a heartbeat interval.")
 	fs.UintVar(&cfg.ElectionMs, "election-timeout", cfg.ElectionMs, "Time (in milliseconds) for an election to timeout.")
 	fs.Int64Var(&cfg.QuotaBackendBytes, "quota-backend-bytes", cfg.QuotaBackendBytes, "Raise alarms when backend size exceeds the given quota. 0 means use the default quota.")
+	fs.IntVar(&cfg.MaxRequestBytes, "max-request-bytes", cfg.MaxRequestBytes, "maximum request size that raft accepts (size <=0 defaults to 1.5 MB). be aware that large request size can incur performance penalty.")


UintVar to avoid negatives

heyitsanthony · 2017-05-23T00:09:17Z

etcdserver/api/v3rpc/grpc.go

@@ -36,7 +36,10 @@ func Server(s *etcdserver.EtcdServer, tls *tls.Config) *grpc.Server {
 	}


this patch should be labeled v3rpc: ... instead of etcdserver: ... since it touches the v3rpc package

fanminshi · 2017-05-23T20:17:21Z

all fixed PTAL

xiang90 · 2017-05-24T03:07:21Z

clientv3/integration/kv_test.go

+		_, err := cli.Put(context.TODO(), test.key, string(valueBytes))
+		hasError := (err != nil)
+		if hasError != test.expectError {
+			fmt.Printf("error %v", err)


remove this print

xiang90 · 2017-05-24T03:07:42Z

etcdmain/config.go

@@ -138,6 +138,7 @@ func newConfig() *config {
 	fs.UintVar(&cfg.TickMs, "heartbeat-interval", cfg.TickMs, "Time (in milliseconds) of a heartbeat interval.")
 	fs.UintVar(&cfg.ElectionMs, "election-timeout", cfg.ElectionMs, "Time (in milliseconds) for an election to timeout.")
 	fs.Int64Var(&cfg.QuotaBackendBytes, "quota-backend-bytes", cfg.QuotaBackendBytes, "Raise alarms when backend size exceeds the given quota. 0 means use the default quota.")
+	fs.UintVar(&cfg.MaxRequestBytes, "max-request-bytes", cfg.MaxRequestBytes, "maximum request size that raft accepts; defaults to 1.5 MB. be aware that large request size can incur performance penalty.")


maximum > Maximum

xiang90 · 2017-05-24T03:08:19Z

etcdmain/help.go

@@ -66,6 +66,8 @@ member flags:
 		comma-separated whitelist of origins for CORS (cross-origin resource sharing).
 	--quota-backend-bytes '0'
 		raise alarms when backend size exceeds the given quota (0 defaults to low space quota).
+	--max-request-bytes '0'


0-> 1,500,000?

xiang90 · 2017-05-24T03:08:47Z

etcdserver/server.go

@@ -83,6 +83,8 @@ const (

 	// maxPendingRevokes is the maximum number of outstanding expired lease revocations.
 	maxPendingRevokes = 16
+
+	recommendedMaxRequestBytes = 10 * 1024 * 1024


defaultMaxRequestBytes

oh. i see. nvm

fanminshi · 2017-05-24T17:38:44Z

All fixed. PTAL

heyitsanthony · 2017-05-24T17:41:22Z

clientv3/integration/kv_test.go

@@ -895,3 +895,31 @@ func TestKVGetResetLoneEndpoint(t *testing.T) {
 	case <-donec:
 	}
 }
+
+// TestKVPutLargeRequests ensures that custom MaxRequestBytes works as intended.
+func TestKVPutLargeRequests(t *testing.T) {


this probably belongs in github.com/coreos/etcd/integration since it's only testing server functionality

heyitsanthony · 2017-05-24T17:41:32Z

clientv3/integration/kv_test.go

+	defer testutil.AfterTest(t)
+	tests := []struct {
+		key             string
+		MaxRequestBytes uint


s/MaxRequestBytes/maxRequestBytes

heyitsanthony · 2017-05-24T17:44:04Z

etcdserver/api/v3rpc/grpc.go

@@ -36,8 +36,11 @@ func Server(s *etcdserver.EtcdServer, tls *tls.Config) *grpc.Server {
 	}
 	opts = append(opts, grpc.UnaryInterceptor(newUnaryInterceptor(s)))
 	opts = append(opts, grpc.StreamInterceptor(newStreamInterceptor(s)))
-
+	// set MaxMsgSize be a bit higher than that of MaxRequestBytes bypasses grpc request size check


I don't understand this comment; where is 1024 coming from?

I noticed that grpc does an internal check for the incoming request. if the incoming request is greater than MaxMsgSize, it returns a grpc.internal error to client immediately. thus, the request never goes to etcd server and this check will never be performed.

if len(data) > maxRequestBytes { return nil, ErrRequestTooLarge }

https://github.com/coreos/etcd/blob/master/etcdserver/v3_server.go#L609.

which breaks some of our tests that expect ErrRequestTooLarge.

Just realized that adding 1024 won't work. If the request size is > (MaxRequestBytes + 1024), then grpc will return an error to client immediately.

I need to rework on handling ErrRequestTooLarge. It seems me there is no way to bypass the grpc request size check completely and let the etcd server to do the check for us.

ok, so should the server limit be MaxRequestBytes + grpcOverheadBytes with const grpcOverheadBytes = 512 * 1024 to be careful about the rpc overhead without totally abandoning the grpc request size checking?

that was what I was thinking. the code works as expected if request size < (MaxRequestBytes + grpcOverheadBytes).

If request size > MaxRequestBytes && request size < (MaxRequestBytes + grpcOverheadBytes), client expects an ErrRequestTooLarge error.

If request size > (MaxRequestBytes + grpcOverheadBytes), the client will receive an grpc error grpc: received message larger than max... instead.

I wonder whether this behavior is okay. client might recv two different errors that mean pretty much the same thing.

it already behaves that way now, doesn't it?

you are right.

heyitsanthony · 2017-05-24T17:49:50Z

etcdmain/config.go

@@ -138,6 +138,7 @@ func newConfig() *config {
 	fs.UintVar(&cfg.TickMs, "heartbeat-interval", cfg.TickMs, "Time (in milliseconds) of a heartbeat interval.")
 	fs.UintVar(&cfg.ElectionMs, "election-timeout", cfg.ElectionMs, "Time (in milliseconds) for an election to timeout.")
 	fs.Int64Var(&cfg.QuotaBackendBytes, "quota-backend-bytes", cfg.QuotaBackendBytes, "Raise alarms when backend size exceeds the given quota. 0 means use the default quota.")
+	fs.UintVar(&cfg.MaxRequestBytes, "max-request-bytes", cfg.MaxRequestBytes, "Maximum request size that raft accepts; defaults to 1.5 MB. be aware that large request size can incur performance penalty.")


Just Maximum client request size in bytes the server will accept.?

The default shouldn't be in the help message since it's already printed with etcd --help if cfg.MaxRequestBytes is set to the default. The warning probably isn't needed either-- when wouldn't permitting bigger requests be more expensive?

heyitsanthony · 2017-05-24T17:50:22Z

etcdmain/help.go

@@ -66,6 +66,8 @@ member flags:
 		comma-separated whitelist of origins for CORS (cross-origin resource sharing).
 	--quota-backend-bytes '0'
 		raise alarms when backend size exceeds the given quota (0 defaults to low space quota).
+	--max-request-bytes '1572864'
+		maximum request size that raft accepts; defaults to 1.5 MB. be aware that large request size can incur performance penalty.


update this too, the default is already given in the line above

heyitsanthony · 2017-05-24T17:53:46Z

etcdserver/config.go

@@ -54,6 +54,8 @@ type ServerConfig struct {

 	AutoCompactionRetention int
 	QuotaBackendBytes       int64
+	// MaxRequestBytes is the max request size that raft accepts.


MaxRequestBytes is the maximum request size to send over raft.

raft will accept whatever is sent to it, this check is only API-level

fanminshi · 2017-05-24T19:59:25Z

All fixed PTAL

heyitsanthony

some last nits about the test

heyitsanthony · 2017-05-24T21:17:35Z

integration/v3_grpc_test.go

+		_, err := kvcli.Put(context.TODO(), reqput)
+		hasError := (err != nil)
+		if hasError != test.expectError {
+			t.Fatalf("expected error? %v, but got %v case:[%+v] \n", test.expectError, hasError, test)


test loops like this print the test number first in the error message:

for i, test := range tests { ... if hasError != test.expectError { t.Errorf("#%d: expected error %v, got %v", i, test.expectError, err)` } ...

}

heyitsanthony · 2017-05-24T21:21:01Z

integration/v3_grpc_test.go

+		key             string
+		maxRequestBytes uint
+		valueSize       int
+		expectError     bool


Use the error string here?

expectError error ... {..., rpctypes.ErrGRPCRequestTooLarge}, {..., nil}, } ... if !eqErrGRPC(err, rpctypes.ErrGRPCRequestTooLarge) { t.Errorf(...) }

fanminshi · 2017-05-24T21:50:16Z

All fixed.

heyitsanthony suggested changes May 23, 2017

View reviewed changes

heyitsanthony added this to the v3.3.0 milestone May 23, 2017

heyitsanthony reviewed May 23, 2017

View reviewed changes

gyuho added the kind/release-note label May 23, 2017

fanminshi force-pushed the make_maxRequestBytes_configurable branch 3 times, most recently from 67919ef to ee5e99a Compare May 23, 2017 20:17

fanminshi force-pushed the make_maxRequestBytes_configurable branch 3 times, most recently from 1180f25 to ed664e7 Compare May 23, 2017 22:50

xiang90 reviewed May 24, 2017

View reviewed changes

fanminshi force-pushed the make_maxRequestBytes_configurable branch from ed664e7 to e09e6ae Compare May 24, 2017 17:37

heyitsanthony suggested changes May 24, 2017

View reviewed changes

fanminshi force-pushed the make_maxRequestBytes_configurable branch 2 times, most recently from 65d7e58 to 5d98f27 Compare May 24, 2017 19:58

fanminshi force-pushed the make_maxRequestBytes_configurable branch from 5d98f27 to 4a5a3f3 Compare May 24, 2017 20:24

heyitsanthony suggested changes May 24, 2017

View reviewed changes

fanminshi force-pushed the make_maxRequestBytes_configurable branch from 4a5a3f3 to 22a956b Compare May 24, 2017 21:46

heyitsanthony approved these changes May 25, 2017

View reviewed changes

fanminshi force-pushed the make_maxRequestBytes_configurable branch from 22a956b to c107f27 Compare May 25, 2017 17:59

fanminshi added 2 commits May 25, 2017 11:01

etcdserver: add --max-request-bytes flag

9e77400

v3rpc: change grpc max recv size as needed.

68a72c6

fanminshi force-pushed the make_maxRequestBytes_configurable branch from c107f27 to 68a72c6 Compare May 25, 2017 18:02

fanminshi merged commit 2f84f3d into etcd-io:master May 25, 2017

gyuho removed the kind/need-release-note-v3.3 label Nov 22, 2017

gyuho mentioned this pull request Jan 11, 2018

Documentation/dev-guide: update "Request size limit" with v3.2.12 #9129

Merged

mateuszdrab mentioned this pull request Feb 10, 2023

[BUG] rke2-etcd-snapshots configmap grows too large when snapshot retention is high rancher/rke2#4495

Closed

		@@ -36,7 +36,10 @@ func Server(s etcdserver.EtcdServer, tls tls.Config) *grpc.Server {
		}

etcd: make max request bytes configurable #7968

etcd: make max request bytes configurable #7968

Conversation

fanminshi commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi commented May 24, 2017

heyitsanthony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi commented May 24, 2017

fanminshi May 23, 2017 •

edited

Loading

fanminshi May 24, 2017 •

edited

Loading