Refactor _claim_pci_device_for_interface_attach to prepare for qos

This removes some of the non PCI specific part of the
_claim_pci_device_for_interface_attach() to reuse them later for the qos
handling. The goal is to have the create_resource_requests separated
from the actual PCI claim.

Change-Id: Id19702da05cac25192f6aa21f8f4449602ef7729
Blueprint: support-interface-attach-with-qos-ports

Author: Balazs Gibizer

nova/compute/manager.py

@@ -7518,68 +7518,36 @@ def _claim_pci_device_for_interface_attach(
         self,
         context: nova.context.RequestContext,
         instance: 'objects.Instance',
-        requested_networks: 'objects.NetworkRequestsList'
+        pci_reqs: 'objects.InstancePCIRequests',
     ) -> ty.Optional['objects.PciDevice']:
-        """Claim PCI device if the requested interface needs it
-
-        If a PCI device is claimed then the requested_networks is updated
-        with the pci request id and the pci_requests and pci_devices fields
-        of the instance is also updated accordingly.
+        """Claim PCI devices if there are PCI requests
 
         :param context: nova.context.RequestContext
         :param instance: the objects.Instance to where the interface is being
             attached
-        :param requested_networks: the objects.NetworkRequestList describing
-            the requested interface. The requested_networks.objects list shall
-            have a single item.
+        :param pci_reqs: A InstancePCIRequests object describing the
+            needed PCI devices
         :raises InterfaceAttachPciClaimFailed: if the PCI device claim fails
-        :raises InterfaceAttachFailed: if more than one interface is requested
         :returns: An objects.PciDevice describing the claimed PCI device for
             the interface or None if no device is requested
         """
 
-        if len(requested_networks) != 1:
-            LOG.warning(
-                "Interface attach only supports one interface per attach "
-                "request", instance=instance)
-            raise exception.InterfaceAttachFailed(instance_uuid=instance.uuid)
-
-        requested_network = requested_networks[0]
-
-        pci_numa_affinity_policy = hardware.get_pci_numa_policy_constraint(
-            instance.flavor, instance.image_meta)
-        pci_reqs = objects.InstancePCIRequests(
-            requests=[], instance_uuid=instance.uuid)
-        self.network_api.create_resource_requests(
-            context, requested_networks, pci_reqs,
-            affinity_policy=pci_numa_affinity_policy)
-
         if not pci_reqs.requests:
-            # The requested port does not require a PCI device so nothing to do
             return None
 
-        if len(pci_reqs.requests) > 1:
-            LOG.warning(
-                "Interface attach only supports one interface per attach "
-                "request", instance=instance)
-            raise exception.InterfaceAttachFailed(instance_uuid=instance.uuid)
-
-        pci_req = pci_reqs.requests[0]
-
         devices = self.rt.claim_pci_devices(
             context, pci_reqs, instance.numa_topology)
 
         if not devices:
             LOG.info('Failed to claim PCI devices during interface attach '
-                     'for PCI request %s', pci_req, instance=instance)
+                     'for PCI request %s', pci_reqs, instance=instance)
             raise exception.InterfaceAttachPciClaimFailed(
                 instance_uuid=instance.uuid)
 
+        # NOTE(gibi): We assume that maximum one PCI devices is attached per
+        # interface attach request.
         device = devices[0]
-
-        # Update the requested network with the pci request id
-        requested_network.pci_request_id = pci_req.request_id
-        instance.add_pci_device_and_request(device, pci_req)
+        instance.pci_devices.objects.append(device)
 
         return device
 
@@ -7643,8 +7611,35 @@ def _attach_interface(self, context, instance, network_id, port_id,
             ]
         )
 
-        pci_device = self._claim_pci_device_for_interface_attach(
-            context, instance, requested_networks)
+        if len(requested_networks) != 1:
+            LOG.warning(
+                "Interface attach only supports one interface per attach "
+                "request", instance=instance)
+            raise exception.InterfaceAttachFailed(instance_uuid=instance.uuid)
+
+        pci_numa_affinity_policy = hardware.get_pci_numa_policy_constraint(
+            instance.flavor, instance.image_meta)
+        pci_reqs = objects.InstancePCIRequests(
+            requests=[], instance_uuid=instance.uuid)
+        self.network_api.create_resource_requests(
+            context, requested_networks, pci_reqs,
+            affinity_policy=pci_numa_affinity_policy)
+
+        # We only support one port per attach request so we at most have one
+        # pci request
+        pci_req = None
+        if pci_reqs.requests:
+            pci_req = pci_reqs.requests[0]
+            requested_networks[0].pci_request_id = pci_req.request_id
+            instance.pci_requests.requests.append(pci_req)
+
+        try:
+            pci_device = self._claim_pci_device_for_interface_attach(
+                context, instance, pci_reqs)
+        except exception.InterfaceAttachPciClaimFailed:
+            with excutils.save_and_reraise_exception():
+                if pci_req:
+                    instance.pci_requests.requests.remove(pci_req)
 
         network_info = self.network_api.allocate_for_instance(
             context,

nova/objects/instance.py

@@ -1225,6 +1225,7 @@ def remove_pci_device_and_request(self, pci_device):
             pci_req for pci_req in self.pci_requests.requests
             if pci_req.request_id != pci_device.request_id]
 
+    # TODO(gibi): remove this as it is unused
     def add_pci_device_and_request(self, pci_device, pci_request):
         self.pci_requests.requests.append(pci_request)
         self.pci_devices.objects.append(pci_device)

nova/tests/unit/compute/test_compute.py

@@ -10145,11 +10145,12 @@ def test_attach_interface(self, mock_notify):
             mock.patch.dict(self.compute.driver.capabilities,
                              supports_attach_interface=True),
             mock.patch('oslo_concurrency.lockutils.lock'),
+            mock.patch("nova.network.neutron.API.create_resource_requests"),
             mock.patch.object(
                 self.compute,
                 "_claim_pci_device_for_interface_attach",
                 return_value=None)
-        ) as (cap, mock_lock, mock_claim_pci):
+        ) as (cap, mock_lock, mock_create_resource_req, mock_claim_pci):
             vif = self.compute.attach_interface(self.context,
                                                 instance,
                                                 network_id,
@@ -10175,7 +10176,12 @@ def test_attach_interface(self, mock_notify):
                 mock.ANY, delay=mock.ANY, do_log=mock.ANY, fair=mock.ANY,
                 semaphores=mock.ANY)
         mock_claim_pci.assert_called_once_with(
-            self.context, instance, network_requests)
+            self.context, instance,
+            test.MatchType(objects.InstancePCIRequests))
+        pci_reqs = mock_claim_pci.mock_calls[0][1][2]
+        self.assertEqual(instance.uuid, pci_reqs.instance_uuid)
+        self.assertEqual([], pci_reqs.requests)
+
         return nwinfo, port_id
 
     @mock.patch.object(compute_utils, 'notify_about_instance_action')
@@ -10262,10 +10268,11 @@ def test_interface_tagged_attach(self, mock_notify):
             mock.patch.dict(self.compute.driver.capabilities,
                             supports_attach_interface=True,
                             supports_tagged_attach_interface=True),
+            mock.patch("nova.network.neutron.API.create_resource_requests"),
             mock.patch.object(self.compute,
                               '_claim_pci_device_for_interface_attach',
                               return_value=None)
-        ) as (mock_capabilities, mock_claim_pci):
+        ) as (mock_capabilities, mock_create_resource_req, mock_claim_pci):
             vif = self.compute.attach_interface(self.context,
                                                 instance,
                                                 network_id,
@@ -10289,7 +10296,11 @@ def test_interface_tagged_attach(self, mock_notify):
             mock.call(self.context, instance, self.compute.host,
                       action='interface_attach', phase='end')])
         mock_claim_pci.assert_called_once_with(
-            self.context, instance, network_requests)
+            self.context, instance,
+            test.MatchType(objects.InstancePCIRequests))
+        pci_reqs = mock_claim_pci.mock_calls[0][1][2]
+        self.assertEqual(instance.uuid, pci_reqs.instance_uuid)
+        self.assertEqual([], pci_reqs.requests)
 
         return nwinfo, port_id
 
@@ -10329,11 +10340,12 @@ def test_attach_interface_failed(self):
                               'deallocate_port_for_instance'),
             mock.patch.dict(self.compute.driver.capabilities,
                             supports_attach_interface=True),
+            mock.patch("nova.network.neutron.API.create_resource_requests"),
             mock.patch.object(self.compute,
                               '_claim_pci_device_for_interface_attach',
                               return_value=None),
         ) as (mock_notify, mock_attach, mock_allocate, mock_deallocate,
-              mock_dict, mock_claim_pci):
+              mock_dict, mock_create_resource_req, mock_claim_pci):
 
             mock_allocate.return_value = nwinfo
             mock_attach.side_effect = exception.NovaException("attach_failed")
@@ -10361,7 +10373,11 @@ def test_attach_interface_failed(self):
                           exception=mock_attach.side_effect,
                           phase='error')])
             mock_claim_pci.assert_called_once_with(
-                self.context, instance, network_requests)
+                self.context, instance,
+                test.MatchType(objects.InstancePCIRequests))
+            pci_reqs = mock_claim_pci.mock_calls[0][1][2]
+            self.assertEqual(instance.uuid, pci_reqs.instance_uuid)
+            self.assertEqual([], pci_reqs.requests)
 
     def test_attach_sriov_interface_failed_in_driver(self):
         new_type = flavors.get_flavor_by_flavor_id('4')

nova/tests/unit/compute/test_compute_mgr.py

@@ -2543,8 +2543,11 @@ def test_attach_interface_failure(self):
         f_instance = objects.Instance._from_db_object(self.context,
                                                       objects.Instance(),
                                                       db_instance)
+        f_instance.flavor = objects.Flavor()
+        f_instance.system_metadata = {}
         e = exception.InterfaceAttachFailed(instance_uuid=f_instance.uuid)
 
+        @mock.patch('nova.network.neutron.API.create_resource_requests')
         @mock.patch.object(self.compute,
                            '_claim_pci_device_for_interface_attach',
                            return_value=None)
@@ -2555,7 +2558,9 @@ def test_attach_interface_failure(self):
                            'allocate_for_instance', side_effect=e)
         @mock.patch.object(self.compute, '_instance_update',
                            side_effect=lambda *a, **k: {})
-        def do_test(update, meth, add_fault, notify, event, mock_claim_pci):
+        def do_test(
+                update, meth, add_fault, notify, event, mock_claim_pci,
+                mock_create_resource_req):
             self.assertRaises(exception.InterfaceAttachFailed,
                               self.compute.attach_interface,
                               self.context, f_instance, uuids.network_id,