From 982e2ee02d7b1f1a367081d9d660a1d37505ce05 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 6 Mar 2020 10:43:09 +0000 Subject: [PATCH] Use neutronclient's port binding APIs Take advantage of the neutronclient bindings for the port binding APIs added in neutronclient 7.1.0 to avoid having to vendor this stuff ourselves. Change-Id: Icc284203fb53658abe304f24a62705217f90b22b Signed-off-by: Stephen Finucane --- lower-constraints.txt | 2 +- nova/exception.py | 8 +- nova/network/neutron.py | 287 ++++++++------------- nova/tests/fixtures/neutron.py | 89 ++----- nova/tests/functional/test_servers.py | 22 +- nova/tests/unit/network/test_neutron.py | 316 +++++++++++------------- requirements.txt | 2 +- 7 files changed, 277 insertions(+), 449 deletions(-) diff --git a/lower-constraints.txt b/lower-constraints.txt index 1de5eb823673..59448edd4162 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -122,7 +122,7 @@ python-glanceclient==2.8.0 python-ironicclient==3.0.0 python-keystoneclient==3.15.0 python-mimeparse==1.6.0 -python-neutronclient==6.7.0 +python-neutronclient==7.1.0 python-subunit==1.4.0 pytz==2018.3 PyYAML==5.1 diff --git a/nova/exception.py b/nova/exception.py index df2ed7eed4f4..f5e393e5a6d5 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -816,13 +816,13 @@ class PortBindingFailed(Invalid): class PortBindingDeletionFailed(NovaException): - msg_fmt = _("Failed to delete binding for port %(port_id)s and host " - "%(host)s.") + msg_fmt = _("Failed to delete binding for port(s) %(port_id)s on host " + "%(host)s; please check neutron logs for more information") class PortBindingActivationFailed(NovaException): - msg_fmt = _("Failed to activate binding for port %(port_id)s and host " - "%(host)s.") + msg_fmt = _("Failed to activate binding for port %(port_id)s on host " + "%(host)s; please check neutron logs for more information") class PortUpdateFailed(Invalid): diff --git a/nova/network/neutron.py b/nova/network/neutron.py index 9c00b017a68b..4059c1f76be1 100644 --- a/nova/network/neutron.py +++ b/nova/network/neutron.py @@ -255,26 +255,6 @@ def get_client(context, admin=False): admin=admin or context.is_admin) -def _get_ksa_client(context, admin=False): - """Returns a keystoneauth Adapter - - This method should only be used if python-neutronclient does not yet - provide the necessary API bindings. - - :param context: User request context - :param admin: If True, uses the configured credentials, else uses the - existing auth_token in the context (the user token). - :returns: keystoneauth1 Adapter object - """ - auth_plugin = _get_auth_plugin(context, admin=admin) - session = _get_session() - client = utils.get_ksa_adapter( - 'network', ksa_auth=auth_plugin, ksa_session=session) - client.additional_headers = {'accept': 'application/json'} - client.connect_retries = CONF.neutron.http_retries - return client - - def _is_not_duplicate(item, items, items_list_name, instance): present = item in items @@ -417,25 +397,29 @@ class API: :param host: host from which to delete port bindings :raises: PortBindingDeletionFailed if port binding deletion fails. """ + client = get_client(context, admin=True) failed_port_ids = [] + for port in ports: # This call is safe in that 404s for non-existing # bindings are ignored. try: - self.delete_port_binding( - context, port['id'], host) - except exception.PortBindingDeletionFailed: - # delete_port_binding will log an error for each - # failure but since we're iterating a list we want - # to keep track of all failures to build a generic - # exception to raise + client.delete_port_binding(port['id'], host) + except neutron_client_exc.NeutronClientException as exc: + # We can safely ignore 404s since we're trying to delete + # the thing that wasn't found anyway, but for everything else + # we should log an error + if exc.status_code == 404: + continue + failed_port_ids.append(port['id']) + LOG.exception( + "Failed to delete binding for port %(port_id)s on host " + "%(host)s", {'port_id': port['id'], 'host': host}) + if failed_port_ids: - msg = (_("Failed to delete binding for port(s) " - "%(port_ids)s and host %(host)s.") % - {'port_ids': ','.join(failed_port_ids), - 'host': host}) - raise exception.PortBindingDeletionFailed(msg) + raise exception.PortBindingDeletionFailed( + port_id=','.join(failed_port_ids), host=host) def _get_available_networks(self, context, project_id, net_ids=None, neutron=None, @@ -1329,9 +1313,9 @@ class API: LOG.debug('Instance does not have any ports.', instance=instance) return {} - client = _get_ksa_client(context, admin=True) + client = get_client(context, admin=True) - bindings_by_port_id = {} + bindings_by_port_id: ty.Dict[str, ty.Any] = {} for vif in network_info: # Now bind each port to the destination host and keep track of each # port that is bound to the resulting binding so we can rollback in @@ -1348,46 +1332,28 @@ class API: else: binding['profile'] = port_profiles[port_id] - data = dict(binding=binding) - resp = self._create_port_binding(context, client, port_id, data) - if resp: - bindings_by_port_id[port_id] = resp.json()['binding'] - else: + data = {'binding': binding} + try: + binding = client.create_port_binding(port_id, data)['binding'] + except neutron_client_exc.NeutronClientException: # Something failed, so log the error and rollback any # successful bindings. - LOG.error('Binding failed for port %s and host %s. ' - 'Error: (%s %s)', - port_id, host, resp.status_code, resp.text, - instance=instance) + LOG.error('Binding failed for port %s and host %s.', + port_id, host, instance=instance, exc_info=True) for rollback_port_id in bindings_by_port_id: try: - self.delete_port_binding( - context, rollback_port_id, host) - except exception.PortBindingDeletionFailed: - LOG.warning('Failed to remove binding for port %s on ' - 'host %s.', rollback_port_id, host, - instance=instance) + client.delete_port_binding(rollback_port_id, host) + except neutron_client_exc.NeutronClientException as exc: + if exc.status_code != 404: + LOG.warning('Failed to remove binding for port %s ' + 'on host %s.', rollback_port_id, host, + instance=instance) raise exception.PortBindingFailed(port_id=port_id) + bindings_by_port_id[port_id] = binding + return bindings_by_port_id - @staticmethod - def _create_port_binding(context, client, port_id, data): - """Creates a port binding with the specified data. - - :param context: The request context for the operation. - :param client: keystoneauth1.adapter.Adapter - :param port_id: The ID of the port on which to create the binding. - :param data: dict of port binding data (requires at least the host), - for example:: - - {'binding': {'host': 'dest.host.com'}} - :return: requests.Response object - """ - return client.post( - '/v2.0/ports/%s/bindings' % port_id, json=data, raise_exc=False, - global_request_id=context.global_id) - def delete_port_binding(self, context, port_id, host): """Delete the port binding for the given port ID and host @@ -1400,104 +1366,19 @@ class API: :raises: nova.exception.PortBindingDeletionFailed if a non-404 error response is received from neutron. """ - client = _get_ksa_client(context, admin=True) - resp = self._delete_port_binding(context, client, port_id, host) - if resp: - LOG.debug('Deleted binding for port %s and host %s.', - port_id, host) - else: + client = get_client(context, admin=True) + try: + client.delete_port_binding(port_id, host) + except neutron_client_exc.NeutronClientException as exc: # We can safely ignore 404s since we're trying to delete # the thing that wasn't found anyway. - if resp.status_code != 404: - # Log the details, raise an exception. - LOG.error('Unexpected error trying to delete binding ' - 'for port %s and host %s. Code: %s. ' - 'Error: %s', port_id, host, - resp.status_code, resp.text) + if exc.status_code != 404: + LOG.error( + 'Unexpected error trying to delete binding for port %s ' + 'and host %s.', port_id, host, exc_info=True) raise exception.PortBindingDeletionFailed( port_id=port_id, host=host) - @staticmethod - def _delete_port_binding(context, client, port_id, host): - """Deletes the binding for the given host on the given port. - - :param context: The request context for the operation. - :param client: keystoneauth1.adapter.Adapter - :param port_id: ID of the port from which to delete the binding - :param host: A string name of the host on which the port is bound - :return: requests.Response object - """ - return client.delete( - '/v2.0/ports/%s/bindings/%s' % (port_id, host), raise_exc=False, - global_request_id=context.global_id) - - def activate_port_binding(self, context, port_id, host): - """Activates an inactive port binding. - - If there are two port bindings to different hosts, activating the - inactive binding atomically changes the other binding to inactive. - - :param context: The request context for the operation. - :param port_id: The ID of the port with an inactive binding on the - host. - :param host: The host on which the inactive port binding should be - activated. - :raises: nova.exception.PortBindingActivationFailed if a non-409 error - response is received from neutron. - """ - client = _get_ksa_client(context, admin=True) - # This is a bit weird in that we don't PUT and update the status - # to ACTIVE, it's more like a POST action method in the compute API. - resp = self._activate_port_binding(context, client, port_id, host) - if resp: - LOG.debug('Activated binding for port %s and host %s.', - port_id, host) - # A 409 means the port binding is already active, which shouldn't - # happen if the caller is doing things in the correct order. - elif resp.status_code == 409: - LOG.warning('Binding for port %s and host %s is already ' - 'active.', port_id, host) - else: - # Log the details, raise an exception. - LOG.error('Unexpected error trying to activate binding ' - 'for port %s and host %s. Code: %s. ' - 'Error: %s', port_id, host, resp.status_code, - resp.text) - raise exception.PortBindingActivationFailed( - port_id=port_id, host=host) - - @staticmethod - def _activate_port_binding(context, client, port_id, host): - """Activates an inactive port binding. - - :param context: The request context for the operation. - :param client: keystoneauth1.adapter.Adapter - :param port_id: ID of the port to activate the binding on - :param host: A string name of the host identifying the binding to be - activated - :return: requests.Response object - """ - return client.put( - '/v2.0/ports/%s/bindings/%s/activate' % (port_id, host), - raise_exc=False, - global_request_id=context.global_id) - - @staticmethod - def _get_port_binding(context, client, port_id, host): - """Returns a port binding of a given port on a given host - - :param context: The request context for the operation. - :param client: keystoneauth1.adapter.Adapter - :param port_id: ID of the port to get the binding - :param host: A string name of the host identifying the binding to be - returned - :return: requests.Response object - """ - return client.get( - '/v2.0/ports/%s/bindings/%s' % (port_id, host), - raise_exc=False, - global_request_id=context.global_id) - def _get_pci_device_profile(self, pci_dev): dev_spec = self.pci_whitelist.get_devspec(pci_dev) if dev_spec: @@ -2865,43 +2746,73 @@ class API: 'updated later.', instance=instance) return - client = _get_ksa_client(context, admin=True) + client = get_client(context, admin=True) dest_host = migration['dest_compute'] for vif in instance.get_network_info(): # Not all compute migration flows use the port binding-extended # API yet, so first check to see if there is a binding for the # port and destination host. - resp = self._get_port_binding( - context, client, vif['id'], dest_host) - if resp: - if resp.json()['binding']['status'] != 'ACTIVE': - self.activate_port_binding(context, vif['id'], dest_host) - # TODO(mriedem): Do we need to call - # _clear_migration_port_profile? migrate_instance_finish - # would normally take care of clearing the "migrating_to" - # attribute on each port when updating the port's - # binding:host_id to point to the destination host. - else: - # We might be racing with another thread that's handling - # post-migrate operations and already activated the port - # binding for the destination host. - LOG.debug('Port %s binding to destination host %s is ' - 'already ACTIVE.', vif['id'], dest_host, - instance=instance) - elif resp.status_code == 404: - # If there is no port binding record for the destination host, - # we can safely assume none of the ports attached to the + try: + binding = client.show_port_binding( + vif['id'], dest_host + )['binding'] + except neutron_client_exc.NeutronClientException as exc: + if exc.status_code != 404: + # We don't raise an exception here because we assume that + # port bindings will be updated correctly when + # migrate_instance_finish runs + LOG.error( + 'Unexpected error trying to get binding info ' + 'for port %s and destination host %s.', + vif['id'], dest_host, exc_info=True) + continue + + # ...but if there is no port binding record for the destination + # host, we can safely assume none of the ports attached to the # instance are using the binding-extended API in this flow and # exit early. return - else: - # We don't raise an exception here because we assume that - # port bindings will be updated correctly when - # migrate_instance_finish runs. - LOG.error('Unexpected error trying to get binding info ' - 'for port %s and destination host %s. Code: %i. ' - 'Error: %s', vif['id'], dest_host, resp.status_code, - resp.text) + + if binding['status'] == 'ACTIVE': + # We might be racing with another thread that's handling + # post-migrate operations and already activated the port + # binding for the destination host. + LOG.debug( + 'Port %s binding to destination host %s is already ACTIVE', + vif['id'], dest_host, instance=instance) + continue + + try: + # This is a bit weird in that we don't PUT and update the + # status to ACTIVE, it's more like a POST action method in the + # compute API. + client.activate_port_binding(vif['id'], dest_host) + LOG.debug( + 'Activated binding for port %s and host %s', + vif['id'], dest_host) + except neutron_client_exc.NeutronClientException as exc: + # A 409 means the port binding is already active, which + # shouldn't happen if the caller is doing things in the correct + # order. + if exc.status_code == 409: + LOG.warning( + 'Binding for port %s and host %s is already active', + vif['id'], dest_host, exc_info=True) + continue + + # Log the details, raise an exception. + LOG.error( + 'Unexpected error trying to activate binding ' + 'for port %s and host %s.', + vif['id'], dest_host, exc_info=True) + raise exception.PortBindingActivationFailed( + port_id=vif['id'], host=dest_host) + + # TODO(mriedem): Do we need to call + # _clear_migration_port_profile? migrate_instance_finish + # would normally take care of clearing the "migrating_to" + # attribute on each port when updating the port's + # binding:host_id to point to the destination host. def migrate_instance_finish( self, context, instance, migration, provider_mappings): diff --git a/nova/tests/fixtures/neutron.py b/nova/tests/fixtures/neutron.py index a05975c0a5d5..58b70986b4a0 100644 --- a/nova/tests/fixtures/neutron.py +++ b/nova/tests/fixtures/neutron.py @@ -15,18 +15,14 @@ import copy import random import fixtures -from keystoneauth1 import adapter as ksa_adap -import mock from neutronclient.common import exceptions as neutron_client_exc import os_resource_classes as orc -from oslo_serialization import jsonutils from oslo_utils import uuidutils from nova import exception from nova.network import constants as neutron_constants from nova.network import model as network_model from nova.tests.fixtures import nova as nova_fixtures -from nova.tests.unit import fake_requests class _FakeNeutronClient: @@ -665,25 +661,6 @@ class NeutronFixture(fixtures.Fixture): lambda *args, **kwargs: network_model.NetworkInfo.hydrate( self.nw_info)) - # Stub out port binding APIs which go through a KSA client Adapter - # rather than python-neutronclient. - self.test.stub_out( - 'nova.network.neutron._get_ksa_client', - lambda *args, **kwargs: mock.Mock( - spec=ksa_adap.Adapter)) - self.test.stub_out( - 'nova.network.neutron.API._create_port_binding', - self.create_port_binding) - self.test.stub_out( - 'nova.network.neutron.API._delete_port_binding', - self.delete_port_binding) - self.test.stub_out( - 'nova.network.neutron.API._activate_port_binding', - self.activate_port_binding) - self.test.stub_out( - 'nova.network.neutron.API._get_port_binding', - self.get_port_binding) - self.test.stub_out( 'nova.network.neutron.get_client', self._get_client) @@ -692,13 +669,12 @@ class NeutronFixture(fixtures.Fixture): admin = admin or context.is_admin and not context.auth_token return _FakeNeutronClient(self, admin) - def create_port_binding(self, context, client, port_id, data): + def create_port_binding(self, port_id, body): if port_id not in self._ports: - return fake_requests.FakeResponse( - 404, content='Port %s not found' % port_id) + raise neutron_client_exc.NeutronClientException(status_code=404) port = self._ports[port_id] - binding = copy.deepcopy(data['binding']) + binding = copy.deepcopy(body['binding']) # NOTE(stephenfin): We don't allow changing of backend binding['vif_type'] = port['binding:vif_type'] @@ -713,61 +689,36 @@ class NeutronFixture(fixtures.Fixture): self._port_bindings[port_id][binding['host']] = binding - return fake_requests.FakeResponse( - 200, content=jsonutils.dumps({'binding': binding}), - ) + return {'binding': binding} - def _get_failure_response_if_port_or_binding_not_exists( - self, port_id, host, - ): + def _validate_port_binding(self, port_id, host_id): if port_id not in self._ports: - return fake_requests.FakeResponse( - 404, content='Port %s not found' % port_id) - if host not in self._port_bindings[port_id]: - return fake_requests.FakeResponse( - 404, - content='Binding for host %s for port %s not found' - % (host, port_id)) + raise neutron_client_exc.NeutronClientException(status_code=404) - def delete_port_binding(self, context, client, port_id, host): - failure = self._get_failure_response_if_port_or_binding_not_exists( - port_id, host) - if failure is not None: - return failure + if host_id not in self._port_bindings[port_id]: + raise neutron_client_exc.NeutronClientException(status_code=404) - del self._port_bindings[port_id][host] + def delete_port_binding(self, port_id, host_id): + self._validate_port_binding(port_id, host_id) + del self._port_bindings[port_id][host_id] - return fake_requests.FakeResponse(204) - - def _activate_port_binding(self, port_id, host): + def _activate_port_binding(self, port_id, host_id): # It makes sure that only one binding is active for a port - for h, binding in self._port_bindings[port_id].items(): - if h == host: + for host, binding in self._port_bindings[port_id].items(): + if host == host_id: # NOTE(gibi): neutron returns 409 if this binding is already # active but nova does not depend on this behaviour yet. binding['status'] = 'ACTIVE' else: binding['status'] = 'INACTIVE' - def activate_port_binding(self, context, client, port_id, host): - failure = self._get_failure_response_if_port_or_binding_not_exists( - port_id, host) - if failure is not None: - return failure + def activate_port_binding(self, port_id, host_id): + self._validate_port_binding(port_id, host_id) + self._activate_port_binding(port_id, host_id) - self._activate_port_binding(port_id, host) - - return fake_requests.FakeResponse(200) - - def get_port_binding(self, context, client, port_id, host): - failure = self._get_failure_response_if_port_or_binding_not_exists( - port_id, host) - if failure is not None: - return failure - - binding = {"binding": self._port_bindings[port_id][host]} - return fake_requests.FakeResponse( - 200, content=jsonutils.dumps(binding)) + def show_port_binding(self, port_id, host_id): + self._validate_port_binding(port_id, host_id) + return {'binding': self._port_bindings[port_id][host_id]} def _list_resource(self, resources, retrieve_all, **_params): # If 'fields' is passed we need to strip that out since it will mess diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index 2cba010af731..f41abfd2c8cc 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -12,6 +12,7 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. + import collections import copy import datetime @@ -8383,10 +8384,11 @@ class CrossCellResizeWithQoSPort(PortResourceRequestBasedSchedulingTestBase): server = self._create_server_with_ports_and_check_allocation( non_qos_normal_port, qos_normal_port, qos_sriov_port) - orig_create_binding = neutronapi.API._create_port_binding + orig_create_binding = self.neutron.create_port_binding hosts = { - 'host1': self.compute1_rp_uuid, 'host2': self.compute2_rp_uuid} + 'host1': self.compute1_rp_uuid, 'host2': self.compute2_rp_uuid, + } # Add an extra check to our neutron fixture. This check makes sure that # the RP sent in the binding corresponds to host of the binding. In a @@ -8394,21 +8396,23 @@ class CrossCellResizeWithQoSPort(PortResourceRequestBasedSchedulingTestBase): # 1907522 showed we fail this check for cross cell migration with qos # ports in a real deployment. So to reproduce that bug we need to have # the same check in our test env too. - def spy_on_create_binding(context, client, port_id, data): + def spy_on_create_binding(port_id, data): host_rp_uuid = hosts[data['binding']['host']] device_rp_uuid = data['binding']['profile'].get('allocation') if port_id == qos_normal_port['id']: if device_rp_uuid != self.ovs_bridge_rp_per_host[host_rp_uuid]: raise exception.PortBindingFailed(port_id=port_id) elif port_id == qos_sriov_port['id']: - if (device_rp_uuid not in - self.sriov_dev_rp_per_host[host_rp_uuid].values()): + if ( + device_rp_uuid not in + self.sriov_dev_rp_per_host[host_rp_uuid].values() + ): raise exception.PortBindingFailed(port_id=port_id) - return orig_create_binding(context, client, port_id, data) + return orig_create_binding(port_id, data) with mock.patch( - 'nova.network.neutron.API._create_port_binding', + 'nova.tests.fixtures.NeutronFixture.create_port_binding', side_effect=spy_on_create_binding, autospec=True ): # We expect the migration to fail as the only available target @@ -8440,8 +8444,8 @@ class CrossCellResizeWithQoSPort(PortResourceRequestBasedSchedulingTestBase): self._create_networking_rp_tree('host3', self.compute3_rp_uuid) with mock.patch( - 'nova.network.neutron.API._create_port_binding', - side_effect=spy_on_create_binding, autospec=True + 'nova.tests.fixtures.NeutronFixture.create_port_binding', + side_effect=spy_on_create_binding, autospec=True ): server = self._migrate_server(server) self.assertEqual('host3', server['OS-EXT-SRV-ATTR:host']) diff --git a/nova/tests/unit/network/test_neutron.py b/nova/tests/unit/network/test_neutron.py index a85a19e285e1..8c0f6cfbeffb 100644 --- a/nova/tests/unit/network/test_neutron.py +++ b/nova/tests/unit/network/test_neutron.py @@ -49,7 +49,6 @@ from nova import policy from nova import service_auth from nova import test from nova.tests.unit import fake_instance -from nova.tests.unit import fake_requests as fake_req CONF = cfg.CONF @@ -251,8 +250,6 @@ class TestNeutronClient(test.NoDBTestCase): auth_token='token') cl = neutronapi.get_client(my_context) self.assertEqual(retries, cl.httpclient.connect_retries) - kcl = neutronapi._get_ksa_client(my_context) - self.assertEqual(retries, kcl.connect_retries) class TestAPIBase(test.TestCase): @@ -4894,24 +4891,23 @@ class TestAPI(TestAPIBase): constants.BINDING_PROFILE: migrate_profile, constants.BINDING_HOST_ID: instance.host}]} self.api.list_ports = mock.Mock(return_value=get_ports) - update_port_mock = mock.Mock() - get_client_mock.return_value.update_port = update_port_mock - with mock.patch.object(self.api, 'delete_port_binding') as del_binding: - with mock.patch.object(self.api, 'supports_port_binding_extension', - return_value=True): - self.api.setup_networks_on_host(self.context, - instance, - host='new-host', - teardown=True) - update_port_mock.assert_called_once_with( - port_id, {'port': { - constants.BINDING_PROFILE: migrate_profile}}) - del_binding.assert_called_once_with( - self.context, port_id, 'new-host') + mocked_client = get_client_mock.return_value - @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock()) + with mock.patch.object(self.api, 'supports_port_binding_extension', + return_value=True): + self.api.setup_networks_on_host(self.context, + instance, + host='new-host', + teardown=True) + + mocked_client.update_port.assert_called_once_with( + port_id, {'port': {constants.BINDING_PROFILE: migrate_profile}}) + mocked_client.delete_port_binding.assert_called_once_with( + port_id, 'new-host') + + @mock.patch.object(neutronapi, 'get_client') def test_update_port_profile_for_migration_teardown_true_with_profile_exc( - self, get_client_mock): + self, get_client_mock): """Tests that delete_port_binding raises PortBindingDeletionFailed which is raised through to the caller. """ @@ -4930,25 +4926,27 @@ class TestAPI(TestAPIBase): constants.BINDING_HOST_ID: instance.host}]} self.api.list_ports = mock.Mock(return_value=get_ports) self.api._clear_migration_port_profile = mock.Mock() - with mock.patch.object( - self.api, 'delete_port_binding', - side_effect=exception.PortBindingDeletionFailed( - port_id=uuids.port1, host='new-host')) as del_binding: - with mock.patch.object(self.api, 'supports_port_binding_extension', - return_value=True): - ex = self.assertRaises( - exception.PortBindingDeletionFailed, - self.api.setup_networks_on_host, - self.context, instance, host='new-host', teardown=True) - # Make sure both ports show up in the exception message. - self.assertIn(uuids.port1, str(ex)) - self.assertIn(uuids.port2, str(ex)) + NeutronError = exceptions.NeutronClientException(status_code=500) + mocked_client = get_client_mock.return_value + mocked_client.delete_port_binding.side_effect = NeutronError + + with mock.patch.object(self.api, 'supports_port_binding_extension', + return_value=True): + ex = self.assertRaises( + exception.PortBindingDeletionFailed, + self.api.setup_networks_on_host, + self.context, instance, host='new-host', teardown=True) + # Make sure both ports show up in the exception message. + self.assertIn(uuids.port1, str(ex)) + self.assertIn(uuids.port2, str(ex)) + self.api._clear_migration_port_profile.assert_called_once_with( self.context, instance, get_client_mock.return_value, get_ports['ports']) - del_binding.assert_has_calls([ - mock.call(self.context, uuids.port1, 'new-host'), - mock.call(self.context, uuids.port2, 'new-host')]) + mocked_client.delete_port_binding.assert_has_calls([ + mock.call(uuids.port1, 'new-host'), + mock.call(uuids.port2, 'new-host'), + ]) @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock()) def test_update_port_profile_for_migration_teardown_true_no_profile( @@ -6101,7 +6099,7 @@ class TestAPI(TestAPIBase): mock_get_inst.assert_called_once_with(ctxt, instance.uuid) mock_get_map.assert_called_once_with(ctxt, instance.uuid) - @mock.patch('nova.network.neutron._get_ksa_client', + @mock.patch('nova.network.neutron.get_client', new_callable=mock.NonCallableMock) # asserts not called def test_migrate_instance_start_no_binding_ext(self, get_client_mock): """Tests that migrate_instance_start exits early if neutron doesn't @@ -6112,63 +6110,65 @@ class TestAPI(TestAPIBase): self.api.migrate_instance_start( self.context, mock.sentinel.instance, {}) - @mock.patch('nova.network.neutron._get_ksa_client') + @mock.patch('nova.network.neutron.get_client') def test_migrate_instance_start_activate(self, get_client_mock): """Tests the happy path for migrate_instance_start where the binding for the port(s) attached to the instance are activated on the destination host. """ binding = {'binding': {'status': 'INACTIVE'}} - resp = fake_req.FakeResponse(200, content=jsonutils.dumps(binding)) - get_client_mock.return_value.get.return_value = resp + mocked_client = get_client_mock.return_value + mocked_client.show_port_binding.return_value = binding # Just create a simple instance with a single port. instance = objects.Instance(info_cache=objects.InstanceInfoCache( network_info=model.NetworkInfo([model.VIF(uuids.port_id)]))) migration = objects.Migration( source_compute='source', dest_compute='dest') - with mock.patch.object(self.api, 'activate_port_binding') as activate: - with mock.patch.object(self.api, 'supports_port_binding_extension', - return_value=True): - self.api.migrate_instance_start( - self.context, instance, migration) - activate.assert_called_once_with(self.context, uuids.port_id, 'dest') - get_client_mock.return_value.get.assert_called_once_with( - '/v2.0/ports/%s/bindings/dest' % uuids.port_id, raise_exc=False, - global_request_id=self.context.global_id) - @mock.patch('nova.network.neutron._get_ksa_client') + with mock.patch.object(self.api, 'supports_port_binding_extension', + return_value=True): + self.api.migrate_instance_start( + self.context, instance, migration) + + mocked_client.show_port_binding.assert_called_once_with( + uuids.port_id, 'dest') + mocked_client.activate_port_binding.assert_called_once_with( + uuids.port_id, 'dest') + + @mock.patch('nova.network.neutron.get_client') def test_migrate_instance_start_already_active(self, get_client_mock): """Tests the case that the destination host port binding is already ACTIVE when migrate_instance_start is called so we don't try to activate it again, which would result in a 409 from Neutron. """ binding = {'binding': {'status': 'ACTIVE'}} - resp = fake_req.FakeResponse(200, content=jsonutils.dumps(binding)) - get_client_mock.return_value.get.return_value = resp + mocked_client = get_client_mock.return_value + mocked_client.show_port_binding.return_value = binding # Just create a simple instance with a single port. instance = objects.Instance(info_cache=objects.InstanceInfoCache( network_info=model.NetworkInfo([model.VIF(uuids.port_id)]))) migration = objects.Migration( source_compute='source', dest_compute='dest') - with mock.patch.object(self.api, 'activate_port_binding', - new_callable=mock.NonCallableMock): - with mock.patch.object(self.api, 'supports_port_binding_extension', - return_value=True): - self.api.migrate_instance_start( - self.context, instance, migration) - get_client_mock.return_value.get.assert_called_once_with( - '/v2.0/ports/%s/bindings/dest' % uuids.port_id, raise_exc=False, - global_request_id=self.context.global_id) - @mock.patch('nova.network.neutron._get_ksa_client') + with mock.patch.object(self.api, 'supports_port_binding_extension', + return_value=True): + self.api.migrate_instance_start( + self.context, instance, migration) + + mocked_client.show_port_binding.assert_called_once_with( + uuids.port_id, 'dest') + mocked_client.activate_port_binding.assert_not_called() + + @mock.patch('nova.network.neutron.get_client') def test_migrate_instance_start_no_bindings(self, get_client_mock): """Tests the case that migrate_instance_start is running against new enough neutron for the binding-extended API but the ports don't have a binding resource against the destination host, so no activation happens. """ - get_client_mock.return_value.get.return_value = ( - fake_req.FakeResponse(404)) + NeutronNotFound = exceptions.NeutronClientException(status_code=404) + mocked_client = get_client_mock.return_value + mocked_client.show_port_binding.side_effect = NeutronNotFound # Create an instance with two ports so we can test the short circuit # when we find that the first port doesn't have a dest host binding. instance = objects.Instance(info_cache=objects.InstanceInfoCache( @@ -6176,45 +6176,41 @@ class TestAPI(TestAPIBase): model.VIF(uuids.port1), model.VIF(uuids.port2)]))) migration = objects.Migration( source_compute='source', dest_compute='dest') - with mock.patch.object(self.api, 'activate_port_binding', - new_callable=mock.NonCallableMock): - with mock.patch.object(self.api, 'supports_port_binding_extension', - return_value=True): - self.api.migrate_instance_start( - self.context, instance, migration) - get_client_mock.return_value.get.assert_called_once_with( - '/v2.0/ports/%s/bindings/dest' % uuids.port1, raise_exc=False, - global_request_id=self.context.global_id) - @mock.patch('nova.network.neutron._get_ksa_client') + with mock.patch.object(self.api, 'supports_port_binding_extension', + return_value=True): + self.api.migrate_instance_start( + self.context, instance, migration) + + mocked_client.show_port_binding.assert_called_once_with( + uuids.port1, 'dest') + mocked_client.activate_port_binding.assert_not_called() + + @mock.patch('nova.network.neutron.get_client') def test_migrate_instance_start_get_error(self, get_client_mock): """Tests the case that migrate_instance_start is running against new enough neutron for the binding-extended API but getting the port binding information results in an error response from neutron. """ - get_client_mock.return_value.get.return_value = ( - fake_req.FakeResponse(500)) + NeutronError = exceptions.NeutronClientException(status_code=500) + mocked_client = get_client_mock.return_value + mocked_client.show_port_binding.side_effect = NeutronError instance = objects.Instance(info_cache=objects.InstanceInfoCache( network_info=model.NetworkInfo([ model.VIF(uuids.port1), model.VIF(uuids.port2)]))) migration = objects.Migration( source_compute='source', dest_compute='dest') - with mock.patch.object(self.api, 'activate_port_binding', - new_callable=mock.NonCallableMock): - with mock.patch.object(self.api, 'supports_port_binding_extension', - return_value=True): - self.api.migrate_instance_start( - self.context, instance, migration) - self.assertEqual(2, get_client_mock.return_value.get.call_count) - get_client_mock.return_value.get.assert_has_calls([ - mock.call( - '/v2.0/ports/%s/bindings/dest' % uuids.port1, - raise_exc=False, - global_request_id=self.context.global_id), - mock.call( - '/v2.0/ports/%s/bindings/dest' % uuids.port2, - raise_exc=False, - global_request_id=self.context.global_id)]) + + with mock.patch.object(self.api, 'supports_port_binding_extension', + return_value=True): + self.api.migrate_instance_start( + self.context, instance, migration) + + self.assertEqual(2, mocked_client.show_port_binding.call_count) + mocked_client.show_port_binding.assert_has_calls([ + mock.call(uuids.port1, 'dest'), + mock.call(uuids.port2, 'dest'), + ]) @mock.patch('nova.network.neutron.get_client') def test_get_requested_resource_for_instance_no_resource_request( @@ -6778,7 +6774,7 @@ class TestAPIPortbinding(TestAPIBase): 'fake_host', 'setup_instance_network_on_host', self.context, instance, 'fake_host') - @mock.patch('nova.network.neutron._get_ksa_client', + @mock.patch('nova.network.neutron.get_client', new_callable=mock.NonCallableMock) def test_bind_ports_to_host_no_ports(self, mock_client): self.assertDictEqual({}, @@ -6787,11 +6783,11 @@ class TestAPIPortbinding(TestAPIBase): objects.Instance(info_cache=None), 'fake-host')) - @mock.patch('nova.network.neutron._get_ksa_client') + @mock.patch('nova.network.neutron.get_client') def test_bind_ports_to_host(self, mock_client): """Tests a single port happy path where everything is successful.""" - def post_side_effect(*args, **kwargs): - self.assertDictEqual(binding, kwargs['json']) + def fake_create(port_id, data): + self.assertDictEqual(binding, data) return mock.DEFAULT nwinfo = model.NetworkInfo([model.VIF(uuids.port)]) @@ -6801,21 +6797,22 @@ class TestAPIPortbinding(TestAPIBase): binding = {'binding': {'host': 'fake-host', 'vnic_type': 'normal', 'profile': {'foo': 'bar'}}} + mocked_client = mock_client.return_value + mocked_client.create_port_binding.return_value = binding + mocked_client.create_port_binding.side_effect = fake_create - resp = fake_req.FakeResponse(200, content=jsonutils.dumps(binding)) - mock_client.return_value.post.return_value = resp - mock_client.return_value.post.side_effect = post_side_effect result = self.api.bind_ports_to_host( ctxt, inst, 'fake-host', {uuids.port: 'normal'}, {uuids.port: {'foo': 'bar'}}) - self.assertEqual(1, mock_client.return_value.post.call_count) + + self.assertEqual(1, mocked_client.create_port_binding.call_count) self.assertDictEqual({uuids.port: binding['binding']}, result) - @mock.patch('nova.network.neutron._get_ksa_client') + @mock.patch('nova.network.neutron.get_client') def test_bind_ports_to_host_with_vif_profile_and_vnic(self, mock_client): """Tests bind_ports_to_host with default/non-default parameters.""" - def post_side_effect(*args, **kwargs): - self.assertDictEqual(binding, kwargs['json']) + def fake_create(port_id, data): + self.assertDictEqual(binding, data) return mock.DEFAULT ctxt = context.get_context() @@ -6828,11 +6825,13 @@ class TestAPIPortbinding(TestAPIBase): binding = {'binding': {'host': 'fake-host', 'vnic_type': 'direct', 'profile': vif_profile}} - resp = fake_req.FakeResponse(200, content=jsonutils.dumps(binding)) - mock_client.return_value.post.return_value = resp - mock_client.return_value.post.side_effect = post_side_effect + mocked_client = mock_client.return_value + mocked_client.create_port_binding.return_value = binding + mocked_client.create_port_binding.side_effect = fake_create + result = self.api.bind_ports_to_host(ctxt, inst, 'fake-host') - self.assertEqual(1, mock_client.return_value.post.call_count) + + self.assertEqual(1, mocked_client.create_port_binding.call_count) self.assertDictEqual({uuids.port: binding['binding']}, result) # assert that that if vnic_type and profile are set in VIF object @@ -6848,14 +6847,17 @@ class TestAPIPortbinding(TestAPIBase): binding = {'binding': {'host': 'fake-host', 'vnic_type': 'direct-overridden', 'profile': {'foo': 'overridden'}}} - resp = fake_req.FakeResponse(200, content=jsonutils.dumps(binding)) - mock_client.return_value.post.return_value = resp + mocked_client = mock_client.return_value + mocked_client.create_port_binding.return_value = binding + mocked_client.create_port_binding.side_effect = fake_create + result = self.api.bind_ports_to_host( ctxt, inst, 'fake-host', vnic_type_per_port, vif_profile_per_port) - self.assertEqual(2, mock_client.return_value.post.call_count) + + self.assertEqual(2, mocked_client.create_port_binding.call_count) self.assertDictEqual({uuids.port: binding['binding']}, result) - @mock.patch('nova.network.neutron._get_ksa_client') + @mock.patch('nova.network.neutron.get_client') def test_bind_ports_to_host_rollback(self, mock_client): """Tests a scenario where an instance has two ports, and binding the first is successful but binding the second fails, so the code will @@ -6865,43 +6867,42 @@ class TestAPIPortbinding(TestAPIBase): model.VIF(uuids.ok), model.VIF(uuids.fail)]) inst = objects.Instance( info_cache=objects.InstanceInfoCache(network_info=nwinfo)) + NeutronError = exceptions.NeutronClientException(status_code=500) - def fake_post(url, *args, **kwargs): - if uuids.ok in url: - mock_response = fake_req.FakeResponse( - 200, content='{"binding": {"host": "fake-host"}}') - else: - mock_response = fake_req.FakeResponse(500, content='error') - return mock_response + def fake_create(port_id, host): + if port_id == uuids.ok: + return {'binding': {'host': 'fake-host'}} - mock_client.return_value.post.side_effect = fake_post - with mock.patch.object(self.api, 'delete_port_binding', - # This will be logged but not re-raised. - side_effect=exception.PortBindingDeletionFailed( - port_id=uuids.ok, host='fake-host' - )) as mock_delete: - self.assertRaises(exception.PortBindingFailed, - self.api.bind_ports_to_host, - self.context, inst, 'fake-host') - # assert that post was called twice and delete once - self.assertEqual(2, mock_client.return_value.post.call_count) - mock_delete.assert_called_once_with(self.context, uuids.ok, - 'fake-host') + raise NeutronError - @mock.patch('nova.network.neutron._get_ksa_client') + mocked_client = mock_client.return_value + mocked_client.create_port_binding.side_effect = fake_create + mocked_client.delete_port_binding.side_effect = NeutronError + + self.assertRaises(exception.PortBindingFailed, + self.api.bind_ports_to_host, + self.context, inst, 'fake-host') + + # assert that create was called twice and delete once + self.assertEqual(2, mocked_client.create_port_binding.call_count) + self.assertEqual(1, mocked_client.delete_port_binding.call_count) + mocked_client.delete_port_binding.assert_called_once_with( + uuids.ok, 'fake-host') + + @mock.patch('nova.network.neutron.get_client') def test_delete_port_binding(self, mock_client): # Create three ports where: # - one is successfully unbound # - one is not found # - one fails to be unbound - def fake_delete(url, *args, **kwargs): - if uuids.ok in url: - return fake_req.FakeResponse(204) - else: - status_code = 404 if uuids.notfound in url else 500 - return fake_req.FakeResponse(status_code) + def fake_delete(port_id, host): + if port_id == uuids.ok: + return - mock_client.return_value.delete.side_effect = fake_delete + status_code = 404 if port_id == uuids.notfound else 500 + raise exceptions.NeutronClientException(status_code=status_code) + + mock_client.return_value.delete_port_binding.side_effect = fake_delete for port_id in (uuids.ok, uuids.notfound, uuids.fail): if port_id == uuids.fail: self.assertRaises(exception.PortBindingDeletionFailed, @@ -6911,45 +6912,6 @@ class TestAPIPortbinding(TestAPIBase): self.api.delete_port_binding(self.context, port_id, 'fake-host') - @mock.patch('nova.network.neutron._get_ksa_client') - def test_activate_port_binding(self, mock_client): - """Tests the happy path of activating an inactive port binding.""" - resp = fake_req.FakeResponse(200) - mock_client.return_value.put.return_value = resp - self.api.activate_port_binding(self.context, uuids.port_id, - 'fake-host') - mock_client.return_value.put.assert_called_once_with( - '/v2.0/ports/%s/bindings/fake-host/activate' % uuids.port_id, - raise_exc=False, - global_request_id=self.context.global_id) - - @mock.patch('nova.network.neutron._get_ksa_client') - @mock.patch('nova.network.neutron.LOG.warning') - def test_activate_port_binding_already_active( - self, mock_log_warning, mock_client): - """Tests the 409 case of activating an already active port binding.""" - mock_client.return_value.put.return_value = fake_req.FakeResponse(409) - self.api.activate_port_binding(self.context, uuids.port_id, - 'fake-host') - mock_client.return_value.put.assert_called_once_with( - '/v2.0/ports/%s/bindings/fake-host/activate' % uuids.port_id, - raise_exc=False, - global_request_id=self.context.global_id) - self.assertEqual(1, mock_log_warning.call_count) - self.assertIn('is already active', mock_log_warning.call_args[0][0]) - - @mock.patch('nova.network.neutron._get_ksa_client') - def test_activate_port_binding_fails(self, mock_client): - """Tests the unknown error case of binding activation.""" - mock_client.return_value.put.return_value = fake_req.FakeResponse(500) - self.assertRaises(exception.PortBindingActivationFailed, - self.api.activate_port_binding, - self.context, uuids.port_id, 'fake-host') - mock_client.return_value.put.assert_called_once_with( - '/v2.0/ports/%s/bindings/fake-host/activate' % uuids.port_id, - raise_exc=False, - global_request_id=self.context.global_id) - class TestAllocateForInstance(test.NoDBTestCase): def setUp(self): diff --git a/requirements.txt b/requirements.txt index ae762a3c9f69..e822c73bab5b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,7 +26,7 @@ iso8601>=0.1.11 # MIT jsonschema>=3.2.0 # MIT python-cinderclient!=4.0.0,>=3.3.0 # Apache-2.0 keystoneauth1>=3.16.0 # Apache-2.0 -python-neutronclient>=6.7.0 # Apache-2.0 +python-neutronclient>=7.1.0 # Apache-2.0 python-glanceclient>=2.8.0 # Apache-2.0 requests>=2.25.1 # Apache-2.0 stevedore>=1.20.0 # Apache-2.0