From 19203d684d2c9836c02608185fa30eafa494f069 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Thu, 18 Dec 2025 17:48:10 +0100 Subject: [PATCH] [hacking]Do not mock threading.Event Such mock is too wide and will cause issues with our basic libraries and test infrastructure leading to race conditions and threads leaked across tests. We needed to remove a bunch of such mocks found by the new rule. In some cases we needed to make the mocking more specific for a given Event instance, in other case the mock was not needed at all and the test case was still not taking excessive time. Related-Bug: #2136815 Change-Id: I3ae3740eb07bade4e0883db3e02c0a81e92b9a36 Signed-off-by: Balazs Gibizer --- HACKING.rst | 2 + nova/hacking/checks.py | 27 +++++++++ .../libvirt/test_pci_sriov_servers.py | 57 +++++-------------- nova/tests/unit/test_hacking.py | 51 +++++++++++++++++ nova/tests/unit/virt/libvirt/test_driver.py | 46 ++++++++------- nova/virt/libvirt/driver.py | 5 +- tox.ini | 1 + 7 files changed, 124 insertions(+), 65 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index e42e0b9294..b82558e571 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -81,6 +81,8 @@ Nova Specific Commandments from stdlib instead. E.g. eventlet.sleep => time.sleep - [N374] Don't use time.sleep(0) to trigger eventlet yielding. Use nova.utils.cooperative_yield() instead. +- [N375] Don't mock threading.Event directly, target the mock to your + specific Event instance. Creating Unit Tests ------------------- diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 9f33a724d4..8275a8d562 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -149,6 +149,12 @@ eventlet_yield_re = re.compile(r".*time\.sleep\(0\).*") eventlet_primitives_re = re.compile( r".*(eventlet)\.(semaphore|timeout|event).*" r"|from\s+eventlet\s+import\s+(semaphore|timeout|event)") +threading_event_mock_re = re.compile( + r"mock.patch\(.threading.Event.wait|" + r"mock.patch.object\(threading, .Event.wait|" + r"mock.patch\(.threading.Event|" + r"mock.patch.object\(threading, .Event" +) class BaseASTChecker(ast.NodeVisitor): @@ -1143,3 +1149,24 @@ def check_eventlet_yield(logical_line, filename): if match: yield (0, msg) + + +@core.flake8ext +def check_threading_event_mock(physical_line, filename): + """Check to prevent mocking threading.Event creation and usage. + + N375 + """ + msg = ( + "N375: Mocking threading.Event creation or usage leads to unexpected" + "behavior in our base libs like oslo.service.LoopingCall and our test" + "fixtures and causes leaked calls across test cases. Target your" + "mocking to the specific Event instance instead.") + + if filename == 'nova/tests/unit/test_hacking.py': + return + + match = re.search(threading_event_mock_re, physical_line) + + if match: + return 0, msg diff --git a/nova/tests/functional/libvirt/test_pci_sriov_servers.py b/nova/tests/functional/libvirt/test_pci_sriov_servers.py index 35fc589487..ad2033b011 100644 --- a/nova/tests/functional/libvirt/test_pci_sriov_servers.py +++ b/nova/tests/functional/libvirt/test_pci_sriov_servers.py @@ -2545,15 +2545,7 @@ class SRIOVServersTest(_PCIServersWithMigrationTestBase): port['binding:profile'], ) - # By mocking threading.Event.wait we prevent the test to wait until the - # timeout happens. - # We return True signaling that the event is set, i.e. the libvirt - # event the caller is waiting for has been received. - # Note: This mock behavior cannot be added to the fixture because - # many unit tests rely on it for different side effects. - with mock.patch("threading.Event.wait", side_effect=[True]): - # now live migrate that server - self._live_migrate(server, "completed") + self._live_migrate(server, "completed") dst_xml = self._get_xml(self.comp1, server) self.assertPCIDeviceCounts(self.comp0, total=2, free=2) self.assertPCIDeviceCounts(self.comp1, total=2, free=0) @@ -2904,21 +2896,14 @@ class SRIOVServersTest(_PCIServersWithMigrationTestBase): port['binding:profile'], ) - # By mocking threading.Event.wait we prevent the test to wait until the - # timeout happens. - # We return True signaling that the event is set, i.e. the libvirt - # event the caller is waiting for has been received. - # Note: This mock behavior cannot be added to the fixture because - # many unit tests rely on it for different side effects. - with mock.patch("threading.Event.wait", side_effect=[True]): - # now live migrate that server - # The OpenStackApiException means the server failed to be migrated - exc = self.assertRaises( - client.OpenStackApiException, - self._live_migrate, - server, - "completed", - ) + # now live migrate that server + # The OpenStackApiException means the server failed to be migrated + exc = self.assertRaises( + client.OpenStackApiException, + self._live_migrate, + server, + "completed", + ) self.assertEqual(500, exc.response.status_code) self.assertIn('NoValidHost', str(exc)) self.assertPCIDeviceCounts(self.comp0, total=2, free=0) @@ -3115,15 +3100,8 @@ class SRIOVServersTest(_PCIServersWithMigrationTestBase): port['binding:profile'], ) - # By mocking threading.Event.wait we prevent the test to wait until the - # timeout happens. - # We return True signaling that the event is set, i.e. the libvirt - # event the caller is waiting for has been received. - # Note: This mock behavior cannot be added to the fixture because - # many unit tests rely on it for different side effects. - with mock.patch("threading.Event.wait", side_effect=[True]): - # now live migrate that server - self._live_migrate(server, "completed") + # now live migrate that server + self._live_migrate(server, "completed") self.assert_placement_pci_view( self.comp0, inventories={"0000:81:00.0": {'CUSTOM_A16_16A': 1}}, @@ -3591,7 +3569,7 @@ class SRIOVServersTest(_PCIServersWithMigrationTestBase): This should succeed since we support this, via detach and attach of the PCI device. """ - + self.flags(device_detach_timeout="1", group="libvirt") # start two compute services with differing PCI device inventory source_pci_info = fakelibvirt.HostPCIDevicesInfo( num_pfs=1, num_vfs=4, numa_node=0) @@ -3692,15 +3670,8 @@ class SRIOVServersTest(_PCIServersWithMigrationTestBase): pf_port['binding:profile'], ) - # By mocking threading.Event.wait we prevent the test to wait until the - # timeout happens. - # We return True signaling that the event is set, i.e. the libvirt - # event the caller is waiting for has been received. - # Note: This mock behavior cannot be added to the fixture because - # many unit tests rely on it for different side effects. - with mock.patch("threading.Event.wait", side_effect=[True]): - # now live migrate that server - self._live_migrate(server, "completed") + # now live migrate that server + self._live_migrate(server, "completed") # we should now have transitioned our usage to the destination, freeing # up the source in the process diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index 9560035ce5..9ecf99519f 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -1105,3 +1105,54 @@ class HackingTestCase(test.NoDBTestCase): time.sleep(1) """ self._assert_has_no_errors(code, checks.check_eventlet_yield) + + def test_check_threading_event_mock(self): + code = """ + import threading + + with mock.patch('threading.Event.wait') as m: + pass + + with mock.patch("threading.Event.wait") as m: + pass + + with mock.patch.object(threading, 'Event.wait') as m: + pass + + with mock.patch("threading.Event") as m: + pass + + with mock.patch.object(threading, 'Event') as m: + pass + + @mock.patch('threading.Event.wait', new=mock.Mock()) + def test_foo(self): + pass + + @mock.patch("threading.Event.wait", new=mock.Mock()) + def test_foo(self): + pass + + @mock.patch.object(threading, 'Event.wait', new=mock.Mock()) + def test_foo(self): + pass + + @mock.patch("threading.Event", new=mock.Mock()) + def test_foo(self): + pass + + @mock.patch.object(threading, 'Event', new=mock.Mock()) + def test_foo(self): + pass + """ + errors = [(x + 1, 0, 'N375') + for x in [2, 5, 8, 11, 14, 17, 21, 25, 29, 33]] + self._assert_has_errors( + code, checks.check_threading_event_mock, expected_errors=errors) + + code = """ + my_event = threading.Event() + with mock.patch.object(my_event, 'wait') as m: + pass + """ + self._assert_has_no_errors(code, checks.check_threading_event_mock) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 44ee4cf2ca..0b6057e3a5 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -10925,10 +10925,10 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_build_metadata.assert_called_with(self.context, instance) mock_save.assert_called_with() - @mock.patch('threading.Event', new=mock.Mock()) @mock.patch('nova.virt.libvirt.host.Host._get_domain') def test_detach_volume_with_vir_domain_affect_live_flag(self, mock_get_domain, use_alias=True): + self.flags(device_detach_timeout="1", group="libvirt") drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) instance = objects.Instance(**self.test_instance) volume_id = uuids.volume @@ -26314,7 +26314,6 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): self._test_attach_interface( power_state.SHUTDOWN, fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG) - @mock.patch('threading.Event.wait', new=mock.Mock()) def _test_detach_interface(self, state, device_not_found=False): # setup some mocks instance = self._create_instance() @@ -26433,6 +26432,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): mock_unplug.assert_called_once_with(instance, network_info[0]) def test_detach_interface_with_running_instance(self): + self.flags(device_detach_timeout="1", group="libvirt") self._test_detach_interface(power_state.RUNNING) def test_detach_interface_with_running_instance_device_not_found(self): @@ -26441,6 +26441,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): self._test_detach_interface(power_state.RUNNING, device_not_found=True) def test_detach_interface_with_pause_instance(self): + self.flags(device_detach_timeout="1", group="libvirt") self._test_detach_interface(power_state.PAUSED) def test_detach_interface_with_shutdown_instance(self): @@ -26469,12 +26470,12 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): self.assertIn('the device is no longer found on the guest', str(mock_log.warning.call_args[0])) - @mock.patch('threading.Event.wait', new=mock.Mock()) @mock.patch.object(FakeVirtDomain, 'info') @mock.patch.object(FakeVirtDomain, 'detachDeviceFlags') @mock.patch.object(host.Host, '_get_domain') def test_detach_interface_device_with_same_mac_address( self, mock_get_domain, mock_detach, mock_info): + self.flags(device_detach_timeout="1", group="libvirt") instance = self._create_instance() network_info = _fake_network_info(self) domain = FakeVirtDomain(fake_xml=""" @@ -26904,7 +26905,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): # check that the internal event handling is cleaned up self.assertEqual(set(), drvr._device_event_handler._waiters) - @mock.patch('threading.Event.wait') + @mock.patch( + 'nova.virt.libvirt.driver.AsyncDeviceEventsHandler.Waiter.wait') @ddt.data(power_state.RUNNING, power_state.PAUSED) def test__detach_with_retry_timeout_retry_succeeds( self, state, mock_event_wait @@ -26933,12 +26935,12 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): None, ] ) - # By mocking threading.Event.wait we prevent the test to wait until the - # timeout happens, and by returning False first we simulate to the - # caller that the wait returned not because the event is set but - # because timeout happened. Then during the retry we return True - # signalling that the event is set, i.e. the libvirt event the caller - # is waiting for has been received + # By mocking AsyncDeviceEventsHandler.Waiter.wait we prevent the test + # to wait until the timeout happens, and by returning False first we + # simulate to the caller that the wait returned not because the event + # is set but because timeout happened. Then during the retry we return + # True signalling that the event is set, i.e. the libvirt event the + # caller is waiting for has been received mock_event_wait.side_effect = [False, True] drvr._detach_with_retry( @@ -26960,7 +26962,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): # check that the internal event handling is cleaned up self.assertEqual(set(), drvr._device_event_handler._waiters) - @mock.patch('threading.Event.wait') + @mock.patch( + 'nova.virt.libvirt.driver.AsyncDeviceEventsHandler.Waiter.wait') def test__detach_with_retry_timeout_retry_unplug_in_progress( self, mock_event_wait ): @@ -26990,12 +26993,12 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): None, ] ) - # By mocking threading.Event.wait we prevent the test to wait until the - # timeout happens, and by returning False first we simulate to the - # caller that the wait returned not because the event is set but - # because timeout happened. Then during the retry we return True - # signalling that the event is set, i.e. the libvirt event the caller - # is waiting for has been received + # By mocking AsyncDeviceEventsHandler.Waiter.wait we prevent the test + # to wait until the timeout happens, and by returning False first we + # simulate to the caller that the wait returned not because the event + # is set but because timeout happened. Then during the retry we return + # True signalling that the event is set, i.e. the libvirt event the + # caller is waiting for has been received mock_event_wait.side_effect = [False, True] # there will be two detach attempts @@ -27039,7 +27042,8 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): # check that the internal event handling is cleaned up self.assertEqual(set(), drvr._device_event_handler._waiters) - @mock.patch('threading.Event.wait') + @mock.patch( + 'nova.virt.libvirt.driver.AsyncDeviceEventsHandler.Waiter.wait') @ddt.data(power_state.RUNNING, power_state.PAUSED) def test__detach_with_retry_timeout_run_out_of_retries( self, state, mock_event_wait @@ -27061,9 +27065,9 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): mock_get_device_conf_func = mock.Mock(return_value=mock_dev) - # By mocking threading.Event.wait we prevent the test to wait until the - # timeout happens, and by returning False we simulate to the - # caller that the wait returned not because the event is set but + # By mocking AsyncDeviceEventsHandler.Waiter.wait we prevent the test + # to wait until the timeout happens, and by returning False we simulate + # to the caller that the wait returned not because the event is set but # because timeout happened. mock_event_wait.return_value = False diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 131c6e9e57..a8f291c320 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -303,6 +303,9 @@ class AsyncDeviceEventsHandler: self.device_name == event.dev and isinstance(event, tuple(self.event_types))) + def wait(self, timeout: float) -> bool: + return self.threading_event.wait(timeout) + def __repr__(self) -> str: return ( "AsyncDeviceEventsHandler.Waiter(" @@ -360,7 +363,7 @@ class AsyncDeviceEventsHandler: the event to be received :returns: The received libvirt event, or None in case of timeout """ - token.threading_event.wait(timeout) + token.wait(timeout) with self._lock: self._waiters.remove(token) diff --git a/tox.ini b/tox.ini index 4536aaf8ed..b309179506 100644 --- a/tox.ini +++ b/tox.ini @@ -399,6 +399,7 @@ extension = N372 = checks:check_set_daemon N373 = checks:check_eventlet_primitives N374 = checks:check_eventlet_yield + N375 = checks:check_threading_event_mock paths = ./nova/hacking