From 8364abecfafdbe2a7f65ea5e7b787bcbb6412844 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Tue, 22 May 2018 15:38:09 -0700 Subject: [PATCH] Reject networks with QoS policy When nova needs to create ports in Neutron in a network that has minimum bandwidth policy Nova would need to create allocation for the bandwidth resources. The port creation happens in the compute manager after the scheduling and resource claiming. Supporting for this is considered out of scope for the first iteration of this feature. To avoid resource allocation inconsistencies this patch propose to reject such request. This rejection does not break any existing use case as minimum bandwidth policy rule only supported by the SRIOV Neutron backend but Nova only support booting with SRIOV port if those ports are precreated in Neutron. Co-Authored-By: Elod Illes Change-Id: I7e1edede827cf8469771c0496b1dce55c627cf5d blueprint: bandwidth-resource-provider --- .../openstack/compute/attach_interfaces.py | 3 +- nova/compute/manager.py | 3 +- nova/exception.py | 5 ++ nova/network/neutronv2/api.py | 24 ++++++++++ nova/tests/functional/test_servers.py | 47 +++++++++++++++++++ nova/tests/unit/network/test_neutronv2.py | 43 +++++++++++++++++ nova/virt/fake.py | 6 +++ ...orks-with-qos-policy-2746c74fd1f3ff26.yaml | 9 ++++ 8 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/reject-networks-with-qos-policy-2746c74fd1f3ff26.yaml diff --git a/nova/api/openstack/compute/attach_interfaces.py b/nova/api/openstack/compute/attach_interfaces.py index 717b85f132f6..891747650ab6 100644 --- a/nova/api/openstack/compute/attach_interfaces.py +++ b/nova/api/openstack/compute/attach_interfaces.py @@ -137,7 +137,8 @@ class InterfaceAttachmentController(wsgi.Controller): exception.PortNotUsable, exception.AttachInterfaceNotSupported, exception.SecurityGroupCannotBeApplied, - exception.NetworkInterfaceTaggedAttachNotSupported) as e: + exception.NetworkInterfaceTaggedAttachNotSupported, + exception.NetworksWithQoSPolicyNotSupported) as e: raise exc.HTTPBadRequest(explanation=e.format_message()) except (exception.InstanceIsLocked, exception.FixedIpAlreadyInUse, diff --git a/nova/compute/manager.py b/nova/compute/manager.py index a24743c2044c..0306415d4eb3 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -2170,7 +2170,8 @@ class ComputeManager(manager.Manager): except (exception.VirtualInterfaceCreateException, exception.VirtualInterfaceMacAddressException, exception.FixedIpInvalidOnHost, - exception.UnableToAutoAllocateNetwork) as e: + exception.UnableToAutoAllocateNetwork, + exception.NetworksWithQoSPolicyNotSupported) as e: LOG.exception('Failed to allocate network(s)', instance=instance) self._notify_about_instance_usage(context, instance, diff --git a/nova/exception.py b/nova/exception.py index b5ee5a4a4fad..27761383400c 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -2148,6 +2148,11 @@ class AttachInterfaceWithQoSPolicyNotSupported(AttachInterfaceNotSupported): "instance %(instance_uuid)s.") +class NetworksWithQoSPolicyNotSupported(Invalid): + msg_fmt = _("Using networks with QoS policy is not supported for " + "instance %(instance_uuid)s. (Network ID is %(network_id)s)") + + class InvalidReservedMemoryPagesOption(Invalid): msg_fmt = _("The format of the option 'reserved_huge_pages' is invalid. " "(found '%(conf)s') Please refer to the nova " diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 2ebf8f2c9d69..bfcc5de8061c 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -472,6 +472,15 @@ class API(base_api.NetworkAPI): return nets + def _cleanup_created_port(self, port_client, port_id, instance): + try: + port_client.delete_port(port_id) + except neutron_client_exc.NeutronClientException: + LOG.exception( + 'Failed to delete port %(port_id)s while cleaning up after an ' + 'error.', {'port_id': port_id}, + instance=instance) + def _create_port_minimal(self, port_client, instance, network_id, fixed_ip=None, security_group_ids=None): """Attempts to create a port for the instance on the given network. @@ -487,6 +496,8 @@ class API(base_api.NetworkAPI): :raises NoMoreFixedIps: If neutron fails with IpAddressGenerationFailure error. :raises: PortBindingFailed: If port binding failed. + :raises NetworksWithQoSPolicyNotSupported: if the created port has + resource request. """ # Set the device_id so it's clear who this port was created for, # and to stop other instances trying to use it @@ -505,6 +516,19 @@ class API(base_api.NetworkAPI): port = port_response['port'] port_id = port['id'] + + # NOTE(gibi): Checking if the created port has resource request as + # such ports are currently not supported as they would at least + # need resource allocation manipulation in placement but might also + # need a new scheduling if resource on this host is not available. + if port.get('resource_request', None): + self._cleanup_created_port(port_client, port_id, instance) + # NOTE(gibi): This limitation regarding server create can be + # removed when the port creation is moved to the conductor. But + # this code also limits attaching a network that has QoS + # minimum bandwidth rule. + raise exception.NetworksWithQoSPolicyNotSupported( + instance_uuid=instance.uuid, network_id=network_id) try: _ensure_no_port_binding_failure(port) except exception.PortBindingFailed: diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index 9fc03758c8e3..92764a722f3f 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -14,6 +14,7 @@ # under the License. import collections +import copy import datetime import time import zlib @@ -5395,3 +5396,49 @@ class PortResourceRequestBasedSchedulingTest( self.assertIn('Attaching interfaces with QoS policy is ' 'not supported for instance', six.text_type(ex)) + + @mock.patch('nova.tests.fixtures.NeutronFixture.create_port') + def test_interface_attach_with_network_create_port_has_resource_request( + self, mock_neutron_create_port): + # create a server + server = self._create_server( + flavor=self.flavor, + networks=[{'port': self.neutron.port_1['id']}]) + self._wait_for_state_change(self.admin_api, server, 'ACTIVE') + + # the interfaceAttach operation below will result in a new port being + # created in the network that is attached. Make sure that neutron + # returns a port that has resource request. + mock_neutron_create_port.return_value = ( + {'port': copy.deepcopy(self.neutron.port_with_resource_request)}) + + # try to attach a network + post = { + 'interfaceAttachment': { + 'net_id': self.neutron.network_1['id'] + }} + ex = self.assertRaises(client.OpenStackApiException, + self.api.attach_interface, + server['id'], post) + self.assertEqual(400, ex.response.status_code) + self.assertIn('Using networks with QoS policy is not supported for ' + 'instance', + six.text_type(ex)) + + @mock.patch('nova.tests.fixtures.NeutronFixture.create_port') + def test_create_server_with_network_create_port_has_resource_request( + self, mock_neutron_create_port): + # the server create operation below will result in a new port being + # created in the network. Make sure that neutron returns a port that + # has resource request. + mock_neutron_create_port.return_value = ( + {'port': copy.deepcopy(self.neutron.port_with_resource_request)}) + + server = self._create_server( + flavor=self.flavor, + networks=[{'uuid': self.neutron.network_1['id']}]) + server = self._wait_for_state_change(self.admin_api, server, 'ERROR') + + self.assertEqual(500, server['fault']['code']) + self.assertIn('Failed to allocate the network', + server['fault']['message']) diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index 41fed9875233..96e00708005e 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -3846,6 +3846,49 @@ class TestNeutronv2WithMock(_TestNeutronv2Common): self.assertEqual(expected_exception_msg, str(exc)) self.assertTrue(create_port_mock.called) + def test_create_port_minimal_raise_qos_not_supported(self): + instance = fake_instance.fake_instance_obj(self.context) + mock_client = mock.MagicMock() + mock_client.create_port.return_value = {'port': { + 'id': uuids.port_id, + 'resource_request': {'resources': {'CUSTOM_RESOURCE_CLASS': 42}} + }} + + exc = self.assertRaises(exception.NetworksWithQoSPolicyNotSupported, + self.api._create_port_minimal, + mock_client, instance, uuids.my_netid1) + expected_exception_msg = ('Using networks with QoS policy is not ' + 'supported for instance %(instance)s. ' + '(Network ID is %(net_id)s)' % + {'instance': instance.uuid, + 'net_id': uuids.my_netid1}) + self.assertEqual(expected_exception_msg, six.text_type(exc)) + mock_client.delete_port.assert_called_once_with(uuids.port_id) + + @mock.patch('nova.network.neutronv2.api.LOG') + def test_create_port_minimal_raise_qos_not_supported_cleanup_fails( + self, mock_log): + instance = fake_instance.fake_instance_obj(self.context) + mock_client = mock.MagicMock() + mock_client.create_port.return_value = {'port': { + 'id': uuids.port_id, + 'resource_request': {'resources': {'CUSTOM_RESOURCE_CLASS': 42}} + }} + mock_client.delete_port.side_effect = \ + exceptions.NeutronClientException() + + exc = self.assertRaises(exception.NetworksWithQoSPolicyNotSupported, + self.api._create_port_minimal, + mock_client, instance, uuids.my_netid1) + expected_exception_msg = ('Using networks with QoS policy is not ' + 'supported for instance %(instance)s. ' + '(Network ID is %(net_id)s)' % + {'instance': instance.uuid, + 'net_id': uuids.my_netid1}) + self.assertEqual(expected_exception_msg, six.text_type(exc)) + mock_client.delete_port.assert_called_once_with(uuids.port_id) + self.assertTrue(mock_log.exception.called) + def test_get_network_detail_not_found(self): api = neutronapi.API() expected_exc = exceptions.NetworkNotFoundClient() diff --git a/nova/virt/fake.py b/nova/virt/fake.py index b4e222217d35..6b0f59b11783 100644 --- a/nova/virt/fake.py +++ b/nova/virt/fake.py @@ -191,6 +191,12 @@ class FakeDriver(driver.ComputeDriver): def spawn(self, context, instance, image_meta, injected_files, admin_password, allocations, network_info=None, block_device_info=None): + + # simulate a real driver triggering the async network allocation as it + # might cause an error + if network_info: + [ip for vif in network_info for ip in vif.fixed_ips()] + uuid = instance.uuid state = power_state.RUNNING flavor = instance.flavor diff --git a/releasenotes/notes/reject-networks-with-qos-policy-2746c74fd1f3ff26.yaml b/releasenotes/notes/reject-networks-with-qos-policy-2746c74fd1f3ff26.yaml new file mode 100644 index 000000000000..94c7ec2266c0 --- /dev/null +++ b/releasenotes/notes/reject-networks-with-qos-policy-2746c74fd1f3ff26.yaml @@ -0,0 +1,9 @@ +--- +other: + - | + The ``POST /servers/{server_id}/os-interface`` request and the + ``POST /servers`` request will be rejected with HTTP 400 if the Neutron + network referenced in the request body has `QoS minimum bandwidth rule`_ + attached as Nova currently cannot support such operations. + + .. _QoS minimum bandwidth rule: https://docs.openstack.org/neutron/latest/admin/config-qos.html