Allow use of Redis for all caches #4371

56quarters · 2023-03-03T19:18:53Z

What this PR does

This change pulls in a version of dskit that supports using Redis implementations for the cache.Cache and cache.RemoteCacheClient interfaces. The query-frontend caching integration test now runs with both Memcached and Redis caches. Redis can also be enabled for the microservices docker compose environment.

Which issue(s) this PR fixes or relates to

See #4236
Fixes #4341

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

56quarters · 2023-03-03T20:48:54Z

Note that I am purposefully not including updates to the Jsonnet or Helm chart as part of these changes (there's enough to review as-is).

cmd/mimir/help.txt.tmpl

lamida · 2023-03-06T07:11:17Z

pkg/storage/tsdb/index_cache.go

@@ -119,3 +125,17 @@ func newMemcachedIndexCache(cfg cache.MemcachedConfig, logger log.Logger, regist

 	return indexcache.NewTracingIndexCache(c, logger), nil
 }
+
+func newRedisIndexCache(cfg cache.RedisClientConfig, logger log.Logger, registerer prometheus.Registerer) (indexcache.IndexCache, error) {
+	client, err := cache.NewRedisClient(logger, "index-cache", cfg, prometheus.WrapRegistererWithPrefix("thanos_", registerer))


We had a discussion to deprecate thanos_ metrics prefix. Should we just use cortex_ prefix for the new change in this PR? Applicable also for memcached client metrics prefix few lines above.

I'd rather make the change to all cache metrics at the same time. It's the same amount of work if we support cortex_ for Redis metrics and thanos_ for Memcached vs using thanos_ for both now and cortex_ for both later.

To be more specific, using thanos_ here now means Redis will work with our dashboards as currently written with no extra effort. Then, they can be migrated to use the cortex_ prefix at the same time as the Memcached backend also with no extra effort.

I agree with Nick. I think for now we should just aim for consistency (unfortunately...).

pracucci

Very good job, LGTM! I just left a couple of comments I would be glad if you could look at before merging. I would love to merge this PR asap, cause I want to add metadata cache support to compactor and I would love to do it on top of this. Thanks!

pracucci · 2023-03-07T09:54:24Z

docs/sources/mimir/operators-guide/configure/about-versioning.md

 - Query-scheduler
  - `-query-scheduler.querier-forget-delay`
  - Max number of used instances (`-query-scheduler.max-used-instances`)
 - Store-gateway
  - `-blocks-storage.bucket-store.chunks-cache.fine-grained-chunks-caching-enabled`
+  - Use of Redis cache backend (`-blocks-storage.bucket-store.chunks-cache.backend=redis`, `-blocks-storage.bucket-store.index-cache.backend=redis`, `-blocks-storage.bucket-store.metadata-cache.backend=redis`)


[nit] The metadata cache is used also by the querier.

pracucci · 2023-03-07T10:01:43Z

pkg/storage/tsdb/index_cache.go

-	cfg.Memcached.RegisterFlagsWithPrefix(f, prefix+"memcached.")
+	cfg.InMemory.RegisterFlagsWithPrefix(prefix+"inmemory.", f)
+	cfg.Memcached.RegisterFlagsWithPrefix(prefix+"memcached.", f)
+	cfg.Redis.RegisterFlagsWithPrefix(prefix+"redis.", f)
 }

 // Validate the config.


Can you change the IndexCacheConfig.Validate() below to run the validation on BackendConfig.Validate() instead of just BackendConfig.Memcached.Validate(), please?

D'oh, will do.

Exposing the errors here so I can write a test against the validation method: grafana/dskit#276

pracucci · 2023-03-07T10:02:22Z

pkg/storage/tsdb/index_cache.go

@@ -119,3 +125,17 @@ func newMemcachedIndexCache(cfg cache.MemcachedConfig, logger log.Logger, regist

 	return indexcache.NewTracingIndexCache(c, logger), nil
 }
+
+func newRedisIndexCache(cfg cache.RedisClientConfig, logger log.Logger, registerer prometheus.Registerer) (indexcache.IndexCache, error) {
+	client, err := cache.NewRedisClient(logger, "index-cache", cfg, prometheus.WrapRegistererWithPrefix("thanos_", registerer))


I agree with Nick. I think for now we should just aim for consistency (unfortunately...).

See grafana/mimir#4371 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

…276) * Export Redis config validation errors so they can be tested against See grafana/mimir#4371 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

pracucci

LGTM, thanks!

This change pulls in a version of dskit that supports using Redis implementations for the `cache.Cache` and `cache.RemoteCacheClient` interfaces. The query-frontend caching integration test now runs with both Memcached and Redis caches. Redis can also be enabled for the microservices docker compose environment. See #4236 Fixes #4341 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

56quarters force-pushed the 56quarters/enable-redis-support branch 2 times, most recently from ea0aa80 to 8e15750 Compare March 3, 2023 20:03

56quarters marked this pull request as ready for review March 3, 2023 21:28

56quarters requested review from a team as code owners March 3, 2023 21:28

56quarters mentioned this pull request Mar 3, 2023

Support Redis as backend for all Mimir caches #4236

Closed

charleskorn reviewed Mar 6, 2023

View reviewed changes

cmd/mimir/help.txt.tmpl Outdated Show resolved Hide resolved

lamida reviewed Mar 6, 2023

View reviewed changes

56quarters mentioned this pull request Mar 6, 2023

Add cache selection config in mimir jsonnet #4386

Merged

3 tasks

pracucci approved these changes Mar 7, 2023

View reviewed changes

56quarters added a commit to grafana/dskit that referenced this pull request Mar 7, 2023

Export Redis config validation errors so they can be tested against

f198f6c

See grafana/mimir#4371 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

56quarters mentioned this pull request Mar 7, 2023

Export Redis config validation errors so they can be tested against grafana/dskit#276

Merged

2 tasks

pracucci approved these changes Mar 7, 2023

View reviewed changes

56quarters added 4 commits March 7, 2023 12:32

Changelog + update error messages

5398c18

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

Format cache backends consistently

fdd103e

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

Code review changes

07eff10

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

56quarters force-pushed the 56quarters/enable-redis-support branch from ba50778 to 07eff10 Compare March 7, 2023 17:35

Regenerate docs

268e57d

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

56quarters merged commit 607d7dc into main Mar 7, 2023

56quarters deleted the 56quarters/enable-redis-support branch March 7, 2023 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow use of Redis for all caches #4371

Allow use of Redis for all caches #4371

56quarters commented Mar 3, 2023 •

edited

Loading

56quarters commented Mar 3, 2023

lamida Mar 6, 2023 •

edited

Loading

56quarters Mar 6, 2023

56quarters Mar 6, 2023

pracucci Mar 7, 2023

pracucci left a comment

pracucci Mar 7, 2023

pracucci Mar 7, 2023

56quarters Mar 7, 2023

56quarters Mar 7, 2023

pracucci Mar 7, 2023

pracucci left a comment

Allow use of Redis for all caches #4371

Allow use of Redis for all caches #4371

Conversation

56quarters commented Mar 3, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

56quarters commented Mar 3, 2023

lamida Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

56quarters commented Mar 3, 2023 •

edited

Loading

lamida Mar 6, 2023 •

edited

Loading