Add systemd notify and watchdog support #2438
Conversation
ekohl
force-pushed
the
systemd-integration
branch
from
9e8c8c3
to
389ce58
Compare
nateberkopec
added
feature
systemd
waiting-for-review
That's fine. I appreciate the effort and desire to share the credit :) |
ekohl
force-pushed
the
systemd-integration
branch
from
9b5e8ba
to
ef3e75f
Compare
|
Added a |
|
Hmmm... any idea why it doesn't work with JRuby? It should in theory, right? |
|
It gave an error on bind with the unix socket, I'm not sure why. https://github.com/puma/puma/blob/master/docs/systemd.md#socket-activation does state:
Following the trail of issues it looks like this is still an issue on JRuby. |
lib/puma/systemd.rb
Outdated
| private | ||
|
|
||
| def watchdog_sleep_time | ||
| return unless SdNotify.watchdog? |
There was a problem hiding this comment.
I think it would improve the understanding here to move this up to just after the def start_watchdog line.
This takes the work by @acmh and improves on it. This is done by squashing all commits and rebasing it. Then the following changes were made: * Dropped SD_NOTIFY env var. There is aleady the NOTIFY_SOCKET env var presented by systemd and is redundant. * Move code is pushed in Puma::Systemd * on_reload now emits RELOADING=1 notification to systemd * Drop lower bound check on usec. Systemd can only be configured in seconds and it's hard to misconfigure. The actual code should be safe. * Clean up integration tests and skip on JRuby
ekohl
force-pushed
the
systemd-integration
branch
from
ef3e75f
to
f098486
Compare
|
LGTM but would appreciate review from others. We'll merge this in after 5.0.3 releases. |
|
Congratz @ekohl , thank you for the credit! Good work! |
|
Thanks for the initial work @acmh! It was a very solid base to improve on. |
|
I think i have a weird configuration issue using type=notify, everything starts up fine, serving request and then systemd kills at the WatchdogSec interval. I'll come back with better details after i poke around some more. Maybe i missed something with sd_notify |
| ping_f = watchdog_sleep_time | ||
|
|
||
| log "Pinging systemd watchdog every #{ping_f.round(1)} sec" | ||
| Thread.new do |
There was a problem hiding this comment.
Hmm... doesn't it defeat the purpose if you start the watchdog as a separate thread? How would that ever fail? It should either be integrated into the main event loop or at least somehow check whether the service is actually still working.
There was a problem hiding this comment.
I think that's a fair point but I wouldn't know how to do it better. Perhaps this is a good enough implementation for now and it allows for a better implementation in the future. Maybe the thread should perform a request to see if it's served? The question then becomes: which one? Should it be configurable?
Did you figure it out? Perhaps you should start without the watchdog to see if that does work. |
|
@rromanchuk I just ran into a similar issue with a different daemon... if your |
|
With Sidekiq I decided to vendor the code for the |
Description
This is a rebased version of #2260 with additional changes. I squashed all commits from the other PR into a single one to keep credits for @acmh while also making it easy to rebase. All my changes are on top of it.
It is more than 169 lines change, but splitting it up might be hard with credit. If desired, I can first submit the additional events as a separate patch.
Your checklist for this pull request
[changelog skip]or[ci skip]to the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.