launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/reverse-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/reverse-add_missing_leases-return-value/+merge/118504

I'm working on OMShell updates for new DHCP leases.  The plan was to have the app-server code that processes lease updates to notify OMShell as well.  But the eternal confusion between what's DHCP and what's DNS was hiding a mistake: the updates needed to happen on the node-group worker whose DHCP server issued the new leases in the first place.  There's just no point letting that information flow up to the central server and then back to the worker through rabbit!

Instead, we will make the omshell updates in the worker _in addition to_ sending the latest leases to the server.  There is no particular ordering between the two, so we can use subtasks etc.  But it does mean that my branch at https://code.launchpad.net/~jtv/maas/add_missing_leases-return-value/+merge/118464 is no longer needed, and hence this reverse merge.


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

=== modified file 'src/maasserver/models/dhcplease.py'
--- src/maasserver/models/dhcplease.py	2012-08-07 03:02:02 +0000
+++ src/maasserver/models/dhcplease.py	2012-08-07 08:18:18 +0000
@@ -71,30 +71,19 @@
         `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:
-            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())
+        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)
 
     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-08-07 03:02:02 +0000
+++ src/maasserver/tests/test_dhcplease.py	2012-08-07 08:18:18 +0000
@@ -150,36 +150,6 @@
         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 = {}