Add XDS keepalives #1747
Add XDS keepalives #1747
Conversation
| @@ -125,6 +125,11 @@ static_resources: | |||
| http2_protocol_options: {} | |||
| name: xds_cluster | |||
| type: STRICT_DNS | |||
| upstream_connection_options: | |||
| tcp_keepalive: | |||
| keepalive_probes: 3 | |||
There was a problem hiding this comment.
where are these number came from?
There was a problem hiding this comment.
I used the same ones as Contour, but we can use anything really
There was a problem hiding this comment.
it's OK now, just want to clearify.
There was a problem hiding this comment.
since we control the server (xds server https://github.com/envoyproxy/gateway/blob/72bfe9bde95015b6731857f52ea30d27ee85145d/internal/xds/server/runner/runner.go#L94C12-L94C22 & https://pkg.go.dev/net#ListenConfig) and client (this envoy cluster), can we use higher values to reduce cpu consumption ?
There was a problem hiding this comment.
how does controlling the server help? the goal is for the client detect half closed situation fairly quickly. i think the server will detect this anyway whenever it sends an update. i dont feel strongly about what values we use though, if you have a preference i am all ears!
There was a problem hiding this comment.
awesome thanks for doing this in the client/envoy, this will also need to be done in the peer/server
grpc.KeepaliveParams
https://pkg.go.dev/google.golang.org/grpc/keepalive#ServerParameters
There was a problem hiding this comment.
This is for server detecting client going away, right? I feel its a separate discussion, I suppose the goal would be saving some resources on server side, but it won't matter for clients, right?
There was a problem hiding this comment.
for grpc keep alive the server needs to be set up with similar grpc keep alive settings, else the client might receive GO_AWAY ENHANCE_YOUR_CALM
https://grpc.io/docs/guides/keepalive/#background
I think its fine to set it to 30s and 5s timeout, these can be revisited later
Codecov Report
@@ Coverage Diff @@
## main #1747 +/- ##
==========================================
+ Coverage 64.88% 64.92% +0.04%
==========================================
Files 84 84
Lines 12192 12195 +3
==========================================
+ Hits 7911 7918 +7
+ Misses 3774 3771 -3
+ Partials 507 506 -1
|
|
hey @jackkleeman can you also force push with a signed commit since DCO is failing |
jackkleeman
force-pushed
the
issue-1746
branch
2 times, most recently
from
6743c7f
to
3ed5553
Compare
|
done @arkodg |
jackkleeman
force-pushed
the
issue-1746
branch
2 times, most recently
from
b118f59
to
ded1dec
Compare
Fixes envoyproxy#1746 Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
|
I can't merge - please go ahead when happy to |
Fixes #1746