launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/add_missing_leases-return-value into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/add_missing_leases-return-value/+merge/118464

This is the first part of a change pre-imped with Julian.  It's small and straightforward.  When we update a node group's leases, one step is to insert new leases into the database.  We'll need to register those incrementally with OMShell, for which purpose we must know what leases have changed.  As per the pre-imp, this does not apply to the removal of leases — that's entirely driven by the MAAS server, so a lease can never disappear at the DHCP server's initiative.

There is no particular reason why I made this return a dict (as opposed to, say, a sequence of IP addresses) other than consistency.  It's nice to have only one type of “bunch of lease descriptions” to worry about besides sequences/containers of DHCPLease objects.  I don't think we'll be needing the MAC addresses, but it'll be easy enough to strip this down later.  Normally I'd oppose fetching the data for the hell of it, but here it really doesn't complicate things at all.  Size of the dict might be a consideration, if it weren't a subset of one we're already handling.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/add_missing_leases-return-value/+merge/118464
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/add_missing_leases-return-value into lp:maas.

=== modified file 'src/maasserver/models/dhcplease.py'
--- src/maasserver/models/dhcplease.py	2012-07-25 15:25:42 +0000
+++ src/maasserver/models/dhcplease.py	2012-08-07 03:09:18 +0000
@@ -71,19 +71,30 @@
         `nodegroup` has a DHCPLease with the same `ip` field.  There
         can't be any DHCPLease entries with the same `ip` as in `leases`
         but a different `mac`.
+
+        :param nodegroup: :class:`NodeGroup` whose leases are being updated.
+        :param leases: A dict describing all current IP/MAC mappings as
+            managed by the node group's DHCP server.  Keys are IP
+            addresses, values are MAC addresses.
+        :return: A dict of leases (also mapping IP addresses to MAC addresses)
+            that have been newly created.  This will be a subset of `leases`.
         """
         leased_ips = self._get_leased_ips(nodegroup)
         new_leases = tuple(
             (nodegroup.id, ip, mac)
             for ip, mac in leases.items() if ip not in leased_ips)
-        if len(new_leases) > 0:
-            cursor = connection.cursor()
-            new_tuples = ", ".join(
-                cursor.mogrify("%s", [lease]) for lease in new_leases)
-            cursor.execute("""
-                INSERT INTO maasserver_dhcplease (nodegroup_id, ip, mac)
-                VALUES %s
-                """ % new_tuples)
+        if len(new_leases) == 0:
+            return {}
+
+        cursor = connection.cursor()
+        new_tuples = ", ".join(
+            cursor.mogrify("%s", [lease]) for lease in new_leases)
+        cursor.execute("""
+            INSERT INTO maasserver_dhcplease (nodegroup_id, ip, mac)
+            VALUES %s
+            RETURNING ip, mac
+            """ % new_tuples)
+        return dict(cursor.fetchall())
 
     def update_leases(self, nodegroup, leases):
         """Refresh our knowledge of a node group's IP mappings.

=== modified file 'src/maasserver/tests/test_dhcplease.py'
--- src/maasserver/tests/test_dhcplease.py	2012-07-25 15:25:42 +0000
+++ src/maasserver/tests/test_dhcplease.py	2012-08-07 03:09:18 +0000
@@ -150,6 +150,36 @@
         DHCPLease.objects.update_leases(nodegroup, {})
         self.assertEqual(1, recorder.call_count)
 
+    def test_add_missing_leases_returns_new_leases(self):
+        nodegroup = factory.make_node_group()
+        new_lease = {
+            factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+        }
+        self.assertEqual(
+            new_lease.copy(),
+            DHCPLease.objects._add_missing_leases(
+                nodegroup, new_lease.copy()))
+
+    def test_add_missing_leases_returns_empty_dict_if_no_new_leases(self):
+        nodegroup = factory.make_node_group()
+        old_lease = factory.make_dhcp_lease(nodegroup)
+        self.assertEqual(
+            {},
+            DHCPLease.objects._add_missing_leases(
+                nodegroup, {old_lease.ip: old_lease.mac}))
+
+    def test_add_missing_leases_ignores_removed_leases(self):
+        nodegroup = factory.make_node_group()
+        old_lease = factory.make_dhcp_lease(nodegroup)
+        ignore_unused(old_lease)
+        new_lease = {
+            factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+        }
+        self.assertEqual(
+            new_lease.copy(),
+            DHCPLease.objects._add_missing_leases(
+                nodegroup, new_lease.copy()))
+
     def test_get_hostname_ip_mapping_returns_mapping(self):
         nodegroup = factory.make_node_group()
         expected_mapping = {}