Pidfile redux#17
Conversation
This commit adds support for creating a pidfile on launch. It adds switches "--pidfile" and "-Z"; the short option is "-Z" because "p" and "P" were unavailable. I was unable to determine a way to clean up the pidfile on termination. So the pidfile created will end up being stale and is up to the invoker to deal with.
This commit adds in support for removing the pidfile on shutdown. For this to function, ucarp must be invoked with the --shutdown (-z) option.
There was a problem hiding this comment.
Why 100 * sizeof (long)? (and why use malloc() at all?)
There was a problem hiding this comment.
Just to make sure there is enough space for writing the pid to a string. I'm not an every day C programmer. If there is a better way of doing it, please educate me.
There was a problem hiding this comment.
'bit short on time, hence briefly; I'd go along this:
fwrite(&pid, sizeof(long), 1, pidfile);
POSIX(2008) specifies that pid_t may not exceed long. One might test for the actual size of pid, though, as it's dependent on the OS installation/site admin (PID_MAX on Linuxen).
There was a problem hiding this comment.
Thank you. I will make the change.
|
Okay, I switched to simply using |
|
Hey @jsumners. Thanks for submitting this PR and improving it. There have been a few submissions for creating a pid-file directly from the program, so this feature is obviously an important one. |
|
😞 It would be nice if the other PR used the same shorthand switch ( |
|
To give my opinion: if the fist letter of the switch is already used in both lower and upper case, then I tend to select one that is not yet use at all (i.e., none of the lower or upper case). |
|
I went with |
|
I have no preference of one over the other. Unfortunately, one of you will be disappointed in either case, as it sounds you're both using your branch in production. I can't help that.. @jsumners, I understand writing pid-files is an important feature to many, but I didn't need that ability when using upstream's version in Ubuntu 16.04. Why is it important to your application? |
|
More like it's important to systemd so that it can track the process correctly. And re-reviewing my ucarp RPM, it looks like I am using the long form |
|
Ah, ok. |
|
It's only available in the EPEL repository, and they packaged it a bit wrong. Look at mine instead: Specifically, https://github.com/jsumners/ucarp-rhel7/blob/d29991855f79b90cd50546fa0874cf052f270f4d/files/ucarp is what is launched to start the processes. |
|
You have Type=forking commented out in your service file there, which means it defaults to Type=simple, and there is no need for a pidfile at all. With Type=simple, systemd runs the process in the foreground and handles daemonization and pid tracking natively. From https://www.freedesktop.org/software/systemd/man/systemd.service.html:
and
The only reason I can see for using Type=forking here is if there are dependent services. Traditional svsvinit daemons signaled their ready-state by forking. Systemd can use that to detect a failed startup (eg. cannot bind, or expected file missing). With simple service types, systemd considers the service successfully launched as soon as the exec completes. I don't think that would make a difference with ucarp. The appearance of the VIP is not a sign of successful service launching; it's a sign of the service becoming MASTER. You'd likely want any dependent services to start regardless of the ucarp state on startup. You'd want to know ucarp failed to start, of course, but it's not a reason to stop the boot process, and issues could likely be resolved without needing a reboot. That said, the EPEL RPM apparently does use Type=forking, and they must have systemd guess the resulting daemon's pid. What is your objection to their packaging, if you can remember? |
|
To add to that:
Since ucarp is a single process, it should be able to guess the PID correctly if the type is set to forking, even if no pidfile is specified. I was under the impression, with systemd's use of cgroups, it could follow a process from startup to daemonization even if there are multiple instances launched at the same time, being that those processes are launched in different cgroups. @miconda, do you have any insight into this? Why are you finding the need to write a pidfile on startup? Reopening this for now while we hash this out... |
I'm not claiming to know all of the bullshit involved with systemd. All I know is it wasn't working with
Their launch script is still the same as it was for SysV init. It is not really compatible with systemd. See https://src.fedoraproject.org/cgit/rpms/ucarp.git/tree/ucarp |
|
Yeah, it looks like the guy who put together the RPM for EPEL didn't put much work into making it compliant with systemd and making sure it works as as intended. FWIW, I'm not a big systemd fan myself, bit it's what's been chosen by most distros now.. |
|
Following up with a bit delay, being caught by some other tasks lately ... Even systemd or other start-stop init.d scripts can follow a daemon app by pid and write the pid file themselves, there are so many distros out there and startup systems, many not having such capability. Moreover, in some cases I have to use ucarp in rather old systems, released when no systemd was out. Anyhow, my main need is to be able to use ucarp directly with Writing a pid file is a common feature of daemons out there, is not hard to implement and makes possible to supervise ucarp on all systems, with or without systemd. |
5 participants
As with pull request #16, this pull request adds support for a pidfile except it fixes the issue where the pidfile was not removed on shutdown. With this pull request if ucarp is invoked with
--pidfileand--shutdownthen the pidfile will unlinked on termination.