Query ports with admin client to get resource_request

The port.resource_request field is admin only. Nova depends on the
value of this field to do a proper scheduling and resource allocation
and deallocation for ports with resource request as well as to update
the port.binding:profile.allocation field with the resource providers
the requested resources are fulfilled from. However in some cases nova
does not use a neutron admin client / elevated context to read the
port. In this case neutron returns None for the port.resource_request
field and nova thinks that the port has no resource request.

This patch fixes all three places where previous testing showed that
context elevation was missing.

Change-Id: Icb35e20179572fb713a397b4605312cf3294b41b
Closes-Bug: #1945310
(cherry picked from commit 49b481e)

Author: Balazs Gibizer

nova/compute/api.py

@@ -5141,7 +5141,11 @@ def attach_interface(self, context, instance, network_id, port_id,
             context, instance, instance_actions.ATTACH_INTERFACE)
 
         if port_id:
-            port = self.network_api.show_port(context, port_id)['port']
+            # We need to query the port with admin context as
+            # ensure_compute_version_for_resource_request depends on the
+            # port.resource_request field which only returned for admins
+            port = self.network_api.show_port(
+                context.elevated(), port_id)['port']
             if port.get('binding:vnic_type', "normal") == "vdpa":
                 # FIXME(sean-k-mooney): Attach works but detach results in a
                 # QEMU error; blocked until this is resolved

nova/network/neutron.py

@@ -1046,8 +1046,11 @@ def get_binding_profile_allocation(
             of request group id: resource provider UUID mapping if the port has
             an extended resource request.
         """
+        # We need to use an admin client as the port.resource_request is admin
+        # only
+        neutron_admin = get_client(context, admin=True)
         neutron = get_client(context)
-        port = self._show_port(context, port_id, neutron_client=neutron)
+        port = self._show_port(context, port_id, neutron_client=neutron_admin)
         if self._has_resource_request(context, port, neutron):
             return self._get_binding_profile_allocation(
                 context, port, neutron, resource_provider_mapping)
@@ -1724,12 +1727,15 @@ def deallocate_port_for_instance(self, context, instance, port_id):
                  Note that right now this dict only contains a single key as a
                  neutron port only allocates from a single resource provider.
         """
+        # We need to use an admin client as the port.resource_request is admin
+        # only
+        neutron_admin = get_client(context, admin=True)
         neutron = get_client(context)
         port_allocation: ty.Dict = {}
         try:
             # NOTE(gibi): we need to read the port resource information from
             # neutron here as we might delete the port below
-            port = neutron.show_port(port_id)['port']
+            port = neutron_admin.show_port(port_id)['port']
         except exception.PortNotFound:
             LOG.debug('Unable to determine port %s resource allocation '
                       'information as the port no longer exists.', port_id)

nova/tests/functional/test_servers_resource_request.py

@@ -14,7 +14,6 @@
 
 import copy
 import logging
-import unittest
 
 from keystoneauth1 import adapter
 import mock
@@ -821,11 +820,6 @@ def setUp(self):
             'os_compute_api:os-evacuate': '@',
         })
 
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_interface_attach_with_resource_request_old_compute(self):
-        super().test_interface_attach_with_resource_request_old_compute()
-
 
 class PortResourceRequestBasedSchedulingTest(
         PortResourceRequestBasedSchedulingTestBase):
@@ -1529,11 +1523,6 @@ def setUp(self):
             'os_compute_api:os-services:list': '@',
         })
 
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_interface_detach_with_port_with_bandwidth_request(self):
-        super().test_interface_detach_with_port_with_bandwidth_request()
-
 
 class ExtendedPortResourceRequestBasedSchedulingTestBase(
         PortResourceRequestBasedSchedulingTestBase):
@@ -1680,11 +1669,6 @@ def setUp(self):
             'os_compute_api:os-services:list': '@',
         })
 
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_interface_detach_with_port_with_bandwidth_request(self):
-        super().test_interface_detach_with_port_with_bandwidth_request()
-
 
 class ServerMoveWithPortResourceRequestTest(
         PortResourceRequestBasedSchedulingTestBase):
@@ -2650,23 +2634,6 @@ def setUp(self):
             'os_compute_api:os-instance-actions:list': '@',
         })
 
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_live_migrate_with_qos_port(self, host=None):
-        super().test_live_migrate_with_qos_port()
-
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_live_migrate_with_qos_port_with_target_host(self):
-        super(
-        ).test_live_migrate_with_qos_port_with_target_host()
-
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_live_migrate_with_qos_port_reschedule_success(self):
-        super(
-        ).test_live_migrate_with_qos_port_reschedule_success()
-
 
 class ServerMoveWithMultiGroupResourceRequestBasedSchedulingTest(
     ExtendedPortResourceRequestBasedSchedulingTestBase,
@@ -2717,23 +2684,6 @@ def setUp(self):
             'os_compute_api:os-instance-actions:list': '@',
         })
 
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_live_migrate_with_qos_port(self, host=None):
-        super().test_live_migrate_with_qos_port()
-
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_live_migrate_with_qos_port_with_target_host(self):
-        super(
-        ).test_live_migrate_with_qos_port_with_target_host()
-
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_live_migrate_with_qos_port_reschedule_success(self):
-        super(
-        ).test_live_migrate_with_qos_port_reschedule_success()
-
 
 class LiveMigrateAbortWithPortResourceRequestTest(
         PortResourceRequestBasedSchedulingTestBase):
@@ -3153,8 +3103,3 @@ def setUp(self):
             'os_compute_api:servers:resize': '@',
             'os_compute_api:os-evacuate': '@',
         })
-
-    # this is bug 1945310
-    @unittest.expectedFailure
-    def test_interface_attach(self, mock_get_service):
-        super().test_interface_attach()

nova/tests/unit/compute/test_api.py

@@ -7267,6 +7267,10 @@ def test_tagged_interface_attach(self, mock_attach, mock_record):
         mock_record.assert_called_once_with(
             self.context, instance, instance_actions.ATTACH_INTERFACE)
 
+    @mock.patch(
+        'nova.context.RequestContext.elevated',
+        new=mock.Mock(return_value=mock.sentinel.admin_context)
+    )
     @mock.patch(
         'nova.network.neutron.API.has_extended_resource_request_extension',
         new=mock.Mock(return_value=False),
@@ -7292,10 +7296,15 @@ def test_attach_interface_qos_aware_port_old_compute(
                 self.context, instance,
                 'foo_net_id', 'foo_port_id', None
             )
-        mock_show_port.assert_called_once_with(self.context, 'foo_port_id')
+        mock_show_port.assert_called_once_with(
+            mock.sentinel.admin_context, 'foo_port_id')
         mock_get_service.assert_called_once_with(
             self.context, instance.host, 'nova-compute')
 
+    @mock.patch(
+        'nova.context.RequestContext.elevated',
+        new=mock.Mock(return_value=mock.sentinel.admin_context)
+    )
     @mock.patch(
         'nova.network.neutron.API.has_extended_resource_request_extension',
         new=mock.Mock(return_value=False)
@@ -7326,14 +7335,18 @@ def test_attach_interface_qos_aware_port(
                 mock.sentinel.port_id, mock.sentinel.ip, mock.sentinel.tag)
 
         mock_show_port.assert_called_once_with(
-            self.context, mock.sentinel.port_id)
+            mock.sentinel.admin_context, mock.sentinel.port_id)
         mock_get_service.assert_called_once_with(
             self.context, instance.host, 'nova-compute')
         mock_attach.assert_called_once_with(
             self.context, instance=instance, network_id=mock.sentinel.net_id,
             port_id=mock.sentinel.port_id, requested_ip=mock.sentinel.ip,
             tag=mock.sentinel.tag)
 
+    @mock.patch(
+        'nova.context.RequestContext.elevated',
+        new=mock.Mock(return_value=mock.sentinel.admin_context)
+    )
     @mock.patch(
         'nova.network.neutron.API.has_extended_resource_request_extension',
         new=mock.Mock(return_value=True),
@@ -7367,10 +7380,15 @@ def test_attach_interface_extended_qos_port_old_compute(
                 self.context, instance,
                 'foo_net_id', 'foo_port_id', None
             )
-        mock_show_port.assert_called_once_with(self.context, 'foo_port_id')
+        mock_show_port.assert_called_once_with(
+            mock.sentinel.admin_context, 'foo_port_id')
         mock_get_service.assert_called_once_with(
             self.context, instance.host, 'nova-compute')
 
+    @mock.patch(
+        'nova.context.RequestContext.elevated',
+        new=mock.Mock(return_value=mock.sentinel.admin_context)
+    )
     @mock.patch(
         'nova.network.neutron.API.has_extended_resource_request_extension',
         new=mock.Mock(return_value=True)
@@ -7404,7 +7422,7 @@ def test_attach_interface_extended_qos_port(
                 mock.sentinel.port_id, mock.sentinel.ip, mock.sentinel.tag)
 
         mock_show_port.assert_called_once_with(
-            self.context, mock.sentinel.port_id)
+            mock.sentinel.admin_context, mock.sentinel.port_id)
         mock_get_service.assert_called_once_with(
             self.context, instance.host, 'nova-compute')
         mock_attach.assert_called_once_with(

nova/tests/unit/network/test_neutron.py

@@ -1750,7 +1750,9 @@ def _test_deallocate_port_for_instance(self, number, mock_get_client,
         mocked_client.delete_port.assert_called_once_with(port_data[0]['id'])
         mocked_client.show_port.assert_called_once_with(port_data[0]['id'])
         expected_get_client_calls = [
-            mock.call(self.context), mock.call(self.context, admin=True)]
+            mock.call(self.context, admin=True),
+            mock.call(self.context, admin=True),
+        ]
         if number == 2:
             expected_get_client_calls.append(mock.call(self.context,
                                                        admin=True))
@@ -7146,25 +7148,33 @@ def test__has_resource_request(self, mock_has_extended_res_req):
         "nova.network.neutron.API.has_extended_resource_request_extension",
         new=mock.Mock(return_value=False),
     )
-    @mock.patch("neutronclient.v2_0.client.Client.show_port")
-    def test_get_binding_profile_allocation_no_request(self, mock_show_port):
-        mock_show_port.return_value = {
+    @mock.patch("nova.network.neutron.get_client")
+    def test_get_binding_profile_allocation_no_request(
+        self, mock_get_client
+    ):
+        mock_get_client.return_value.show_port.return_value = {
             "port": {
             }
         }
         self.assertIsNone(
             self.api.get_binding_profile_allocation(
                 self.context, uuids.port_uuid, {}))
+        mock_get_client.assert_has_calls(
+            [
+                mock.call(self.context, admin=True),
+                mock.call(self.context),
+            ]
+        )
 
     @mock.patch(
         "nova.network.neutron.API.has_extended_resource_request_extension",
         new=mock.Mock(return_value=False),
     )
-    @mock.patch("neutronclient.v2_0.client.Client.show_port")
+    @mock.patch("nova.network.neutron.get_client")
     def test_get_binding_profile_allocation_legacy_request(
-        self, mock_show_port
+        self, mock_get_client
     ):
-        mock_show_port.return_value = {
+        mock_get_client.return_value.show_port.return_value = {
             "port": {
                 "id": uuids.port_uuid,
                 "resource_request": {
@@ -7188,11 +7198,11 @@ def test_get_binding_profile_allocation_legacy_request(
         "nova.network.neutron.API.has_extended_resource_request_extension",
         new=mock.Mock(return_value=True),
     )
-    @mock.patch("neutronclient.v2_0.client.Client.show_port")
+    @mock.patch("nova.network.neutron.get_client")
     def test_get_binding_profile_allocation_extended_request(
-        self, mock_show_port
+        self, mock_get_client
     ):
-        mock_show_port.return_value = {
+        mock_get_client.return_value.show_port.return_value = {
             "port": {
                 "id": uuids.port_uuid,
                 "resource_request": {