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

Deprecate ingester.ring.final-sleep in favor of shutdown-delay #3431

Closed
wants to merge 1 commit into from

Conversation

56quarters
Copy link
Contributor

What this PR does

Deprecate ingester ring specific option for delaying shutdown after a SIGTERM. Instead use the shutdown-delay option which can be added to any component (it is not ingester or ring specific).

Signed-off-by: Nick Pillitteri nick.pillitteri@grafana.com

Which issue(s) this PR fixes or relates to

See #3298

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Deprecate ingester ring specific option for delaying shutdown after a
SIGTERM. Instead use the `shutdown-delay` option which can be added to
any component (it is not ingester or ring specific).

See #3298

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters force-pushed the 56quarters/deprecate-final-sleep branch from 9da0b4a to 2d36459 Compare November 10, 2022 20:20
@56quarters 56quarters marked this pull request as ready for review November 10, 2022 20:44
@56quarters 56quarters requested review from a team as code owners November 10, 2022 20:44
@@ -18,8 +18,6 @@ ingester_client:

ingester:
ring:
# We want to start immediately.
Copy link
Contributor

Choose a reason for hiding this comment

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

am I misunderstanding what final_sleep does or does this comment not make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the comment was just wrong. Maybe it used to apply to join_after?

@pstibrany
Copy link
Member

pstibrany commented Nov 11, 2022

Shutdown delay and "final sleep" serve different purpose. I don't think we should proceed with this PR.

Shutdown delay -- delays shutdown. It is implemented as "sleep" after SIGTERM is received, but before shutdown of Mimir starts. In other words shutdown delay delays initiation (!) of shutdown, not termination of process after shutdown actions have been performed.

Final sleep is applied after ring's lifecycler has finished with chunk transfer (not used by Mimir) and flushing of data to storage (optionally used by Mimir). This is done as part of shutdown actions (ie. after shutdown delay has expired). It is used to delay stopping of Mimir process so that we can still scrape final metrics from it.

@56quarters
Copy link
Contributor Author

That makes sense to me. Thank you for catching this @pstibrany!

@56quarters 56quarters closed this Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants