Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move call of stopIncomingRequests before lifecycler shutdown. #2219

Merged
merged 2 commits into from
Mar 6, 2020
Merged

Move call of stopIncomingRequests before lifecycler shutdown. #2219

merged 2 commits into from
Mar 6, 2020

Conversation

pstibrany
Copy link
Contributor

Lifecycler was recently refactored (by #2166), which unfortunately breaks some expectations in Loki. This PR makes fixing Loki possible.

Before #2166, Lifecycler's Shutdown method called flushTransferer.StopIncomingRequests() and only then stopped lifecycler's loop.

After #2166, this has been modified. Lifecycler is now stopped via StopAsync method (through embedded BasicService), which cancels context used by loop() method, which makes loop() exit. stopping() method is called afterwards. This method still calls flushTransferer.StopIncomingRequests(), but by this time, loop has already finished.

In Cortex, this doesn't make much difference – only Ingester implements StopIncomingRequests method with non-empty body, and it simply sets the readonly flag. In Loki, StopIncomingRequests blocks until data transfer has finished, but it expects loop to be still running.

Solution to the issue is removing StopIncomingRequests from FlushTransferer interface – logically, it doesn't belong there anyway – and let Ingester call it explicitely before stopping the lifecycler. This reverts the method call order back to original.

Why doesn't StopIncomingRequests belong to FlushTransferer? All other FlushTransferer methods are called by lifecycler after main lifecycler loop has exited, during lifecycler's shutdown process. StopIncomingRequests was the only method that was called before triggering lifecycler's shutdown procedure.

(Fix in Loki is the same – call StopIncomingRequests from Ingester's shutdown method, before stopping the lifecycler. I have patch, incl. Cortex update ready.)

cc @rfratto

It worked like this before recent refactorings, although it was
called indirectly via lifecycler shutdown.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
pkg/ring/flush.go Outdated Show resolved Hide resolved
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@gouthamve gouthamve merged commit 64f66d8 into cortexproject:master Mar 6, 2020
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I'm joining the party late, but LGTM to me too 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants