From e55748ba69593fbd249a1c0385339e352f43d07c Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Tue, 22 Aug 2023 16:16:58 -0700 Subject: [PATCH] Make Ansible variable freezing more efficient We currently iterate over every job/host/etc variable in the freeze playbook. The reason is because if any value in any variable is Undefined according to jinja, the Ansible combine filter throws an error. What we want to do in Zuul is merge any variable we can, but if any is undefined, we skip it. Thus, the process of combining the variables one at a time in a task and ignoring errors. This process can be slow, especially if we have start with a large amount of data in one of the early variables. The combine filter needs to reprocess the large data repeatedly for each additional variable. To improve the process, we create a new action plugin, "zuul_freeze" which takes a list of variables we want to freeze, then templates them one at a time and stores the result in a cacheable fact. This is the essence of what we were trying to accomplish with the combine filter. Change-Id: Ie41f404762daa1b1a5ae47f6ec1aa1954ad36a39 --- .../unsafe-vars/git/org_project/zuul.yaml | 3 ++ zuul/ansible/6/action/zuul_freeze.py | 1 + zuul/ansible/8/action/zuul_freeze.py | 1 + zuul/ansible/base/action/zuul_freeze.py | 53 +++++++++++++++++++ zuul/executor/server.py | 16 ++---- 5 files changed, 61 insertions(+), 13 deletions(-) create mode 120000 zuul/ansible/6/action/zuul_freeze.py create mode 120000 zuul/ansible/8/action/zuul_freeze.py create mode 100644 zuul/ansible/base/action/zuul_freeze.py diff --git a/tests/fixtures/config/unsafe-vars/git/org_project/zuul.yaml b/tests/fixtures/config/unsafe-vars/git/org_project/zuul.yaml index 58da53f8d5..4a7af39560 100644 --- a/tests/fixtures/config/unsafe-vars/git/org_project/zuul.yaml +++ b/tests/fixtures/config/unsafe-vars/git/org_project/zuul.yaml @@ -9,6 +9,9 @@ vars: latesub: "{{ latefact | default('undefined') }}" jobvar: "{{ base_secret.secret | default('undefined') }}" + # Make sure we have a top level variable that is undefined to + # ensure that it doesn't cause all values to be omitted. + undefvar: "{{ undefinedvar }}" run: playbooks/testjob-run.yaml - job: diff --git a/zuul/ansible/6/action/zuul_freeze.py b/zuul/ansible/6/action/zuul_freeze.py new file mode 120000 index 0000000000..32adfa3d09 --- /dev/null +++ b/zuul/ansible/6/action/zuul_freeze.py @@ -0,0 +1 @@ +../../base/action/zuul_freeze.py \ No newline at end of file diff --git a/zuul/ansible/8/action/zuul_freeze.py b/zuul/ansible/8/action/zuul_freeze.py new file mode 120000 index 0000000000..32adfa3d09 --- /dev/null +++ b/zuul/ansible/8/action/zuul_freeze.py @@ -0,0 +1 @@ +../../base/action/zuul_freeze.py \ No newline at end of file diff --git a/zuul/ansible/base/action/zuul_freeze.py b/zuul/ansible/base/action/zuul_freeze.py new file mode 100644 index 0000000000..827d2b15a7 --- /dev/null +++ b/zuul/ansible/base/action/zuul_freeze.py @@ -0,0 +1,53 @@ +# Copyright (C) 2023 Acme Gating, LLC +# +# This module is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This software is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this software. If not, see . + +from ansible.plugins.action import ActionBase +from ansible.template import recursive_check_defined + + +class ActionModule(ActionBase): + def run(self, tmp=None, task_vars=None): + """This module accepts one parameter: + + _zuul_freeze_vars is a list of variable names to template. + + It stores the templated variables in a cacheable fact named + _zuul_frozen. + + If any variable is undefined or recursively references any + undefined variables, it is omitted from the result. + + """ + if task_vars is None: + task_vars = dict() + results = super(ActionModule, self).run(tmp, task_vars) + del tmp # tmp no longer has any effect + + varlist = self._task.args.get('_zuul_freeze_vars') + ret = {} + for var in varlist: + try: + # Template the variable (convert_bare means treat a + # bare variable name as {{ var }}. + value = self._templar.template(var, convert_bare=True) + recursive_check_defined(value) + ret[var] = value + except Exception: + pass + + results['ansible_facts'] = {'_zuul_frozen': ret} + results['_ansible_facts_cacheable'] = True + + return results diff --git a/zuul/executor/server.py b/zuul/executor/server.py index ac11312a9e..5d66ffb489 100644 --- a/zuul/executor/server.py +++ b/zuul/executor/server.py @@ -3060,21 +3060,11 @@ class AnsibleJob(object): } for host in self.host_list + [localhost]: tasks = [{ - 'set_fact': { - '_zuul_frozen': {}, - 'cacheable': True, + 'zuul_freeze': { + '_zuul_freeze_vars': list( + self.original_hostvars[host['name']].keys()), }, }] - for var in self.original_hostvars[host['name']].keys(): - val = "{{ _zuul_frozen | combine({'%s': %s}) }}" % (var, var) - task = { - 'set_fact': { - '_zuul_frozen': val, - 'cacheable': True, - }, - 'ignore_errors': True, - } - tasks.append(task) play = { 'hosts': host['name'], 'tasks': tasks,