#47951 Add PIDFile option to systemd service file
Closed: Fixed None Opened 4 years ago by pviktori.

When stopping a dirsrv service, systemd currently guesses the PID to wait on. To make it explicit, a line like this should be added to /usr/lib/systemd/system/dirsrv@.service:

PIDFile=/var/run/dirsrv/slapd-%i.pid

I think this may be fixed with https://fedorahosted.org/389/ticket/47977 as this advertises the correct pid file to systemd via the notification, and notifies of the process status with startup / shutdown.

"""
If set to forking, it is expected that the process configured with ExecStart= will call fork() as part of its start-up. The parent process is expected to exit when start-up is complete and all communication channels are set up. The child continues to run as the main daemon process. This is the behavior of traditional UNIX daemons. If this setting is used, it is recommended to also use the PIDFile= option, so that systemd can identify the main process of the daemon. systemd will proceed with starting follow-up units as soon as the parent process exits.
"""

If we start to use type=notify, this is not needed. PIDFile only affects type=forking.

So we need to decide if there is value in allowing users to "choose" between forking or notify, or if we want to support a single option under systemd. (Of course, we still support forking for everything besides systemd).

As I sent out a request for Ticket 47977, could you make the system work with as well as without WITH_SYSTEMD?

Most of your commit for this fix in the master branch is in "#ifdef WITH_SYSTEMD" except systemd.template.service.in. If we don't configure with --with-systemd, the setup-ds.pl does not start the server, which is not good. I think we should support both with or without cases... Could it be possible to prepare 2 systemd.template.service.in files, one with Type=forking and another with Type=notify. And if WITH_SYSTEMD is enabled, use the file with Type=notify to generate /usr/lib/systemd/system/dirsrv@.service?

And could you please elaborate why startpid is no longer needed?

remove the startpid as it's no longer needed

Thanks!

I think that we should actually make it to install systemd unit files, you need --with-systemd.

Startpid isn't needed, because type=notify doesn't for the ns-slapd process, so we only need to track the mainpid. Startpid might be needed for type=forking, but we never actually did anything with it ....

Replying to [comment:12 firstyear]:

I think that we should actually make it to install systemd unit files, you need --with-systemd.

Startpid isn't needed, because type=notify doesn't for the ns-slapd process, so we only need to track the mainpid. Startpid might be needed for type=forking, but we never actually did anything with it ....

Is this still true with your configure patch []?
[
] https://fedorahosted.org/389/ticket/47977#comment:11

And I'm curious how PIDFile is being used...
{{{
24 PIDFile=@localstatedir@/run/@package_name@/slapd-%i.pid
25 ExecStart=@sbindir@/ns-slapd -D @instconfigdir@/slapd-%i -i @localstatedir@/run/@package_name@/slapd-%i.pid
}}}
Can we do something like this? :)
{{{
24 PIDFile=@localstatedir@/run/@package_name@/slapd-%i.pid
25 ExecStart=@sbindir@/ns-slapd -D @instconfigdir@/slapd-%i -i PIDFile
}}}

Replying to [comment:13 nhosoi]:

Replying to [comment:12 firstyear]:

I think that we should actually make it to install systemd unit files, you need --with-systemd.

Startpid isn't needed, because type=notify doesn't for the ns-slapd process, so we only need to track the mainpid. Startpid might be needed for type=forking, but we never actually did anything with it ....

Is this still true with your configure patch []?
[
] https://fedorahosted.org/389/ticket/47977#comment:11

And I'm curious how PIDFile is being used...
{{{
24 PIDFile=@localstatedir@/run/@package_name@/slapd-%i.pid
25 ExecStart=@sbindir@/ns-slapd -D @instconfigdir@/slapd-%i -i @localstatedir@/run/@package_name@/slapd-%i.pid
}}}

Systemd still uses it for tracking what pid is the parent to watch, and to detect if the process has died abnormally.

Additionally, if someone does flick this back to forking, it will be able to be tracked / started properly.

Can we do something like this? :)
{{{
24 PIDFile=@localstatedir@/run/@package_name@/slapd-%i.pid
25 ExecStart=@sbindir@/ns-slapd -D @instconfigdir@/slapd-%i -i PIDFile
}}}

Sadly not. A limitation of systemd is that these variables are not in the enivronment, so this is not possible.

Thanks, William! How about this part?

Is this still true with your configure patch [*]?

Replying to [comment:13 nhosoi]:

Replying to [comment:12 firstyear]:

I think that we should actually make it to install systemd unit files, you need --with-systemd.

Startpid isn't needed, because type=notify doesn't for the ns-slapd process, so we only need to track the mainpid. Startpid might be needed for type=forking, but we never actually did anything with it ....

Is this still true with your configure patch []?
[
] https://fedorahosted.org/389/ticket/47977#comment:11
{{{
Bug Description: The systemd unit files now depend on parts of the
dirsrv code base that depends on with-systemd (see, sd_notify)
As a result, it's possible to create a non-functional build of ds
by building without "with-systemd" and then attempting to run it.
The creates an issue where type=notify cannot be satisfied in the
start up procedure.

Fix Description: This makes all unit file components of systemd
depend on with-systemd in order to be processed and used.
}}}

Replying to [comment:15 nhosoi]:

Thanks, William! How about this part?

Is this still true with your configure patch [*]?

Replying to [comment:13 nhosoi]:

Replying to [comment:12 firstyear]:

I think that we should actually make it to install systemd unit files, you need --with-systemd.

Startpid isn't needed, because type=notify doesn't for the ns-slapd process, so we only need to track the mainpid. Startpid might be needed for type=forking, but we never actually did anything with it ....

Is this still true with your configure patch []?
[
] https://fedorahosted.org/389/ticket/47977#comment:11
{{{
Bug Description: The systemd unit files now depend on parts of the
dirsrv code base that depends on with-systemd (see, sd_notify)
As a result, it's possible to create a non-functional build of ds
by building without "with-systemd" and then attempting to run it.
The creates an issue where type=notify cannot be satisfied in the
start up procedure.

Fix Description: This makes all unit file components of systemd
depend on with-systemd in order to be processed and used.
}}}

Yes. StartPidFile isn't needed. With both forking and notify types startpid == pid. So it served no purpose with systemd. I did some testing of this recently.

To ssh://git.fedorahosted.org/git/389/ds.git
069b94a..d4deb29 master -> master

commit d4deb29

The part of the fix which removes the starpidfile options has unwanted side effects.

There is no longer a startpidfile path provided, but the server still calls write_start_pid_file()

and uses /dev/null in this case. The unwanted side effect is that it also changes permissions of /dev/null and other users no longer are able to use it.

As long as the code around start pid file is not completely cleaned up it should be provided as option in the start command.

I think that it's a better progression to clean the call to write_start_pid_file(), rather than to put this back. Given this will only affect 1.3.5.X and up, existing 1.3.4.X and lower will retain the start pid file.

So I think that fixing the write_start_pid_file() calls is a better solution. I will look at this soon.

The comment for the original patch says that startpid is no longer needed. If this is true, the corresponding code should be completely removed.

startpid is still relevant for the shell script start-dirsrv, and perhaps the rc.d script. It's just systemd where we don't need it.

I can review this however to be sure.

Right, startpid is still used in the rc.d script. For now, I think we can safely leave the code there.

Replying to [comment:24 firstyear]:

Right, startpid is still used in the rc.d script. For now, I think we can safely leave the code there.

One question regarding startpidfile. Could you please take a look at this commit (in the master branch)? It removed the starpidfile check from start-dirsrv since it issued an unnecessary failure due to the lack of startpidfile. Is it okay? Or should we make it somehow conditional (not sure if it is even possible or not...)

commit 46dbd62
Ticket #48779 - Remove startpidfile check in start-dirsrv

Yes, but it's still in the wrappers/initscript.in file.

I think that it's okay to leave startpid out of start-dirsrv.

I think startpid was added to resolve and issue where it took too long for the pid file to be written. Perhaps if we see this again, the approach would be to replace pid with the "startpid" and ditch the other "late" pid file?

So for now, I'll leave the startpid code there, but it's just not in use.

commit 3348bcf
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
b2ef43e..3348bcf master -> master

Metadata Update from @nhosoi:
- Issue assigned to firstyear
- Issue set to the milestone: 1.3.5 backlog

2 years ago

Login to comment on this ticket.

Metadata