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

5 files changed, 55 insertions, 72 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index d41d8b6109..4382a5adfb 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -5141,7 +5141,11 @@ class API:
             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
diff --git a/nova/network/neutron.py b/nova/network/neutron.py
index 00b9b4009e..d8c58c6e13 100644
--- a/nova/network/neutron.py
+++ b/nova/network/neutron.py
@@ -1046,8 +1046,11 @@ class API:
             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 @@ class API:
                  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)
diff --git a/nova/tests/functional/test_servers_resource_request.py b/nova/tests/functional/test_servers_resource_request.py
index e2746b3669..bb071543fd 100644
--- a/nova/tests/functional/test_servers_resource_request.py
+++ b/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 @@ class NonAdminUnsupportedPortResourceRequestBasedSchedulingTest(
             '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 @@ class NonAdminPortResourceRequestTests(
             '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 @@ class NonAdminMultiGroupResReqTests(
             '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 @@ class NonAdminServerMoveWithPortResourceRequestTests(
             '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 @@ class NonAdminServerMoveWithMultiGroupResReqTests(
             '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 @@ class NonAdminExtendedResourceRequestOldCompute(
             '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()
diff --git a/nova/tests/unit/compute/test_api.py b/nova/tests/unit/compute/test_api.py
index 64be53f923..188294c3c4 100644
--- a/nova/tests/unit/compute/test_api.py
+++ b/nova/tests/unit/compute/test_api.py
@@ -7268,6 +7268,10 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase):
             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,11 +7296,16 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase):
                 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,7 +7335,7 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase):
                 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(
@@ -7335,6 +7344,10 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase):
             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,11 +7380,16 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase):
                 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 @@ class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase):
                 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(
diff --git a/nova/tests/unit/network/test_neutron.py b/nova/tests/unit/network/test_neutron.py
index a55c091635..5056b70c4e 100644
--- a/nova/tests/unit/network/test_neutron.py
+++ b/nova/tests/unit/network/test_neutron.py
@@ -1750,7 +1750,9 @@ class TestAPI(TestAPIBase):
         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 @@ class TestAPI(TestAPIBase):
         "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 @@ class TestAPI(TestAPIBase):
         "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": {