From ce8368019dbfd8c328edcf14df2a383f0710bcd3 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Mon, 14 Sep 2026 18:10:40 +0200 Subject: [PATCH] perf(constructor): carry the applications SPOT through the merge block 113113e553 parks the carrier after the token store, 165 lines below the merge block, so each of the block's six tasks still renders the applications SPOT cold in its own forked worker. Measured in CI run wp-swarm-tor2 they cost 323.51s in the PASS 1 main play, 180.39s in the backup play and 479.79s in PASS 2. The cost is the render, not the work. Two tasks in that block share the exact construct `defaults_X | combine(X | default({}, true), recursive=True)`: design takes 0.247s and service_provider 75.94s. The difference is that defaults_service_provider is dense with lookup('config'/'users'/'domains'/'tls') and every one of them lands in get_merged_applications. The carrier cannot simply move up. get_merged_applications serves the carrier fact whenever the key matches, and the carrier action goes through that same path, so a second carrier task would be handed the first one's payload instead of rendering. merged_applications_cache_key reads neither group_names nor the token file, and with a real templar 71 of 265 applications differ by group_names alone, so a carrier left standing across add_host or the token store would be served where a fresh render belongs. So the block opens its own carrier and drops it again at the end. Nothing outside the block changes: every task from sys-version to the stage-wide carrier renders exactly as before. Placement follows measurement. A per-key perturbation test over the whole 265-application payload, with the credential regeneration noise floor subtracted and every role in group_names, finds exactly two reads of the seven facts the block sets: .web-app-matomo.services.matomo.excluded_ips[0] <- networks .web-app-wordpress.services.wordpress.title <- service_provider networks is the block's first task and service_provider its last, which is why the carrier opens after the one and is dropped before the other takes effect. The block's tasks already render before service_provider is merged today, so the frozen payload is what they would have rendered anyway. The action gains a `clear` argument so the playbook never spells the private fact name. Parking the carrier serves a payload in 0.000s against a 3.417s cold render; a wrong key and a cleared fact both force the render again, which is what makes the drop task work. Worth roughly 806s of the 20199s job, against a floor of 673.6s. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/action/applications_carrier.py | 36 ++++++++++++++--- tasks/stages/01_constructor.yml | 7 ++++ .../action/test_applications_carrier.py | 40 +++++++++++++++++++ 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/plugins/action/applications_carrier.py b/plugins/action/applications_carrier.py index 5991ff5cd8..f7711ed295 100644 --- a/plugins/action/applications_carrier.py +++ b/plugins/action/applications_carrier.py @@ -5,19 +5,31 @@ Ansible forks a fresh worker per task, so the in-process render cache of a place all later workers inherit. This action renders in the worker of one constructor task and returns the payload as a host fact; every later worker inherits that fact at fork time and ``get_merged_applications`` serves it -instead of rendering. Sits after the token store in -``tasks/stages/01_constructor.yml`` so the embedded user records already -carry the loaded tokens:: +instead of rendering:: - name: 🧊 Render the applications SPOT once and carry it into every task's worker applications_carrier: {} +``tasks/stages/01_constructor.yml`` parks two of them. The stage-wide one +sits after the token store so the embedded user records already carry the +loaded tokens, and after the ``add_host`` tasks because 114 roles gate +services on ``group_names`` and the payload bakes those in. The +merge block opens an earlier, block-scoped one and drops it again at the +block's end, because ``merged_applications_cache_key`` reads neither +``group_names`` nor the token file: a carrier left standing across either +would be served unchanged where a fresh render belongs:: + + - name: 🧊 Drop the merge-block carrier so later stages re-render + applications_carrier: + clear: true + The result is masked by the action itself: the fact is the whole rendered tree, about 0.4 MB with every application's credentials, and printing it at ``-v`` is neither credential debugging nor readable. Args: - none. Any argument is an error. + clear: drop the carrier fact instead of rendering. Defaults to false. + Any other argument is an error. """ from __future__ import annotations @@ -25,6 +37,7 @@ from __future__ import annotations from typing import Any from ansible.errors import AnsibleActionFail +from ansible.module_utils.parsing.convert_bool import boolean from ansible.plugins.action import ActionBase from ansible.plugins.loader import lookup_loader @@ -39,8 +52,19 @@ class ActionModule(ActionBase): ) -> dict[str, Any]: result = super().run(tmp, task_vars) task_vars = task_vars or {} - if self._task.args: - raise AnsibleActionFail("applications_carrier takes no arguments") + args = self._task.args or {} + unexpected = sorted(set(args) - {"clear"}) + if unexpected: + raise AnsibleActionFail( + f"applications_carrier: unknown argument(s) {', '.join(unexpected)}; " + "the only argument is 'clear'" + ) + + if boolean(args.get("clear", False)): + result.update( + changed=False, ansible_facts={APPLICATIONS_RENDERED_FACT: None} + ) + return result lookup = lookup_loader.get( "applications", loader=self._loader, templar=self._templar diff --git a/tasks/stages/01_constructor.yml b/tasks/stages/01_constructor.yml index 70a1a2aab4..5f6b5c33e5 100644 --- a/tasks/stages/01_constructor.yml +++ b/tasks/stages/01_constructor.yml @@ -28,6 +28,9 @@ ansible.builtin.set_fact: networks: "{{ defaults_networks | combine(networks | default({}, true), recursive=True) }}" + - name: 🧊 Render the applications SPOT once for the merge block + applications_carrier: {} + - name: "✅ Sanity check: all group_names must exist in applications" ansible.builtin.assert: that: @@ -96,6 +99,10 @@ ansible.builtin.set_fact: service_provider: "{{ defaults_service_provider | combine(service_provider | default({}, true), recursive=True) }}" + - name: 🧊 Drop the merge-block carrier so later stages re-render + applications_carrier: + clear: true + - name: "💾 Persist {{ SOFTWARE_NAME }} version" ansible.builtin.include_role: name: sys-version diff --git a/tests/unit/python/plugins/action/test_applications_carrier.py b/tests/unit/python/plugins/action/test_applications_carrier.py index 5e80413bbb..841b2eea44 100644 --- a/tests/unit/python/plugins/action/test_applications_carrier.py +++ b/tests/unit/python/plugins/action/test_applications_carrier.py @@ -68,6 +68,46 @@ class TestApplicationsCarrierActionPlugin(unittest.TestCase): ): action.run(task_vars={}) + def test_clear_drops_the_fact_without_rendering(self): + lookup = _FakeLookup({"key": ["roles", []], "applications": {}}) + action = _make_action(_FakeTask(args={"clear": True})) + with ( + patch( + "plugins.action.applications_carrier.ActionBase.run", + autospec=True, + return_value={}, + ), + patch( + "plugins.action.applications_carrier.lookup_loader.get", + return_value=lookup, + ), + ): + result = action.run(task_vars={"applications": {}}) + + self.assertIsNone(result["ansible_facts"][APPLICATIONS_RENDERED_FACT]) + self.assertFalse(result["changed"]) + self.assertEqual(lookup.calls, []) + + def test_clear_false_still_renders(self): + carrier = {"key": ["roles", ["a", "b", "c", "d"]], "applications": {"x": {}}} + lookup = _FakeLookup(carrier) + action = _make_action(_FakeTask(args={"clear": "no"})) + with ( + patch( + "plugins.action.applications_carrier.ActionBase.run", + autospec=True, + return_value={}, + ), + patch( + "plugins.action.applications_carrier.lookup_loader.get", + return_value=lookup, + ), + ): + result = action.run(task_vars={"applications": {}}) + + self.assertIs(result["ansible_facts"][APPLICATIONS_RENDERED_FACT], carrier) + self.assertEqual(len(lookup.calls), 1) + if __name__ == "__main__": unittest.main()