Loki: Fix multitenant querying #7708

DylanGuedes · 2022-11-16T08:38:50Z

What this PR does / why we need it:
We recently broke multitenant querying because of recent changes to how timeouts working across Loki. This PR fixes this by:

Adapt the timeout wrapper to work with multitenant queries. It will take the shortest timeout across all given tenants
Adapt the query engine timeout assigning to work with multitenant queries. It will take the shortest timeout between all the tenants
Adapt query sharding to use smallest max query parallelism across given tenants
Add a functional test to ensure multitenant is behaving as expected

Signed-off-by: DylanGuedes [email protected]
Signed-off-by: Mehmet Burak Devecí [email protected]

Which issue(s) this PR fixes:
#7696

Special notes for your reviewer:
The regression was probably introduced by #7555

grafanabot · 2022-11-16T08:46:15Z

./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%
+        distributor	0%
+            querier	0.2%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

chaudum · 2022-11-16T15:11:16Z

@DylanGuedes can you check with #7703 so you don't do things twice? :)

DylanGuedes · 2022-11-21T15:10:41Z

@DylanGuedes can you check with #7703 so you don't do things twice? :)

Good call. I left a comment on his PR recommending that we proceed with this PR instead, since I'm adding a test that checks that this is really working and I noticed we also have to adapt things in other places.

grafanabot · 2022-11-21T15:22:06Z

./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%
+        distributor	0%
+            querier	0.2%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-11-21T15:38:07Z

./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%
+        distributor	0%
+            querier	0.2%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

JordanRushing · 2022-11-21T15:55:51Z

Does it make sense to opt for the lower of multiple limits rather than the higher? The former doesn't introduce unanticipated increases in resource consumption whereas the latter does if I understand correctly.

DylanGuedes · 2022-11-21T16:03:22Z

Does it make sense to opt for the lower of multiple limits rather than the higher? The former doesn't introduce unanticipated increases in resource consumption whereas the latter does if I understand correctly.

Good question. I feel like we can find arguments for both, but personally, I think going for the higher values is better because it avoids that scenario where a smaller tenant makes queries not operational for bigger tenants if they have limits that are smaller enough.

jeschkies

This is a really great find. Thanks for these awesome tests! However, we should always use the most restrictive limit. See #5626 for the original introduction.

pkg/logql/engine.go

jeschkies · 2022-11-21T16:02:35Z

pkg/querier/queryrange/querysharding.go

@@ -99,17 +99,25 @@ func (ast *astMapperware) Do(ctx context.Context, r queryrangebase.Request) (que
 		return ast.next.Do(ctx, r)
 	}

-	userID, err := tenant.TenantID(ctx)
+	// use biggest max query parallelism across all given tenants


Same here. We should take the most restrictive.

pkg/querier/http.go

JordanRushing · 2022-11-21T16:09:24Z

integration/multi_tenant_queries_test.go

+	"github.com/grafana/loki/integration/cluster"
+)
+
+func TestMultiTenantQuery(t *testing.T) {


JordanRushing

Approved pending additional dialogue about picking the higher vs. lower limit for a multi-tenant query.

DylanGuedes · 2022-11-21T16:17:47Z

This is a really great find. Thanks for these awesome tests! However, we should always use the most restrictive limit. See #5626 for the original introduction.

Thanks, I hope those will help! Regarding the limits: I'm not strongly opinionated for proceeding with the bigger values, but personally, I think they work better here. That said, I'll wait a little more for others to share opinions before reverting the change etc.

grafanabot · 2022-11-21T16:40:48Z

./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.1%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

DylanGuedes · 2022-11-23T12:39:31Z

@jeschkies @JordanRushing I pushed a commit using the smallest limits instead, WDYT?

grafanabot · 2022-11-23T12:42:55Z

./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%
+        distributor	0%
+            querier	0.1%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-11-23T12:56:40Z

./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.1%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

jeschkies

See my comment. I am happy to take over the PR if you give me right access to your repository.

grafanabot · 2022-11-23T14:40: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%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

DylanGuedes · 2022-11-23T15:08:55Z

See my comment. I am happy to take over the PR if you give me right access to your repository.

I pushed a commit addressing it, WDYT?

jeschkies

🎉

grafanabot · 2022-11-24T12:14:28Z

Hello @DylanGuedes!
Backport pull requests need to be either:

Pull requests which address bugs,
Urgent fixes which need product approval, in order to get merged,
Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

**What this PR does / why we need it**: We recently broke multitenant querying because of recent changes to how timeouts working across Loki. This PR fixes this by: - Adapt the timeout wrapper to work with multitenant queries. It will take the shortest timeout across all given tenants - Adapt the query engine timeout assigning to work with multitenant queries. It will take the shortest timeout between all the tenants - Adapt query sharding to use smallest max query parallelism across given tenants - Add a functional test to ensure multitenant is behaving as expected Signed-off-by: DylanGuedes <[email protected]> Signed-off-by: Mehmet Burak Devecí <[email protected]> **Which issue(s) this PR fixes**: grafana#7696 **Special notes for your reviewer**: The regression was probably introduced by grafana#7555

grafanabot · 2023-01-25T14:23:52Z

The backport to release-2.7.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-7708-to-release-2.7.x origin/release-2.7.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x ad2260aec2d4d587c50a3dfb68d2c89ed2bf3157
# Push it to GitHub
git push --set-upstream origin backport-7708-to-release-2.7.x
git switch main
# Remove the local backport branch
git branch -D backport-7708-to-release-2.7.x

Then, create a pull request where the base branch is release-2.7.x and the compare/head branch is backport-7708-to-release-2.7.x.

**What this PR does / why we need it**: We recently broke multitenant querying because of recent changes to how timeouts working across Loki. This PR fixes this by: - Adapt the timeout wrapper to work with multitenant queries. It will take the shortest timeout across all given tenants - Adapt the query engine timeout assigning to work with multitenant queries. It will take the shortest timeout between all the tenants - Adapt query sharding to use smallest max query parallelism across given tenants - Add a functional test to ensure multitenant is behaving as expected Signed-off-by: DylanGuedes <[email protected]> Signed-off-by: Mehmet Burak Devecí <[email protected]> **Which issue(s) this PR fixes**: #7696 **Special notes for your reviewer**: The regression was probably introduced by #7555

Co-authored-by: Dylan Guedes <[email protected]> Co-authored-by: Owen Diehl <[email protected]>

pull-request-size bot added the size/S label Nov 16, 2022

DylanGuedes mentioned this pull request Nov 21, 2022

Loki: Use default limits when cant get the tenantId #7703

Closed

5 tasks

pull-request-size bot added size/M and removed size/S labels Nov 21, 2022

DylanGuedes marked this pull request as ready for review November 21, 2022 15:10

DylanGuedes requested a review from a team as a code owner November 21, 2022 15:10

DylanGuedes added the backport release-2.7.x add to a PR to backport it into release 2.7.x label Nov 21, 2022

DylanGuedes changed the title ~~Loki: Use longest timeout across all tenants~~ Loki: Fix multitenant querying Nov 21, 2022

DylanGuedes added 3 commits November 21, 2022 12:16

Use longest timeout across all tenants.

09c71b9

Implement multitenant check tests.

b0a4966

Add missing CHANGELOG entry.

ce855f1

DylanGuedes force-pushed the fix-multitenant-query branch from 84d26a0 to ce855f1 Compare November 21, 2022 15:16

Add test that checks multitenant behavior.

79e7398

pull-request-size bot added size/L and removed size/M labels Nov 21, 2022

jeschkies self-requested a review November 21, 2022 15:48

jeschkies requested changes Nov 21, 2022

View reviewed changes

JordanRushing reviewed Nov 21, 2022

View reviewed changes

pkg/querier/http.go Outdated Show resolved Hide resolved

JordanRushing reviewed Nov 21, 2022

View reviewed changes

pkg/querier/http.go Outdated Show resolved Hide resolved

JordanRushing reviewed Nov 21, 2022

View reviewed changes

JordanRushing approved these changes Nov 21, 2022

View reviewed changes

Simplify timeout assigning.

ba2529d

DylanGuedes added 2 commits November 21, 2022 13:27

Use tenants instead of tenantIDS for consistency.

c64a892

Join tenants string when logging message.

4b77032

Use smallest/shortest limits instead.

d381fbd

jeschkies requested changes Nov 23, 2022

View reviewed changes

Use validation.SmallestValue* functions instead.

cd034e3

jeschkies approved these changes Nov 24, 2022

View reviewed changes

DylanGuedes merged commit ad2260a into grafana:main Nov 24, 2022

chaudum added type/bug Somehing is not working as expected backport release-2.7.x add to a PR to backport it into release 2.7.x and removed backport release-2.7.x add to a PR to backport it into release 2.7.x labels Jan 25, 2023

grafanabot added the backport-failed label Jan 25, 2023

chaudum mentioned this pull request Jan 25, 2023

Backport PRs #7555 and #7708 to 2.7 release branch #8280

Merged

chaudum added a commit that referenced this pull request Jan 25, 2023

Backport PRs #7555 and #7708 to 2.7 release branch (#8280)

b18aee4

Co-authored-by: Dylan Guedes <[email protected]> Co-authored-by: Owen Diehl <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki: Fix multitenant querying #7708

Loki: Fix multitenant querying #7708

DylanGuedes commented Nov 16, 2022 •

edited

Loading

grafanabot commented Nov 16, 2022

chaudum commented Nov 16, 2022

DylanGuedes commented Nov 21, 2022

grafanabot commented Nov 21, 2022

grafanabot commented Nov 21, 2022

JordanRushing commented Nov 21, 2022 •

edited

Loading

DylanGuedes commented Nov 21, 2022

jeschkies left a comment •

edited

Loading

jeschkies Nov 21, 2022

JordanRushing Nov 21, 2022

JordanRushing left a comment

DylanGuedes commented Nov 21, 2022

grafanabot commented Nov 21, 2022

DylanGuedes commented Nov 23, 2022

grafanabot commented Nov 23, 2022

grafanabot commented Nov 23, 2022

jeschkies left a comment

grafanabot commented Nov 23, 2022

DylanGuedes commented Nov 23, 2022

jeschkies left a comment

grafanabot commented Nov 24, 2022

grafanabot commented Jan 25, 2023

Loki: Fix multitenant querying #7708

Loki: Fix multitenant querying #7708

Conversation

DylanGuedes commented Nov 16, 2022 • edited Loading

grafanabot commented Nov 16, 2022

chaudum commented Nov 16, 2022

DylanGuedes commented Nov 21, 2022

grafanabot commented Nov 21, 2022

grafanabot commented Nov 21, 2022

JordanRushing commented Nov 21, 2022 • edited Loading

DylanGuedes commented Nov 21, 2022

jeschkies left a comment • edited Loading

Choose a reason for hiding this comment

jeschkies Nov 21, 2022

Choose a reason for hiding this comment

JordanRushing Nov 21, 2022

Choose a reason for hiding this comment

JordanRushing left a comment

Choose a reason for hiding this comment

DylanGuedes commented Nov 21, 2022

grafanabot commented Nov 21, 2022

DylanGuedes commented Nov 23, 2022

grafanabot commented Nov 23, 2022

grafanabot commented Nov 23, 2022

jeschkies left a comment

Choose a reason for hiding this comment

grafanabot commented Nov 23, 2022

DylanGuedes commented Nov 23, 2022

jeschkies left a comment

Choose a reason for hiding this comment

grafanabot commented Nov 24, 2022

grafanabot commented Jan 25, 2023

DylanGuedes commented Nov 16, 2022 •

edited

Loading

JordanRushing commented Nov 21, 2022 •

edited

Loading

jeschkies left a comment •

edited

Loading