Enable bucket index by default #924

andyasp · 2022-01-27T14:43:11Z

What this PR does:

Enables the bucket index by default

~~WIP: Looking into the test failures~~

Which issue(s) this PR fixes:

Fixes https://github.com/grafana/mimir-squad/issues/498

Checklist

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

pracucci · 2022-01-27T15:54:04Z

Thanks for working on this! I agree on enabling it by default. Changing all integration tests could be annoying (and would make them even more fragile cause we have to run compactor, wait for it, ...). I would suggest to selectively disable -blocks-storage.bucket-store.bucket-index.enabled in integration tests querying the long-term storage where it's not already enabled.

andyasp · 2022-01-28T19:48:23Z

I'll work on this PR next week. I'm having some issues running integration tests locally that I still haven't been able to resolve.

pracucci · 2022-01-31T11:41:54Z

I'll work on this PR next week. I'm having some issues running integration tests locally that I still haven't been able to resolve.

Feel free to reach out if you need help.

andyasp · 2022-02-02T18:30:06Z

I was able to iterate on the integration tests locally. They still fail when trying to run too many at once, but that wasn't a blocker here. Thanks to @treid314 for helping me look at the last unit test failure, which was an interaction with a mock not expecting index read attempts.

pkg/storage/tsdb/config.go

pkg/storegateway/gateway_test.go

pkg/storegateway/bucket_stores_test.go

pracucci

Good job! I left few minor comments I would be glad to see addressed before we merge it.

pracucci · 2022-02-03T15:52:44Z

operations/mimir/config.libsonnet

@@ -434,5 +434,7 @@

    // Bucket index is updated by compactor on each cleanup cycle.
    'blocks-storage.bucket-store.sync-interval': $._config.compactor_cleanup_interval,
-  } else {},
+  } else {
+    'blocks-storage.bucket-store.bucket-index.enabled': false,


I would do the opposite in our jsonnet. If bucket_index_enabled is true (and you change the default it to true) then we don't add blocks-storage.bucket-store.bucket-index.enabled': true, because that's the default, otherwise we add 'blocks-storage.bucket-store.bucket-index.enabled': false, as you did.

If I understand correctly, you wanted the blocks-storage.bucket-store.bucket-index.enabled': true to be deleted. I remember wondering about this when I made the change. Is the intention to never set flags that are defaults to minimize the configuration and let future changes be picked up more easily?

Is the intention to never set flags that are defaults to minimize the configuration and let future changes be picked up more easily?

Yes, that's the idea, whenever it makes sense. I think in this case it does.

pracucci · 2022-02-03T15:53:59Z

pkg/storage/tsdb/config.go

@@ -340,7 +340,7 @@ type BucketIndexConfig struct {
 }

 func (cfg *BucketIndexConfig) RegisterFlagsWithPrefix(f *flag.FlagSet, prefix string) {
-	f.BoolVar(&cfg.Enabled, prefix+"enabled", false, "True to enable querier and store-gateway to discover blocks in the storage via bucket index instead of bucket scanning.")
+	f.BoolVar(&cfg.Enabled, prefix+"enabled", true, "If enabled, a querier/store-gateway can discover blocks by reading a bucket index instead of bucket scanning.")


[nit] I just wanna make sure it's not they "can" (as an option) but they only discover blocks this way.

Suggested change

f.BoolVar(&cfg.Enabled, prefix+"enabled", true, "If enabled, a querier/store-gateway can discover blocks by reading a bucket index instead of bucket scanning.")

f.BoolVar(&cfg.Enabled, prefix+"enabled", true, "If enabled, queriers and store-gateways discover blocks by reading a bucket index (created and updated by the compactor) instead of periodically scanning the bucket.")

pkg/storegateway/bucket_stores_test.go

pracucci · 2022-02-03T15:58:51Z

CHANGELOG.md

@@ -256,6 +256,7 @@
 * [CHANGE] Alertmanager: `-experimental.alertmanager.enable-api` flag has been renamed to `-alertmanager.enable-api` and is now stable. #913
 * [CHANGE] Ingester: changed default value of `-blocks-storage.tsdb.retention-period` from `6h` to `24h`. #966
 * [CHANGE] Changed default value of `-querier.query-ingesters-within` and `-blocks-storage.tsdb.close-idle-tsdb-timeout` from `0` to `13h`. #966
+* [CHANGE] Changed the default value of `-blocks-storage.bucket-store.bucket-index.enabled` to `true`. #924


We should also mention that this means that running compactor is mandatory with the default configuration, because the bucket index is written by the compactor and if it's missing queries on long-term storage will not work.

pracucci · 2022-02-03T15:59:31Z

operations/mimir/config.libsonnet

@@ -60,7 +60,7 @@

    // Enable use of bucket index by querier, ruler and store-gateway.
    // Bucket index is generated by compactor from Cortex 1.7, there is no flag required to enable this on compactor.
-    bucket_index_enabled: false,
+    bucket_index_enabled: true,


Please mention this change in the CHANGELOG section related to the mixin.

I didn't even realize that was a section of the CHANGELOG. Is the following sufficient or should it also mention the compactor requirement?

[CHANGE] Changed the default of `bucket_index_enabled` to `true`. #924

No need to mention the compactor requirement because if you use our jsonnet the compactor is already always running.

CHANGELOG section related to the mixin.

I made a mistake. Not the mixin, but the jsonnet one (they're 2 distinct sections). Mixin is just dashboards + alerts.

👍 swapped to the jsonnet section.

pracucci · 2022-02-03T16:03:21Z

docs/sources/blocks-storage/bucket-index.md

@@ -34,7 +34,7 @@ The `bucket-index.json.gz` contains:

 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`.


At the top of this file you can find:

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).

Could you rephrase it to clarify that it's enabled by default, please?

I think we can say something like: bucket index is enabled by default,b ut can be disabled via -blocks-storage.bucket-store.bucket-index.enabled=false (not recommended).

Went with:

The bucket index is enabled by default, but it is optional. It can be disabled via -blocks-storage.bucket-store.bucket-index.enabled=false (or its respective YAML config option). Disabling the bucket index is not recommended.

pracucci

LGTM (modulo a nit I'm just going to merge)

docs/sources/blocks-storage/bucket-index.md

pracucci · 2022-02-07T09:00:41Z

operations/mimir/config.libsonnet

@@ -434,5 +434,7 @@

    // Bucket index is updated by compactor on each cleanup cycle.
    'blocks-storage.bucket-store.sync-interval': $._config.compactor_cleanup_interval,
-  } else {},
+  } else {
+    'blocks-storage.bucket-store.bucket-index.enabled': false,


Is the intention to never set flags that are defaults to minimize the configuration and let future changes be picked up more easily?

Yes, that's the idea, whenever it makes sense. I think in this case it does.

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

andyasp · 2022-02-07T20:43:07Z

Thanks for the help with getting this in!

andyasp force-pushed the aasp/enable-bucket-index branch from 1d02f04 to f9e6c8d Compare January 27, 2022 14:47

andyasp force-pushed the aasp/enable-bucket-index branch from f9e6c8d to df5344a Compare January 27, 2022 16:29

andyasp force-pushed the aasp/enable-bucket-index branch 2 times, most recently from 843f946 to 208efc3 Compare February 2, 2022 18:12

andyasp marked this pull request as ready for review February 2, 2022 18:30

treid314 approved these changes Feb 2, 2022

View reviewed changes

pkg/storage/tsdb/config.go Outdated Show resolved Hide resolved

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

replay reviewed Feb 3, 2022

View reviewed changes

pkg/storegateway/bucket_stores_test.go Show resolved Hide resolved

andyasp force-pushed the aasp/enable-bucket-index branch 3 times, most recently from 36dd1c0 to ad7f7e2 Compare February 3, 2022 15:06

pracucci self-requested a review February 3, 2022 15:50

pracucci reviewed Feb 3, 2022

View reviewed changes

andyasp added 6 commits February 4, 2022 12:03

Enable bucket index by default

c91d2eb

Address review comments

efa6b4e

Update documentation

c5fe607

Address review comments

45a2d86

Generate doc/test changes

7f74769

Move CHANGELOG entry from mixin to jsonnet

007c1fc

andyasp force-pushed the aasp/enable-bucket-index branch from 03b8c8f to 007c1fc Compare February 4, 2022 17:05

pracucci approved these changes Feb 7, 2022

View reviewed changes

Update docs/sources/blocks-storage/bucket-index.md

03777a4

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

pracucci merged commit bb843d6 into main Feb 7, 2022

pracucci deleted the aasp/enable-bucket-index branch February 7, 2022 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable bucket index by default #924

Enable bucket index by default #924

andyasp commented Jan 27, 2022 •

edited

Loading

pracucci commented Jan 27, 2022

andyasp commented Jan 28, 2022

pracucci commented Jan 31, 2022

andyasp commented Feb 2, 2022

pracucci left a comment

pracucci Feb 3, 2022

andyasp Feb 3, 2022

pracucci Feb 7, 2022

pracucci Feb 3, 2022

pracucci Feb 3, 2022

pracucci Feb 3, 2022

andyasp Feb 3, 2022

pracucci Feb 3, 2022

andyasp Feb 3, 2022

pracucci Feb 3, 2022

pracucci Feb 3, 2022

andyasp Feb 3, 2022

pracucci left a comment

pracucci Feb 7, 2022

andyasp commented Feb 7, 2022

	f.BoolVar(&cfg.Enabled, prefix+"enabled", true, "If enabled, a querier/store-gateway can discover blocks by reading a bucket index instead of bucket scanning.")
	f.BoolVar(&cfg.Enabled, prefix+"enabled", true, "If enabled, queriers and store-gateways discover blocks by reading a bucket index (created and updated by the compactor) instead of periodically scanning the bucket.")

		@@ -34,7 +34,7 @@ The `bucket-index.json.gz` contains:

		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`.

Enable bucket index by default #924

Enable bucket index by default #924

Conversation

andyasp commented Jan 27, 2022 • edited Loading

pracucci commented Jan 27, 2022

andyasp commented Jan 28, 2022

pracucci commented Jan 31, 2022

andyasp commented Feb 2, 2022

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

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

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

andyasp commented Feb 7, 2022

andyasp commented Jan 27, 2022 •

edited

Loading