Remove deprecated configurations #6673

leizor · 2023-11-15T18:52:56Z

What this PR does

In preparation for release 2.11, this PR performs the scheduled removals of the following deprecated configuration parameters:

-querier.query-ingesters-within
-querier.iterators
-querier.batch-iterators
-blocks-storage.bucket-store.max-chunk-pool-bytes
-blocks-storage.bucket-store.chunk-pool-min-bucket-size-bytes
-blocks-storage.bucket-store.chunk-pool-max-bucket-size-bytes
-blocks-storage.bucket-store.bucket-index.enabled

I've separated each deprecation removal into its own commit so it might be easiest to review this PR one commit at a time.

Which issue(s) this PR fixes or relates to

#6670

Checklist

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

The `-querier.query-ingesters-within` config has been moved from a global config to a per-tenant limit config. See #4287 for more details.

The `-querier.iterators` and `-querier.batch-iterators` configuration parameters have been removed. See #5114 for more details.

The following deprecated flags are removed: - `-blocks-storage.bucket-store.max-chunk-pool-bytes` - `-blocks-storage.bucket-store.chunk-pool-min-bucket-size-bytes` - `-blocks-storage.bucket-store.chunk-pool-max-bucket-size-bytes` See #4996 for more details.

This configuration parameter has been removed. Mimir is running with bucket index enabled by default since 2.0 and it is now not possible to disable it. See #5051 for more details.

ying-jeanne · 2023-11-16T20:05:18Z

CHANGELOG.md

@@ -4,6 +4,14 @@

 ### Grafana Mimir

+* [CHANGE] The following deprecated configurations have been removed: #6673
+  * `-querier.query-ingesters-within`


We probably aslo need to update runbook https://github.com/grafana/mimir/blob/main/docs/sources/mimir/manage/mimir-runbooks/_index.md. -querier.query-ingesters-within is still mentioned.

I think the flag still exists, but it's now a tenant-level override

56quarters

LGTM

Followup to #6673 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

dimitarvdimitrov · 2023-11-29T13:21:57Z

pkg/querier/blocks_store_queryable.go

-	var finder BlocksFinder
-	if storageCfg.BucketStore.BucketIndex.DeprecatedEnabled {
-		finder = NewBucketIndexBlocksFinder(BucketIndexBlocksFinderConfig{
-			IndexLoader: bucketindex.LoaderConfig{
-				CheckInterval:         time.Minute,
-				UpdateOnStaleInterval: storageCfg.BucketStore.SyncInterval,
-				UpdateOnErrorInterval: storageCfg.BucketStore.BucketIndex.UpdateOnErrorInterval,
-				IdleTimeout:           storageCfg.BucketStore.BucketIndex.IdleTimeout,
-			},
-			MaxStalePeriod:           storageCfg.BucketStore.BucketIndex.MaxStalePeriod,
-			IgnoreDeletionMarksDelay: storageCfg.BucketStore.IgnoreDeletionMarksDelay,
-		}, bucketClient, limits, logger, reg)
-	} else {
-		finder = NewBucketScanBlocksFinder(BucketScanBlocksFinderConfig{
-			ScanInterval:             storageCfg.BucketStore.SyncInterval,
-			TenantsConcurrency:       storageCfg.BucketStore.TenantSyncConcurrency,
-			MetasConcurrency:         storageCfg.BucketStore.MetaSyncConcurrency,
-			CacheDir:                 storageCfg.BucketStore.SyncDir,
-			IgnoreDeletionMarksDelay: storageCfg.BucketStore.IgnoreDeletionMarksDelay,
-		}, bucketClient, limits, logger, reg)
-	}


the default should be that the index is always used, not that it's never used

The use of BucketScanBlocksFinder was supposed to be removed in #6673 but its counterpart BucketIndexBlocksFinder was accidentally removed instead. This reverses that.

* Use BucketIndexBlocksFinder instead of BucketScanBlocksFinder The use of BucketScanBlocksFinder was supposed to be removed in #6673 but its counterpart BucketIndexBlocksFinder was accidentally removed instead. This reverses that. * Remove BlocksFinderBucketScan * Update docs wrt. bucket scanning option * Remove alert and runbook for bucket scanning

* Use BucketIndexBlocksFinder instead of BucketScanBlocksFinder The use of BucketScanBlocksFinder was supposed to be removed in #6673 but its counterpart BucketIndexBlocksFinder was accidentally removed instead. This reverses that. * Remove BlocksFinderBucketScan * Update docs wrt. bucket scanning option * Remove alert and runbook for bucket scanning (cherry picked from commit ff8a70a)

…6790) * Use BucketIndexBlocksFinder instead of BucketScanBlocksFinder The use of BucketScanBlocksFinder was supposed to be removed in #6673 but its counterpart BucketIndexBlocksFinder was accidentally removed instead. This reverses that. * Remove BlocksFinderBucketScan * Update docs wrt. bucket scanning option * Remove alert and runbook for bucket scanning (cherry picked from commit ff8a70a) Co-authored-by: Justin Lei <lei.justin@gmail.com>

dimitarvdimitrov · 2023-12-04T14:30:04Z

pkg/storegateway/bucket_stores.go

-	var fetcher block.MetadataFetcher
-	if u.cfg.BucketStore.BucketIndex.DeprecatedEnabled {
-		fetcher = NewBucketIndexMetadataFetcher(
-			userID,
-			u.bucket,
-			u.limits,
-			u.logger,
-			fetcherReg,
-			filters,
-		)
-	} else {
-		var err error
-		fetcher, err = block.NewMetaFetcher(
-			userLogger,
-			u.cfg.BucketStore.MetaSyncConcurrency,
-			userBkt,
-			u.syncDirForUser(userID), // The fetcher stores cached metas in the "meta-syncer/" sub directory
-			fetcherReg,
-			filters,
-		)
-		if err != nil {
-			return nil, err
-		}


opened #6808 to make the bucket index the default

pracucci · 2023-12-04T14:37:19Z

integration/querier_test.go

+			// Wait until the querier has discovered the uploaded blocks.
+			require.NoError(t, querier.WaitSumMetrics(e2e.Equals(2), "cortex_blocks_meta_synced"))


Why did we keep this, which was running when bucket index was disabled?

Fixed by #6779

pracucci · 2023-12-04T14:37:45Z

integration/querier_test.go

+			// Wait until the querier has discovered the uploaded blocks (discovered both by the querier and store-gateway).
+			require.NoError(t, cluster.WaitSumMetricsWithOptions(e2e.Equals(float64(2*cluster.NumInstances()*2)), []string{"cortex_blocks_meta_synced"}, e2e.WithLabelMatchers(
+				labels.MustNewMatcher(labels.MatchEqual, "component", "querier"))))


Same here

Fixed by #6779

pracucci · 2023-12-04T14:39:52Z

pkg/querier/blocks_store_queryable.go

@@ -209,27 +209,13 @@ func NewBlocksStoreQueryableFromConfig(querierCfg Config, gatewayCfg storegatewa
 	bucketClient = cachingBucket

 	// Create the blocks finder.
-	var finder BlocksFinder
-	if storageCfg.BucketStore.BucketIndex.DeprecatedEnabled {
-		finder = NewBucketIndexBlocksFinder(BucketIndexBlocksFinderConfig{


Wrong removal, fixed by #6779

pracucci · 2023-12-04T14:44:05Z

pkg/querier/querier.go

 	queryMetrics := stats.NewQueryMetrics(reg)

-	distributorQueryable := newDistributorQueryable(distributor, iteratorFunc, limits, queryMetrics, logger)
+	distributorQueryable := newDistributorQueryable(distributor, mergeChunks, limits, queryMetrics, logger)


Why mergeChunks? The default was -querier.batch-iterators=true which means batch.NewChunkMergeIterator was in use.

changed in #6814

pracucci · 2023-12-04T14:50:02Z

pkg/storegateway/bucket_stores.go

@@ -448,29 +448,20 @@ func (u *BucketStores) getOrCreateStore(userID string) (*BucketStore, error) {
 	}

 	// Instantiate a different blocks metadata fetcher based on whether bucket index is enabled or not.
-	var fetcher block.MetadataFetcher
-	if u.cfg.BucketStore.BucketIndex.DeprecatedEnabled {
-		fetcher = NewBucketIndexMetadataFetcher(


Fixed by #6808

dimitarvdimitrov · 2023-12-04T15:00:38Z

pkg/querier/querier_test.go

-
-	var cfg Config
-	flagext.DefaultValues(&cfg)
-	cfg.BatchIterators = true // Always use the Batch iterator - regression test


given this was testing with batch iterators, then should we also keep that test (assuming that batch iterators are always on from now on)?

I think we want to restore it. It was introduced earlier this year: #4376

restored in #6814

dimitarvdimitrov

I realized this PR doesn't update about-versioning.md which mentions these config options as deprecated. Can you update it @leizor ?

leizor force-pushed the leizor/prep-release-2.11 branch 6 times, most recently from 9159a1f to abb0dac Compare November 15, 2023 22:05

leizor added 2 commits November 15, 2023 14:11

Remove -querier.query-ingesters-within config

1d6581c

The `-querier.query-ingesters-within` config has been moved from a global config to a per-tenant limit config. See #4287 for more details.

Remove -querier.iterators and -querier.batch-iterators

cc2cbda

The `-querier.iterators` and `-querier.batch-iterators` configuration parameters have been removed. See #5114 for more details.

leizor force-pushed the leizor/prep-release-2.11 branch from abb0dac to 3b3ecdd Compare November 15, 2023 22:12

leizor changed the title ~~Prep for release 2.11~~ Remove deprecated configurations Nov 15, 2023

leizor force-pushed the leizor/prep-release-2.11 branch from 3b3ecdd to f9366f5 Compare November 15, 2023 22:20

leizor force-pushed the leizor/prep-release-2.11 branch 3 times, most recently from 7592ea0 to 5e5a68c Compare November 16, 2023 17:41

leizor added 2 commits November 16, 2023 11:40

Remove -blocks-storage.bucket-store.bucket-index.enabled config

d75c63f

This configuration parameter has been removed. Mimir is running with bucket index enabled by default since 2.0 and it is now not possible to disable it. See #5051 for more details.

Update CHANGELOG.md

94fc1cb

leizor force-pushed the leizor/prep-release-2.11 branch from 5e5a68c to 94fc1cb Compare November 16, 2023 19:40

leizor marked this pull request as ready for review November 16, 2023 19:56

leizor requested review from a team as code owners November 16, 2023 19:56

ying-jeanne reviewed Nov 16, 2023

View reviewed changes

56quarters approved these changes Nov 17, 2023

View reviewed changes

leizor merged commit da7527d into main Nov 17, 2023
30 checks passed

leizor deleted the leizor/prep-release-2.11 branch November 17, 2023 22:39

krajorama added a commit that referenced this pull request Nov 21, 2023

Fix changelog, flag not removed

1584b6b

Followup to #6673 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama mentioned this pull request Nov 21, 2023

Fix changelog, flag not removed #6699

Merged

1 task

krajorama added a commit that referenced this pull request Nov 21, 2023

Fix changelog, flag not removed (#6699)

8359e55

Followup to #6673 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

dimitarvdimitrov reviewed Nov 29, 2023

View reviewed changes

leizor mentioned this pull request Nov 29, 2023

Use BucketIndexBlocksFinder instead of BucketScanBlocksFinder #6779

Merged

4 tasks

dimitarvdimitrov mentioned this pull request Dec 4, 2023

store-gateway: use bucket index instead of scanning the bucket #6808

Merged

dimitarvdimitrov reviewed Dec 4, 2023

View reviewed changes

pracucci reviewed Dec 4, 2023

View reviewed changes

dimitarvdimitrov reviewed Dec 4, 2023

View reviewed changes

dimitarvdimitrov mentioned this pull request Dec 4, 2023

querier: default to using batch.NewChunkMergeIterator #6814

Merged

dimitarvdimitrov reviewed Dec 11, 2023

View reviewed changes

charleskorn mentioned this pull request Dec 13, 2023

Deprecate and eventually remove non-default chunk iterators #5107

Closed

leizor mentioned this pull request Dec 19, 2023

Delete docs on removed configuration parameters #6967

Merged

4 tasks

duricanikolic mentioned this pull request Feb 7, 2024

Get rid of iterators.chunkIterator and iterators.chunkMergeIterator #7313

Merged

2 tasks

eyenx mentioned this pull request May 28, 2024

chore(mimir): update to 5.3.0 adfinis/helm-charts#1256

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecated configurations #6673

Remove deprecated configurations #6673

leizor commented Nov 15, 2023 •

edited

Loading

ying-jeanne Nov 16, 2023 •

edited

Loading

leizor Nov 16, 2023

56quarters left a comment

dimitarvdimitrov Nov 29, 2023

dimitarvdimitrov Dec 4, 2023

pracucci Dec 4, 2023

pracucci Dec 4, 2023

pracucci Dec 4, 2023

pracucci Dec 4, 2023

pracucci Dec 4, 2023

pracucci Dec 4, 2023

dimitarvdimitrov Dec 11, 2023

pracucci Dec 4, 2023

dimitarvdimitrov Dec 4, 2023

pracucci Dec 4, 2023

dimitarvdimitrov Dec 11, 2023

dimitarvdimitrov left a comment

		// Wait until the querier has discovered the uploaded blocks.
		require.NoError(t, querier.WaitSumMetrics(e2e.Equals(2), "cortex_blocks_meta_synced"))

Remove deprecated configurations #6673

Remove deprecated configurations #6673

Conversation

leizor commented Nov 15, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

ying-jeanne Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

56quarters 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

Choose a reason for hiding this comment

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

leizor commented Nov 15, 2023 •

edited

Loading

ying-jeanne Nov 16, 2023 •

edited

Loading