*: configure server keepalive #8535

gyuho · 2017-09-09T01:23:32Z

heyitsanthony · 2017-09-12T09:58:31Z

please rebase with the watch keepalive test when #8545 is merged; this can't be merged without tests

Also, the keepalive test can drop the time.Sleep calls:

+++ b/clientv3/integration/watch_keepalive_test.go
@@ -66,14 +66,12 @@ func TestWatchKeepAlive(t *testing.T) {
        // ep[0] keepalive time-out after DialKeepAliveTime + DialKeepAliveTimeout
        // wait extra for processing network error for endpoint switching
        timeout := ccfg.DialKeepAliveTime + ccfg.DialKeepAliveTimeout + ccfg.DialTimeout
-       time.Sleep(timeout)
-
        if _, err = clus.Client(1).Put(context.TODO(), "foo", "bar"); err != nil {
                t.Fatal(err)
        }
        select {
        case <-wch:
-       case <-time.After(5 * time.Second):
+       case <-time.After(timeout):
                t.Fatal("took too long to receive events")
        }
 
@@ -82,14 +80,12 @@ func TestWatchKeepAlive(t *testing.T) {
        defer clus.Members[1].Unblackhole()
 
        // wait for ep[0] recover, ep[1] fail
-       time.Sleep(timeout)
-
        if _, err = clus.Client(0).Put(context.TODO(), "foo", "bar"); err != nil {
                t.Fatal(err)
        }
        select {
        case <-wch:
-       case <-time.After(5 * time.Second):
+       case <-time.After(timeout):
                t.Fatal("took too long to receive events")
        }
 }

xiang90 · 2017-09-22T20:19:28Z

what is the blocked on?

gyuho · 2017-09-23T00:38:20Z

@xiang90 This is blocked on #8581, failing a bunch of CIs since #8545 (see #8592). Will investigate more next week.

xiang90 · 2017-09-28T01:17:19Z

embed/config.go

+		SnapCount:            etcdserver.DefaultSnapCount,
+		MaxTxnOps:            DefaultMaxTxnOps,
+		MaxRequestBytes:      DefaultMaxRequestBytes,
+		GRPCKeepAliveMinTime: 5 * time.Second,


define this as a const?

xiang90 · 2017-09-28T01:18:27Z

etcdmain/config.go

@@ -143,6 +144,9 @@ func newConfig() *config {
 	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.MaxTxnOps, "max-txn-ops", cfg.MaxTxnOps, "Maximum number of operations permitted in a transaction.")
 	fs.UintVar(&cfg.MaxRequestBytes, "max-request-bytes", cfg.MaxRequestBytes, "Maximum client request size in bytes the server will accept.")
+	fs.DurationVar(&cfg.GRPCKeepAliveMinTime, "grpc-keepalive-min-time", cfg.Config.GRPCKeepAliveMinTime, "Minimum interval duration that a client should wait before pinging server.")
+	fs.DurationVar(&cfg.GRPCKeepAliveInterval, "grpc-keepalive-interval", time.Duration(0), "Frequency duration of server-to-client ping to check if a connection is alive.")


explain what does the default value do?

xiang90 · 2017-09-28T01:18:32Z

etcdmain/config.go

@@ -143,6 +144,9 @@ func newConfig() *config {
 	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.MaxTxnOps, "max-txn-ops", cfg.MaxTxnOps, "Maximum number of operations permitted in a transaction.")
 	fs.UintVar(&cfg.MaxRequestBytes, "max-request-bytes", cfg.MaxRequestBytes, "Maximum client request size in bytes the server will accept.")
+	fs.DurationVar(&cfg.GRPCKeepAliveMinTime, "grpc-keepalive-min-time", cfg.Config.GRPCKeepAliveMinTime, "Minimum interval duration that a client should wait before pinging server.")
+	fs.DurationVar(&cfg.GRPCKeepAliveInterval, "grpc-keepalive-interval", time.Duration(0), "Frequency duration of server-to-client ping to check if a connection is alive.")
+	fs.DurationVar(&cfg.GRPCKeepAliveTimeout, "grpc-keepalive-timeout", time.Duration(0), "Additional duration of wait before closing a non-responsive connection.")


i think we should find a good default value to enable keepalive related options.

Defined default interval 5s, timeout 3s.

xiang90 · 2017-09-28T01:24:11Z

clientv3/integration/watch_keepalive_test.go

+	clus.Members[0].Blackhole()
+
+	// expects endpoint switch to ep[1]
+	cli.SetEndpoints(clus.Members[0].GRPCAddr(), clus.Members[1].GRPCAddr())


will setenpoints cause a shuffle directly? or client will still pin to the original one?

It will trigger gRPC to close ep[0], and client will pin ep[1].

xiang90 · 2017-09-28T01:25:29Z

clientv3/integration/watch_keepalive_test.go

+
+	clus := integration.NewClusterV3(t, &integration.ClusterConfig{
+		Size:                  3,
+		GRPCKeepAliveMinTime:  time.Millisecond, // avoid too_many_pings


if we are testing the client-side keepalive here, why do we need to enable server-side keepalive? we probably can also move this test into another pr?

Removed server parameters for this test, leaving server-to-client ping test as TODO.

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>

gyuho · 2017-09-28T18:37:33Z

@xiang90 Used default values in grpc-go for keepalive server interval and timeout. PTAL.

xiang90 · 2017-09-28T19:26:53Z

etcdmain/help.go

@@ -70,6 +70,12 @@ member flags:
 		maximum number of operations permitted in a transaction.
 	--max-request-bytes '1572864'
 		maximum client request size in bytes the server will accept.
+	--grpc-keepalive-min-time '5s'
+		minimum duration interval that a client should wait before pinging server.
+	--grpc-keepalive-interval '2h'


2h seems too high. probably 5 minutes or something? is there a recommended vault from gRPC-go?

gRPC Go defaults to 2h. Should etcd use 5min?

2h is fine if it is gRPC's default.

xiang90 · 2017-09-28T19:58:26Z

lgtm

gyuho added the kind/release-note label Sep 9, 2017

gyuho added the WIP label Sep 12, 2017

gyuho force-pushed the keepalive-server branch from 496df50 to 4020dfa Compare September 18, 2017 21:42

gyuho changed the title *: configure server keepalive *: configure server keepalive, test watch keepalive Sep 18, 2017

gyuho force-pushed the keepalive-server branch 4 times, most recently from aeea06b to bfea25b Compare September 19, 2017 02:11

gyuho removed the WIP label Sep 27, 2017

gyuho force-pushed the keepalive-server branch from bfea25b to 6df878a Compare September 27, 2017 21:32

etcd-io deleted a comment from codecov-io Sep 27, 2017

gyuho force-pushed the keepalive-server branch 2 times, most recently from e41d020 to 47ae89e Compare September 27, 2017 23:21

xiang90 reviewed Sep 28, 2017

View reviewed changes

gyuho force-pushed the keepalive-server branch from 47ae89e to 955f1a1 Compare September 28, 2017 17:05

gyuho changed the title *: configure server keepalive, test watch keepalive *: configure server keepalive, test client watch keepalive Sep 28, 2017

gyuho force-pushed the keepalive-server branch from 955f1a1 to 75c1d4d Compare September 28, 2017 17:32

gyuho changed the title *: configure server keepalive, test client watch keepalive *: configure server keepalive Sep 28, 2017

gyuho force-pushed the keepalive-server branch from 75c1d4d to 3f51072 Compare September 28, 2017 17:39

api/rpc: accept grpc.ServerOption's for keepalive policy

32e15d7

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>

gyuho force-pushed the keepalive-server branch from 3f51072 to d3d7bb3 Compare September 28, 2017 17:57

gyuho added the WIP label Sep 28, 2017

gyuho force-pushed the keepalive-server branch from d3d7bb3 to 72d7693 Compare September 28, 2017 18:08

embed: define keepalive server options

157c8ec

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>

gyuho force-pushed the keepalive-server branch from 72d7693 to 7c8fa01 Compare September 28, 2017 18:22

etcdmain: add 'grpc-keepalive-*' flags

4b3d400

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>

gyuho force-pushed the keepalive-server branch from 7c8fa01 to 4b3d400 Compare September 28, 2017 18:24

gyuho removed the WIP label Sep 28, 2017

xiang90 reviewed Sep 28, 2017

View reviewed changes

gyuho merged commit 10202a5 into etcd-io:master Sep 28, 2017

gyuho deleted the keepalive-server branch September 28, 2017 20:26

gyuho mentioned this pull request Oct 4, 2017

grpc keepalive: test server-to-client HTTP/2 pings #8645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: configure server keepalive #8535

*: configure server keepalive #8535

gyuho commented Sep 9, 2017 •

edited

heyitsanthony commented Sep 12, 2017

xiang90 commented Sep 22, 2017

gyuho commented Sep 23, 2017 •

edited

xiang90 Sep 28, 2017

xiang90 Sep 28, 2017

xiang90 Sep 28, 2017

xiang90 Sep 28, 2017

gyuho Sep 28, 2017

xiang90 Sep 28, 2017

gyuho Sep 28, 2017

xiang90 Sep 28, 2017

gyuho Sep 28, 2017

gyuho commented Sep 28, 2017

xiang90 Sep 28, 2017

gyuho Sep 28, 2017

xiang90 Sep 28, 2017

xiang90 commented Sep 28, 2017

*: configure server keepalive #8535

*: configure server keepalive #8535

Conversation

gyuho commented Sep 9, 2017 • edited

heyitsanthony commented Sep 12, 2017

xiang90 commented Sep 22, 2017

gyuho commented Sep 23, 2017 • edited

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

gyuho commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented Sep 28, 2017

gyuho commented Sep 9, 2017 •

edited

gyuho commented Sep 23, 2017 •

edited