Skip to content

Lifecycler errors don't make process exit immediately, but let other modules shutdown properly #2251

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Mar 11, 2020
Merged

Lifecycler errors don't make process exit immediately, but let other modules shutdown properly #2251

merged 11 commits into from
Mar 11, 2020

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Mar 11, 2020 •
edited
Loading

What this PR does: This PR does two things:

  • removes os.Exit calls from lifecycler. Lifecycler now enters Failed state, and reports problem via failure case.
  • components that use Lifecycler now watch it for failures, and if Lifecycler fails, these components enter Failed state as well. To do that, new ServiceFailureWatcher was introduced. It can watch either a service, or service manager.

Checklist

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

@pstibrany pstibrany changed the title Lifecycler errors Lifecycler errors don't make process exit immediately, but let other modules shutdown properly Mar 11, 2020
@pstibrany
Copy link
Contributor Author

cc @rfratto @cyriltovena – you may want to know about this. If merged, it will be post-0.7.

@pstibrany
Added ServiceFailureWatcher
a0e687b 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Lifecycler now returns erors instead of exiting the process.
b723b41 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Components using the lifecycler now fail when lifecycler fails.
edc6a73 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Don't run any cleanup if loop has failed.
0aa4378 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Include main component name in the error.
3ebebd0 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Updated CHANGELOG.md
b2d2db8 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Added trivial test, mostly to show that nil watcher returns nil channel.
9800794 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Changed error text to make lint happy
1954e55 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
pracucci
pracucci approved these changes Mar 11, 2020
View reviewed changes
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.

LGTM! I left few comments. Please take a look, but they're non blocking.

@pstibrany
Moved failure watcher to services package.
223bff1 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Renamed serviceWatcher to subservicesWatcher
6324d2f 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Renamed files
2ce7f7c 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Copy link
Contributor Author

Thanks for review, I accepted all your suggestions.

@pstibrany pstibrany merged commit 58c081a into cortexproject:master Mar 11, 2020
@pstibrany pstibrany deleted the lifecycler-errors branch May 8, 2020 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pstibrany @pracucci