compactor: multi-store support #7447

ashwanthgoli · 2022-10-18T09:15:51Z

What this PR does / why we need it:
This PR adds multi-store support for compactors. Since loki allows users to configure mutiple stores using schema_config, compactor should be able to operate on multiple object stores that contain index. Currently, it can perform compaction on indexes in a single store.

To maintain backward compatibility: if boltdb.shipper.compactor.shared-store is set, compactor will only operate on that store, else compactor will be initialized to operate on all the object store indexes (boltdb, tsdb) defined in the schema config.

This PR also adds a new config option to define where delete requests are to be stored - boltdb.shipper.compactor.delete-request-store. If it's not set, boltdb.shipper.compactor.shared-store is used for storing them, this is to ensure no config changes are required by the users when upgrading. Refer to docs/sources/upgrading/_index.md for more details.

Which issue(s) this PR fixes:
Fixes #7276

Checklist

Reviewed the CONTRIBUTING.md guide
Documentation added
Tests updated
CHANGELOG.md updated
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>

grafanabot · 2022-10-18T10:55:59Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.5%

grafanabot · 2022-10-18T11:06:35Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.5%

dannykopping

I can't comment on the compaction behaviour because I'm not too familiar with it, but overall the changes LGTM.

I've left a few nits for clarity and idiomatic Go practices

pkg/loki/modules.go

pkg/storage/config/schema_config.go

pkg/storage/stores/indexshipper/compactor/compactor.go

dannykopping · 2022-10-19T07:21:56Z

pkg/storage/stores/indexshipper/compactor/compactor.go

 	f.StringVar(&cfg.SharedStoreKeyPrefix, "boltdb.shipper.compactor.shared-store.key-prefix", "index/", "Prefix to add to Object Keys in Shared store. Path separator(if any) should always be a '/'. Prefix should never start with a separator but should always end with it.")
 	f.DurationVar(&cfg.CompactionInterval, "boltdb.shipper.compactor.compaction-interval", 10*time.Minute, "Interval at which to re-run the compaction operation.")
 	f.DurationVar(&cfg.ApplyRetentionInterval, "boltdb.shipper.compactor.apply-retention-interval", 0, "Interval at which to apply/enforce retention. 0 means run at same interval as compaction. If non-zero, it should always be a multiple of compaction interval.")
 	f.DurationVar(&cfg.RetentionDeleteDelay, "boltdb.shipper.compactor.retention-delete-delay", 2*time.Hour, "Delay after which chunks will be fully deleted during retention.")
 	f.BoolVar(&cfg.RetentionEnabled, "boltdb.shipper.compactor.retention-enabled", false, "(Experimental) Activate custom (per-stream,per-tenant) retention.")
 	f.IntVar(&cfg.RetentionDeleteWorkCount, "boltdb.shipper.compactor.retention-delete-worker-count", 150, "The total amount of worker to use to delete chunks.")
+	f.StringVar(&cfg.DeleteRequestStore, "boltdb.shipper.compactor.delete-request-store", "", "Store used for managing delete requests. If not set, shared_store is used as a fallback.")


Do we need to even provide this as an option? Under what circumstances do you see a separate store being needed?
I do like that you're falling back to the shared store, though.

It need not be a separate store, users can choose from one of the object stores used for index.

When it comes to index, we know where to route reads/writes because of the schema_config, but I believe the same cannot be done when storing delete requests as they don't have any period associated with them.

We could implicitly assume that delete requests should always go to the object store referred to in the latest schema_config entry, but this would result in compactor not processing any pending requests from older stores. I thought making this configurable would allow users to change it as they see fit.

I could also document this better

pkg/storage/stores/indexshipper/compactor/compactor.go

pkg/storage/stores/indexshipper/compactor/retention/retention.go

JStickler

[Doc squad] I had a couple of small suggestions for wording.

docs/sources/upgrading/_index.md

JStickler

[Docs squad} Documentation LGTM. Couple of small suggestions.

docs/sources/configuration/_index.md

docs/sources/upgrading/_index.md

sandeepsukhani

overall changes look good to me with the initial pass. I need to think more on how we approach the deletion markers migration.

sandeepsukhani · 2023-03-03T08:36:28Z

docs/sources/upgrading/_index.md

+
+If `-boltdb.shipper.compactor.shared-store` is not set, it defaults to the `object_store` configured in the latest `period_config` that uses either the tsdb or boltdb-shipper index.
+
+In releases 2.7.5 and later, the Compactor supports index compaction on multiple stores.


Suggested change

In releases 2.7.5 and later, the Compactor supports index compaction on multiple stores.

In releases 2.7.5 and later, the Compactor supports index compaction on multiple buckets/object stores.

sandeepsukhani · 2023-03-03T08:49:30Z

pkg/loki/config_wrapper.go

-		if i := lastBoltdbShipperConfig(r.SchemaConfig.Configs); i != len(r.SchemaConfig.Configs) {
-			betterBoltdbShipperDefaults(r, &defaults, r.SchemaConfig.Configs[i])
-		}
-
 		if i := lastTSDBConfig(r.SchemaConfig.Configs); i != len(r.SchemaConfig.Configs) {
 			betterTSDBShipperDefaults(r, &defaults, r.SchemaConfig.Configs[i])
 		}

+		if i := lastBoltdbShipperConfig(r.SchemaConfig.Configs); i != len(r.SchemaConfig.Configs) {
+			betterBoltdbShipperDefaults(r, &defaults, r.SchemaConfig.Configs[i])
+		}
+


I think we should use the last boltdb-shipper or tsdb config. Here we would just overwrite the value set by the previous one, which could be different. We can maybe define a separate function for compactor betterCompactorDefaults

sandeepsukhani · 2023-03-03T09:05:38Z

pkg/storage/stores/indexshipper/compactor/compactor.go

 		}

-		if err := c.initDeletes(r, limits); err != nil {
-			return err
+		if err := retention.MigrateMarkers(filepath.Join(c.cfg.WorkingDirectory, "retention"), deleteRequestStore); err != nil {


deletion markers might not always belong to objects in the delete requests store. There is a high chance when someone wants to start using a new bucket, they would change the bucket for delete requests store as well. In that case we will try deleting objects from new bucket here while the objects were stored in older bucket. Will think more and see what we can do here.

sandeepsukhani · 2023-03-03T09:20:49Z

pkg/storage/stores/indexshipper/compactor/compactor.go

-			encoder = client.FSEncoder
+		deleteRequestStore := c.cfg.DeleteRequestStore
+		// if -boltdb.shipper.compactor.shared-store is set, use it instead to ensure backward compatibility.
+		if c.cfg.SharedStoreType != "" {


I think we should give precedence to c.cfg.DeleteRequestStore since it is a new and explicit config. Any change in that config would have been made by user deliberately. Also, it looks inconsistent not honoring an explicit config.

Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>

…nfig

sandeepsukhani

The changes look great. Just have some non-blocking changes for code readablitiy so just approving the PR. Will merge it once the feedback has been addressed.

sandeepsukhani · 2023-03-21T06:18:30Z

pkg/storage/stores/indexshipper/compactor/retention/retention.go

@@ -87,12 +90,11 @@ type Marker struct {
 	markTimeout      time.Duration
 }

-func NewMarker(workingDirectory string, expiration ExpirationChecker, markTimeout time.Duration, chunkClient client.Client, r prometheus.Registerer) (*Marker, error) {
-	metrics := newMarkerMetrics(r)
+func NewMarker(workingDirectory, objectStoreType string, expiration ExpirationChecker, markTimeout time.Duration, chunkClient client.Client, r prometheus.Registerer) (*Marker, error) {


unused param objectStoreType

sandeepsukhani · 2023-03-21T06:19:09Z

pkg/storage/stores/indexshipper/compactor/retention/retention.go

@@ -272,8 +274,9 @@ type Sweeper struct {
 	sweeperMetrics  *sweeperMetrics
 }

-func NewSweeper(workingDir string, deleteClient ChunkClient, deleteWorkerCount int, minAgeDelete time.Duration, r prometheus.Registerer) (*Sweeper, error) {
+func NewSweeper(workingDir, objectStoreType string, deleteClient ChunkClient, deleteWorkerCount int, minAgeDelete time.Duration, r prometheus.Registerer) (*Sweeper, error) {


used param objectStoreType

sandeepsukhani · 2023-04-24T12:33:07Z

pkg/storage/stores/indexshipper/compactor/compactor.go

@@ -79,6 +80,8 @@ type Config struct {
 	RetentionDeleteDelay      time.Duration   `yaml:"retention_delete_delay"`
 	RetentionDeleteWorkCount  int             `yaml:"retention_delete_worker_count"`
 	RetentionTableTimeout     time.Duration   `yaml:"retention_table_timeout"`
+	DeleteRequestStore        string          `yaml:"delete_request_store"`
+	LegacySharedStoreDefault  string          `yaml:"-" doc:"hidden"`


We can be more specific here and call it like DefaultDeleteRequestStore or LegacyDefaultDeleteRequestStore.

sandeepsukhani · 2023-04-24T12:38:05Z

pkg/storage/config/schema_config.go

-	return usingForPeriodConfigs(configs, fn)
+// ContainsObjectStorageIndex returns true if the current or any of the upcoming periods
+// use an object store index.
+func ContainsObjectStorageIndex(configs []PeriodConfig) bool {


I called it UsingObjectStoreIndex because it checked whether current or upcoming index is an object storage based index. This new name makes it confusing. Maybe swap UsingObjectStoreIndex and ContainsObjectStorageIndex names?

sandeepsukhani · 2023-04-24T12:42:13Z

pkg/storage/stores/indexshipper/compactor/retention/retention.go

+
+// since compactor supports multiple stores, markers need to be written to store specific dir.
+// MigrateMarkers checks for markers in retention dir and migrates them.
+func MigrateMarkers(workingDir string, store string) error {


Let us be more specific and call it CopyMarkers? Also maybe make the comment more descriptive saying we are copying markers to store specific marker directory?

Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>

sandeepsukhani

LGTM

**What this PR does / why we need it**: noticed that a couple of unreleased changes are incorrectly showing under 2.8.0 release, moving them to the right place. also adds the missing changelog entry for #7447 **Checklist** - [X] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>

ashwanthgoli added 2 commits October 18, 2022 10:32

compactor: add multi-store support

28e490a

Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>

compactor: add test cases for multi-store

8a120f1

ashwanthgoli requested a review from a team as a code owner October 18, 2022 09:15

pull-request-size bot added the size/L label Oct 18, 2022

ashwanthgoli added 2 commits October 18, 2022 15:48

fix lint in compactor_test

161cb2a

add documentation

91b8f02

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Oct 18, 2022

dannykopping reviewed Oct 19, 2022

View reviewed changes

ashwanthgoli marked this pull request as draft January 19, 2023 13:57

ashwanthgoli added 7 commits February 9, 2023 16:50

nit

555b921

Merge branch 'main' into ashwanth/compactor-multi-store

fccb9c0

add better defaults for delete_request_store

5a06049

fix tests

47a5828

add upgrade instructions

16bf55d

prefer tsdb compactor defaults over boltdb

195167d

Merge branch 'main' into ashwanth/compactor-multi-store

a85d01c

ashwanthgoli marked this pull request as ready for review February 23, 2023 10:32

ashwanthgoli requested a review from JStickler as a code owner February 23, 2023 10:32

ashwanthgoli requested a review from a team February 23, 2023 10:32

JStickler reviewed Feb 23, 2023

View reviewed changes

docs squad suggestions

8ab700d

JStickler approved these changes Feb 27, 2023

View reviewed changes

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved

docs/sources/upgrading/_index.md Outdated Show resolved Hide resolved

docs/sources/upgrading/_index.md Outdated Show resolved Hide resolved

ashwanthgoli added 2 commits February 28, 2023 11:03

docs squad suggestions

0a0d7c0

run make doc

55bd54e

sandeepsukhani reviewed Mar 3, 2023

View reviewed changes

ashwanthgoli added 3 commits March 8, 2023 14:24

migrate markers based on shared store or it's legacy defaults

b563f6c

upgrade upgrade instructions

5a01a55

nit

a27165d

Merge branch 'main' into ashwanth/compactor-multi-store

cd34d49

ashwanthgoli requested a review from sandeepsukhani March 8, 2023 09:36

ashwanthgoli added 3 commits March 8, 2023 15:52

hide yaml field

bed6d81

chunk client use fs encoder

d512928

migrate markers to all store specific dirs

146e906

pull-request-size bot added size/XL and removed size/L labels Apr 14, 2023

ashwanthgoli added 7 commits April 14, 2023 14:48

Merge branch 'main' into ashwanth/compactor-multi-store

501fcfc

update release version in upgrade notes

2b473e3

fix lint

7d0f01d

Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>

init deletes before marker

bccb9ce

Merge branch 'main' into ashwanth/compactor-multi-store

48b44be

do not set compactor shared_store defaults based on common storage co…

5871a03

…nfig

update deletion test to use multiple periods

b17b302

sandeepsukhani approved these changes Apr 24, 2023

View reviewed changes

ashwanthgoli added 5 commits April 25, 2023 13:18

minor changes for code readablitiy

774e712

Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>

fixup! minor changes for code readablitiy

8cf21f4

update release notes

5225c80

remove release version from upgrade notes

c2eaa8d

Merge branch 'main' into ashwanth/compactor-multi-store

4d300ae

sandeepsukhani approved these changes Apr 25, 2023

View reviewed changes

sandeepsukhani merged commit 52cd0a3 into main Apr 25, 2023
4 checks passed

sandeepsukhani deleted the ashwanth/compactor-multi-store branch April 25, 2023 10:04

ashwanthgoli mentioned this pull request Jun 22, 2023

changelog: moving unreleased changes to the main section #9769

Merged

7 tasks

ashwanthgoli mentioned this pull request Jun 30, 2023

Period config activated ahead of its from date #9837

Closed

ashwanthgoli mentioned this pull request Sep 1, 2023

Only the data of the current object store is returned when multiple schema configs on different object stores #9148

Open

clwluvw mentioned this pull request Oct 20, 2023

Wrong compactor index client for schema #10985

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compactor: multi-store support #7447

compactor: multi-store support #7447

ashwanthgoli commented Oct 18, 2022 •

edited

grafanabot commented Oct 18, 2022

grafanabot commented Oct 18, 2022

dannykopping left a comment

dannykopping Oct 19, 2022

ashwanthgoli Oct 19, 2022

JStickler left a comment

JStickler left a comment

sandeepsukhani left a comment

sandeepsukhani Mar 3, 2023

sandeepsukhani Mar 3, 2023

sandeepsukhani Mar 3, 2023

sandeepsukhani Mar 3, 2023

sandeepsukhani left a comment

sandeepsukhani Mar 21, 2023

sandeepsukhani Mar 21, 2023

sandeepsukhani Apr 24, 2023

sandeepsukhani Apr 24, 2023

sandeepsukhani Apr 24, 2023

sandeepsukhani left a comment


		If `-boltdb.shipper.compactor.shared-store` is not set, it defaults to the `object_store` configured in the latest `period_config` that uses either the tsdb or boltdb-shipper index.

		In releases 2.7.5 and later, the Compactor supports index compaction on multiple stores.

compactor: multi-store support #7447

compactor: multi-store support #7447

Conversation

ashwanthgoli commented Oct 18, 2022 • edited

grafanabot commented Oct 18, 2022

grafanabot commented Oct 18, 2022

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JStickler left a comment

Choose a reason for hiding this comment

JStickler left a comment

Choose a reason for hiding this comment

sandeepsukhani 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

sandeepsukhani 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

sandeepsukhani left a comment

Choose a reason for hiding this comment

ashwanthgoli commented Oct 18, 2022 •

edited