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

[phased-restart] add support for systemd type notify #2572

Closed
josef-krabath opened this issue Mar 10, 2021 · 10 comments · Fixed by #2591
Closed

[phased-restart] add support for systemd type notify #2572

josef-krabath opened this issue Mar 10, 2021 · 10 comments · Fixed by #2591

Comments

@josef-krabath
Copy link

josef-krabath commented Mar 10, 2021

The PR #2438 added support for systemd services of type notify.

# lib/puma/systemd.rb
module Puma
  class Systemd
    ...
    
    def hook_events
      @events.on_booted { SdNotify.ready }
      @events.on_stopped { SdNotify.stopping }
      @events.on_restart { SdNotify.reloading }
    end
    
    ...
  end
end

The Problem:
The systemd status gets updated as expected when performing a hot restart.
But the status stays active when performing a phased restart.
In the current implementation, SdNotify.reloading is never called during a phased restart.

A Common Scenario:
If we perform a phased restart, Puma terminates an old worker and starts a new one.
And if Puma is not able to boot up a new worker for some reason, it keeps trying.

In this case, the systemd status shouldn't stay active.
During a phased restart the status should be reloading.

Additional Context:
We monitor the status of all our systemd processes.
If the status of a process is not active any more (for a certain period of time), we get notified.

It would be awesome to keep this monitoring approach for Puma as well.

@MSP-Greg
Copy link
Member

@josefbilendo

To clarify, with a phased-restart, would you like on_restart to be called when Puma starts the restart process, and on_booted to be called when all workers are booted? Or maybe when the first worker is booted?

@josef-krabath
Copy link
Author

@MSP-Greg

In my opinion we should invoke on_restart when Puma starts the phased-restart
and call on_booted when all workers are booted up.

I think it makes sense to keep the "reloading" state till the phased-restart is complete – because that's a more accurate representation of the real state.

@MSP-Greg
Copy link
Member

@josefbilendo

I've got an 'almost finished' patch for this.

I'm wondering if it would be better to add another hook, maybe 'on_restart_*', where * is one of 'done', 'finished', or 'complete'?

@josef-krabath
Copy link
Author

@MSP-Greg
Thanks a lot!

I don't think there is anything against implementing another hook as long as we update the service state.

@MSP-Greg
Copy link
Member

@josefbilendo Thanks for the reply. Right now I've got it working as you mentioned, iow, both hot & phased restart (both cluster & single) are calling @events.on_booted when the workers have booted.

Not sure about adding another method name...

@nateberkopec Thoughts? The passing test file in the new system:
https://github.com/MSP-Greg/puma/blob/test-update/test/test_popen_systemd.rb

@nateberkopec
Copy link
Member

@MSP-Greg thoughts on adding a new method name/event type specifically, you mean?

@MSP-Greg
Copy link
Member

@nateberkopec

Yes. I don't really have an opinion, wasn't sure if there was a need...

@josef-krabath
Copy link
Author

@MSP-Greg

I've got an 'almost finished' patch for this.

How is your progress on this issue?
Can I help you somehow?

@MSP-Greg
Copy link
Member

MSP-Greg commented Apr 6, 2021

@josefbilendo

Sorry, the original work I did on this was tied into a bunch of other small changes...

Anyway, I'll push a PR today or tomorrow.

@josef-krabath
Copy link
Author

@MSP-Greg
Thanks a lot!!

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

Successfully merging a pull request may close this issue.

3 participants