diff --git a/internal/internal.go b/internal/internal.go index 8fb17727cc74..c8a8c76d628c 100644 --- a/internal/internal.go +++ b/internal/internal.go @@ -38,6 +38,10 @@ var ( // KeepaliveMinPingTime is the minimum ping interval. This must be 10s by // default, but tests may wish to set it lower for convenience. KeepaliveMinPingTime = 10 * time.Second + // KeepaliveMinServerPingTime is the minimum ping interval for servers. + // This must be 1s by default, but tests may wish to set it lower for + // convenience. + KeepaliveMinServerPingTime = time.Second // ParseServiceConfig parses a JSON representation of the service config. ParseServiceConfig any // func(string) *serviceconfig.ParseResult // EqualServiceConfigForTesting is for testing service config generation and diff --git a/server.go b/server.go index 7ec70fda4caa..244123c6c5a8 100644 --- a/server.go +++ b/server.go @@ -290,9 +290,9 @@ func InitialConnWindowSize(s int32) ServerOption { // KeepaliveParams returns a ServerOption that sets keepalive and max-age parameters for the server. func KeepaliveParams(kp keepalive.ServerParameters) ServerOption { - if kp.Time > 0 && kp.Time < time.Second { + if kp.Time > 0 && kp.Time < internal.KeepaliveMinServerPingTime { logger.Warning("Adjusting keepalive ping interval to minimum period of 1s") - kp.Time = time.Second + kp.Time = internal.KeepaliveMinServerPingTime } return newFuncServerOption(func(o *serverOptions) { diff --git a/test/channelz_test.go b/test/channelz_test.go index c57ba8fd7a53..63f703c3128d 100644 --- a/test/channelz_test.go +++ b/test/channelz_test.go @@ -864,17 +864,6 @@ func doServerSideInitiatedFailedStreamWithGoAway(ctx context.Context, tc testgrp } } -func doIdleCallToInvokeKeepAlive(tc testgrpc.TestServiceClient, t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - _, err := tc.FullDuplexCall(ctx) - if err != nil { - t.Fatalf("TestService/FullDuplexCall(_) = _, %v, want ", err) - } - // Allow for at least 2 keepalives (1s per ping interval) - time.Sleep(4 * time.Second) - cancel() -} - func (s) TestCZClientSocketMetricsStreamsAndMessagesCount(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() @@ -1290,45 +1279,46 @@ func (s) TestCZServerSocketMetricsStreamsAndMessagesCount(t *testing.T) { } func (s) TestCZServerSocketMetricsKeepAlive(t *testing.T) { + defer func(t time.Duration) { internal.KeepaliveMinServerPingTime = t }(internal.KeepaliveMinServerPingTime) + internal.KeepaliveMinServerPingTime = 50 * time.Millisecond + czCleanup := channelz.NewChannelzStorageForTesting() defer czCleanupWrapper(czCleanup, t) e := tcpClearRREnv te := newTest(t, e) // We setup the server keepalive parameters to send one keepalive every - // second, and verify that the actual number of keepalives is very close to - // the number of seconds elapsed in the test. We had a bug wherein the - // server was sending one keepalive every [Time+Timeout] instead of every - // [Time] period, and since Timeout is configured to a low value here, we - // should be able to verify that the fix works with the above mentioned - // logic. + // 50ms, and verify that the actual number of keepalives is very close to + // Time/50ms. We had a bug wherein the server was sending one keepalive + // every [Time+Timeout] instead of every [Time] period, and since Timeout + // is configured to a high value here, we should be able to verify that the + // fix works with the above mentioned logic. kpOption := grpc.KeepaliveParams(keepalive.ServerParameters{ - Time: time.Second, - Timeout: 100 * time.Millisecond, + Time: 50 * time.Millisecond, + Timeout: 5 * time.Second, }) te.customServerOptions = append(te.customServerOptions, kpOption) te.startServer(&testServer{security: e.security}) defer te.tearDown() cc := te.clientConn() - tc := testgrpc.NewTestServiceClient(cc) - start := time.Now() - doIdleCallToInvokeKeepAlive(tc, t) - if err := verifyResultWithDelay(func() (bool, error) { - ss, _ := channelz.GetServers(0, 0) - if len(ss) != 1 { - return false, fmt.Errorf("there should be one server, not %d", len(ss)) - } - ns, _ := channelz.GetServerSockets(ss[0].ID, 0, 0) - if len(ns) != 1 { - return false, fmt.Errorf("there should be one server normal socket, not %d", len(ns)) - } - wantKeepalivesCount := int64(time.Since(start).Seconds()) - 1 - if gotKeepalivesCount := ns[0].SocketData.KeepAlivesSent; gotKeepalivesCount != wantKeepalivesCount { - return false, fmt.Errorf("got keepalivesCount: %v, want keepalivesCount: %v", gotKeepalivesCount, wantKeepalivesCount) - } - return true, nil - }); err != nil { - t.Fatal(err) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + awaitState(ctx, t, cc, connectivity.Ready) + + // Allow about 5 pings to happen (250ms/50ms). + time.Sleep(255 * time.Millisecond) + + ss, _ := channelz.GetServers(0, 0) + if len(ss) != 1 { + t.Fatalf("there should be one server, not %d", len(ss)) + } + ns, _ := channelz.GetServerSockets(ss[0].ID, 0, 0) + if len(ns) != 1 { + t.Fatalf("there should be one server normal socket, not %d", len(ns)) + } + const wantMin, wantMax = 3, 7 + if got := ns[0].SocketData.KeepAlivesSent; got < wantMin || got > wantMax { + t.Fatalf("got keepalivesCount: %v, want keepalivesCount: [%v,%v]", got, wantMin, wantMax) } }