launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1044206-get-new-leases-from-db into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)
Related bugs:
  Bug #1044206 in MAAS: "omshell task is never called when leases update"
  https://bugs.launchpad.net/maas/+bug/1044206

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1044206-get-new-leases-from-db/+merge/122440

I'm replacing an earlier approach, which went in the direction of the worker remembering its known leases and figuring out which ones were new.  I did originally complete that direction in another branch, but abandoned it for this simpler solution.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-1044206-get-new-leases-from-db/+merge/122440
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/bug-1044206-get-new-leases-from-db into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-08-24 12:52:34 +0000
+++ src/maasserver/api.py	2012-09-03 05:12:19 +0000
@@ -933,14 +933,12 @@
     @api_exported('POST')
     def update_leases(self, request, name):
         leases = get_mandatory_param(request.data, 'leases')
-        new_leases = request.data.get('new_leases', None)
         nodegroup = get_nodegroup_for_worker(request, name)
         leases = json.loads(leases)
-        DHCPLease.objects.update_leases(nodegroup, leases)
-        if new_leases is not None:
-            new_leases = json.loads(new_leases)
+        new_leases = DHCPLease.objects.update_leases(nodegroup, leases)
+        if len(new_leases) > 0:
             nodegroup.add_dhcp_host_maps(
-                {ip: mac for ip, mac in leases.items() if ip in new_leases})
+                {ip: leases[ip] for ip in new_leases if ip in leases})
         return HttpResponse("Leases updated.", status=httplib.OK)
 
 

=== modified file 'src/maasserver/models/dhcplease.py'
--- src/maasserver/models/dhcplease.py	2012-08-07 07:35:34 +0000
+++ src/maasserver/models/dhcplease.py	2012-09-03 05:12:19 +0000
@@ -71,6 +71,8 @@
         `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`.
+
+        :return: Iterable of newly-leased IP addresses.
         """
         leased_ips = self._get_leased_ips(nodegroup)
         new_leases = tuple(
@@ -84,6 +86,7 @@
                 INSERT INTO maasserver_dhcplease (nodegroup_id, ip, mac)
                 VALUES %s
                 """ % new_tuples)
+        return [ip for nodegroup_id, ip, mac in new_leases]
 
     def update_leases(self, nodegroup, leases):
         """Refresh our knowledge of a node group's IP mappings.
@@ -97,10 +100,12 @@
             addresses, values are MAC addresses.  Any :class:`DHCPLease`
             entries for `nodegroup` that are not in `leases` will be
             deleted.
+        :return: Iterable of IP addresses that were newly leased.
         """
         self._delete_obsolete_leases(nodegroup, leases)
-        self._add_missing_leases(nodegroup, leases)
+        new_leases = self._add_missing_leases(nodegroup, leases)
         post_updates.send(sender=self)
+        return new_leases
 
     def get_hostname_ip_mapping(self, nodegroup):
         """Return a mapping {hostnames -> ips} for the currently leased

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-08-27 10:37:14 +0000
+++ src/maasserver/tests/test_api.py	2012-09-03 05:12:19 +0000
@@ -2515,7 +2515,6 @@
             {
                 'op': 'update_leases',
                 'leases': json.dumps(new_leases),
-                'new_leases': json.dumps(new_leases.keys()),
             })
         self.assertEqual(
             (httplib.OK, "Leases updated."),
@@ -2525,6 +2524,7 @@
             Omshell.create.extract_args())
 
     def test_update_leases_does_not_add_old_leases(self):
+        self.patch(Omshell, 'create')
         enable_dhcp_management()
         nodegroup = factory.make_node_group()
         client = make_worker_client(nodegroup)
@@ -2534,7 +2534,6 @@
             {
                 'op': 'update_leases',
                 'leases': json.dumps(factory.make_random_leases()),
-                'new_leases': json.dumps([]),
             })
         self.assertEqual(
             (httplib.OK, "Leases updated."),

=== modified file 'src/maasserver/tests/test_dhcplease.py'
--- src/maasserver/tests/test_dhcplease.py	2012-08-20 07:28:41 +0000
+++ src/maasserver/tests/test_dhcplease.py	2012-09-03 05:12:19 +0000
@@ -63,6 +63,22 @@
         DHCPLease.objects.update_leases(nodegroup, lease)
         self.assertEqual(lease, map_leases(nodegroup))
 
+    def test_update_leases_returns_new_leases(self):
+        nodegroup = factory.make_node_group()
+        obsolete_lease = factory.make_dhcp_lease(nodegroup=nodegroup)
+        ignore_unused(obsolete_lease)
+        remaining_lease = factory.make_dhcp_lease(nodegroup=nodegroup)
+        new_lease = factory.make_random_leases()
+
+        surviving_leases = {
+            remaining_lease.ip: remaining_lease.mac,
+            new_lease.keys()[0]: new_lease.values()[0],
+        }
+
+        self.assertItemsEqual(
+            new_lease.keys(),
+            DHCPLease.objects.update_leases(nodegroup, surviving_leases))
+
     def test_update_leases_deletes_obsolete_lease(self):
         nodegroup = factory.make_node_group()
         factory.make_dhcp_lease(nodegroup=nodegroup)