Merge lp:~julian-edwards/maas/allocate-ip-on-start-2 into lp:~maas-committers/maas/trunk

[Julian Edwards]

=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py	2014-06-10 14:45:14 +0000
+++ src/maasserver/models/__init__.py	2014-06-11 05:52:49 +0000
@@ -51,7 +51,6 @@
 from maasserver.models.dhcplease import DHCPLease
 from maasserver.models.downloadprogress import DownloadProgress
 from maasserver.models.filestorage import FileStorage
-from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.models.macaddress import MACAddress
 from maasserver.models.macipaddresslink import MACStaticIPAddressLink
 from maasserver.models.network import Network
@@ -59,6 +58,7 @@
 from maasserver.models.nodegroup import NodeGroup
 from maasserver.models.nodegroupinterface import NodeGroupInterface
 from maasserver.models.sshkey import SSHKey
+from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.models.tag import Tag
 from maasserver.models.user import create_user
 from maasserver.models.userprofile import UserProfile
 
=== modified file 'src/maasserver/models/macaddress.py'
--- src/maasserver/models/macaddress.py	2014-06-10 14:45:14 +0000
+++ src/maasserver/models/macaddress.py	2014-06-11 05:52:49 +0000
@@ -90,7 +90,10 @@
     def claim_static_ip(self, alloc_type=IPADDRESS_TYPE.AUTO):
         """Assign a static IP to this MAC.
 
-        TODO: Also set a host DHCP entry.
+        It is the caller's responsibility to create a celery Task that will
+        write the dhcp host.  It is not done here because celery doesn't
+        guarantee job ordering, and if the host entry is written after
+        the host boots it is too late.
 
         :param alloc_type: See :class:`StaticIPAddress`.alloc_type.
         :return: A :class:`StaticIPAddress` object. Returns None if
 
=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-06-10 15:26:47 +0000
+++ src/maasserver/models/node.py	2014-06-11 05:52:49 +0000
@@ -28,6 +28,7 @@
 from string import whitespace
 from uuid import uuid1
 
+import celery
 from django.contrib.auth.models import User
 from django.core.exceptions import (
     PermissionDenied,
@@ -54,16 +55,20 @@
     NODE_STATUS,
     NODE_STATUS_CHOICES,
     NODE_STATUS_CHOICES_DICT,
+    NODEGROUPINTERFACE_MANAGEMENT,
     )
 from maasserver.exceptions import NodeStateViolation
 from maasserver.fields import (
     JSONObjectField,
     MAC,
     )
-from maasserver.models import StaticIPAddress
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.config import Config
 from maasserver.models.dhcplease import DHCPLease
+from maasserver.models.staticipaddress import (
+    StaticIPAddress,
+    StaticIPAddressExhaustion,
+    )
 from maasserver.models.tag import Tag
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.models.zone import Zone
@@ -74,6 +79,7 @@
 from piston.models import Token
 from provisioningserver.drivers.osystem import OperatingSystemRegistry
 from provisioningserver.tasks import (
+    add_new_dhcp_host_map,
     power_off,
     power_on,
     remove_dhcp_host_map,
@@ -400,9 +406,24 @@
             else:
                 do_start = True
             if do_start:
-                power_on.apply_async(
-                    queue=node.work_queue, args=[node_power_type],
-                    kwargs=power_params)
+                try:
+                    tasks = node.claim_static_ips()
+                except StaticIPAddressExhaustion:
+                    # TODO: send error back to user, or fall back to a
+                    # dynamic IP?
+                    logger.error(
+                        "Node %s: Unable to allocate static IP due to address"
+                        " exhaustion." % node.system_id)
+                    continue
+
+                task = power_on.si(node_power_type, **power_params)
+                task.set(queue=node.work_queue)
+                tasks.append(task)
+                chained_tasks = celery.chain(tasks)
+                chained_tasks.apply_async()
+                # TODO: if any of this fails it needs to release the
+                # static IPs back to the pool.  As part of the robustness
+                # work coming up, it also needs to inform the user.
                 processed_nodes.append(node)
         return processed_nodes
 
@@ -566,6 +587,49 @@
         else:
             return self.hostname
 
+    def claim_static_ips(self):
+        """Assign static IPs for our MACs and return an array of Celery tasks
+        that need executing.  If nothing needs executing, the empty array
+        is returned.
+
+        Each MAC on the node that is connected to a managed cluster
+        interface will get an IP.
+
+        This operation is atomic, claiming an IP on a particular MAC fails
+        then none of the MACs will get an IP and StaticIPAddressExhaustion
+        is raised.
+        """
+        # TODO: Release claimed MACs inside loop if fail to claim single
+        # one (ie make this atomic).
+        tasks = []
+        # Get a new AUTO static IP for each MAC on a managed interface.
+        macs = self.mac_addresses_on_managed_interfaces()
+        for mac in macs:
+            sip = mac.claim_static_ip()
+            # This is creating an array of celery 'Signatures' which will be
+            # chained together later.  We make the Signatures immutable
+            # otherwise the result of the previous in the chain is passed to
+            # the next, this is done with the "si()" call.
+            # See docs.celeryproject.org/en/latest/userguide/canvas.html
+
+            # Note that this may be None if the static range is not yet
+            # defined, which will be the case when migrating from older
+            # versions of the code.
+            if sip is not None:
+                # Delete any existing dynamic maps first.
+                del_existing = self._build_dynamic_host_map_deletion_task()
+                if del_existing is not None:
+                    # del_existing is a chain so does not need an explicit
+                    # queue to be set as each subtask will have one.
+                    tasks.append(del_existing)
+                dhcp_key = self.nodegroup.dhcp_key
+                mapping = {sip.ip: mac.mac_address}
+                dhcp_task = add_new_dhcp_host_map.si(
+                    mapping, '127.0.0.1', dhcp_key)
+                dhcp_task.set(queue=self.work_queue)
+                tasks.append(dhcp_task)
+        return tasks
+
     def ip_addresses(self):
         """IP addresses allocated to this node.
 
@@ -632,6 +696,16 @@
             query = dhcpleases_qs.filter(mac__in=macs)
             return query.values_list('ip', flat=True)
 
+    def mac_addresses_on_managed_interfaces(self):
+        """Return MACAddresses for this node that have a managed cluster
+        interface."""
+        # Avoid circular imports
+        from maasserver.models import MACAddress
+        unmanaged = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
+        return MACAddress.objects.filter(
+            node=self, cluster_interface__isnull=False).exclude(
+            cluster_interface__management=unmanaged)
+
     def tag_names(self):
         # We don't use self.tags.values_list here because this does not
         # take advantage of the cache.
@@ -761,6 +835,21 @@
             raise NodeStateViolation(
                 "Cannot delete node %s: node is in state %s."
                 % (self.system_id, NODE_STATUS_CHOICES_DICT[self.status]))
+        # Delete any dynamic host maps in the DHCP server.
+        self._delete_dynamic_host_maps()
+        # Delete the related mac addresses.
+        # The DHCPLease objects corresponding to these MACs will be deleted
+        # as well. See maasserver/models/dhcplease:delete_lease().
+        self.macaddress_set.all().delete()
+
+        super(Node, self).delete()
+
+    def _build_dynamic_host_map_deletion_task(self):
+        """Create a chained celery task that will delete dhcp host maps.
+
+        Return None if there is nothing to delete.
+        """
+        tasks = []
         nodegroup = self.nodegroup
         if len(nodegroup.get_managed_interfaces()) > 0:
             # Delete the host map(s) in the DHCP server.
@@ -772,14 +861,19 @@
                     ip_address=lease.ip,
                     server_address="127.0.0.1",
                     omapi_key=nodegroup.dhcp_key)
-                remove_dhcp_host_map.apply_async(
-                    queue=nodegroup.uuid, kwargs=task_kwargs)
-        # Delete the related mac addresses.
-        # The DHCPLease objects corresponding to these MACs will be deleted
-        # as well. See maasserver/models/dhcplease:delete_lease().
-        self.macaddress_set.all().delete()
+                task = remove_dhcp_host_map.si(**task_kwargs)
+                task.set(queue=self.work_queue)
+                tasks.append(task)
+        if len(tasks) > 0:
+            return celery.chain(tasks)
+        return None
 
-        super(Node, self).delete()
+    def _delete_dynamic_host_maps(self):
+        """If any DHCPLeases exist for this node, remove any associated
+        host maps."""
+        chain = self._build_dynamic_host_map_deletion_task()
+        if chain is not None:
+            chain.apply_async()
 
     def set_random_hostname(self):
         """Set 5 character `hostname` using non-ambiguous characters.
 
=== modified file 'src/maasserver/models/staticipaddress.py'
--- src/maasserver/models/staticipaddress.py	2014-06-05 01:28:35 +0000
+++ src/maasserver/models/staticipaddress.py	2014-06-11 05:52:49 +0000
@@ -120,7 +120,7 @@
         # __iter__ does not work here for some reason, so using
         # iteritems().
         # XXX: convert this into a reverse_map_enum in maasserver.utils.
-        for k,v in IPADDRESS_TYPE.__dict__.iteritems():
+        for k, v in IPADDRESS_TYPE.__dict__.iteritems():
             if v == self.alloc_type:
                 strtype = k
                 break
@@ -135,4 +135,3 @@
         After return, this object is no longer valid.
         """
         self.delete()
-
 
=== modified file 'src/maasserver/models/tests/test_macaddress.py'
--- src/maasserver/models/tests/test_macaddress.py	2014-06-10 14:45:14 +0000
+++ src/maasserver/models/tests/test_macaddress.py	2014-06-11 05:52:49 +0000
@@ -84,25 +84,13 @@
 
 class TestMACAddressForStaticIPClaiming(MAASServerTestCase):
 
-    def make_node_with_mac_attached_to_nodegroupinterface(self):
-        nodegroup = factory.make_node_group()
-        node = factory.make_node(mac=True, nodegroup=nodegroup)
-        low_ip, high_ip = factory.make_ip_range()
-        ngi = factory.make_node_group_interface(
-            nodegroup, static_ip_range_low=low_ip.ipv4().format(),
-            static_ip_range_high=high_ip.ipv4().format())
-        mac = node.get_primary_mac()
-        mac.cluster_interface = ngi
-        mac.save()
-        return node
-
     def test_claim_static_ip_returns_none_if_no_cluster_interface(self):
         # If mac.cluster_interface is None, we can't allocate any IP.
         mac = factory.make_mac_address()
         self.assertIsNone(mac.claim_static_ip())
 
     def test_claim_static_ip_reserves_an_ip_address(self):
-        node = self.make_node_with_mac_attached_to_nodegroupinterface()
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
         claimed_ip = mac.claim_static_ip()
         self.assertIsInstance(claimed_ip, StaticIPAddress)
@@ -111,13 +99,13 @@
             IPADDRESS_TYPE.AUTO, StaticIPAddress.objects.all()[0].alloc_type)
 
     def test_claim_static_ip_sets_type_as_required(self):
-        node = self.make_node_with_mac_attached_to_nodegroupinterface()
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
         claimed_ip = mac.claim_static_ip(alloc_type=IPADDRESS_TYPE.STICKY)
         self.assertEqual(IPADDRESS_TYPE.STICKY, claimed_ip.alloc_type)
 
     def test_claim_static_ip_returns_none_if_no_static_range_defined(self):
-        node = self.make_node_with_mac_attached_to_nodegroupinterface()
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
         mac.cluster_interface.static_ip_range_low = None
         mac.cluster_interface.static_ip_range_high = None
 
=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-06-10 14:45:14 +0000
+++ src/maasserver/models/tests/test_node.py	2014-06-11 05:52:49 +0000
@@ -17,6 +17,7 @@
 from datetime import timedelta
 import random
 
+import celery
 from django.core.exceptions import ValidationError
 from maasserver.clusterrpc.power_parameters import get_power_types
 from maasserver.enum import (
@@ -55,12 +56,12 @@
     NodeUserData,
     )
 from provisioningserver.power.poweraction import PowerAction
+from provisioningserver.tasks import Omshell
 from testtools.matchers import (
     AllMatch,
     Contains,
     Equals,
     MatchesAll,
-    MatchesListwise,
     Not,
     )
 
@@ -294,19 +295,26 @@
         lease = factory.make_dhcp_lease()
         node = factory.make_node(nodegroup=lease.nodegroup)
         node.add_mac_address(lease.mac)
-        mocked_task = self.patch(node_module, "remove_dhcp_host_map")
-        mocked_apply_async = self.patch(mocked_task, "apply_async")
+        self.patch(Omshell, 'remove')
         node.delete()
-        args, kwargs = mocked_apply_async.call_args
-        expected = (
-            Equals(kwargs['queue']),
+        self.assertThat(
+            self.celery.tasks[0]['kwargs'],
             Equals({
                 'ip_address': lease.ip,
                 'server_address': "127.0.0.1",
                 'omapi_key': lease.nodegroup.dhcp_key,
                 }))
-        observed = node.work_queue, kwargs['kwargs']
-        self.assertThat(observed, MatchesListwise(expected))
+
+    def test_delete_dynamic_host_maps_sends_to_correct_queue(self):
+        lease = factory.make_dhcp_lease()
+        node = factory.make_node(nodegroup=lease.nodegroup)
+        node.add_mac_address(lease.mac)
+        self.patch(Omshell, 'remove')
+        option_call = self.patch(celery.canvas.Signature, 'set')
+        work_queue = node.work_queue
+        node.delete()
+        args, kwargs = option_call.call_args
+        self.assertEqual(work_queue, kwargs['queue'])
 
     def test_delete_node_removes_multiple_host_maps(self):
         lease1 = factory.make_dhcp_lease()
@@ -314,10 +322,9 @@
         node = factory.make_node(nodegroup=lease1.nodegroup)
         node.add_mac_address(lease1.mac)
         node.add_mac_address(lease2.mac)
-        mocked_task = self.patch(node_module, "remove_dhcp_host_map")
-        mocked_apply_async = self.patch(mocked_task, "apply_async")
+        self.patch(Omshell, 'remove')
         node.delete()
-        self.assertEqual(2, mocked_apply_async.call_count)
+        self.assertEqual(2, len(self.celery.tasks))
 
     def test_set_random_hostname_set_hostname(self):
         # Blank out enlistment_domain.
@@ -986,6 +993,31 @@
         node = factory.make_node(architecture=full_arch)
         self.assertEqual((main_arch, sub_arch), node.split_arch())
 
+    def test_mac_addresses_on_managed_interfaces_returns_only_managed(self):
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface()
+        primary_cluster_interface = node.get_primary_mac().cluster_interface
+        primary_cluster_interface.management = (
+            NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+        primary_cluster_interface.save()
+
+        mac_with_no_interface = factory.make_mac_address(node=node)
+        mac_with_no_interface = mac_with_no_interface  # STFU linter
+        unmanaged_interface = factory.make_node_group_interface(
+            nodegroup=node.nodegroup,
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+
+        mac_with_unmanaged_interface = factory.make_mac_address(
+            node=node, cluster_interface=unmanaged_interface)
+        mac_with_unmanaged_interface = mac_with_unmanaged_interface  # linter
+
+        observed = node.mac_addresses_on_managed_interfaces()
+        self.assertItemsEqual([node.get_primary_mac()], observed)
+
+    def test_mac_addresses_on_managed_interfaces_returns_empty_if_none(self):
+        node = factory.make_node(mac=True)
+        observed = node.mac_addresses_on_managed_interfaces()
+        self.assertItemsEqual([], observed)
+
 
 class NodeRoutersTest(MAASServerTestCase):
 
@@ -1274,13 +1306,69 @@
                 self.celery.tasks[0]['kwargs']['mac_address'],
             ))
 
+    def test_start_nodes_issues_dhcp_host_task(self):
+        user = factory.make_user()
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface(
+            owner=user, power_type='ether_wake')
+        omshell_create = self.patch(Omshell, 'create')
+        output = Node.objects.start_nodes([node.system_id], user)
+
+        # Check that the single node was started, and that the tasks
+        # issued are all there and in the right order.
+        self.assertItemsEqual([node], output)
+        self.assertEqual(
+            [
+                'provisioningserver.tasks.add_new_dhcp_host_map',
+                'provisioningserver.tasks.power_on',
+            ],
+            [
+                task['task'].name for task in self.celery.tasks
+            ])
+
+        # Also check that Omshell.create() was called with the right
+        # parameters.
+        mac = node.get_primary_mac()
+        [ip] = mac.ip_addresses.all()
+        expected_ip = ip.ip
+        expected_mac = mac.mac_address
+        args, kwargs = omshell_create.call_args
+        self.assertEqual((expected_ip, expected_mac), args)
+
+    def test_start_nodes_clears_existing_dynamic_maps(self):
+        user = factory.make_user()
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface(
+            owner=user, power_type='ether_wake')
+        factory.make_dhcp_lease(
+            nodegroup=node.nodegroup, mac=node.get_primary_mac().mac_address)
+        self.patch(Omshell, 'create')
+        self.patch(Omshell, 'remove')
+        output = Node.objects.start_nodes([node.system_id], user)
+
+        # Check that the single node was started, and that the tasks
+        # issued are all there and in the right order.
+        self.assertItemsEqual([node], output)
+        self.assertEqual(
+            [
+                'provisioningserver.tasks.remove_dhcp_host_map',
+                'provisioningserver.tasks.add_new_dhcp_host_map',
+                'provisioningserver.tasks.power_on',
+            ],
+            [
+                task['task'].name for task in self.celery.tasks
+            ])
+
     def test_start_nodes_task_routed_to_nodegroup_worker(self):
+        # Startup jobs are chained, so the normal way of inspecting a
+        # task directly for routing options doesn't work here, because
+        # in EAGER mode that we use in the test suite, the options are
+        # not passed all the way down to the tasks.  Instead, we patch
+        # some celery code to inspect the options that were passed.
         user = factory.make_user()
         node, mac = self.make_node_with_mac(
             user, power_type='ether_wake')
-        task = self.patch(node_module, 'power_on')
+        option_call = self.patch(celery.canvas.Signature, 'set')
         Node.objects.start_nodes([node.system_id], user)
-        args, kwargs = task.apply_async.call_args
+        args, kwargs = option_call.call_args
         self.assertEqual(node.work_queue, kwargs['queue'])
 
     def test_start_nodes_does_not_attempt_power_task_if_no_power_type(self):
@@ -1401,3 +1489,67 @@
         node = factory.make_node(netboot=True)
         node.set_netboot(False)
         self.assertFalse(node.netboot)
+
+    def test_claim_static_ips_ignores_unmanaged_macs(self):
+        node = factory.make_node()
+        for _ in range(0, 10):
+            factory.make_mac_address(node=node)
+        observed = node.claim_static_ips()
+        self.assertItemsEqual([], observed)
+
+    def test_claim_static_ips_creates_task_for_each_managed_mac(self):
+        nodegroup = factory.make_node_group()
+        node = factory.make_node(nodegroup=nodegroup)
+
+        # Add a bunch of MACs attached to managed interfaces.
+        for _ in range(0, 10):
+            low_ip, high_ip = factory.make_ip_range()
+            ngi = factory.make_node_group_interface(
+                nodegroup, static_ip_range_low=low_ip.ipv4().format(),
+                static_ip_range_high=high_ip.ipv4().format(),
+                management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+            mac = factory.make_mac_address(node=node)
+            mac.cluster_interface = ngi
+            mac.save()
+
+        observed = node.claim_static_ips()
+        expected = ['provisioningserver.tasks.add_new_dhcp_host_map'] * 10
+
+        self.assertEqual(
+            expected,
+            [task.task for task in observed]
+            )
+
+    def test_claim_static_ips_creates_deletion_task(self):
+        # If dhcp leases exist before creating a static IP, the code
+        # should attempt to remove their host maps.
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface()
+        factory.make_dhcp_lease(
+            nodegroup=node.nodegroup, mac=node.get_primary_mac().mac_address)
+
+        observed = node.claim_static_ips()
+
+        self.assertEqual(
+            [
+                'celery.chain',
+                'provisioningserver.tasks.add_new_dhcp_host_map',
+            ],
+            [
+                task.task for task in observed
+            ])
+
+        # Probe the chain to make sure it has the deletion task.
+        self.assertEqual(
+            'provisioningserver.tasks.remove_dhcp_host_map',
+            observed[0].tasks[0].task,
+            )
+
+    def test_claim_static_ips_ignores_interface_with_no_static_range(self):
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface()
+        ngi = node.get_primary_mac().cluster_interface
+        ngi.static_ip_range_low = None
+        ngi.static_ip_range_high = None
+        ngi.save()
+
+        observed = node.claim_static_ips()
+        self.assertItemsEqual([], observed)
 
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2014-06-11 04:38:41 +0000
+++ src/maasserver/testing/factory.py	2014-06-11 05:52:49 +0000
@@ -354,18 +354,37 @@
         """Generate a random MAC address, in the form of a MAC object."""
         return MAC(self.getRandomMACAddress())
 
-    def make_mac_address(self, address=None, node=None, networks=None):
+    def make_mac_address(self, address=None, node=None, networks=None,
+                         **kwargs):
         """Create a MACAddress model object."""
         if node is None:
             node = self.make_node()
         if address is None:
             address = self.getRandomMACAddress()
-        mac = MACAddress(mac_address=MAC(address), node=node)
+        mac = MACAddress(mac_address=MAC(address), node=node, **kwargs)
         mac.save()
         if networks is not None:
             mac.networks.add(*networks)
         return mac
 
+    def make_node_with_mac_attached_to_nodegroupinterface(self, **kwargs):
+        """Create a Node that has a MACAddress which has a
+        NodeGroupInterface.
+
+        :param **kwargs: Additional parameters to pass to make_node.
+        """
+        nodegroup = self.make_node_group()
+        node = self.make_node(mac=True, nodegroup=nodegroup, **kwargs)
+        low_ip, high_ip = factory.make_ip_range()
+        ngi = self.make_node_group_interface(
+            nodegroup, static_ip_range_low=low_ip.ipv4().format(),
+            static_ip_range_high=high_ip.ipv4().format(),
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+        mac = node.get_primary_mac()
+        mac.cluster_interface = ngi
+        mac.save()
+        return node
+
     def make_staticipaddress(self, ip=None, alloc_type=IPADDRESS_TYPE.AUTO,
                              mac=None):
         """Create and return a StaticIPAddress model object.