Merge lp:~jtv/maas/packaging-custom-dhcpd into lp:~maas-maintainers/maas/packaging

[Jeroen T. Vermeulen]

Upstart script for custom instance of dhcpd, using custom config, pidfile, leases file, and interfaces config so that we can dictate which interfaces it should listen on.

The interfaces config is to be written into /var/lib/maas/dhcpd-interfaces by the write_dhcp_config task. (It's not quite static config, so it belongs in /var/lib rather than /etc). Until it exists, it would be foolish or even harmful to start dhcpd; in that case the script aborts. The write_dhcp_config task will restart it when (re)configuring dhcpd.

Pre-imp (ages ago now) with roaksoax, who told me what name this file needs to have in order to get installed properly.

Jeroen

[Scott Moser (smoser)]

This will not work.

As I showed at https://code.launchpad.net/~smoser/maas/packaging.lp1049177/+merge/124083/comments/267072 and /etc/init/isc-dhcp-server.conf, the permissions on the leases file must be root:root and must be or the daemon will fail due to apparmor restrictions.

Additionally, you can avoid a fork and have the same functionality with:
  read INTERFACES < "${INTERFACES_FILE}"

review: Needs Fixing

[Jeroen T. Vermeulen (jtv)]

Wow, that could have been a little clearer: there is no explicit mention of this in the referred text, but it in turn has a link to a script that, near the bottom, sets the ownership of the leases file to root:root. Now, how does the regular dhcpd write to a leases file if it has to be root-owned? Or is that a restriction we face but the regular dhcpd doesn't? And why does the whole directory need to be root-owned?

Also, I think there could be something important missing from the text above. “and must be or the daemon will fail” — must be what?

[Scott Moser (smoser)]

"must be root:root and must be, or the daemon will fail"
I missed the ','.

I agree, that what you were trying to do seems logically to be a better solution. I tried the very same thing, and lost probably an hour in it figuring out why it didnt' work. If I had just started with the working version (/etc/init/isc-dhcp-server) I'd have not lost time.

You're certainly welcome to try to improve this, but if you are going to attempt to do so, please verify that it actually works.

[Jeroen T. Vermeulen (jtv)]

Scott, does the latest version address your concerns?

For the historical record: it turns out isc-dhcp-server underwent a significant change in Quantal. In Precise, the leases file must be owned by dhcpd; in Quantal, it must be owned by root.

I've updated my branches (this one and lp:~jtv/maas/custom-dhcp for the maas codebase) to reflect the Quantal situation. For simplicity I also changed the apparmor snippet to grant access to /var/lib/maas/dhcpd.leases rather than /var/lib/maas/dhcpd/*.leases.

I ran dhcpd using the resulting upstart script and apparmor snippet (making apparmor reload its config, of course) and it got past the checking and opening of the leases file OK. I had it set up not to serve any real DHCP, but it said that it wrote 0 leases — the expected number — whereas all failed attempts broke before that point.

My testing was still haphazard: all configuration etc. was done by hand. The next thing I'd like to learn is the proper way to combine a maas branch with a packaging branch, so I can build and install test packages.

Jeroen

[Scott Moser (smoser)]

8 -/var/lib/maas/dhcpd/*.leases* lrw,
9 +/var/lib/maas/dhcpd.leases* lrw,

I'd just leave that as it was. I do not think that the '*' hits anything other than 'dchpd', but in the dhcpv6 code, it is used to as dhcpd6.leases. So, if it were up to me, I'd just leave it as it is.

66 + chmod a+r $LFILE

Was there some explicit reason for this invention?
Neither precise nor quantal's isc-dhcp-server.conf file do this. I suspect it does nothing as the default umask for upstart jobs is probably 'a+r'.

Also, I mentioned previously:
 INTERFACES=`cat $INTERFACES_FILE`
is better done as:
 read INTERFACES < "${INTERFACES_FILE}"

It saves a fork.

[Scott Moser (smoser)]

I'll go ahead and approve this, but I really would like those things fixed.
Changing things from a production working example (isc-dhcp-server.conf) with minimal test and minimal time to fix fallout is probably not well advised at this point. I'm just trying to save us heartache.

review: Approve

[Jeroen T. Vermeulen (jtv)]

I met you halfway on the leases file's location: /var/lib/maas/dhcpd*.leases — that matches the IPv6 version as well as the IPv4 version, but still leaves out the extra directory level.

—

Yes, there is a reason for the “chmod a+r”: MAAS currently reads the leases file.

—

I replaced the “cat” with the “read.” I don't think saving a rare fork() is going to meet our scalability challenge though. :)

Jeroen

[Scott Moser (smoser)]

> I met you halfway on the leases file's location: /var/lib/maas/dhcpd*.leases —
> that matches the IPv6 version as well as the IPv4 version, but still leaves
> out the extra directory level.
>
> —
>
> Yes, there is a reason for the “chmod a+r”: MAAS currently reads the leases
> file.

As I said, there is no reason for explicitly setting this. Heres an example why:
$ cat /etc/init/foo.conf
task
script
 {
 echo "my umask: $(umask)"
 rm -f /tmp/foo
 touch /tmp/foo
 ls -l /tmp/foo
 } > /tmp/foo.out 2>&1
end script

$ sudo start foo ; cat /tmp/foo.out
foo stop/waiting
my umask: 0022
-rw-r--r-- 1 root root 0 Sep 19 10:12 /tmp/foo

$ sudo sh -c 'umask 666; sudo start foo' ; cat /tmp/foo.out
foo stop/waiting
my umask: 0022
-rw-r--r-- 1 root root 0 Sep 19 10:13 /tmp/foo

That shows at 0022 is the umask for upstart (which is exactly what you're chmoding to). So the chmod is un-necessary.

> I replaced the “cat” with the “read.” I don't think saving a rare fork() is
> going to meet our scalability challenge though. :)

The fork there in this case is about 100 times slower. Agreed, it doesn't matter here, but if 2 solutions do the same thing but 1 is 2 orders of magnitude faster, better to do that one.
fork is about 1000

[Jeroen T. Vermeulen (jtv)]

> > Yes, there is a reason for the “chmod a+r”: MAAS currently reads the leases
> > file.
>
> As I said, there is no reason for explicitly setting this. Heres an example
> why:

You said “probably.” I didn't take the risk. Being certain that this isn't dependent on some obscure factor that may change is worth more than saving a rare fork().

Jeroen

=== modified file 'debian/maas-dhcp.apparmor'
--- debian/maas-dhcp.apparmor	2012-09-13 00:20:44 +0000
+++ debian/maas-dhcp.apparmor	2012-09-20 05:32:19 +0000
@@ -3,5 +3,5 @@
 /run/maas/dhcp/*.pid lrw,
 /run/maas/dhcp/*.trace lrw,
 /run/maas/dhcp/*.leases* lrw,
-/var/lib/maas/dhcpd/*.leases* lrw,
+/var/lib/maas/dhcpd*.leases* lrw,
 /etc/maas/dhcpd.conf r,

=== added file 'debian/maas-dhcp.maas-dhcp-server.upstart'
--- debian/maas-dhcp.maas-dhcp-server.upstart	1970-01-01 00:00:00 +0000
+++ debian/maas-dhcp.maas-dhcp-server.upstart	2012-09-20 05:32:19 +0000
@@ -0,0 +1,57 @@
+description "MAAS instance of ISC DHCP server"
+author "Jeroen Vermeulen <[e-mail]>"
+
+start on runlevel [2345]
+stop on runlevel [!2345]
+
+env CONFIG_FILE=/etc/maas/dhcpd.conf
+env PID_DIR=/run/maas/dhcp
+env PID_FILE=$PID_DIR/dhcpd.pid
+env LEASES_FILE=/var/lib/maas/dhcpd.leases
+
+# This is where we write what interfaces dhcpd should listen on.
+env INTERFACES_FILE=/var/lib/maas/dhcpd-interfaces
+
+pre-start script
+    if [ ! -f $CONFIG_FILE ]; then
+        echo "$CONFIG_FILE does not exist.  Aborting."
+        stop
+        exit 0
+    fi
+
+    if [ ! -f $INTERFACES_FILE ]; then
+        echo "$INTERFACES_FILE does not exist.  Aborting."
+        stop
+        exit 0
+    fi
+
+    if ! /usr/sbin/dhcpd -t -q -4 -cf $CONFIG_FILE > /dev/null 2>&1; then
+        echo "dhcpd self-test failed.  Please fix the config file."
+        echo "The error was: "
+        /usr/sbin/dhcpd -t -4 -cf $CONFIG_FILE
+        stop
+        exit 0
+    fi
+end script
+
+respawn
+script
+    read INTERFACES < "${INTERFACES_FILE}"
+
+    # Allow dhcp server to write lease and pid file.
+    mkdir -p $PID_DIR
+    chown dhcpd:dhcpd $PID_DIR
+
+    # As of Quantal, the leases file must be owned by root:root (even though
+    # the daemon will run under an unprivileged user).
+    # In Precise, ownership was supposed to be dhcp:dhcp.
+    [ -e $LEASES_FILE ] || touch $LEASES_FILE
+    for LFILE in $LEASES_FILE $LEASES_FILE~; do
+        if [ -e $LFILE ]; then
+            chown root:root $LFILE
+            chmod a+r $LFILE
+        fi
+    done
+
+    exec /usr/sbin/dhcpd -user dhcpd -group dhcpd -f -q -4 -pf $PID_FILE -cf $CONFIG_FILE -lf $LEASES_FILE $INTERFACES
+end script