diff --git a/changelog/69724.fixed.md b/changelog/69724.fixed.md new file mode 100644 index 000000000000..ea56ced3d3a2 --- /dev/null +++ b/changelog/69724.fixed.md @@ -0,0 +1 @@ +Fixed the intermittent ``duplicate HTTP post method definition`` failure in the -W parallel docs builds (Prepare Release and Documentation jobs) by marking the HTTP routes documented on the rest_tornado and rest_wsgi pages with ``:noindex:``, leaving rest_cherrypy as the single indexed instance of each shared route. diff --git a/changelog/69728.fixed.md b/changelog/69728.fixed.md new file mode 100644 index 000000000000..ca9a2e1b7240 --- /dev/null +++ b/changelog/69728.fixed.md @@ -0,0 +1 @@ +Fixed the Rocky Linux 9 integration tcp/zeromq CI jobs failing most PR runs: the startup_states and salt_call ownership test fixtures left their extra minions' accepted keys on the shared session master after stopping the minions, so later netapi tests targeting ``*`` matched dead minions (wrong minion lists and 30 second timeouts). The fixtures now delete their minion keys at teardown. diff --git a/changelog/69730.fixed.md b/changelog/69730.fixed.md new file mode 100644 index 000000000000..f07f2f6bc20a --- /dev/null +++ b/changelog/69730.fixed.md @@ -0,0 +1 @@ +Fixed the master logging ``Event iteration failed with exception: 'list' object has no attribute 'items'`` for every failing state compilation: the return of a failed compile is a list of error strings, not a mapping of state results, and the event tagger assumed a dict. diff --git a/changelog/69738.fixed.md b/changelog/69738.fixed.md new file mode 100644 index 000000000000..17df8786228d --- /dev/null +++ b/changelog/69738.fixed.md @@ -0,0 +1 @@ +Fixed the flaky ssh test_renderer_file: salt-ssh slsutil.renderer does not ship a rendered file's jinja imports (map.jinja) to the target, so the renderer tests only passed when an earlier state test had warmed the salt-ssh file cache. Prime the cache in the fixture so they are deterministic. diff --git a/doc/_ext/salthttpanchors.py b/doc/_ext/salthttpanchors.py new file mode 100644 index 000000000000..5822384a2a43 --- /dev/null +++ b/doc/_ext/salthttpanchors.py @@ -0,0 +1,45 @@ +""" +Keep HTML anchors on ``:noindex:``'d httpdomain directives. + +sphinxcontrib-httpdomain's ``add_target_and_index`` intentionally splits its +two jobs: it always appends the ``#--`` anchor to the signature +node, and only gates the global route *registration* behind ``:noindex:``. +Sphinx's ``ObjectDescription.run`` however skips the whole method when +``noindex`` is set, so the anchor (and its permalink) is lost along with the +index entry. Anchors are per-page HTML ids and cannot collide across pages, +so restoring them is safe; only the global registration can produce the +parallel-build duplicate-route warnings. + +Hide the option from Sphinx's outer gate and re-present it to httpdomain's +inner gate, so ``:noindex:`` means what httpdomain meant it to mean: no index +entry, anchor kept. +""" + +from sphinxcontrib.httpdomain import HTTPDomain + + +def _make_anchored(cls): + class AnchoredHTTPResource(cls): + def run(self): + self._salt_noindex = "noindex" in self.options + self.options.pop("noindex", None) + return super().run() + + def add_target_and_index(self, name_cls, sig, signode): + if self._salt_noindex: + self.options["noindex"] = None + try: + super().add_target_and_index(name_cls, sig, signode) + finally: + if self._salt_noindex: + self.options.pop("noindex", None) + + AnchoredHTTPResource.__name__ = f"Anchored{cls.__name__}" + return AnchoredHTTPResource + + +def setup(app): + app.setup_extension("sphinxcontrib.httpdomain") + for name, cls in list(HTTPDomain.directives.items()): + app.add_directive_to_domain("http", name, _make_anchored(cls), override=True) + return {"parallel_read_safe": True, "parallel_write_safe": True} diff --git a/doc/conf.py b/doc/conf.py index 64d38b0e7106..0450729512cc 100644 --- a/doc/conf.py +++ b/doc/conf.py @@ -176,6 +176,7 @@ def _safe_urlsplit(url, scheme="", allow_fragments=True): "sphinx.ext.imgconverter", "sphinx.ext.intersphinx", "sphinxcontrib.httpdomain", + "salthttpanchors", "saltrepo", "myst_parser", #'saltautodoc', # Must be AFTER autodoc diff --git a/salt/netapi/rest_tornado/saltnado.py b/salt/netapi/rest_tornado/saltnado.py index 99971b778b9d..079662ead805 100644 --- a/salt/netapi/rest_tornado/saltnado.py +++ b/salt/netapi/rest_tornado/saltnado.py @@ -633,6 +633,7 @@ def get(self): # pylint: disable=arguments-differ All logins are done over post, this is a parked endpoint .. http:get:: /login + :noindex: :status 401: |401| :status 406: |406| @@ -672,6 +673,7 @@ def post(self): # pylint: disable=arguments-differ :ref:`Authenticate ` against Salt's eauth system .. http:post:: /login + :noindex: :reqheader X-Auth-Token: |req_token| :reqheader Accept: |req_accept| @@ -803,6 +805,7 @@ def get(self): # pylint: disable=arguments-differ An endpoint to determine salt-api capabilities .. http:get:: / + :noindex: :reqheader Accept: |req_accept| @@ -841,6 +844,7 @@ def post(self): # pylint: disable=arguments-differ Send one or more Salt commands (lowstates) in the request body .. http:post:: / + :noindex: :reqheader X-Auth-Token: |req_token| :reqheader Accept: |req_accept| @@ -1219,6 +1223,7 @@ def get(self, mid=None): # pylint: disable=W0221 details .. http:get:: /minions/(mid) + :noindex: :reqheader X-Auth-Token: |req_token| :reqheader Accept: |req_accept| @@ -1266,6 +1271,7 @@ def post(self): Start an execution command and immediately return the job id .. http:post:: /minions + :noindex: :reqheader X-Auth-Token: |req_token| :reqheader Accept: |req_accept| @@ -1345,6 +1351,7 @@ def get(self, jid=None): # pylint: disable=W0221 the return from a single job .. http:get:: /jobs/(jid) + :noindex: List jobs or show a single job from the job cache. @@ -1445,6 +1452,7 @@ def post(self): ` .. http:post:: /run + :noindex: This entry point is primarily for "one-off" commands. Each request must pass full Salt authentication credentials. Otherwise this URL @@ -1519,6 +1527,7 @@ def get(self): event is formatted as JSON. .. http:get:: /events + :noindex: :status 200: |200| :status 401: |401| @@ -1678,6 +1687,7 @@ def post(self, tag_suffix=None): # pylint: disable=W0221 Fire an event in Salt with a custom event tag and data .. http:post:: /hook + :noindex: :status 200: |200| :status 401: |401| diff --git a/salt/netapi/rest_wsgi.py b/salt/netapi/rest_wsgi.py index 50dfabe23c1e..1f448be4447d 100644 --- a/salt/netapi/rest_wsgi.py +++ b/salt/netapi/rest_wsgi.py @@ -65,6 +65,7 @@ ============== .. http:post:: / + :noindex: **Example request** for a basic ``test.ping``:: diff --git a/salt/utils/event.py b/salt/utils/event.py index 6cfecd9e4e84..93a3d0518ea0 100644 --- a/salt/utils/event.py +++ b/salt/utils/event.py @@ -937,6 +937,18 @@ def _fire_ret_load_specific_fun(self, load, fun_index=0): ret = load.get("return", {}) retcode = load["retcode"] + if not isinstance(ret, dict): + # A failing state compilation returns a list of error strings (or + # a plain string from some renderers) instead of a mapping of + # per-state results, so there are no state tags to fire sub + # events for. + log.debug( + "Skipping sub event for job %s: return is a %s, not a dict", + load.get("jid"), + type(ret).__name__, + ) + return + try: for tag, data in ret.items(): data["retcode"] = retcode diff --git a/tests/pytests/integration/cli/test_salt_call_ownership.py b/tests/pytests/integration/cli/test_salt_call_ownership.py index 175931da6079..8fe529328b80 100644 --- a/tests/pytests/integration/cli/test_salt_call_ownership.py +++ b/tests/pytests/integration/cli/test_salt_call_ownership.py @@ -64,6 +64,10 @@ def non_root_minion(salt_master, salt_factories): ) with factory.started(): yield factory + # The minion process is stopped at this point, but its accepted key stays + # on the shared session master, where later tests that target '*' (the + # netapi integration tests) would match it as a dead minion. Remove it. + salt_master.salt_key_cli().run("-d", factory.id, "-y") @pytest.mark.skipif(shutil.which("sudo") is None, reason="sudo is not available") diff --git a/tests/pytests/integration/minion/test_startup_states.py b/tests/pytests/integration/minion/test_startup_states.py index d8b891b35c42..f76760b0ba21 100644 --- a/tests/pytests/integration/minion/test_startup_states.py +++ b/tests/pytests/integration/minion/test_startup_states.py @@ -29,6 +29,10 @@ def salt_minion_startup_states_empty_string(salt_master, salt_minion_id): ) with factory.started(): yield factory + # The minion process is stopped at this point, but its accepted key stays + # on the shared session master, where later tests that target '*' (the + # netapi integration tests) would match it as a dead minion. Remove it. + salt_master.salt_key_cli().run("-d", factory.id, "-y") @pytest.fixture @@ -43,6 +47,10 @@ def salt_minion_startup_states_highstate(salt_master, salt_minion_id): ) with factory.started(): yield factory + # The minion process is stopped at this point, but its accepted key stays + # on the shared session master, where later tests that target '*' (the + # netapi integration tests) would match it as a dead minion. Remove it. + salt_master.salt_key_cli().run("-d", factory.id, "-y") @pytest.fixture @@ -58,6 +66,10 @@ def salt_minion_startup_states_sls(salt_master, salt_minion_id): ) with factory.started(): yield factory + # The minion process is stopped at this point, but its accepted key stays + # on the shared session master, where later tests that target '*' (the + # netapi integration tests) would match it as a dead minion. Remove it. + salt_master.salt_key_cli().run("-d", factory.id, "-y") @pytest.fixture @@ -73,6 +85,10 @@ def salt_minion_startup_states_top(salt_master, salt_minion_id): ) with factory.started(): yield factory + # The minion process is stopped at this point, but its accepted key stays + # on the shared session master, where later tests that target '*' (the + # netapi integration tests) would match it as a dead minion. Remove it. + salt_master.salt_key_cli().run("-d", factory.id, "-y") def test_startup_states_empty_string( diff --git a/tests/pytests/integration/ssh/conftest.py b/tests/pytests/integration/ssh/conftest.py index c451a4850e58..b123d9de473c 100644 --- a/tests/pytests/integration/ssh/conftest.py +++ b/tests/pytests/integration/ssh/conftest.py @@ -66,7 +66,7 @@ def _reap_stray_processes(): @pytest.fixture(scope="module") -def state_tree(base_env_state_tree_root_dir): +def state_tree(base_env_state_tree_root_dir, salt_ssh_cli): # Remove unused import from top file to avoid salt-ssh file sync issues # Note: top file references "basic" but we create "test.sls" - this appears # intentional as tests run state.sls directly and don't use the top file @@ -96,6 +96,12 @@ def state_tree(base_env_state_tree_root_dir): "test.sls", state_file, base_env_state_tree_root_dir ) with top_tempfile, map_tempfile, state_tempfile: + # slsutil.renderer over salt-ssh fetches the requested file but does + # not ship its jinja-imported files (map.jinja) to the target; only a + # state run syncs the full state tree to the target's file cache. + # Prime that cache once so the renderer tests are deterministic + # instead of depending on an earlier state test having warmed it. + salt_ssh_cli.run("state.apply", "test", test=True) yield diff --git a/tests/pytests/unit/utils/event/test_event.py b/tests/pytests/unit/utils/event/test_event.py index f4b6c1599966..3b3c2944bde6 100644 --- a/tests/pytests/unit/utils/event/test_event.py +++ b/tests/pytests/unit/utils/event/test_event.py @@ -1,4 +1,5 @@ import hashlib +import logging import os import stat import time @@ -340,3 +341,71 @@ def test_master_pub_permissions(sock_dir): assert bool(os.lstat(p).st_mode & stat.S_IRUSR) assert not bool(os.lstat(p).st_mode & stat.S_IRGRP) assert not bool(os.lstat(p).st_mode & stat.S_IROTH) + + +@pytest.fixture +def ret_load_event(sock_dir): + with salt.utils.event.SaltEvent( + "master", str(sock_dir), opts={"transport": "zeromq"}, listen=False + ) as event: + with patch.object(event, "fire_event") as fire_event: + yield event, fire_event + + +def test_fire_ret_load_list_return_skips_quietly_69730(ret_load_event, caplog): + """ + A failing state compilation returns a list of error strings rather than + a mapping of per-state results. fire_ret_load used to hand that list to + _fire_ret_load_specific_fun, which crashed on ret.items() and logged + "Event iteration failed with exception: 'list' object has no attribute + 'items'" at ERROR for every failed compile. There are no state tags in + such a return, so it must be skipped without logging an error and + without firing sub events. + """ + event, fire_event = ret_load_event + # The exact shape the master receives for a failed state.apply compile: + # fun in SUB_EVENT, a non-zero retcode, and a list-of-errors return. + load = { + "id": "minion", + "jid": "20260706000000000000", + "fun": "state.sls", + "retcode": 1, + "return": ["Rendering SLS 'base:broken' failed: Jinja error"], + } + with caplog.at_level(logging.ERROR, logger="salt.utils.event"): + event.fire_ret_load(load) + assert "Event iteration failed" not in caplog.text + fire_event.assert_not_called() + + +def test_fire_ret_load_dict_return_still_fires_sub_events_69730(ret_load_event, caplog): + """ + Guard against overcorrection: a dict-shaped failing state return (the + normal case) must keep firing the per-tag failure events exactly as + before the non-dict guard was added. This passes with and without the + fix. + """ + event, fire_event = ret_load_event + tag = "file_|-broken_|-/etc/broken_|-managed" + load = { + "id": "minion", + "jid": "20260706000000000000", + "fun": "state.sls", + "retcode": 2, + "return": {tag: {"result": False, "comment": "no such file"}}, + } + with caplog.at_level(logging.ERROR, logger="salt.utils.event"): + event.fire_ret_load(load) + assert "Event iteration failed" not in caplog.text + assert fire_event.call_count == 2 + # old-style duplicate event: . tag + first_data, first_tag = fire_event.call_args_list[0][0] + assert first_tag == "file.managed" + assert first_data["retcode"] == 2 + # namespaced job sub event, enriched with job metadata + second_data, second_tag = fire_event.call_args_list[1][0] + assert second_tag == "salt/job/20260706000000000000/sub/minion/error/state.sls" + assert second_data["jid"] == "20260706000000000000" + assert second_data["id"] == "minion" + assert second_data["success"] is False + assert second_data["fun"] == "state.sls"