Generate request_id for Flavor based InstancePCIRequest

The InstancePCIRequest.request_id is used to correlate allocated
PciDevice objects with the InstancePCIRequest object triggered the PCI
allocation. For neutron port based PCI requests the
IstancePCIRequest.request_id was already set to a generated UUID by
nova. But for Flavor based request the request_id was kept None. The
placement PCI scheduling code depends on the request_id to be a unique
identifier of the request. So this patch starts filling the request_id
for flavor based requests as well.

This change showed than in some places nova still uses the request_id ==
None condition to distinguish between flavor based and neutron based
requests. This logic is now adapted to use the newer and better
InstancePCIRequest.source based approach. Also we took the opportunity
to move the logic of querying PCI devices allocated to an instance to the
Instance ovo.

This change fills the request_id for newly created flavor based
InstancePCIRequest ovos. But the change in logic to use the
InstancePCIRequest.source property instead of the request_id == None
condition works even if the request_id is None for already existing
InstancePCIRequest objects. So this patch does not include a data
migration logic to fill request_id for existing objects.

blueprint: pci-device-tracking-in-placement
Change-Id: I53e03ff7a0221db682b043fb6d5adba3f5c9fdbe

Author: gibizer

nova/network/neutron.py

@@ -43,7 +43,6 @@
 from nova.network import model as network_model
 from nova import objects
 from nova.objects import fields as obj_fields
-from nova.pci import manager as pci_manager
 from nova.pci import request as pci_request
 from nova.pci import utils as pci_utils
 from nova.pci import whitelist as pci_whitelist
@@ -1631,8 +1630,7 @@ def _populate_neutron_binding_profile(self, instance, pci_request_id,
             pci_request_id cannot be found on the instance.
         """
         if pci_request_id:
-            pci_devices = pci_manager.get_instance_pci_devs(
-                instance, pci_request_id)
+            pci_devices = instance.get_pci_devices(request_id=pci_request_id)
             if not pci_devices:
                 # The pci_request_id likely won't mean much except for tracing
                 # through the logs since it is generated per request.
@@ -1662,8 +1660,7 @@ def _populate_pci_mac_address(instance, pci_request_id, port_req_body):
         Currently this is done only for PF passthrough.
         """
         if pci_request_id is not None:
-            pci_devs = pci_manager.get_instance_pci_devs(
-                instance, pci_request_id)
+            pci_devs = instance.get_pci_devices(request_id=pci_request_id)
             if len(pci_devs) != 1:
                 # NOTE(ndipanov): We shouldn't ever get here since
                 # InstancePCIRequest instances built from network requests

nova/objects/instance.py

@@ -13,6 +13,7 @@
 #    under the License.
 
 import contextlib
+import typing as ty
 
 from oslo_config import cfg
 from oslo_db import exception as db_exc
@@ -1226,6 +1227,46 @@ 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]
 
+    def get_pci_devices(
+        self,
+        source: ty.Optional[int] = None,
+        request_id: ty.Optional[str] = None,
+    ) -> ty.List["objects.PciDevice"]:
+        """Return the PCI devices allocated to the instance
+
+        :param source: Filter by source. It can be
+            InstancePCIRequest.FLAVOR_ALIAS or InstancePCIRequest.NEUTRON_PORT
+            or None. None means returns devices from both type of requests.
+        :param request_id: Filter by PciDevice.request_id. None means do not
+            filter by request_id.
+        :return: a list of matching PciDevice objects
+        """
+        if not self.pci_devices:
+            # return early to avoid an extra lazy load on self.pci_requests
+            # if there are no devices allocated to be filtered
+            return []
+        else:
+            devs = self.pci_devices.objects
+
+        if request_id is not None:
+            devs = [dev for dev in devs if dev.request_id == request_id]
+
+        if source is not None:
+            # NOTE(gibi): this happens to work for the old requests when the
+            # request has request_id None and therefore the device allocated
+            # due to that request has request_id None too, so they will be
+            # mapped via the None key.
+            req_id_to_req = {
+                req.request_id: req for req in self.pci_requests.requests
+            }
+            devs = [
+                dev
+                for dev in devs
+                if (req_id_to_req[dev.request_id].source == source)
+            ]
+
+        return devs
+
 
 def _make_instance_list(context, inst_list, db_inst_list, expected_attrs):
     get_fault = expected_attrs and 'fault' in expected_attrs

nova/pci/manager.py

@@ -480,24 +480,3 @@ def clean_usage(
                 devs = self.allocations.pop(uuid, [])
                 for dev in devs:
                     self._free_device(dev)
-
-
-def get_instance_pci_devs(
-    inst: 'objects.Instance', request_id: str = None,
-) -> ty.List['objects.PciDevice']:
-    """Get the devices allocated to one or all requests for an instance.
-
-    - For generic PCI request, the request id is None.
-    - For sr-iov networking, the request id is a valid uuid
-    - There are a couple of cases where all the PCI devices allocated to an
-      instance need to be returned. Refer to libvirt driver that handles
-      soft_reboot and hard_boot of 'xen' instances.
-    """
-    pci_devices = inst.pci_devices
-    if pci_devices is None:
-        return []
-
-    return [
-        device for device in pci_devices if
-        device.request_id == request_id or request_id == 'all'
-    ]

nova/pci/request.py

@@ -43,6 +43,7 @@
 import jsonschema
 from oslo_log import log as logging
 from oslo_serialization import jsonutils
+from oslo_utils import uuidutils
 
 import nova.conf
 from nova import context as ctx
@@ -183,7 +184,9 @@ def _translate_alias_to_requests(
             count=int(count),
             spec=spec,
             alias_name=name,
-            numa_policy=policy))
+            numa_policy=policy,
+            request_id=uuidutils.generate_uuid(),
+        ))
     return pci_requests
 
 

nova/tests/unit/network/test_neutron.py

@@ -42,7 +42,6 @@
 from nova.objects import fields as obj_fields
 from nova.objects import network_request as net_req_obj
 from nova.objects import virtual_interface as obj_vif
-from nova.pci import manager as pci_manager
 from nova.pci import request as pci_request
 from nova.pci import utils as pci_utils
 from nova.pci import whitelist as pci_whitelist
@@ -7738,11 +7737,11 @@ def test_populate_neutron_extension_values_binding(self, mock_get_client):
             'vf_num': 1,
         }))
     @mock.patch.object(pci_whitelist.Whitelist, 'get_devspec')
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     def test_populate_neutron_extension_values_binding_sriov(
         self, mock_get_instance_pci_devs, mock_get_pci_device_devspec):
         host_id = 'my_host_id'
-        instance = {'host': host_id}
+        instance = objects.Instance(host=host_id)
         port_req_body = {'port': {}}
         pci_req_id = 'my_req_id'
         pci_dev = {'vendor_id': '1377',
@@ -7783,11 +7782,11 @@ def test_populate_neutron_extension_values_binding_sriov(
         })
     )
     @mock.patch.object(pci_whitelist.Whitelist, 'get_devspec')
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     def test_populate_neutron_extension_values_binding_sriov_card_serial(
         self, mock_get_instance_pci_devs, mock_get_pci_device_devspec):
         host_id = 'my_host_id'
-        instance = {'host': host_id}
+        instance = objects.Instance(host=host_id)
         port_req_body = {'port': {}}
         pci_req_id = 'my_req_id'
         pci_dev = {'vendor_id': 'a2d6',
@@ -7867,11 +7866,11 @@ def test_populate_neutron_extension_values_with_arq(self,
         })
     )
     @mock.patch.object(pci_whitelist.Whitelist, 'get_devspec')
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     def test_populate_neutron_extension_values_binding_sriov_with_cap(
         self, mock_get_instance_pci_devs, mock_get_pci_device_devspec):
         host_id = 'my_host_id'
-        instance = {'host': host_id}
+        instance = objects.Instance(host=host_id)
         port_req_body = {'port': {
                              constants.BINDING_PROFILE: {
                                 'capabilities': ['switchdev']}}}
@@ -7907,12 +7906,12 @@ def test_populate_neutron_extension_values_binding_sriov_with_cap(
                              constants.BINDING_PROFILE])
 
     @mock.patch.object(pci_whitelist.Whitelist, 'get_devspec')
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     def test_populate_neutron_extension_values_binding_sriov_pf(
         self, mock_get_instance_pci_devs, mock_get_devspec
     ):
         host_id = 'my_host_id'
-        instance = {'host': host_id}
+        instance = objects.Instance(host=host_id)
         port_req_body = {'port': {}}
 
         pci_dev = objects.PciDevice(
@@ -8041,11 +8040,11 @@ def test__get_pci_device_profile_pf(self, mock_get_pci_device_devspec):
         )
 
     @mock.patch.object(pci_whitelist.Whitelist, 'get_devspec')
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     def test_populate_neutron_extension_values_binding_sriov_fail(
         self, mock_get_instance_pci_devs, mock_get_pci_device_devspec):
         host_id = 'my_host_id'
-        instance = {'host': host_id}
+        instance = objects.Instance(host=host_id)
         port_req_body = {'port': {}}
         pci_req_id = 'my_req_id'
         pci_objs = [objects.PciDevice(vendor_id='1377',
@@ -8062,7 +8061,7 @@ def test_populate_neutron_extension_values_binding_sriov_fail(
             self.api._populate_neutron_binding_profile,
             instance, pci_req_id, port_req_body, None)
 
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs', return_value=[])
+    @mock.patch('nova.objects.Instance.get_pci_devices', return_value=[])
     def test_populate_neutron_binding_profile_pci_dev_not_found(
             self, mock_get_instance_pci_devs):
         api = neutronapi.API()
@@ -8073,7 +8072,7 @@ def test_populate_neutron_binding_profile_pci_dev_not_found(
                           api._populate_neutron_binding_profile,
                           instance, pci_req_id, port_req_body, None)
         mock_get_instance_pci_devs.assert_called_once_with(
-            instance, pci_req_id)
+            request_id=pci_req_id)
 
     @mock.patch.object(
         pci_utils, 'is_physical_function',
@@ -8089,7 +8088,7 @@ def test_populate_neutron_binding_profile_pci_dev_not_found(
         new=mock.MagicMock(side_effect=(lambda vf_a: {
             '0000:0a:00.0': '52:54:00:1e:59:c6'}.get(vf_a)))
     )
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     def test_pci_parse_whitelist_called_once(
         self, mock_get_instance_pci_devs
     ):
@@ -8108,7 +8107,7 @@ def test_pci_parse_whitelist_called_once(
         # after the 'device_spec' is set in this test case.
         api = neutronapi.API()
         host_id = 'my_host_id'
-        instance = {'host': host_id}
+        instance = objects.Instance(host=host_id)
         pci_req_id = 'my_req_id'
         port_req_body = {'port': {}}
         pci_dev = {'vendor_id': '1377',
@@ -8144,7 +8143,7 @@ def _populate_pci_mac_address_fakes(self):
         vf.update_device(pci_dev)
         return instance, pf, vf
 
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     @mock.patch.object(pci_utils, 'get_mac_by_pci_address')
     def test_populate_pci_mac_address_pf(self, mock_get_mac_by_pci_address,
                                          mock_get_instance_pci_devs):
@@ -8158,7 +8157,7 @@ def test_populate_pci_mac_address_pf(self, mock_get_mac_by_pci_address,
         self.api._populate_pci_mac_address(instance, 0, req)
         self.assertEqual(expected_port_req_body, req)
 
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     @mock.patch.object(pci_utils, 'get_mac_by_pci_address')
     def test_populate_pci_mac_address_vf(self, mock_get_mac_by_pci_address,
                                          mock_get_instance_pci_devs):
@@ -8170,7 +8169,7 @@ def test_populate_pci_mac_address_vf(self, mock_get_mac_by_pci_address,
         self.api._populate_pci_mac_address(instance, 42, port_req_body)
         self.assertEqual(port_req_body, req)
 
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     @mock.patch.object(pci_utils, 'get_mac_by_pci_address')
     def test_populate_pci_mac_address_vf_fail(self,
                                               mock_get_mac_by_pci_address,
@@ -8185,7 +8184,7 @@ def test_populate_pci_mac_address_vf_fail(self,
         self.api._populate_pci_mac_address(instance, 42, port_req_body)
         self.assertEqual(port_req_body, req)
 
-    @mock.patch.object(pci_manager, 'get_instance_pci_devs')
+    @mock.patch('nova.objects.Instance.get_pci_devices')
     @mock.patch('nova.network.neutron.LOG.error')
     def test_populate_pci_mac_address_no_device(self, mock_log_error,
                                                 mock_get_instance_pci_devs):