From 4cb493fe4f79e736ae4fd7874b02d1d5ceed02c1 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Tue, 29 Oct 2019 16:43:04 +0100 Subject: [PATCH] Use admin neutron client to see if instance has qos ports The nova-api checks at each move* operation if the instance has qos port attached as not all the move operations are supported for such servers. Nova uses the request context to initialize the neutron client for the port query. However neutron does not return the value of the resource_request of the port if it is queried with a non admin client. This causes that if the move operation is initiated by a non admin then nova thinks that the ports do not have resource request. This patch creates an admin context for this neutron query. The new functional tests are not added before this patch in a regression test like way as existing functional tests are reused with different setup and doing that without the fix causes a lot of different failure scenarios. Note that neutron fixture is changed to simulate the different behavior in case of different request context are used to initialize the client. *: Note that Id5f2f4f22b856c989e2eef8ed56b9829d1bcefb6 removed the check for evacuate in Ussuri but exists in Train and Stein. Conflicts: nova/tests/fixtures.py Due to d2d4317e1a31f6ac40c5f29b57cd4ea196dc8780 is missing from stable/train Another difference from the original patch is in nova/api/openstack/compute/evacuate.py due to Id5f2f4f22b856c989e2eef8ed56b9829d1bcefb6 is only merged in Ussuri. Change-Id: I3cf6eb4654663865d9258c38f05cd05974ffcf9d Closes-Bug: #1850280 (cherry picked from commit 899976960503524b8e5c6588e339742ca4bf8158) --- nova/api/openstack/common.py | 11 +++++--- nova/api/openstack/compute/evacuate.py | 2 +- nova/api/openstack/compute/migrate_server.py | 4 +-- nova/api/openstack/compute/servers.py | 2 +- nova/api/openstack/compute/shelve.py | 2 +- nova/tests/fixtures.py | 23 ++++++++++++++++- nova/tests/functional/integrated_helpers.py | 2 +- nova/tests/functional/test_servers.py | 25 +++++++++++++++++++ .../openstack/compute/test_migrate_server.py | 3 +-- .../openstack/compute/test_server_actions.py | 3 +-- nova/tests/unit/api/openstack/test_common.py | 19 ++++++++++++++ 11 files changed, 82 insertions(+), 14 deletions(-) diff --git a/nova/api/openstack/common.py b/nova/api/openstack/common.py index c2bab9da0dfa..5b64ef5e48e0 100644 --- a/nova/api/openstack/common.py +++ b/nova/api/openstack/common.py @@ -28,6 +28,7 @@ from nova.api.openstack import api_version_request from nova.compute import task_states from nova.compute import vm_states import nova.conf +from nova import context as nova_context from nova import exception from nova.i18n import _ from nova.network.neutronv2 import constants @@ -573,8 +574,7 @@ def supports_port_resource_request_during_move(req): return False -def instance_has_port_with_resource_request( - context, instance_uuid, network_api): +def instance_has_port_with_resource_request(instance_uuid, network_api): # TODO(gibi): Use instance.info_cache to see if there is VIFs with # allocation key in the profile. If there is no such VIF for an instance @@ -583,7 +583,12 @@ def instance_has_port_with_resource_request( # offloaded then we still have to hit neutron. search_opts = {'device_id': instance_uuid, 'fields': [constants.RESOURCE_REQUEST]} - ports = network_api.list_ports(context, **search_opts).get('ports', []) + # NOTE(gibi): We need to use an admin context to query neutron ports as + # neutron does not fill the resource_request field in the port response if + # we query with a non admin context. + admin_context = nova_context.get_admin_context() + ports = network_api.list_ports( + admin_context, **search_opts).get('ports', []) for port in ports: if port.get(constants.RESOURCE_REQUEST): return True diff --git a/nova/api/openstack/compute/evacuate.py b/nova/api/openstack/compute/evacuate.py index 180da2a0d19a..f366ba8c8c66 100644 --- a/nova/api/openstack/compute/evacuate.py +++ b/nova/api/openstack/compute/evacuate.py @@ -121,7 +121,7 @@ class EvacuateController(wsgi.Controller): # extra API call to neutron when we support move operations with ports # having resource requests. if (common.instance_has_port_with_resource_request( - context, instance.uuid, self.network_api) and not + instance.uuid, self.network_api) and not common.supports_port_resource_request_during_move(req)): msg = _("The evacuate action on a server with ports having " "resource requests, like a port with a QoS minimum " diff --git a/nova/api/openstack/compute/migrate_server.py b/nova/api/openstack/compute/migrate_server.py index dd5eff8e3a54..f67e10a50c5b 100644 --- a/nova/api/openstack/compute/migrate_server.py +++ b/nova/api/openstack/compute/migrate_server.py @@ -59,7 +59,7 @@ class MigrateServerController(wsgi.Controller): expected_attrs=['flavor']) if common.instance_has_port_with_resource_request( - context, instance.uuid, self.network_api): + instance.uuid, self.network_api): # TODO(gibi): Remove when nova only supports compute newer than # Train source_service = objects.Service.get_by_host_and_binary( @@ -132,7 +132,7 @@ class MigrateServerController(wsgi.Controller): # extra API call to neutron when we support move operations with ports # having resource requests. if (common.instance_has_port_with_resource_request( - context, instance.uuid, self.network_api) and not + instance.uuid, self.network_api) and not common.supports_port_resource_request_during_move(req)): msg = _("The os-migrateLive action on a server with ports having " "resource requests, like a port with a QoS minimum " diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 2148fc99ff66..b7dae423665d 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -941,7 +941,7 @@ class ServersController(wsgi.Controller): 'project_id': instance.project_id}) if common.instance_has_port_with_resource_request( - context, instance_id, self.network_api): + instance_id, self.network_api): # TODO(gibi): Remove when nova only supports compute newer than # Train source_service = objects.Service.get_by_host_and_binary( diff --git a/nova/api/openstack/compute/shelve.py b/nova/api/openstack/compute/shelve.py index ae8bea989ce2..d26a58001e03 100644 --- a/nova/api/openstack/compute/shelve.py +++ b/nova/api/openstack/compute/shelve.py @@ -97,7 +97,7 @@ class ShelveController(wsgi.Controller): # having resource requests. if (instance.vm_state == vm_states.SHELVED_OFFLOADED and common.instance_has_port_with_resource_request( - context, instance.uuid, self.network_api) and + instance.uuid, self.network_api) and not common.supports_port_resource_request_during_move( req)): msg = _("The unshelve action on a server with ports having " diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index 4944afc6739d..30d76b2c123e 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -1512,7 +1512,20 @@ class NeutronFixture(fixtures.Fixture): self.fake_delete_port_binding) self.test.stub_out('nova.network.neutronv2.api.get_client', - lambda *args, **kwargs: self) + self._get_client) + + def _get_client(self, context, admin=False): + # NOTE(gibi): This is a hack. As we return the same fixture for each + # get_client call there is no way to later know that a call came + # through which client. We store the parameters of the last get_client + # call. Later we should return a new client object from this call that + # is wrapping the fixture and this client can remember how it was + # initialized. + + # This logic is copied from nova.network.neutronv2.api._get_auth_plugin + self.is_admin_client = (admin or + (context.is_admin and not context.auth_token)) + return self @staticmethod def fake_create_port_binding(context, client, port_id, data): @@ -1577,6 +1590,14 @@ class NeutronFixture(fixtures.Fixture): _params.pop('fields', None) ports = [p for p in self._ports.values() if all(p.get(opt) == _params[opt] for opt in _params)] + if not self.is_admin_client: + # Neutron returns None instead of the real resource_request if + # the ports are queried by a non-admin. So simulate this behavior + # here + for port in ports: + if 'resource_request' in port: + port['resource_request'] = None + return {'ports': copy.deepcopy(ports)} def list_subnets(self, retrieve_all=True, **_params): diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index 767b5104e918..4a9b6715e60c 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -426,7 +426,7 @@ class ProviderUsageBaseTestCase(test.TestCase, InstanceHelperMixin): self.flags(compute_driver=self.compute_driver) super(ProviderUsageBaseTestCase, self).setUp() - self.useFixture(policy_fixture.RealPolicyFixture()) + self.policy = self.useFixture(policy_fixture.RealPolicyFixture()) self.neutron = self.useFixture(nova_fixtures.NeutronFixture(self)) self.useFixture(nova_fixtures.AllServicesCurrent()) diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index 2cdd330c5a5e..7cc393ee18a5 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -5902,6 +5902,31 @@ class UnsupportedPortResourceRequestBasedSchedulingTest( self._wait_for_state_change(self.admin_api, server, 'ACTIVE') +class NonAdminUnsupportedPortResourceRequestBasedSchedulingTest( + UnsupportedPortResourceRequestBasedSchedulingTest): + + def setUp(self): + super( + NonAdminUnsupportedPortResourceRequestBasedSchedulingTest, + self).setUp() + # switch to non admin api + self.api = self.api_fixture.api + self.api.microversion = self.microversion + + # allow non-admin to call the operations + self.policy.set_rules({ + 'os_compute_api:os-evacuate': '@', + 'os_compute_api:servers:create': '@', + 'os_compute_api:servers:create:attach_network': '@', + 'os_compute_api:servers:show': '@', + 'os_compute_api:os-attach-interfaces': '@', + 'os_compute_api:os-attach-interfaces:create': '@', + 'os_compute_api:os-shelve:shelve': '@', + 'os_compute_api:os-shelve:unshelve': '@', + 'os_compute_api:os-migrate-server:migrate_live': '@', + }) + + class PortResourceRequestBasedSchedulingTest( PortResourceRequestBasedSchedulingTestBase): """Tests creating a server with a pre-existing port that has a resource diff --git a/nova/tests/unit/api/openstack/compute/test_migrate_server.py b/nova/tests/unit/api/openstack/compute/test_migrate_server.py index 2bd91c6385ab..99572af1bd1c 100644 --- a/nova/tests/unit/api/openstack/compute/test_migrate_server.py +++ b/nova/tests/unit/api/openstack/compute/test_migrate_server.py @@ -314,8 +314,7 @@ class MigrateServerTestsV21(admin_only_action_common.CommonTests): instance['uuid'], body={'migrate': None}) mock_has_res_req.assert_called_once_with( - self.req.environ['nova.context'], instance['uuid'], - self.controller.network_api) + instance['uuid'], self.controller.network_api) mock_get_service.assert_called_once_with( self.req.environ['nova.context'], instance['host'], 'nova-compute') diff --git a/nova/tests/unit/api/openstack/compute/test_server_actions.py b/nova/tests/unit/api/openstack/compute/test_server_actions.py index da3813ba7820..ae4a0dbde5e4 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_actions.py +++ b/nova/tests/unit/api/openstack/compute/test_server_actions.py @@ -1240,7 +1240,6 @@ class ServerActionsControllerTestV21(test.TestCase): self.req, FAKE_UUID, body=body) mock_has_res_req.assert_called_once_with( - self.req.environ['nova.context'], FAKE_UUID, - self.controller.network_api) + FAKE_UUID, self.controller.network_api) mock_get_service.assert_called_once_with( self.req.environ['nova.context'], 'fake_host', 'nova-compute') diff --git a/nova/tests/unit/api/openstack/test_common.py b/nova/tests/unit/api/openstack/test_common.py index 9eecb1fa3193..4329ca9da66d 100644 --- a/nova/tests/unit/api/openstack/test_common.py +++ b/nova/tests/unit/api/openstack/test_common.py @@ -27,7 +27,9 @@ import webob.multidict from nova.api.openstack import common from nova.compute import task_states from nova.compute import vm_states +from nova import context from nova import exception +from nova import network from nova import test from nova.tests.unit.api.openstack import fakes @@ -449,6 +451,23 @@ class MiscFunctionsTest(test.TestCase): self.assertRaises(exception.InvalidInput, common.is_all_tenants, search_opts) + def test_instance_has_port_with_resource_request(self): + network_api = mock.Mock(spec=network.API()) + network_api.list_ports.return_value = {'ports': [ + {'resource_request': mock.sentinel.request} + ]} + res = common.instance_has_port_with_resource_request( + mock.sentinel.uuid, network_api) + + self.assertTrue(res) + network_api.list_ports.assert_called_once_with( + test.MatchType(context.RequestContext), + device_id=mock.sentinel.uuid, fields=['resource_request']) + # assert that the neutron call uses an admin context + ctxt = network_api.mock_calls[0][1][0] + self.assertTrue(ctxt.is_admin) + self.assertIsNone(ctxt.auth_token) + class TestCollectionLinks(test.NoDBTestCase): """Tests the _get_collection_links method."""