From 1d6581c6a07d8f851990bacbe0cfdd7186a89de9 Mon Sep 17 00:00:00 2001 From: Justin Lei Date: Wed, 15 Nov 2023 10:47:52 -0800 Subject: [PATCH 1/5] Remove -querier.query-ingesters-within config The `-querier.query-ingesters-within` config has been moved from a global config to a per-tenant limit config. See https://github.com/grafana/mimir/pull/4287 for more details. --- pkg/mimir/mimir_test.go | 25 ------------------------- pkg/mimir/modules.go | 9 --------- pkg/querier/querier.go | 10 ++-------- 3 files changed, 2 insertions(+), 42 deletions(-) diff --git a/pkg/mimir/mimir_test.go b/pkg/mimir/mimir_test.go index 36cbbd78488..d7c49d35a6b 100644 --- a/pkg/mimir/mimir_test.go +++ b/pkg/mimir/mimir_test.go @@ -472,31 +472,6 @@ func TestConfig_ValidateLimits(t *testing.T) { } } -// Temporary behavior to keep supporting global query-ingesters-within flag for two Mimir versions -// TODO: Remove in Mimir 2.11.0 -func TestQueryIngestersWithinGlobalConfigIsUsedInsteadOfDefaultLimitConfig(t *testing.T) { - dir := t.TempDir() - - cfg := Config{} - flagext.DefaultValues(&cfg) - - cfg.Querier.QueryIngestersWithin = 5 * time.Hour - cfg.Target = []string{Overrides} - - cfg.RuntimeConfig.LoadPath = []string{filepath.Join(dir, "config.yaml")} - - c, err := New(cfg, prometheus.NewPedanticRegistry()) - require.NoError(t, err) - - _, err = c.ModuleManager.InitModuleServices(cfg.Target...) - require.NoError(t, err) - defer c.Server.Stop() - - duration := c.Overrides.QueryIngestersWithin("test") - - require.Equal(t, 5*time.Hour, duration) -} - func TestConfig_validateFilesystemPaths(t *testing.T) { cwd, err := os.Getwd() require.NoError(t, err) diff --git a/pkg/mimir/modules.go b/pkg/mimir/modules.go index d382abd75f2..54f7080a2e2 100644 --- a/pkg/mimir/modules.go +++ b/pkg/mimir/modules.go @@ -28,7 +28,6 @@ import ( "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/config" - "github.com/prometheus/common/model" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/rules" prom_storage "github.com/prometheus/prometheus/storage" @@ -353,14 +352,6 @@ func (t *Mimir) initRuntimeConfig() (services.Service, error) { loader := runtimeConfigLoader{validate: t.Cfg.ValidateLimits} t.Cfg.RuntimeConfig.Loader = loader.load - // QueryIngestersWithin is moving from a global config that can in the querier yaml to a limit config - // We need to preserve the option in the querier yaml for two releases - // If the querier config is configured by the user, the default limit is overwritten - // TODO: Remove in Mimir 2.11.0 - if t.Cfg.Querier.QueryIngestersWithin != querier.DefaultQuerierCfgQueryIngestersWithin { - t.Cfg.LimitsConfig.QueryIngestersWithin = model.Duration(t.Cfg.Querier.QueryIngestersWithin) - } - // DeprecatedCacheUnalignedRequests is moving from a global config that can in the frontend yaml to a limit config // We need to preserve the option in the frontend yaml for two releases // If the frontend config is configured by the user, the default limit is overwritten diff --git a/pkg/querier/querier.go b/pkg/querier/querier.go index 8293b5f5dcf..bebde88bb6c 100644 --- a/pkg/querier/querier.go +++ b/pkg/querier/querier.go @@ -40,9 +40,8 @@ import ( // Config contains the configuration require to create a querier type Config struct { - Iterators bool `yaml:"iterators" category:"deprecated"` // Deprecated: Deprecated in Mimir 2.9.0, remove in Mimir 2.11.0 (https://github.com/grafana/mimir/issues/5107) - BatchIterators bool `yaml:"batch_iterators" category:"deprecated"` // Deprecated: Deprecated in Mimir 2.9.0, remove in Mimir 2.11.0 (https://github.com/grafana/mimir/issues/5107) - QueryIngestersWithin time.Duration `yaml:"query_ingesters_within" category:"advanced" doc:"hidden"` // Deprecated: Deprecated in Mimir 2.9.0, remove in Mimir 2.11.0 + Iterators bool `yaml:"iterators" category:"deprecated"` // Deprecated: Deprecated in Mimir 2.9.0, remove in Mimir 2.11.0 (https://github.com/grafana/mimir/issues/5107) + BatchIterators bool `yaml:"batch_iterators" category:"deprecated"` // Deprecated: Deprecated in Mimir 2.9.0, remove in Mimir 2.11.0 (https://github.com/grafana/mimir/issues/5107) // QueryStoreAfter the time after which queries should also be sent to the store and not just ingesters. QueryStoreAfter time.Duration `yaml:"query_store_after" category:"advanced"` @@ -98,11 +97,6 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.Uint64Var(&cfg.StreamingChunksPerIngesterSeriesBufferSize, "querier.streaming-chunks-per-ingester-buffer-size", 256, "Number of series to buffer per ingester when streaming chunks from ingesters.") f.Uint64Var(&cfg.StreamingChunksPerStoreGatewaySeriesBufferSize, "querier.streaming-chunks-per-store-gateway-buffer-size", 256, "Number of series to buffer per store-gateway when streaming chunks from store-gateways.") - // The querier.query-ingesters-within flag has been moved to the limits.go file - // We still need to set a default value for cfg.QueryIngestersWithin since we need to keep supporting the querier yaml field until Mimir 2.11.0 - // TODO: Remove in Mimir 2.11.0 - cfg.QueryIngestersWithin = DefaultQuerierCfgQueryIngestersWithin - cfg.EngineConfig.RegisterFlags(f) } From cc2cbda41be82f7485eb5caaa6ee67f37a6c3a38 Mon Sep 17 00:00:00 2001 From: Justin Lei Date: Wed, 15 Nov 2023 11:39:02 -0800 Subject: [PATCH 2/5] Remove -querier.iterators and -querier.batch-iterators The `-querier.iterators` and `-querier.batch-iterators` configuration parameters have been removed. See https://github.com/grafana/mimir/pull/5114 for more details. --- cmd/mimir/config-descriptor.json | 22 ---- cmd/mimir/help-all.txt.tmpl | 4 - .../configuration-parameters/index.md | 11 -- pkg/querier/querier.go | 23 +--- pkg/querier/querier_test.go | 110 +++--------------- 5 files changed, 15 insertions(+), 155 deletions(-) diff --git a/cmd/mimir/config-descriptor.json b/cmd/mimir/config-descriptor.json index 4e067924a45..0df7a897392 100644 --- a/cmd/mimir/config-descriptor.json +++ b/cmd/mimir/config-descriptor.json @@ -1658,28 +1658,6 @@ "required": false, "desc": "", "blockEntries": [ - { - "kind": "field", - "name": "iterators", - "required": false, - "desc": "Use iterators to execute query, as opposed to fully materialising the series in memory.", - "fieldValue": null, - "fieldDefaultValue": false, - "fieldFlag": "querier.iterators", - "fieldType": "boolean", - "fieldCategory": "deprecated" - }, - { - "kind": "field", - "name": "batch_iterators", - "required": false, - "desc": "Use batch iterators to execute query, as opposed to fully materialising the series in memory. Takes precedent over the -querier.iterators flag.", - "fieldValue": null, - "fieldDefaultValue": true, - "fieldFlag": "querier.batch-iterators", - "fieldType": "boolean", - "fieldCategory": "deprecated" - }, { "kind": "field", "name": "query_store_after", diff --git a/cmd/mimir/help-all.txt.tmpl b/cmd/mimir/help-all.txt.tmpl index 2aaaca4adf5..a612c101cf4 100644 --- a/cmd/mimir/help-all.txt.tmpl +++ b/cmd/mimir/help-all.txt.tmpl @@ -1611,8 +1611,6 @@ Usage of ./cmd/mimir/mimir: Minimum time to wait for ring stability at startup, if set to positive value. Set to 0 to disable. -print.config Print the config and exit. - -querier.batch-iterators - [deprecated] Use batch iterators to execute query, as opposed to fully materialising the series in memory. Takes precedent over the -querier.iterators flag. (default true) -querier.cardinality-analysis-enabled Enables endpoints used for cardinality analysis. -querier.default-evaluation-interval duration @@ -1667,8 +1665,6 @@ Usage of ./cmd/mimir/mimir: Override the expected name on the server certificate. -querier.id string Querier ID, sent to the query-frontend to identify requests from the same querier. Defaults to hostname. - -querier.iterators - [deprecated] Use iterators to execute query, as opposed to fully materialising the series in memory. -querier.label-names-and-values-results-max-size-bytes int Maximum size in bytes of distinct label names and values. When querier receives response from ingester, it merges the response with responses from other ingesters. This maximum size limit is applied to the merged(distinct) results. If the limit is reached, an error is returned. (default 419430400) -querier.label-values-max-cardinality-label-names-per-request int diff --git a/docs/sources/mimir/references/configuration-parameters/index.md b/docs/sources/mimir/references/configuration-parameters/index.md index ee36bea7a4c..4f681bdd262 100644 --- a/docs/sources/mimir/references/configuration-parameters/index.md +++ b/docs/sources/mimir/references/configuration-parameters/index.md @@ -1168,17 +1168,6 @@ instance_limits: The `querier` block configures the querier. ```yaml -# (deprecated) Use iterators to execute query, as opposed to fully materialising -# the series in memory. -# CLI flag: -querier.iterators -[iterators: | default = false] - -# (deprecated) Use batch iterators to execute query, as opposed to fully -# materialising the series in memory. Takes precedent over the -# -querier.iterators flag. -# CLI flag: -querier.batch-iterators -[batch_iterators: | default = true] - # (advanced) The time after which a metric should be queried from storage and # not just ingesters. 0 means all queries are sent to store. If this option is # enabled, the time range of the query sent to the store-gateway will be diff --git a/pkg/querier/querier.go b/pkg/querier/querier.go index bebde88bb6c..1c78142c0db 100644 --- a/pkg/querier/querier.go +++ b/pkg/querier/querier.go @@ -24,9 +24,7 @@ import ( "github.com/prometheus/prometheus/util/annotations" "golang.org/x/sync/errgroup" - "github.com/grafana/mimir/pkg/querier/batch" "github.com/grafana/mimir/pkg/querier/engine" - "github.com/grafana/mimir/pkg/querier/iterators" "github.com/grafana/mimir/pkg/querier/stats" "github.com/grafana/mimir/pkg/storage/chunk" "github.com/grafana/mimir/pkg/storage/lazyquery" @@ -40,9 +38,6 @@ import ( // Config contains the configuration require to create a querier type Config struct { - Iterators bool `yaml:"iterators" category:"deprecated"` // Deprecated: Deprecated in Mimir 2.9.0, remove in Mimir 2.11.0 (https://github.com/grafana/mimir/issues/5107) - BatchIterators bool `yaml:"batch_iterators" category:"deprecated"` // Deprecated: Deprecated in Mimir 2.9.0, remove in Mimir 2.11.0 (https://github.com/grafana/mimir/issues/5107) - // QueryStoreAfter the time after which queries should also be sent to the store and not just ingesters. QueryStoreAfter time.Duration `yaml:"query_store_after" category:"advanced"` MaxQueryIntoFuture time.Duration `yaml:"max_query_into_future" category:"advanced"` @@ -78,10 +73,6 @@ var ( func (cfg *Config) RegisterFlags(f *flag.FlagSet) { cfg.StoreGatewayClient.RegisterFlagsWithPrefix("querier.store-gateway-client", f) - // TODO: these two flags were deprecated in Mimir 2.9.0, remove them in Mimir 2.11.0 (https://github.com/grafana/mimir/issues/5107) - f.BoolVar(&cfg.Iterators, "querier.iterators", false, "Use iterators to execute query, as opposed to fully materialising the series in memory.") - f.BoolVar(&cfg.BatchIterators, "querier.batch-iterators", true, "Use batch iterators to execute query, as opposed to fully materialising the series in memory. Takes precedent over the -querier.iterators flag.") - f.DurationVar(&cfg.MaxQueryIntoFuture, "querier.max-query-into-future", 10*time.Minute, "Maximum duration into the future you can query. 0 to disable.") f.DurationVar(&cfg.QueryStoreAfter, queryStoreAfterFlag, 12*time.Hour, "The time after which a metric should be queried from storage and not just ingesters. 0 means all queries are sent to store. If this option is enabled, the time range of the query sent to the store-gateway will be manipulated to ensure the query end is not more recent than 'now - query-store-after'.") f.BoolVar(&cfg.ShuffleShardingIngestersEnabled, "querier.shuffle-sharding-ingesters-enabled", true, fmt.Sprintf("Fetch in-memory series from the minimum set of required ingesters, selecting only ingesters which may have received series since -%s. If this setting is false or -%s is '0', queriers always query all ingesters (ingesters shuffle sharding on read path is disabled).", validation.QueryIngestersWithinFlag, validation.QueryIngestersWithinFlag)) @@ -115,15 +106,6 @@ func (cfg *Config) ValidateLimits(limits validation.Limits) error { return nil } -func getChunksIteratorFunction(cfg Config) chunkIteratorFunc { - if cfg.BatchIterators { - return batch.NewChunkMergeIterator - } else if cfg.Iterators { - return iterators.NewChunkMergeIterator - } - return mergeChunks -} - // ShouldQueryIngesters provides a check for whether the ingesters will be used for a given query. func ShouldQueryIngesters(queryIngestersWithin time.Duration, now time.Time, queryMaxT int64) bool { if queryIngestersWithin != 0 { @@ -148,12 +130,11 @@ func ShouldQueryBlockStore(queryStoreAfter time.Duration, now time.Time, queryMi // New builds a queryable and promql engine. func New(cfg Config, limits *validation.Overrides, distributor Distributor, storeQueryable storage.Queryable, reg prometheus.Registerer, logger log.Logger, tracker *activitytracker.ActivityTracker) (storage.SampleAndChunkQueryable, storage.ExemplarQueryable, *promql.Engine) { - iteratorFunc := getChunksIteratorFunction(cfg) queryMetrics := stats.NewQueryMetrics(reg) - distributorQueryable := newDistributorQueryable(distributor, iteratorFunc, limits, queryMetrics, logger) + distributorQueryable := newDistributorQueryable(distributor, mergeChunks, limits, queryMetrics, logger) - queryable := newQueryable(distributorQueryable, storeQueryable, iteratorFunc, cfg, limits, queryMetrics, logger) + queryable := newQueryable(distributorQueryable, storeQueryable, mergeChunks, cfg, limits, queryMetrics, logger) exemplarQueryable := newDistributorExemplarQueryable(distributor, logger) lazyQueryable := storage.QueryableFunc(func(minT int64, maxT int64) (storage.Querier, error) { diff --git a/pkg/querier/querier_test.go b/pkg/querier/querier_test.go index 2a52e4d7597..5b71d01dc8a 100644 --- a/pkg/querier/querier_test.go +++ b/pkg/querier/querier_test.go @@ -207,26 +207,22 @@ func TestQuerier(t *testing.T) { }, } - for _, query := range queries { - for _, iterators := range []bool{false, true} { - t.Run(fmt.Sprintf("%s/iterators=%t", query.query, iterators), func(t *testing.T) { - // Generate TSDB head used to simulate querying the long-term storage. - db, through := mockTSDB(t, model.Time(0), int(chunks*samplesPerChunk), sampleRate, chunkOffset, int(samplesPerChunk), query.valueType) + for _, q := range queries { + t.Run(q.query, func(t *testing.T) { + // Generate TSDB head used to simulate querying the long-term storage. + db, through := mockTSDB(t, model.Time(0), int(chunks*samplesPerChunk), sampleRate, chunkOffset, int(samplesPerChunk), q.valueType) - cfg.Iterators = iterators - - // No samples returned by ingesters. - distributor := &mockDistributor{} - distributor.On("Query", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&client.QueryResponse{}, nil) - distributor.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(client.CombinedQueryStreamResponse{}, nil) + // No samples returned by ingesters. + distributor := &mockDistributor{} + distributor.On("Query", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&client.QueryResponse{}, nil) + distributor.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(client.CombinedQueryStreamResponse{}, nil) - overrides, err := validation.NewOverrides(defaultLimitsConfig(), nil) - require.NoError(t, err) + overrides, err := validation.NewOverrides(defaultLimitsConfig(), nil) + require.NoError(t, err) - queryable, _, _ := New(cfg, overrides, distributor, db, nil, log.NewNopLogger(), nil) - testRangeQuery(t, queryable, through, query) - }) - } + queryable, _, _ := New(cfg, overrides, distributor, db, nil, log.NewNopLogger(), nil) + testRangeQuery(t, queryable, through, q) + }) } } @@ -320,86 +316,6 @@ func TestQuerier_QueryableReturnsChunksOutsideQueriedRange(t *testing.T) { }, m[0].Floats) } -// TestBatchMergeChunks is a regression test to catch one particular case -// when the Batch merger iterator was corrupting memory by not copying -// Batches by value because the Batch itself was not possible to copy -// by value. -func TestBatchMergeChunks(t *testing.T) { - var ( - logger = log.NewNopLogger() - queryStart = mustParseTime("2021-11-01T06:00:00Z") - queryEnd = mustParseTime("2021-11-01T06:01:00Z") - queryStep = time.Second - ) - - var cfg Config - flagext.DefaultValues(&cfg) - cfg.BatchIterators = true // Always use the Batch iterator - regression test - - limits := defaultLimitsConfig() - limits.QueryIngestersWithin = 0 // Always query ingesters in this test. - overrides, err := validation.NewOverrides(limits, nil) - require.NoError(t, err) - - s1 := []mimirpb.Sample{} - s2 := []mimirpb.Sample{} - - for i := 0; i < 12; i++ { - s1 = append(s1, mimirpb.Sample{Value: float64(i * 15000), TimestampMs: queryStart.Add(time.Duration(i) * time.Second).UnixMilli()}) - if i != 9 { // let series 3 miss a point - s2 = append(s2, mimirpb.Sample{Value: float64(i * 15000), TimestampMs: queryStart.Add(time.Duration(i) * time.Second).UnixMilli()}) - } - } - - c1 := convertToChunks(t, samplesToInterface(s1)) - c2 := convertToChunks(t, samplesToInterface(s2)) - chunks12 := []client.Chunk{} - chunks12 = append(chunks12, c1...) - chunks12 = append(chunks12, c2...) - - chunks21 := []client.Chunk{} - chunks21 = append(chunks21, c2...) - chunks21 = append(chunks21, c1...) - - // Mock distributor to return chunks that need merging. - distributor := &mockDistributor{} - distributor.On("QueryStream", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return( - client.CombinedQueryStreamResponse{ - Chunkseries: []client.TimeSeriesChunk{ - // Series with chunks in the 1,2 order, that need merge - { - Labels: []mimirpb.LabelAdapter{{Name: labels.MetricName, Value: "one"}, {Name: labels.InstanceName, Value: "foo"}}, - Chunks: chunks12, - }, - // Series with chunks in the 2,1 order, that need merge - { - Labels: []mimirpb.LabelAdapter{{Name: labels.MetricName, Value: "one"}, {Name: labels.InstanceName, Value: "bar"}}, - Chunks: chunks21, - }, - }, - }, - nil) - - engine := promql.NewEngine(promql.EngineOpts{ - Logger: logger, - MaxSamples: 1e6, - Timeout: 1 * time.Minute, - }) - - queryable, _, _ := New(cfg, overrides, distributor, nil, nil, logger, nil) - ctx := user.InjectOrgID(context.Background(), "user-1") - query, err := engine.NewRangeQuery(ctx, queryable, nil, `rate({__name__=~".+"}[10s])`, queryStart, queryEnd, queryStep) - require.NoError(t, err) - - r := query.Exec(ctx) - m, err := r.Matrix() - require.NoError(t, err) - - require.Equal(t, 2, m.Len()) - require.ElementsMatch(t, m[0].Floats, m[1].Floats) - require.ElementsMatch(t, m[0].Histograms, m[1].Histograms) -} - func mockTSDB(t *testing.T, mint model.Time, samples int, step, chunkOffset time.Duration, samplesPerChunk int, valueType func(model.Time) chunkenc.ValueType) (storage.Queryable, model.Time) { dir := t.TempDir() From 4eb6c90741f8ddf4a15da041fc4e93b3ed6a4e33 Mon Sep 17 00:00:00 2001 From: Justin Lei Date: Wed, 15 Nov 2023 12:43:39 -0800 Subject: [PATCH 3/5] Remove deprecated bucket store flags 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 https://github.com/grafana/mimir/pull/4996 for more details. --- cmd/mimir/config-descriptor.json | 33 ------------------- cmd/mimir/help-all.txt.tmpl | 6 ---- .../configuration-parameters/index.md | 13 -------- .../components/config/kustomization.yaml | 2 -- .../charts/mimir-distributed/CHANGELOG.md | 1 + pkg/storage/tsdb/config.go | 26 --------------- 6 files changed, 1 insertion(+), 80 deletions(-) diff --git a/cmd/mimir/config-descriptor.json b/cmd/mimir/config-descriptor.json index 0df7a897392..a6603c592d7 100644 --- a/cmd/mimir/config-descriptor.json +++ b/cmd/mimir/config-descriptor.json @@ -8118,39 +8118,6 @@ "fieldType": "duration", "fieldCategory": "advanced" }, - { - "kind": "field", - "name": "max_chunk_pool_bytes", - "required": false, - "desc": "Max size - in bytes - of a chunks pool, used to reduce memory allocations. The pool is shared across all tenants. 0 to disable the limit.", - "fieldValue": null, - "fieldDefaultValue": 2147483648, - "fieldFlag": "blocks-storage.bucket-store.max-chunk-pool-bytes", - "fieldType": "int", - "fieldCategory": "deprecated" - }, - { - "kind": "field", - "name": "chunk_pool_min_bucket_size_bytes", - "required": false, - "desc": "Size - in bytes - of the smallest chunks pool bucket.", - "fieldValue": null, - "fieldDefaultValue": 16000, - "fieldFlag": "blocks-storage.bucket-store.chunk-pool-min-bucket-size-bytes", - "fieldType": "int", - "fieldCategory": "deprecated" - }, - { - "kind": "field", - "name": "chunk_pool_max_bucket_size_bytes", - "required": false, - "desc": "Size - in bytes - of the largest chunks pool bucket.", - "fieldValue": null, - "fieldDefaultValue": 50000000, - "fieldFlag": "blocks-storage.bucket-store.chunk-pool-max-bucket-size-bytes", - "fieldType": "int", - "fieldCategory": "deprecated" - }, { "kind": "field", "name": "series_hash_cache_max_size_bytes", diff --git a/cmd/mimir/help-all.txt.tmpl b/cmd/mimir/help-all.txt.tmpl index a612c101cf4..38547f511cf 100644 --- a/cmd/mimir/help-all.txt.tmpl +++ b/cmd/mimir/help-all.txt.tmpl @@ -303,10 +303,6 @@ Usage of ./cmd/mimir/mimir: The maximum allowed age of a bucket index (last updated) before queries start failing because the bucket index is too old. The bucket index is periodically updated by the compactor, and this check is enforced in the querier (at query time). (default 1h0m0s) -blocks-storage.bucket-store.bucket-index.update-on-error-interval duration How frequently a bucket index, which previously failed to load, should be tried to load again. This option is used only by querier. (default 1m0s) - -blocks-storage.bucket-store.chunk-pool-max-bucket-size-bytes int - [deprecated] Size - in bytes - of the largest chunks pool bucket. (default 50000000) - -blocks-storage.bucket-store.chunk-pool-min-bucket-size-bytes int - [deprecated] Size - in bytes - of the smallest chunks pool bucket. (default 16000) -blocks-storage.bucket-store.chunks-cache.attributes-in-memory-max-items int Maximum number of object attribute items to keep in a first level in-memory LRU cache. Metadata will be stored and fetched in-memory before hitting the cache backend. 0 to disable the in-memory cache. (default 50000) -blocks-storage.bucket-store.chunks-cache.attributes-ttl duration @@ -527,8 +523,6 @@ Usage of ./cmd/mimir/mimir: [experimental] If enabled, store-gateway will persist a sparse version of the index-header to disk on construction and load sparse index-headers from disk instead of the whole index-header. (default true) -blocks-storage.bucket-store.index-header.verify-on-load If true, verify the checksum of index headers upon loading them (either on startup or lazily when lazy loading is enabled). Setting to true helps detect disk corruption at the cost of slowing down index header loading. - -blocks-storage.bucket-store.max-chunk-pool-bytes uint - [deprecated] Max size - in bytes - of a chunks pool, used to reduce memory allocations. The pool is shared across all tenants. 0 to disable the limit. (default 2147483648) -blocks-storage.bucket-store.max-concurrent int Max number of concurrent queries to execute against the long-term storage. The limit is shared across all tenants. (default 100) -blocks-storage.bucket-store.meta-sync-concurrency int diff --git a/docs/sources/mimir/references/configuration-parameters/index.md b/docs/sources/mimir/references/configuration-parameters/index.md index 4f681bdd262..0b16b836f03 100644 --- a/docs/sources/mimir/references/configuration-parameters/index.md +++ b/docs/sources/mimir/references/configuration-parameters/index.md @@ -3570,19 +3570,6 @@ bucket_store: # CLI flag: -blocks-storage.bucket-store.ignore-blocks-within [ignore_blocks_within: | default = 10h] - # (deprecated) Max size - in bytes - of a chunks pool, used to reduce memory - # allocations. The pool is shared across all tenants. 0 to disable the limit. - # CLI flag: -blocks-storage.bucket-store.max-chunk-pool-bytes - [max_chunk_pool_bytes: | default = 2147483648] - - # (deprecated) Size - in bytes - of the smallest chunks pool bucket. - # CLI flag: -blocks-storage.bucket-store.chunk-pool-min-bucket-size-bytes - [chunk_pool_min_bucket_size_bytes: | default = 16000] - - # (deprecated) Size - in bytes - of the largest chunks pool bucket. - # CLI flag: -blocks-storage.bucket-store.chunk-pool-max-bucket-size-bytes - [chunk_pool_max_bucket_size_bytes: | default = 50000000] - # (advanced) Max size - in bytes - of the in-memory series hash cache. The # cache is shared across all tenants and it's used only when query sharding is # enabled. diff --git a/operations/compare-helm-with-jsonnet/components/config/kustomization.yaml b/operations/compare-helm-with-jsonnet/components/config/kustomization.yaml index 136cf6bdee1..5600170057b 100644 --- a/operations/compare-helm-with-jsonnet/components/config/kustomization.yaml +++ b/operations/compare-helm-with-jsonnet/components/config/kustomization.yaml @@ -115,8 +115,6 @@ patches: path: /config/store_gateway/sharding_ring/tokens_file_path - op: remove path: /config/store_gateway/sharding_ring/wait_stability_min_duration - - op: remove - path: /config/blocks_storage/bucket_store/max_chunk_pool_bytes - target: kind: MimirConfig diff --git a/operations/helm/charts/mimir-distributed/CHANGELOG.md b/operations/helm/charts/mimir-distributed/CHANGELOG.md index 743cd887ef5..b5ecb94d3a5 100644 --- a/operations/helm/charts/mimir-distributed/CHANGELOG.md +++ b/operations/helm/charts/mimir-distributed/CHANGELOG.md @@ -28,6 +28,7 @@ Entries should include a reference to the Pull Request that introduced the chang ## main / unreleased +* [CHANGE] Remove deprecated configuration parameter `blocks_storage.bucket_store.max_chunk_pool_bytes`. #6673 * [CHANGE] Reduce `-server.grpc-max-concurrent-streams` from 1000 to 500 for ingester and to 100 for all components. #5666 * [CHANGE] Changed default `clusterDomain` from `cluster.local` to `cluster.local.` to reduce the number of DNS lookups made by Mimir. #6389 * [ENHANCEMENT] Update the `rollout-operator` subchart to `0.9.2`. #6022 #6110 #6558 diff --git a/pkg/storage/tsdb/config.go b/pkg/storage/tsdb/config.go index dccde6bc954..e16fce7ad83 100644 --- a/pkg/storage/tsdb/config.go +++ b/pkg/storage/tsdb/config.go @@ -79,12 +79,6 @@ const ( // posting list in the index. Each posting is 4 bytes (uint32) which are the offset of the series in the index file. BytesPerPostingInAPostingList = 4 - // ChunkPoolDefaultMinBucketSize is the default minimum bucket size (bytes) of the chunk pool. - ChunkPoolDefaultMinBucketSize = EstimatedMaxChunkSize // Deprecated. TODO: Remove in Mimir 2.11. - - // ChunkPoolDefaultMaxBucketSize is the default maximum bucket size (bytes) of the chunk pool. - ChunkPoolDefaultMaxBucketSize = 50e6 // Deprecated. TODO: Remove in Mimir 2.11. - // DefaultPostingOffsetInMemorySampling represents default value for --store.index-header-posting-offsets-in-mem-sampling. // 32 value is chosen as it's a good balance for common setups. Sampling that is not too large (too many CPU cycles) and // not too small (too much memory). @@ -102,9 +96,6 @@ const ( DefaultMaxTSDBOpeningConcurrencyOnStartup = 10 - maxChunksBytesPoolFlag = "blocks-storage.bucket-store.max-chunk-pool-bytes" - minBucketSizeBytesFlag = "blocks-storage.bucket-store.chunk-pool-min-bucket-size-bytes" - maxBucketSizeBytesFlag = "blocks-storage.bucket-store.chunk-pool-max-bucket-size-bytes" seriesSelectionStrategyFlag = "blocks-storage.bucket-store.series-selection-strategy" bucketIndexFlagPrefix = "blocks-storage.bucket-store.bucket-index." ) @@ -388,11 +379,6 @@ type BucketStoreConfig struct { BucketIndex BucketIndexConfig `yaml:"bucket_index"` IgnoreBlocksWithin time.Duration `yaml:"ignore_blocks_within" category:"advanced"` - // Chunk pool. - DeprecatedMaxChunkPoolBytes uint64 `yaml:"max_chunk_pool_bytes" category:"deprecated"` // Deprecated. TODO: Remove in Mimir 2.11. - DeprecatedChunkPoolMinBucketSizeBytes int `yaml:"chunk_pool_min_bucket_size_bytes" category:"deprecated"` // Deprecated. TODO: Remove in Mimir 2.11. - DeprecatedChunkPoolMaxBucketSizeBytes int `yaml:"chunk_pool_max_bucket_size_bytes" category:"deprecated"` // Deprecated. TODO: Remove in Mimir 2.11. - // Series hash cache. SeriesHashCacheMaxBytes uint64 `yaml:"series_hash_cache_max_size_bytes" category:"advanced"` @@ -444,9 +430,6 @@ func (cfg *BucketStoreConfig) RegisterFlags(f *flag.FlagSet) { f.StringVar(&cfg.SyncDir, "blocks-storage.bucket-store.sync-dir", "./tsdb-sync/", "Directory to store synchronized TSDB index headers. This directory is not required to be persisted between restarts, but it's highly recommended in order to improve the store-gateway startup time.") f.DurationVar(&cfg.SyncInterval, "blocks-storage.bucket-store.sync-interval", 15*time.Minute, "How frequently to scan the bucket, or to refresh the bucket index (if enabled), in order to look for changes (new blocks shipped by ingesters and blocks deleted by retention or compaction).") - f.Uint64Var(&cfg.DeprecatedMaxChunkPoolBytes, maxChunksBytesPoolFlag, uint64(2*units.Gibibyte), "Max size - in bytes - of a chunks pool, used to reduce memory allocations. The pool is shared across all tenants. 0 to disable the limit.") - f.IntVar(&cfg.DeprecatedChunkPoolMinBucketSizeBytes, minBucketSizeBytesFlag, ChunkPoolDefaultMinBucketSize, "Size - in bytes - of the smallest chunks pool bucket.") - f.IntVar(&cfg.DeprecatedChunkPoolMaxBucketSizeBytes, maxBucketSizeBytesFlag, ChunkPoolDefaultMaxBucketSize, "Size - in bytes - of the largest chunks pool bucket.") f.Uint64Var(&cfg.SeriesHashCacheMaxBytes, "blocks-storage.bucket-store.series-hash-cache-max-size-bytes", uint64(1*units.Gibibyte), "Max size - in bytes - of the in-memory series hash cache. The cache is shared across all tenants and it's used only when query sharding is enabled.") f.IntVar(&cfg.MaxConcurrent, "blocks-storage.bucket-store.max-concurrent", 100, "Max number of concurrent queries to execute against the long-term storage. The limit is shared across all tenants.") f.IntVar(&cfg.TenantSyncConcurrency, "blocks-storage.bucket-store.tenant-sync-concurrency", 10, "Maximum number of concurrent tenants synching blocks.") @@ -481,15 +464,6 @@ func (cfg *BucketStoreConfig) Validate(logger log.Logger) error { if err := cfg.BucketIndex.Validate(logger); err != nil { return errors.Wrap(err, "bucket-index configuration") } - if cfg.DeprecatedMaxChunkPoolBytes != uint64(2*units.Gibibyte) { - util.WarnDeprecatedConfig(maxChunksBytesPoolFlag, logger) - } - if cfg.DeprecatedChunkPoolMinBucketSizeBytes != ChunkPoolDefaultMinBucketSize { - util.WarnDeprecatedConfig(minBucketSizeBytesFlag, logger) - } - if cfg.DeprecatedChunkPoolMaxBucketSizeBytes != ChunkPoolDefaultMaxBucketSize { - util.WarnDeprecatedConfig(maxBucketSizeBytesFlag, logger) - } if !util.StringsContain(validSeriesSelectionStrategies, cfg.SeriesSelectionStrategyName) { return errors.New("invalid series-selection-strategy, set one of " + strings.Join(validSeriesSelectionStrategies, ", ")) } From d75c63f2757b7ab00e90dbefa9801b9f376b8853 Mon Sep 17 00:00:00 2001 From: Justin Lei Date: Wed, 15 Nov 2023 13:22:18 -0800 Subject: [PATCH 4/5] Remove -blocks-storage.bucket-store.bucket-index.enabled config 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 https://github.com/grafana/mimir/pull/5051 for more details. --- cmd/mimir/config-descriptor.json | 11 - cmd/mimir/help-all.txt.tmpl | 2 - .../configuration-parameters/index.md | 6 - integration/backward_compatibility_test.go | 4 + integration/configs.go | 3 - ...getting_started_with_gossiped_ring_test.go | 1 - integration/ingester_limits_test.go | 15 +- integration/querier_test.go | 34 +-- integration/ruler_test.go | 4 - pkg/querier/blocks_store_queryable.go | 28 +-- pkg/storage/tsdb/config.go | 7 +- pkg/storegateway/bucket_stores.go | 39 ++-- pkg/storegateway/bucket_stores_test.go | 1 - pkg/storegateway/gateway_test.go | 214 ++++++++---------- 14 files changed, 140 insertions(+), 229 deletions(-) diff --git a/cmd/mimir/config-descriptor.json b/cmd/mimir/config-descriptor.json index a6603c592d7..68076930a39 100644 --- a/cmd/mimir/config-descriptor.json +++ b/cmd/mimir/config-descriptor.json @@ -8059,17 +8059,6 @@ "required": false, "desc": "", "blockEntries": [ - { - "kind": "field", - "name": "enabled", - "required": false, - "desc": "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.", - "fieldValue": null, - "fieldDefaultValue": true, - "fieldFlag": "blocks-storage.bucket-store.bucket-index.enabled", - "fieldType": "boolean", - "fieldCategory": "deprecated" - }, { "kind": "field", "name": "update_on_error_interval", diff --git a/cmd/mimir/help-all.txt.tmpl b/cmd/mimir/help-all.txt.tmpl index 38547f511cf..9a1606a80da 100644 --- a/cmd/mimir/help-all.txt.tmpl +++ b/cmd/mimir/help-all.txt.tmpl @@ -295,8 +295,6 @@ Usage of ./cmd/mimir/mimir: This option controls how many series to fetch per batch. The batch size must be greater than 0. (default 5000) -blocks-storage.bucket-store.block-sync-concurrency int Maximum number of concurrent blocks synching per tenant. (default 20) - -blocks-storage.bucket-store.bucket-index.enabled - [deprecated] 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. (default true) -blocks-storage.bucket-store.bucket-index.idle-timeout duration How long a unused bucket index should be cached. Once this timeout expires, the unused bucket index is removed from the in-memory cache. This option is used only by querier. (default 1h0m0s) -blocks-storage.bucket-store.bucket-index.max-stale-period duration diff --git a/docs/sources/mimir/references/configuration-parameters/index.md b/docs/sources/mimir/references/configuration-parameters/index.md index 0b16b836f03..4a8c5c4fc75 100644 --- a/docs/sources/mimir/references/configuration-parameters/index.md +++ b/docs/sources/mimir/references/configuration-parameters/index.md @@ -3538,12 +3538,6 @@ bucket_store: [ignore_deletion_mark_delay: | default = 1h] bucket_index: - # (deprecated) 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. - # CLI flag: -blocks-storage.bucket-store.bucket-index.enabled - [enabled: | default = true] - # (advanced) How frequently a bucket index, which previously failed to load, # should be tried to load again. This option is used only by querier. # CLI flag: -blocks-storage.bucket-store.bucket-index.update-on-error-interval diff --git a/integration/backward_compatibility_test.go b/integration/backward_compatibility_test.go index 4d3c827da18..1e5550057ec 100644 --- a/integration/backward_compatibility_test.go +++ b/integration/backward_compatibility_test.go @@ -132,6 +132,10 @@ func runBackwardCompatibilityTest(t *testing.T, previousImage string, oldFlagsMa // The distributor should have 512 tokens for the ingester ring and 1 for the distributor ring require.NoError(t, distributor.WaitSumMetrics(e2e.Equals(512+1), "cortex_ring_tokens_total")) + // Start the compactor to have the bucket index created before querying. + compactor := e2emimir.NewCompactor("compactor", consul.NetworkHTTPEndpoint(), flags) + require.NoError(t, s.StartAndWaitReady(compactor)) + checkQueries(t, consul, previousImage, flags, oldFlagsMapper, s, 1, instantQueryTest{ expr: "series_1", time: series1Timestamp, diff --git a/integration/configs.go b/integration/configs.go index a362927685e..0f7f0417697 100644 --- a/integration/configs.go +++ b/integration/configs.go @@ -142,7 +142,6 @@ var ( BlocksStorageFlags = func() map[string]string { return map[string]string{ "-blocks-storage.tsdb.block-ranges-period": "1m", - "-blocks-storage.bucket-store.bucket-index.enabled": "false", "-blocks-storage.bucket-store.ignore-blocks-within": "0", "-blocks-storage.bucket-store.sync-interval": "5s", "-blocks-storage.tsdb.retention-period": "5m", @@ -192,8 +191,6 @@ blocks_storage: bucket_store: sync_interval: 5s - bucket_index: - enabled: false s3: bucket_name: mimir-blocks diff --git a/integration/getting_started_with_gossiped_ring_test.go b/integration/getting_started_with_gossiped_ring_test.go index 184b1582f5f..1325a95ffd0 100644 --- a/integration/getting_started_with_gossiped_ring_test.go +++ b/integration/getting_started_with_gossiped_ring_test.go @@ -38,7 +38,6 @@ func TestGettingStartedWithGossipedRing(t *testing.T) { flags := map[string]string{ // decrease timeouts to make test faster. should still be fine with two instances only "-ingester.ring.observe-period": "5s", // to avoid conflicts in tokens - "-blocks-storage.bucket-store.bucket-index.enabled": "false", "-blocks-storage.bucket-store.sync-interval": "1s", // sync continuously "-blocks-storage.bucket-store.ignore-blocks-within": "0", "-blocks-storage.backend": "s3", diff --git a/integration/ingester_limits_test.go b/integration/ingester_limits_test.go index 93d22e2fe9e..42e4e7c17be 100644 --- a/integration/ingester_limits_test.go +++ b/integration/ingester_limits_test.go @@ -187,14 +187,13 @@ overrides: require.NoError(t, copyFileToSharedDir(s, "docs/configurations/single-process-config-blocks.yaml", mimirConfigFile)) flags := map[string]string{ - "-runtime-config.reload-period": "100ms", - "-blocks-storage.backend": "filesystem", - "-blocks-storage.filesystem.dir": "/tmp", - "-blocks-storage.storage-prefix": "blocks", - "-blocks-storage.bucket-store.bucket-index.enabled": "false", - "-ruler-storage.backend": "filesystem", - "-ruler-storage.local.directory": "/tmp", // Avoid warning "unable to list rules". - "-runtime-config.file": filepath.Join(e2e.ContainerSharedDir, overridesFile), + "-runtime-config.reload-period": "100ms", + "-blocks-storage.backend": "filesystem", + "-blocks-storage.filesystem.dir": "/tmp", + "-blocks-storage.storage-prefix": "blocks", + "-ruler-storage.backend": "filesystem", + "-ruler-storage.local.directory": "/tmp", // Avoid warning "unable to list rules". + "-runtime-config.file": filepath.Join(e2e.ContainerSharedDir, overridesFile), } cortex1 := e2emimir.NewSingleBinary("cortex-1", flags, e2emimir.WithConfigFile(mimirConfigFile), e2emimir.WithPorts(9009, 9095)) require.NoError(t, s.StartAndWaitReady(cortex1)) diff --git a/integration/querier_test.go b/integration/querier_test.go index 463e4071836..c90bdffab1b 100644 --- a/integration/querier_test.go +++ b/integration/querier_test.go @@ -47,7 +47,6 @@ func testQuerierWithBlocksStorageRunningInMicroservicesMode(t *testing.T, stream tests := map[string]struct { tenantShardSize int indexCacheBackend string - bucketIndexEnabled bool queryShardingEnabled bool }{ "shard size 0, inmemory index cache": { @@ -62,19 +61,9 @@ func testQuerierWithBlocksStorageRunningInMicroservicesMode(t *testing.T, stream tenantShardSize: 1, indexCacheBackend: tsdb.IndexCacheBackendMemcached, }, - "shard size 0, inmemory index cache, bucket index enabled": { - indexCacheBackend: tsdb.IndexCacheBackendInMemory, - bucketIndexEnabled: true, - }, - "shard size 1, memcached index cache, bucket index enabled": { - tenantShardSize: 1, - indexCacheBackend: tsdb.IndexCacheBackendMemcached, - bucketIndexEnabled: true, - }, - "shard size 1, ingester gRPC streaming enabled, memcached index cache, bucket index enabled, query sharding enabled": { + "shard size 1, ingester gRPC streaming enabled, memcached index cache, query sharding enabled": { tenantShardSize: 1, indexCacheBackend: tsdb.IndexCacheBackendMemcached, - bucketIndexEnabled: true, queryShardingEnabled: true, }, } @@ -166,7 +155,6 @@ func testQuerierWithBlocksStorageRunningInMicroservicesMode(t *testing.T, stream "-blocks-storage.bucket-store.index-cache.memcached.addresses": "dns+" + memcached.NetworkEndpoint(e2ecache.MemcachedPort), "-blocks-storage.bucket-store.sync-interval": "1s", "-blocks-storage.bucket-store.index-cache.backend": testCfg.indexCacheBackend, - "-blocks-storage.bucket-store.bucket-index.enabled": strconv.FormatBool(testCfg.bucketIndexEnabled), "-store-gateway.tenant-shard-size": fmt.Sprintf("%d", testCfg.tenantShardSize), "-query-frontend.query-stats-enabled": "true", "-query-frontend.parallelize-shardable-queries": strconv.FormatBool(testCfg.queryShardingEnabled), @@ -199,10 +187,8 @@ func testQuerierWithBlocksStorageRunningInMicroservicesMode(t *testing.T, stream // the store-gateway ring if blocks sharding is enabled. require.NoError(t, querier.WaitSumMetrics(e2e.Equals(float64(512+(512*storeGateways.NumInstances()))), "cortex_ring_tokens_total")) - if !testCfg.bucketIndexEnabled { - // Wait until the querier has discovered the uploaded blocks. - require.NoError(t, querier.WaitSumMetrics(e2e.Equals(2), "cortex_blocks_meta_synced")) - } + // Wait until the querier has discovered the uploaded blocks. + require.NoError(t, querier.WaitSumMetrics(e2e.Equals(2), "cortex_blocks_meta_synced")) // Wait until the store-gateway has synched the new uploaded blocks. When sharding is enabled // we don't known which store-gateway instance will synch the blocks, so we need to wait on @@ -335,7 +321,6 @@ func testQuerierWithBlocksStorageRunningInMicroservicesMode(t *testing.T, stream func TestQuerierWithBlocksStorageRunningInSingleBinaryMode(t *testing.T) { tests := map[string]struct { indexCacheBackend string - bucketIndexEnabled bool queryShardingEnabled bool }{ "inmemory index cache": { @@ -344,10 +329,6 @@ func TestQuerierWithBlocksStorageRunningInSingleBinaryMode(t *testing.T) { "memcached index cache": { indexCacheBackend: tsdb.IndexCacheBackendMemcached, }, - "memcached index cache, bucket index enabled": { - indexCacheBackend: tsdb.IndexCacheBackendMemcached, - bucketIndexEnabled: true, - }, "inmemory index cache, query sharding enabled": { indexCacheBackend: tsdb.IndexCacheBackendInMemory, queryShardingEnabled: true, @@ -382,7 +363,6 @@ func TestQuerierWithBlocksStorageRunningInSingleBinaryMode(t *testing.T) { "-blocks-storage.tsdb.retention-period": ((blockRangePeriod * 2) - 1).String(), "-blocks-storage.bucket-store.index-cache.backend": testCfg.indexCacheBackend, "-blocks-storage.bucket-store.index-cache.memcached.addresses": "dns+" + memcached.NetworkEndpoint(e2ecache.MemcachedPort), - "-blocks-storage.bucket-store.bucket-index.enabled": strconv.FormatBool(testCfg.bucketIndexEnabled), // Ingester. "-ingester.ring.store": "consul", @@ -459,11 +439,9 @@ func TestQuerierWithBlocksStorageRunningInSingleBinaryMode(t *testing.T) { require.NoError(t, cluster.WaitSumMetrics(e2e.Equals(float64(3*cluster.NumInstances())), "cortex_ingester_memory_series_created_total")) require.NoError(t, cluster.WaitSumMetrics(e2e.Equals(float64(2*cluster.NumInstances())), "cortex_ingester_memory_series_removed_total")) - if !testCfg.bucketIndexEnabled { - // 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")))) - } + // 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")))) // Wait until the store-gateway has synched the new uploaded blocks. The number of blocks loaded // may be greater than expected if the compactor is running (there may have been compacted). diff --git a/integration/ruler_test.go b/integration/ruler_test.go index 935ffe96593..7ce1b50711e 100644 --- a/integration/ruler_test.go +++ b/integration/ruler_test.go @@ -364,8 +364,6 @@ func TestRulerSharding(t *testing.T) { BlocksStorageFlags(), RulerShardingFlags(consul.NetworkHTTPEndpoint()), map[string]string{ - // Enable the bucket index so we can skip the initial bucket scan. - "-blocks-storage.bucket-store.bucket-index.enabled": "true", // Disable rule group limit "-ruler.max-rule-groups-per-tenant": "0", }, @@ -556,8 +554,6 @@ func TestRulerMetricsForInvalidQueriesAndNoFetchedSeries(t *testing.T) { RulerFlags(), BlocksStorageFlags(), map[string]string{ - // Enable the bucket index so we can skip the initial bucket scan. - "-blocks-storage.bucket-store.bucket-index.enabled": "true", // Evaluate rules often, so that we don't need to wait for metrics to show up. "-ruler.evaluation-interval": "2s", "-ruler.poll-interval": "2s", diff --git a/pkg/querier/blocks_store_queryable.go b/pkg/querier/blocks_store_queryable.go index 2406bea1dec..8eed5dc1d71 100644 --- a/pkg/querier/blocks_store_queryable.go +++ b/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{ - 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) - } + 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) storesRingCfg := gatewayCfg.ShardingRing.ToRingConfig() storesRingBackend, err := kv.NewClient( diff --git a/pkg/storage/tsdb/config.go b/pkg/storage/tsdb/config.go index e16fce7ad83..e077252375a 100644 --- a/pkg/storage/tsdb/config.go +++ b/pkg/storage/tsdb/config.go @@ -477,23 +477,18 @@ func (cfg *BucketStoreConfig) Validate(logger log.Logger) error { } type BucketIndexConfig struct { - DeprecatedEnabled bool `yaml:"enabled" category:"deprecated"` // Deprecated. TODO: Remove in Mimir 2.11. UpdateOnErrorInterval time.Duration `yaml:"update_on_error_interval" category:"advanced"` IdleTimeout time.Duration `yaml:"idle_timeout" category:"advanced"` MaxStalePeriod time.Duration `yaml:"max_stale_period" category:"advanced"` } func (cfg *BucketIndexConfig) RegisterFlagsWithPrefix(f *flag.FlagSet, prefix string) { - f.BoolVar(&cfg.DeprecatedEnabled, 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.") f.DurationVar(&cfg.UpdateOnErrorInterval, prefix+"update-on-error-interval", time.Minute, "How frequently a bucket index, which previously failed to load, should be tried to load again. This option is used only by querier.") f.DurationVar(&cfg.IdleTimeout, prefix+"idle-timeout", time.Hour, "How long a unused bucket index should be cached. Once this timeout expires, the unused bucket index is removed from the in-memory cache. This option is used only by querier.") f.DurationVar(&cfg.MaxStalePeriod, prefix+"max-stale-period", time.Hour, "The maximum allowed age of a bucket index (last updated) before queries start failing because the bucket index is too old. The bucket index is periodically updated by the compactor, and this check is enforced in the querier (at query time).") } // Validate the config. -func (cfg *BucketIndexConfig) Validate(logger log.Logger) error { - if !cfg.DeprecatedEnabled { - util.WarnDeprecatedConfig(bucketIndexFlagPrefix+"enabled", logger) - } +func (cfg *BucketIndexConfig) Validate(_ log.Logger) error { return nil } diff --git a/pkg/storegateway/bucket_stores.go b/pkg/storegateway/bucket_stores.go index c1290bbd5b2..05da739c931 100644 --- a/pkg/storegateway/bucket_stores.go +++ b/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( - 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 - } + var ( + fetcher block.MetadataFetcher + 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 } bucketStoreOpts := []BucketStoreOption{ @@ -480,7 +471,7 @@ func (u *BucketStores) getOrCreateStore(userID string) (*BucketStore, error) { WithLazyLoadingGate(u.lazyLoadingGate), } - bs, err := NewBucketStore( + bs, err = NewBucketStore( userID, userBkt, fetcher, diff --git a/pkg/storegateway/bucket_stores_test.go b/pkg/storegateway/bucket_stores_test.go index 9f574a25834..17895a78d9c 100644 --- a/pkg/storegateway/bucket_stores_test.go +++ b/pkg/storegateway/bucket_stores_test.go @@ -573,7 +573,6 @@ func prepareStorageConfig(t *testing.T) mimir_tsdb.BlocksStorageConfig { cfg := mimir_tsdb.BlocksStorageConfig{} flagext.DefaultValues(&cfg) - cfg.BucketStore.BucketIndex.DeprecatedEnabled = false cfg.BucketStore.SyncDir = tmpDir return cfg diff --git a/pkg/storegateway/gateway_test.go b/pkg/storegateway/gateway_test.go index c90e50173d2..23f71017593 100644 --- a/pkg/storegateway/gateway_test.go +++ b/pkg/storegateway/gateway_test.go @@ -288,82 +288,78 @@ func TestStoreGateway_InitialSyncWithWaitRingTokensStability(t *testing.T) { } for testName, testData := range tests { - for _, bucketIndexEnabled := range []bool{true, false} { - // capture running variables, otherwise reused due to t.Parallel leading to corrupt test - testName := testName - testData := testData - bucketIndexEnabled := bucketIndexEnabled - t.Run(fmt.Sprintf("%s (bucket index enabled = %v)", testName, bucketIndexEnabled), func(t *testing.T) { - t.Parallel() - ctx := context.Background() - ringStore, closer := consul.NewInMemoryClientWithConfig(ring.GetCodec(), consul.Config{ - MaxCasRetries: 20, - CasRetryDelay: 500 * time.Millisecond, - }, log.NewNopLogger(), nil) - t.Cleanup(func() { assert.NoError(t, closer.Close()) }) - - // Create the configured number of gateways. - var gateways []*StoreGateway - registries := dskit_metrics.NewTenantRegistries(log.NewNopLogger()) - - for i := 1; i <= testData.numGateways; i++ { - instanceID := fmt.Sprintf("gateway-%d", i) + // capture running variables, otherwise reused due to t.Parallel leading to corrupt test + testName := testName + testData := testData + t.Run(testName, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + ringStore, closer := consul.NewInMemoryClientWithConfig(ring.GetCodec(), consul.Config{ + MaxCasRetries: 20, + CasRetryDelay: 500 * time.Millisecond, + }, log.NewNopLogger(), nil) + t.Cleanup(func() { assert.NoError(t, closer.Close()) }) - storageCfg := mockStorageConfig(t) - storageCfg.BucketStore.SyncInterval = time.Hour // Do not trigger the periodic sync in this test. We want the initial sync only. - storageCfg.BucketStore.BucketIndex.DeprecatedEnabled = bucketIndexEnabled + // Create the configured number of gateways. + var gateways []*StoreGateway + registries := dskit_metrics.NewTenantRegistries(log.NewNopLogger()) - limits := defaultLimitsConfig() - gatewayCfg := mockGatewayConfig() - gatewayCfg.ShardingRing.ReplicationFactor = testData.replicationFactor - gatewayCfg.ShardingRing.InstanceID = instanceID - gatewayCfg.ShardingRing.InstanceAddr = fmt.Sprintf("127.0.0.%d", i) - gatewayCfg.ShardingRing.RingCheckPeriod = time.Hour // Do not check the ring topology changes in this test. We want the initial sync only. - gatewayCfg.ShardingRing.WaitStabilityMinDuration = 4 * time.Second - gatewayCfg.ShardingRing.WaitStabilityMaxDuration = 30 * time.Second - limits.StoreGatewayTenantShardSize = testData.tenantShardSize + for i := 1; i <= testData.numGateways; i++ { + instanceID := fmt.Sprintf("gateway-%d", i) - overrides, err := validation.NewOverrides(limits, nil) - require.NoError(t, err) + storageCfg := mockStorageConfig(t) + storageCfg.BucketStore.SyncInterval = time.Hour // Do not trigger the periodic sync in this test. We want the initial sync only. - reg := prometheus.NewPedanticRegistry() - g, err := newStoreGateway(gatewayCfg, storageCfg, bucketClient, ringStore, overrides, log.NewNopLogger(), reg, nil) - require.NoError(t, err) - t.Cleanup(func() { assert.NoError(t, services.StopAndAwaitTerminated(ctx, g)) }) + limits := defaultLimitsConfig() + gatewayCfg := mockGatewayConfig() + gatewayCfg.ShardingRing.ReplicationFactor = testData.replicationFactor + gatewayCfg.ShardingRing.InstanceID = instanceID + gatewayCfg.ShardingRing.InstanceAddr = fmt.Sprintf("127.0.0.%d", i) + gatewayCfg.ShardingRing.RingCheckPeriod = time.Hour // Do not check the ring topology changes in this test. We want the initial sync only. + gatewayCfg.ShardingRing.WaitStabilityMinDuration = 4 * time.Second + gatewayCfg.ShardingRing.WaitStabilityMaxDuration = 30 * time.Second + limits.StoreGatewayTenantShardSize = testData.tenantShardSize - gateways = append(gateways, g) - registries.AddTenantRegistry(instanceID, reg) - } + overrides, err := validation.NewOverrides(limits, nil) + require.NoError(t, err) - // Start all gateways concurrently. - for _, g := range gateways { - require.NoError(t, g.StartAsync(ctx)) - } + reg := prometheus.NewPedanticRegistry() + g, err := newStoreGateway(gatewayCfg, storageCfg, bucketClient, ringStore, overrides, log.NewNopLogger(), reg, nil) + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, services.StopAndAwaitTerminated(ctx, g)) }) - // Wait until all gateways are running. - for _, g := range gateways { - require.NoError(t, g.AwaitRunning(ctx)) - } + gateways = append(gateways, g) + registries.AddTenantRegistry(instanceID, reg) + } - // At this point we expect that all gateways have done the initial sync and - // they have synched only their own blocks, because they waited for a stable - // ring before starting the initial sync. - metrics := registries.BuildMetricFamiliesPerTenant() - assert.Equal(t, float64(testData.expectedBlocksLoaded), metrics.GetSumOfGauges("cortex_bucket_store_blocks_loaded")) - assert.Equal(t, float64(2*testData.numGateways), metrics.GetSumOfGauges("cortex_bucket_stores_tenants_discovered")) - - if testData.tenantShardSize > 0 { - assert.Equal(t, float64(testData.tenantShardSize*numBlocks), metrics.GetSumOfGauges("cortex_blocks_meta_synced")) - assert.Equal(t, float64(testData.tenantShardSize*numUsers), metrics.GetSumOfGauges("cortex_bucket_stores_tenants_synced")) - } else { - assert.Equal(t, float64(testData.numGateways*numBlocks), metrics.GetSumOfGauges("cortex_blocks_meta_synced")) - assert.Equal(t, float64(testData.numGateways*numUsers), metrics.GetSumOfGauges("cortex_bucket_stores_tenants_synced")) - } + // Start all gateways concurrently. + for _, g := range gateways { + require.NoError(t, g.StartAsync(ctx)) + } - // We expect that all gateways have only run the initial sync and not the periodic one. - assert.Equal(t, float64(testData.numGateways), metrics.GetSumOfCounters("cortex_storegateway_bucket_sync_total")) - }) - } + // Wait until all gateways are running. + for _, g := range gateways { + require.NoError(t, g.AwaitRunning(ctx)) + } + + // At this point we expect that all gateways have done the initial sync and + // they have synched only their own blocks, because they waited for a stable + // ring before starting the initial sync. + metrics := registries.BuildMetricFamiliesPerTenant() + assert.Equal(t, float64(testData.expectedBlocksLoaded), metrics.GetSumOfGauges("cortex_bucket_store_blocks_loaded")) + assert.Equal(t, float64(2*testData.numGateways), metrics.GetSumOfGauges("cortex_bucket_stores_tenants_discovered")) + + if testData.tenantShardSize > 0 { + assert.Equal(t, float64(testData.tenantShardSize*numBlocks), metrics.GetSumOfGauges("cortex_blocks_meta_synced")) + assert.Equal(t, float64(testData.tenantShardSize*numUsers), metrics.GetSumOfGauges("cortex_bucket_stores_tenants_synced")) + } else { + assert.Equal(t, float64(testData.numGateways*numBlocks), metrics.GetSumOfGauges("cortex_blocks_meta_synced")) + assert.Equal(t, float64(testData.numGateways*numUsers), metrics.GetSumOfGauges("cortex_bucket_stores_tenants_synced")) + } + + // We expect that all gateways have only run the initial sync and not the periodic one. + assert.Equal(t, float64(testData.numGateways), metrics.GetSumOfCounters("cortex_storegateway_bucket_sync_total")) + }) } } @@ -406,7 +402,6 @@ func TestStoreGateway_BlocksSyncWithDefaultSharding_RingTopologyChangedAfterScal storageCfg := mockStorageConfig(t) storageCfg.BucketStore.SyncInterval = time.Hour // Do not trigger the periodic sync in this test. We want it to be triggered by ring topology changed. - storageCfg.BucketStore.BucketIndex.DeprecatedEnabled = true limits := defaultLimitsConfig() gatewayCfg := mockGatewayConfig() @@ -1048,55 +1043,50 @@ func TestStoreGateway_SeriesQueryingShouldRemoveExternalLabels(t *testing.T) { require.NoError(t, err) } - for _, bucketIndexEnabled := range []bool{true, false} { - t.Run(fmt.Sprintf("bucket index enabled = %v", bucketIndexEnabled), func(t *testing.T) { - // Create a store-gateway used to query back the series from the blocks. - gatewayCfg := mockGatewayConfig() - storageCfg := mockStorageConfig(t) - storageCfg.BucketStore.BucketIndex.DeprecatedEnabled = bucketIndexEnabled + // Create a store-gateway used to query back the series from the blocks. + gatewayCfg := mockGatewayConfig() + storageCfg := mockStorageConfig(t) - ringStore, closer := consul.NewInMemoryClient(ring.GetCodec(), log.NewNopLogger(), nil) - t.Cleanup(func() { assert.NoError(t, closer.Close()) }) + ringStore, closer := consul.NewInMemoryClient(ring.GetCodec(), log.NewNopLogger(), nil) + t.Cleanup(func() { assert.NoError(t, closer.Close()) }) - g, err := newStoreGateway(gatewayCfg, storageCfg, bucketClient, ringStore, defaultLimitsOverrides(t), logger, nil, nil) - require.NoError(t, err) - require.NoError(t, services.StartAndAwaitRunning(ctx, g)) - t.Cleanup(func() { assert.NoError(t, services.StopAndAwaitTerminated(ctx, g)) }) + g, err := newStoreGateway(gatewayCfg, storageCfg, bucketClient, ringStore, defaultLimitsOverrides(t), logger, nil, nil) + require.NoError(t, err) + require.NoError(t, services.StartAndAwaitRunning(ctx, g)) + t.Cleanup(func() { assert.NoError(t, services.StopAndAwaitTerminated(ctx, g)) }) - srv := newStoreGatewayTestServer(t, g) + srv := newStoreGatewayTestServer(t, g) - for _, streamingBatchSize := range []int{0, 1, 5} { - t.Run(fmt.Sprintf("streamingBatchSize=%d", streamingBatchSize), func(t *testing.T) { - // Query back all series. - req := &storepb.SeriesRequest{ - MinTime: minT, - MaxTime: maxT, - Matchers: []storepb.LabelMatcher{ - {Type: storepb.LabelMatcher_RE, Name: "__name__", Value: ".*"}, - }, - StreamingChunksBatchSize: uint64(streamingBatchSize), - } - seriesSet, warnings, _, _, err := srv.Series(setUserIDToGRPCContext(ctx, userID), req) - require.NoError(t, err) - assert.Empty(t, warnings) - assert.Len(t, seriesSet, numSeries) + for _, streamingBatchSize := range []int{0, 1, 5} { + t.Run(fmt.Sprintf("streamingBatchSize=%d", streamingBatchSize), func(t *testing.T) { + // Query back all series. + req := &storepb.SeriesRequest{ + MinTime: minT, + MaxTime: maxT, + Matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_RE, Name: "__name__", Value: ".*"}, + }, + StreamingChunksBatchSize: uint64(streamingBatchSize), + } + seriesSet, warnings, _, _, err := srv.Series(setUserIDToGRPCContext(ctx, userID), req) + require.NoError(t, err) + assert.Empty(t, warnings) + assert.Len(t, seriesSet, numSeries) - for seriesID := 0; seriesID < numSeries; seriesID++ { - actual := seriesSet[seriesID] + for seriesID := 0; seriesID < numSeries; seriesID++ { + actual := seriesSet[seriesID] - // Ensure Mimir external labels have been removed. - assert.Equal(t, []mimirpb.LabelAdapter{{Name: "series_id", Value: strconv.Itoa(seriesID)}}, actual.Labels) + // Ensure Mimir external labels have been removed. + assert.Equal(t, []mimirpb.LabelAdapter{{Name: "series_id", Value: strconv.Itoa(seriesID)}}, actual.Labels) - // Ensure samples have been correctly queried. The store-gateway doesn't deduplicate chunks, - // so the same sample is returned twice because in this test we query two identical blocks. - samples, err := readSamplesFromChunks(actual.Chunks) - require.NoError(t, err) - assert.Equal(t, []sample{ - {t: minT + (step * int64(seriesID)), v: float64(seriesID)}, - {t: minT + (step * int64(seriesID)), v: float64(seriesID)}, - }, samples) - } - }) + // Ensure samples have been correctly queried. The store-gateway doesn't deduplicate chunks, + // so the same sample is returned twice because in this test we query two identical blocks. + samples, err := readSamplesFromChunks(actual.Chunks) + require.NoError(t, err) + assert.Equal(t, []sample{ + {t: minT + (step * int64(seriesID)), v: float64(seriesID)}, + {t: minT + (step * int64(seriesID)), v: float64(seriesID)}, + }, samples) } }) } @@ -1168,7 +1158,6 @@ func TestStoreGateway_Series_QuerySharding(t *testing.T) { // Create a store-gateway. gatewayCfg := mockGatewayConfig() storageCfg := mockStorageConfig(t) - storageCfg.BucketStore.BucketIndex.DeprecatedEnabled = true ringStore, closer := consul.NewInMemoryClient(ring.GetCodec(), log.NewNopLogger(), nil) t.Cleanup(func() { assert.NoError(t, closer.Close()) }) @@ -1247,7 +1236,6 @@ func TestStoreGateway_Series_QueryShardingShouldGuaranteeSeriesShardingConsisten // Create a store-gateway. gatewayCfg := mockGatewayConfig() storageCfg := mockStorageConfig(t) - storageCfg.BucketStore.BucketIndex.DeprecatedEnabled = true ringStore, closer := consul.NewInMemoryClient(ring.GetCodec(), log.NewNopLogger(), nil) t.Cleanup(func() { assert.NoError(t, closer.Close()) }) @@ -1329,7 +1317,6 @@ func TestStoreGateway_Series_QueryShardingConcurrency(t *testing.T) { // Create a store-gateway. gatewayCfg := mockGatewayConfig() storageCfg := mockStorageConfig(t) - storageCfg.BucketStore.BucketIndex.DeprecatedEnabled = true ringStore, closer := consul.NewInMemoryClient(ring.GetCodec(), log.NewNopLogger(), nil) t.Cleanup(func() { assert.NoError(t, closer.Close()) }) @@ -1520,7 +1507,6 @@ func mockStorageConfig(t *testing.T) mimir_tsdb.BlocksStorageConfig { cfg := mimir_tsdb.BlocksStorageConfig{} flagext.DefaultValues(&cfg) - cfg.BucketStore.BucketIndex.DeprecatedEnabled = false // mocks used in tests don't expect index reads cfg.BucketStore.IgnoreBlocksWithin = 0 cfg.BucketStore.SyncDir = tmpDir From 94fc1cb26ceb4c97b2d7f40fed07e4b4985df7d8 Mon Sep 17 00:00:00 2001 From: Justin Lei Date: Wed, 15 Nov 2023 14:11:38 -0800 Subject: [PATCH 5/5] Update CHANGELOG.md --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index adc58f4296d..b3cdaecf188 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ ### Grafana Mimir +* [CHANGE] The following deprecated configurations have been removed: #6673 + * `-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` * [CHANGE] Querier: Split worker GRPC config into separate client configs for the frontend and scheduler to allow TLS to be configured correctly when specifying the `tls_server_name`. The GRPC config specified under `-querier.frontend-client.*` will no longer apply to the scheduler client, and will need to be set explicitly under `-querier.scheduler-client.*`. #6445 #6573 * [CHANGE] Store-gateway: enable sparse index headers by default. Sparse index headers reduce the time to load an index header up to 90%. #6005 * [CHANGE] Store-gateway: lazy-loading concurrency limit default value is now 4. #6004