Give each deltaproxy sub-proxy its own loader namespace (#70144)

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.

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

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)

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()