diff --git a/CHANGELOG.md b/CHANGELOG.md index b336932015..3140fcd7ba 100644 --- a/CHANGELOG.md +++ b/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` diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md index cf80e1becd..240e81e2df 100644 --- a/docs/configuration/config-file-reference.md +++ b/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: | default = 5m] +[grpc_server_min_time_between_pings: | 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: | default = false] +[grpc_server_ping_without_stream_allowed: | default = true] # Output log messages in the given format. Valid formats: [logfmt, json] # CLI flag: -log.format diff --git a/pkg/cortex/cortex.go b/pkg/cortex/cortex.go index a681d21ca7..6ef83cb971 100644 --- a/pkg/cortex/cortex.go +++ b/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 diff --git a/pkg/cortex/cortex_test.go b/pkg/cortex/cortex_test.go index 1df7baec5e..84a65b4db6 100644 --- a/pkg/cortex/cortex_test.go +++ b/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")