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

etcd: make max request bytes configurable #7968

Merged
merged 2 commits into from
May 25, 2017

Conversation

fanminshi
Copy link
Member

Fixes #7923

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

why export this?

Copy link
Member Author

Choose a reason for hiding this comment

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

no need to export. it was a typo.

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

// maxPendingRevokes is the maximum number of outstanding expired lease revocations.
maxPendingRevokes = 16

defaultMaxRequestBytes = 1.5 * 1024 * 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

AutoCompactionRetention int
QuotaBackendBytes int64
// the max request size that raft accepts.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

set this unconditionally instead of hardcoding 4MB?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

etcdmain/help.go Outdated
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@fanminshi fanminshi May 23, 2017

Choose a reason for hiding this comment

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

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.

embed/config.go Outdated
@@ -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"`
Copy link
Contributor

Choose a reason for hiding this comment

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

uint to avoid < 0 cases

@@ -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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

UintVar to avoid negatives

@heyitsanthony heyitsanthony added this to the v3.3.0 milestone May 23, 2017
@@ -36,7 +36,10 @@ func Server(s *etcdserver.EtcdServer, tls *tls.Config) *grpc.Server {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

all fixed PTAL

@fanminshi fanminshi force-pushed the make_maxRequestBytes_configurable branch 3 times, most recently from 1180f25 to ed664e7 Compare May 23, 2017 22:50
_, err := cli.Put(context.TODO(), test.key, string(valueBytes))
hasError := (err != nil)
if hasError != test.expectError {
fmt.Printf("error %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this print

@@ -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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

maximum > Maximum

etcdmain/help.go Outdated
@@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

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

0-> 1,500,000?

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

// maxPendingRevokes is the maximum number of outstanding expired lease revocations.
maxPendingRevokes = 16

recommendedMaxRequestBytes = 10 * 1024 * 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

defaultMaxRequestBytes

Copy link
Contributor

Choose a reason for hiding this comment

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

oh. i see. nvm

@fanminshi fanminshi force-pushed the make_maxRequestBytes_configurable branch from ed664e7 to e09e6ae Compare May 24, 2017 17:37
@fanminshi
Copy link
Member Author

All fixed. PTAL

@@ -895,3 +895,31 @@ func TestKVGetResetLoneEndpoint(t *testing.T) {
case <-donec:
}
}

// TestKVPutLargeRequests ensures that custom MaxRequestBytes works as intended.
func TestKVPutLargeRequests(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

s/MaxRequestBytes/maxRequestBytes

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@fanminshi fanminshi May 24, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right.

@@ -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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

etcdmain/help.go Outdated
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

AutoCompactionRetention int
QuotaBackendBytes int64
// MaxRequestBytes is the max request size that raft accepts.
Copy link
Contributor

Choose a reason for hiding this comment

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

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 fanminshi force-pushed the make_maxRequestBytes_configurable branch 2 times, most recently from 65d7e58 to 5d98f27 Compare May 24, 2017 19:58
@fanminshi
Copy link
Member Author

All fixed PTAL

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

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

some last nits about the test

_, 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

key string
maxRequestBytes uint
valueSize int
expectError bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the error string here?

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

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

All fixed.

@fanminshi fanminshi force-pushed the make_maxRequestBytes_configurable branch from 22a956b to c107f27 Compare May 25, 2017 17:59
@fanminshi fanminshi force-pushed the make_maxRequestBytes_configurable branch from c107f27 to 68a72c6 Compare May 25, 2017 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants