-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
embed: add GRPCAdditionalServerOptions
config
#14066
Conversation
Codecov Report
@@ Coverage Diff @@
## main #14066 +/- ##
==========================================
- Coverage 75.60% 75.27% -0.34%
==========================================
Files 457 457
Lines 37084 37117 +33
==========================================
- Hits 28038 27939 -99
- Misses 7312 7414 +102
- Partials 1734 1764 +30
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Comments:
|
|
server/etcdserver/api/v3rpc/grpc.go
Outdated
@@ -66,8 +62,8 @@ func Server(s *etcdserver.EtcdServer, tls *tls.Config, interceptor grpc.UnarySer | |||
opts = append(opts, grpc.UnaryInterceptor(grpc_middleware.ChainUnaryServer(chainUnaryInterceptors...))) | |||
opts = append(opts, grpc.StreamInterceptor(grpc_middleware.ChainStreamServer(chainStreamInterceptors...))) | |||
|
|||
opts = append(opts, grpc.MaxRecvMsgSize(int(s.Cfg.MaxRequestBytes+grpcOverheadBytes))) | |||
opts = append(opts, grpc.MaxSendMsgSize(maxSendBytes)) | |||
opts = append(opts, grpc.MaxRecvMsgSize(s.Cfg.MaxRecvMsgSize)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if MaxRequestBytes
is different from s.Cfg.MaxRecvMsgSize
? If you configure a big value for s.Cfg.MaxRecvMsgSize
, i.e. 10MB, while the MaxRequestBytes
is 1 MB, then etcdserver may still reject the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean MaxRecvMsgSize
should be larger or equal to MaxRequestBytes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason to add the two parameters.
Did you see any issue without adding MaxRecvMsgSize/MaxSendMsgSize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we have a feature that needs to change MaxRecvMsgSize
to 150M. I was wondering if etcd can offer a separate config to customize it without changing MaxRequestBytes
. For now, when we start a cluster, it will give a warning about the MaxRequestBytes
exceeds the recommended size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments:
- etcd is designed to serve small metadata. Why do you need to change
MaxRecvMsgSize
to 150M? Can you elaborate your use case? - Have you verified your case based on this PR with
MaxRecvMsgSize
being set to 150M, while keep--max-request-bytes
unchanged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any update on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply.
- etcd is designed to serve small metadata. Why do you need to change MaxRecvMsgSize to 150M? Can you elaborate your use case?
We will register our service into the embed etcd. See https://github.com/tikv/pd/blob/0d05fba64372408c5af9a78cd9b47b58c4925ca9/server/server.go#L277-L280. For example, the store heartbeat https://github.com/tikv/pd/blob/0d05fba64372408c5af9a78cd9b47b58c4925ca9/server/grpc_service.go#L570.When we need to recover our cluster, the heartbeat request will bring the meta of data, which could be large. We utilize the meta to decide which part of the data should be recovered.
- Have you verified your case based on this PR with MaxRecvMsgSize being set to 150M, while keep --max-request-bytes unchanged?
not yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider registering a hook to alter the grpc options as part of the config:
func additionalGrpcServerOptions() grpc.ServerOption[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually:
grpcAdditionalServerOptions grpc.ServerOption[]
in the config would do the trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grpcAdditionalServerOptions grpc.ServerOption[]
server/config/config.go
Outdated
@@ -129,6 +129,11 @@ type ServerConfig struct { | |||
// MaxRequestBytes is the maximum request size to send over raft. | |||
MaxRequestBytes uint | |||
|
|||
// MaxRecvMsgSize is the max gRPC message size in bytes the server can receive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding
grpcAdditionalServerOptions grpc.ServerOption[]
as a generic options instead ?
@ptabor One item in my to do list is to sort out all the thresholds in etcd, including this one. Will discuss this in a separate session once my current task of refactoring lease is done. |
@ahrtr As this sound as a snowflake use-case (grpc shared between etcd and other service), I think we should not be in the business of looking at the limits... and allow programmers (only in embed server) to pass additional grpc settings, that etcd passes-through. |
Overall adding a generic The good side is flexibility, because users can set any additional options they want. The down side is it may have impact on the gRPC service provided by etcd itself. Another perspective to think about this is that we should take all services, including etcd build-in services and users registered services, under control. Once users see any issue for the embedded etcd, they will also raise issue in etcd community. Based on the principle of equal responsibility and obligation, If we need to support such case, then we should can add more restriction on the services registered by users, instead of just providing flexibility without any restriction. So users shouldn't set any additional options, instead they should follow all the existing options from this perspective. |
The new feature of ServiceRegister was introduced in pull/7215. Again, the good side of the PR is users can reuse the same port as etcd. But the bad side is it introduces additional unnecessary complexity to etcd. Personally I don't like the PR ( I mean 7215), and the better choice should be resolving it out of the box (such as providing a proxy/gateway in front of embedded etcd and users' gRPC service) instead of hacking etcd itself. |
MaxRecvMsgSize/MaxSendMsgSize
configGRPCAdditionalServerOptions
config
IMO, no matter if it's a requirement of user-defined services, we still need to provide the ability to change the default gRPC settings. |
Besides, supporting the custom interceptor for embed etcd is another need. See #13468 |
Note that the users registered service and the etcd build-in gRPC services share the same gRPC channel. Any options will have impact on both of them. Please just use the existing flag |
Hi @ahrtr We have some requirements with the
IMO, |
server/embed/etcd.go
Outdated
@@ -719,6 +719,7 @@ func (e *Etcd) serveClients() (err error) { | |||
Time: e.cfg.GRPCKeepAliveInterval, | |||
Timeout: e.cfg.GRPCKeepAliveTimeout, | |||
})) | |||
gopts = append(gopts, e.cfg.GRPCAdditionalServerOptions...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to move out this if statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
server/embed/etcd.go
Outdated
@@ -719,6 +719,7 @@ func (e *Etcd) serveClients() (err error) { | |||
Time: e.cfg.GRPCKeepAliveInterval, | |||
Timeout: e.cfg.GRPCKeepAliveTimeout, | |||
})) | |||
gopts = append(gopts, e.cfg.GRPCAdditionalServerOptions...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
GrpcURL string | ||
GrpcBridge *bridge | ||
GrpcServerOpts []grpc.ServerOption | ||
GrpcAdditionalServerOpts []grpc.ServerOption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need it here ?
I think that MustNewMember
could just propagate MemberConfig.GrpcAdditionalServerOpts
into GrpcServerOpts
and not store it here explicitly.
I am still not convinced that we really need to add Note that there are several QoS related PRs, but none of them is merged so far. If your discussion is based on the QoS feature, then it isn't valid at the moment. More flexibility also means more chance to make mistake. If there is no any strong incentive or reasonable benefit, then we shouldn't add it. Please also make sure you well understood my previous two comments, issuecomment-1155881593 and issuecomment-1155894742 |
https://github.com/grpc/grpc-go/blob/28de4866ce7440b675662abbdd5c43b476bd4dae/server.go#L124 |
Thanks @ptabor for the feedback. I read the 14066#discussion_r887444999 before. etcd has already supported users registering custom service. If users need to configure the max request size, we already have MaxRequestBytes ( FYI. there is a on-going PR 14081 for the There are 24 options in total which are supported by gRPC 1.47 for now, see server.go#L143-L168 . Currently etcd exposes about half of them. Some options/API (such as
The number of options will not explode. Please see my comment above.
This might be a valid point. |
@ahrtr @ptabor Thanks for your reply. #14066 (comment) make sense to me. But for now, we indeed have two requirements:
|
I have some questions:
I think there still have the same problems. so if the |
BTW, If you really concert about it, I think etcd |
I am OK to revisit this PR, but it needs at least three maintainers' approval. |
9f9ce3f
to
6a6b493
Compare
Sorry for the late response. Please rebase this PR. |
done |
Signed-off-by: Ryan Leung <[email protected]>
server/embed/config.go
Outdated
// GRPCAdditionalServerOptions is the additional server option hook | ||
// for changing the default internal gRPC configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// GRPCAdditionalServerOptions is the additional server option hook | |
// for changing the default internal gRPC configuration. | |
// GRPCAdditionalServerOptions is the additional server option hook | |
// for changing the default internal gRPC configuration. Note these | |
// additional configurations take precedence over the existing individual | |
// configurations if present. Please refer to | |
// https://github.com/etcd-io/etcd/pull/14066#issuecomment-1248682996 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Overall looks good to me. cc @fuweid @jmhbnz @serathius @tjungblu |
same. /lgtm |
Signed-off-by: Ryan Leung <[email protected]>
@rleungx FYI. #18015 (comment) |
Got it. |
@ahrtr Is there anything I need to do to get this PR merged? |
// GRPCAdditionalServerOptions is the additional server option hook | ||
// for changing the default internal gRPC configuration. Note these | ||
// additional configurations take precedence over the existing individual | ||
// configurations if present. Please refer to | ||
// https://github.com/etcd-io/etcd/pull/14066#issuecomment-1248682996 | ||
GRPCAdditionalServerOptions []grpc.ServerOption `json:"grpc-additional-server-options"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if we also add a page under the https://etcd.io/docs/v3.5/dev-guide/ to document this. Can you raise a ticket in https://github.com/etcd-io/website/issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been reviewing this pull request back and forth. The implementation looks good. But I see some previous discussion about whether this is a feature we want to bring or not. It seems like the latest is that there is a consensus. Maybe it would be good to hear from other team members, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this @rleungx.
Given this is a more complex configuration option I suggest adding a commented out example in https://github.com/etcd-io/etcd/blob/main/etcd.conf.yml.sample showing how this option could be configured via configuration file if desired.
The new option is only for the embedded use case. Users won't be able to configure it through the command line or config file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarification @ahrtr.
LGTM
Please also update changelog-3.6, which can be done in a separate PR. @rleungx |
Sure. Once it is merged, I will raise another PR to update it. |
@ahrtr Sorry to bother you, can this PR accept a pick back to 3.5? I can submit a PR for review. Any suggestions are valuable to me. |
We may not backport this (feature) to stable release without strong justification. |
This PR introduces GRPCAdditionalServerOptions which allow changing the internal gRPC settings. Sometimes, we may register our own gRPC service into
etcd
and change themax-request-bytes
might affect the internal etcd logic.