Merge "Update pci stat pools based on PCI device changes" into stable/victoria

Author: Zuul

nova/pci/manager.py

@@ -225,6 +225,7 @@ def _set_hvdevs(self, devices):
                     self.stale[new_value['address']] = new_value
                 else:
                     existed.update_device(new_value)
+                    self.stats.update_device(existed)
 
         # Track newly discovered devices.
         for dev in [dev for dev in devices if

nova/pci/stats.py

@@ -104,6 +104,32 @@ def _create_pool_keys_from_dev(self, dev):
             pool['parent_ifname'] = dev.extra_info['parent_ifname']
         return pool
 
+    def _get_pool_with_device_type_mismatch(self, dev):
+        """Check for device type mismatch in the pools for a given device.
+
+        Return (pool, device) if device type does not match or a single None
+        if the device type matches.
+        """
+        for pool in self.pools:
+            for device in pool['devices']:
+                if device.address == dev.address:
+                    if dev.dev_type != pool["dev_type"]:
+                        return pool, device
+                    return None
+
+        return None
+
+    def update_device(self, dev):
+        """Update a device to its matching pool."""
+        pool_device_info = self._get_pool_with_device_type_mismatch(dev)
+        if pool_device_info is None:
+            return
+
+        pool, device = pool_device_info
+        pool['devices'].remove(device)
+        self._decrease_pool_count(self.pools, pool)
+        self.add_device(dev)
+
     def add_device(self, dev):
         """Add a device to its matching pool."""
         dev_pool = self._create_pool_keys_from_dev(dev)

nova/tests/functional/libvirt/test_pci_sriov_servers.py

@@ -70,6 +70,7 @@ class SRIOVServersTest(_PCIServersTestBase):
         {
             'vendor_id': fakelibvirt.PCI_VEND_ID,
             'product_id': fakelibvirt.PF_PROD_ID,
+            'physical_network': 'physnet4',
         },
         {
             'vendor_id': fakelibvirt.PCI_VEND_ID,
@@ -103,6 +104,20 @@ def setUp(self):
         # fixture already stubbed.
         self.neutron = self.useFixture(base.LibvirtNeutronFixture(self))
 
+    def _disable_sriov_in_pf(self, pci_info):
+        # Check for PF and change the capability from virt_functions
+        # Delete all the VFs
+        vfs_to_delete = []
+
+        for device_name, device in pci_info.devices.items():
+            if 'virt_functions' in device.pci_device:
+                device.generate_xml(skip_capability=True)
+            elif 'phys_function' in device.pci_device:
+                vfs_to_delete.append(device_name)
+
+        for device in vfs_to_delete:
+            del pci_info.devices[device]
+
     def test_create_server_with_VF(self):
         """Create a server with an SR-IOV VF-type PCI device."""
 
@@ -266,6 +281,69 @@ def test_get_server_diagnostics_server_with_VF(self):
         )
         self.assertIsNone(diagnostics['nic_details'][1]['tx_packets'])
 
+    def test_create_server_after_change_in_nonsriov_pf_to_sriov_pf(self):
+        # Starts a compute with PF not configured with SRIOV capabilities
+        # Updates the PF with SRIOV capability and restart the compute service
+        # Then starts a VM with the sriov port. The VM should be in active
+        # state with sriov port attached.
+
+        # To emulate the device type changing, we first create a
+        # HostPCIDevicesInfo object with PFs and VFs. Then we make a copy
+        # and remove the VFs and the virt_function capability. This is
+        # done to ensure the physical function product id is same in both
+        # the versions.
+        pci_info = fakelibvirt.HostPCIDevicesInfo(num_pfs=1, num_vfs=1)
+        pci_info_no_sriov = copy.deepcopy(pci_info)
+
+        # Disable SRIOV capabilties in PF and delete the VFs
+        self._disable_sriov_in_pf(pci_info_no_sriov)
+
+        fake_connection = self._get_connection(pci_info=pci_info_no_sriov,
+                                               hostname='test_compute0')
+        self.mock_conn.return_value = fake_connection
+
+        self.compute = self.start_service('compute', host='test_compute0')
+
+        ctxt = context.get_admin_context()
+        pci_devices = objects.PciDeviceList.get_by_compute_node(
+            ctxt,
+            objects.ComputeNode.get_by_nodename(
+                ctxt, 'test_compute0',
+            ).id,
+        )
+        self.assertEqual(1, len(pci_devices))
+        self.assertEqual('type-PCI', pci_devices[0].dev_type)
+
+        # Update connection with original pci info with sriov PFs
+        fake_connection = self._get_connection(pci_info=pci_info,
+                                               hostname='test_compute0')
+        self.mock_conn.return_value = fake_connection
+
+        # Restart the compute service
+        self.restart_compute_service(self.compute)
+
+        # Verify if PCI devices are of type type-PF or type-VF
+        pci_devices = objects.PciDeviceList.get_by_compute_node(
+            ctxt,
+            objects.ComputeNode.get_by_nodename(
+                ctxt, 'test_compute0',
+            ).id,
+        )
+        for pci_device in pci_devices:
+            self.assertIn(pci_device.dev_type, ['type-PF', 'type-VF'])
+
+        # create the port
+        self.neutron.create_port({'port': self.neutron.network_4_port_1})
+
+        # create a server using the VF via neutron
+        flavor_id = self._create_flavor()
+        self._create_server(
+            flavor_id=flavor_id,
+            networks=[
+                {'port': base.LibvirtNeutronFixture.network_4_port_1['id']},
+            ],
+        )
+
 
 class SRIOVAttachDetachTest(_PCIServersTestBase):
     # no need for aliases as these test will request SRIOV via neutron

nova/tests/unit/pci/test_stats.py

@@ -561,6 +561,30 @@ def test_remove_device(self):
         self.pci_stats.remove_device(dev2)
         self._assertPools()
 
+    def test_update_device(self):
+        # Update device type of one of the device from type-PCI to
+        # type-PF. Verify if the existing pool is updated and a new
+        # pool is created with dev_type type-PF.
+        self._create_pci_devices()
+        dev1 = self.pci_tagged_devices.pop()
+        dev1.dev_type = 'type-PF'
+        self.pci_stats.update_device(dev1)
+        self.assertEqual(3, len(self.pci_stats.pools))
+        self._assertPoolContent(self.pci_stats.pools[0], '1137', '0072',
+                                len(self.pci_untagged_devices))
+        self.assertEqual(self.pci_untagged_devices,
+                         self.pci_stats.pools[0]['devices'])
+        self._assertPoolContent(self.pci_stats.pools[1], '1137', '0071',
+                                len(self.pci_tagged_devices),
+                                physical_network='physnet1')
+        self.assertEqual(self.pci_tagged_devices,
+                         self.pci_stats.pools[1]['devices'])
+        self._assertPoolContent(self.pci_stats.pools[2], '1137', '0071',
+                                1,
+                                physical_network='physnet1')
+        self.assertEqual(dev1,
+                         self.pci_stats.pools[2]['devices'][0])
+
 
 class PciDeviceVFPFStatsTestCase(test.NoDBTestCase):
 

nova/tests/unit/virt/libvirt/fakelibvirt.py

@@ -284,49 +284,62 @@ def __init__(self, dev_type, slot, function, iommu_group, numa_node,
         :param multiple_gpu_types: (bool) Supports different vGPU types
         """
 
+        self.dev_type = dev_type
+        self.slot = slot
+        self.function = function
+        self.iommu_group = iommu_group
+        self.numa_node = numa_node
+        self.vf_ratio = vf_ratio
+        self.multiple_gpu_types = multiple_gpu_types
+        self.parent = parent
+        self.generate_xml()
+
+    def generate_xml(self, skip_capability=False):
         vend_id = PCI_VEND_ID
         vend_name = PCI_VEND_NAME
-        if dev_type == 'PCI':
-            if vf_ratio:
+        capability = ''
+        if self.dev_type == 'PCI':
+            if self.vf_ratio:
                 raise ValueError('vf_ratio does not apply for PCI devices')
 
             prod_id = PCI_PROD_ID
             prod_name = PCI_PROD_NAME
             driver = PCI_DRIVER_NAME
-            capability = ''
-        elif dev_type == 'PF':
+        elif self.dev_type == 'PF':
             prod_id = PF_PROD_ID
             prod_name = PF_PROD_NAME
             driver = PF_DRIVER_NAME
-            capability = self.cap_templ % {
-                'cap_type': PF_CAP_TYPE,
-                'addresses': '\n'.join([
-                    self.addr_templ % {
-                        # these are the slot, function values of the child VFs
-                        # we can only assign 8 functions to a slot (0-7) so
-                        # bump the slot each time we exceed this
-                        'slot': slot + (x // 8),
-                        # ...and wrap the function value
-                        'function': x % 8,
-                    # the offset is because the PF is occupying function 0
-                    } for x in range(1, vf_ratio + 1)])
-            }
-        elif dev_type == 'VF':
+            if not skip_capability:
+                capability = self.cap_templ % {
+                    'cap_type': PF_CAP_TYPE,
+                    'addresses': '\n'.join([
+                        self.addr_templ % {
+                            # these are the slot, function values of the child
+                            # VFs, we can only assign 8 functions to a slot
+                            # (0-7) so bump the slot each time we exceed this
+                            'slot': self.slot + (x // 8),
+                            # ...and wrap the function value
+                            'function': x % 8,
+                        # the offset is because the PF is occupying function 0
+                        } for x in range(1, self.vf_ratio + 1)])
+                }
+        elif self.dev_type == 'VF':
             prod_id = VF_PROD_ID
             prod_name = VF_PROD_NAME
             driver = VF_DRIVER_NAME
-            capability = self.cap_templ % {
-                'cap_type': VF_CAP_TYPE,
-                'addresses': self.addr_templ % {
-                    # this is the slot, function value of the parent PF
-                    # if we're e.g. device 8, we'll have a different slot
-                    # to our parent so reverse this
-                    'slot': slot - ((vf_ratio + 1) // 8),
-                    # the parent PF is always function 0
-                    'function': 0,
+            if not skip_capability:
+                capability = self.cap_templ % {
+                    'cap_type': VF_CAP_TYPE,
+                    'addresses': self.addr_templ % {
+                        # this is the slot, function value of the parent PF
+                        # if we're e.g. device 8, we'll have a different slot
+                        # to our parent so reverse this
+                        'slot': self.slot - ((self.vf_ratio + 1) // 8),
+                        # the parent PF is always function 0
+                        'function': 0,
+                    }
                 }
-            }
-        elif dev_type == 'MDEV_TYPES':
+        elif self.dev_type == 'MDEV_TYPES':
             prod_id = MDEV_CAPABLE_PROD_ID
             prod_name = MDEV_CAPABLE_PROD_NAME
             driver = MDEV_CAPABLE_DRIVER_NAME
@@ -336,36 +349,37 @@ def __init__(self, dev_type, slot, function, iommu_group, numa_node,
                 'type_id': NVIDIA_11_VGPU_TYPE,
                 'instances': 16,
             }]
-            if multiple_gpu_types:
+            if self.multiple_gpu_types:
                 types.append(self.mdevtypes_templ % {
                     'type_id': NVIDIA_12_VGPU_TYPE,
                     'instances': 8,
                 })
-            capability = self.cap_templ % {
-                'cap_type': MDEV_CAPABLE_CAP_TYPE,
-                'addresses': '\n'.join(types)
-            }
+            if not skip_capability:
+                capability = self.cap_templ % {
+                    'cap_type': MDEV_CAPABLE_CAP_TYPE,
+                    'addresses': '\n'.join(types)
+                }
             self.is_capable_of_mdevs = True
         else:
             raise ValueError('Expected one of: PCI, VF, PCI')
 
         self.pci_device = self.pci_device_template % {
-            'slot': slot,
-            'function': function,
+            'slot': self.slot,
+            'function': self.function,
             'vend_id': vend_id,
             'vend_name': vend_name,
             'prod_id': prod_id,
             'prod_name': prod_name,
             'driver': driver,
             'capability': capability,
-            'iommu_group': iommu_group,
-            'numa_node': numa_node,
-            'parent': parent or self.pci_default_parent
+            'iommu_group': self.iommu_group,
+            'numa_node': self.numa_node,
+            'parent': self.parent or self.pci_default_parent
         }
         # -1 is the sentinel set in /sys/bus/pci/devices/*/numa_node
         # for no NUMA affinity. When the numa_node is set to -1 on a device
         # Libvirt omits the NUMA element so we remove it.
-        if numa_node == -1:
+        if self.numa_node == -1:
             self.pci_device = self.pci_device.replace("<numa node='-1'/>", "")
 
     def XMLDesc(self, flags):
@@ -943,6 +957,20 @@ def _parse_definition(self, xml):
                         nic_info['source'] = source.get('network')
                     elif nic_info['type'] == 'bridge':
                         nic_info['source'] = source.get('bridge')
+                    elif nic_info['type'] == 'hostdev':
+                        # <interface type='hostdev'> is for VF when vnic_type
+                        # is direct. Add sriov vf pci information in nic_info
+                        address = source.find('./address')
+                        pci_type = address.get('type')
+                        pci_domain = address.get('domain').replace('0x', '')
+                        pci_bus = address.get('bus').replace('0x', '')
+                        pci_slot = address.get('slot').replace('0x', '')
+                        pci_function = address.get('function').replace(
+                            '0x', '')
+                        pci_device = "%s_%s_%s_%s_%s" % (pci_type, pci_domain,
+                                                         pci_bus, pci_slot,
+                                                         pci_function)
+                        nic_info['source'] = pci_device
 
                 nics_info += [nic_info]
 
@@ -984,11 +1012,32 @@ def _parse_definition(self, xml):
 
         return definition
 
+    def verify_hostdevs_interface_are_vfs(self):
+        """Verify for interface type hostdev if the pci device is VF or not.
+        """
+
+        error_message = ("Interface type hostdev is currently supported on "
+                         "SR-IOV Virtual Functions only")
+
+        nics = self._def['devices'].get('nics', [])
+        for nic in nics:
+            if nic['type'] == 'hostdev':
+                pci_device = nic['source']
+                pci_info_from_connection = self._connection.pci_info.devices[
+                    pci_device]
+                if 'phys_function' not in pci_info_from_connection.pci_device:
+                    raise make_libvirtError(
+                        libvirtError,
+                        error_message,
+                        error_code=VIR_ERR_CONFIG_UNSUPPORTED,
+                        error_domain=VIR_FROM_DOMAIN)
+
     def create(self):
         self.createWithFlags(0)
 
     def createWithFlags(self, flags):
         # FIXME: Not handling flags at the moment
+        self.verify_hostdevs_interface_are_vfs()
         self._state = VIR_DOMAIN_RUNNING
         self._connection._mark_running(self)
         self._has_saved_state = False
@@ -1112,7 +1161,7 @@ def XMLDesc(self, flags):
 
         nics = ''
         for nic in self._def['devices']['nics']:
-            if 'source' in nic:
+            if 'source' in nic and nic['type'] != 'hostdev':
                 nics += '''<interface type='%(type)s'>
           <mac address='%(mac)s'/>
           <source %(type)s='%(source)s'/>

releasenotes/notes/bug-1892361-pci-deivce-type-update-c407a66fd37f6405.yaml

@@ -0,0 +1,12 @@
+---
+fixes:
+  - |
+    Fixes `bug 1892361`_ in which the pci stat pools are not updated when an
+    existing device is enabled with SRIOV capability. Restart of nova-compute
+    service updates the pci device type from type-PCI to type-PF but the pools
+    still maintain the device type as type-PCI. And so the PF is considered for
+    allocation to instance that requests vnic_type=direct. With this fix, the
+    pci device type updates are detected and the pci stat pools are updated
+    properly.
+
+    .. _bug 1892361: https://bugs.launchpad.net/nova/+bug/1892361