Merge lp:~blake-rouse/maas/fix-1559398 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Add maas-dhcp-notify wrapper that forks maas-rack dhcp-notify and returns. This fixes the issue where if a execute action in isc-dhcp-server takes to long it affects the performance of isc-dhcp-server.

I have QA'd this and previously commissioning with just 40 nodes would result only half getting an actual lease. This change allowed all to get a lease and commission successfully.

Packaging change is also required: https://code.launchpad.net/~blake-rouse/maas/fix-1559398-packaging/+merge/289713

[Mike Pontillo (mpontillo)]

See one minor nit below.

My only concern with this branch is that instead of queuing up on a blocking write, we're creating an explosion of processes whenever a large number of DHCP notifications happen.

While this might scale to ~40 nodes, it may not scale to hundreds or thousands; we may fail to create more processes and lose notifications.

Long-term, we should queue up DHCP notifications in memory as fast as we can read them, and a worker should dequeue and process them. (if we still have problems scaling, we can scan the queue for redundant notifications, too.)

review: Approve

[Mike Pontillo (mpontillo)]

One other data point: I wrote a simple script to accept connections on a UNIX datagram socket:

https://paste.ubuntu.com/15468191/

... with a "processing delay" of one second after reading each datagram.

Using the Python client, I can easily get ~10 packets ahead of the server while it takes its time dequeuing the packets, using a command similar to:

bin/maas-rack dhcp-notify --action commit --mac 74:d4:35:89:bd:a9 --ip-family ipv4 --ip 192.168.10.60 --lease-time 43200 --hostname test-$SEQ --socket /tmp/test-socket

Using netcat, I can send in 500+ packets before it starts blocking on write, using a command similar to:

echo -n "{\"ip\": \"192.168.10.60\", \"mac\": \"74:d4:35:89:bd:a9\", \"ip_family\": \"ipv4\", \"hostname\": \"test-$SEQ\", \"lease_time\": 43200, \"action\": \"commit\", \"timestamp\": 1458597246}" | nc -w0 -uU /tmp/test-socket

[Mike Pontillo (mpontillo)]

Upon further testing, I don't think the fork approach is viable. I was able to cripple my system after ~150-200 processes forked, which could easily happen in MAAS if someone brings a rack online.

review: Disapprove

[Mike Pontillo (mpontillo)]

It looks like something we're doing to set up the notification process is very slow (on the order of half a second, or ~100 notifications in a little less than a minute). Since we don't want to pull in a dependency on netcat, I looked at other ways to send the lease notification message faster.

If I use the following test code to write to the socket, it's plenty fast (~100 notifications in ~1.2 seconds on my system):

python -c "import socket; conn = socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM); conn.connect('/tmp/test-socket'); conn.send('<lease-json>'); conn.close()"

Something we're doing to set up the command framework and parse the arguments may be what's making it slow. (I already tried eliminating the dependency on the JSON module, and that didn't make any difference.)

[Mike Pontillo (mpontillo)]

Looks much better; thanks for the fixes.

I see you've included the host-cleaning functionality in this utility as well. (I didn't look at it in too much detail, but it seems a little scary to do this with a regular expression; you might want to verify that it can't ever match the word "host" inside, for example, a lease block.) So a couple of comments on that:

 - It might be safer to use the ISC parser than a regular expression.
 - I hope we're going to ensure that dhcpd is disabled while we clean the file, since it could be large, and in the time it takes to rewrite the file another lease could easily have been allocated.
 - Could we possibly read the file to determine which host entries we want to remove, and use OMAPI to remove them?

review: Approve

[Mike Pontillo (mpontillo)]

One other comment. Since you've added the 'clean' functionality to this code, you could probably make the socket notifier portion faster if you move "import re" to the clean function, that way it isn't imported if we aren't cleaning the leases.

[Blake Rouse (blake-rouse)]

Good idea. Moved the imports to each sub command so that they run faster.

Also the clean is called specifically from systemd when the the service before it is started. So its 100% not running when that is called. This ensures that its not being modified at the same time, as its not even running.

The test includes hostname in the lease entry that my previous regex was matching. This regex is specific to a host block not any host in the lease section.

=== modified file 'etc/maas/templates/dhcp/dhcpd.conf.template'
--- etc/maas/templates/dhcp/dhcpd.conf.template	2016-03-17 19:09:25 +0000
+++ etc/maas/templates/dhcp/dhcpd.conf.template	2016-03-22 17:04:41 +0000
@@ -89,7 +89,7 @@
    set cllt = binary-to-ascii(10, 32, "", encode-int(lease-time, 32));
    set clht = pick-first-value(option host-name, "(none)");
    execute(
-       "/usr/sbin/maas-rack", "dhcp-notify",
+       "/usr/sbin/maas-dhcp-helper", "notify",
        "--action", "commit", "--mac", clhw,
        "--ip-family", "ipv4", "--ip", clip,
        "--lease-time", cllt, "--hostname", clht);
@@ -98,7 +98,7 @@
    set clhw = binary-to-ascii(16, 8, ":", substring(hardware, 1, 6));
    set clip = binary-to-ascii(10, 8, ".", leased-address);
    execute(
-       "/usr/sbin/maas-rack", "dhcp-notify",
+       "/usr/sbin/maas-dhcp-helper", "notify",
        "--action", "expiry", "--mac", clhw,
        "--ip-family", "ipv4", "--ip", clip);
 }
@@ -106,7 +106,7 @@
    set clhw = binary-to-ascii(16, 8, ":", substring(hardware, 1, 6));
    set clip = binary-to-ascii(10, 8, ".", leased-address);
    execute(
-       "/usr/sbin/maas-rack", "dhcp-notify",
+       "/usr/sbin/maas-dhcp-helper", "notify",
        "--action", "release", "--mac", clhw,
        "--ip-family", "ipv4", "--ip", clip);
 }
 
=== added file 'scripts/maas-dhcp-helper'
--- scripts/maas-dhcp-helper	1970-01-01 00:00:00 +0000
+++ scripts/maas-dhcp-helper	2016-03-22 17:04:41 +0000
@@ -0,0 +1,128 @@
+#!/usr/bin/env python3
+# -*- mode: python -*-
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Utility script used by maas-dhcpd and maas-dhcpd6.
+
+`notify` is called by maas-dhcpd and maas-dhcpd6 with the on execute command
+from isc-dhcp-server when a lease is committed, released, or expired. Sends
+a JSON encoded message over the `dhcpd.sock` to the running rackd process.
+
+`clean` is called by maas-dhcpd and maas-dhcpd6 when either service starts
+to remove any host entries from the passed leases file. This is used to make
+sure that when maas-dhcpd and maas-dhcpd6 start that only the host entries
+from the dhcpd.conf or dhcpd6.conf are used.
+"""
+
+import argparse
+from contextlib import closing
+import json
+import re
+import socket
+import time
+
+
+def notify_add_arguments(parser):
+    """Initialise options for sending DHCP notification over the dhcpd.sock.
+
+    :param parser: An instance of :class:`ArgumentParser`.
+    """
+    parser.add_argument(
+        "--action", action="store", required=True,
+        choices=['commit', 'expiry', 'release'], help=(
+            "Action taken by DHCP server for the lease."))
+    parser.add_argument(
+        "--mac", action="store", required=True, help=(
+            "MAC address for lease."))
+    parser.add_argument(
+        "--ip-family", action="store", required=True, choices=['ipv4', 'ipv6'],
+        help="IP address family for lease.")
+    parser.add_argument(
+        "--ip", action="store", required=True, help=(
+            "IP address for lease."))
+    parser.add_argument(
+        "--lease-time", action="store", type=int, required=False, help=(
+            "Length of time before the lease expires."))
+    parser.add_argument(
+        "--hostname", action="store", required=False, help=(
+            "Hostname of the machine for the lease."))
+    parser.add_argument(
+        "--socket", action="store", required=False,
+        default="/var/lib/maas/dhcpd.sock", help=argparse.SUPPRESS)
+
+
+def notify(args):
+    """Write DHCP action notification to the `dhcpd.sock`."""
+    notify_packet = {
+        "action": args.action,
+        "mac": args.mac,
+        "ip_family": args.ip_family,
+        "ip": args.ip,
+        "timestamp": int(time.time()),
+    }
+
+    # Lease time is required by the commit action and hostname is optional.
+    if args.action == "commit":
+        notify_packet["lease_time"] = args.lease_time
+        hostname = args.hostname
+        has_hostname = (
+            hostname is not None and
+            len(hostname) > 0 and
+            not hostname.isspace())
+        if has_hostname:
+            notify_packet["hostname"] = hostname
+
+    # Connect to the socket and send the packet over as JSON.
+    payload = json.dumps(notify_packet)
+    conn = socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
+    conn.connect(args.socket)
+    with closing(conn):
+        conn.send(payload.encode("utf-8"))
+
+
+def clean_add_arguments(parser):
+    """Initialise options for cleaning the dhcpd.leases file.
+
+    :param parser: An instance of :class:`ArgumentParser`.
+    """
+    parser.add_argument(
+        "leases_file", action="store",
+        help=("Leases file to remove host entries from."))
+
+
+def clean(args):
+    """Remove all host entries from leases file."""
+    with open(args.leases_file, "r", encoding="utf-8") as fp:
+        content = fp.read()
+    cleaned_content = re.sub("host [^{]+{[^}]+}\n", "", content)
+    with open(args.leases_file, "w", encoding="utf-8") as fp:
+        fp.write(cleaned_content)
+
+
+def main():
+    """Main entry point for the script."""
+    parser = argparse.ArgumentParser()
+    subparsers = parser.add_subparsers(title="command")
+    subparsers.required = True
+    subparsers.dest = "command"
+
+    # Notify.
+    notify_parser = subparsers.add_parser(
+        "notify", help="Write DHCP action notification to the `dhcpd.sock`.")
+    notify_parser.set_defaults(handler=notify)
+    notify_add_arguments(notify_parser)
+
+    # Clean.
+    clean_parser = subparsers.add_parser(
+        "clean", help="Remove all host entries from leases file.")
+    clean_parser.set_defaults(handler=clean)
+    clean_add_arguments(clean_parser)
+
+    # Run the parser and the command.
+    args = parser.parse_args()
+    args.handler(args)
+
+
+if __name__ == "__main__":
+    main()
 
=== modified file 'setup.py'
--- setup.py	2016-01-04 16:24:36 +0000
+++ setup.py	2016-03-22 17:04:41 +0000
@@ -81,6 +81,8 @@
         ('/usr/bin',
             ['scripts/maas-generate-winrm-cert',
              'scripts/uec2roottar']),
+        ('/usr/sbin',
+            ['scripts/maas-dhcp-notify']),
     ],
 
     classifiers=[
 
=== modified file 'src/provisioningserver/__main__.py'
--- src/provisioningserver/__main__.py	2016-03-17 04:26:14 +0000
+++ src/provisioningserver/__main__.py	2016-03-22 17:04:41 +0000
@@ -8,7 +8,6 @@
 import provisioningserver.boot.install_bootloader
 import provisioningserver.boot.install_grub
 import provisioningserver.cluster_config_command
-import provisioningserver.dhcp.notifier
 import provisioningserver.support_dump
 import provisioningserver.upgrade_cluster
 from provisioningserver.utils.script import (
@@ -23,7 +22,6 @@
     'atomic-delete': AtomicDeleteScript,
     'check-for-shared-secret': security.CheckForSharedSecretScript,
     'config': provisioningserver.cluster_config_command,
-    'dhcp-notify': provisioningserver.dhcp.notifier,
     'install-shared-secret': security.InstallSharedSecretScript,
     'install-uefi-config': provisioningserver.boot.install_grub,
     'support-dump': provisioningserver.support_dump,
 
=== removed file 'src/provisioningserver/dhcp/notifier.py'
--- src/provisioningserver/dhcp/notifier.py	2015-12-23 15:59:21 +0000
+++ src/provisioningserver/dhcp/notifier.py	1970-01-01 00:00:00 +0000
@@ -1,73 +0,0 @@
-# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Write DHCP action notification to the `dhcpd.sock`."""
-
-__all__ = [
-    "add_arguments",
-    "run",
-    ]
-
-import argparse
-from contextlib import closing
-import json
-import socket
-import time
-
-
-def add_arguments(parser):
-    """Initialise options for sending DHCP notification over the dhcpd.sock.
-
-    :param parser: An instance of :class:`ArgumentParser`.
-    """
-    parser.add_argument(
-        "--action", action="store", required=True,
-        choices=['commit', 'expiry', 'release'], help=(
-            "Action taken by DHCP server for the lease."))
-    parser.add_argument(
-        "--mac", action="store", required=True, help=(
-            "MAC address for lease."))
-    parser.add_argument(
-        "--ip-family", action="store", required=True, choices=['ipv4', 'ipv6'],
-        help="IP address family for lease.")
-    parser.add_argument(
-        "--ip", action="store", required=True, help=(
-            "IP address for lease."))
-    parser.add_argument(
-        "--lease-time", action="store", type=int, required=False, help=(
-            "Length of time before the lease expires."))
-    parser.add_argument(
-        "--hostname", action="store", required=False, help=(
-            "Hostname of the machine for the lease."))
-    parser.add_argument(
-        "--socket", action="store", required=False,
-        default="/var/lib/maas/dhcpd.sock", help=argparse.SUPPRESS)
-
-
-def run(args):
-    """Write DHCP action notification to the `dhcpd.sock`."""
-    notify_packet = {
-        "action": args.action,
-        "mac": args.mac,
-        "ip_family": args.ip_family,
-        "ip": args.ip,
-        "timestamp": int(time.time()),
-    }
-
-    # Lease time is required by the commit action and hostname is optional.
-    if args.action == "commit":
-        notify_packet["lease_time"] = args.lease_time
-        hostname = args.hostname
-        has_hostname = (
-            hostname is not None and
-            len(hostname) > 0 and
-            not hostname.isspace())
-        if has_hostname:
-            notify_packet["hostname"] = hostname
-
-    # Connect to the socket and send the packet over as JSON.
-    payload = json.dumps(notify_packet)
-    conn = socket.socket(socket.AF_UNIX, socket.SOCK_DGRAM)
-    conn.connect(args.socket)
-    with closing(conn):
-        conn.send(payload.encode("utf-8"))
 
=== added file 'src/provisioningserver/dhcp/tests/test_helper_clean.py'
--- src/provisioningserver/dhcp/tests/test_helper_clean.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/dhcp/tests/test_helper_clean.py	2016-03-22 17:04:41 +0000
@@ -0,0 +1,103 @@
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for maas-dhcp-support clean command."""
+
+__all__ = []
+
+from maastesting import root
+from maastesting.testcase import MAASTestCase
+from provisioningserver.utils.fs import read_text_file
+from provisioningserver.utils.shell import call_and_check
+
+
+LEASES_FILE_WITH_HOSTS = """\
+lease 192.168.10.67 {
+  starts 2 2016/03/22 13:44:15;
+  ends 3 2016/03/23 01:44:15;
+  cltt 2 2016/03/22 13:44:15;
+  binding state active;
+  next binding state free;
+  rewind binding state free;
+  hardware ethernet 74:d4:35:89:bc:f2;
+  set clht = "zoochemical-carlton";
+  set cllt = "43200";
+  set clip = "192.168.10.67";
+  set clhw = "74:d4:35:89:bc:f2";
+  set vendor-class-identifier = "Linux ipconfig";
+  client-hostname "zoochemical-carlton";
+}
+host 74-d4-35-89-b9-e8 {
+  dynamic;
+  hardware ethernet 74:d4:35:89:b9:e8;
+  fixed-address 192.168.10.5;
+}
+lease 192.168.10.69 {
+  starts 2 2016/03/22 13:44:15;
+  ends 3 2016/03/23 01:44:15;
+  cltt 2 2016/03/22 13:44:15;
+  binding state active;
+  next binding state free;
+  rewind binding state free;
+  hardware ethernet 74:d4:35:89:bd:25;
+  set clht = "undetesting-johnetta";
+  set cllt = "43200";
+  set clip = "192.168.10.69";
+  set clhw = "74:d4:35:89:bd:25";
+  set vendor-class-identifier = "Linux ipconfig";
+}
+host 74-d4-35-89-bc-26 {
+  dynamic;
+  hardware ethernet 74:d4:35:89:bc:26;
+  fixed-address 192.168.10.7;
+}
+host 74-d4-35-89-bc-23 {
+  dynamic;
+  deleted;
+}
+"""
+
+
+LEASES_FILE_WITHOUT_HOSTS = """\
+lease 192.168.10.67 {
+  starts 2 2016/03/22 13:44:15;
+  ends 3 2016/03/23 01:44:15;
+  cltt 2 2016/03/22 13:44:15;
+  binding state active;
+  next binding state free;
+  rewind binding state free;
+  hardware ethernet 74:d4:35:89:bc:f2;
+  set clht = "zoochemical-carlton";
+  set cllt = "43200";
+  set clip = "192.168.10.67";
+  set clhw = "74:d4:35:89:bc:f2";
+  set vendor-class-identifier = "Linux ipconfig";
+  client-hostname "zoochemical-carlton";
+}
+lease 192.168.10.69 {
+  starts 2 2016/03/22 13:44:15;
+  ends 3 2016/03/23 01:44:15;
+  cltt 2 2016/03/22 13:44:15;
+  binding state active;
+  next binding state free;
+  rewind binding state free;
+  hardware ethernet 74:d4:35:89:bd:25;
+  set clht = "undetesting-johnetta";
+  set cllt = "43200";
+  set clip = "192.168.10.69";
+  set clhw = "74:d4:35:89:bd:25";
+  set vendor-class-identifier = "Linux ipconfig";
+}
+"""
+
+
+class TestDHCPClean(MAASTestCase):
+
+    def test_removes_hosts_from_leases_file(self):
+        path = self.make_file(contents=LEASES_FILE_WITH_HOSTS)
+        call_and_check([
+            "%s/scripts/maas-dhcp-helper" % root,
+            "clean",
+            path,
+        ])
+        self.assertEquals(LEASES_FILE_WITHOUT_HOSTS, read_text_file(path))
 
=== renamed file 'src/provisioningserver/dhcp/tests/test_notifier.py' => 'src/provisioningserver/dhcp/tests/test_helper_notify.py'
--- src/provisioningserver/dhcp/tests/test_notifier.py	2016-03-07 23:20:52 +0000
+++ src/provisioningserver/dhcp/tests/test_helper_notify.py	2016-03-22 17:04:41 +0000
@@ -1,7 +1,7 @@
-# Copyright 2014-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Tests for maas-rack dhcp-notify command."""
+"""Tests for maas-dhcp-support notify command."""
 
 __all__ = []
 
@@ -67,8 +67,8 @@
         self.addCleanup(service.stopService)
 
         call_and_check([
-            "%s/bin/maas-rack" % root,
-            "dhcp-notify",
+            "%s/scripts/maas-dhcp-helper" % root,
+            "notify",
             '--action', action,
             '--mac', mac,
             '--ip-family', ip_family,