fixes #19480 - add systemd notify support #525
Conversation
domcleal
force-pushed
the
19480-systemd-notify
branch
3 times, most recently
from
abb9a72
to
d673f5f
Compare
extra/systemd/foreman-proxy.service
Outdated
| Type=notify | ||
| User=foreman-proxy | ||
| ExecStart=/usr/share/foreman-proxy/bin/smart-proxy --no-daemonize | ||
| EnvironmentFile=-/etc/sysconfig/foreman-proxy |
There was a problem hiding this comment.
This is RH specific. Debian has /etc/default but I'm wondering if the environment is even used. If users need it they can override it in /etc/systemd/system/foreman-proxy.service.d/override.conf.
There was a problem hiding this comment.
Good point, I had been thinking that packages for other OSes could patch the location, but preferring the systemd overrides seems a much better default. This can be added back in for RPM packages for their backwards-compatibility. Line removed.
domcleal
force-pushed
the
19480-systemd-notify
branch
from
d673f5f
to
48ca056
Compare
lib/proxy/sd_notify.rb
Outdated
| end | ||
|
|
||
| def notify(message) | ||
| socket.sendmsg(message.chomp + "\n") # ensure trailing \n |
There was a problem hiding this comment.
You aren't closing the socket when you are done with it. Or is this intentional so other changes in state (restarts, for example) can be passed to systemd?
There was a problem hiding this comment.
It was relying on the GC to finalise the object, which would autoclose it. I've changed it to explicitly close the socket as soon as it's done to free up the fd.
lib/smart_proxy_main.rb
Outdated
| @@ -43,6 +43,7 @@ | |||
|
|
|||
| module Proxy | |||
| SETTINGS = Settings.load_global_settings | |||
| SETTINGS.apply_argv(ARGV) | |||
There was a problem hiding this comment.
More of a nitpick: I think Settings::Global#apply_argv call could be made from Settings.load_global_settings, mostly to keep settings initialization logic in one spot. If so, Settings.load_global_settings should probably be renamed to something like Settings.initialize_global_settings...
There was a problem hiding this comment.
Thanks, updated as suggested.
test/sd_notify_test.rb
Outdated
|
|
||
| class SdNotifyTest < Test::Unit::TestCase | ||
| def test_active_with_notify_socket | ||
| with_notify_socket('/run/systemd/notify') do |
There was a problem hiding this comment.
Any reason you are using an actual systemd socket instead of a temporary one here?
There was a problem hiding this comment.
It's actually a mocked environment variable, just set to a real life path. I've changed it to /mock to make this clearer.
There was a problem hiding this comment.
Ahh, yes, Proxy::SdNotify.new.active? only checks that the environment variable isn't nil. The change made it clear(er) though, thanks.
When started under systemd, the launcher now logs via the notify socket API when all configured WEBrick servers have started so the service state is reported accurately (rather than the daemonization point). An example unit file is now shipped that uses a --no-daemonize argument to override the config file, so misconfiguration will not prevent the service from starting correctly.
domcleal
force-pushed
the
19480-systemd-notify
branch
from
48ca056
to
0723b1a
Compare
|
PR updated, thanks for the reviews. |
|
Merged, thanks @domcleal! |
When started under systemd, the launcher now logs via the notify socket
API when all configured WEBrick servers have started so the service
state is reported accurately (rather than the daemonization point).
An example unit file is now shipped that uses a --no-daemonize
argument to override the config file, so misconfiguration will not
prevent the service from starting correctly.