From ee7858ffcaf3f874feb0ea0c68871f2c93254fdd Mon Sep 17 00:00:00 2001 From: Moshe Levi Date: Wed, 20 Sep 2017 15:24:29 +0300 Subject: [PATCH] Don't overwrite binding-profile Currently when providing existing direct port, nova-compute will overwrite the binding-profile information with pci_vendor_info and pci_slot. The binding-profile will be used to request NIC capabilities for SR-IOV ports [1]. This also allows to distinguish which neutron mechanism driver will bind the port [2]. This patch updates the behaviour that on update port it will update, rather than overwrite, the binding-profile information with pci_vendor_info and pci_slot. And on unbind port it will remove only the pci_vendor_info and pci_slot from the port binding-profile rather than unsetting the entire field. [1] https://review.openstack.org/#/c/435954/ [2] https://review.openstack.org/#/c/499203/ Closes-Bug: #1719327 Change-Id: I80106707a037d567d0f690570f2cf9cfcd30d594 --- nova/network/neutronv2/api.py | 29 ++++++- nova/tests/fixtures.py | 10 ++- nova/tests/functional/wsgi/test_interfaces.py | 7 ++ nova/tests/unit/network/test_neutronv2.py | 85 +++++++++++++++++-- 4 files changed, 117 insertions(+), 14 deletions(-) diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 0a1162d161..178c89152c 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -15,6 +15,7 @@ # under the License. # +import copy import time from keystoneauth1 import loading as ks_loading @@ -508,7 +509,17 @@ class API(base_api.NetworkAPI): continue port_req_body = {'port': {'device_id': '', 'device_owner': ''}} port_req_body['port'][BINDING_HOST_ID] = None - port_req_body['port'][BINDING_PROFILE] = {} + port = self._show_port(context, port_id, + neutron_client=neutron, + fields=BINDING_PROFILE) + port_profile = port.get(BINDING_PROFILE, {}) + # NOTE: We're doing this to remove the binding information + # for the physical device but don't want to overwrite the other + # information in the binding profile. + for profile_key in ('pci_vendor_info', 'pci_slot'): + if profile_key in port_profile: + del port_profile[profile_key] + port_req_body['port'][BINDING_PROFILE] = port_profile if self._has_dns_extension(): port_req_body['port']['dns_name'] = '' try: @@ -903,7 +914,7 @@ class API(base_api.NetworkAPI): created_port_ids = self._update_ports_for_instance( context, instance, neutron, admin_client, requests_and_created_ports, nets, - bind_host_id, available_macs) + bind_host_id, available_macs, requested_ports_dict) # # Perform a full update of the network_info_cache, @@ -928,7 +939,7 @@ class API(base_api.NetworkAPI): def _update_ports_for_instance(self, context, instance, neutron, admin_client, requests_and_created_ports, nets, - bind_host_id, available_macs): + bind_host_id, available_macs, requested_ports_dict): """Update ports from network_requests. Updates the pre-existing ports and the ones created in @@ -946,6 +957,8 @@ class API(base_api.NetworkAPI): :param nets: a dict of network_id to networks returned from neutron :param bind_host_id: a string for port['binding:host_id'] :param available_macs: a list of available mac addresses + :param requested_ports_dict: dict, keyed by port ID, of ports requested + by the user :returns: tuple with the following:: * list of network dicts in their requested order @@ -980,6 +993,11 @@ class API(base_api.NetworkAPI): zone = 'compute:%s' % instance.availability_zone port_req_body = {'port': {'device_id': instance.uuid, 'device_owner': zone}} + if (requested_ports_dict and + request.port_id in requested_ports_dict and + requested_ports_dict[request.port_id].get(BINDING_PROFILE)): + port_req_body['port'][BINDING_PROFILE] = ( + requested_ports_dict[request.port_id][BINDING_PROFILE]) try: self._populate_neutron_extension_values( context, instance, request.pci_request_id, port_req_body, @@ -1077,7 +1095,10 @@ class API(base_api.NetworkAPI): if pci_request_id: pci_dev = pci_manager.get_instance_pci_devs( instance, pci_request_id).pop() - profile = self._get_pci_device_profile(pci_dev) + if port_req_body['port'].get(BINDING_PROFILE) is None: + port_req_body['port'][BINDING_PROFILE] = {} + profile = copy.deepcopy(port_req_body['port'][BINDING_PROFILE]) + profile.update(self._get_pci_device_profile(pci_dev)) port_req_body['port'][BINDING_PROFILE] = profile @staticmethod diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index 35dfe89cd5..be9e662ccf 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -1216,6 +1216,13 @@ class NeutronFixture(fixtures.Fixture): else: return None + def _filter_ports(self, **_params): + ports = copy.deepcopy(self._ports) + for opt in _params: + filtered_ports = [p for p in ports if p.get(opt) == _params[opt]] + ports = filtered_ports + return {'ports': ports} + def list_extensions(self, *args, **kwargs): return copy.deepcopy({'extensions': self._extensions}) @@ -1229,12 +1236,13 @@ class NeutronFixture(fixtures.Fixture): for current_port in self._ports: if current_port['id'] == port: self._ports.remove(current_port) + return def list_networks(self, retrieve_all=True, **_params): return copy.deepcopy({'networks': self._networks}) def list_ports(self, retrieve_all=True, **_params): - return copy.deepcopy({'ports': self._ports}) + return self._filter_ports(**_params) def list_subnets(self, retrieve_all=True, **_params): return copy.deepcopy({'subnets': self._subnets}) diff --git a/nova/tests/functional/wsgi/test_interfaces.py b/nova/tests/functional/wsgi/test_interfaces.py index 8d8be93eeb..c1470d866d 100644 --- a/nova/tests/functional/wsgi/test_interfaces.py +++ b/nova/tests/functional/wsgi/test_interfaces.py @@ -124,6 +124,13 @@ class InterfaceFullstackWithNeutron(test_servers.ServersTestBase): found_server = self._wait_for_state_change(created_server, 'BUILD') self.assertEqual('ACTIVE', found_server['status']) + post = { + 'interfaceAttachment': { + 'net_id': "3cb9bc59-5699-4588-a4b1-b87f96708bc6" + } + } + self.api.attach_interface(created_server_id, post) + response = self.api.get_port_interfaces(created_server_id)[0] port_id = response['port_id'] diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index 309b4160a8..d6ae001235 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -1539,6 +1539,8 @@ class TestNeutronv2(TestNeutronv2Base): self.moxed_client) if requested_networks: for net, fip, port, request_id in requested_networks: + self.moxed_client.show_port(port, fields='binding:profile' + ).AndReturn({'port': ret_data[0]}) self.moxed_client.update_port(port) for port in ports: self.moxed_client.delete_port(port).InAnyOrder("delete_port_group") @@ -4320,16 +4322,16 @@ class TestNeutronv2WithMock(test.TestCase): def test_unbind_ports_get_client(self, mock_neutron): self._test_unbind_ports_get_client(mock_neutron) - def _test_unbind_ports(self, mock_neutron): + @mock.patch('nova.network.neutronv2.api.API._show_port') + def _test_unbind_ports(self, mock_neutron, mock_show): mock_client = mock.Mock() mock_update_port = mock.Mock() mock_client.update_port = mock_update_port mock_ctx = mock.Mock(is_admin=False) - mock_neutron.return_value = mock_client ports = ["1", "2", "3"] - + mock_show.side_effect = [{"id": "1"}, {"id": "2"}, {"id": "3"}] api = neutronapi.API() - api._unbind_ports(mock_ctx, ports, mock_client) + api._unbind_ports(mock_ctx, ports, mock_neutron, mock_client) body = {'port': {'device_id': '', 'device_owner': ''}} body['port'][neutronapi.BINDING_HOST_ID] = None @@ -4644,11 +4646,13 @@ class TestNeutronv2WithMock(test.TestCase): self.api.get_floating_ips_by_project, self.context) - def test_unbind_ports_reset_dns_name(self): + @mock.patch('nova.network.neutronv2.api.API._show_port') + def test_unbind_ports_reset_dns_name(self, mock_show): neutron = mock.Mock() port_client = mock.Mock() self.api.extensions = [constants.DNS_INTEGRATION] ports = [uuids.port_id] + mock_show.return_value = {'id': uuids.port} self.api._unbind_ports(self.context, ports, neutron, port_client) port_req_body = {'port': {'binding:host_id': None, 'binding:profile': {}, @@ -4658,6 +4662,29 @@ class TestNeutronv2WithMock(test.TestCase): port_client.update_port.assert_called_once_with( uuids.port_id, port_req_body) + @mock.patch('nova.network.neutronv2.api.API._show_port') + def test_unbind_ports_reset_binding_profile(self, mock_show): + neutron = mock.Mock() + port_client = mock.Mock() + ports = [uuids.port_id] + mock_show.return_value = { + 'id': uuids.port, + 'binding:profile': {'pci_vendor_info': '1377:0047', + 'pci_slot': '0000:0a:00.1', + 'physical_network': 'phynet1', + 'capabilities': ['switchdev']} + } + self.api._unbind_ports(self.context, ports, neutron, port_client) + port_req_body = {'port': {'binding:host_id': None, + 'binding:profile': + {'physical_network': 'phynet1', + 'capabilities': ['switchdev']}, + 'device_id': '', + 'device_owner': ''} + } + port_client.update_port.assert_called_once_with( + uuids.port_id, port_req_body) + def test_make_floating_ip_obj(self): self._test_make_floating_ip_obj() @@ -4759,7 +4786,7 @@ class TestNeutronv2WithMock(test.TestCase): self.api._update_ports_for_instance, self.context, instance, ntrn, ntrn, requests_and_created_ports, nets, bind_host_id=None, - available_macs=None) + available_macs=None, requested_ports_dict=None) # assert the calls mock_update_port.assert_has_calls([ mock.call(ntrn, instance, uuids.preexisting_port_id, mock.ANY), @@ -4788,12 +4815,14 @@ class TestNeutronv2WithMock(test.TestCase): '172.24.4.227') self.assertIsNone(fip) + @mock.patch('nova.network.neutronv2.api.API._show_port') @mock.patch.object(neutronapi.LOG, 'exception') - def test_unbind_ports_portnotfound(self, mock_log): + def test_unbind_ports_portnotfound(self, mock_log, mock_show): api = neutronapi.API() neutron_client = mock.Mock() neutron_client.update_port = mock.Mock( side_effect=exceptions.PortNotFoundClient) + mock_show.return_value = {'id': uuids.port} api._unbind_ports(self.context, [uuids.port_id], neutron_client, neutron_client) neutron_client.update_port.assert_called_once_with( @@ -4802,12 +4831,14 @@ class TestNeutronv2WithMock(test.TestCase): 'binding:profile': {}, 'binding:host_id': None}}) mock_log.assert_not_called() + @mock.patch('nova.network.neutronv2.api.API._show_port') @mock.patch.object(neutronapi.LOG, 'exception') - def test_unbind_ports_unexpected_error(self, mock_log): + def test_unbind_ports_unexpected_error(self, mock_log, mock_show): api = neutronapi.API() neutron_client = mock.Mock() neutron_client.update_port = mock.Mock( side_effect=test.TestingException) + mock_show.return_value = {'id': uuids.port} api._unbind_ports(self.context, [uuids.port_id], neutron_client, neutron_client) neutron_client.update_port.assert_called_once_with( @@ -4964,6 +4995,41 @@ class TestNeutronv2Portbinding(TestNeutronv2Base): self.assertEqual(profile, port_req_body['port'][neutronapi.BINDING_PROFILE]) + @mock.patch.object(pci_whitelist.Whitelist, 'get_devspec') + @mock.patch.object(pci_manager, 'get_instance_pci_devs') + def test_populate_neutron_extension_values_binding_sriov_with_cap(self, + mock_get_instance_pci_devs, + mock_get_pci_device_devspec): + api = neutronapi.API() + host_id = 'my_host_id' + instance = {'host': host_id} + port_req_body = {'port': { + neutronapi.BINDING_PROFILE: { + 'capabilities': ['switchdev']}}} + pci_req_id = 'my_req_id' + pci_dev = {'vendor_id': '1377', + 'product_id': '0047', + 'address': '0000:0a:00.1', + } + PciDevice = collections.namedtuple('PciDevice', + ['vendor_id', 'product_id', 'address']) + mydev = PciDevice(**pci_dev) + profile = {'pci_vendor_info': '1377:0047', + 'pci_slot': '0000:0a:00.1', + 'physical_network': 'phynet1', + 'capabilities': ['switchdev'], + } + + mock_get_instance_pci_devs.return_value = [mydev] + devspec = mock.Mock() + devspec.get_tags.return_value = {'physical_network': 'phynet1'} + mock_get_pci_device_devspec.return_value = devspec + api._populate_neutron_binding_profile(instance, + pci_req_id, port_req_body) + + self.assertEqual(profile, + port_req_body['port'][neutronapi.BINDING_PROFILE]) + @mock.patch.object(pci_whitelist.Whitelist, 'get_devspec') @mock.patch.object(pci_manager, 'get_instance_pci_devs') def test_populate_neutron_extension_values_binding_sriov_fail( @@ -5554,6 +5620,7 @@ class TestAllocateForInstance(test.NoDBTestCase): nets = {uuids.net1: net1, uuids.net2: net2} bind_host_id = "bind_host_id" available_macs = ["mac1", "mac2"] + requested_ports_dict = {uuids.port1: {}, uuids.port2: {}} mock_neutron.list_extensions.return_value = {"extensions": [ {"name": "asdf"}]} @@ -5565,7 +5632,7 @@ class TestAllocateForInstance(test.NoDBTestCase): created_port_ids = api._update_ports_for_instance( self.context, self.instance, mock_neutron, mock_admin, requests_and_created_ports, nets, - bind_host_id, available_macs) + bind_host_id, available_macs, requested_ports_dict) # TODO(johngarbutt) need to build on this test so we can replace # all the mox based tests