From 17140c44b8730141a818cff3c3bd14a597de55c2 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Fri, 16 Feb 2024 00:48:30 -0700 Subject: [PATCH 1/5] Update deltaproxy test timeout --- .../integration/cli/test_salt_deltaproxy.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/pytests/integration/cli/test_salt_deltaproxy.py b/tests/pytests/integration/cli/test_salt_deltaproxy.py index a765164c95d..99c8368b73d 100644 --- a/tests/pytests/integration/cli/test_salt_deltaproxy.py +++ b/tests/pytests/integration/cli/test_salt_deltaproxy.py @@ -19,7 +19,7 @@ pytestmark = [ reason="Deltaproxy minions do not currently work on spawning platforms.", ), pytest.mark.core_test, - pytest.mark.timeout_unless_on_windows(320), + pytest.mark.timeout_unless_on_windows(400), ] @@ -182,7 +182,7 @@ def test_exit_status_correct_usage( proxy_minion_id, defaults=config_defaults, extra_cli_arguments_after_first_start_failure=["--log-level=info"], - start_timeout=240, + start_timeout=320, ) for minion_id in (proxy_minion_id, proxy_one, proxy_two): @@ -285,7 +285,7 @@ def test_missing_pillar_file( proxy_minion_id, defaults=config_defaults, extra_cli_arguments_after_first_start_failure=["--log-level=info"], - start_timeout=240, + start_timeout=320, ) for minion_id in (proxy_minion_id, proxy_one, proxy_two): @@ -409,7 +409,7 @@ def test_invalid_connection( proxy_minion_id, defaults=config_defaults, extra_cli_arguments_after_first_start_failure=["--log-level=info"], - start_timeout=240, + start_timeout=320, ) for minion_id in ( @@ -538,7 +538,7 @@ def ping(): proxy_minion_id, defaults=config_defaults, extra_cli_arguments_after_first_start_failure=["--log-level=info"], - start_timeout=240, + start_timeout=320, ) for minion_id in (proxy_minion_id, proxy_one, proxy_two): @@ -671,7 +671,7 @@ def ping(): proxy_minion_id, defaults=config_defaults, extra_cli_arguments_after_first_start_failure=["--log-level=info"], - start_timeout=240, + start_timeout=320, ) for minion_id in (proxy_minion_id, proxy_one, proxy_two): @@ -812,7 +812,7 @@ def test_exit_status_correct_usage_large_number_of_minions( proxy_minion_id, defaults=config_defaults, extra_cli_arguments_after_first_start_failure=["--log-level=info"], - start_timeout=240, + start_timeout=320, ) for minion_id in [proxy_minion_id] + sub_proxies: From 1ad68fcd93cdf458ac6a76e45fc6b980a8584afa Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Fri, 16 Feb 2024 17:02:51 +0000 Subject: [PATCH 2/5] Increase timeouts on problematic tests and default to `func_only=True` --- tests/conftest.py | 13 +++++++++---- tests/integration/ssh/test_state.py | 1 - tests/pytests/functional/loader/test_loader.py | 2 +- tests/pytests/functional/states/test_pkg.py | 2 +- tests/pytests/integration/cli/test_syndic_eauth.py | 2 +- .../pytests/integration/daemons/test_memory_leak.py | 1 + tests/pytests/integration/modules/test_virt.py | 2 +- tests/pytests/integration/netapi/test_ssh_client.py | 2 +- tests/pytests/integration/states/test_beacon.py | 2 +- 9 files changed, 16 insertions(+), 11 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index f0c24f43d1b..5f79f67e8d6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -460,9 +460,12 @@ def pytest_collection_modifyitems(config, items): if marker is not None: if not salt.utils.platform.is_windows(): # Apply the marker since we're not on windows - item.add_marker( - pytest.mark.timeout(*marker.args, **marker.kwargs.copy()) - ) + marker_kwargs = marker.kwargs.copy() + if "func_only" not in marker_kwargs: + # Default to counting only the test execution for the timeouts, ie, + # withough including the fixtures setup time towards the timeout. + marker_kwargs["func_only"] = True + item.add_marker(pytest.mark.timeout(*marker.args, **marker_kwargs)) else: if ( not salt.utils.platform.is_windows() @@ -473,7 +476,9 @@ def pytest_collection_modifyitems(config, items): ): # Let's apply the timeout marker on the test, if the marker # is not already applied - item.add_marker(pytest.mark.timeout(90)) + # Default to counting only the test execution for the timeouts, ie, + # withough including the fixtures setup time towards the timeout. + item.add_marker(pytest.mark.timeout(90, func_only=True)) for fixture in item.fixturenames: if fixture not in item._fixtureinfo.name2fixturedefs: continue diff --git a/tests/integration/ssh/test_state.py b/tests/integration/ssh/test_state.py index 3951358b9e2..e5dcd3053b5 100644 --- a/tests/integration/ssh/test_state.py +++ b/tests/integration/ssh/test_state.py @@ -56,7 +56,6 @@ class SSHStateTest(SSHCase): self.assertTrue(check_file) @pytest.mark.slow_test - @pytest.mark.timeout_unless_on_windows(120) def test_state_sls_id(self): """ test state.sls_id with salt-ssh diff --git a/tests/pytests/functional/loader/test_loader.py b/tests/pytests/functional/loader/test_loader.py index 00c16708e8f..f34ca2239cf 100644 --- a/tests/pytests/functional/loader/test_loader.py +++ b/tests/pytests/functional/loader/test_loader.py @@ -9,7 +9,7 @@ from tests.support.pytest.helpers import FakeSaltExtension pytestmark = [ # These are slow because they create a virtualenv and install salt in it pytest.mark.slow_test, - pytest.mark.timeout_unless_on_windows(120), + pytest.mark.timeout_unless_on_windows(240), ] diff --git a/tests/pytests/functional/states/test_pkg.py b/tests/pytests/functional/states/test_pkg.py index 746b25f4502..9ea9aa98aa1 100644 --- a/tests/pytests/functional/states/test_pkg.py +++ b/tests/pytests/functional/states/test_pkg.py @@ -19,7 +19,7 @@ pytestmark = [ pytest.mark.slow_test, pytest.mark.skip_if_not_root, pytest.mark.destructive_test, - pytest.mark.timeout_unless_on_windows(120), + pytest.mark.timeout_unless_on_windows(240), ] diff --git a/tests/pytests/integration/cli/test_syndic_eauth.py b/tests/pytests/integration/cli/test_syndic_eauth.py index 07908629683..a37127c3949 100644 --- a/tests/pytests/integration/cli/test_syndic_eauth.py +++ b/tests/pytests/integration/cli/test_syndic_eauth.py @@ -12,7 +12,7 @@ log = logging.getLogger(__name__) pytestmark = [ pytest.mark.core_test, - pytest.mark.timeout_unless_on_windows(600, func_only=True), + pytest.mark.timeout_unless_on_windows(600), ] diff --git a/tests/pytests/integration/daemons/test_memory_leak.py b/tests/pytests/integration/daemons/test_memory_leak.py index fb608fc1864..a81c69c150b 100644 --- a/tests/pytests/integration/daemons/test_memory_leak.py +++ b/tests/pytests/integration/daemons/test_memory_leak.py @@ -6,6 +6,7 @@ import pytest pytestmark = [ pytest.mark.slow_test, + pytest.mark.timeout_unless_on_windows(240), ] diff --git a/tests/pytests/integration/modules/test_virt.py b/tests/pytests/integration/modules/test_virt.py index fbb9c3d0a99..c3be6417d05 100644 --- a/tests/pytests/integration/modules/test_virt.py +++ b/tests/pytests/integration/modules/test_virt.py @@ -15,7 +15,7 @@ log = logging.getLogger(__name__) pytestmark = [ pytest.mark.slow_test, - pytest.mark.timeout_unless_on_windows(120, func_only=True), + pytest.mark.timeout_unless_on_windows(120), pytest.mark.skip_if_binaries_missing("docker"), ] diff --git a/tests/pytests/integration/netapi/test_ssh_client.py b/tests/pytests/integration/netapi/test_ssh_client.py index 20eb789b8f3..77c662fdea3 100644 --- a/tests/pytests/integration/netapi/test_ssh_client.py +++ b/tests/pytests/integration/netapi/test_ssh_client.py @@ -135,7 +135,7 @@ def test_ssh_disabled(client, auth_creds): assert ret is None -@pytest.mark.timeout_unless_on_windows(360, func_only=True) +@pytest.mark.timeout_unless_on_windows(360) def test_shell_inject_ssh_priv( client, salt_ssh_roster_file, rosters_dir, tmp_path, salt_auto_account, grains ): diff --git a/tests/pytests/integration/states/test_beacon.py b/tests/pytests/integration/states/test_beacon.py index d84d962f18f..5d6737e6a18 100644 --- a/tests/pytests/integration/states/test_beacon.py +++ b/tests/pytests/integration/states/test_beacon.py @@ -9,7 +9,7 @@ log = logging.getLogger(__name__) @pytest.mark.slow_test -@pytest.mark.timeout_unless_on_windows(120) +@pytest.mark.timeout_unless_on_windows(240) def test_present_absent(salt_master, salt_minion, salt_call_cli): ret = salt_call_cli.run("beacons.reset") From a974d5d7db907b4573f43de375c7f876cf99d014 Mon Sep 17 00:00:00 2001 From: Cian Yong Leow Date: Tue, 6 Feb 2024 17:47:48 +0000 Subject: [PATCH 3/5] Fix exceptions being set on completed futures Fixes an issue in the ZeroMQ transport where caught exceptions were being set on futures that had already completed (i.e. done) and were logging lots of error messages. --- changelog/66006.fixed.md | 1 + salt/transport/zeromq.py | 6 ++-- tests/pytests/unit/transport/test_zeromq.py | 36 ++++++++++++++++++++- 3 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 changelog/66006.fixed.md diff --git a/changelog/66006.fixed.md b/changelog/66006.fixed.md new file mode 100644 index 00000000000..ca7906d76f7 --- /dev/null +++ b/changelog/66006.fixed.md @@ -0,0 +1 @@ +Fix exceptions being set on futures that are already done in ZeroMQ transport \ No newline at end of file diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index 5b006b88592..96f12a0d6f7 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -607,14 +607,16 @@ class AsyncReqMessageClient: try: recv = yield self.socket.recv() except zmq.eventloop.future.CancelledError as exc: - future.set_exception(exc) + if not future.done(): + future.set_exception(exc) return if not future.done(): data = salt.payload.loads(recv) future.set_result(data) except Exception as exc: # pylint: disable=broad-except - future.set_exception(exc) + if not future.done(): + future.set_exception(exc) class ZeroMQSocketMonitor: diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py index c5d5242319f..97d65a204ad 100644 --- a/tests/pytests/unit/transport/test_zeromq.py +++ b/tests/pytests/unit/transport/test_zeromq.py @@ -13,6 +13,7 @@ import uuid import msgpack import pytest +import zmq.eventloop.future import salt.channel.client import salt.channel.server @@ -27,7 +28,7 @@ import salt.utils.platform import salt.utils.process import salt.utils.stringutils from salt.master import SMaster -from tests.support.mock import MagicMock, patch +from tests.support.mock import AsyncMock, MagicMock, patch try: from M2Crypto import RSA @@ -1489,6 +1490,39 @@ async def test_client_timeout_msg(minion_opts): client.close() +async def test_client_send_recv_on_cancelled_error(minion_opts): + client = salt.transport.zeromq.AsyncReqMessageClient( + minion_opts, "tcp://127.0.0.1:4506" + ) + + mock_future = MagicMock(**{"done.return_value": True}) + + try: + client.socket = AsyncMock() + client.socket.recv.side_effect = zmq.eventloop.future.CancelledError + await client._send_recv({"meh": "bah"}, mock_future) + + mock_future.set_exception.assert_not_called() + finally: + client.close() + + +async def test_client_send_recv_on_exception(minion_opts): + client = salt.transport.zeromq.AsyncReqMessageClient( + minion_opts, "tcp://127.0.0.1:4506" + ) + + mock_future = MagicMock(**{"done.return_value": True}) + + try: + client.socket = None + await client._send_recv({"meh": "bah"}, mock_future) + + mock_future.set_exception.assert_not_called() + finally: + client.close() + + def test_pub_client_init(minion_opts, io_loop): minion_opts["id"] = "minion" minion_opts["__role"] = "syndic" From ed67c89403c14e5b9799a49a0543e2f0fe2950e8 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 15 Feb 2024 14:53:19 -0700 Subject: [PATCH 4/5] Fix pre-commit --- changelog/66006.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/66006.fixed.md b/changelog/66006.fixed.md index ca7906d76f7..aa05c9c19af 100644 --- a/changelog/66006.fixed.md +++ b/changelog/66006.fixed.md @@ -1 +1 @@ -Fix exceptions being set on futures that are already done in ZeroMQ transport \ No newline at end of file +Fix exceptions being set on futures that are already done in ZeroMQ transport From c3826c61433ab784d7387eb7f1ed2262599dcb3e Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Sat, 17 Feb 2024 11:46:46 +0000 Subject: [PATCH 5/5] Increase timeouts on problematic test --- .../integration/daemons/test_memory_leak.py | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/tests/pytests/integration/daemons/test_memory_leak.py b/tests/pytests/integration/daemons/test_memory_leak.py index a81c69c150b..d61bb85b736 100644 --- a/tests/pytests/integration/daemons/test_memory_leak.py +++ b/tests/pytests/integration/daemons/test_memory_leak.py @@ -6,19 +6,15 @@ import pytest pytestmark = [ pytest.mark.slow_test, - pytest.mark.timeout_unless_on_windows(240), + pytest.mark.timeout_unless_on_windows(360), ] @pytest.fixture -def testfile_path(tmp_path): - return tmp_path / "testfile" - - -@pytest.fixture -def file_add_delete_sls(testfile_path, base_env_state_tree_root_dir): +def file_add_delete_sls(tmp_path, salt_master): + path = tmp_path / "testfile" sls_name = "file_add" - sls_contents = """ + sls_contents = f""" add_file: file.managed: - name: {path} @@ -36,16 +32,13 @@ def file_add_delete_sls(testfile_path, base_env_state_tree_root_dir): echo: cmd.run: - name: \"echo 'This is a test!'\" - """.format( - path=testfile_path - ) - with pytest.helpers.temp_file( - "{}.sls".format(sls_name), sls_contents, base_env_state_tree_root_dir - ): + """ + with salt_master.state_tree.base.temp_file(f"{sls_name}.sls", sls_contents): yield sls_name @pytest.mark.skip_on_fips_enabled_platform +@pytest.mark.skip_on_windows(reason="Windows is a spawning platform, won't work") @pytest.mark.skip_on_darwin(reason="MacOS is a spawning platform, won't work") @pytest.mark.flaky(max_runs=4) def test_memory_leak(salt_cli, salt_minion, file_add_delete_sls):