Refactor Index Header Configs #5726

lamida · 2023-08-14T08:55:54Z

What this PR does

Refactor Index Header configurations. Non-experimental configs are marked as deprecated and new configurations are created in indexheader.Config. Experimental configs are immediately moved to indexheader.Config without deprecation.

Which issue(s) this PR fixes or relates to

Fixes #5727

Checklist

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

pkg/storegateway/indexheader/header.go

colega · 2023-08-16T11:32:43Z

pkg/storage/tsdb/config.go

+	// Take value from Indexheader DeprecatedConfig if it is set
+	if cfg.DeprecatedIndexHeaderLazyLoadingEnabled {
+		cfg.IndexHeader.LazyLoadingEnabled = cfg.DeprecatedIndexHeaderLazyLoadingEnabled
+	}


We're registring flags here, not parsing them, so no flags were set yet.

Missed that fact. I moved to Mimir.initStoreGateway. Please advise if it is the good place for setting this or whether do we need to set this deprecated flag override in the first place.

…aded block in store gateway (#5095) * Add lazy loaded trackers of store gateway index headers Signed-off-by: Jon Kartago Lamida <[email protected]> * Add period to persist list of lazy loaded blocks Signed-off-by: Jon Kartago Lamida <[email protected]> * Track only lazy loaded block during persist time Signed-off-by: Jon Kartago Lamida <[email protected]> * Use protobuf to persist the tracker state of lazy loaded block Signed-off-by: Jon Kartago Lamida <[email protected]> * Add proto suffix for the binary file Signed-off-by: Jon Kartago Lamida <[email protected]> * Rename protobuf name Signed-off-by: Jon Kartago Lamida <[email protected]> * Initiate HeaderLazyLoadedTracker in bucket per tenant Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix outdated constructor in test Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove unused LazyLoaderTracker in binary_reader Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove unnecessary newline Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix HeadersLazyLoadedTracker receiver must be pointer Signed-off-by: Jon Kartago Lamida <[email protected]> * Small grammar correction in comment Signed-off-by: Jon Kartago Lamida <[email protected]> * Rename constant to lazyLoadedFile Signed-off-by: Jon Kartago Lamida <[email protected]> * Use hardcoded 1 minute to persist list of lazyLoadedBlock Signed-off-by: Jon Kartago Lamida <[email protected]> * Add layzLoaded tracker test Signed-off-by: Jon Kartago Lamida <[email protected]> * Add logger when lazyLoadedTracker file to persist Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix linter Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Dimitar Dimitrov <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Dimitar Dimitrov <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Dimitar Dimitrov <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Dimitar Dimitrov <[email protected]> * Unexport lazyLoadedTracker method Signed-off-by: Jon Kartago Lamida <[email protected]> * Try to set checksum Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Oleg Zaytsev <[email protected]> * Use one goroutine for closeIdleReader and lazyLoad persist Signed-off-by: Jon Kartago Lamida <[email protected]> * Use pb suffix in test Signed-off-by: Jon Kartago Lamida <[email protected]> * Refactor the structure of tracker Signed-off-by: Jon Kartago Lamida <[email protected]> * Clean lint Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix test Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix test Signed-off-by: Jon Kartago Lamida <[email protected]> * Use fsync Signed-off-by: Jon Kartago Lamida <[email protected]> * Return err Signed-off-by: Jon Kartago Lamida <[email protected]> * Use json Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Dimitar Dimitrov <[email protected]> * Remove defer and use t.CleanUp Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix linter Signed-off-by: Jon Kartago Lamida <[email protected]> * Still fixing linter Signed-off-by: Jon Kartago Lamida <[email protected]> * Revert idle timeout in test Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Dimitar Dimitrov <[email protected]> * Revert newline Signed-off-by: Jon Kartago Lamida <[email protected]> * Revert newline Signed-off-by: Jon Kartago Lamida <[email protected]> * Revert newline Signed-off-by: Jon Kartago Lamida <[email protected]> * Apply PR feedbacks Signed-off-by: Jon Kartago Lamida <[email protected]> * Read the file to assert the lazyloaded heaeder Signed-off-by: Jon Kartago Lamida <[email protected]> * Don't use /tmp directory for lazyloaded header Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix test Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Dimitar Dimitrov <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Dimitar Dimitrov <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Charles Korn <[email protected]> * Apply PR suggestion Signed-off-by: Jon Kartago Lamida <[email protected]> * Extract fsync operation Signed-off-by: Jon Kartago Lamida <[email protected]> * Add assertion on LoadedBlocks Signed-off-by: Jon Kartago Lamida <[email protected]> * Update comment Signed-off-by: Jon Kartago Lamida <[email protected]> * Add more proper LoadedBlock test Signed-off-by: Jon Kartago Lamida <[email protected]> * Add license header Signed-off-by: Jon Kartago Lamida <[email protected]> * Update fsync method name Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Charles Korn <[email protected]> * Address PR feedbacks Signed-off-by: Jon Kartago Lamida <[email protected]> * Update method comment Signed-off-by: Jon Kartago Lamida <[email protected]> * Update method comment Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Charles Korn <[email protected]> * Add comments to explain why we use os.Rename Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove unrelevant test Signed-off-by: Jon Kartago Lamida <[email protected]> * Add assertion to make sure there is only one lazyReader Signed-off-by: Jon Kartago Lamida <[email protected]> * Use io.Reader for fsync data Signed-off-by: Jon Kartago Lamida <[email protected]> * Ramove table test for a single test case only Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/util/atomicfs/fsync_test.go Co-authored-by: Marco Pracucci <[email protected]> * Apply PR suggestions Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix test Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/util/atomicfs/fsync_test.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Charles Korn <[email protected]> * Clean import Signed-off-by: Jon Kartago Lamida <[email protected]> --------- Signed-off-by: Jon Kartago Lamida <[email protected]> Co-authored-by: Dimitar Dimitrov <[email protected]> Co-authored-by: Oleg Zaytsev <[email protected]> Co-authored-by: Charles Korn <[email protected]> Co-authored-by: Marco Pracucci <[email protected]>

* Start to do eager load of index header during startup Signed-off-by: Jon Kartago Lamida <[email protected]> * Make EagerLoading as public method Signed-off-by: Jon Kartago Lamida <[email protected]> * Add eager loading test Signed-off-by: Jon Kartago Lamida <[email protected]> * Small refactoring of test Signed-off-by: Jon Kartago Lamida <[email protected]> * Add godoc Signed-off-by: Jon Kartago Lamida <[email protected]> * Update reader_pool_test with eager loading logic Signed-off-by: Jon Kartago Lamida <[email protected]> * Small refactoring to make test clearer Signed-off-by: Jon Kartago Lamida <[email protected]> * Appease the linter Signed-off-by: Jon Kartago Lamida <[email protected]> * Rename private method for test Signed-off-by: Jon Kartago Lamida <[email protected]> * Add config to enable or disable index headers eager loading Signed-off-by: Jon Kartago Lamida <[email protected]> * Changelog Signed-off-by: Jon Kartago Lamida <[email protected]> * Changelog Signed-off-by: Jon Kartago Lamida <[email protected]> * Go select can't be inside method Signed-off-by: Jon Kartago Lamida <[email protected]> * Run make doc Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix test Signed-off-by: Jon Kartago Lamida <[email protected]> * Update CHANGELOG.md Co-authored-by: Charles Korn <[email protected]> * Update pkg/storegateway/indexheader/lazy_binary_reader.go Co-authored-by: Charles Korn <[email protected]> * Apply configs PR feedbacks Signed-off-by: Jon Kartago Lamida <[email protected]> * Pass BucketStoreConfig as argument Signed-off-by: Jon Kartago Lamida <[email protected]> * Pass eagerLoadIndexReaderEnabled into snapshotConfig Signed-off-by: Jon Kartago Lamida <[email protected]> * Don't pass lazyLoadedSnapshot into LazyBinaryReader Signed-off-by: Jon Kartago Lamida <[email protected]> * Only initialize tickerLazyLoad when flag is enabled Signed-off-by: Jon Kartago Lamida <[email protected]> * Apply PR feedbacks Signed-off-by: Jon Kartago Lamida <[email protected]> * Update config description Signed-off-by: Jon Kartago Lamida <[email protected]> * Update usedAt to current time in eager loading Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Dimitar Dimitrov <[email protected]> * Fix snapshotByte name Signed-off-by: Jon Kartago Lamida <[email protected]> * Address PR feedbacks Signed-off-by: Jon Kartago Lamida <[email protected]> * Rename initLazyBinaryForTest Signed-off-by: Jon Kartago Lamida <[email protected]> * Apply PR suggestion Signed-off-by: Jon Kartago Lamida <[email protected]> * Refactor test Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix comment Signed-off-by: Jon Kartago Lamida <[email protected]> * Apply more PR suggestions Signed-off-by: Jon Kartago Lamida <[email protected]> * Refactor configs Signed-off-by: Jon Kartago Lamida <[email protected]> * Header eager loading only must be done during initialSync Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove unnecesary comment Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove snapshot file regardless snapshot loading result Signed-off-by: Jon Kartago Lamida <[email protected]> * Fix compilation error Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/header.go Co-authored-by: Charles Korn <[email protected]> * Regenerate doc for updated command line args Signed-off-by: Jon Kartago Lamida <[email protected]> * Update error log Signed-off-by: Jon Kartago Lamida <[email protected]> * Update comment Signed-off-by: Jon Kartago Lamida <[email protected]> * Update comment Signed-off-by: Jon Kartago Lamida <[email protected]> * Update test Signed-off-by: Jon Kartago Lamida <[email protected]> * Add test case Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/storegateway/indexheader/reader_pool_test.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/storegateway/indexheader/lazy_binary_reader.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/storegateway/indexheader/reader_pool.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/storegateway/indexheader/header.go Co-authored-by: Charles Korn <[email protected]> * Update changelog Signed-off-by: Jon Kartago Lamida <[email protected]> --------- Signed-off-by: Jon Kartago Lamida <[email protected]> Co-authored-by: Charles Korn <[email protected]> Co-authored-by: Dimitar Dimitrov <[email protected]>

Signed-off-by: Jon Kartago Lamida <[email protected]>

CHANGELOG.md

docs/sources/mimir/references/configuration-parameters/index.md

pkg/mimir/modules.go

Signed-off-by: Jon Kartago Lamida <[email protected]>

dimitarvdimitrov · 2023-08-23T12:49:15Z

pkg/mimir/modules.go

-	if t.Cfg.BlocksStorage.BucketStore.DeprecatedIndexHeaderLazyLoadingIdleTimeout > 0 {
+	// TODO: Remove in Mimir 2.12.
+	// Take the non-default value (default value is 1h) from IndexHeader deprecated config and if it is set, set it to the new config
+	if t.Cfg.BlocksStorage.BucketStore.DeprecatedIndexHeaderLazyLoadingIdleTimeout != 60*time.Minute {


Should we move the 60m value into a constant and reuse it in the flag registration? That will also make the comment redundant

I tried to move to tsdb package but it result an import cycle. Let me find a good common package to place this.

Signed-off-by: Jon Kartago Lamida <[email protected]>

dimitarvdimitrov

awesome, LGTM!

lamida marked this pull request as ready for review August 14, 2023 09:46

lamida requested review from a team as code owners August 14, 2023 09:46

lamida requested a review from colega August 14, 2023 09:47

lamida mentioned this pull request Aug 14, 2023

store-gateway: retain lazy-loaded index headers between restarts #5606

Merged

3 tasks

dimitarvdimitrov reviewed Aug 14, 2023

View reviewed changes

pkg/storegateway/indexheader/header.go Outdated Show resolved Hide resolved

lamida changed the base branch from lamida/sg-lazy-load to main August 14, 2023 13:54

lamida requested a review from dimitarvdimitrov August 14, 2023 13:59

lamida force-pushed the lamida/index-header-configs branch from d917db4 to c0a6575 Compare August 14, 2023 14:49

colega reviewed Aug 16, 2023

View reviewed changes

lamida requested a review from colega August 17, 2023 07:50

lamida force-pushed the lamida/index-header-configs branch from 07a4f41 to fa9fc82 Compare August 17, 2023 09:14

lamida and others added 14 commits August 18, 2023 16:29

Fix golangci-lint

867b24c

Signed-off-by: Jon Kartago Lamida <[email protected]>

Move indexheader config to its own package

d6c1aaa

Signed-off-by: Jon Kartago Lamida <[email protected]>

Remove and deprecate index header config outside indexheader package

3a4513b

Signed-off-by: Jon Kartago Lamida <[email protected]>

Remove deprecated config in mimir 2.12

28700f1

Signed-off-by: Jon Kartago Lamida <[email protected]>

Add changelogs

de3687d

Signed-off-by: Jon Kartago Lamida <[email protected]>

Update configuration test

ad26dff

Signed-off-by: Jon Kartago Lamida <[email protected]>

Address PR feedbacks

d05ed50

Signed-off-by: Jon Kartago Lamida <[email protected]>

Remove redundant index_header prefix in config

01c8f2e

Signed-off-by: Jon Kartago Lamida <[email protected]>

Address PR feedbacks

ca66fda

Signed-off-by: Jon Kartago Lamida <[email protected]>

Move comment to the struct

7861c19

Signed-off-by: Jon Kartago Lamida <[email protected]>

Fix golangci-lint

ecc51a8

Signed-off-by: Jon Kartago Lamida <[email protected]>

Move the deprecated flag set to setupModules

847b2e0

Signed-off-by: Jon Kartago Lamida <[email protected]>

lamida force-pushed the lamida/index-header-configs branch from fa9fc82 to 847b2e0 Compare August 18, 2023 08:30

dimitarvdimitrov reviewed Aug 22, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

docs/sources/mimir/references/configuration-parameters/index.md Show resolved Hide resolved

pkg/mimir/modules.go Outdated Show resolved Hide resolved

pkg/mimir/modules.go Outdated Show resolved Hide resolved

lamida added 2 commits August 23, 2023 10:18

Update changelog

982d0f1

Signed-off-by: Jon Kartago Lamida <[email protected]>

Take deprecated lazy loading flag only if it is the non default

22a5d0c

Signed-off-by: Jon Kartago Lamida <[email protected]>

Take deprecated lazy loading flag only if it is the non default

3e6a40f

Signed-off-by: Jon Kartago Lamida <[email protected]>

lamida requested a review from dimitarvdimitrov August 23, 2023 02:39

lamida added 2 commits August 23, 2023 10:41

Fix const import causing import cycle

54cfc1c

Signed-off-by: Jon Kartago Lamida <[email protected]>

Update comment to make it less confusing

6bb5dba

Signed-off-by: Jon Kartago Lamida <[email protected]>

dimitarvdimitrov reviewed Aug 23, 2023

View reviewed changes

Extract constant for deprecated lazyloading configs

c7cb6f1

Signed-off-by: Jon Kartago Lamida <[email protected]>

dimitarvdimitrov approved these changes Aug 23, 2023

View reviewed changes

dimitarvdimitrov merged commit 9c763b3 into main Aug 23, 2023

dimitarvdimitrov deleted the lamida/index-header-configs branch August 23, 2023 14:59

lamida mentioned this pull request Aug 28, 2023

Update runbook and documentation after index header config refactor #5847

Open

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

Refactor Index Header Configs #5726

Refactor Index Header Configs #5726

lamida commented Aug 14, 2023 •

edited

Loading

colega Aug 16, 2023

lamida Aug 17, 2023

dimitarvdimitrov Aug 23, 2023

lamida Aug 23, 2023

dimitarvdimitrov left a comment

Refactor Index Header Configs #5726

Refactor Index Header Configs #5726

Conversation

lamida commented Aug 14, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

colega Aug 16, 2023

Choose a reason for hiding this comment

lamida Aug 17, 2023

Choose a reason for hiding this comment

dimitarvdimitrov Aug 23, 2023

Choose a reason for hiding this comment

lamida Aug 23, 2023

Choose a reason for hiding this comment

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

lamida commented Aug 14, 2023 •

edited

Loading