From 9480ffc974f2f88d2a854e63d57556afcf2887e8 Mon Sep 17 00:00:00 2001
From: John Garbutt <john.garbutt@rackspace.com>
Date: Wed, 15 Jun 2016 10:43:11 +0100
Subject: [PATCH] Extract _update_ports_for_instance

To make allocate_for_instance easier to read, extract port update into
its own method called _update_ports_for_instance.

blueprint prep-for-network-aware-scheduling

Change-Id: I3ba46a831e1954de12a6c70a44113d2b75b874a2
---
 nova/network/neutronv2/api.py             |  72 +++++++++++----
 nova/tests/unit/network/test_neutronv2.py | 107 ++++++++++++++++++++--
 2 files changed, 153 insertions(+), 26 deletions(-)

diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py
index 3faa4c73faca..4acc275804a5 100644
--- a/nova/network/neutronv2/api.py
+++ b/nova/network/neutronv2/api.py
@@ -690,6 +690,9 @@ class API(base_api.NetworkAPI):
         # We do not want to create a new neutron session for each call
         neutron = get_client(context)
 
+        #
+        # Validate ports and networks with neutron
+        #
         requested_networks = kwargs.get('requested_networks')
         ports, ordered_networks = self._validate_requested_port_ids(
             context, instance, neutron, requested_networks)
@@ -716,13 +719,59 @@ class API(base_api.NetworkAPI):
         #
         # Update existing and newly created ports
         #
-        dhcp_opts = kwargs.get('dhcp_options', None)
+        dhcp_opts = kwargs.get('dhcp_options')
         bind_host_id = kwargs.get('bind_host_id')
 
         hypervisor_macs = kwargs.get('macs', None)
         available_macs = _filter_hypervisor_macs(instance, ports,
                                                  hypervisor_macs)
 
+        # We always need admin_client to build nw_info,
+        # we sometimes need it when updating ports
+        admin_client = get_client(context, admin=True)
+
+        ordered_nets, ordered_ports, preexisting_port_ids, \
+            created_port_ids = self._update_ports_for_instance(
+                context, instance,
+                neutron, admin_client, requests_and_created_ports, nets,
+                bind_host_id, dhcp_opts, available_macs)
+
+        #
+        # Perform a full update of the network_info_cache,
+        # including re-fetching lots of the required data from neutron
+        #
+        nw_info = self.get_instance_nw_info(
+            context, instance, networks=ordered_nets,
+            port_ids=ordered_ports,
+            admin_client=admin_client,
+            preexisting_port_ids=preexisting_port_ids,
+            update_cells=True)
+        # NOTE(danms): Only return info about ports we created in this run.
+        # In the initial allocation case, this will be everything we created,
+        # and in later runs will only be what was created that time. Thus,
+        # this only affects the attach case, not the original use for this
+        # method.
+        return network_model.NetworkInfo([vif for vif in nw_info
+                                          if vif['id'] in created_port_ids +
+                                          preexisting_port_ids])
+
+    def _update_ports_for_instance(self, context, instance, neutron,
+            admin_client, requests_and_created_ports, nets,
+            bind_host_id, dhcp_opts, available_macs):
+        """Create port for network_requests that don't have a port_id
+
+        :param context: The request context.
+        :param instance: nova.objects.instance.Instance object.
+        :param neutron: client using user context
+        :param admin_client: client using admin context
+        :param requests_and_created_ports: [(NetworkRequest, created_port_id)]
+        :param nets: a dict of network_id to networks returned from neutron
+        :param bind_host_id: a string for port['binding:host_id']
+        :param dhcp_opts: a list dicts that contain dhcp option name and value
+            e.g. [{'opt_name': 'tftp-server', 'opt_value': '1.2.3.4'}]
+        :param available_macs: a list of available mac addresses
+        """
+
         # The neutron client and port_client (either the admin context or
         # tenant context) are read here. The reason for this is that there are
         # a number of different calls for the instance allocation.
@@ -730,9 +779,7 @@ class API(base_api.NetworkAPI):
         port_client = (neutron if not
                        self._has_port_binding_extension(context,
                            refresh_cache=True, neutron=neutron) else
-                       get_client(context, admin=True))
-        # Store the admin client - this is used later
-        admin_client = port_client if neutron != port_client else None
+                       admin_client)
 
         preexisting_port_ids = []
         created_port_ids = []
@@ -803,20 +850,9 @@ class API(base_api.NetworkAPI):
                                        preexisting_port_ids,
                                        neutron, port_client)
                     self._delete_ports(neutron, instance, created_port_ids)
-        nw_info = self.get_instance_nw_info(
-            context, instance, networks=nets_in_requested_order,
-            port_ids=ports_in_requested_order,
-            admin_client=admin_client,
-            preexisting_port_ids=preexisting_port_ids,
-            update_cells=True)
-        # NOTE(danms): Only return info about ports we created in this run.
-        # In the initial allocation case, this will be everything we created,
-        # and in later runs will only be what was created that time. Thus,
-        # this only affects the attach case, not the original use for this
-        # method.
-        return network_model.NetworkInfo([vif for vif in nw_info
-                                          if vif['id'] in created_port_ids +
-                                          preexisting_port_ids])
+
+        return (nets_in_requested_order, ports_in_requested_order,
+            preexisting_port_ids, created_port_ids)
 
     def _refresh_neutron_extensions_cache(self, context, neutron=None):
         """Refresh the neutron extensions cache when necessary."""
diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py
index b43a7614adb2..287626f89934 100644
--- a/nova/tests/unit/network/test_neutronv2.py
+++ b/nova/tests/unit/network/test_neutronv2.py
@@ -540,7 +540,7 @@ class TestNeutronv2Base(test.TestCase):
                                  self.instance,
                                  networks=nets_in_requested_net_order,
                                  port_ids=ports_in_requested_net_order,
-                                 admin_client=None,
+                                 admin_client=self.moxed_client,
                                  preexisting_port_ids=preexisting_port_ids,
                                  update_cells=True
                                 ).AndReturn(self._returned_nw_info)
@@ -549,16 +549,17 @@ class TestNeutronv2Base(test.TestCase):
 
     def _stub_allocate_for_instance_port_binding(self, api, portbinding,
             has_dns_extension):
+        if portbinding:
+            neutronapi.get_client(mox.IgnoreArg()).AndReturn(
+                self.moxed_client)
+        neutronapi.get_client(
+            mox.IgnoreArg(), admin=True).AndReturn(
+            self.moxed_client)
         has_portbinding = False
         if portbinding:
             has_portbinding = True
             api.extensions[constants.PORTBINDING_EXT] = 1
             self.mox.StubOutWithMock(api, '_refresh_neutron_extensions_cache')
-            neutronapi.get_client(mox.IgnoreArg()).AndReturn(
-                self.moxed_client)
-            neutronapi.get_client(
-                mox.IgnoreArg(), admin=True).AndReturn(
-                self.moxed_client)
             api._refresh_neutron_extensions_cache(mox.IgnoreArg(),
                 neutron=self.moxed_client)
             self.mox.StubOutWithMock(api, '_has_port_binding_extension')
@@ -1264,6 +1265,9 @@ class TestNeutronv2(TestNeutronv2Base):
             (request, ('portid_' + request.network_id))
             for request in requested_networks
         ]
+        neutronapi.get_client(
+            mox.IgnoreArg(), admin=True).AndReturn(
+            self.moxed_client)
         index = 0
         for network in self.nets2:
             binding_port_req_body = {
@@ -4285,13 +4289,55 @@ class TestNeutronv2ExtraDhcpOpts(TestNeutronv2Base):
                                     dhcp_options=dhcp_opts)
 
 
-class TestAllocateForInstanceHelpers(test.NoDBTestCase):
+class TestAllocateForInstance(test.NoDBTestCase):
     def setUp(self):
-        super(TestAllocateForInstanceHelpers, self).setUp()
+        super(TestAllocateForInstance, self).setUp()
         self.context = context.RequestContext('userid', uuids.my_tenant)
         self.instance = objects.Instance(uuid=uuids.instance,
             project_id=uuids.tenant_id, hostname="host")
 
+    def test_allocate_for_instance_raises_invalid_input(self):
+        api = neutronapi.API()
+        self.instance.project_id = ""
+
+        self.assertRaises(exception.InvalidInput,
+            api.allocate_for_instance, self.context, self.instance)
+
+    @mock.patch.object(neutronapi.API, 'get_instance_nw_info')
+    @mock.patch.object(neutronapi.API, '_update_ports_for_instance')
+    @mock.patch.object(neutronapi, '_filter_hypervisor_macs')
+    @mock.patch.object(neutronapi.API, '_create_ports_for_instance')
+    @mock.patch.object(neutronapi.API, '_process_security_groups')
+    @mock.patch.object(neutronapi.API, '_clean_security_groups')
+    @mock.patch.object(neutronapi.API, '_validate_requested_network_ids')
+    @mock.patch.object(neutronapi.API, '_validate_requested_port_ids')
+    @mock.patch.object(neutronapi, 'get_client')
+    def test_allocate_for_instance_minimal_args(self, mock_get_client,
+            mock_validate_ports, mock_validate_nets, mock_clean_sg, mock_sg,
+            mock_create_ports, mock_filter_macs, mock_update_ports, mock_gni):
+
+        api = neutronapi.API()
+        mock_get_client.side_effect = ["user", "admin"]
+        mock_validate_ports.return_value = ("ports", "ordered_nets")
+        mock_validate_nets.return_value = "nets"
+        mock_clean_sg.return_value = "security_groups"
+        mock_sg.return_value = "security_group_ids"
+        mock_create_ports.return_value = "requests_and_created_ports"
+        mock_filter_macs.return_value = "available_macs"
+        mock_update_ports.return_value = (
+            "nets", "ports", [uuids.preexist], [uuids.created])
+        mock_gni.return_value = [
+            {"id": uuids.created}, {"id": uuids.preexist}, {"id": "foo"}
+        ]
+
+        result = api.allocate_for_instance(self.context, self.instance)
+
+        # TODO(johngarbutt) we need to replace the old mox coverage
+        # with new tests that can build on this very poor test
+        self.assertEqual(len(result), 2)
+        self.assertEqual(result[0], {"id": uuids.created})
+        self.assertEqual(result[1], {"id": uuids.preexist})
+
     def test_populate_mac_address_skip_if_none(self):
         api = neutronapi.API()
         port_req_body = {}
@@ -4613,6 +4659,51 @@ class TestAllocateForInstanceHelpers(test.NoDBTestCase):
 
         self.assertFalse(mock_client.create_port.called)
 
+    @mock.patch.object(objects.VirtualInterface, "create")
+    def test_update_ports_for_instance_with_portbinding(self, mock_create):
+        api = neutronapi.API()
+        self.instance.availability_zone = "test_az"
+        mock_neutron = mock.Mock()
+        mock_admin = mock.Mock()
+        requests_and_created_ports = [
+            (objects.NetworkRequest(
+                network_id=uuids.net1), uuids.port1),
+            (objects.NetworkRequest(
+                network_id=uuids.net2, port_id=uuids.port2), None)]
+        net1 = {"id": uuids.net1}
+        net2 = {"id": uuids.net2}
+        nets = {uuids.net1: net1, uuids.net2: net2}
+        bind_host_id = "bind_host_id"
+        dhcp_opts = [{'opt_name': 'tftp-server', 'opt_value': '1.2.3.4'}]
+        available_macs = ["mac1", "mac2"]
+
+        mock_neutron.list_extensions.return_value = {"extensions": [
+            {"name": "asdf"}, {"name": constants.PORTBINDING_EXT}]}
+        port1 = {"port": {"id": uuids.port1, "mac_address": "mac1r"}}
+        port2 = {"port": {"id": uuids.port2, "mac_address": "mac2r"}}
+        mock_admin.update_port.side_effect = [port1, port2]
+
+        ordered_nets, ordered_ports, preexisting_port_ids, \
+            created_port_ids = api._update_ports_for_instance(
+                self.context, self.instance,
+                mock_neutron, mock_admin, requests_and_created_ports, nets,
+                bind_host_id, dhcp_opts, available_macs)
+
+        # TODO(johngarbutt) need to build on this test so we can replace
+        # all the mox based tests
+        self.assertEqual([net1, net2], ordered_nets, "ordered_nets")
+        self.assertEqual([uuids.port1, uuids.port2], ordered_ports,
+            "ordered_ports")
+        self.assertEqual([uuids.port2], preexisting_port_ids, "preexisting")
+        self.assertEqual([uuids.port1], created_port_ids, "created")
+        mock_admin.update_port.assert_called_with(uuids.port2,
+            {'port': {
+                'device_owner': 'compute:test_az',
+                'mac_address': 'mac1',
+                'binding:host_id': bind_host_id,
+                'extra_dhcp_opts': dhcp_opts,
+                'device_id': self.instance.uuid}})
+
 
 class TestNeutronv2NeutronHostnameDNS(TestNeutronv2Base):
     def setUp(self):