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 2c07aa0)
(cherry picked from commit c4f4ae7)
(cherry picked from commit b8bfa1e)
(cherry picked from commit c446736)

Author: melwitt

nova/tests/fixtures/libvirt.py

@@ -2067,6 +2067,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)
 

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 @@ def setUp(self):
 
         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)

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

@@ -2175,24 +2175,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):

nova/virt/libvirt/host.py

@@ -1565,36 +1565,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: