Change default values for gRPC keepalive ping options. (#4168)

* Change default values for gRPC keepalive ping options.

Signed-off-by: Peter Štibraný <[email protected]>

* Fix changelog entry PR number.

Signed-off-by: Peter Štibraný <[email protected]>

* Updated documentation.

Signed-off-by: Peter Štibraný <[email protected]>

Author: pstibrany

CHANGELOG.md

@@ -7,6 +7,7 @@
   - `-alertmanager.receivers-firewall.block.cidr-networks` renamed to `-alertmanager.receivers-firewall-block-cidr-networks`
   - `-alertmanager.receivers-firewall.block.private-addresses` renamed to `-alertmanager.receivers-firewall-block-private-addresses`
 * [CHANGE] Distributor: Added ring status section in the admin page #4151
+* [CHANGE] Change default value of `-server.grpc.keepalive.min-time-between-pings` to `10s` and `-server.grpc.keepalive.ping-without-stream-allowed` to `true`. #4168
 * [ENHANCEMENT] Alertmanager: introduced new metrics to monitor operation when using `-alertmanager.sharding-enabled`: #4149
   * `cortex_alertmanager_state_fetch_replica_state_total`
   * `cortex_alertmanager_state_fetch_replica_state_failed_total`

docs/configuration/config-file-reference.md

@@ -387,13 +387,13 @@ grpc_tls_config:
 # If client sends keepalive ping more often, server will send GOAWAY and close
 # the connection.
 # CLI flag: -server.grpc.keepalive.min-time-between-pings
-[grpc_server_min_time_between_pings: <duration> | default = 5m]
+[grpc_server_min_time_between_pings: <duration> | default = 10s]
 
 # If true, server allows keepalive pings even when there are no active
 # streams(RPCs). If false, and client sends ping when there are no active
 # streams, server will send GOAWAY and close the connection.
 # CLI flag: -server.grpc.keepalive.ping-without-stream-allowed
-[grpc_server_ping_without_stream_allowed: <boolean> | default = false]
+[grpc_server_ping_without_stream_allowed: <boolean> | default = true]
 
 # Output log messages in the given format. Valid formats: [logfmt, json]
 # CLI flag: -log.format

pkg/cortex/cortex.go

@@ -142,7 +142,7 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
 	f.StringVar(&c.HTTPPrefix, "http.prefix", "/api/prom", "HTTP path prefix for Cortex API.")
 
 	c.API.RegisterFlags(f)
-	c.Server.RegisterFlags(f)
+	c.registerServerFlagsWithChangedDefaultValues(f)
 	c.Distributor.RegisterFlags(f)
 	c.Querier.RegisterFlags(f)
 	c.IngesterClient.RegisterFlags(f)
@@ -281,6 +281,27 @@ func (c *Config) validateYAMLEmptyNodes() error {
 	return nil
 }
 
+func (c *Config) registerServerFlagsWithChangedDefaultValues(fs *flag.FlagSet) {
+	throwaway := flag.NewFlagSet("throwaway", flag.PanicOnError)
+
+	// Register to throwaway flags first. Default values are remembered during registration and cannot be changed,
+	// but we can take values from throwaway flag set and reregister into supplied flags with new default values.
+	c.Server.RegisterFlags(throwaway)
+
+	throwaway.VisitAll(func(f *flag.Flag) {
+		// Ignore errors when setting new values. We have a test to verify that it works.
+		switch f.Name {
+		case "server.grpc.keepalive.min-time-between-pings":
+			_ = f.Value.Set("10s")
+
+		case "server.grpc.keepalive.ping-without-stream-allowed":
+			_ = f.Value.Set("true")
+		}
+
+		fs.Var(f.Value, f.Name, f.Usage)
+	})
+}
+
 // Cortex is the root datastructure for Cortex.
 type Cortex struct {
 	Cfg Config

pkg/cortex/cortex_test.go

@@ -1,13 +1,19 @@
 package cortex
 
 import (
+	"bytes"
 	"context"
+	"flag"
+	"io"
 	"net"
 	"net/url"
 	"strconv"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/weaveworks/common/server"
 	"go.uber.org/atomic"
@@ -203,6 +209,51 @@ func TestGrpcAuthMiddleware(t *testing.T) {
 	}
 }
 
+func TestFlagDefaults(t *testing.T) {
+	c := Config{}
+
+	f := flag.NewFlagSet("test", flag.PanicOnError)
+	c.RegisterFlags(f)
+
+	buf := bytes.Buffer{}
+
+	f.SetOutput(&buf)
+	f.PrintDefaults()
+
+	const delim = '\n'
+
+	minTimeChecked := false
+	pingWithoutStreamChecked := false
+	for {
+		line, err := buf.ReadString(delim)
+		if err == io.EOF {
+			break
+		}
+
+		require.NoError(t, err)
+
+		if strings.Contains(line, "-server.grpc.keepalive.min-time-between-pings") {
+			nextLine, err := buf.ReadString(delim)
+			require.NoError(t, err)
+			assert.Contains(t, nextLine, "(default 10s)")
+			minTimeChecked = true
+		}
+
+		if strings.Contains(line, "-server.grpc.keepalive.ping-without-stream-allowed") {
+			nextLine, err := buf.ReadString(delim)
+			require.NoError(t, err)
+			assert.Contains(t, nextLine, "(default true)")
+			pingWithoutStreamChecked = true
+		}
+	}
+
+	require.True(t, minTimeChecked)
+	require.True(t, pingWithoutStreamChecked)
+
+	require.Equal(t, true, c.Server.GRPCServerPingWithoutStreamAllowed)
+	require.Equal(t, 10*time.Second, c.Server.GRPCServerMinTimeBetweenPings)
+}
+
 // Generates server config, with gRPC listening on random port.
 func getServerConfig(t *testing.T) server.Config {
 	listen, err := net.Listen("tcp", "localhost:0")