From 9bf77e74065f4951dec84f5c4da044db5b22b216 Mon Sep 17 00:00:00 2001
From: Goutham Veeramachaneni <gouthamve@gmail.com>
Date: Tue, 24 Mar 2020 18:53:45 +0100
Subject: [PATCH] Prefix chunks cache flags. (#2241)

* Prefix chunks cache flags.

Fixes https://github.com/cortexproject/cortex/issues/2191

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

* Move background cache prefix to cache from memcache

Fixes #553

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

* Update docs for the cache flag changes.

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

* Fix all docs references

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

* Rebase and update prefixes in doc

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

* Review feedback

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

* Address feedback

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
---
 CHANGELOG.md                                | 34 +++++++++++++++
 docs/configuration/arguments.md             |  8 ++--
 docs/configuration/config-file-reference.md | 47 +++++++++++----------
 docs/guides/caching.md                      |  6 +--
 docs/guides/running.md                      |  2 +-
 k8s/ingester-dep.yaml                       |  6 +--
 k8s/querier-dep.yaml                        |  6 +--
 k8s/ruler-dep.yaml                          |  6 +--
 pkg/chunk/cache/background.go               |  4 +-
 pkg/chunk/cache/memcached_client.go         |  2 +-
 pkg/chunk/chunk_store.go                    |  9 ++--
 11 files changed, 85 insertions(+), 45 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index bc61609713..a24fa29018 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,40 @@
 
 ## master / unreleased
 
+* [CHANGE] Renamed the `memcache.write-back-goroutines` and `memcache.write-back-buffer` flags to `background.write-back-concurrency` and `background.write-back-buffer`. This affects the following flags:
+  - `-frontend.memcache.write-back-buffer` --> `-frontend.background.write-back-buffer` 
+  - `-frontend.memcache.write-back-goroutines` --> `-frontend.background.write-back-concurrency`
+  - `-store.index-cache-read.memcache.write-back-buffer` --> `-store.index-cache-read.background.write-back-buffer`
+  - `-store.index-cache-read.memcache.write-back-goroutines` --> `-store.index-cache-read.background.write-back-concurrency`
+  - `-store.index-cache-write.memcache.write-back-buffer` --> `-store.index-cache-write.background.write-back-buffer`
+  - `-store.index-cache-write.memcache.write-back-goroutines` --> `-store.index-cache-write.background.write-back-concurrency`
+  - `-memcache.write-back-buffer` --> `-store.chunks-cache.background.write-back-buffer`. Note the next change log for the difference.
+  - `-memcache.write-back-goroutines` --> `-store.chunks-cache.background.write-back-concurrency`. Note the next change log for the difference.
+
+* [CHANGE] Renamed the chunk cache flags to have `store.chunks-cache.` as prefix. This means the following flags have been changed:
+  - `-cache.enable-fifocache` --> `-store.chunks-cache.cache.enable-fifocache` 
+  - `-default-validity` --> `-store.chunks-cache.default-validity` 
+  - `-fifocache.duration` --> `-store.chunks-cache.fifocache.duration` 
+  - `-fifocache.size` --> `-store.chunks-cache.fifocache.size` 
+  - `-memcache.write-back-buffer` --> `-store.chunks-cache.background.write-back-buffer`. Note the previous change log for the difference. 
+  - `-memcache.write-back-goroutines` --> `-store.chunks-cache.background.write-back-concurrency`. Note the previous change log for the difference. 
+  - `-memcached.batchsize` --> `-store.chunks-cache.memcached.batchsize` 
+  - `-memcached.consistent-hash` --> `-store.chunks-cache.memcached.consistent-hash` 
+  - `-memcached.expiration` --> `-store.chunks-cache.memcached.expiration` 
+  - `-memcached.hostname` --> `-store.chunks-cache.memcached.hostname` 
+  - `-memcached.max-idle-conns` --> `-store.chunks-cache.memcached.max-idle-conns` 
+  - `-memcached.parallelism` --> `-store.chunks-cache.memcached.parallelism` 
+  - `-memcached.service` --> `-store.chunks-cache.memcached.service` 
+  - `-memcached.timeout` --> `-store.chunks-cache.memcached.timeout` 
+  - `-memcached.update-interval` --> `-store.chunks-cache.memcached.update-interval` 
+  - `-redis.enable-tls` --> `-store.chunks-cache.redis.enable-tls` 
+  - `-redis.endpoint` --> `-store.chunks-cache.redis.endpoint` 
+  - `-redis.expiration` --> `-store.chunks-cache.redis.expiration` 
+  - `-redis.max-active-conns` --> `-store.chunks-cache.redis.max-active-conns` 
+  - `-redis.max-idle-conns` --> `-store.chunks-cache.redis.max-idle-conns` 
+  - `-redis.password` --> `-store.chunks-cache.redis.password` 
+  - `-redis.timeout` --> `-store.chunks-cache.redis.timeout` 
+* [CHANGE] Rename the `-store.chunk-cache-stubs` to `-store.chunks-cache.cache-stubs` to be more inline with above.
 * [CHANGE] Don't support mixed time units anymore for duration. For example, 168h5m0s doesn't work anymore, please use just one unit (s|m|h|d|w|y). #2252
 * [CHANGE] Utilize separate protos for rule state and storage. Experimental ruler API will not be functional until the rollout is complete. #2226
 * [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
diff --git a/docs/configuration/arguments.md b/docs/configuration/arguments.md
index 285de8259a..6cbf64a50d 100644
--- a/docs/configuration/arguments.md
+++ b/docs/configuration/arguments.md
@@ -108,11 +108,11 @@ The ingester query API was improved over time, but defaults to the old behaviour
 
    When caching query results, it is desirable to prevent the caching of very recent results that might still be in flux.  Use this parameter to configure the age of results that should be excluded.
 
-- `-memcached.{hostname, service, timeout}`
+- `-frontend.memcached.{hostname, service, timeout}`
 
    Use these flags to specify the location and timeout of the memcached cluster used to cache query results.
 
-- `-redis.{endpoint, timeout}`
+- `-frontend.redis.{endpoint, timeout}`
 
    Use these flags to specify the location and timeout of the Redis service used to cache query results.
 
@@ -288,7 +288,7 @@ It also talks to a KVStore and has it's own copies of the same flags used by the
 
 - `-ingester.spread-flushes`
 
-  Makes the ingester flush each timeseries at a specific point in the `max-chunk-age` cycle. This means multiple replicas of a chunk are very likely to contain the same contents which cuts chunk storage space by up to 66%. Set `-ingester.chunk-age-jitter` to `0` when using this option. If a chunk cache is configured (via `-memcached.hostname`) then duplicate chunk writes are skipped which cuts write IOPs.
+  Makes the ingester flush each timeseries at a specific point in the `max-chunk-age` cycle. This means multiple replicas of a chunk are very likely to contain the same contents which cuts chunk storage space by up to 66%. Set `-ingester.chunk-age-jitter` to `0` when using this option. If a chunk cache is configured (via `-store.chunks-cache.memcached.hostname`) then duplicate chunk writes are skipped which cuts write IOPs.
 
 - `-ingester.join-after`
 
@@ -329,7 +329,7 @@ It also talks to a KVStore and has it's own copies of the same flags used by the
 
    When `push` requests arrive, pre-allocate this many slots to decode them. Tune this setting to reduce memory allocations and garbage. The optimum value will depend on how many labels are sent with your timeseries samples.
 
-- `-store.chunk-cache-stubs`
+- `-store.chunk-cache.cache-stubs`
 
    Where you don't want to cache every chunk written by ingesters, but you do want to take advantage of chunk write deduplication, this option will make ingesters write a placeholder to the cache for each chunk.
    Make sure you configure ingesters with a different cache to queriers, which need the whole value.
diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md
index a5a212d0bb..df8dcf9f72 100644
--- a/docs/configuration/config-file-reference.md
+++ b/docs/configuration/config-file-reference.md
@@ -644,12 +644,12 @@ results_cache:
     [default_validity: <duration> | default = 0s]
 
     background:
-      # How many goroutines to use to write back to memcache.
-      # CLI flag: -frontend.memcache.write-back-goroutines
+      # At what concurrency to write back to cache.
+      # CLI flag: -frontend.background.write-back-concurrency
       [writeback_goroutines: <int> | default = 10]
 
       # How many key batches to buffer for background write-back.
-      # CLI flag: -frontend.memcache.write-back-buffer
+      # CLI flag: -frontend.background.write-back-buffer
       [writeback_buffer: <int> | default = 10000]
 
     # The memcached_config block configures how data is stored in Memcached (ie.
@@ -1588,14 +1588,14 @@ index_queries_cache_config:
   [default_validity: <duration> | default = 0s]
 
   background:
-    # Cache config for index entry reading. How many goroutines to use to write
-    # back to memcache.
-    # CLI flag: -store.index-cache-read.memcache.write-back-goroutines
+    # Cache config for index entry reading. At what concurrency to write back to
+    # cache.
+    # CLI flag: -store.index-cache-read.background.write-back-concurrency
     [writeback_goroutines: <int> | default = 10]
 
     # Cache config for index entry reading. How many key batches to buffer for
     # background write-back.
-    # CLI flag: -store.index-cache-read.memcache.write-back-buffer
+    # CLI flag: -store.index-cache-read.background.write-back-buffer
     [writeback_buffer: <int> | default = 10000]
 
   # The memcached_config block configures how data is stored in Memcached (ie.
@@ -1633,37 +1633,40 @@ The `chunk_store_config` configures how Cortex stores the data (chunks storage e
 ```yaml
 chunk_cache_config:
   # Cache config for chunks. Enable in-memory cache.
-  # CLI flag: -cache.enable-fifocache
+  # CLI flag: -store.chunks-cache.cache.enable-fifocache
   [enable_fifocache: <boolean> | default = false]
 
   # Cache config for chunks. The default validity of entries for caches unless
   # overridden.
-  # CLI flag: -default-validity
+  # CLI flag: -store.chunks-cache.default-validity
   [default_validity: <duration> | default = 0s]
 
   background:
-    # Cache config for chunks. How many goroutines to use to write back to
-    # memcache.
-    # CLI flag: -memcache.write-back-goroutines
+    # Cache config for chunks. At what concurrency to write back to cache.
+    # CLI flag: -store.chunks-cache.background.write-back-concurrency
     [writeback_goroutines: <int> | default = 10]
 
     # Cache config for chunks. How many key batches to buffer for background
     # write-back.
-    # CLI flag: -memcache.write-back-buffer
+    # CLI flag: -store.chunks-cache.background.write-back-buffer
     [writeback_buffer: <int> | default = 10000]
 
   # The memcached_config block configures how data is stored in Memcached (ie.
   # expiration).
+  # The CLI flags prefix for this block config is: store.chunks-cache
   [memcached: <memcached_config>]
 
   # The memcached_client_config configures the client used to connect to
   # Memcached.
+  # The CLI flags prefix for this block config is: store.chunks-cache
   [memcached_client: <memcached_client_config>]
 
   # The redis_config configures the Redis backend cache.
+  # The CLI flags prefix for this block config is: store.chunks-cache
   [redis: <redis_config>]
 
   # The fifo_cache_config configures the local in-memory cache.
+  # The CLI flags prefix for this block config is: store.chunks-cache
   [fifocache: <fifo_cache_config>]
 
 write_dedupe_cache_config:
@@ -1677,14 +1680,14 @@ write_dedupe_cache_config:
   [default_validity: <duration> | default = 0s]
 
   background:
-    # Cache config for index entry writing. How many goroutines to use to write
-    # back to memcache.
-    # CLI flag: -store.index-cache-write.memcache.write-back-goroutines
+    # Cache config for index entry writing. At what concurrency to write back to
+    # cache.
+    # CLI flag: -store.index-cache-write.background.write-back-concurrency
     [writeback_goroutines: <int> | default = 10]
 
     # Cache config for index entry writing. How many key batches to buffer for
     # background write-back.
-    # CLI flag: -store.index-cache-write.memcache.write-back-buffer
+    # CLI flag: -store.index-cache-write.background.write-back-buffer
     [writeback_buffer: <int> | default = 10000]
 
   # The memcached_config block configures how data is stored in Memcached (ie.
@@ -2088,8 +2091,8 @@ The `limits_config` configures default and per-tenant limits imposed by Cortex s
 
 The `redis_config` configures the Redis backend cache. The supported CLI flags `<prefix>` used to reference this config block are:
 
-- _no prefix_
 - `frontend`
+- `store.chunks-cache`
 - `store.index-cache-read`
 - `store.index-cache-write`
 
@@ -2130,8 +2133,8 @@ The `redis_config` configures the Redis backend cache. The supported CLI flags `
 
 The `memcached_config` block configures how data is stored in Memcached (ie. expiration). The supported CLI flags `<prefix>` used to reference this config block are:
 
-- _no prefix_
 - `frontend`
+- `store.chunks-cache`
 - `store.index-cache-read`
 - `store.index-cache-write`
 
@@ -2155,15 +2158,15 @@ The `memcached_config` block configures how data is stored in Memcached (ie. exp
 
 The `memcached_client_config` configures the client used to connect to Memcached. The supported CLI flags `<prefix>` used to reference this config block are:
 
-- _no prefix_
 - `frontend`
+- `store.chunks-cache`
 - `store.index-cache-read`
 - `store.index-cache-write`
 
 &nbsp;
 
 ```yaml
-# Hostname for memcached service to use when caching chunks. If empty, no
+# Hostname for memcached service to use. If empty and if addresses is unset, no
 # memcached will be used.
 # CLI flag: -<prefix>.memcached.hostname
 [host: <string> | default = ""]
@@ -2198,8 +2201,8 @@ The `memcached_client_config` configures the client used to connect to Memcached
 
 The `fifo_cache_config` configures the local in-memory cache. The supported CLI flags `<prefix>` used to reference this config block are:
 
-- _no prefix_
 - `frontend`
+- `store.chunks-cache`
 - `store.index-cache-read`
 - `store.index-cache-write`
 
diff --git a/docs/guides/caching.md b/docs/guides/caching.md
index 030b3ab472..d55dbb2b30 100644
--- a/docs/guides/caching.md
+++ b/docs/guides/caching.md
@@ -41,9 +41,9 @@ If deploying Cortex on Kubernetes, Cortex should be pointed at a memcached [head
 The flags used to configure memcached are common for each caching caching opportunity, differentiated by a prefix:
 
 ```
--<prefix>.memcache.write-back-buffer int
+-<prefix>.cache.write-back-buffer int
     How many chunks to buffer for background write back. (default 10000)
--<prefix>.memcache.write-back-goroutines int
+-<prefix>.cache.write-back-goroutines int
     How many goroutines to use to write back to memcache. (default 10)
 -<prefix>.memcached.batchsize int
     How many keys to fetch in each batch.
@@ -121,7 +121,7 @@ These are typically a few KB in size, and depend mostly on the duration and enco
 The chunk cache is a write-through cache - chunks are written to the cache as they are flushed to the chunk store.  This ensures the cache always contains the most recent chunks.
 Items stay in the cache indefinitely.
 
-The chunk cache should be configured on the **ingester**, **querier** and **ruler** using the flags without a prefix.
+The chunk cache should be configured on the **ingester**, **querier** and **ruler** using the flags with the prefix `-store.chunks-cache`.
 
 It is best practice to ensure the chunk cache is big enough to accommodate at least 24 hours of chunk data.
 You can use the following query (from the [cortex-mixin](https://github.com/grafana/cortex-jsonnet)) to estimate the required number of memcached replicas:
diff --git a/docs/guides/running.md b/docs/guides/running.md
index d29d8c9e5e..09662907df 100644
--- a/docs/guides/running.md
+++ b/docs/guides/running.md
@@ -245,7 +245,7 @@ the same data:
         -ingester.spread-flushes=true
         -ingester.chunk-age-jitter=0
 
-Add a chunk cache via `-memcached.hostname` to allow writes to be de-duplicated.
+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:
 
diff --git a/k8s/ingester-dep.yaml b/k8s/ingester-dep.yaml
index f23c3c3049..fdb1c1fd04 100644
--- a/k8s/ingester-dep.yaml
+++ b/k8s/ingester-dep.yaml
@@ -47,9 +47,9 @@ spec:
         - -s3.url=s3://abc:123@s3.default.svc.cluster.local:4569
         - -dynamodb.url=dynamodb://user:pass@dynamodb.default.svc.cluster.local:8000
         - -schema-config-file=/etc/cortex/schema.yaml
-        - -memcached.hostname=memcached.default.svc.cluster.local
-        - -memcached.timeout=100ms
-        - -memcached.service=memcached
+        - -store.chunks-cache.memcached.hostname=memcached.default.svc.cluster.local
+        - -store.chunks-cache.memcached.timeout=100ms
+        - -store.chunks-cache.memcached.service=memcached
         ports:
         - containerPort: 80
         readinessProbe:
diff --git a/k8s/querier-dep.yaml b/k8s/querier-dep.yaml
index 36c1400b33..6636140914 100644
--- a/k8s/querier-dep.yaml
+++ b/k8s/querier-dep.yaml
@@ -25,9 +25,9 @@ spec:
         - -dynamodb.url=dynamodb://user:pass@dynamodb.default.svc.cluster.local:8000
         - -schema-config-file=/etc/cortex/schema.yaml
         - -querier.frontend-address=query-frontend.default.svc.cluster.local:9095
-        - -memcached.hostname=memcached.default.svc.cluster.local
-        - -memcached.timeout=100ms
-        - -memcached.service=memcached
+        - -store.chunks-cache.memcached.hostname=memcached.default.svc.cluster.local
+        - -store.chunks-cache.memcached.timeout=100ms
+        - -store.chunks-cache.memcached.service=memcached
         - -distributor.replication-factor=1
         ports:
         - containerPort: 80
diff --git a/k8s/ruler-dep.yaml b/k8s/ruler-dep.yaml
index f603149219..4635c49c52 100644
--- a/k8s/ruler-dep.yaml
+++ b/k8s/ruler-dep.yaml
@@ -27,9 +27,9 @@ spec:
         - -s3.url=s3://abc:123@default.svc.cluster.local:4569/s3
         - -dynamodb.url=dynamodb://user:pass@dynamodb.default.svc.cluster.local:8000
         - -schema-config-file=/etc/cortex/schema.yaml
-        - -memcached.hostname=memcached.default.svc.cluster.local
-        - -memcached.timeout=100ms
-        - -memcached.service=memcached
+        - -store.chunks-cache.memcached.hostname=memcached.default.svc.cluster.local
+        - -store.chunks-cache.memcached.timeout=100ms
+        - -store.chunks-cache.memcached.service=memcached
         - -distributor.replication-factor=1
         ports:
         - containerPort: 80
diff --git a/pkg/chunk/cache/background.go b/pkg/chunk/cache/background.go
index 5b101e4a4c..9ada6b6160 100644
--- a/pkg/chunk/cache/background.go
+++ b/pkg/chunk/cache/background.go
@@ -32,8 +32,8 @@ type BackgroundConfig struct {
 
 // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet
 func (cfg *BackgroundConfig) RegisterFlagsWithPrefix(prefix string, description string, f *flag.FlagSet) {
-	f.IntVar(&cfg.WriteBackGoroutines, prefix+"memcache.write-back-goroutines", 10, description+"How many goroutines to use to write back to memcache.")
-	f.IntVar(&cfg.WriteBackBuffer, prefix+"memcache.write-back-buffer", 10000, description+"How many key batches to buffer for background write-back.")
+	f.IntVar(&cfg.WriteBackGoroutines, prefix+"background.write-back-concurrency", 10, description+"At what concurrency to write back to cache.")
+	f.IntVar(&cfg.WriteBackBuffer, prefix+"background.write-back-buffer", 10000, description+"How many key batches to buffer for background write-back.")
 }
 
 type backgroundCache struct {
diff --git a/pkg/chunk/cache/memcached_client.go b/pkg/chunk/cache/memcached_client.go
index 89ee406065..131b60a404 100644
--- a/pkg/chunk/cache/memcached_client.go
+++ b/pkg/chunk/cache/memcached_client.go
@@ -69,7 +69,7 @@ type MemcachedClientConfig struct {
 
 // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet
 func (cfg *MemcachedClientConfig) RegisterFlagsWithPrefix(prefix, description string, f *flag.FlagSet) {
-	f.StringVar(&cfg.Host, prefix+"memcached.hostname", "", description+"Hostname for memcached service to use when caching chunks. If empty, no memcached will be used.")
+	f.StringVar(&cfg.Host, prefix+"memcached.hostname", "", description+"Hostname for memcached service to use. If empty and if addresses is unset, no memcached will be used.")
 	f.StringVar(&cfg.Service, prefix+"memcached.service", "memcached", description+"SRV service used to discover memcache servers.")
 	f.StringVar(&cfg.Addresses, prefix+"memcached.addresses", "", description+"EXPERIMENTAL: Comma separated addresses list in DNS Service Discovery format: https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery")
 	f.IntVar(&cfg.MaxIdleConns, prefix+"memcached.max-idle-conns", 16, description+"Maximum number of idle connections in pool.")
diff --git a/pkg/chunk/chunk_store.go b/pkg/chunk/chunk_store.go
index 444b539fa5..a4f67a18e1 100644
--- a/pkg/chunk/chunk_store.go
+++ b/pkg/chunk/chunk_store.go
@@ -55,14 +55,17 @@ type StoreConfig struct {
 	// Limits query start time to be greater than now() - MaxLookBackPeriod, if set.
 	MaxLookBackPeriod time.Duration `yaml:"max_look_back_period"`
 
-	// Not visible in yaml because the setting shouldn't be common between ingesters and queriers
+	// Not visible in yaml because the setting shouldn't be common between ingesters and queriers.
+	// This exists in case we don't want to cache all the chunks but still want to take advantage of
+	// ingester chunk write deduplication. But for the queriers we need the full value. So when this option
+	// is set, use different caches for ingesters and queriers.
 	chunkCacheStubs bool // don't write the full chunk to cache, just a stub entry
 }
 
 // RegisterFlags adds the flags required to config this to the given FlagSet
 func (cfg *StoreConfig) RegisterFlags(f *flag.FlagSet) {
-	cfg.ChunkCacheConfig.RegisterFlagsWithPrefix("", "Cache config for chunks. ", f)
-	f.BoolVar(&cfg.chunkCacheStubs, "store.chunk-cache-stubs", false, "If true, don't write the full chunk to cache, just a stub entry.")
+	cfg.ChunkCacheConfig.RegisterFlagsWithPrefix("store.chunks-cache.", "Cache config for chunks. ", f)
+	f.BoolVar(&cfg.chunkCacheStubs, "store.chunks-cache.cache-stubs", false, "If true, don't write the full chunk to cache, just a stub entry.")
 	cfg.WriteDedupeCacheConfig.RegisterFlagsWithPrefix("store.index-cache-write.", "Cache config for index entry writing. ", f)
 
 	f.DurationVar(&cfg.CacheLookupsOlderThan, "store.cache-lookups-older-than", 0, "Cache index entries older than this period. 0 to disable.")