launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/update-leases into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/update-leases/+merge/113553

This is one piece of a puzzle that is still being laid, as sketched out in an extensive pre-imp between Raphaël and myself.

Several of the pieces this one relies upon are not actually there yet; they are just stubs here and will have to be filled in as part of separate tasks that are already on the board.

What this does provide is the logic (and Celery tasks) for:

1. Uploading all of a node group's DHCP lease information if there have been changes.

2. Uploading all of a node group's DHCP lease information.

The first is what you'd normally run when there may have been lease changes.  It's quite cheap to run, so it could be run very frequently.  In the normal case all it does is stat() the leases file.  If leases have been renewed but nothing of interest has changed, it also parses the file.  Only when there are changes that matter does it send anything to the server.

The second is for scrubbing.  There are all sorts of ways in which lease updates could get lost on their way to the MAAS server, from there into the master worker, and finally into a DNS zone file.  And so it's just good housekeeping to re-upload the information regularly — but not as regularly as the first task, because it could be quite expensive.

As you'll note looking through the code, the parser does not actually exist yet.  All the unit tests mock it up.  Nor is there code to do the uploads; that's yet another task we've got lined up.  But there is code to minimize races and avoid unnecessary work.  We faced some nasty chicken-and-egg problems with this work; have to start somewhere.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/update-leases/+merge/113553
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/update-leases into lp:maas.

=== modified file 'etc/celeryconfig.py'
--- etc/celeryconfig.py	2012-07-03 16:33:40 +0000
+++ etc/celeryconfig.py	2012-07-05 11:28:22 +0000
@@ -33,6 +33,10 @@
 # Location of MAAS' bind configuration files.
 DNS_CONFIG_DIR = os.path.join(os.sep, 'var', 'cache', 'bind', 'maas')
 
+# DHCP leases file, as maintained by ISC dhcpd.
+DHCP_LEASES_FILE = '/var/lib/dhcp/dhcpd.leases'
+
+
 try:
     import user_maasceleryconfig
 except ImportError:

=== added file 'src/provisioningserver/dhcp/tests/test_update_leases.py'
--- src/provisioningserver/dhcp/tests/test_update_leases.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/dhcp/tests/test_update_leases.py	2012-07-05 11:28:22 +0000
@@ -0,0 +1,186 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the report_leases task."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from datetime import (
+    datetime,
+    timedelta,
+    )
+
+from maastesting.factory import factory
+from maastesting.fakemethod import FakeMethod
+from maastesting.testcase import TestCase
+from maastesting.utils import (
+    age_file,
+    get_write_time,
+    )
+from provisioningserver.dhcp import update_leases
+
+
+class StopExecuting(BaseException):
+    """Exception class to stop execution at a desired point.
+
+    This is deliberately not just an :class:`Exception`.  We want to
+    interrupt the code that's being tested, not just exercise its
+    error-handling capabilities.
+    """
+
+
+class TestUpdateLeases(TestCase):
+
+    def make_lease(self):
+        """Create a leases dict with one, arbitrary lease in it."""
+        return {factory.getRandomIPAddress(): factory.getRandomMACAddress()}
+
+    def fake_leases_file(self, leases=None, age=None):
+        """Create a fake leases file.
+
+        :param leases: Dict of leases (mapping IP addresses to MACs).
+        :param age: Number of seconds since last modification to leases file.
+        :return: Path/name of temporary file.
+        """
+        if leases is None:
+            leases = {}
+        leases = leases.copy()
+        leases_file = self.make_file()
+        if age is not None:
+            age_file(leases_file, age)
+        timestamp = get_write_time(leases_file)
+        self.patch(update_leases, 'DHCP_LEASES_FILE', leases_file)
+        # TODO: We don't have a lease-file parser yet.  For now, just
+        # fake up a "parser" that returns the given data.
+        self.patch(
+            update_leases, 'parse_leases', lambda: (timestamp, leases))
+        return leases_file
+
+    def test_check_lease_changes_returns_tuple_if_no_state_cached(self):
+        update_leases.record_lease_state(None, None)
+        leases = self.make_lease()
+        leases_file = self.fake_leases_file(leases)
+        self.assertEqual(
+            (get_write_time(leases_file), leases),
+            update_leases.check_lease_changes())
+
+    def test_check_lease_changes_returns_tuple_if_lease_changed(self):
+        ip = factory.getRandomIPAddress()
+        leases = {ip: factory.getRandomMACAddress()}
+        update_leases.record_lease_state(
+            datetime.utcnow() - timedelta(seconds=10), leases.copy())
+        leases[ip] = factory.getRandomMACAddress()
+        leases_file = self.fake_leases_file(leases)
+        self.assertEqual(
+            (get_write_time(leases_file), leases),
+            update_leases.check_lease_changes())
+
+    def test_check_lease_changes_does_not_parse_unchanged_leases_file(self):
+        parser = FakeMethod()
+        leases_file = self.fake_leases_file()
+        self.patch(update_leases, 'parse_leases', parser)
+        update_leases.record_lease_state(get_write_time(leases_file), {})
+        update_leases.update_leases()
+        self.assertSequenceEqual([], parser.calls)
+
+    def test_check_lease_changes_returns_tuple_if_lease_added(self):
+        leases = self.make_lease()
+        update_leases.record_lease_state(
+            datetime.utcnow() - timedelta(seconds=10), leases.copy())
+        leases[factory.getRandomIPAddress()] = factory.getRandomMACAddress()
+        leases_file = self.fake_leases_file(leases)
+        self.assertEqual(
+            (get_write_time(leases_file), leases),
+            update_leases.check_lease_changes())
+
+    def test_check_lease_changes_returns_tuple_if_leases_dropped(self):
+        update_leases.record_lease_state(
+            datetime.utcnow() - timedelta(seconds=10), self.make_lease())
+        leases_file = self.fake_leases_file({})
+        self.assertEqual(
+            (get_write_time(leases_file), {}),
+            update_leases.check_lease_changes())
+
+    def test_check_lease_changes_returns_None_if_no_change(self):
+        leases = self.make_lease()
+        leases_file = self.fake_leases_file(leases)
+        update_leases.record_lease_state(
+            get_write_time(leases_file), leases.copy())
+        self.assertIsNone(update_leases.check_lease_changes())
+
+    def test_check_lease_changes_ignores_irrelevant_changes(self):
+        leases = self.make_lease()
+        self.fake_leases_file(leases, age=10)
+        update_leases.record_lease_state(datetime.utcnow(), leases.copy())
+        self.assertIsNone(update_leases.check_lease_changes())
+
+    def test_update_leases_sends_leases_if_changed(self):
+        update_leases.record_lease_state(None, None)
+        send_leases = FakeMethod()
+        self.patch(update_leases, 'send_leases', send_leases)
+        leases = self.make_lease()
+        self.fake_leases_file(leases)
+        update_leases.update_leases()
+        self.assertSequenceEqual([(leases, )], send_leases.extract_args())
+
+    def test_update_leases_does_nothing_without_lease_changes(self):
+        send_leases = FakeMethod()
+        self.patch(update_leases, 'send_leases', send_leases)
+        leases = self.make_lease()
+        leases_file = self.fake_leases_file(leases)
+        update_leases.record_lease_state(
+            get_write_time(leases_file), leases.copy())
+        self.assertSequenceEqual([], send_leases.calls)
+
+    def test_update_leases_records_update(self):
+        update_leases.record_lease_state(None, None)
+        self.fake_leases_file()
+        self.patch(update_leases, 'send_leases', FakeMethod())
+        update_leases.update_leases()
+        self.assertIsNone(update_leases.check_lease_changes())
+
+    def test_update_leases_records_state_before_sending(self):
+        update_leases.record_lease_state(None, None)
+        self.fake_leases_file()
+        self.patch(
+            update_leases, 'send_leases', FakeMethod(failure=StopExecuting()))
+        try:
+            update_leases.update_leases()
+        except StopExecuting:
+            pass
+        self.assertIsNone(update_leases.check_lease_changes())
+
+    def test_upload_leases_sends_leases_unconditionally(self):
+        send_leases = FakeMethod()
+        leases = self.make_lease()
+        leases_file = self.fake_leases_file(leases)
+        update_leases.record_lease_state(get_write_time
+            (leases_file), leases.copy())
+        self.patch(update_leases, 'send_leases', send_leases)
+        update_leases.upload_leases()
+        self.assertSequenceEqual([(leases, )], send_leases.extract_args())
+
+    def test_upload_leases_records_update(self):
+        update_leases.record_lease_state(None, None)
+        self.fake_leases_file()
+        self.patch(update_leases, 'send_leases', FakeMethod())
+        update_leases.upload_leases()
+        self.assertIsNone(update_leases.check_lease_changes())
+
+    def test_upload_leases_records_state_before_sending(self):
+        update_leases.record_lease_state(None, None)
+        self.fake_leases_file()
+        self.patch(
+            update_leases, 'send_leases', FakeMethod(failure=StopExecuting()))
+        try:
+            update_leases.upload_leases()
+        except StopExecuting:
+            pass
+        self.assertIsNone(update_leases.check_lease_changes())

=== added file 'src/provisioningserver/dhcp/update_leases.py'
--- src/provisioningserver/dhcp/update_leases.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/dhcp/update_leases.py	2012-07-05 11:28:22 +0000
@@ -0,0 +1,118 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Send lease updates to the server.
+
+This code runs inside node-group workers.  It watches for changes to DHCP
+leases, and notifies the MAAS server so that it can rewrite DNS zone files
+as appropriate.
+
+Leases in this module are represented as dicts, mapping each leased IP
+address to the MAC address that it belongs to.
+
+The modification time and leases of the last-uploaded leases are cached,
+so as to suppress unwanted redundant updates.  This cache is updated
+*before* the actual upload, so as to prevent thundering-herd problems:
+if an upload takes too long for whatever reason, subsequent updates
+should not be uploaded until the first upload is done.  Some uploads may
+be lost due to concurrency or failures, but the situation will right
+itself eventually.
+"""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'upload_leases',
+    'update_leases',
+    ]
+
+
+from os import stat
+
+from celery.decorators import task
+from celeryconfig import DHCP_LEASES_FILE
+
+# Modification time on last-processed leases file.
+recorded_leases_time = None
+
+# Leases as last parsed.
+recorded_leases = None
+
+
+def get_leases_timestamp():
+    """Return the last modification timestamp of the DHCP leases file."""
+    return stat(DHCP_LEASES_FILE).st_mtime
+
+
+def parse_leases():
+    """Parse the DHCP leases file.
+
+    :return: A tuple: (timestamp, leases).  The `timestamp` is the last
+        modification time of the leases file, and `leases` is a dict
+        mapping leased IP addresses to their associated MAC addresses.
+    """
+    # TODO: Implement leases-file parser here.
+
+
+def check_lease_changes():
+    """Has the DHCP leases file changed in any significant way?"""
+    if get_leases_timestamp() == recorded_leases_time:
+        return None
+    timestamp, leases = parse_leases()
+    if leases == recorded_leases:
+        return None
+    else:
+        return timestamp, leases
+
+
+def record_lease_state(last_change, leases):
+    """Record a snapshot of the state of DHCP leases.
+
+    :param last_change: Modification date on the leases file with the given
+        leases.
+    :param leases: A dict mapping each leased IP address to the MAC address
+        that it has been assigned to.
+    """
+    global recorded_leases_time
+    global recorded_leases
+    recorded_leases_time = last_change
+    recorded_leases = leases
+
+
+def send_leases(leases):
+    """Send snapshot of current leases to the MAAS server."""
+    # TODO: Implement API call for uploading leases.
+
+
+@task
+def upload_leases(**kwargs):
+    """Unconditionally send the current DHCP leases to the server.
+
+    Run this periodically just so no changes slip through the cracks.
+    Examples of such cracks would be: subtle races, failure to upload,
+    server restarts, or zone-file update commands getting lost on their
+    way to the DNS server.
+    """
+    timestamp, leases = parse_leases()
+    record_lease_state(timestamp, leases)
+    send_leases(leases)
+
+
+@task
+def update_leases(**kwargs):
+    """Check for DHCP lease updates, and send them to the server if needed.
+
+    Run this whenever a lease has been added, removed, or changed.  It
+    will be very cheap to run if the leases file has not been touched,
+    and it won't upload unless there have been relevant changes.
+    """
+    updated_lease_info = check_lease_changes()
+    if updated_lease_info is not None:
+        timestamp, leases = updated_lease_info
+        record_lease_state(timestamp, leases)
+        send_leases(leases)