From 71aa17a487136be5e938192857721d9119222811 Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Thu, 21 Jul 2022 18:21:51 +0200 Subject: [PATCH 1/2] Reproducer for bug 1951656 Due to a new mdev naming, we can't parse it. Change-Id: I0f785178b132dfef668829558dea9f7e674abadb Related-Bug: #1951656 (cherry picked from commit 185201974775bab966f4e5ca3bbdc31b8269fa4c) (cherry picked from commit 857df72d3166a8f7e8a8cdfeabb62ad6ead46565) --- .../regressions/test_bug_1951656.py | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 nova/tests/functional/regressions/test_bug_1951656.py diff --git a/nova/tests/functional/regressions/test_bug_1951656.py b/nova/tests/functional/regressions/test_bug_1951656.py new file mode 100644 index 00000000000..9aad191072c --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_1951656.py @@ -0,0 +1,83 @@ +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslo_utils import uuidutils + + +from nova.tests.fixtures import libvirt as fakelibvirt +from nova.tests.functional.libvirt import test_vgpu +from nova.virt.libvirt import utils as libvirt_utils + + +class VGPUTestsLibvirt7_7(test_vgpu.VGPUTestBase): + + def _create_mdev(self, physical_device, mdev_type, uuid=None): + # We need to fake the newly created sysfs object by adding a new + # FakeMdevDevice in the existing persisted Connection object so + # when asking to get the existing mdevs, we would see it. + if not uuid: + uuid = uuidutils.generate_uuid() + mdev_name = libvirt_utils.mdev_uuid2name(uuid) + libvirt_parent = self.pci2libvirt_address(physical_device) + + # Libvirt 7.7 now creates mdevs with a parent_addr suffix. + new_mdev_name = '_'.join([mdev_name, libvirt_parent]) + + # Here, we get the right compute thanks by the self.current_host that + # was modified just before + connection = self.computes[ + self._current_host].driver._host.get_connection() + connection.mdev_info.devices.update( + {mdev_name: fakelibvirt.FakeMdevDevice(dev_name=new_mdev_name, + type_id=mdev_type, + parent=libvirt_parent)}) + return uuid + + def setUp(self): + super(VGPUTestsLibvirt7_7, self).setUp() + extra_spec = {"resources:VGPU": "1"} + self.flavor = self._create_flavor(extra_spec=extra_spec) + + # Start compute1 supporting only nvidia-11 + self.flags( + enabled_mdev_types=fakelibvirt.NVIDIA_11_VGPU_TYPE, + group='devices') + + self.compute1 = self.start_compute_with_vgpu('host1') + + def test_create_servers_with_vgpu(self): + + # Create a single instance against a specific compute node. + self._create_server( + image_uuid='155d900f-4e14-4e4c-a73d-069cbf4541e6', + flavor_id=self.flavor, host=self.compute1.host, + networks='auto', expected_state='ACTIVE') + + # TODO(sbauza): Modify this once bug #1851656 is fixed. + # mdev_name2uuid() raises a badly formed hexadecimal UUID string error + self.assertRaises(ValueError, + self.assert_mdev_usage, + self.compute1, expected_amount=1) + + # Now, the problem is that we can't create new instances with VGPUs + # from this host. + server = self._create_server( + image_uuid='155d900f-4e14-4e4c-a73d-069cbf4541e6', + flavor_id=self.flavor, host=self.compute1.host, + networks='auto', expected_state='ERROR') + # The error is due to a bad mdev name parsing + self.assertIn('fault', server) + # since we only have one host, we have a RescheduledException as this + # service was creating an exception and we can't use another one. + self.assertIn('Exceeded maximum number of retries', + server['fault']['message']) From 28053917200e3e242148672efda0e1a2b043dc48 Mon Sep 17 00:00:00 2001 From: Billy Olsen Date: Thu, 21 Apr 2022 19:42:27 -0700 Subject: [PATCH 2/2] Handle mdev devices in libvirt 7.7+ Libvirt 7.7 changed the mdev device naming to include the parent PCI device when listing node devices. The domain, however, will still only see the UUID and not see the parent PCI device. Changing the parsing to simply drop the PCI identifier is not enough as the device cannot be found when attempting to lookup the new ID. Modify the Libvirt Driver's _get_mediated_device_information to tolerate different formats of the mdev name. This first uses the legacy behavior by trying to lookup the device name that is passed in (typically mdev_ format) and if that is not found, iterates the list of mdev node devices until the right UUID is found and selects that one. Note that the lookup of the mdev device by UUID are needed in order to keep the ability to recreate assigned mediated devices on a reboot of the compute node. Additionally, the libvirt utils parsing method mdev_name2uuid, has been updated to tolerate both mdev_ and mdev__ formats. Closes-Bug: 1951656 Change-Id: Ifed0fa16053228990a6a8df8d4c666521db7e329 (cherry picked from commit a28b907c4f0dbba6e141a8fbea807e6cb0438977) (cherry picked from commit 98d8c9eaa3c415cc234193e6a9115db887751363) --- .../regressions/test_bug_1951656.py | 22 +++------- nova/tests/unit/virt/libvirt/test_config.py | 26 +++++++++++ nova/virt/libvirt/config.py | 3 ++ nova/virt/libvirt/driver.py | 43 +++++++++++++++++-- nova/virt/libvirt/host.py | 2 +- nova/virt/libvirt/utils.py | 28 +++++++++--- 6 files changed, 97 insertions(+), 27 deletions(-) diff --git a/nova/tests/functional/regressions/test_bug_1951656.py b/nova/tests/functional/regressions/test_bug_1951656.py index 9aad191072c..d705ff6fe31 100644 --- a/nova/tests/functional/regressions/test_bug_1951656.py +++ b/nova/tests/functional/regressions/test_bug_1951656.py @@ -63,21 +63,11 @@ def test_create_servers_with_vgpu(self): flavor_id=self.flavor, host=self.compute1.host, networks='auto', expected_state='ACTIVE') - # TODO(sbauza): Modify this once bug #1851656 is fixed. - # mdev_name2uuid() raises a badly formed hexadecimal UUID string error - self.assertRaises(ValueError, - self.assert_mdev_usage, - self.compute1, expected_amount=1) - - # Now, the problem is that we can't create new instances with VGPUs - # from this host. - server = self._create_server( + self.assert_mdev_usage(self.compute1, expected_amount=1) + + self._create_server( image_uuid='155d900f-4e14-4e4c-a73d-069cbf4541e6', flavor_id=self.flavor, host=self.compute1.host, - networks='auto', expected_state='ERROR') - # The error is due to a bad mdev name parsing - self.assertIn('fault', server) - # since we only have one host, we have a RescheduledException as this - # service was creating an exception and we can't use another one. - self.assertIn('Exceeded maximum number of retries', - server['fault']['message']) + networks='auto', expected_state='ACTIVE') + + self.assert_mdev_usage(self.compute1, expected_amount=2) diff --git a/nova/tests/unit/virt/libvirt/test_config.py b/nova/tests/unit/virt/libvirt/test_config.py index 396edfd0248..c7577745ab4 100644 --- a/nova/tests/unit/virt/libvirt/test_config.py +++ b/nova/tests/unit/virt/libvirt/test_config.py @@ -3135,6 +3135,32 @@ def test_config_mdev_device(self): config.LibvirtConfigNodeDeviceMdevInformation) self.assertEqual("nvidia-11", obj.mdev_information.type) self.assertEqual(12, obj.mdev_information.iommu_group) + self.assertIsNone(obj.mdev_information.uuid) + + def test_config_mdev_device_uuid(self): + xmlin = """ + + mdev_b2107403_110c_45b0_af87_32cc91597b8a_0000_41_00_0 + /sys/devices/pci0000:40/0000:40:03.1/0000:41:00.0/b2107403-110c-45b0-af87-32cc91597b8a + pci_0000_41_00_0 + + vfio_mdev + + + + b2107403-110c-45b0-af87-32cc91597b8a + + + """ + + obj = config.LibvirtConfigNodeDevice() + obj.parse_str(xmlin) + self.assertIsInstance(obj.mdev_information, + config.LibvirtConfigNodeDeviceMdevInformation) + self.assertEqual("nvidia-442", obj.mdev_information.type) + self.assertEqual(57, obj.mdev_information.iommu_group) + self.assertEqual("b2107403-110c-45b0-af87-32cc91597b8a", + obj.mdev_information.uuid) def test_config_vdpa_device(self): xmlin = """ diff --git a/nova/virt/libvirt/config.py b/nova/virt/libvirt/config.py index 1a81be3ade5..47e92e3ca91 100644 --- a/nova/virt/libvirt/config.py +++ b/nova/virt/libvirt/config.py @@ -3299,6 +3299,7 @@ def __init__(self, **kwargs): root_name="capability", **kwargs) self.type = None self.iommu_group = None + self.uuid = None def parse_dom(self, xmldoc): super(LibvirtConfigNodeDeviceMdevInformation, @@ -3308,6 +3309,8 @@ def parse_dom(self, xmldoc): self.type = c.get('id') if c.tag == "iommuGroup": self.iommu_group = int(c.get('number')) + if c.tag == "uuid": + self.uuid = c.text class LibvirtConfigNodeDeviceVpdCap(LibvirtConfigObject): diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index cc5e4b5da52..e792655a7de 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -8019,15 +8019,52 @@ def _get_mdev_capable_devices(self, types=None): def _get_mediated_device_information(self, devname): """Returns a dict of a mediated device.""" - virtdev = self._host.device_lookup_by_name(devname) + # LP #1951656 - In Libvirt 7.7, the mdev name now includes the PCI + # address of the parent device (e.g. mdev__) due to + # the mdevctl allowing for multiple mediated devs having the same UUID + # defined (only one can be active at a time). Since the guest + # information doesn't have the parent ID, try to lookup which + # mediated device is available that matches the UUID. If multiple + # devices are found that match the UUID, then this is an error + # condition. + try: + virtdev = self._host.device_lookup_by_name(devname) + except libvirt.libvirtError as ex: + if ex.get_error_code() != libvirt.VIR_ERR_NO_NODE_DEVICE: + raise + mdevs = [dev for dev in self._host.list_mediated_devices() + if dev.startswith(devname)] + # If no matching devices are found, simply raise the original + # exception indicating that no devices are found. + if not mdevs: + raise + elif len(mdevs) > 1: + msg = ("The mediated device name %(devname)s refers to a UUID " + "that is present in multiple libvirt mediated devices. " + "Matching libvirt mediated devices are %(devices)s. " + "Mediated device UUIDs must be unique for Nova." % + {'devname': devname, + 'devices': ', '.join(mdevs)}) + raise exception.InvalidLibvirtMdevConfig(reason=msg) + + LOG.debug('Found requested device %s as %s. Using that.', + devname, mdevs[0]) + virtdev = self._host.device_lookup_by_name(mdevs[0]) xmlstr = virtdev.XMLDesc(0) cfgdev = vconfig.LibvirtConfigNodeDevice() cfgdev.parse_str(xmlstr) + # Starting with Libvirt 7.3, the uuid information is available in the + # node device information. If its there, use that. Otherwise, + # fall back to the previous behavior of parsing the uuid from the + # devname. + if cfgdev.mdev_information.uuid: + mdev_uuid = cfgdev.mdev_information.uuid + else: + mdev_uuid = libvirt_utils.mdev_name2uuid(cfgdev.name) device = { "dev_id": cfgdev.name, - # name is like mdev_00ead764_fdc0_46b6_8db9_2963f5c815b4 - "uuid": libvirt_utils.mdev_name2uuid(cfgdev.name), + "uuid": mdev_uuid, # the physical GPU PCI device "parent": cfgdev.parent, "type": cfgdev.mdev_information.type, diff --git a/nova/virt/libvirt/host.py b/nova/virt/libvirt/host.py index 92f58e6899a..ebcc1125345 100644 --- a/nova/virt/libvirt/host.py +++ b/nova/virt/libvirt/host.py @@ -1503,7 +1503,7 @@ def list_mdev_capable_devices(self, flags=0): def list_mediated_devices(self, flags=0): """Lookup mediated devices. - :returns: a list of virNodeDevice instance + :returns: a list of strings with the name of the instance """ return self._list_devices("mdev", flags=flags) diff --git a/nova/virt/libvirt/utils.py b/nova/virt/libvirt/utils.py index 834f242c792..a1b9459b7e6 100644 --- a/nova/virt/libvirt/utils.py +++ b/nova/virt/libvirt/utils.py @@ -581,17 +581,31 @@ def get_default_machine_type(arch: str) -> ty.Optional[str]: def mdev_name2uuid(mdev_name: str) -> str: - """Convert an mdev name (of the form mdev_) to a - uuid (of the form 8-4-4-4-12). + """Convert an mdev name (of the form mdev_ or + mdev__) to a uuid + (of the form 8-4-4-4-12). + + :param mdev_name: the name of the mdev to parse the UUID from + :returns: string containing the uuid """ - return str(uuid.UUID(mdev_name[5:].replace('_', '-'))) + mdev_uuid = mdev_name[5:].replace('_', '-') + # Unconditionnally remove the PCI address from the name + mdev_uuid = mdev_uuid[:36] + return str(uuid.UUID(mdev_uuid)) + +def mdev_uuid2name(mdev_uuid: str, parent: str = None) -> str: + """Convert an mdev uuid (of the form 8-4-4-4-12) and optionally its parent + device to a name (of the form mdev_[_]). -def mdev_uuid2name(mdev_uuid: str) -> str: - """Convert an mdev uuid (of the form 8-4-4-4-12) to a name (of the form - mdev_). + :param mdev_uuid: the uuid of the mediated device + :param parent: the parent device id for the mediated device + :returns: name of the mdev to reference in libvirt """ - return "mdev_" + mdev_uuid.replace('-', '_') + name = "mdev_" + mdev_uuid.replace('-', '_') + if parent and parent.startswith('pci_'): + name = name + parent[4:] + return name def get_flags_by_flavor_specs(flavor: 'objects.Flavor') -> ty.Set[str]: