diff --git a/changelog/44937.fixed.md b/changelog/44937.fixed.md new file mode 100644 index 000000000000..4f8d4d768e32 --- /dev/null +++ b/changelog/44937.fixed.md @@ -0,0 +1 @@ +Fixed non-deterministic pillar rendering when multiple ``pillar_roots`` environments matched the same minion. ``Pillar.get_tops`` collected saltenvs into a ``set`` and iterated them in hash order, so top-file processing order depended on ``PYTHONHASHSEED`` and varied per ``salt-call`` invocation. An earlier change made ``_get_envs`` return an ordered list, but the caller wrapped the result back into a ``set``. ``get_tops`` now uses an insertion-ordered dict so iteration follows ``pillar_roots`` config order. diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 611f59f53847..e3a0ec8355fc 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -720,7 +720,12 @@ def get_tops(self): errors = [] # Gather initial top files try: - saltenvs = set() + # Use a dict (insertion-ordered) instead of a set so the + # iteration order matches the order returned by ``_get_envs``, + # which itself reflects the order of ``pillar_roots`` in the + # config. ``set`` iteration order depends on PYTHONHASHSEED + # and produced non-deterministic top-file processing (#44937). + saltenvs = {} if self.opts["pillarenv"]: # If the specified pillarenv is not present in the available # pillar environments, do not cache the pillar top file. @@ -732,11 +737,13 @@ def get_tops(self): ", ".join(self.opts["pillar_roots"]), ) else: - saltenvs.add(self.opts["pillarenv"]) + saltenvs[self.opts["pillarenv"]] = None else: - saltenvs.update(self._get_envs()) + for env in self._get_envs(): + saltenvs[env] = None if self.opts.get("pillar_source_merging_strategy", None) == "none": - saltenvs &= {self.saltenv or "base"} + only = self.saltenv or "base" + saltenvs = {only: None} if only in saltenvs else {} for saltenv in saltenvs: top = self.client.cache_file(self.opts["state_top"], saltenv) diff --git a/tests/pytests/unit/pillar/test_pillar.py b/tests/pytests/unit/pillar/test_pillar.py index 47818c4b04c6..f24e4b61db98 100644 --- a/tests/pytests/unit/pillar/test_pillar.py +++ b/tests/pytests/unit/pillar/test_pillar.py @@ -35,6 +35,58 @@ def test_pillar_envs_order(envs, temp_salt_minion, tmp_path): assert pillar._get_envs() == ["base"] + envs +def test_pillar_get_tops_iterates_envs_in_pillar_roots_order( + temp_salt_minion, tmp_path +): + """ + Regression test for issue #44937. + + ``Pillar.get_tops`` used to collect saltenvs into a ``set`` which is + iterated in hash order. With a randomized ``PYTHONHASHSEED`` this made + top-file processing non-deterministic. The fix uses an insertion-ordered + dict so iteration follows the order returned by ``_get_envs``, which + itself follows ``pillar_roots`` config order. + + This test exercises ``get_tops`` directly with multiple envs whose + top.sls all match the same minion and asserts the resulting ``tops`` + keys appear in ``_get_envs`` order. + """ + envs = ["base", "foo", "bar", "baz"] + for envname in envs: + env_dir = tmp_path / envname + env_dir.mkdir() + (env_dir / "top.sls").write_text( + f"{envname}:\n '{temp_salt_minion.id}':\n - data\n", + encoding="utf-8", + ) + (env_dir / "data.sls").write_text(f"key: {envname}\n", encoding="utf-8") + + opts = temp_salt_minion.config.copy() + opts["pillarenv"] = None + opts["pillar_roots"] = OrderedDict( + (envname, [str(tmp_path / envname)]) for envname in envs + ) + grains = salt.loader.grains(opts) + pillar = salt.pillar.Pillar( + opts=opts, + grains=grains, + minion_id=temp_salt_minion.id, + saltenv="base", + ) + tops, errors = pillar.get_tops() + assert not errors, errors + # ``_get_envs`` prepends "base" then appends the remaining roots in + # config order. Any env whose top.sls rendered should appear in + # ``tops`` in that same order. + expected_order = pillar._get_envs() + actual_order = list(tops.keys()) + assert actual_order == expected_order, ( + f"get_tops iteration order {actual_order!r} does not match " + f"_get_envs order {expected_order!r}; pillar top-file processing " + f"is non-deterministic (issue #44937)." + ) + + def test_pillar_get_tops_should_not_error_when_merging_strategy_is_none_and_no_pillarenv( temp_salt_minion, ):