Add a Ring to IndexGateway #5358

JordanRushing · 2022-02-09T21:04:39Z

Signed-off-by: Jordan Rushing [email protected]

What this PR does / why we need it:

This PR introduces a ring to Loki's IndexGateway to provide better horizontal scalability via per-tenant hashing with changes to the associated Index files provided by per-tenant compaction introduced in #5026 which will reduce overall startup time for new IndexGateway instances.

IndexGateway now includes a new configuration, RingMode. When this mode is enabled each IndexGateway instance becomes responsible for a subset of tenant indices.

In PRs to follow, we will update TableManager to reduce the amount of tenant data downloaded and add metrics.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

N/A

Checklist

Documentation added
Tests updated
Add an entry in the CHANGELOG.md about the changes.

DylanGuedes

a few nits

Signed-off-by: Jordan Rushing <[email protected]>

- Implement missing methods for the IndexGateway to be used as a BasicLifecyclerDelegate. The methods are stored in a lifecycle file and follow the same approach of the Ruler ring - Make Gateway public and add missing parameters to the IndexGateway's initialization method

Signed-off-by: Jordan Rushing <[email protected]>

- Add memberlist as a dependency of the IndexGateway - Add IndexGateway support for the common configuration section - Modify Loki to register IndexGateway flags. This fixes the default flag values for the IndexGateway - Make IndexGateway configurations public. Without this, we can't use them.

- If IndexGateway is running with a ring, it uses the IdleService. Otherwise, it uses the BasicService - Implement IndexGateway support to handle ring HTTP pages

- Implement new IndexGatewayRing service - Add IndexGatewayRing as a Store dependency - Modify store to pass IndexGatewayRing as a parameter - Implement IndexGatewayClient ring mode

- Moves IndexGateway configuration to the IndexGatewayClient struct within the boltdb-shipper - Reuse the IndexGatewayClient everywhere - Implement IndexGateway gRPC pool

- Instead, add it to the store configuration and bubble it down to deeper modules.

Signed-off-by: JordanRushing <[email protected]>

trevorwhitney

way to put a ring on it, look good.

trevorwhitney · 2022-03-30T21:45:49Z

pkg/loki/loki.go

@@ -486,6 +490,7 @@ func (t *Loki) setupModuleManager() error {
 	mm.RegisterModule(Compactor, t.initCompactor)
 	mm.RegisterModule(IndexGateway, t.initIndexGateway)
 	mm.RegisterModule(QueryScheduler, t.initQueryScheduler)
+	mm.RegisterModule(IndexGatewayRing, t.initIndexGatewayRing, modules.UserInvisibleModule)


Is the reason this needs to be a top level module (vs. having the logic to enable/disable the ring be inside the initIndexGateway code) because it's a shared dependency between both the Store and the IndexGateway? Can those module be enabled/disabled independent of one another?

For context: there's an index gateway server and index gateway clients. The clients needs a read ring (provided by the IndexGatewayRing service), while the server needs the whole index gateway service. So, the server needs way more stuff than the other services (ex: the store) as they only need a ring, and sharing the whole server with the store was too much.

Now, to your question: yes, this had to be a top-level module, but not because the ring is shared between the gateway server and client, because although both use the ring, they use it differently (ex: the client only requires a read ring, the server requires a lifecycler+a normal ring). This had to be a top-level module to isolate a very lean read ring, used by the clients.

To your last question: Yes, they can be disabled/independent because you can run an index gateway ring without any clients connecting to it, and you can also have an index gateway client without any server registered in the ring and it will still run - but won't communicate with any index gateway.

trevorwhitney · 2022-03-30T21:47:34Z

pkg/loki/modules.go

+		return nil, err
+	}
+
+	t.Server.HTTP.Path("/indexgateway/ring").Methods("GET", "POST").Handler(gateway)


note, when this is merged, we'll have to update the enterprise gateway in GEL as well as the nginx configs in a few helm charts to include this new route.

sandeepsukhani

Added some minor suggestions but otherwise the changes LGTM! I am wondering if we should have some tests in the gateway client in ring mode but we will have to mock most of the things so whether it would make a good test 🤔

pkg/storage/stores/shipper/indexgateway/gateway.go

pkg/storage/stores/shipper/gateway_client.go

Signed-off-by: JordanRushing <[email protected]>

- Add `replication_factor` flag to the IndexGateway config struct - Modify Index Gateway ring config struct to be inline with a new struct, since it doesn't expose a replication factor config - Modify dynamic config wrapper to reuse a common replication factor on the Index Gateway ring

- If we don't randomize access, we'll always access same Index Gateway instances in same order for the same tenant

…-gateway

pkg/storage/stores/shipper/indexgateway/config.go

pkg/loki/config_wrapper.go

…-gateway

- This is accomplished by using `NewIgnoreUnhealthyInstancesReplicationStrategy` - It is already done by the server

pkg/loki/config_wrapper.go

- Remove unnecessary/wrong comments - Only set replication factor at a single place

periklis

Looking on this PR from a high level perspective, it is a great piece of work to make Loki tenant x horizontal scalable. So a big thanks for this!

periklis · 2022-04-12T12:20:13Z

pkg/storage/stores/shipper/gateway_client.go

+		if err != nil {
+			level.Error(util_log.Logger).Log("msg", fmt.Sprintf("failed to get client for instance %s", addr), "err", err)
+			continue
+		}
+
+		client := (genericClient.(indexgatewaypb.IndexGatewayClient))
+		if err := s.clientDoQueries(ctx, gatewayQueries, queryKeyQueryMap, callback, client); err != nil {
+			level.Error(util_log.Logger).Log("msg", fmt.Sprintf("client do queries failed for instance %s", addr), "err", err)
+			continue
+		}


My suggestion here (or in any other place more suitable), it would be nice to have some sort of reporting on failures based on tenant IDs. For example we report per-tenant rate limits or distributor append failures. The index gateway is another great piece where looking on errors I should be able to determine which tenant is impacted by which index gateway replica. WDYT?

hey that's a great idea. We already have plans to add metrics in a follow-up PR (and there are other things missing too), but I'll make sure to add this to our TODO list.

just giving you a heads-up: I tried but looks like we can't determine which tenant ID was related to a failure here. This should actually be reported by the index gateway and the table manager. I'm sure we already have error logs for when something unexpected happens there, I'll check if we can add any new metrics there too.

sandeepsukhani

LGTM

JordanRushing added the component/index-gateway label Feb 9, 2022

JordanRushing self-assigned this Feb 9, 2022

JordanRushing assigned DylanGuedes Feb 9, 2022

pull-request-size bot added the size/M label Feb 9, 2022

DylanGuedes reviewed Feb 9, 2022

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Feb 10, 2022

DylanGuedes force-pushed the add-ring-to-index-gateway branch from 18071fa to 7d5b156 Compare February 24, 2022 20:00

JordanRushing and others added 21 commits March 1, 2022 15:00

Begin to add a Ring to IndexGateway

0ba13f0

Signed-off-by: Jordan Rushing <[email protected]>

Fix failing linter

325ada2

Signed-off-by: Jordan Rushing <[email protected]>

Implement NewBasicService for the IndexGateway.

c7797f4

- If IndexGateway is running with a ring, it uses the IdleService. Otherwise, it uses the BasicService - Implement IndexGateway support to handle ring HTTP pages

Test IndexGateway dynamic configuration.

b6d6629

Implement new IndexGatewayGRPCPool entity.

99df166

Make IndexGateway attributes public.

6eb1595

Implement IndexGatewayRing reader.

0417045

- Implement new IndexGatewayRing service - Add IndexGatewayRing as a Store dependency - Modify store to pass IndexGatewayRing as a parameter - Implement IndexGatewayClient ring mode

Implement Ring mode in the IndexGatewayClient.

55db3eb

- Moves IndexGateway configuration to the IndexGatewayClient struct within the boltdb-shipper - Reuse the IndexGatewayClient everywhere - Implement IndexGateway gRPC pool

Add new ring index gateway parameter to new Store calls.

d884301

Use errors.Wrap instead of fmt.Errorf.

6cc5034

Extract tenantID from context instead of iterating on queries.

82393f1

Remove indexGateway ring param.

a4f717c

- Instead, add it to the store configuration and bubble it down to deeper modules.

Split IndexGateway server from client implementation.

88224a8

Fix imports order.

6d68a6c

Remove ring as parameter from IndexGateway-related funcs.

947611c

Fix default flag value and IndexQuerier type.

1004469

Remove additional mode field and reuse it from cfg.

f0a6b31

Remove redundant service init.

cccb298

Add sanity check for IndexGateway client constructor.

e824c21

Update GatewayClient to use dskit tenant package

ebf12c0

Signed-off-by: JordanRushing <[email protected]>

JordanRushing marked this pull request as ready for review March 30, 2022 18:39

JordanRushing requested a review from a team as a code owner March 30, 2022 18:39

JordanRushing changed the title ~~Begin to add a Ring to IndexGateway~~ Add a Ring to IndexGateway Mar 30, 2022

trevorwhitney reviewed Mar 30, 2022

View reviewed changes

sandeepsukhani approved these changes Apr 8, 2022

View reviewed changes

pkg/storage/stores/shipper/indexgateway/gateway.go Outdated Show resolved Hide resolved

pkg/storage/stores/shipper/gateway_client.go Show resolved Hide resolved

JordanRushing and others added 4 commits April 8, 2022 11:18

Add listenport configuration for IndexGateway and Ring

ccbf492

Signed-off-by: JordanRushing <[email protected]>

Randomize replication set access.

a5dda13

- If we don't randomize access, we'll always access same Index Gateway instances in same order for the same tenant

Merge branch 'main' of github.com:grafana/loki into add-ring-to-index…

bda00af

…-gateway

sandeepsukhani reviewed Apr 11, 2022

View reviewed changes

pkg/storage/stores/shipper/indexgateway/config.go Outdated Show resolved Hide resolved

sandeepsukhani reviewed Apr 11, 2022

View reviewed changes

pkg/loki/config_wrapper.go Outdated Show resolved Hide resolved

DylanGuedes added 6 commits April 11, 2022 10:08

Merge branch 'main' of github.com:grafana/loki into add-ring-to-index…

03c5a7c

…-gateway

Remove unwanted merge HEAD tags.

62dd62a

Move away from stores/chunk package.

2d602ae

Pass util_log in factory.

54d6a27

Change index gateway client ring to ignore replicas.

571b90e

- This is accomplished by using `NewIgnoreUnhealthyInstancesReplicationStrategy` - It is already done by the server

Refactor where the common replication factor is applied.

6fbd178

sandeepsukhani reviewed Apr 12, 2022

View reviewed changes

pkg/loki/config_wrapper.go Outdated Show resolved Hide resolved

sandeepsukhani reviewed Apr 12, 2022

View reviewed changes

pkg/loki/config_wrapper.go Outdated Show resolved Hide resolved

sandeepsukhani reviewed Apr 12, 2022

View reviewed changes

pkg/loki/config_wrapper.go Outdated Show resolved Hide resolved

Housekeeping config_wrapper IndexGateway configs.

b118248

- Remove unnecessary/wrong comments - Only set replication factor at a single place

periklis reviewed Apr 12, 2022

View reviewed changes

sandeepsukhani approved these changes Apr 13, 2022

View reviewed changes

sandeepsukhani merged commit 1dc0d97 into grafana:main Apr 13, 2022

sandeepsukhani mentioned this pull request Apr 13, 2022

Introduce a more scalable index-gateway API. #5892

Merged

3 tasks

This was referenced Apr 27, 2022

Update index gateway ring docs #6039

Closed

Update consistent hash ring docs with new index gateway ring #6041

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Ring to IndexGateway #5358

Add a Ring to IndexGateway #5358

JordanRushing commented Feb 9, 2022 •

edited

Loading

DylanGuedes left a comment

trevorwhitney left a comment

trevorwhitney Mar 30, 2022

DylanGuedes Apr 11, 2022

trevorwhitney Mar 30, 2022

sandeepsukhani left a comment

periklis left a comment

periklis Apr 12, 2022

DylanGuedes Apr 12, 2022

DylanGuedes Apr 18, 2022

sandeepsukhani left a comment

Add a Ring to IndexGateway #5358

Add a Ring to IndexGateway #5358

Conversation

JordanRushing commented Feb 9, 2022 • edited Loading

DylanGuedes left a comment

Choose a reason for hiding this comment

trevorwhitney left a comment

Choose a reason for hiding this comment

trevorwhitney Mar 30, 2022

Choose a reason for hiding this comment

DylanGuedes Apr 11, 2022

Choose a reason for hiding this comment

trevorwhitney Mar 30, 2022

Choose a reason for hiding this comment

sandeepsukhani left a comment

Choose a reason for hiding this comment

periklis left a comment

Choose a reason for hiding this comment

periklis Apr 12, 2022

Choose a reason for hiding this comment

DylanGuedes Apr 12, 2022

Choose a reason for hiding this comment

DylanGuedes Apr 18, 2022

Choose a reason for hiding this comment

sandeepsukhani left a comment

Choose a reason for hiding this comment

JordanRushing commented Feb 9, 2022 •

edited

Loading