index-shipper: add support for multiple stores #7754

ashwanthgoli · 2022-11-23T11:06:42Z

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

What this PR does / why we need it:
Currently loki initializes a single instance of index-shipper to handle all the table ranges (from across periods) for a given index type boltdb-shipper, tsdb. Since index-shipper only has the object client handle to the store defined by shared_store_type, it limits the index uploads to a single store. Setting shared_store_type to a different store at a later point in time would mean losing access to the indexes stored in the previously configured store.

With this PR, we initialize a separate index-shipper & table manager for each period if shared_store_type is not explicity configured. This offers the flexibility to store index in multiple stores (across providers).

Note:

usage of shared_store_type in this commit text refers to one of these config options depending on the index in use: -boltdb.shipper.shared-store, -tsdb.shipper.shared-store
shared_store_type used to default to the object_store from the latest period_config if not explicitly configured. This PR removes these defaults in favor of supporting index uploads to multiple stores.

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

Special notes for your reviewer:
All the instances of downloads table manager operate on the same cacheDir. But it shouldn't be a problem as the tableRanges do not overlap across periods.

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-12-26T12:01:32Z

./tools/diff_coverage.sh ../loki-target-branch/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.1%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
-            storage	-2.8%
+           chunkenc	0%
+              logql	0%
+               loki	1.2%

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

sandeepsukhani · 2023-03-20T11:01:57Z

pkg/storage/stores/tsdb/head_manager.go

+	return nil
+}
+
+func (m *HeadManager) buildLegacyWALs() error {


lets name it a bit better to make it clearer

Suggested change

func (m *HeadManager) buildLegacyWALs() error {

func (m *HeadManager) buildTSDBFromLegacyWALs() error {

sandeepsukhani · 2023-03-20T11:02:14Z

pkg/storage/stores/tsdb/head_manager.go

+	return m.buildWALs(parentDir, true)
+}
+
+func (m *HeadManager) buildWALs(dir string, legacy bool) error {


lets name it a bit better to make it clearer

Suggested change

func (m *HeadManager) buildWALs(dir string, legacy bool) error {

func (m *HeadManager) buildTSDBFromWALs(dir string, legacy bool) error {

sandeepsukhani · 2023-03-20T11:47:25Z

pkg/storage/stores/tsdb/store.go

-// we do not need to build store for each schema config since we do not do any schema specific handling yet.
-// If we do need to do schema specific handling, it would be a good idea to abstract away the handling since
-// running multiple head managers would be complicated and wasteful.
+// NewStore creates a new tsdb index ReaderWriter.
 var NewStore = func() newStoreFactoryFunc {


newStoreFactoryFunc was just a hack to make tsdb store instances a singleton. I think we can get rid of it.

sandeepsukhani · 2023-03-20T11:54:07Z

Great job Ashwanth! The changes look great to me. I think we can merge the PR once we get to test it in a dev cell and ensure things work as expected.

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

sandeepsukhani

The changes look great and thanks for taking care of all the feedback. Just two minor suggestions, otherwise we are good to merge this.

integration/loki_micro_services_test.go

pkg/storage/stores/tsdb/head_manager.go

I'll raise another pr to verify the flush behavior This reverts commit ee7228c.

… multiple stores (#10306) Add PR #7754 to release notes for next release --------- Co-authored-by: Ashwanth Goli <iamashwanth@gmail.com>

pull-request-size bot added the size/L label Nov 23, 2022

ashwanthgoli force-pushed the ashwanth/index-shipper-multi-store branch from 28c6293 to 1d4792a Compare December 12, 2022 04:33

pull-request-size bot added size/XL and removed size/L labels Dec 12, 2022

config_wrapper: Add support for named stores

fde9a74

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

pull-request-size bot added size/XXL and removed size/XL labels Dec 16, 2022

ashwanthgoli added 8 commits December 21, 2022 17:03

Merge branch 'main' into ashwanth/multi-store-single-provider

07cec72

doc-parser: correctly display map elems referring root blocks

6ef4b98

Merge branch 'main' into ashwanth/multi-store-single-provider

5d45288

doc: make named_stores a root block

15d8bda

update documentation

ac51a85

add changelog

4591364

test: reset boltdb shipper before store init

f1ea08c

Merge branch 'main' into ashwanth/multi-store-single-provider

a09d87f

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

ashwanthgoli added 3 commits December 28, 2022 12:02

nit

c0bc6c3

Merge branch 'main' into ashwanth/multi-store-single-provider

695115f

index-shipper: add support for multiple stores

054358a

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

ashwanthgoli force-pushed the ashwanth/index-shipper-multi-store branch from a7b32d7 to 054358a Compare December 28, 2022 07:37

ashwanthgoli changed the base branch from main to ashwanth/multi-store-single-provider December 28, 2022 07:38

pull-request-size bot added size/XL and removed size/XXL labels Dec 28, 2022

ashwanthgoli added 4 commits December 28, 2022 13:10

fix testcases

aabd28b

fix TestMicroServicesDeleteRequest

9162270

add integration test for multi store

75a725a

fix lint

cb382e2

pull-request-size bot added size/XXL and removed size/XL labels Dec 28, 2022

ashwanthgoli added 4 commits March 13, 2023 11:23

nit

ed62241

Merge branch 'main' into ashwanth/index-shipper-multi-store

29e3a04

fix broken tests

be34089

add testcases for TableRanges and GetIndexStoreTableRanges

4a7843f

ashwanthgoli requested a review from sandeepsukhani March 13, 2023 08:05

ashwanthgoli added 4 commits March 15, 2023 18:31

Merge branch 'main' into ashwanth/index-shipper-multi-store

6724a5f

use yaml merger for adding schema configs

3c95c4b

Merge branch 'main' into ashwanth/index-shipper-multi-store

79f1e5e

fix build

d6ab4ff

sandeepsukhani approved these changes Mar 20, 2023

View reviewed changes

ashwanthgoli added 7 commits March 20, 2023 18:35

review suggestions

addb36f

do not use store specific dir for tsdb manager

ab67fed

Merge branch 'main' into ashwanth/index-shipper-multi-store

00b2595

Merge branch 'main' into ashwanth/index-shipper-multi-store

06b0f72

remove storage.Store scoped singleton of tsdb.Store

70a5a6e

tidy

8782890

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

nit

942a561

sandeepsukhani approved these changes Apr 13, 2023

View reviewed changes

integration/loki_micro_services_test.go Show resolved Hide resolved

pkg/storage/stores/tsdb/head_manager.go Show resolved Hide resolved

ashwanthgoli added 2 commits April 13, 2023 18:45

loki_micro_services_test: verify chunks are flushed

ee7228c

Revert "loki_micro_services_test: verify chunks are flushed"

bc9ae87

I'll raise another pr to verify the flush behavior This reverts commit ee7228c.

sandeepsukhani merged commit 5cef03d into main Apr 14, 2023
4 checks passed

sandeepsukhani deleted the ashwanth/index-shipper-multi-store branch April 14, 2023 06:02

ashwanthgoli mentioned this pull request Jun 30, 2023

Period config activated ahead of its from date #9837

Closed

ashwanthgoli added the add-to-release-notes label Aug 22, 2023

grafanabot mentioned this pull request Aug 22, 2023

[Release Notes Appender] Add PR #7754: index-shipper: add support for multiple stores #10306

Merged

grafanabot added a commit that referenced this pull request Aug 22, 2023

Add PR #7754 to release notes for next release

7618596

Erichorng mentioned this pull request Aug 30, 2023

Multi-store support for index and compactor doesn't work as expect. #10397

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

index-shipper: add support for multiple stores #7754

index-shipper: add support for multiple stores #7754

ashwanthgoli commented Nov 23, 2022 •

edited

grafanabot commented Dec 26, 2022

sandeepsukhani Mar 20, 2023

sandeepsukhani Mar 20, 2023

sandeepsukhani Mar 20, 2023

sandeepsukhani commented Mar 20, 2023

sandeepsukhani left a comment

	func (m *HeadManager) buildLegacyWALs() error {
	func (m *HeadManager) buildTSDBFromLegacyWALs() error {

	func (m *HeadManager) buildWALs(dir string, legacy bool) error {
	func (m *HeadManager) buildTSDBFromWALs(dir string, legacy bool) error {

index-shipper: add support for multiple stores #7754

index-shipper: add support for multiple stores #7754

Conversation

ashwanthgoli commented Nov 23, 2022 • edited

grafanabot commented Dec 26, 2022

sandeepsukhani Mar 20, 2023

Choose a reason for hiding this comment

sandeepsukhani Mar 20, 2023

Choose a reason for hiding this comment

sandeepsukhani Mar 20, 2023

Choose a reason for hiding this comment

sandeepsukhani commented Mar 20, 2023

sandeepsukhani left a comment

Choose a reason for hiding this comment

ashwanthgoli commented Nov 23, 2022 •

edited