From c4f4ae784f2078b652d07053d5a69a81dba1a8f5 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Thu, 20 Feb 2025 04:09:44 +0000 Subject: [PATCH] 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 (cherry picked from commit 2c07aa06458a2e70d79f8b3c68960bdccd11ab57) --- nova/tests/fixtures/libvirt.py | 11 +++ .../regressions/test_bug_2098892.py | 69 +------------------ nova/tests/unit/virt/libvirt/test_host.py | 24 +++---- nova/virt/libvirt/host.py | 20 +++--- 4 files changed, 33 insertions(+), 91 deletions(-) diff --git a/nova/tests/fixtures/libvirt.py b/nova/tests/fixtures/libvirt.py index cafe55a580bd..089d22c6cbd6 100644 --- a/nova/tests/fixtures/libvirt.py +++ b/nova/tests/fixtures/libvirt.py @@ -2247,6 +2247,17 @@ class Connection(object): 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/functional/regressions/test_bug_2098892.py b/nova/tests/functional/regressions/test_bug_2098892.py index 909a77bc92cd..a83b03158137 100644 --- a/nova/tests/functional/regressions/test_bug_2098892.py +++ b/nova/tests/functional/regressions/test_bug_2098892.py @@ -10,7 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -from nova import test from nova.tests.fixtures import libvirt as fakelibvirt from nova.tests.functional.libvirt import test_vgpu @@ -47,73 +46,7 @@ class VGPUTestsListDevices(test_vgpu.VGPUTestBase): self.start_compute_with_vgpu('host1') - def fake_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)}') - - # FIXME(melwitt): We need to patch this only for this test because if - # we add it to the LibvirtFixture right away, it will cause the - # following additional tests to fail: - # - # nova.tests.functional.libvirt.test_reshape.VGPUReshapeTests - # test_create_servers_with_vgpu - # - # nova.tests.functional.libvirt.test_vgpu.DifferentMdevClassesTests - # test_create_servers_with_different_mdev_classes - # test_resize_servers_with_mlx5 - # - # nova.tests.functional.libvirt.test_vgpu.VGPULimitMultipleTypesTests - # test_create_servers_with_vgpu - # - # nova.tests.functional.libvirt.test_vgpu.VGPULiveMigrationTests - # test_live_migrate_server - # test_live_migration_fails_on_old_source - # test_live_migration_fails_due_to_non_supported_mdev_types - # test_live_migration_fails_on_old_destination - # - # nova.tests.functional.libvirt. - # test_vgpu.VGPULiveMigrationTestsLMFailed - # test_live_migrate_server - # test_live_migration_fails_on_old_source - # test_live_migration_fails_due_to_non_supported_mdev_types - # test_live_migration_fails_on_old_destination - # - # nova.tests.functional.libvirt.test_vgpu.VGPUMultipleTypesTests - # test_create_servers_with_specific_type - # test_create_servers_with_vgpu - # - # nova.tests.functional.libvirt.test_vgpu.VGPUTests - # test_multiple_instance_create - # test_create_servers_with_vgpu - # test_create_server_with_two_vgpus_isolated - # test_resize_servers_with_vgpu - # - # nova.tests.functional.libvirt.test_vgpu.VGPUTestsLibvirt7_3 - # test_create_servers_with_vgpu - # test_create_server_with_two_vgpus_isolated - # test_resize_servers_with_vgpu - # test_multiple_instance_create - # - # nova.tests.functional.regressions. - # test_bug_1951656.VGPUTestsLibvirt7_7 - # test_create_servers_with_vgpu - self.stub_out( - 'nova.tests.fixtures.libvirt.Connection.nodeDeviceLookupByName', - fake_nodeDeviceLookupByName) - def test_update_available_resource(self): # We only want to verify no errors were logged by # update_available_resource (logging under the 'except Exception:'). - # FIXME(melwitt): This currently will log an error and traceback - # because of the bug. Update this when the bug is fixed. - e = self.assertRaises( - test.TestingException, self._run_periodics, raise_on_error=True) - self.assertIn('TypeError', str(e)) + self._run_periodics(raise_on_error=True) diff --git a/nova/tests/unit/virt/libvirt/test_host.py b/nova/tests/unit/virt/libvirt/test_host.py index 5394671584a5..66462e0d1860 100644 --- a/nova/tests/unit/virt/libvirt/test_host.py +++ b/nova/tests/unit/virt/libvirt/test_host.py @@ -2268,24 +2268,24 @@ class LibvirtTpoolProxyTestCase(test.NoDBTestCase): 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 73ceffdb10ac..4a8d3a6f5d83 100644 --- a/nova/virt/libvirt/host.py +++ b/nova/virt/libvirt/host.py @@ -1620,36 +1620,34 @@ class Host(object): 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: