Record SRIOV PF MAC in the binding profile

Today Nova updates the mac_address of a direct-physical port to reflect
the MAC address of the physical device the port is bound to. But this
can only be done before the port is bound. However during migration Nova
does not update the MAC when the port is bound to a different physical
device on the destination host.

This patch extends the libvirt virt driver to provide the MAC address of
the PF in the pci_info returned to the resource tracker. This
information will be then persisted in the extra_info field of the
PciDevice object.

Then the port update logic during migration, resize, live
migration, evacuation and unshelve is also extended to record the MAC of
physical device in the port binding profile according to the device on
the destination host.

The related neutron change Ib0638f5db69cb92daf6932890cb89e83cf84f295
uses this info from the binding profile to update the mac_address field
of the port when the binding is activated.

Closes-Bug: #1942329

Conflicts:
    nova/network/neutron.py
    nova/objects/pci_device.py
    nova/tests/fixtures/libvirt.py
    nova/tests/unit/network/test_neutron.py
    nova/tests/unit/virt/libvirt/test_host.py
    nova/virt/fake.py
    nova/virt/libvirt/host.py

Most of the conlficts are due to the lack of the vpd feature
and I83a128a260acdd8bf78fede566af6881b8b82a9c

the addtional unit tests in test_neutron and test_compute_mgr
are form the vpd feature but have been modified to drop assertions
related to the vpd feature while keeping the assertions related
to SRIOV PF MAC adress binding.

Change-Id: Iad5e70b43a65c076134e1874cb8e75d1ba214fde
(cherry picked from commit cd03bbc)
(cherry picked from commit 8133770)

Author: Balazs Gibizer

nova/compute/manager.py

@@ -10820,6 +10820,8 @@ def _update_migrate_vifs_profile_with_pci(self,
             profile['pci_slot'] = pci_dev.address
             profile['pci_vendor_info'] = ':'.join([pci_dev.vendor_id,
                                                    pci_dev.product_id])
+            if pci_dev.mac_address:
+                profile['device_mac_address'] = pci_dev.mac_address
             mig_vif.profile = profile
             LOG.debug("Updating migrate VIF profile for port %(port_id)s:"
                       "%(profile)s", {'port_id': port_id,

nova/compute/resource_tracker.py

@@ -328,7 +328,12 @@ def _move_claim(
             migration_id=migration.id,
             old_numa_topology=instance.numa_topology,
             new_numa_topology=claim.claimed_numa_topology,
-            old_pci_devices=instance.pci_devices,
+            # NOTE(gibi): the _update_usage_from_migration call below appends
+            # the newly claimed pci devices to the instance.pci_devices list
+            # to keep the migration context independent we need to make a copy
+            # that list here. We need a deep copy as we need to duplicate
+            # the instance.pci_devices.objects list
+            old_pci_devices=copy.deepcopy(instance.pci_devices),
             new_pci_devices=claimed_pci_devices,
             old_pci_requests=instance.pci_requests,
             new_pci_requests=new_pci_requests,

nova/network/neutron.py

@@ -667,7 +667,8 @@ def _unbind_ports(self, context, ports,
             # for the physical device but don't want to overwrite the other
             # information in the binding profile.
             for profile_key in ('pci_vendor_info', 'pci_slot',
-                                constants.ALLOCATION, 'arq_uuid'):
+                                constants.ALLOCATION, 'arq_uuid',
+                                'device_mac_address'):
                 if profile_key in port_profile:
                     del port_profile[profile_key]
             port_req_body['port'][constants.BINDING_PROFILE] = port_profile
@@ -1286,6 +1287,10 @@ def _update_ports_for_instance(self, context, instance, neutron,
                     network=network, neutron=neutron,
                     bind_host_id=bind_host_id,
                     port_arq=port_arq)
+                # NOTE(gibi): Remove this once we are sure that the fix for
+                # bug 1942329 is always present in the deployed neutron. The
+                # _populate_neutron_extension_values() call above already
+                # populated this MAC to the binding profile instead.
                 self._populate_pci_mac_address(instance,
                     request.pci_request_id, port_req_body)
 
@@ -1506,11 +1511,27 @@ def delete_port_binding(self, context, port_id, host):
     def _get_pci_device_profile(self, pci_dev):
         dev_spec = self.pci_whitelist.get_devspec(pci_dev)
         if dev_spec:
-            return {'pci_vendor_info': "%s:%s" %
-                        (pci_dev.vendor_id, pci_dev.product_id),
-                    'pci_slot': pci_dev.address,
-                    'physical_network':
-                        dev_spec.get_tags().get('physical_network')}
+            dev_profile = {
+                'pci_vendor_info': "%s:%s"
+                % (pci_dev.vendor_id, pci_dev.product_id),
+                'pci_slot': pci_dev.address,
+                'physical_network': dev_spec.get_tags().get(
+                    'physical_network'
+                ),
+            }
+            if pci_dev.dev_type == obj_fields.PciDeviceType.SRIOV_PF:
+                # In general the MAC address information flows fom the neutron
+                # port to the device in the backend. Except for direct-physical
+                # ports. In that case the MAC address flows from the physical
+                # device, the PF, to the neutron port. So when such a port is
+                # being bound to a host the port's MAC address needs to be
+                # updated. Nova needs to put the new MAC into the binding
+                # profile.
+                if pci_dev.mac_address:
+                    dev_profile['device_mac_address'] = pci_dev.mac_address
+
+            return dev_profile
+
         raise exception.PciDeviceNotFound(node_id=pci_dev.compute_node_id,
                                           address=pci_dev.address)
 
@@ -3512,11 +3533,10 @@ def _get_pci_mapping_for_migration(self, instance, migration):
                   migration.get('status') == 'reverted')
         return instance.migration_context.get_pci_mapping_for_migration(revert)
 
-    def _get_port_pci_dev(self, context, instance, port):
+    def _get_port_pci_dev(self, instance, port):
         """Find the PCI device corresponding to the port.
         Assumes the port is an SRIOV one.
 
-        :param context: The request context.
         :param instance: The instance to which the port is attached.
         :param port: The Neutron port, as obtained from the Neutron API
             JSON form.
@@ -3604,15 +3624,14 @@ def _update_port_binding_for_instance(
                             raise exception.PortUpdateFailed(port_id=p['id'],
                                 reason=_("Unable to correlate PCI slot %s") %
                                          pci_slot)
-                # NOTE(artom) If migration is None, this is an unshevle, and we
-                # need to figure out the pci_slot from the InstancePCIRequest
-                # and PciDevice objects.
+                # NOTE(artom) If migration is None, this is an unshelve, and we
+                # need to figure out the pci related binding information from
+                # the InstancePCIRequest and PciDevice objects.
                 else:
-                    pci_dev = self._get_port_pci_dev(context, instance, p)
+                    pci_dev = self._get_port_pci_dev(instance, p)
                     if pci_dev:
                         binding_profile.update(
-                            self._get_pci_device_profile(pci_dev)
-                        )
+                            self._get_pci_device_profile(pci_dev))
                         updates[constants.BINDING_PROFILE] = binding_profile
 
             # NOTE(gibi): during live migration the conductor already sets the

nova/objects/pci_device.py

@@ -148,6 +148,12 @@ def obj_make_compatible(self, primitive, target_version):
                     reason='dev_type=%s not supported in version %s' % (
                         dev_type, target_version))
 
+    def __repr__(self):
+        return (
+            f'PciDevice(address={self.address}, '
+            f'compute_node_id={self.compute_node_id})'
+        )
+
     def update_device(self, dev_dict):
         """Sync the content from device dictionary to device object.
 
@@ -175,6 +181,9 @@ def update_device(self, dev_dict):
                 # NOTE(ralonsoh): list of parameters currently added to
                 # "extra_info" dict:
                 #     - "capabilities": dict of (strings/list of strings)
+                #     - "parent_ifname": the netdev name of the parent (PF)
+                #        device of a VF
+                #     - "mac_address": the MAC address of the PF
                 extra_info = self.extra_info
                 data = v if isinstance(v, str) else jsonutils.dumps(v)
                 extra_info.update({k: data})
@@ -511,6 +520,13 @@ def free(self, instance=None):
     def is_available(self):
         return self.status == fields.PciDeviceStatus.AVAILABLE
 
+    @property
+    def mac_address(self):
+        """The MAC address of the PF physical device or None if the device is
+        not a PF or if the MAC is not available.
+        """
+        return self.extra_info.get('mac_address')
+
 
 @base.NovaObjectRegistry.register
 class PciDeviceList(base.ObjectListBase, base.NovaObject):
@@ -550,3 +566,6 @@ def get_by_parent_address(cls, context, node_id, parent_addr):
                                                            parent_addr)
         return base.obj_make_list(context, cls(context), objects.PciDevice,
                                   db_dev_list)
+
+    def __repr__(self):
+        return f"PciDeviceList(objects={[repr(obj) for obj in self.objects]})"

nova/tests/fixtures/libvirt.py

@@ -299,7 +299,7 @@ def __init__(
         self, dev_type, bus, slot, function, iommu_group, numa_node, *,
         vf_ratio=None, multiple_gpu_types=False, generic_types=False,
         parent=None, vend_id=None, vend_name=None, prod_id=None,
-        prod_name=None, driver_name=None,
+        prod_name=None, driver_name=None, mac_address=None,
     ):
         """Populate pci devices
 
@@ -321,6 +321,8 @@ def __init__(
         :param prod_id: (str) The product ID.
         :param prod_name: (str) The product name.
         :param driver_name: (str) The driver name.
+        :param mac_address: (str) The MAC of the device.
+            Used in case of SRIOV PFs
         """
 
         self.dev_type = dev_type
@@ -339,6 +341,7 @@ def __init__(
         self.prod_id = prod_id
         self.prod_name = prod_name
         self.driver_name = driver_name
+        self.mac_address = mac_address
 
         self.generate_xml()
 
@@ -352,7 +355,9 @@ def generate_xml(self, skip_capability=False):
             assert not self.vf_ratio, 'vf_ratio does not apply for PCI devices'
 
         if self.dev_type in ('PF', 'VF'):
-            assert self.vf_ratio, 'require vf_ratio for PFs and VFs'
+            assert (
+                self.vf_ratio is not None
+            ), 'require vf_ratio for PFs and VFs'
 
         if self.dev_type == 'VF':
             assert self.parent, 'require parent for VFs'
@@ -460,6 +465,10 @@ def generate_xml(self, skip_capability=False):
     def XMLDesc(self, flags):
         return self.pci_device
 
+    @property
+    def address(self):
+        return "0000:%02x:%02x.%1x" % (self.bus, self.slot, self.function)
+
 
 # TODO(stephenfin): Remove all of these HostFooDevicesInfo objects in favour of
 # a unified devices object
@@ -572,7 +581,7 @@ def add_device(
         self, dev_type, bus, slot, function, iommu_group, numa_node,
         vf_ratio=None, multiple_gpu_types=False, generic_types=False,
         parent=None, vend_id=None, vend_name=None, prod_id=None,
-        prod_name=None, driver_name=None,
+        prod_name=None, driver_name=None, mac_address=None,
     ):
         pci_dev_name = _get_libvirt_nodedev_name(bus, slot, function)
 
@@ -593,7 +602,9 @@ def add_device(
             vend_name=vend_name,
             prod_id=prod_id,
             prod_name=prod_name,
-            driver_name=driver_name)
+            driver_name=driver_name,
+            mac_address=mac_address,
+        )
         self.devices[pci_dev_name] = dev
         return dev
 
@@ -612,6 +623,13 @@ def get_all_mdev_capable_devices(self):
         return [dev for dev in self.devices
                 if self.devices[dev].is_capable_of_mdevs]
 
+    def get_pci_address_mac_mapping(self):
+        return {
+            device.address: device.mac_address
+            for dev_addr, device in self.devices.items()
+            if device.mac_address
+        }
+
 
 class FakeMdevDevice(object):
     template = """
@@ -2141,6 +2159,15 @@ class LibvirtFixture(fixtures.Fixture):
     """
     def __init__(self, stub_os_vif=True):
         self.stub_os_vif = stub_os_vif
+        self.pci_address_to_mac_map = collections.defaultdict(
+            lambda: '52:54:00:1e:59:c6')
+
+    def update_sriov_mac_address_mapping(self, pci_address_to_mac_map):
+        self.pci_address_to_mac_map.update(pci_address_to_mac_map)
+
+    def fake_get_mac_by_pci_address(self, pci_addr, pf_interface=False):
+        res = self.pci_address_to_mac_map[pci_addr]
+        return res
 
     def setUp(self):
         super().setUp()
@@ -2162,6 +2189,10 @@ def setUp(self):
             'nova.pci.utils.get_ifname_by_pci_address',
             return_value='fake_pf_interface_name'))
 
+        self.useFixture(fixtures.MockPatch(
+            'nova.pci.utils.get_mac_by_pci_address',
+            new=self.fake_get_mac_by_pci_address))
+
         # libvirt calls out to sysfs to get the vfs ID during macvtap plug
         self.useFixture(fixtures.MockPatch(
             'nova.pci.utils.get_vf_num_by_pci_address', return_value=1))

nova/tests/functional/libvirt/base.py

@@ -42,7 +42,7 @@ def setUp(self):
         super(ServersTestBase, self).setUp()
 
         self.useFixture(nova_fixtures.LibvirtImageBackendFixture())
-        self.useFixture(nova_fixtures.LibvirtFixture())
+        self.libvirt = self.useFixture(nova_fixtures.LibvirtFixture())
         self.useFixture(nova_fixtures.OSBrickFixture())
 
         self.useFixture(fixtures.MockPatch(
@@ -134,6 +134,12 @@ def _start_compute(hostname, host_info):
                 host_info, pci_info, mdev_info, vdpa_info, libvirt_version,
                 qemu_version, hostname,
             )
+            # If the compute is configured with PCI devices then we need to
+            # make sure that the stubs around sysfs has the MAC address
+            # information for the PCI PF devices
+            if pci_info:
+                self.libvirt.update_sriov_mac_address_mapping(
+                    pci_info.get_pci_address_mac_mapping())
             # This is fun. Firstly we need to do a global'ish mock so we can
             # actually start the service.
             with mock.patch('nova.virt.libvirt.host.Host.get_connection',
@@ -361,6 +367,22 @@ class LibvirtNeutronFixture(nova_fixtures.NeutronFixture):
         'binding:vnic_type': 'direct',
     }
 
+    network_4_port_pf = {
+        'id': 'c6f51315-9202-416f-9e2f-eb78b3ac36d9',
+        'network_id': network_4['id'],
+        'status': 'ACTIVE',
+        'mac_address': 'b5:bc:2e:e7:51:01',
+        'fixed_ips': [
+            {
+                'ip_address': '192.168.4.8',
+                'subnet_id': subnet_4['id']
+            }
+        ],
+        'binding:vif_details': {'vlan': 42},
+        'binding:vif_type': 'hostdev_physical',
+        'binding:vnic_type': 'direct-physical',
+    }
+
     def __init__(self, test):
         super(LibvirtNeutronFixture, self).__init__(test)
         self._networks = {