From 00457565d506e95d1da5c7c9a8276928bbca221e Mon Sep 17 00:00:00 2001 From: Mohammed Naser Date: Fri, 6 May 2022 17:18:35 -0400 Subject: [PATCH 1/3] Fix race condition in _get_pci_passthrough_devices The call to _get_pci_passthrough_devices could fail because a network device could have disappeared which would cause a traceback in the logs. This wraps the function in a safe way to return an empty array if it fails, which will clean-up the logs if the device disappears Closes-Bug: #1972028 Change-Id: I46d3bbe122d9f8452f168286391bab67ecea3128 --- nova/virt/libvirt/driver.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index fef9d5083e3..55c470b166c 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -7959,13 +7959,27 @@ def _get_pci_passthrough_devices(self): dev.name(): dev for dev in self._host.list_all_devices(flags=dev_flags) } - net_devs = [dev for dev in devices.values() if "net" in dev.listCaps()] + + # NOTE(mnaser): The listCaps() function can raise an exception if the + # device disappeared while we're looping, this method + # returns an empty list rather than raising an exception + # which will remove the device for Nova's resource + # tracker, but that is OK since the device disappeared. + def _safe_list_caps(dev): + try: + return dev.listCaps() + except libvirt.libvirtError: + return [] + + net_devs = [ + dev for dev in devices.values() if "net" in _safe_list_caps(dev) + ] vdpa_devs = [ - dev for dev in devices.values() if "vdpa" in dev.listCaps() + dev for dev in devices.values() if "vdpa" in _safe_list_caps(dev) ] pci_devs = { name: dev for name, dev in devices.items() - if "pci" in dev.listCaps()} + if "pci" in _safe_list_caps(dev)} pci_info = [ self._host._get_pcidev_info( name, dev, net_devs, From 7932b20eecd45e13532d0f0fb54b706002f88716 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Fri, 18 Oct 2024 02:54:02 +0000 Subject: [PATCH 2/3] libvirt: Wrap un-proxied listDevices() and listAllDevices() This is similar to change I668643c836d46a25df46d4c99a973af5e50a39db where the objects returned in a list from a libvirt call were not tpool.Proxy wrapped. Because the objects are not wrapped, calling methods on them such as listCaps() can block all other greenthreads and can cause nova-compute to freeze for hours in certain scenarios. This adds the same wrapping to libvirt calls which return lists of virNodeDevice. Closes-Bug: #2091033 Change-Id: I60d6f04d374e9ede5895a43b7a75e955b0fea3c5 --- nova/tests/unit/virt/libvirt/test_host.py | 42 +++++++++++++++++++ nova/virt/libvirt/host.py | 9 +++- ...libvirt-list-devices-7cd218c1a33535c9.yaml | 11 +++++ 3 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/unproxied-libvirt-list-devices-7cd218c1a33535c9.yaml diff --git a/nova/tests/unit/virt/libvirt/test_host.py b/nova/tests/unit/virt/libvirt/test_host.py index 6fb6f98281c..444186c8047 100644 --- a/nova/tests/unit/virt/libvirt/test_host.py +++ b/nova/tests/unit/virt/libvirt/test_host.py @@ -2128,6 +2128,48 @@ def test_tpool_list_all_connections(self): self.assertIsInstance(domain, tpool.Proxy) self.assertIn(domain.UUIDString(), (uuids.vm1, uuids.vm2)) + def _add_fake_host_devices(self): + self.conn._obj.pci_info = fakelibvirt.HostPCIDevicesInfo( + num_pci=1, num_pfs=2, num_vfs=2, num_mdevcap=3) + mdevs = { + 'mdev_4b20d080_1b54_4048_85b3_a6a62d165c01': + fakelibvirt.FakeMdevDevice( + dev_name='mdev_4b20d080_1b54_4048_85b3_a6a62d165c01', + type_id=fakelibvirt.NVIDIA_11_VGPU_TYPE, + parent=fakelibvirt.MDEVCAP_DEV1_PCI_ADDR), + } + self.conn._obj.mdev_info = fakelibvirt.HostMdevDevicesInfo( + devices=mdevs) + + def test_tpool_list_all_devices(self): + self._add_fake_host_devices() + devs = self.host.list_all_devices( + fakelibvirt.VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV) + self.assertEqual(8, len(devs)) + for dev in devs: + self.assertIsInstance(dev, tpool.Proxy) + + def test_tpool_list_pci_devices(self): + self._add_fake_host_devices() + devs = self.host.list_pci_devices() + self.assertEqual(8, len(devs)) + for dev in devs: + self.assertIsInstance(dev, tpool.Proxy) + + def test_tpool_list_mdev_capable_devices(self): + self._add_fake_host_devices() + devs = self.host.list_mdev_capable_devices() + self.assertEqual(3, len(devs)) + for dev in devs: + self.assertIsInstance(dev, tpool.Proxy) + + def test_tpool_list_mediated_devices(self): + self._add_fake_host_devices() + devs = self.host.list_mediated_devices() + self.assertEqual(1, len(devs)) + for dev in devs: + self.assertIsInstance(dev, tpool.Proxy) + class LoadersTestCase(test.NoDBTestCase): diff --git a/nova/virt/libvirt/host.py b/nova/virt/libvirt/host.py index 6c1eab17cb5..0e6a4f30bc7 100644 --- a/nova/virt/libvirt/host.py +++ b/nova/virt/libvirt/host.py @@ -1513,7 +1513,9 @@ def _list_devices(self, cap, flags=0): :returns: a list of virNodeDevice instance """ try: - return self.get_connection().listDevices(cap, flags) + devs = [self._wrap_libvirt_proxy(dev) + for dev in self.get_connection().listDevices(cap, flags)] + return devs except libvirt.libvirtError as ex: error_code = ex.get_error_code() if error_code == libvirt.VIR_ERR_NO_SUPPORT: @@ -1533,7 +1535,10 @@ def list_all_devices( :returns: a list of virNodeDevice xml strings. """ try: - return self.get_connection().listAllDevices(flags) or [] + alldevs = [ + self._wrap_libvirt_proxy(dev) + for dev in self.get_connection().listAllDevices(flags)] or [] + return alldevs except libvirt.libvirtError as ex: LOG.warning(ex) return [] diff --git a/releasenotes/notes/unproxied-libvirt-list-devices-7cd218c1a33535c9.yaml b/releasenotes/notes/unproxied-libvirt-list-devices-7cd218c1a33535c9.yaml new file mode 100644 index 00000000000..eaf7b9b1ca8 --- /dev/null +++ b/releasenotes/notes/unproxied-libvirt-list-devices-7cd218c1a33535c9.yaml @@ -0,0 +1,11 @@ +fixes: + - | + `Bug #2091033`_: Fixed calls to libvirt ``listDevices()`` and + ``listAllDevices()`` from potentially blocking all other greenthreads + in ``nova-compute``. Under certain circumstances, it was possible for + the ``nova-compute`` service to freeze with all other greenthreads + blocked and unable to perform any other activities including logging. + This issue has been fixed by wrapping the libvirt ``listDevices()`` + and ``listAllDevices()`` calls with ``eventlet.tpool.Proxy``. + + .. _Bug #2091033: https://bugs.launchpad.net/nova/+bug/2091033 From 95f46c2f15d3f7acff0685a10727acee70673b02 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Thu, 20 Feb 2025 04:09:44 +0000 Subject: [PATCH 3/3] libvirt: Fix regression of listDevices() return type This a partial revert of change I60d6f04d374e9ede5895a43b7a75e955b0fea3c5 which added tpool.Proxy wrapping to the listDevices() and listAllDevices() methods. The regression was caught during downstream testing with vGPUs and the update_available_resource() periodic task was failing with: TypeError: virNodeDeviceLookupByName() argument 2 must be str or None, not Proxy It turns out that while the listAllDevices() method returns a list of virNodeDevice objects [1], the listDevices() method returns a list of string names [2] and is generated from the corresponding function in C [3]. The error was not caught by unit or functional testing because those test environments intentionally do not import the libvirt Python module -- so mocked code in the LibvirtFixture runs instead. Also, the update_available_resource() method has a 'except Exception:' at the end which logs an error but does not re-raise. So it would not cause a functional test to fail. This reverts the change that caused the regression, updates potentially confusing docstrings, adds type annotations to the methods that use listDevices(), and moves the nodeDeviceLookupByName type checking into the LibvirtFixture. Closes-Bug: #2098892 [1] https://github.com/libvirt/libvirt-python/blob/408815a/libvirt-override-virConnect.py#L520-L524 [2] https://github.com/libvirt/libvirt-python/blob/408815a/libvirt-override-api.xml#L448-L453 [3] https://libvirt.org/html/libvirt-libvirt-nodedev.html#virNodeListDevices Change-Id: Ib5befdd3c13367daa208ff969f66cba693ae2c76 --- nova/tests/fixtures/libvirt.py | 11 +++++++++++ nova/tests/unit/virt/libvirt/test_host.py | 24 +++++++++++------------ nova/virt/libvirt/host.py | 20 +++++++++---------- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/nova/tests/fixtures/libvirt.py b/nova/tests/fixtures/libvirt.py index 5ccf01e40f9..c7692b3100a 100644 --- a/nova/tests/fixtures/libvirt.py +++ b/nova/tests/fixtures/libvirt.py @@ -2052,6 +2052,17 @@ def device_lookup_by_name(self, dev_name): return self.pci_info.get_device_by_name(dev_name) def nodeDeviceLookupByName(self, name): + # See bug https://bugs.launchpad.net/nova/+bug/2098892 + # We don't test this by importing the libvirt module because the + # libvirt module is forbidden to be imported into our test + # environment. It is excluded from test-requirements.txt and we + # also use the ImportModulePoisonFixture in nova/test.py to prevent + # use of modules such as libvirt. + if not isinstance(name, str) and name is not None: + raise TypeError( + 'virNodeDeviceLookupByName() argument 2 must be str or ' + f'None, not {type(name)}') + if name.startswith('mdev'): return self.mdev_info.get_device_by_name(name) diff --git a/nova/tests/unit/virt/libvirt/test_host.py b/nova/tests/unit/virt/libvirt/test_host.py index 444186c8047..3cf252ecc12 100644 --- a/nova/tests/unit/virt/libvirt/test_host.py +++ b/nova/tests/unit/virt/libvirt/test_host.py @@ -2151,24 +2151,24 @@ def test_tpool_list_all_devices(self): def test_tpool_list_pci_devices(self): self._add_fake_host_devices() - devs = self.host.list_pci_devices() - self.assertEqual(8, len(devs)) - for dev in devs: - self.assertIsInstance(dev, tpool.Proxy) + dev_names = self.host.list_pci_devices() + self.assertEqual(8, len(dev_names)) + for name in dev_names: + self.assertIsInstance(name, str) def test_tpool_list_mdev_capable_devices(self): self._add_fake_host_devices() - devs = self.host.list_mdev_capable_devices() - self.assertEqual(3, len(devs)) - for dev in devs: - self.assertIsInstance(dev, tpool.Proxy) + dev_names = self.host.list_mdev_capable_devices() + self.assertEqual(3, len(dev_names)) + for name in dev_names: + self.assertIsInstance(name, str) def test_tpool_list_mediated_devices(self): self._add_fake_host_devices() - devs = self.host.list_mediated_devices() - self.assertEqual(1, len(devs)) - for dev in devs: - self.assertIsInstance(dev, tpool.Proxy) + dev_names = self.host.list_mediated_devices() + self.assertEqual(1, len(dev_names)) + for name in dev_names: + self.assertIsInstance(name, str) class LoadersTestCase(test.NoDBTestCase): diff --git a/nova/virt/libvirt/host.py b/nova/virt/libvirt/host.py index 0e6a4f30bc7..fa94860db11 100644 --- a/nova/virt/libvirt/host.py +++ b/nova/virt/libvirt/host.py @@ -1486,36 +1486,34 @@ def get_vdpa_device_path( nodedev = self.get_vdpa_nodedev_by_address(pci_address) return nodedev.vdpa_capability.dev_path - def list_pci_devices(self, flags=0): + def list_pci_devices(self, flags: int = 0) -> ty.List[str]: """Lookup pci devices. - :returns: a list of virNodeDevice instance + :returns: a list of strings, names of the virNodeDevice instances """ return self._list_devices("pci", flags=flags) - def list_mdev_capable_devices(self, flags=0): + def list_mdev_capable_devices(self, flags: int = 0) -> ty.List[str]: """Lookup devices supporting mdev capabilities. - :returns: a list of virNodeDevice instance + :returns: a list of strings, names of the virNodeDevice instances """ return self._list_devices("mdev_types", flags=flags) - def list_mediated_devices(self, flags=0): + def list_mediated_devices(self, flags: int = 0) -> ty.List[str]: """Lookup mediated devices. - :returns: a list of strings with the name of the instance + :returns: a list of strings, names of the virNodeDevice instances """ return self._list_devices("mdev", flags=flags) - def _list_devices(self, cap, flags=0): + def _list_devices(self, cap, flags: int = 0) -> ty.List[str]: """Lookup devices. - :returns: a list of virNodeDevice instance + :returns: a list of strings, names of the virNodeDevice instances """ try: - devs = [self._wrap_libvirt_proxy(dev) - for dev in self.get_connection().listDevices(cap, flags)] - return devs + return self.get_connection().listDevices(cap, flags) except libvirt.libvirtError as ex: error_code = ex.get_error_code() if error_code == libvirt.VIR_ERR_NO_SUPPORT: