womax/nova
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge "[hacking]Do not mock threading.Event"

Browse Source
This commit is contained in:
Zuul
2026-01-27 20:42:15 +00:00
committed by Gerrit Code Review
parent 134d3ac476 19203d684d
commit ce286865f9
7 changed files with 124 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files
HACKING.rst
+2
View File
@@ -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
+27
View File
@@ -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
+14 -43
View File
@@ -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
nova/tests/unit/test_hacking.py
+51
View File
@@ -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
+25 -21
View File
@@ -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
nova/virt/libvirt/driver.py
+4 -1
View File
@@ -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
+1
View File
@@ -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