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