Add bucket index support to querier #3614

pracucci · 2020-12-17T11:13:15Z

What this PR does:
In this PR I'm adding the bucket index support to querier. Using the bucket index in the querier is optional and disabled by default (and will be until marked experimental).

Which issue(s) this PR fixes:
N/A

Checklist

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

Signed-off-by: Marco Pracucci <[email protected]>

pstibrany

Wow, great job! No major problems found, logic is solid. 👏

pkg/querier/blocks_finder_bucket_scan.go

pkg/querier/blocks_finder_bucket_index.go

pkg/storage/tsdb/caching_bucket.go

pkg/storage/tsdb/bucketindex/loader.go

pkg/querier/blocks_finder_bucket_index_test.go

pkg/storage/tsdb/bucketindex/loader_test.go

Signed-off-by: Marco Pracucci <[email protected]>

pracucci · 2020-12-17T14:33:11Z

Thanks @pstibrany for your deep review! After reading your comments I realised it was definitely a draft what I've submitted 😉 I've addressed all comments, except a couple I've commented about.

Signed-off-by: Marco Pracucci <[email protected]>

pstibrany · 2020-12-17T16:59:17Z

docs/blocks-storage/bucket-index.md

+
+The bucket index is a **per-tenant file containing the list of blocks and block deletion marks** in the storage. The bucket index itself is stored in the backend object storage, is periodically updated by the compactor and used by queriers to discover blocks in the storage.
+
+The bucket index usage is **optional** and can be enabled via `-blocks-storage.bucket-store.bucket-index.enabled=true` (or its respective YAML config option).


Explain why it is useful?

Done. WDYT?

Signed-off-by: Marco Pracucci <[email protected]>

pstibrany · 2020-12-17T18:04:34Z

docs/blocks-storage/bucket-index.md

+
+If a bucket index is unused for a long time (configurable via `-blocks-storage.bucket-store.bucket-index.idle-timeout`), e.g. because that querier instance is not receiving any query from the tenant, the querier will offload it, stopping to keep it updated at regular intervals. This is particularly for tenants which are resharded to different queriers when [shuffle sharding](../guides/shuffle-sharding.md) is enabled.
+
+Finally, the querier, at query time, checks how old is a bucket index (based on its `updated_at`) and fail a query if its age is older than `-blocks-storage.bucket-store.bucket-index.max-stale-period`. This circuit breaker is used to ensure queriers will not return any partial query results due to a stale view over the long-term storage.


You have also added caching of index into caching-bucket. Is that worth mentioning as well?

Yes, sure, done. WDYT?

docs/blocks-storage/compactor.md

docs/blocks-storage/querier.md

Signed-off-by: Marco Pracucci <[email protected]>

ranton256 · 2020-12-17T23:26:16Z

docs/blocks-storage/_index.md

 The **[store-gateway](./store-gateway.md)** is responsible to query blocks and is used by the [querier](./querier.md) at query time. The store-gateway is required when running the blocks storage.

-The **[compactor](./compactor.md)** is responsible to merge and deduplicate smaller blocks into larger ones, in order to reduce the number of blocks stored in the long-term storage for a given tenant and query them more efficiently. It also keeps the bucket index updated and, for this reason, it's a required component.
+The **[compactor](./compactor.md)** is responsible to merge and deduplicate smaller blocks into larger ones, in order to reduce the number of blocks stored in the long-term storage for a given tenant and query them more efficiently. It also keeps the [bucket index](./bucket-index.md) updated and, for this reason, it's a required component.


Is this going to cause issues for users running with compactor now(not sure if there are such people)?

Also, bucket-index.md says the bucket index is optional which seems like it contradicts this statement.

Is this going to cause issues for users running with compactor now(not sure if there are such people)?

Did you mean "with compactor" or "without compactor"? I'm no sure I understand the question.

Also, bucket-index.md says the bucket index is optional which seems like it contradicts this statement.

The compactor always writes the bucket index. The flag to enable bucket index is whether it should be used or not in queriers (and in the store-gateway in the upcoming PR too). The point is that, before enabling the bucket index in the queriers and store-gateway, you have to rollout the compactor first, so that the bucket index for all tenants is created before you enable it in querier and store-gateway. To simplify it (at least I thought it would have simplified), I've kept it always enabled in the compactor.

ranton256 · 2020-12-17T23:29:01Z

docs/blocks-storage/bucket-index.md

+
+The [compactor](./compactor.md) periodically scans the bucket and uploads an updated bucket index to the storage. The frequency at which the bucket index is updated can be configured via `-compactor.cleanup-interval`.
+
+The bucket index is built and updated by the compactor even if `-blocks-storage.bucket-store.bucket-index.enabled` has **not** been enabled. This is intentional and the overhead introduced by keeping the bucket index is non significative.


This mostly answers my earlier question about non-optional compactor to create optional bucket index, but it still may be a bit unclear to someone new.

I've updated the doc. Is it more clear now?

ranton256 · 2020-12-17T23:31:43Z

docs/blocks-storage/bucket-index.md

+
+If a bucket index is unused for a long time (configurable via `-blocks-storage.bucket-store.bucket-index.idle-timeout`), e.g. because that querier instance is not receiving any query from the tenant, the querier will offload it, stopping to keep it updated at regular intervals. This is particularly for tenants which are resharded to different queriers when [shuffle sharding](../guides/shuffle-sharding.md) is enabled.
+
+Finally, the querier, at query time, checks how old is a bucket index (based on its `updated_at`) and fail a query if its age is older than `-blocks-storage.bucket-store.bucket-index.max-stale-period`. This circuit breaker is used to ensure queriers will not return any partial query results due to a stale view over the long-term storage.


It isn't possible to fall back to the behavior used when the bucket index is not enabled? Or this is undesirable or some reason?

I don't think falling back is a viable solution. The "bucket scan" logic requires a preventive bucket scanning, which we don't do when we enable the bucket index. Lazily bucket scanning would be too slow to do at query time. Moreover, fallback logic unfrequently exercised would bring further risks that fallback logic doesn't work as expected.

In my opinion, when you run Cortex with bucket index, the bucket index is an essential part of the system and it's required to be kept updated. The compactor already exports a metric with the timestamp of the last time the bucket index of each tenant has been updated, so that we can alert on it before the max-stale-period is reached.

What do you think?

docs/blocks-storage/querier.md

pkg/storage/tsdb/bucketindex/loader.go

Signed-off-by: Marco Pracucci <[email protected]>

pracucci · 2020-12-21T12:48:04Z

@pstibrany @ranton256 I'm going to merge this PR, but feel free to add further comments (if any). I will promptly address post-merge comments too.

* Introduced BucketIndexBlocksFinder Signed-off-by: Marco Pracucci <[email protected]> * Improved and tested bucket index Loader Signed-off-by: Marco Pracucci <[email protected]> * Integrated bucket index in querier and added caching support Signed-off-by: Marco Pracucci <[email protected]> * Fixed unit tests Signed-off-by: Marco Pracucci <[email protected]> * Fixed typo in comment Signed-off-by: Marco Pracucci <[email protected]> * Fail queries if bucket index is too old Signed-off-by: Marco Pracucci <[email protected]> * Fixed linter Signed-off-by: Marco Pracucci <[email protected]> * Addressed review comments Signed-off-by: Marco Pracucci <[email protected]> * Addressed more comments Signed-off-by: Marco Pracucci <[email protected]> * Updated doc Signed-off-by: Marco Pracucci <[email protected]> * Added CHANGELOG entry Signed-off-by: Marco Pracucci <[email protected]> * Added integration test Signed-off-by: Marco Pracucci <[email protected]> * Fixed integration test Signed-off-by: Marco Pracucci <[email protected]> * Replaced 1st with first Signed-off-by: Marco Pracucci <[email protected]> * Loader refactoring Signed-off-by: Marco Pracucci <[email protected]> * Clarified max size cached item Signed-off-by: Marco Pracucci <[email protected]> * Updated bucket index doc Signed-off-by: Marco Pracucci <[email protected]> * Updated doc Signed-off-by: Marco Pracucci <[email protected]>

pracucci added 4 commits December 17, 2020 09:36

Introduced BucketIndexBlocksFinder

c922c4a

Signed-off-by: Marco Pracucci <[email protected]>

Improved and tested bucket index Loader

a8bf92d

Signed-off-by: Marco Pracucci <[email protected]>

Integrated bucket index in querier and added caching support

e1e9039

Signed-off-by: Marco Pracucci <[email protected]>

Fixed unit tests

e93aa5a

Signed-off-by: Marco Pracucci <[email protected]>

pracucci requested a review from pstibrany December 17, 2020 11:13

pull-request-size bot added the size/XXL label Dec 17, 2020

pracucci added 3 commits December 17, 2020 12:22

Fixed typo in comment

7aedad1

Signed-off-by: Marco Pracucci <[email protected]>

Fail queries if bucket index is too old

5139f2d

Signed-off-by: Marco Pracucci <[email protected]>

Fixed linter

601f05c

Signed-off-by: Marco Pracucci <[email protected]>

pstibrany approved these changes Dec 17, 2020

View reviewed changes

Addressed review comments

1367aec

Signed-off-by: Marco Pracucci <[email protected]>

pracucci added 3 commits December 17, 2020 15:42

Addressed more comments

1f8c3f6

Signed-off-by: Marco Pracucci <[email protected]>

Updated doc

b3a4080

Signed-off-by: Marco Pracucci <[email protected]>

Added CHANGELOG entry

c466b56

Signed-off-by: Marco Pracucci <[email protected]>

pstibrany reviewed Dec 17, 2020

View reviewed changes

pracucci added 2 commits December 17, 2020 18:08

Added integration test

31b83bc

Signed-off-by: Marco Pracucci <[email protected]>

Fixed integration test

9f6748d

Signed-off-by: Marco Pracucci <[email protected]>

pstibrany reviewed Dec 17, 2020

View reviewed changes

docs/blocks-storage/compactor.md Outdated Show resolved Hide resolved

pstibrany reviewed Dec 17, 2020

View reviewed changes

docs/blocks-storage/querier.md Outdated Show resolved Hide resolved

Replaced 1st with first

65d9bff

Signed-off-by: Marco Pracucci <[email protected]>

ranton256 reviewed Dec 18, 2020

View reviewed changes

pracucci added 2 commits December 18, 2020 08:58

Loader refactoring

716710c

Signed-off-by: Marco Pracucci <[email protected]>

Clarified max size cached item

9ed6fd7

Signed-off-by: Marco Pracucci <[email protected]>

ranton256 approved these changes Dec 18, 2020

View reviewed changes

pracucci added 2 commits December 19, 2020 15:47

Updated bucket index doc

7e9b43b

Signed-off-by: Marco Pracucci <[email protected]>

Updated doc

a89c483

Signed-off-by: Marco Pracucci <[email protected]>

pracucci merged commit 8460a88 into cortexproject:master Dec 21, 2020

pracucci mentioned this pull request Dec 21, 2020

Add bucket index support to store gateway #3625

Merged

3 tasks


		The bucket index is a per-tenant file containing the list of blocks and block deletion marks in the storage. The bucket index itself is stored in the backend object storage, is periodically updated by the compactor and used by queriers to discover blocks in the storage.

		The bucket index usage is optional and can be enabled via `-blocks-storage.bucket-store.bucket-index.enabled=true` (or its respective YAML config option).


		If a bucket index is unused for a long time (configurable via `-blocks-storage.bucket-store.bucket-index.idle-timeout`), e.g. because that querier instance is not receiving any query from the tenant, the querier will offload it, stopping to keep it updated at regular intervals. This is particularly for tenants which are resharded to different queriers when [shuffle sharding](../guides/shuffle-sharding.md) is enabled.

		Finally, the querier, at query time, checks how old is a bucket index (based on its `updated_at`) and fail a query if its age is older than `-blocks-storage.bucket-store.bucket-index.max-stale-period`. This circuit breaker is used to ensure queriers will not return any partial query results due to a stale view over the long-term storage.


		The [compactor](./compactor.md) periodically scans the bucket and uploads an updated bucket index to the storage. The frequency at which the bucket index is updated can be configured via `-compactor.cleanup-interval`.

		The bucket index is built and updated by the compactor even if `-blocks-storage.bucket-store.bucket-index.enabled` has not been enabled. This is intentional and the overhead introduced by keeping the bucket index is non significative.

Add bucket index support to querier #3614

Add bucket index support to querier #3614

Uh oh!

Conversation

pracucci commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci commented Dec 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pracucci commented Dec 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pracucci commented Dec 17, 2020 •

edited

Loading