Adds a new entity indexgateway.RingManager, responsible for taking care of the index gateway ring for both, index gateway clients and servers. Now, the IndexGatewayRing service is basically a wrapper for the indexgateway.RingManager.
Add the IndexGatewayRing service as a dependency for the IndexGateway
Adds a new OwnsTenant function as a parameter to the shipper. This function replies to whether a tenant is assigned to some instance or not.
Modifies the TableManager to only download data from tenants assigned to it through the new OwnsTenant function.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:
This depends on PR#5946.

Checklist

Documentation added
Tests updated
Is this an important fix or new feature? Add an entry in theCHANGELOG.md.
Changes that require user attention or interaction to upgrade are documented indocs/sources/upgrading/_index.md

DylanGuedes added4 commits

April 18, 2022 10:27

Add two metrics to the IndexGateway.

0f90a99

- Add a new `query_readiness_duration_seconds` metric, that reports  query readiness duration of a tablemanager/index gateway instance. We  should use it later to report performance against the ring mode- Add a new `usersToBeQueryReadyForTotal` metric, that reports number of  users involved in the query readiness operation. We should use it  later to correlate number of users with the query readiness duration.

RemoveusersToBeQueryReadyForTotal.

63321f9

- It will report all users always for now, so it isn't too helpful the  way it is.

Rename metric help text to not mislead people.

cbbd4b6

Log queryReadiness duration.

f9c520a

pull-request-sizebot added the size/L label

Apr 20, 2022

DylanGuedes force-pushed thetable-manager-using-ring branch 2 times, most recently from7193545 to5551b48Compare

April 20, 2022 18:42

DylanGuedes marked this pull request as ready for review

April 20, 2022 21:26

DylanGuedes requested a review froma team as acode owner

April 20, 2022 21:26

DylanGuedes requested review fromcyriltovena andsandeepsukhani

April 22, 2022 13:27

DylanGuedes requested a review fromKMiller-Grafana as acode owner

April 22, 2022 16:43

JordanRushing reviewed

Apr 22, 2022

View reviewed changes

pkg/storage/stores/shipper/indexgateway/gateway.go OutdatedShow resolvedHide resolved

JordanRushing approved these changes

Apr 22, 2022

View reviewed changes

Copy link

Contributor

JordanRushing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM!

Fix where log message and duration and triggered.

b4737dc

DylanGuedes force-pushed thetable-manager-using-ring branch fromab4ea75 to57340b6Compare

April 25, 2022 15:27

pull-request-sizebot added size/XXL and removed size/L labels

Apr 25, 2022

DylanGuedesand others added10 commits

April 25, 2022 12:28

Add two metrics to the IndexGateway.

dca92fe

- Add a new `query_readiness_duration_seconds` metric, that reports  query readiness duration of a tablemanager/index gateway instance. We  should use it later to report performance against the ring mode- Add a new `usersToBeQueryReadyForTotal` metric, that reports number of  users involved in the query readiness operation. We should use it  later to correlate number of users with the query readiness duration.

RemoveusersToBeQueryReadyForTotal.

02da788

- It will report all users always for now, so it isn't too helpful the  way it is.

Rename metric help text to not mislead people.

0be5dcd

Log queryReadiness duration.

860f855

Fix where log message and duration and triggered.

7bac294

Use boundaries to skip users in TableManager.

6632319

- Separate the assigning of indexClient in the IndexGateway to allow  initializing the shipper with the IndexGateway ring- Add new TenantBoundariesClient entity, that answers if a given  tenantID should be ignored or not- Use the TenantBoundariesClient implemented by the IndexGateway in the  downloads TableManager

Add IndexGateway configuration docs.

c1a83ac

Add tests for pkg/util/ring TokenFor() and IsAssignedKey()

db60fdb

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Small whitespace fix in pkg/util/ring_test.go

e9f9b28

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Apply suggestions from code review

44e249c

Rewrite QueryIndex error phrase.Co-authored-by: JordanRushing <rushing.jordan@gmail.com>

DylanGuedes force-pushed thetable-manager-using-ring branch from57340b6 to44e249cCompare

April 25, 2022 15:29

pull-request-sizebot removed the size/XXL label

Apr 25, 2022

Copy link

Contributor

grafanabot commentedMay 6, 2022

./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.

+           ingester0%+        distributor0%+            querier0%+ querier/queryrange0%+               iter0%+            storage0%+           chunkenc0%+              logql0%-               loki-4.2%

Copy link

Contributor

grafanabot commentedMay 6, 2022

./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.

+           ingester0%+        distributor0.4%+            querier0%+ querier/queryrange0%+               iter0%+            storage0%+           chunkenc0%+              logql0%+               loki0%

Log len of users instead of all of them to avoid log msgs.

f747d82

Copy link

Contributor

grafanabot commentedMay 6, 2022

./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.

+           ingester0%+        distributor0%+            querier0%+ querier/queryrange0%+               iter0%+            storage0%+           chunkenc0%+              logql0%+               loki0%

Merge branch 'main' of github.com:grafana/loki into table-manager-usi…

1ec30f0

…ng-ring

Copy link

Contributor

grafanabot commentedMay 18, 2022

./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.

+           ingester0%+        distributor0%+            querier0%+ querier/queryrange0%+               iter0%+            storage0%+           chunkenc0%+              logql0%+               loki0%

Add docstrings to new public functions.

7d90edd

Copy link

Contributor

grafanabot commentedMay 18, 2022

./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.

+           ingester0%+        distributor0.3%+            querier0%+ querier/queryrange0%+               iter0%+            storage0%+           chunkenc0%+              logql0%+               loki0%

sandeepsukhani requested changes

May 19, 2022

View reviewed changes

Copy link

Contributor

sandeepsukhani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I have added few minor nits but overall code looks great to me.

pkg/loki/modules.go OutdatedShow resolvedHide resolved

pkg/storage/stores/shipper/downloads/table_manager.go OutdatedShow resolvedHide resolved

pkg/storage/stores/shipper/indexgateway/ringmanager.go OutdatedShow resolvedHide resolved

pkg/storage/stores/shipper/gateway_client.go OutdatedShow resolvedHide resolved

pkg/util/ring.go Outdated

		bufDescs,bufHosts,bufZones:=ring.MakeBuffersForGet()
		token:=TokenFor(key,""/* labels */)
		rs,err:=ringClient.Get(token,ring.WriteNoExtend,bufDescs,bufHosts,bufZones)
		iferr==nil {

Copy link

Contributor

sandeepsukhaniMay 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Let us log the error if not nil

pkg/util/ring.go Outdated

		}

		// HasDedicatedAddress specify an instance support for replying its own address.
		typeHasDedicatedAddressinterface {

Copy link

Contributor

sandeepsukhaniMay 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Naming is hard :)
It doesn't sound correct to me. What do you think about usingRingInstanceInfoReader or simply going back to previous implementation to pass the address directly?

Copy link

ContributorAuthor

DylanGuedesMay 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hahaha it is!

Agreed, it looks odd. I returned to pass the address directly, lmk what you think.

Apply suggestions from code review

3500c5a

Document that tenant filtering is only applied during query readiness.Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

Copy link

Contributor

grafanabot commentedMay 19, 2022

./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.

+           ingester0%+        distributor0%+            querier0%+ querier/queryrange0%+               iter0%+            storage0%+           chunkenc0%+              logql0%+               loki0%

Modify IsAssignedKey to expect address directly instead of lifecycler.

a55bdc9

- Also removes HasDedicatedAddress, since it isn't necessary anymore

Copy link

Contributor

grafanabot commentedMay 19, 2022

./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.

+           ingester0%+        distributor0%+            querier0%+ querier/queryrange0%+               iter0%+            storage0%+           chunkenc0%+              logql0%+               loki0%

DylanGuedes added5 commits

May 19, 2022 08:40

Log error message when IsInReplicationSet fails.

a6a2b47

Modify IsAssignedKey to return true by default.

da4c7ee

Remove wrong assigning of Service for clientMode.

d4207a7

Log gateway requests before error checks.

3d32ed2

Remove unnecessary endpoint registration.

103c977

sandeepsukhani approved these changes

May 19, 2022

View reviewed changes

Copy link

Contributor

sandeepsukhani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

just a minor nit, otherwise it LGTM

pkg/util/ring.go OutdatedShow resolvedHide resolved

Fix lint.

69f19d1

Copy link

Contributor

grafanabot commentedMay 19, 2022

./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.

+           ingester0%+        distributor0%+            querier0%+ querier/queryrange0%+               iter0%+            storage0%+           chunkenc0%+              logql0%+               loki0.1%

Update pkg/util/ring.go

bbbcd5e

Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

Copy link

Contributor

grafanabot commentedMay 19, 2022

./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.

+           ingester0%-        distributor-0.3%+            querier0%+ querier/queryrange0%+               iter0%+            storage0%+           chunkenc0%+              logql0%+               loki0.1%

sandeepsukhani approved these changes

May 20, 2022

View reviewed changes

Copy link

Contributor

sandeepsukhani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM! Thanks for patiently taking care of all the feedback!

sandeepsukhani merged commit440fb7d intografana:main

May 20, 2022

chaudum removed the type/docs label

Jun 13, 2022

This was referencedJun 28, 2022

dannykopping/remove cache statsdannykopping/loki#13

Closed

dannykopping/remove cache statsdannykopping/loki#14

Draft

osg-grafana added type/docsIssues related to technical documentation; the Docs Squad uses this label across many repositories and removed area/docs labels

Oct 19, 2022