From ec3fd30cb0bec8cc6c08a3208698e8de8eaef5cf Mon Sep 17 00:00:00 2001 From: Goutham Veeramachaneni Date: Mon, 30 Mar 2020 18:43:43 +0200 Subject: [PATCH] Saner defaults for configs (#2344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Saner defaults for configs Signed-off-by: Goutham Veeramachaneni * Set spread_flushes to true in the code. Signed-off-by: Tom Wilkie * Make mockIngester implement HealthCheck and io.Closer so tests pass. Signed-off-by: Tom Wilkie * Address feedback Signed-off-by: Goutham Veeramachaneni * Verify that querier sees correct ring before using it. Signed-off-by: Peter Štibraný * Address feedback Signed-off-by: Goutham Veeramachaneni Co-authored-by: Tom Wilkie Co-authored-by: Peter Štibraný --- CHANGELOG.md | 11 +++++++++++ docs/configuration/arguments.md | 2 +- docs/configuration/config-file-reference.md | 18 +++++++++--------- docs/guides/running.md | 10 ---------- integration/ingester_hand_over_test.go | 10 ++++++++++ pkg/chunk/cache/memcached.go | 2 +- pkg/chunk/cache/memcached_client.go | 2 +- pkg/distributor/distributor_test.go | 8 ++++++++ pkg/ingester/client/pool.go | 2 +- pkg/ingester/ingester.go | 4 ++-- pkg/querier/querier.go | 4 ++-- pkg/ring/kv/consul/client.go | 4 ++-- 12 files changed, 48 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac20ee46e..9af8e1eba7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,17 @@ * [CHANGE] Frontend worker in querier now starts after all Querier module dependencies are started. This fixes issue where frontend worker started to send queries to querier before it was ready to serve them (mostly visible when using experimental blocks storage). #2246 * [CHANGE] Lifecycler component now enters Failed state on errors, and doesn't exit the process. (Important if you're vendoring Cortex and use Lifecycler) #2251 * [CHANGE] `/ready` handler now returns 200 instead of 204. #2330 +* [CHANGE] Better defaults for the following options: #2344 + - `-.consul.consistent-reads`: Old default: `true`, new default: `false`. This reduces the load on Consul. + - `-.consul.watch-rate-limit`: Old default: 0, new default: 1. This rate limits the reads to 1 per second. Which is good enough for ring watches. + - `-distributor.health-check-ingesters`: Old default: `false`, new default: `true`. + - `-ingester.max-stale-chunk-idle`: Old default: 0, new default: 2m. This lets us expire series that we know are stale early. + - `-ingester.spread-flushes`: Old default: false, new default: true. This allows to better de-duplicate data and use less space. + - `-ingester.chunk-age-jitter`: Old default: 20mins, new default: 0. This is to enable the `-ingester.spread-flushes` to true. + - `-.memcached.batchsize`: Old default: 0, new default: 1024. This allows batching of requests and keeps the concurrent requests low. + - `-.memcached.consistent-hash`: Old default: false, new default: true. This allows for better cache hits when the memcaches are scaled up and down. + - `-querier.batch-iterators`: Old default: false, new default: true. + - `-querier.ingester-streaming`: Old default: false, new default: true. * [FEATURE] Added experimental storage API to the ruler service that is enabled when the `-experimental.ruler.enable-api` is set to true #2269 * `-ruler.storage.type` flag now allows `s3`,`gcs`, and `azure` values * `-ruler.storage.(s3|gcs|azure)` flags exist to allow the configuration of object clients set for rule storage diff --git a/docs/configuration/arguments.md b/docs/configuration/arguments.md index 5d88260afa..e8d53d5bd3 100644 --- a/docs/configuration/arguments.md +++ b/docs/configuration/arguments.md @@ -284,7 +284,7 @@ It also talks to a KVStore and has it's own copies of the same flags used by the - `-ingester.chunk-age-jitter` - To reduce load on the database exactly 12 hours after starting, the age limit is reduced by a varying amount up to this. (default 20m) + To reduce load on the database exactly 12 hours after starting, the age limit is reduced by a varying amount up to this. Don't enable this along with `-ingester.spread-flushes` (default 0m) - `-ingester.spread-flushes` diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md index 6c2d8ffa3b..f37d6d4f49 100644 --- a/docs/configuration/config-file-reference.md +++ b/docs/configuration/config-file-reference.md @@ -312,7 +312,7 @@ pool: # Run a health check on each ingester client during periodic cleanup. # CLI flag: -distributor.health-check-ingesters - [health_check_ingesters: | default = false] + [health_check_ingesters: | default = true] ha_tracker: # Enable the distributors HA tracker so that it can accept samples from @@ -565,7 +565,7 @@ lifecycler: # flushing. 0 disables it and a stale series is not flushed until the # max-chunk-idle timeout is reached. # CLI flag: -ingester.max-stale-chunk-idle -[max_stale_chunk_idle_time: | default = 0s] +[max_stale_chunk_idle_time: | default = 2m0s] # Timeout for individual flush operations. # CLI flag: -ingester.flush-op-timeout @@ -586,7 +586,7 @@ lifecycler: # If true, spread series flushes across the whole period of # -ingester.max-chunk-age. # CLI flag: -ingester.spread-flushes -[spread_flushes: | default = false] +[spread_flushes: | default = true] # Period with which to update the per-user ingestion rates. # CLI flag: -ingester.rate-update-period @@ -614,11 +614,11 @@ The `querier_config` configures the Cortex querier. # Use batch iterators to execute query, as opposed to fully materialising the # series in memory. Takes precedent over the -querier.iterators flag. # CLI flag: -querier.batch-iterators -[batch_iterators: | default = false] +[batch_iterators: | default = true] # Use streaming RPCs to query ingester. # CLI flag: -querier.ingester-streaming -[ingester_streaming: | default = false] +[ingester_streaming: | default = true] # Maximum number of samples a single query can load into memory. # CLI flag: -querier.max-samples @@ -1928,12 +1928,12 @@ The `consul_config` configures the consul client. The supported CLI flags `.consul.consistent-reads -[consistent_reads: | default = true] +[consistent_reads: | default = false] # Rate limit when watching key or prefix in Consul, in requests per second. 0 # disables the rate limit. # CLI flag: -.consul.watch-rate-limit -[watch_rate_limit: | default = 0] +[watch_rate_limit: | default = 1] # Burst size used in rate limit. Values less than 1 are treated as 1. # CLI flag: -.consul.watch-burst-size @@ -2205,7 +2205,7 @@ The `memcached_config` block configures how data is stored in Memcached (ie. exp # How many keys to fetch in each batch. # CLI flag: -.memcached.batchsize -[batch_size: | default = 0] +[batch_size: | default = 1024] # Maximum active requests to memcache. # CLI flag: -.memcached.parallelism @@ -2252,7 +2252,7 @@ The `memcached_client_config` configures the client used to connect to Memcached # Use consistent hashing to distribute to memcache servers. # CLI flag: -.memcached.consistent-hash -[consistent_hash: | default = false] +[consistent_hash: | default = true] ``` ### `fifo_cache_config` diff --git a/docs/guides/running.md b/docs/guides/running.md index e244b4d229..471a9870a4 100644 --- a/docs/guides/running.md +++ b/docs/guides/running.md @@ -241,14 +241,4 @@ time() - sum by (statefulset_kubernetes_io_pod_name) (prometheus_remote_storage_ ### Optimising Storage -These ingester options reduce the chance of storing multiple copies of -the same data: - - -ingester.spread-flushes=true - -ingester.chunk-age-jitter=0 - Add a chunk cache via `-store.chunks-cache.memcached.hostname` to allow writes to be de-duplicated. - -As recommended under [Chunk encoding](#chunk-encoding), use Bigchunk: - - -ingester.chunk-encoding=3 # bigchunk diff --git a/integration/ingester_hand_over_test.go b/integration/ingester_hand_over_test.go index 3180058e4a..d9a45046e8 100644 --- a/integration/ingester_hand_over_test.go +++ b/integration/ingester_hand_over_test.go @@ -86,10 +86,20 @@ func runIngesterHandOverTest(t *testing.T, flags map[string]string, setup func(t }), "") require.NoError(t, s.Start(ingester2)) + // Wait a bit to make sure that querier is caught up. Otherwise, we may be querying for data, + // while querier still knows about old ingester only. + require.NoError(t, querier.WaitForMetricWithLabels(e2e.EqualsSingle(1), "cortex_ring_members", map[string]string{"name": "ingester", "state": "ACTIVE"})) + require.NoError(t, querier.WaitForMetricWithLabels(e2e.EqualsSingle(1), "cortex_ring_members", map[string]string{"name": "ingester", "state": "PENDING"})) + // Stop ingester-1. This function will return once the ingester-1 is successfully // stopped, which means the transfer to ingester-2 is completed. require.NoError(t, s.Stop(ingester1)) + // Make sure querier now sees only new ingester. We check that by verifying that there is only one ACTIVE, but no PENDING or JOINING ingester. + require.NoError(t, querier.WaitForMetricWithLabels(e2e.EqualsSingle(1), "cortex_ring_members", map[string]string{"name": "ingester", "state": "ACTIVE"})) + require.NoError(t, querier.WaitForMetricWithLabels(e2e.EqualsSingle(0), "cortex_ring_members", map[string]string{"name": "ingester", "state": "JOINING"})) + require.NoError(t, querier.WaitForMetricWithLabels(e2e.EqualsSingle(0), "cortex_ring_members", map[string]string{"name": "ingester", "state": "PENDING"})) + // Query the series again. result, err = c.Query("series_1", now) require.NoError(t, err) diff --git a/pkg/chunk/cache/memcached.go b/pkg/chunk/cache/memcached.go index 3319110e97..0b14180e11 100644 --- a/pkg/chunk/cache/memcached.go +++ b/pkg/chunk/cache/memcached.go @@ -50,7 +50,7 @@ type MemcachedConfig struct { // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet func (cfg *MemcachedConfig) RegisterFlagsWithPrefix(prefix, description string, f *flag.FlagSet) { f.DurationVar(&cfg.Expiration, prefix+"memcached.expiration", 0, description+"How long keys stay in the memcache.") - f.IntVar(&cfg.BatchSize, prefix+"memcached.batchsize", 0, description+"How many keys to fetch in each batch.") + f.IntVar(&cfg.BatchSize, prefix+"memcached.batchsize", 1024, description+"How many keys to fetch in each batch.") f.IntVar(&cfg.Parallelism, prefix+"memcached.parallelism", 100, description+"Maximum active requests to memcache.") } diff --git a/pkg/chunk/cache/memcached_client.go b/pkg/chunk/cache/memcached_client.go index 131b60a404..ed97f12b15 100644 --- a/pkg/chunk/cache/memcached_client.go +++ b/pkg/chunk/cache/memcached_client.go @@ -75,7 +75,7 @@ func (cfg *MemcachedClientConfig) RegisterFlagsWithPrefix(prefix, description st f.IntVar(&cfg.MaxIdleConns, prefix+"memcached.max-idle-conns", 16, description+"Maximum number of idle connections in pool.") f.DurationVar(&cfg.Timeout, prefix+"memcached.timeout", 100*time.Millisecond, description+"Maximum time to wait before giving up on memcached requests.") f.DurationVar(&cfg.UpdateInterval, prefix+"memcached.update-interval", 1*time.Minute, description+"Period with which to poll DNS for memcache servers.") - f.BoolVar(&cfg.ConsistentHash, prefix+"memcached.consistent-hash", false, description+"Use consistent hashing to distribute to memcache servers.") + f.BoolVar(&cfg.ConsistentHash, prefix+"memcached.consistent-hash", true, description+"Use consistent hashing to distribute to memcache servers.") } // NewMemcachedClient creates a new MemcacheClient that gets its server list diff --git a/pkg/distributor/distributor_test.go b/pkg/distributor/distributor_test.go index 6a97642581..c22c7af718 100644 --- a/pkg/distributor/distributor_test.go +++ b/pkg/distributor/distributor_test.go @@ -897,6 +897,14 @@ type mockIngester struct { queryDelay time.Duration } +func (i *mockIngester) Check(ctx context.Context, in *grpc_health_v1.HealthCheckRequest, opts ...grpc.CallOption) (*grpc_health_v1.HealthCheckResponse, error) { + return &grpc_health_v1.HealthCheckResponse{}, nil +} + +func (i *mockIngester) Close() error { + return nil +} + func (i *mockIngester) Push(ctx context.Context, req *client.WriteRequest, opts ...grpc.CallOption) (*client.WriteResponse, error) { i.Lock() defer i.Unlock() diff --git a/pkg/ingester/client/pool.go b/pkg/ingester/client/pool.go index b6aad69de3..9468ebf6f0 100644 --- a/pkg/ingester/client/pool.go +++ b/pkg/ingester/client/pool.go @@ -39,7 +39,7 @@ type PoolConfig struct { // RegisterFlags adds the flags required to config this to the given FlagSet. func (cfg *PoolConfig) RegisterFlags(f *flag.FlagSet) { f.DurationVar(&cfg.ClientCleanupPeriod, "distributor.client-cleanup-period", 15*time.Second, "How frequently to clean up clients for ingesters that have gone away.") - f.BoolVar(&cfg.HealthCheckIngesters, "distributor.health-check-ingesters", false, "Run a health check on each ingester client during periodic cleanup.") + f.BoolVar(&cfg.HealthCheckIngesters, "distributor.health-check-ingesters", true, "Run a health check on each ingester client during periodic cleanup.") } // Pool holds a cache of grpc_health_v1 clients. diff --git a/pkg/ingester/ingester.go b/pkg/ingester/ingester.go index 5f3fc5ebc1..e0191eaf58 100644 --- a/pkg/ingester/ingester.go +++ b/pkg/ingester/ingester.go @@ -82,10 +82,10 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.DurationVar(&cfg.RetainPeriod, "ingester.retain-period", 5*time.Minute, "Period chunks will remain in memory after flushing.") f.DurationVar(&cfg.FlushOpTimeout, "ingester.flush-op-timeout", 1*time.Minute, "Timeout for individual flush operations.") f.DurationVar(&cfg.MaxChunkIdle, "ingester.max-chunk-idle", 5*time.Minute, "Maximum chunk idle time before flushing.") - f.DurationVar(&cfg.MaxStaleChunkIdle, "ingester.max-stale-chunk-idle", 0, "Maximum chunk idle time for chunks terminating in stale markers before flushing. 0 disables it and a stale series is not flushed until the max-chunk-idle timeout is reached.") + f.DurationVar(&cfg.MaxStaleChunkIdle, "ingester.max-stale-chunk-idle", 2*time.Minute, "Maximum chunk idle time for chunks terminating in stale markers before flushing. 0 disables it and a stale series is not flushed until the max-chunk-idle timeout is reached.") f.DurationVar(&cfg.MaxChunkAge, "ingester.max-chunk-age", 12*time.Hour, "Maximum chunk age before flushing.") f.DurationVar(&cfg.ChunkAgeJitter, "ingester.chunk-age-jitter", 20*time.Minute, "Range of time to subtract from -ingester.max-chunk-age to spread out flushes") - f.BoolVar(&cfg.SpreadFlushes, "ingester.spread-flushes", false, "If true, spread series flushes across the whole period of -ingester.max-chunk-age.") + f.BoolVar(&cfg.SpreadFlushes, "ingester.spread-flushes", true, "If true, spread series flushes across the whole period of -ingester.max-chunk-age.") f.IntVar(&cfg.ConcurrentFlushes, "ingester.concurrent-flushes", 50, "Number of concurrent goroutines flushing to dynamodb.") f.DurationVar(&cfg.RateUpdatePeriod, "ingester.rate-update-period", 15*time.Second, "Period with which to update the per-user ingestion rates.") } diff --git a/pkg/querier/querier.go b/pkg/querier/querier.go index 0bb60b525e..bd0cec1545 100644 --- a/pkg/querier/querier.go +++ b/pkg/querier/querier.go @@ -62,8 +62,8 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.DurationVar(&promql.LookbackDelta, "promql.lookback-delta", promql.LookbackDelta, "Time since the last sample after which a time series is considered stale and ignored by expression evaluations.") } f.BoolVar(&cfg.Iterators, "querier.iterators", false, "Use iterators to execute query, as opposed to fully materialising the series in memory.") - f.BoolVar(&cfg.BatchIterators, "querier.batch-iterators", false, "Use batch iterators to execute query, as opposed to fully materialising the series in memory. Takes precedent over the -querier.iterators flag.") - f.BoolVar(&cfg.IngesterStreaming, "querier.ingester-streaming", false, "Use streaming RPCs to query ingester.") + f.BoolVar(&cfg.BatchIterators, "querier.batch-iterators", true, "Use batch iterators to execute query, as opposed to fully materialising the series in memory. Takes precedent over the -querier.iterators flag.") + f.BoolVar(&cfg.IngesterStreaming, "querier.ingester-streaming", true, "Use streaming RPCs to query ingester.") f.IntVar(&cfg.MaxSamples, "querier.max-samples", 50e6, "Maximum number of samples a single query can load into memory.") f.DurationVar(&cfg.QueryIngestersWithin, "querier.query-ingesters-within", 0, "Maximum lookback beyond which queries are not sent to ingester. 0 means all queries are sent to ingester.") f.DurationVar(&cfg.MaxQueryIntoFuture, "querier.max-query-into-future", 10*time.Minute, "Maximum duration into the future you can query. 0 to disable.") diff --git a/pkg/ring/kv/consul/client.go b/pkg/ring/kv/consul/client.go index bd401bc805..3fdc24c543 100644 --- a/pkg/ring/kv/consul/client.go +++ b/pkg/ring/kv/consul/client.go @@ -63,8 +63,8 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet, prefix string) { f.StringVar(&cfg.Host, prefix+"consul.hostname", "localhost:8500", "Hostname and port of Consul.") f.StringVar(&cfg.ACLToken, prefix+"consul.acl-token", "", "ACL Token used to interact with Consul.") f.DurationVar(&cfg.HTTPClientTimeout, prefix+"consul.client-timeout", 2*longPollDuration, "HTTP timeout when talking to Consul") - f.BoolVar(&cfg.ConsistentReads, prefix+"consul.consistent-reads", true, "Enable consistent reads to Consul.") - f.Float64Var(&cfg.WatchKeyRateLimit, prefix+"consul.watch-rate-limit", 0, "Rate limit when watching key or prefix in Consul, in requests per second. 0 disables the rate limit.") + f.BoolVar(&cfg.ConsistentReads, prefix+"consul.consistent-reads", false, "Enable consistent reads to Consul.") + f.Float64Var(&cfg.WatchKeyRateLimit, prefix+"consul.watch-rate-limit", 1, "Rate limit when watching key or prefix in Consul, in requests per second. 0 disables the rate limit.") f.IntVar(&cfg.WatchKeyBurstSize, prefix+"consul.watch-burst-size", 1, "Burst size used in rate limit. Values less than 1 are treated as 1.") }