[patch] upstart job for freeradius

Thanks Michael!

@Robie: if it looks good I can go ahead and upload myself, I just wanted to get review first.

I asked in #ubuntu-dev just now if we want to convert daemons in universe to upstart when there's no particular
other reason, and the answer was yes, we do.

Have you forgotten to source /etc/default/freeradius to pick up any FREERADIUS_OPTIONS settings?

Apart from that it lgtm, assuming it's tested and it works. Looks like you've taken care to carry over the required initialisation checks (mkdir -p and ownership).

It looks like you've been trumped by Yolanda's dep8 test though - and that hits /etc/init.d/freeradius. Will that break, or continue working OK?

17:48 <slangasek> rbasak: why should you have to maintain a delta for an upstart
                  job now? These should all be upstreamable to Debian
17:49 <rbasak> slangasek: I was asking in response to review a debdiff to introduce
               an upstart job in our delta, to override an init.d from Debian. bug
               1187742. mvo doesn't appear to be here right now.
17:49 <ubottu> bug 1187742 in freeradius (Ubuntu) "[patch] upstart job for
               freeradius" [Wishlist,Triaged] https://launchpad.net/bugs/1187742
17:50 <rbasak> slangasek: I can ask that he send this up to Debian, of course.
17:50 <slangasek> rbasak: right, it should definitely be sent there in parallel
17:50 <cjwatson> It's a pretty recent development that it's possible to upstream
                 these; mvo may just not be aware of it
17:50 <cjwatson> (Well, possible sanely, as opposed to the crazy thing openssh used
                 to do)

Thanks Robie,

good catch about the sourcing of the /etc/defaults file. I noticed that too but forgot to update the debdiff. And thanks to Steve and Colin, I will definitely send the diff to debian first.

This bug was fixed in the package freeradius - 2.1.12+dfsg-1.2ubuntu4

---------------
freeradius (2.1.12+dfsg-1.2ubuntu4) saucy; urgency=low

  * fix FTBFS:
    - add -lpthread to the libfreeradius.so build
    - add -lssl -lcrypto to the libeap.so build
 -- Michael Vogt <email address hidden> Tue, 09 Jul 2013 15:35:08 +0200

The symlinks in the /etc/rc?.d directories are still in place after the upstart job for FreeRADIUS was added via this bug. Is this correct or there is nothing that prevents both the init.d script and the upstart job from attempting to start the job?

Also, the upstart job seems to be tracking incorrectly the freeradius process as "reload freeradius" does not work. I notice that the upstart job does not run the FreeRADIUS executable as a daemon (-f specified) so a PID file is not written to /var/run/freeradius. The init.d script does run the server as a daemon, which is probably more correct.

Should separate bugs be filed for these two issues?

As a workaround I have disabled the upstart job and fallen back to the init.d script which does a fine job tracking the process and supporting reloads and restarts.

On Tue, Aug 26, 2014 at 01:56:53PM -0000, Eloy Paris wrote:
> The symlinks in the /etc/rc?.d directories are still in place after the
> upstart job for FreeRADIUS was added via this bug. Is this correct or
> there is nothing that prevents both the init.d script and the upstart
> job from attempting to start the job?

No, this is fine. I think. It allows a system to switch init systems,
since we're in a multiple init system world now. I believe the packaging
and/or dh_installinit makes sure to not stop duplicate jobs. However,
the user can still do it accidentally, which is bug 1273462. In general,
in this new world users should always use the "service" wrapper to make
sure that the right thing happens.

> Also, the upstart job seems to be tracking incorrectly the freeradius
> process as "reload freeradius" does not work. I notice that the upstart
> job does not run the FreeRADIUS executable as a daemon (-f specified) so
> a PID file is not written to /var/run/freeradius. The init.d script does
> run the server as a daemon, which is probably more correct.
>
> Should separate bugs be filed for these two issues?

I'm not sure this is a bug exactly, since upstart tracks processes so a
PID file shouldn't be needed for a reload, by upstart at least. Running
in the foreground isn't necessarily a problem here, either, depending on
how the process handles that flag.

But if you want to discuss this further, a separate bug would be
useful, since this one is "Fix Released" and so we can't track any
status for your concerns.

To reload freeradius properly upstart job should be fixed. Here is the patch.

Thanks urusha, patch uploaded.

This upstart script, unlike the init.d script, doesn't create a pidfile in /var/run/freeradius even though it creates the folder. This makes monitoring freeradius difficult.

It should be trivial to start it instead with start-stop-daemon and have it properly create the pidfile at /var/run/freeradius/freeradius.pid.

@Samuel

Can you create a new bug so we can track that deficiency, please? As this bug is fixed, your point is likely to get lost otherwise.