diff --git a/changelog/67880.fixed.md b/changelog/67880.fixed.md new file mode 100644 index 000000000000..919724798009 --- /dev/null +++ b/changelog/67880.fixed.md @@ -0,0 +1,2 @@ +Fixes incorrect storage of `_return`-type job results in local_cache when using `multi_returner` configuration. Now strictly follows the logic in `salt.utils.job.store_job` by only executing multi-returner storage when `load['cmd']` is not equal to `'_return'`. +Implemented missing functions in multi_returner: `save_minions`, `get_jids_filter`, `update_endtime`, `save_reg`, and `load_reg`. diff --git a/salt/returners/multi_returner.py b/salt/returners/multi_returner.py index 0017f6e7a2fc..b0e877d0f4b2 100644 --- a/salt/returners/multi_returner.py +++ b/salt/returners/multi_returner.py @@ -4,6 +4,7 @@ """ import logging +from typing import TYPE_CHECKING, Any import salt.minion @@ -14,12 +15,15 @@ # cache of the master mininon for this returner MMINION = None +if TYPE_CHECKING: + __opts__: dict[str, Any] + def _mminion(): """ Create a single mminion for this module to use, instead of reloading all the time """ - global MMINION + global MMINION # pylint: disable=global-statement if MMINION is None: MMINION = salt.minion.MasterMinion(__opts__) @@ -62,13 +66,21 @@ def save_load(jid, clear_load, minions=None): Write load to all returners in multi_returner """ for returner_ in __opts__[CONFIG_KEY]: - _mminion().returners[f"{returner_}.save_load"](jid, clear_load) + if returner_ == "local_cache": + cmd = clear_load.get("cmd") + if cmd == "_return": + continue + _mminion().returners[f"{returner_}.save_load"](jid, clear_load, minions) def save_minions(jid, minions, syndic_id=None): # pylint: disable=unused-argument """ Included for API consistency """ + for returner_ in __opts__[CONFIG_KEY]: + fstr = f"{returner_}.save_minions" + if fstr in _mminion().returners: + _mminion().returners[fstr](jid, minions, syndic_id) def get_load(jid): @@ -104,6 +116,20 @@ def get_jids(): return ret +def get_jids_filter(count, filter_find_job=True): + """ + Return a list of all jobs information filtered by the given criteria. + :param int count: show not more than the count of most recent jobs + :param bool filter_find_jobs: filter out 'saltutil.find_job' jobs + """ + ret = {} + for returner_ in __opts__[CONFIG_KEY]: + fstr = f"{returner_}.get_jids_filter" + if fstr in _mminion().returners: + ret.update(_mminion().returners[fstr](count, filter_find_job)) + return ret + + def clean_old_jobs(): """ Clean out the old jobs from all returners (if you have it) @@ -112,3 +138,38 @@ def clean_old_jobs(): fstr = f"{returner_}.clean_old_jobs" if fstr in _mminion().returners: _mminion().returners[fstr]() + + +def update_endtime(jid, time): + """ + Update (or store) the end time for a given job + + Endtime is stored as a plain text string + """ + for returner_ in __opts__[CONFIG_KEY]: + fstr = f"{returner_}.update_endtime" + if fstr in _mminion().returners: + _mminion().returners[fstr](jid, time) + + +def save_reg(data): + """ + Save the register to msgpack files + + Writes to every configured returner that implements ``save_reg`` so the + multi_returner write-to-all contract is preserved. + """ + for returner_ in __opts__[CONFIG_KEY]: + fstr = f"{returner_}.save_reg" + if fstr in _mminion().returners: + _mminion().returners[fstr](data) + + +def load_reg(): + """ + Load the register from msgpack files + """ + for returner_ in __opts__[CONFIG_KEY]: + fstr = f"{returner_}.load_reg" + if fstr in _mminion().returners: + return _mminion().returners[fstr]() diff --git a/tests/pytests/unit/returners/test_multi_returner.py b/tests/pytests/unit/returners/test_multi_returner.py new file mode 100644 index 000000000000..3e536ed5343d --- /dev/null +++ b/tests/pytests/unit/returners/test_multi_returner.py @@ -0,0 +1,199 @@ +""" +Unit tests for the multi_returner returner. + +Regression tests for the fixes in PR #67880: + +* ``save_load`` must skip ``local_cache`` when ``clear_load['cmd'] == '_return'`` + to mirror the dedup performed by ``salt.utils.job.store_job`` and stop the + minion's ``_return`` payload from overwriting the dispatch-time load that + the master already saved. +* ``save_load`` must forward ``minions`` to each configured sub-returner; + previously the argument was dropped on the way through. +* ``get_jids_filter`` must dispatch under the canonical plural name (the + runner in ``salt/runners/jobs.py`` looks up ``.get_jids_filter``) + and merge results from every configured returner that implements it. +* ``save_reg`` must write to *every* configured returner that implements it, + preserving the multi_returner write-to-all contract. +""" + +import pytest + +import salt.returners.multi_returner as multi_returner +from tests.support.mock import MagicMock, patch + + +@pytest.fixture +def configure_loader_modules(): + return { + multi_returner: { + "__opts__": {multi_returner.CONFIG_KEY: ["local_cache", "fake_returner"]} + } + } + + +@pytest.fixture(autouse=True) +def _reset_mminion_cache(): + """Each test gets a fresh module-level MMINION.""" + with patch.object(multi_returner, "MMINION", None): + yield + + +def _fake_mminion(returners): + """Build a fake mminion whose .returners dict behaves like Salt's loader.""" + fake = MagicMock() + fake.returners = returners + return fake + + +def test_save_load_skips_local_cache_for_return_cmd(): + """ + PR #67880: when the load is the minion's ``_return`` payload, the + dispatch-time load that the master already wrote to local_cache must + not be overwritten. + """ + local_cache_save_load = MagicMock() + fake_save_load = MagicMock() + fake = _fake_mminion( + { + "local_cache.save_load": local_cache_save_load, + "fake_returner.save_load": fake_save_load, + } + ) + with patch.object(multi_returner, "_mminion", return_value=fake): + multi_returner.save_load( + "20260608000000000001", + {"cmd": "_return", "jid": "20260608000000000001", "return": "result"}, + ) + + local_cache_save_load.assert_not_called() + fake_save_load.assert_called_once() + + +def test_save_load_dispatches_for_non_return_cmd(): + """ + Non-``_return`` loads (publish, runner, etc.) must still reach every + configured returner, including local_cache. + """ + local_cache_save_load = MagicMock() + fake_save_load = MagicMock() + fake = _fake_mminion( + { + "local_cache.save_load": local_cache_save_load, + "fake_returner.save_load": fake_save_load, + } + ) + with patch.object(multi_returner, "_mminion", return_value=fake): + multi_returner.save_load( + "20260608000000000002", + {"cmd": "publish", "jid": "20260608000000000002", "tgt": "*"}, + minions=["minion-1", "minion-2"], + ) + + local_cache_save_load.assert_called_once_with( + "20260608000000000002", + {"cmd": "publish", "jid": "20260608000000000002", "tgt": "*"}, + ["minion-1", "minion-2"], + ) + fake_save_load.assert_called_once_with( + "20260608000000000002", + {"cmd": "publish", "jid": "20260608000000000002", "tgt": "*"}, + ["minion-1", "minion-2"], + ) + + +def test_save_load_forwards_minions_argument(): + """ + Prior to PR #67880, ``save_load`` dropped its ``minions`` kwarg before + dispatching — every sub-returner saw ``minions=None`` regardless of + what the master passed. This regression-pins the forwarding. + """ + fake_save_load = MagicMock() + fake = _fake_mminion({"fake_returner.save_load": fake_save_load}) + with patch.dict( + multi_returner.__opts__, {multi_returner.CONFIG_KEY: ["fake_returner"]} + ): + with patch.object(multi_returner, "_mminion", return_value=fake): + multi_returner.save_load( + "20260608000000000003", + {"cmd": "publish", "jid": "20260608000000000003"}, + minions=["minion-a"], + ) + + fake_save_load.assert_called_once_with( + "20260608000000000003", + {"cmd": "publish", "jid": "20260608000000000003"}, + ["minion-a"], + ) + + +def test_get_jids_filter_dispatches_under_canonical_plural_name(): + """ + PR #67880: ``get_jids_filter`` (plural) is the canonical Salt name — + ``salt/runners/jobs.py`` looks up ``.get_jids_filter`` and + ``salt/returners/local_cache.py`` implements ``get_jids_filter``. + The dispatcher and its ``fstr`` lookup must both use the plural form, + otherwise the function is unreachable. + """ + local_cache_filter = MagicMock(return_value={"jid-1": {"Function": "test.ping"}}) + fake_filter = MagicMock(return_value={"jid-2": {"Function": "cmd.run"}}) + fake = _fake_mminion( + { + "local_cache.get_jids_filter": local_cache_filter, + "fake_returner.get_jids_filter": fake_filter, + } + ) + with patch.object(multi_returner, "_mminion", return_value=fake): + ret = multi_returner.get_jids_filter(5, filter_find_job=False) + + local_cache_filter.assert_called_once_with(5, False) + fake_filter.assert_called_once_with(5, False) + assert ret == { + "jid-1": {"Function": "test.ping"}, + "jid-2": {"Function": "cmd.run"}, + } + + +def test_get_jids_filter_attribute_exists_under_plural_name(): + """ + The module-level public name is what the loader exposes. A typo in the + function name (singular ``get_jid_filter``) would break the runner + integration even if the body were correct. + """ + assert hasattr(multi_returner, "get_jids_filter") + assert not hasattr(multi_returner, "get_jid_filter") + + +def test_save_reg_writes_to_every_configured_returner(): + """ + PR #67880: previously ``save_reg`` ``return``-ed after the first match + so any returner past index 0 was silently skipped. This pins the + write-to-all contract that the rest of the module follows. + """ + local_cache_save_reg = MagicMock() + fake_save_reg = MagicMock() + fake = _fake_mminion( + { + "local_cache.save_reg": local_cache_save_reg, + "fake_returner.save_reg": fake_save_reg, + } + ) + payload = {"event": "thorium.fire"} + with patch.object(multi_returner, "_mminion", return_value=fake): + multi_returner.save_reg(payload) + + local_cache_save_reg.assert_called_once_with(payload) + fake_save_reg.assert_called_once_with(payload) + + +def test_save_reg_skips_returners_without_save_reg(): + """ + Returners that don't implement ``save_reg`` are skipped — the + ``fstr in _mminion().returners`` guard must remain in place. + """ + local_cache_save_reg = MagicMock() + fake = _fake_mminion({"local_cache.save_reg": local_cache_save_reg}) + # fake_returner has NO save_reg entry + with patch.object(multi_returner, "_mminion", return_value=fake): + multi_returner.save_reg({"event": "thorium.fire"}) + + local_cache_save_reg.assert_called_once()