Allow dynamic replication set of TSDB blocks on store-gateways #10382

56quarters · 2025-01-08T17:05:15Z

What this PR does

This change adds a setting to allow a subset of TSDB blocks to be replicated to more than the configured replication factor (three by default) on store-gateways.

The DynamicReplication interface in pkg/storegateway/dynamic_replication.go is the core of this change. This new bit of logic determines if a block is eligible to be replicated over more than the default number of store-gateways. It is shared between store-gateways (which must do the replication) and queriers (which need to know which store-gateways to query). It defaults to only doing this for blocks that have samples within the last 25h (configurable). Most of the rest of the changes are using block metadata in places where we previously only used block IDs (ulid.ULID).

Assumptions made in this change:

Default / dynamic replication is driven only by how recent the block is and if the setting is on, nothing fancy.
Dynamic replication means replication to the entire user sub-ring. There are no degrees of extra replication.
The threshold for dynamic replication is a global (not per-tenant) setting.

Which issue(s) this PR fixes or relates to

Fixes #9944

Related grafana/dskit#632

Notes for reviewers

The first commit is the main change to concentrate on which introduces an interface DynamicReplication and implementations which are shared between queriers and store-gateways.

The second commit is changes to the mimir-microservices-mode docker-compose setup to make testing store-gateway changes easier.

Checklist

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

github-actions · 2025-01-08T17:06:40Z

💻 Deploy preview available: https://deploy-preview-mimir-10382-zb444pucvq-vp.a.run.app/docs/mimir/latest/

dimitarvdimitrov

I'm not sure what would happen when a block just crosses the 25h boundary. What if store-gateways drop it before the querier realizes that the block is that old (or the querier made the decision for that 1m ago for example)? You pointed out we retry a fixed 3 times. It's not impossible we retry the block on other store-gateways that have dropped it.

Maybe we should fix the retries before merging this PR. Charles suggested retrying as much as the replication factor (recent blocks we retry "NumInstances" times and others only 3 times)

pkg/querier/blocks_store_queryable.go

pkg/querier/blocks_store_replicated_set.go

56quarters · 2025-01-10T16:20:17Z

I'm not sure what would happen when a block just crosses the 25h boundary. What if store-gateways drop it before the querier realizes that the block is that old (or the querier made the decision for that 1m ago for example)? You pointed out we retry a fixed 3 times. It's not impossible we retry the block on other store-gateways that have dropped it.

Maybe we should fix the retries before merging this PR. Charles suggested retrying as much as the replication factor (recent blocks we retry "NumInstances" times and others only 3 times)

Good point. We could increase the number of retries but I'd rather solve this with some sort of "buffer time" like we do with the upload grace period. Maybe queriers could stop trying to query a block at 25h - $sync_interval * 3 or something similar.

56quarters · 2025-01-10T22:29:35Z

pkg/storegateway/expanded_replication.go

+func (e *MaxTimeExpandedReplication) EligibleForQuerying(b QueryableReplicatedBlock) bool {
+	now := e.now()
+	uploadedDelta := now.Sub(b.GetUploadedAt())
+	return uploadedDelta > e.gracePeriod && e.isMaxTimeInWindow(b, now)


This grace period is for the time the block was uploaded but I think we also need one for the max time? If a block was uploaded hours ago, we still want it to ~~be synced by the store-gateways before the queriers start expecting store-gateways to have it~~ stop being queried by queriers before store-gateways unload it.

Fixed via 450dda1

charleskorn · 2025-01-12T22:18:43Z

pkg/storegateway/expanded_replication.go

+	// To be eligible for querying a block must:
+	// * Have been uploaded more than `gracePeriod` ago since we need to allow store-gateways
+	//   to sync recently uploaded blocks.


Why do we need to check the upload grace period here? Won't this be covered by the logic in BlocksConsistency?

You're right that removing this wouldn't cause issues with queries failing or correctness. However, if we expand the replication set without the grace period we'd end up querying more store-gateways than we would otherwise that may not have the block. So by taking the grace period into account we avoid a little extra work for recently uploaded blocks. I can remove this in the interest of keeping the code simpler.

However, if we expand the replication set without the grace period we'd end up querying more store-gateways than we would otherwise that may not have the block

after the first iteration of the consistency retries, the consistency tracker would exclude the recently uploaded blocks from missingBlocks so we won't be querying them anymore because the constructor of BlocksConsistency excluded them. But we'd still end up querying them once. Maybe what we should do is reassign knownBlocks after we've created the consistencyTracker.

That looks somewhat tangential to this though.

charleskorn

LGTM modulo suggestion about how we handle the grace period

pkg/storegateway/expanded_replication.go

pkg/storegateway/gateway.go

dimitarvdimitrov

one tiny comment on naming, but otherwise, this looks good 🚀

pkg/storegateway/expanded_replication.go

dimitarvdimitrov · 2025-01-20T13:05:53Z

pkg/storegateway/expanded_replication.go

+	// To be eligible for querying a block must:
+	// * Have been uploaded more than `gracePeriod` ago since we need to allow store-gateways
+	//   to sync recently uploaded blocks.


However, if we expand the replication set without the grace period we'd end up querying more store-gateways than we would otherwise that may not have the block

after the first iteration of the consistency retries, the consistency tracker would exclude the recently uploaded blocks from missingBlocks so we won't be querying them anymore because the constructor of BlocksConsistency excluded them. But we'd still end up querying them once. Maybe what we should do is reassign knownBlocks after we've created the consistencyTracker.

That looks somewhat tangential to this though.

This change adds a tenant-specific setting to allow a subset of TSDB blocks to be replicated to more than the configured replication factor (three by default) on store-gateways.

Much like how block uploads have a grace period, add a grace period when querying with expanded replication to make sure store-gateways have enough time to load blocks before they are queried. Signed-off-by: Nick Pillitteri <[email protected]>

Signed-off-by: Nick Pillitteri <[email protected]>

Upload time is checked (and blocks within a grace period are ignored) by the query consistency check. Signed-off-by: Nick Pillitteri <[email protected]>

Apply grace period to sync instead of querying Signed-off-by: Nick Pillitteri <[email protected]>

Signed-off-by: Nick Pillitteri <[email protected]>

Pulls in grafana/dskit#632 Signed-off-by: Nick Pillitteri <[email protected]>

Signed-off-by: Nick Pillitteri <[email protected]>

charleskorn

🚀

56quarters force-pushed the 56quarters/sg-replication branch 5 times, most recently from 872d7ca to a3b09a6 Compare January 9, 2025 22:12

dimitarvdimitrov reviewed Jan 10, 2025

View reviewed changes

pkg/querier/blocks_store_queryable.go Outdated Show resolved Hide resolved

pkg/querier/blocks_store_replicated_set.go Outdated Show resolved Hide resolved

56quarters force-pushed the 56quarters/sg-replication branch from 9a2b10e to 655630e Compare January 10, 2025 22:12

56quarters commented Jan 10, 2025

View reviewed changes

charleskorn reviewed Jan 12, 2025

View reviewed changes

56quarters force-pushed the 56quarters/sg-replication branch from 3d3ff7c to d96f5b6 Compare January 13, 2025 17:24

charleskorn approved these changes Jan 14, 2025

View reviewed changes

pkg/storegateway/expanded_replication.go Outdated Show resolved Hide resolved

56quarters force-pushed the 56quarters/sg-replication branch from 305ff5e to 48d3d73 Compare January 14, 2025 14:52

56quarters changed the title ~~Allow expanded replication set of TSDB blocks on store-gateways~~ Allow dynamic replication set of TSDB blocks on store-gateways Jan 14, 2025

56quarters force-pushed the 56quarters/sg-replication branch from 9d725ea to 5aa26f6 Compare January 14, 2025 20:13

charleskorn reviewed Jan 14, 2025

View reviewed changes

pkg/storegateway/gateway.go Outdated Show resolved Hide resolved

dimitarvdimitrov approved these changes Jan 20, 2025

View reviewed changes

56quarters force-pushed the 56quarters/sg-replication branch from 744614e to 60a61ce Compare January 22, 2025 19:47

56quarters marked this pull request as ready for review January 28, 2025 22:43

56quarters requested review from stevesg, grafanabot, a team and tacole02 as code owners January 28, 2025 22:43

56quarters added 6 commits January 28, 2025 17:45

Allow expanded replication set of TSDB blocks on store-gateways

8870a15

This change adds a tenant-specific setting to allow a subset of TSDB blocks to be replicated to more than the configured replication factor (three by default) on store-gateways.

mimir-microservices-mode: Use default store-gateway replication factor

0a5ce6f

Code review feedback

ad4ff77

Signed-off-by: Nick Pillitteri <[email protected]>

Apply grace period to max time _and_ upload time.

89e1662

Signed-off-by: Nick Pillitteri <[email protected]>

Don't check upload time in expanded replication

2d75f56

Upload time is checked (and blocks within a grace period are ignored) by the query consistency check. Signed-off-by: Nick Pillitteri <[email protected]>

56quarters added 7 commits January 28, 2025 17:45

Code review changes

b1149a3

Apply grace period to sync instead of querying Signed-off-by: Nick Pillitteri <[email protected]>

Rename feature to "dynamic_replication"

1971da7

Signed-off-by: Nick Pillitteri <[email protected]>

Fix comment about how grace period works

c0bb0a5

Signed-off-by: Nick Pillitteri <[email protected]>

Update comments about dynamic replication grace period

bb31f62

Signed-off-by: Nick Pillitteri <[email protected]>

Update to latest dskit commit

403f510

Signed-off-by: Nick Pillitteri <[email protected]>

Switch to latest dskit main

2541b9b

Pulls in grafana/dskit#632 Signed-off-by: Nick Pillitteri <[email protected]>

Code review changes

32f88af

Signed-off-by: Nick Pillitteri <[email protected]>

56quarters force-pushed the 56quarters/sg-replication branch from 6a94bff to 32f88af Compare January 28, 2025 22:47

charleskorn approved these changes Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow dynamic replication set of TSDB blocks on store-gateways #10382

Allow dynamic replication set of TSDB blocks on store-gateways #10382

56quarters commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

dimitarvdimitrov left a comment

56quarters commented Jan 10, 2025

56quarters Jan 10, 2025 •

edited

Loading

56quarters Jan 10, 2025

charleskorn Jan 12, 2025

56quarters Jan 13, 2025

dimitarvdimitrov Jan 20, 2025

charleskorn left a comment

dimitarvdimitrov left a comment

dimitarvdimitrov Jan 20, 2025

charleskorn left a comment

Allow dynamic replication set of TSDB blocks on store-gateways #10382

Are you sure you want to change the base?

Allow dynamic replication set of TSDB blocks on store-gateways #10382

Conversation

56quarters commented Jan 8, 2025 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Notes for reviewers

Checklist

github-actions bot commented Jan 8, 2025 • edited Loading

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

56quarters commented Jan 10, 2025

56quarters Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

56quarters Jan 10, 2025

Choose a reason for hiding this comment

charleskorn Jan 12, 2025

Choose a reason for hiding this comment

56quarters Jan 13, 2025

Choose a reason for hiding this comment

dimitarvdimitrov Jan 20, 2025

Choose a reason for hiding this comment

charleskorn left a comment

Choose a reason for hiding this comment

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

dimitarvdimitrov Jan 20, 2025

Choose a reason for hiding this comment

charleskorn left a comment

Choose a reason for hiding this comment

56quarters commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

56quarters Jan 10, 2025 •

edited

Loading