From 7b2f7a1f96787a490d56289c55b8ad7cb5ac030e Mon Sep 17 00:00:00 2001 From: Moshe Levi Date: Sun, 12 Nov 2017 08:55:15 +0200 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: Id847949b4761d51a14e5c2f39552f60a47889aa9 --- nova/network/neutronv2/api.py | 47 ++++++- nova/tests/fixtures.py | 9 +- nova/tests/functional/wsgi/test_interfaces.py | 7 ++ nova/tests/unit/network/test_neutronv2.py | 117 ++++++++++++++++-- 4 files changed, 164 insertions(+), 16 deletions(-) diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 454ca7fa59c4..43ad02e53bab 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,14 +509,38 @@ 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] = {} + try: + port = self._show_port(context, port_id, + neutron_client=neutron, + fields=BINDING_PROFILE) + except exception.PortNotFound: + LOG.debug('Unable to show port %s as it no longer ' + 'exists.', port_id) + return + except Exception: + # NOTE: In case we can't retrieve the binding:profile assume + # that they are empty + LOG.exception("Unable to get binding:profile for port '%s'", + port_id) + port_profile = {} + else: + 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: port_client.update_port(port_id, port_req_body) except neutron_client_exc.PortNotFoundClient: - LOG.debug('Unable to unbind port %s as it no longer exists.', - port_id) + LOG.debug('Unable to unbind port %s as it no longer ' + 'exists.', port_id) except Exception: LOG.exception("Unable to clear device ID for port '%s'", port_id) @@ -903,7 +928,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 +953,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 +971,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 +1007,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 +1109,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 35dfe89cd587..6df93db4cab0 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}) @@ -1234,7 +1241,7 @@ class NeutronFixture(fixtures.Fixture): 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 8d8be93eeb91..c1470d866d83 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 1fefcfb2f8a9..ff92bd887a8b 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -1540,6 +1540,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") @@ -4321,16 +4323,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 @@ -4645,11 +4647,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': {}, @@ -4659,6 +4663,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() @@ -4760,7 +4787,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), @@ -4789,12 +4816,46 @@ class TestNeutronv2WithMock(test.TestCase): '172.24.4.227') self.assertIsNone(fip) + @mock.patch('nova.network.neutronv2.api.API._show_port', + side_effect=exception.PortNotFound(port_id=uuids.port)) @mock.patch.object(neutronapi.LOG, 'exception') - def test_unbind_ports_portnotfound(self, mock_log): + def test_unbind_ports_port_show_portnotfound(self, mock_log, mock_show): + api = neutronapi.API() + neutron_client = mock.Mock() + mock_show.return_value = {'id': uuids.port} + api._unbind_ports(self.context, [uuids.port_id], + neutron_client, neutron_client) + mock_show.assert_called_once_with( + mock.ANY, uuids.port_id, + fields='binding:profile', + neutron_client=mock.ANY) + mock_log.assert_not_called() + + @mock.patch('nova.network.neutronv2.api.API._show_port', + side_effect=Exception) + @mock.patch.object(neutronapi.LOG, 'exception') + def test_unbind_ports_port_show_unexpected_error(self, + mock_log, + mock_show): + api = neutronapi.API() + neutron_client = mock.Mock() + 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( + uuids.port_id, {'port': { + 'device_id': '', 'device_owner': '', + 'binding:profile': {}, 'binding:host_id': None}}) + self.assertTrue(mock_log.called) + + @mock.patch('nova.network.neutronv2.api.API._show_port') + @mock.patch.object(neutronapi.LOG, 'exception') + 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( @@ -4803,12 +4864,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( @@ -4965,6 +5028,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( @@ -5555,6 +5653,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"}]} @@ -5566,7 +5665,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