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

nghttpx: add systemd support #802

Merged
merged 1 commit into from Feb 10, 2017

Conversation

Projects
None yet
2 participants
@zdzichu
Contributor

zdzichu commented Feb 8, 2017

Add systemd's Type=notify support by sending information about master process PID around forks. Add some hardening option to service unit.

This improves systemd tracking of main process by explicitly stating which PID is master process. No need to use --pid-file.
daemon() usage is disabled when run under systemd. It make little sense in that context, but more importanly daemon() double forks, leaving us with no time to send a message to systemd.
Please note, even if nghttpx was compiled with systemd support, the resulting binary still can be run under other init systems. sd_notifyf() is no-op if NOTIFY_SOCKET env variable is not set.

I've introduced wrapper for sd_notifyf() function in order to reduce amount of #ifdef's.

There is one harmless message from systemd during forking (systemd[1]: ng2.service: Supervising process 9267 which is not our child. We'll most likely not notice when it exits.). It doesn't really matter.

@tatsuhiro-t

Thank you for PR. I made a couple of comments. Please check them out.

Show outdated Hide outdated contrib/nghttpx.service.in
ExecStart=@bindir@/nghttpx --conf=/etc/nghttpx/nghttpx.conf --pid-file=/run/nghttpx.pid --daemon
Type=notify
ExecStart=@bindir@/nghttpx --conf=/etc/nghttpx/nghttpx.conf
ExecReload=/bin/kill --signal USR2 $MAINPID

This comment has been minimized.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Usually, reload means "force reloading configuration". If yes, SIGHUP is better choice. Actually, SIGUSR2 does not cause nghttpx to shut down itself.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Usually, reload means "force reloading configuration". If yes, SIGHUP is better choice. Actually, SIGUSR2 does not cause nghttpx to shut down itself.

This comment has been minimized.

@zdzichu

zdzichu Feb 9, 2017

Contributor

I will change it to SIGHUP.

@zdzichu

zdzichu Feb 9, 2017

Contributor

I will change it to SIGHUP.

Show outdated Hide outdated contrib/nghttpx.service.in
[Install]
WantedBy=multi-user.target

This comment has been minimized.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Please remove this line

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Please remove this line

Show outdated Hide outdated src/shrpx.cc
@@ -56,6 +56,9 @@
#include <sys/time.h>
#endif // HAVE_SYS_TIME_H
#include <sys/resource.h>
#ifdef HAVE_LIBSYSTEMD
#include <systemd/sd-daemon.h>
#endif

This comment has been minimized.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Would be nice to add

#endif // HAVE_LIBSYSTEMD
@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Would be nice to add

#endif // HAVE_LIBSYSTEMD
Show outdated Hide outdated src/shrpx.cc
LOG(NOTICE) << "Daemonising disabled under systemd";
chdir("/");
return 0;
}

This comment has been minimized.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

This actually removes the opportunity from users to run nghttpx in daemon mode without systemd.

Perhaps, it might be nicer to have new option like --systemd option, and only do systemd related stuff, including chdir("/") and sending message to systemd with this option turned on. I'm not totally sure that sd_notifyf is really noop if nghttpx is not started by systemd.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

This actually removes the opportunity from users to run nghttpx in daemon mode without systemd.

Perhaps, it might be nicer to have new option like --systemd option, and only do systemd related stuff, including chdir("/") and sending message to systemd with this option turned on. I'm not totally sure that sd_notifyf is really noop if nghttpx is not started by systemd.

This comment has been minimized.

@zdzichu

zdzichu Feb 9, 2017

Contributor

If not started by systemd, $NOTIFY_SOCKET is not set, so sd_notifyf() returns without doing anything.
If not booted with systemd, function sd_booted() in line 1115 returns false and daemon() is called.

@zdzichu

zdzichu Feb 9, 2017

Contributor

If not started by systemd, $NOTIFY_SOCKET is not set, so sd_notifyf() returns without doing anything.
If not booted with systemd, function sd_booted() in line 1115 returns false and daemon() is called.

This comment has been minimized.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

I see. Thank you. This hunk looks good.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

I see. Thank you. This hunk looks good.

Show outdated Hide outdated src/shrpx.cc
@@ -1245,6 +1275,9 @@ int event_loop() {
redirect_stderr_to_errorlog();
}
// update systemd PID tracking
shrpx_sd_notifyf(0, "MAINPID=%s\n", util::utos(config->pid).c_str());

This comment has been minimized.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Perhaps, "MAINPID=%d\n" ?

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Perhaps, "MAINPID=%d\n" ?

Show outdated Hide outdated src/shrpx.cc
@@ -386,6 +403,9 @@ void exec_binary() {
if (pid == -1) {
auto error = errno;
LOG(ERROR) << "fork() failed errno=" << error;
} else {
// update PID tracking information in systemd
shrpx_sd_notifyf(0, "MAINPID=%s\n", util::utos(pid).c_str());

This comment has been minimized.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Perhaps, "MAINPID=%d\n" ?

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Perhaps, "MAINPID=%d\n" ?

This comment has been minimized.

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Is this required since we eventually do that in the new master process (line1279)?

@tatsuhiro-t

tatsuhiro-t Feb 9, 2017

Collaborator

Is this required since we eventually do that in the new master process (line1279)?

This comment has been minimized.

@zdzichu

zdzichu Feb 9, 2017

Contributor

Yes it is required here. systemd only accepts sd_notify() messages from main PID of the service (this could be relaxed in configuration, but it is more secure that way). Thus, in this place we are still a main PID, but we started new master process and we are basically telling systemd "hey, this is new master process of this service; please listen for notification from this new PID).

We cannot rely on message in line 1279, because it would be ignored by systemd - it would come from (new master) PID, which is not known for systemd as master PID.
Line 1279 works during normal startup because then we are only process in a service, thus we are a MAINPID.

I hope that is clear.

@zdzichu

zdzichu Feb 9, 2017

Contributor

Yes it is required here. systemd only accepts sd_notify() messages from main PID of the service (this could be relaxed in configuration, but it is more secure that way). Thus, in this place we are still a main PID, but we started new master process and we are basically telling systemd "hey, this is new master process of this service; please listen for notification from this new PID).

We cannot rely on message in line 1279, because it would be ignored by systemd - it would come from (new master) PID, which is not known for systemd as master PID.
Line 1279 works during normal startup because then we are only process in a service, thus we are a MAINPID.

I hope that is clear.

This comment has been minimized.

@tatsuhiro-t

tatsuhiro-t Feb 10, 2017

Collaborator

Thank you for clarification.
One thing that worries me is that if new process exits because of configuration error, then systemd gets non existent PID, and still old process continues to exist. systemd may respawn nghttpx process if it finds the managed process is gone, but because old process has still hold ports, new process cannot be started.
For this PR, it is OK for me since we use SIGHUP for reloading with systemd, and hot swapping should still be done manually.

@tatsuhiro-t

tatsuhiro-t Feb 10, 2017

Collaborator

Thank you for clarification.
One thing that worries me is that if new process exits because of configuration error, then systemd gets non existent PID, and still old process continues to exist. systemd may respawn nghttpx process if it finds the managed process is gone, but because old process has still hold ports, new process cannot be started.
For this PR, it is OK for me since we use SIGHUP for reloading with systemd, and hot swapping should still be done manually.

nghttpx: add systemd support
  Add systemd's Type=notify support by sending information about
 master process PID around forks.
  Add some hardening option to service unit.
@zdzichu

This comment has been minimized.

Show comment
Hide comment
@zdzichu

zdzichu Feb 9, 2017

Contributor

Thank you for your review! I updated the patchset according to your remarks.

I've also changed the service unit: KillSignal=SIGQUIT is better thanExecStop=/bin/kill…

Also, around daemon() call, I've added check for systemd API ("NOTIFY_SOCKET"). This way even if nghttpx is run in systemd-managed system, but not from an unit - like for example from shell - it will behave properly.

Contributor

zdzichu commented Feb 9, 2017

Thank you for your review! I updated the patchset according to your remarks.

I've also changed the service unit: KillSignal=SIGQUIT is better thanExecStop=/bin/kill…

Also, around daemon() call, I've added check for systemd API ("NOTIFY_SOCKET"). This way even if nghttpx is run in systemd-managed system, but not from an unit - like for example from shell - it will behave properly.

@tatsuhiro-t

This comment has been minimized.

Show comment
Hide comment
@tatsuhiro-t

tatsuhiro-t Feb 10, 2017

Collaborator

Thank you. PR looks good. Will merge soon.

Collaborator

tatsuhiro-t commented Feb 10, 2017

Thank you. PR looks good. Will merge soon.

@tatsuhiro-t tatsuhiro-t added this to the v1.20.0 milestone Feb 10, 2017

@tatsuhiro-t tatsuhiro-t merged commit 9d2503f into nghttp2:master Feb 10, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@c0bw3b c0bw3b referenced this pull request Nov 25, 2017

Open

nghttp2: 1.24.0 -> 1.28.0 + split lib and apps #32040

4 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment