Add single compactor http client for delete and gennumber clients #7453

periklis · 2022-10-18T14:08:35Z

What this PR does / why we need it:
This PR completes work started with #7426 as there are two types of http clients for using the compactor retention API, one for the deletes and one for the generation numbers. However #7426 introduced by a hindsight on the existence of generation number client the boltdb.shipper.compactor.delete_client which is replaced in this PR with a single boltdb.shipper.compactor.client. Latter enables to set the TLS settings for the underlying http client when calling the compactor retention API.

Furthermore this change set includes also a fix for enabling the generation number middlewares only if the compactor retention API is enabled. Currently this produced non-blocking but confusing 404 errors like the following on each query and rule execution:

level=error ts=2022-10-18T14:13:41.598649348Z caller=delete_requests_client.go:211 msg="error getting delete requests from the store" err="unexpected status code: 404"
ts=2022-10-18T14:13:41.598697295Z caller=spanlogger.go:80 user=application level=error msg="failed loading deletes for user" err="unexpected status code: 404"

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Added backport label to fix the CLI flags from boltdb.shipper.compactor.delete_client to boltdb.shipper.compactor.client before we go with #7455 out or soon after. (cc @MichelHollands @trevorwhitney)

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

grafanabot · 2022-10-18T14:15:34Z

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

grafanabot · 2022-10-18T17:11: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.2%

trevorwhitney · 2022-10-18T17:57:23Z

@periklis thanks for adding the backport label, aiming on end of week for actually cutting 2.7 so should be plenty of time to get this in.

grafanabot · 2022-10-18T17:57:44Z

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

grafanabot · 2022-10-19T08:19:55Z

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

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

periklis · 2022-10-21T12:01:33Z

@trevorwhitney Reminder about this one

trevorwhitney

Looks good Peri! One question and one suggestion:

why did the simplification of the client require so much additional checking of whether or not retention was enabled that wasn't previously needed? My assumption here is that previously we were creating the client but just not using it when retention was not enabled, but I'm assuming something about this change makes that problematic?
how would you feel about moving the t.CfgCompactorConfig.RetentionEnabled check into t.supportIndexDeleteRequests, or are there cases we would support the index delete requests even if retention was not enabled?

trevorwhitney · 2022-10-25T21:24:23Z

pkg/loki/modules.go

@@ -395,7 +395,7 @@ func (t *Loki) initQuerier() (services.Service, error) {
 	toMerge := []middleware.Interface{
 		httpreq.ExtractQueryMetricsMiddleware(),
 	}
-	if t.supportIndexDeleteRequest() {
+	if t.supportIndexDeleteRequest() && t.Cfg.CompactorConfig.RetentionEnabled {


what's this additional check for, why wasn't it previously needed? You check it in combination with t.supportIndexDeleteRequests in multiple places so I'm curious if it should be moved into the t.supportIndexDeleteRequests function?

As mentioned above in the description this extra check is needed when the deletes-based retention is entirely disabled. This is the case by default IIRC and produces red-herring errors in the querier and ruler logs like this:

level=error ts=2022-10-18T14:13:41.598649348Z caller=delete_requests_client.go:211 msg="error getting delete requests from the store" err="unexpected status code: 404" ts=2022-10-18T14:13:41.598697295Z caller=spanlogger.go:80 user=application level=error msg="failed loading deletes for user" err="unexpected status code: 404"

I found moving it into the supportIndexDeleteRequests obscures the fact that this method is dedicated to check if the underlying index store supports deletes. Adding RetentionEnabled in there would make the check for retention hidden/implicit with the store capabilities. OTH as proposed in the implementation it is more explicit for the reader to know that we need an index store that supports deletes and explicitly opt-in to use the feature.

trevorwhitney · 2022-10-25T21:26:12Z

pkg/loki/modules.go

@@ -1160,7 +1167,7 @@ func (t *Loki) initUsageReport() (services.Service, error) {
 }

 func (t *Loki) deleteRequestsClient(clientType string, limits *validation.Overrides) (deletion.DeleteRequestsClient, error) {
-	if !t.supportIndexDeleteRequest() {
+	if !t.supportIndexDeleteRequest() || !t.Cfg.CompactorConfig.RetentionEnabled {


again, this seems to add to the argument this additional check should be moved into that function.

Same as above, it is more explicit for the reader that we need an index store that supports deletes and explicitly opt-in to use the feature.

trevorwhitney · 2022-10-25T21:35:44Z

pkg/querier/queryrange/queryrangebase/results_cache.go

@@ -214,7 +217,7 @@ func (s resultsCache) Do(ctx context.Context, r Request) (Response, error) {
 		return s.next.Do(ctx, r)
 	}

-	if s.cacheGenNumberLoader != nil {
+	if s.cacheGenNumberLoader != nil && s.retentionEnabled {


again I'm not quite sure why we are having to add this check now but it wasn't needed before? seems out of scope with adding TLS support to the client, so I'm curious what motivated it?

Although it seems out of scope, but on the other hand fixing TLS makes the 404s visible in the querier/ruler logs. I know if you run the entire system withough TLS you see the 404s anyway if retention is disabled. In any case both bits missing TLS support and 404s on disabled retentions are hindsights of the compactor client. IMHO the PR is just removing these hindsights and we may need to just re-title/change the changelog entry to be more informative in that direction. WDYT?

fixing TLS makes the 404s visible in the querier/ruler log

sounds like good enough justification to me! thanks for that added context.

trevorwhitney

Thanks for the added context. LGTM!

grafanabot · 2022-11-04T08:27:29Z

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

grafanabot · 2022-11-04T11:46:32Z

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-7453-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 e0a7b28a61e1ef83d62493c52ad1d2a51ba76902
# Push it to GitHub
git push --set-upstream origin backport-7453-to-release-2.7.x
git switch main
# Remove the local backport branch
git branch -D backport-7453-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-7453-to-release-2.7.x.

…afana#7453)

…afana#7453) (cherry picked from commit e0a7b28)

…afana#7453)

periklis self-assigned this Oct 18, 2022

periklis requested a review from a team as a code owner October 18, 2022 14:08

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

periklis mentioned this pull request Oct 18, 2022

operator: Add TLS support for compactor client #7448

Merged

5 tasks

periklis force-pushed the gennumber-client-tls branch from bb6f93e to 0f3d83d Compare October 18, 2022 17:06

pull-request-size bot added size/L and removed size/M labels Oct 18, 2022

periklis changed the title ~~WIP: Add tls settings for gennumber_client~~ WIP: Add single compactor http client for delete and gennumber clients Oct 18, 2022

periklis changed the title ~~WIP: Add single compactor http client for delete and gennumber clients~~ Add single compactor http client for delete and gennumber clients Oct 18, 2022

periklis added the backport release-2.7.x add to a PR to backport it into release 2.7.x label Oct 18, 2022

periklis force-pushed the gennumber-client-tls branch from 0f3d83d to 9132927 Compare October 18, 2022 17:52

periklis force-pushed the gennumber-client-tls branch from 9132927 to 54d7c88 Compare October 19, 2022 08:14

xperimental reviewed Oct 19, 2022

View reviewed changes

pkg/storage/stores/indexshipper/compactor/compactor_client.go Show resolved Hide resolved

trevorwhitney reviewed Oct 25, 2022

View reviewed changes

trevorwhitney approved these changes Nov 3, 2022

View reviewed changes

periklis mentioned this pull request Nov 4, 2022

add grpc support for delete client #7595

Open

Add single compactor http client for delete and gennumber clients

5348eb8

periklis force-pushed the gennumber-client-tls branch from 54d7c88 to 5348eb8 Compare November 4, 2022 08:22

periklis merged commit e0a7b28 into grafana:main Nov 4, 2022

periklis mentioned this pull request Nov 7, 2022

[release-2.7.x] Add single compactor http client for delete and gennumber clients #7607

Merged

5 tasks

lxwzy pushed a commit to lxwzy/loki that referenced this pull request Nov 7, 2022

Add single compactor http client for delete and gennumber clients (gr…

b91f15f

…afana#7453)

changhyuni pushed a commit to changhyuni/loki that referenced this pull request Nov 8, 2022

Add single compactor http client for delete and gennumber clients (gr…

5026b3d

…afana#7453)

periklis added a commit to periklis/loki that referenced this pull request Nov 30, 2022

Add single compactor http client for delete and gennumber clients (gr…

3aba37c

…afana#7453) (cherry picked from commit e0a7b28)

Abuelodelanada pushed a commit to canonical/loki that referenced this pull request Dec 1, 2022

Add single compactor http client for delete and gennumber clients (gr…

10a8d27

…afana#7453)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add single compactor http client for delete and gennumber clients #7453

Add single compactor http client for delete and gennumber clients #7453

periklis commented Oct 18, 2022 •

edited

Loading

grafanabot commented Oct 18, 2022

grafanabot commented Oct 18, 2022

trevorwhitney commented Oct 18, 2022

grafanabot commented Oct 18, 2022

grafanabot commented Oct 19, 2022

periklis commented Oct 21, 2022

trevorwhitney left a comment •

edited

Loading

trevorwhitney Oct 25, 2022

periklis Oct 26, 2022

trevorwhitney Oct 25, 2022

periklis Oct 26, 2022

trevorwhitney Oct 25, 2022

periklis Oct 26, 2022

trevorwhitney Nov 3, 2022

trevorwhitney left a comment

grafanabot commented Nov 4, 2022

grafanabot commented Nov 4, 2022

Add single compactor http client for delete and gennumber clients #7453

Add single compactor http client for delete and gennumber clients #7453

Conversation

periklis commented Oct 18, 2022 • edited Loading

grafanabot commented Oct 18, 2022

grafanabot commented Oct 18, 2022

trevorwhitney commented Oct 18, 2022

grafanabot commented Oct 18, 2022

grafanabot commented Oct 19, 2022

periklis commented Oct 21, 2022

trevorwhitney left a comment • edited Loading

Choose a reason for hiding this comment

trevorwhitney Oct 25, 2022

Choose a reason for hiding this comment

periklis Oct 26, 2022

Choose a reason for hiding this comment

trevorwhitney Oct 25, 2022

Choose a reason for hiding this comment

periklis Oct 26, 2022

Choose a reason for hiding this comment

trevorwhitney Oct 25, 2022

Choose a reason for hiding this comment

periklis Oct 26, 2022

Choose a reason for hiding this comment

trevorwhitney Nov 3, 2022

Choose a reason for hiding this comment

trevorwhitney left a comment

Choose a reason for hiding this comment

grafanabot commented Nov 4, 2022

grafanabot commented Nov 4, 2022

periklis commented Oct 18, 2022 •

edited

Loading

trevorwhitney left a comment •

edited

Loading