clientv3: add keep-alive to connection #8199

sakshamsharma · 2017-06-30T23:14:14Z

this makes the grpc client connection use a keep-alive.

codecov-io · 2017-07-01T00:22:17Z

Codecov Report

Merging #8199 into master will decrease coverage by 0.13%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #8199      +/-   ##
==========================================
- Coverage    76.4%   76.26%   -0.14%     
==========================================
  Files         344      344              
  Lines       26886    26891       +5     
==========================================
- Hits        20541    20509      -32     
- Misses       4865     4907      +42     
+ Partials     1480     1475       -5

Impacted Files	Coverage Δ
clientv3/client.go	`82.68% <0%> (-1.49%)`	⬇️
clientv3/namespace/watch.go	`87.87% <0%> (-12.13%)`	⬇️
pkg/adt/interval_tree.go	`81.08% <0%> (-6.61%)`	⬇️
etcdserver/api/v3election/election.go	`64.7% <0%> (-2.95%)`	⬇️
clientv3/watch.go	`93% <0%> (-2.25%)`	⬇️
rafthttp/peer.go	`91.72% <0%> (-1.51%)`	⬇️
clientv3/balancer.go	`95.08% <0%> (-1.1%)`	⬇️
clientv3/lease.go	`91.57% <0%> (-0.77%)`	⬇️
etcdserver/server.go	`79.6% <0%> (-0.7%)`	⬇️
rafthttp/stream.go	`88.25% <0%> (-0.68%)`	⬇️
... and 10 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 b7cf080...c58464e. Read the comment docs.

xiang90 · 2017-07-01T05:41:25Z

@sakshamsharma I have not got time to check gRPC doc. Can you help me to understand if this is TPC keepalive or HTTP2 layer keepalive? Can we also enforce keepalive at server side for gRPC connections?

sakshamsharma · 2017-07-05T18:25:39Z

This is the connection which takes the KeepAlive as a DialOption (here). It seems to be a gRPC connection, I'm not sure about the underlying connection (there is nothing explicit, like there used to be for the older client)

I tried to make it look like the older client's keepalive. It used to have an explicit http connection as a parameter, which was a struct with a KeepAlive member. In this case, ClientConn does support KeepAlive, so I believe that's the place to enforce it. I'm sorry if I missed addressing something, I could not understand what you meant to ask, by your last sentence.

xiang90 · 2017-07-05T18:35:57Z

It seems to be a gRPC connection, I'm not sure about the underlying connection (there is nothing explicit, like there used to be for the older client)

We need to figure out what layer is this keepalive at. We need to have clear documentation at etcd client side.

I'm sorry if I missed addressing something, I could not understand what you meant to ask, by your last sentence.

see https://godoc.org/google.golang.org/grpc#KeepaliveParams.

heyitsanthony · 2017-07-05T18:46:51Z

@xiang90 it's http2 ping frames

xiang90 · 2017-07-05T18:54:28Z

clientv3/client.go

@@ -215,6 +216,12 @@ func (c *Client) dialSetupOpts(endpoint string, dopts ...grpc.DialOption) (opts
 	if c.cfg.DialTimeout > 0 {
 		opts = []grpc.DialOption{grpc.WithTimeout(c.cfg.DialTimeout)}
 	}
+	if c.cfg.DialKeepAlive > 0 {
+		opts = append(opts, grpc.WithKeepaliveParams(keepalive.ClientParameters{
+			Time:    c.cfg.DialKeepAlive * time.Second,


why do we set time and timeout the same?

It defaults to 20 seconds. Doc link

Perhaps I should remove Timeout altogether, since 20 seconds should be enough for a response.

xiang90 · 2017-07-05T20:26:34Z

clientv3/config.go

@@ -33,6 +33,10 @@ type Config struct {
 	// DialTimeout is the timeout for failing to establish a connection.
 	DialTimeout time.Duration `json:"dial-timeout"`

+	// DialKeepAlive is the time after which client pings the server to see if transport is
+	// alive. It waits for this duration for a response before closing the connection.
+	DialKeepAlive time.Duration `json:"dial-keep-alive"`


why not keep the naming similar to what grpc has. keepliave usually has two thing:

probe internal

timeout

users need to know what this time duration is and why we only let them set one of the two.

dial-keepalive-probe ?
We can add the timeout too though, if you think its useful.

Why not just make this similar to gRPC-go naming?

DailKeepAliveTime?

and why not expose the timeout option?

Okay. Sounds good.

xiang90 · 2017-07-05T20:36:35Z

defer to @heyitsanthony. i am a little bit worry about ending up with duplicating every gRPC option in our config structure. but i think we can try to resolve that later.

heyitsanthony · 2017-07-05T20:38:38Z

clientv3/client.go

@@ -215,6 +216,11 @@ func (c *Client) dialSetupOpts(endpoint string, dopts ...grpc.DialOption) (opts
 	if c.cfg.DialTimeout > 0 {
 		opts = []grpc.DialOption{grpc.WithTimeout(c.cfg.DialTimeout)}
 	}
+	if c.cfg.DialKeepAlive > 0 {
+		opts = append(opts, grpc.WithKeepaliveParams(keepalive.ClientParameters{
+			Time: c.cfg.DialKeepAlive * time.Second,


Time: c.cfg.DialKeepAlive, since it's already a time.Duration?

We want to read the input in seconds, but send it as time.Duration. time.Duration can be anything (it is nanoseconds in this case). Maybe I should change the type of c.cfg.DialKeepAliveTime to int to make it clear? (and add this to comments)

DialTimeout and AutoSyncInterval are also of type time.Duration but they do not multiply by time.Second-- this is inconsistent; just dropping the multiply should be fine

this makes the grpc client connection use a keep-alive.

sakshamsharma · 2017-07-06T19:56:44Z

@heyitsanthony Fixed that inconsistency.

heyitsanthony

lgtm thanks

sakshamsharma · 2017-07-06T20:40:30Z

Not sure about the CI here, but is that a flaky test?

sakshamsharma force-pushed the clientv3-keep-alive branch from 2e0ef6f to c58464e Compare June 30, 2017 23:21

sakshamsharma mentioned this pull request Jul 1, 2017

apiserver etcd v3 client does not close connections to dead etcd node kubernetes/kubernetes#46964

Closed

xiang90 reviewed Jul 5, 2017

View reviewed changes

sakshamsharma force-pushed the clientv3-keep-alive branch from c58464e to 09c8015 Compare July 5, 2017 20:25

xiang90 reviewed Jul 5, 2017

View reviewed changes

heyitsanthony suggested changes Jul 5, 2017

View reviewed changes

clientv3: add keep-alive to connection

2a30a75

this makes the grpc client connection use a keep-alive.

sakshamsharma force-pushed the clientv3-keep-alive branch from 09c8015 to 2a30a75 Compare July 6, 2017 19:56

heyitsanthony approved these changes Jul 6, 2017

View reviewed changes

gyuho added the kind/release-note label Jul 7, 2017

xiang90 merged commit acfde8a into etcd-io:master Jul 8, 2017

gyuho mentioned this pull request Jul 13, 2017

etcdmain: configure keep-alive policy from server #8258

Closed

timothysc mentioned this pull request Jul 21, 2017

Update to etcd 3.1.X kubernetes/kubernetes#49386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clientv3: add keep-alive to connection #8199

clientv3: add keep-alive to connection #8199

sakshamsharma commented Jun 30, 2017

codecov-io commented Jul 1, 2017

xiang90 commented Jul 1, 2017 •

edited

Loading

sakshamsharma commented Jul 5, 2017 •

edited

Loading

xiang90 commented Jul 5, 2017

heyitsanthony commented Jul 5, 2017

xiang90 Jul 5, 2017

sakshamsharma Jul 5, 2017

xiang90 Jul 5, 2017

sakshamsharma Jul 5, 2017

xiang90 Jul 5, 2017

sakshamsharma Jul 5, 2017

xiang90 commented Jul 5, 2017

heyitsanthony Jul 5, 2017

sakshamsharma Jul 5, 2017 •

edited

Loading

heyitsanthony Jul 6, 2017

sakshamsharma commented Jul 6, 2017

heyitsanthony left a comment

sakshamsharma commented Jul 6, 2017

clientv3: add keep-alive to connection #8199

clientv3: add keep-alive to connection #8199

Conversation

sakshamsharma commented Jun 30, 2017

codecov-io commented Jul 1, 2017

Codecov Report

xiang90 commented Jul 1, 2017 • edited Loading

sakshamsharma commented Jul 5, 2017 • edited Loading

xiang90 commented Jul 5, 2017

heyitsanthony commented Jul 5, 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

xiang90 commented Jul 5, 2017

Choose a reason for hiding this comment

sakshamsharma Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshamsharma commented Jul 6, 2017

heyitsanthony left a comment

Choose a reason for hiding this comment

sakshamsharma commented Jul 6, 2017

xiang90 commented Jul 1, 2017 •

edited

Loading

sakshamsharma commented Jul 5, 2017 •

edited

Loading

sakshamsharma Jul 5, 2017 •

edited

Loading