diff --git a/changelog/70144.fixed.md b/changelog/70144.fixed.md new file mode 100644 index 000000000000..e9a080c54e65 --- /dev/null +++ b/changelog/70144.fixed.md @@ -0,0 +1 @@ +Gave each deltaproxy sub-proxy its own loader namespace. Sub-proxies previously shared one module namespace, so the loader handed them the same execution-module objects and whichever sub-proxy packed a module last owned its ``__opts__`` for the life of the process. On a proxy running with ``multiprocessing: False``, targeting two or more sub-proxies in a single job left every later SLS render resolving grains, pillar, ``cachedir`` and ``id`` from the wrong sub-proxy until the deltaproxy was restarted. diff --git a/salt/metaproxy/deltaproxy.py b/salt/metaproxy/deltaproxy.py index cb2b8e2c9a09..53b12391aab0 100644 --- a/salt/metaproxy/deltaproxy.py +++ b/salt/metaproxy/deltaproxy.py @@ -23,6 +23,7 @@ import salt.defaults.exitcodes import salt.engines import salt.loader +import salt.loader.lazy import salt.minion import salt.payload import salt.pillar @@ -462,7 +463,15 @@ async def subproxy_post_master_init(minion_id, uid, opts, main_proxy, main_utils } ) - _proxy_minion = ProxyMinion(proxyopts) + # Give every sub-proxy its own loader namespace. Without this, all + # sub-proxies in a deltaproxy share one module namespace, so the loader + # hands them the *same* module objects and whichever sub-proxy packs a + # module last owns its ``__opts__`` for the life of the process. See + # #70144. + _proxy_minion = ProxyMinion( + proxyopts, + loaded_base_name=f"{minion_id}.{salt.loader.lazy.LOADED_BASE_NAME}", + ) _proxy_minion.proc_dir = salt.minion.get_proc_dir(proxyopts["cachedir"], uid=uid) # And load the modules diff --git a/salt/minion.py b/salt/minion.py index 8292806508a9..315eaf939017 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -2212,6 +2212,7 @@ def _load_modules( utils=self.utils, notify=notify, proxy=proxy, + loaded_base_name=self.loaded_base_name, context=context, ) returners = salt.loader.returners(opts, functions, proxy=proxy, context=context) diff --git a/tests/pytests/unit/metaproxy/test_deltaproxy.py b/tests/pytests/unit/metaproxy/test_deltaproxy.py index ded184c23daf..e5c11e8c5353 100644 --- a/tests/pytests/unit/metaproxy/test_deltaproxy.py +++ b/tests/pytests/unit/metaproxy/test_deltaproxy.py @@ -14,7 +14,9 @@ import tornado.gen import tornado.ioloop +import salt.loader.lazy import salt.metaproxy.deltaproxy as deltaproxy +import salt.minion from tests.support.mock import MagicMock, patch log = logging.getLogger(__name__) @@ -133,8 +135,9 @@ def _fake_load_modules(self, opts=None, grains=None, context=None, **kwargs): return functions, returners, {}, executors class _FakeProxyMinion: - def __init__(self, opts): + def __init__(self, opts, loaded_base_name=None): self.opts = opts + self.loaded_base_name = loaded_base_name self.subprocess_list = MagicMock() self.connected = False @@ -229,3 +232,110 @@ def test_subproxy_post_master_init_packs_per_minion_grains( # control proxy stores the right grains in ``self.deltaproxy_opts``. assert result1["proxy_opts"]["grains"]["serial_number"] == "SN-AAA-001" assert result2["proxy_opts"]["grains"]["serial_number"] == "SN-BBB-002" + + +def test_subproxy_post_master_init_gives_each_subproxy_its_own_loader_namespace( + proxy_opts, fake_main_proxy, fake_main_utils +): + """ + Regression test for #70144. + + Every sub-proxy must be built with its own ``loaded_base_name`` so the + loader gives it distinct module objects. Sharing one namespace makes the + loader hand every sub-proxy the *same* execution-module objects, so + whichever sub-proxy packs a module last owns that module's ``__opts__`` + for the life of the process. On a proxy running ``multiprocessing: + False`` that made every later SLS render resolve grains/pillar/``id`` + from the wrong sub-proxy until the deltaproxy was restarted. + """ + per_minion_grains = { + "minion1": {"serial_number": "SN-AAA-001", "id": "minion1"}, + "minion2": {"serial_number": "SN-BBB-002", "id": "minion2"}, + } + p = _make_subproxy_patches(per_minion_grains) + + loop = tornado.ioloop.IOLoop() + with patch.object( + deltaproxy.salt.config, "proxy_config", p["proxy_config"] + ), patch.object( + deltaproxy.salt.pillar, "get_async_pillar", p["get_pillar"] + ), patch.object( + deltaproxy.salt.loader, "grains", p["grains"] + ), patch.object( + deltaproxy.salt.loader, "proxy", p["proxy_loader"] + ), patch.object( + deltaproxy.salt.loader, "utils", p["utils_loader"] + ), patch.object( + deltaproxy, "ProxyMinion", p["proxy_minion_cls"] + ), patch.object( + deltaproxy.salt.minion, "get_proc_dir", p["get_proc_dir"] + ), patch.object( + deltaproxy.salt.utils.schedule, "Schedule", p["schedule"] + ): + try: + result1 = loop.run_sync( + lambda: deltaproxy.subproxy_post_master_init( + "minion1", 0, proxy_opts, fake_main_proxy, fake_main_utils + ) + ) + result2 = loop.run_sync( + lambda: deltaproxy.subproxy_post_master_init( + "minion2", 0, proxy_opts, fake_main_proxy, fake_main_utils + ) + ) + finally: + loop.close() + + sub1 = result1["proxy_minion"] + sub2 = result2["proxy_minion"] + + # Each sub-proxy is namespaced by its own minion id. + assert sub1.loaded_base_name == f"minion1.{salt.loader.lazy.LOADED_BASE_NAME}" + assert sub2.loaded_base_name == f"minion2.{salt.loader.lazy.LOADED_BASE_NAME}" + + # Inverse must-not: the two sub-proxies must never share a namespace, and + # neither may fall back to the global default that caused #70144. + assert sub1.loaded_base_name != sub2.loaded_base_name + assert sub1.loaded_base_name is not None + assert sub2.loaded_base_name is not None + assert salt.loader.lazy.LOADED_BASE_NAME not in ( + sub1.loaded_base_name.split(".")[0], + sub2.loaded_base_name.split(".")[0], + ) + + +def test_load_modules_forwards_loaded_base_name_to_minion_mods(minion_opts): + """ + Regression test for #70144. + + ``subproxy_post_master_init`` handing each sub-proxy a ``loaded_base_name`` + only isolates them if ``_load_modules`` actually forwards it to + ``salt.loader.minion_mods``. The non-multimaster branch used to drop it, + which left every sub-proxy back in the shared default namespace. + """ + minion_opts["grains"] = {} + minion = salt.minion.Minion( + minion_opts, + loaded_base_name="sub1.salt.loaded", + io_loop=tornado.ioloop.IOLoop(), + ) + try: + with patch.object( + salt.loader, "minion_mods", return_value={} + ) as minion_mods_mock, patch.object( + salt.loader, "returners", return_value={} + ), patch.object( + salt.loader, "executors", return_value={} + ), patch.object( + salt.loader, "utils", return_value={} + ), patch.object( + salt.loader, "grains", return_value={} + ): + minion._load_modules(grains={}) + + assert minion_mods_mock.called + assert ( + minion_mods_mock.call_args.kwargs["loaded_base_name"] == "sub1.salt.loaded" + ) + finally: + minion.destroy()