[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 <gibi@redhat.com>

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
 -------------------

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

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
+        self._live_migrate(server, "completed")
-        # 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")
         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
+        # now live migrate that server
-        # timeout happens.
+        # The OpenStackApiException means the server failed to be migrated
-        # We return True signaling that the event is set, i.e. the libvirt
+        exc = self.assertRaises(
-        # event the caller is waiting for has been received.
+            client.OpenStackApiException,
-        # Note: This mock behavior cannot be added to the fixture because
+            self._live_migrate,
-        # many unit tests rely on it for different side effects.
+            server,
-        with mock.patch("threading.Event.wait", side_effect=[True]):
+            "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
+        # now live migrate that server
-        # timeout happens.
+        self._live_migrate(server, "completed")
-        # 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.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
+        # now live migrate that server
-        # timeout happens.
+        self._live_migrate(server, "completed")
-        # 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")
 
         # we should now have transitioned our usage to the destination, freeing
         # up the source in the process

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)

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
+        # By mocking AsyncDeviceEventsHandler.Waiter.wait we prevent the test
-        # timeout happens, and by returning False first we simulate to the
+        # to wait until the timeout happens, and by returning False first we
-        # caller that the wait returned not because the event is set but
+        # simulate to the caller that the wait returned not because the event
-        # because timeout happened. Then during the retry we return True
+        # is set but because timeout happened. Then during the retry we return
-        # signalling that the event is set, i.e. the libvirt event the caller
+        # True signalling that the event is set, i.e. the libvirt event the
-        # is waiting for has been received
+        # 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
+        # By mocking AsyncDeviceEventsHandler.Waiter.wait we prevent the test
-        # timeout happens, and by returning False first we simulate to the
+        # to wait until the timeout happens, and by returning False first we
-        # caller that the wait returned not because the event is set but
+        # simulate to the caller that the wait returned not because the event
-        # because timeout happened. Then during the retry we return True
+        # is set but because timeout happened. Then during the retry we return
-        # signalling that the event is set, i.e. the libvirt event the caller
+        # True signalling that the event is set, i.e. the libvirt event the
-        # is waiting for has been received
+        # 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
+        # By mocking AsyncDeviceEventsHandler.Waiter.wait we prevent the test
-        # timeout happens, and by returning False we simulate to the
+        # to wait until the timeout happens, and by returning False we simulate
-        # caller that the wait returned not because the event is set but
+        # to the caller that the wait returned not because the event is set but
         # because timeout happened.
         mock_event_wait.return_value = False
 

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)

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