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
This commit is contained in:
melanie witt
11
nova/tests/fixtures/libvirt.py
vendored
11
nova/tests/fixtures/libvirt.py
vendored
@@ -2282,6 +2282,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)
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -1607,36 +1607,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:
|
||||
|
||||
Reference in New Issue
Block a user