feature: Add ingester handler for shutdown and forget tokens #6179

chaudum · 2022-05-17T14:38:59Z

What this PR does / why we need it:

This handler can be used to gracefully shut down a Loki instance and
delete the file that persists the tokens of the ingester ring.

In production environments you usually want to persist ring tokens so
that during a restart of an ingester instance, or during rollout, the
tokens from that instance are not re-distributed to other instances, but
instead kept so that the same streams end up on the same instance once
it is up and running again. For that, the tokens are written to a file
that can be specified via the -ingester.tokens-file-path argument.

In certain cases, however, you want to forget the tokens and
re-distribute them when shutting down an ingester instance. This was
already possible by calling /ingester/flush_shutdown, deleting the
tokens file and terminating the instance. The new handler
/ingester/shutdown_and_forget combines these manual steps into a
single handler.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

The first commit with the vendor changes can be removed once the PR against grafana/dskit has been merged.

Checklist

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

grafanabot · 2022-05-17T15:40:47Z

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

grafanabot · 2022-05-18T18:27:30Z

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

grafanabot · 2022-05-19T08:22:41Z

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

sandeepsukhani

The changes look great. Just left some comments.

sandeepsukhani · 2022-05-19T09:23:52Z

docs/sources/api/_index.md

@@ -47,6 +47,7 @@ These endpoints are exposed by the ingester:

 - [`POST /flush`](#post-flush)
 - [`POST /ingester/flush_shutdown`](#post-ingesterflush_shutdown)
+- [`POST /ingester/shutdown_and_forget`](#post-ingestershutdown_and_forget)


shutdown_and_forget doesn't make it clear what it would forget. What do you think about calling it shutdown_and_forget_ring_tokens or shutdown_and_remove_ring_tokens?
We can also add a query param to flush_shutdown like /ingester/flush_shutdown?forget_ring_tokens=true.

On the one hand I like the idea of a single endpoint, but on the other hand, I find it weird to have query parameters in URLs that only allow POST requests.

It would make sense then to change the URL to /ingester/shutdown and make both flush and delete_tokens URL parameters.

/ingester/shutdown?flush=true /ingester/shutdown?delete_tokens=true

(Of course keep the existing URL and deprecate it.)

What do you think @sandeepsukhani ?

What about /ingester/shutdown/?unregister=true?

Keep in mind as well that documentation can clarify the purpose and mechanism for an API endpoint, if need be.

I find it weird to have query parameters in URLs that only allow POST requests

I agree it looks ugly but it helps us keep it tidy. If we keep adding different paths for various use cases, it would start getting messier.

I think delete_ring_tokens or reset_ring_tokens or something similar would be a bit self-descriptive.

If the endpoint only accepts POSTs, why not include these parameters in a request body? I do like the idea of a single endpoint with parameters.

sandeepsukhani · 2022-05-19T09:31:27Z

pkg/ingester/ingester.go

+	// ingester service as "failed", so Loki will shut down entirely.
+	// The module manager logs the failure `modules.ErrStopProcess` in a special way.
+	if i.lifecycler.ClearTokensOnShutdown() && errs.Err() == nil {
+		return modules.ErrStopProcess


Why can't we just rely on nil error? We can return internal server error from ShutdownAndForgetHandler handler when error is non-nil or am I missing something?

No, because we need to be able to distinguish between stopping of the ingester service that happened through the signal handler, and stopping of the service that was triggered by the shutdown handler. In case of the former, the error will be nil as well.

In both cases, we are returning w.WriteHeader(http.StatusNoContent) so I don't see any special handling in ShutdownAndForgetHandler for either of the cases. Am I missing something?

Ah I see what you mean, but the returned error modules.ErrStopProcess is not only used by the shutdown handler, but also by the service manager in the Run() function, which determines whether to stop Loki when one of its services failed.

loki/pkg/loki/loki.go

Line 385 in 3b3fcf6

level.Info(util_log.Logger).Log("msg", "received stop signal via return error", "module", m, "error", service.FailureCase())

pkg/storage/stores/series/series_index_store_test.go

grafanabot · 2022-05-30T11:55:58Z

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

chaudum · 2022-05-30T12:05:47Z

@sandeepsukhani @dannykopping I opted for /ingester/shutdown and restructured the handler a bit.

pkg/loki/modules.go

docs/sources/api/_index.md

pkg/ingester/ingester.go

grafanabot · 2022-06-01T09:20:13Z

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

grafanabot · 2022-06-02T11:50:57Z

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

chaudum · 2022-06-02T11:51:17Z

I'll squash commits once approved.

dannykopping · 2022-06-02T11:58:40Z

I'll squash commits once approved.

No need, we squash on merge

pkg/ingester/ingester.go

grafanabot · 2022-06-02T12:50:24Z

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

trevorwhitney

Just did a quick review, but looks good!

trevorwhitney · 2022-06-02T19:25:01Z

docs/sources/api/_index.md

+`/ingester/shutdown` is similar to the [`/ingester/flush_shutdown`](#post-ingesterflush_shutdown)
+endpoint, but accepts three URL query parameters `flush`, `delete_ring_tokens`, and `terminate`.
+
+**URL query parameters:**


since this is a POST, curious why we opt for url params intead of a request body?

Good point. Query params are I guess easier to write when you execute the request using curl.

Also, I think we do not follow any strict, restful HTTP API design guides in our endpoint design.

dannykopping

LGTM, nice work!

grafanabot · 2022-06-03T07:59: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.4%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-06-07T11:50:03Z

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

trevorwhitney

LGTM!

Signed-off-by: Christian Haudum <[email protected]>

This handler replaces the deprecated /ingester/flush_shutdown handler and can be used to gracefully shut down a Loki instance and delete the file that persists the tokens of the ingester ring. In production environments you usually want to persist ring tokens so that during a restart of an ingester instance, or during rollout, the tokens from that instance are not re-distributed to other instances, but instead kept so that the same streams end up on the same instance once it is up and running again. For that, the tokens are written to a file that can be specified via the `-ingester.tokens-file-path` argument. In certain cases, however, you want to forget the tokens and re-distribute them when shutting down an ingester instance. This was already possible by calling `/ingester/flush_shutdown`, deleting the tokens file and terminating the process. The new handler `/ingester/shutdown` combines these manual steps into a single handler. Signed-off-by: Christian Haudum <[email protected]>

Signed-off-by: Christian Haudum <[email protected]>

grafanabot · 2022-06-14T13:12:26Z

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

pull-request-size bot added the size/L label May 17, 2022

chaudum force-pushed the chaudum/shutdown-and-forget branch from e3bd8f0 to 5211bb5 Compare May 17, 2022 14:45

github-actions bot added the area/docs label May 17, 2022

chaudum mentioned this pull request May 18, 2022

Add functionality to clear tokens on lifecycler shutdown grafana/dskit#167

Merged

2 tasks

chaudum force-pushed the chaudum/shutdown-and-forget branch from cddb5c9 to cc4695e Compare May 18, 2022 18:22

github-actions bot added the area/docs label May 18, 2022

chaudum force-pushed the chaudum/shutdown-and-forget branch from cc4695e to 4016250 Compare May 19, 2022 08:17

chaudum force-pushed the chaudum/shutdown-and-forget branch from 4016250 to 18c75e2 Compare May 19, 2022 08:49

chaudum marked this pull request as ready for review May 19, 2022 09:09

chaudum requested review from KMiller-Grafana and a team as code owners May 19, 2022 09:09

sandeepsukhani reviewed May 19, 2022

View reviewed changes

chaudum force-pushed the chaudum/shutdown-and-forget branch from 18c75e2 to f2339b0 Compare May 30, 2022 11:50

pull-request-size bot added size/M and removed size/L labels May 30, 2022

chaudum requested review from slim-bean, dannykopping and sandeepsukhani June 1, 2022 07:52

dannykopping reviewed Jun 1, 2022

View reviewed changes

pkg/loki/modules.go Show resolved Hide resolved

docs/sources/api/_index.md Outdated Show resolved Hide resolved

pkg/ingester/ingester.go Outdated Show resolved Hide resolved

pkg/ingester/ingester.go Outdated Show resolved Hide resolved

pull-request-size bot added size/L and removed size/M labels Jun 1, 2022

chaudum force-pushed the chaudum/shutdown-and-forget branch from b836d0d to d4ff4d2 Compare June 2, 2022 11:45

chaudum requested a review from dannykopping June 2, 2022 11:50

dannykopping reviewed Jun 2, 2022

View reviewed changes

pkg/ingester/ingester.go Show resolved Hide resolved

trevorwhitney reviewed Jun 2, 2022

View reviewed changes

chaudum requested a review from dannykopping June 3, 2022 07:45

dannykopping approved these changes Jun 3, 2022

View reviewed changes

chaudum force-pushed the chaudum/shutdown-and-forget branch from 42b9b81 to 3cea8d7 Compare June 7, 2022 11:45

trevorwhitney approved these changes Jun 7, 2022

View reviewed changes

chaudum added area/docs and removed area/docs labels Jun 13, 2022

chaudum added 3 commits June 14, 2022 15:06

Update grafana/dskit to 07166f9

fee076f

Signed-off-by: Christian Haudum <[email protected]>

Add changelog entry

095653c

Signed-off-by: Christian Haudum <[email protected]>

chaudum force-pushed the chaudum/shutdown-and-forget branch from 3cea8d7 to 095653c Compare June 14, 2022 13:07

dannykopping merged commit c12a1f4 into main Jun 14, 2022

dannykopping deleted the chaudum/shutdown-and-forget branch June 14, 2022 13:44

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Add ingester handler for shutdown and forget tokens #6179

feature: Add ingester handler for shutdown and forget tokens #6179

chaudum commented May 17, 2022 •

edited

Loading

grafanabot commented May 17, 2022

grafanabot commented May 18, 2022

grafanabot commented May 19, 2022

sandeepsukhani left a comment

sandeepsukhani May 19, 2022

chaudum May 30, 2022

chaudum May 30, 2022

dannykopping May 30, 2022

sandeepsukhani May 30, 2022 •

edited

Loading

trevorwhitney Jun 2, 2022

sandeepsukhani May 19, 2022

chaudum May 30, 2022

sandeepsukhani May 30, 2022

chaudum May 30, 2022

grafanabot commented May 30, 2022

chaudum commented May 30, 2022

grafanabot commented Jun 1, 2022

grafanabot commented Jun 2, 2022

chaudum commented Jun 2, 2022

dannykopping commented Jun 2, 2022

grafanabot commented Jun 2, 2022

trevorwhitney left a comment

trevorwhitney Jun 2, 2022

chaudum Jun 3, 2022 •

edited

Loading

chaudum Jun 3, 2022

dannykopping left a comment

grafanabot commented Jun 3, 2022

grafanabot commented Jun 7, 2022

trevorwhitney left a comment

grafanabot commented Jun 14, 2022

feature: Add ingester handler for shutdown and forget tokens #6179

feature: Add ingester handler for shutdown and forget tokens #6179

Conversation

chaudum commented May 17, 2022 • edited Loading

grafanabot commented May 17, 2022

grafanabot commented May 18, 2022

grafanabot commented May 19, 2022

sandeepsukhani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandeepsukhani May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafanabot commented May 30, 2022

chaudum commented May 30, 2022

grafanabot commented Jun 1, 2022

grafanabot commented Jun 2, 2022

chaudum commented Jun 2, 2022

dannykopping commented Jun 2, 2022

grafanabot commented Jun 2, 2022

trevorwhitney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaudum Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

grafanabot commented Jun 3, 2022

grafanabot commented Jun 7, 2022

trevorwhitney left a comment

Choose a reason for hiding this comment

grafanabot commented Jun 14, 2022

chaudum commented May 17, 2022 •

edited

Loading

sandeepsukhani May 30, 2022 •

edited

Loading

chaudum Jun 3, 2022 •

edited

Loading