From 728fce0c4474e3ef8d38f62934454070605c8d1a Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Thu, 6 Jun 2019 14:49:27 -0500 Subject: [PATCH] Perf improvement for Templar.is_template (#57489) * Faster is_template --- changelogs/fragments/faster-is-template.yaml | 5 ++ lib/ansible/executor/task_executor.py | 6 +- lib/ansible/inventory/manager.py | 4 +- lib/ansible/parsing/mod_args.py | 4 +- lib/ansible/playbook/helpers.py | 4 +- lib/ansible/playbook/role/definition.py | 2 +- lib/ansible/template/__init__.py | 68 ++++++++++++-------- test/units/template/test_templar.py | 44 ++++++++++--- 8 files changed, 90 insertions(+), 47 deletions(-) create mode 100644 changelogs/fragments/faster-is-template.yaml diff --git a/changelogs/fragments/faster-is-template.yaml b/changelogs/fragments/faster-is-template.yaml new file mode 100644 index 00000000000..a6e6692013c --- /dev/null +++ b/changelogs/fragments/faster-is-template.yaml @@ -0,0 +1,5 @@ +minor_changes: +- Templar - Speed up ``is_template`` by lexing the string, instead of actually + templating the string (https://github.com/ansible/ansible/pull/57489) +- InventoryManager - Speed up host subset calculation by performing direct host + uuid comparisons, instead of Host object comparisons diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index f90b0584c92..bb439565bc6 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -231,7 +231,7 @@ class TaskExecutor: loop_terms = listify_lookup_plugin_terms(terms=self._task.loop, templar=templar, loader=self._loader, fail_on_undefined=fail, convert_bare=False) if not fail: - loop_terms = [t for t in loop_terms if not templar._contains_vars(t)] + loop_terms = [t for t in loop_terms if not templar.is_template(t)] # get lookup mylookup = self._shared_loader_obj.lookup_loader.get(self._task.loop_with, loader=self._loader, templar=templar) @@ -427,7 +427,7 @@ class TaskExecutor: # with_items loop) so don't make the templated string permanent yet. templar = Templar(loader=self._loader, shared_loader_obj=self._shared_loader_obj, variables=variables) task_action = self._task.action - if templar._contains_vars(task_action): + if templar.is_template(task_action): task_action = templar.template(task_action, fail_on_undefined=False) if len(items) > 0 and task_action in self.SQUASH_ACTIONS: @@ -445,7 +445,7 @@ class TaskExecutor: # contains a template that we can squash for template_no_item = template_with_item = None if name: - if templar._contains_vars(name): + if templar.is_template(name): variables[loop_var] = '\0$' template_no_item = templar.template(name, variables, cache=False) variables[loop_var] = '\0@' diff --git a/lib/ansible/inventory/manager.py b/lib/ansible/inventory/manager.py index 9e9f20808fc..e10f56876e5 100644 --- a/lib/ansible/inventory/manager.py +++ b/lib/ansible/inventory/manager.py @@ -360,8 +360,8 @@ class InventoryManager(object): # mainly useful for hostvars[host] access if not ignore_limits and self._subset: # exclude hosts not in a subset, if defined - subset = self._evaluate_patterns(self._subset) - hosts = [h for h in hosts if h in subset] + subset_uuids = [s._uuid for s in self._evaluate_patterns(self._subset)] + hosts = [h for h in hosts if h._uuid in subset_uuids] if not ignore_restrictions and self._restriction: # exclude hosts mentioned in any restriction (ex: failed hosts) diff --git a/lib/ansible/parsing/mod_args.py b/lib/ansible/parsing/mod_args.py index d23f2f2b9d1..9cffb53a42a 100644 --- a/lib/ansible/parsing/mod_args.py +++ b/lib/ansible/parsing/mod_args.py @@ -144,7 +144,7 @@ class ModuleArgsParser: if additional_args: if isinstance(additional_args, string_types): templar = Templar(loader=None) - if templar._contains_vars(additional_args): + if templar.is_template(additional_args): final_args['_variable_params'] = additional_args else: raise AnsibleParserError("Complex args containing variables cannot use bare variables (without Jinja2 delimiters), " @@ -311,7 +311,7 @@ class ModuleArgsParser: elif args.get('_raw_params', '') != '' and action not in RAW_PARAM_MODULES: templar = Templar(loader=None) raw_params = args.pop('_raw_params') - if templar._contains_vars(raw_params): + if templar.is_template(raw_params): args['_variable_params'] = raw_params else: raise AnsibleParserError("this task '%s' has extra params, which is only allowed in the following modules: %s" % (action, diff --git a/lib/ansible/playbook/helpers.py b/lib/ansible/playbook/helpers.py index f9b15dc4bbb..881939f9188 100644 --- a/lib/ansible/playbook/helpers.py +++ b/lib/ansible/playbook/helpers.py @@ -164,7 +164,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h else: is_static = C.DEFAULT_TASK_INCLUDES_STATIC or \ (use_handlers and C.DEFAULT_HANDLER_INCLUDES_STATIC) or \ - (not templar._contains_vars(t.args['_raw_params']) and t.all_parents_static() and not t.loop) + (not templar.is_template(t.args['_raw_params']) and t.all_parents_static() and not t.loop) if is_static: if t.loop is not None: @@ -351,7 +351,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h # template the role name now, if needed all_vars = variable_manager.get_vars(play=play, task=ir) templar = Templar(loader=loader, variables=all_vars) - if templar._contains_vars(ir._role_name): + if templar.is_template(ir._role_name): ir._role_name = templar.template(ir._role_name) # uses compiled list from object diff --git a/lib/ansible/playbook/role/definition.py b/lib/ansible/playbook/role/definition.py index b18f1d16525..fa4a1464e5e 100644 --- a/lib/ansible/playbook/role/definition.py +++ b/lib/ansible/playbook/role/definition.py @@ -129,7 +129,7 @@ class RoleDefinition(Base, Conditional, Taggable, CollectionSearch): if self._variable_manager: all_vars = self._variable_manager.get_vars(play=self._play) templar = Templar(loader=self._loader, variables=all_vars) - if templar._contains_vars(role_name): + if templar.is_template(role_name): role_name = templar.template(role_name) return role_name diff --git a/lib/ansible/template/__init__.py b/lib/ansible/template/__init__.py index ec4bf677137..e942ce5da66 100644 --- a/lib/ansible/template/__init__.py +++ b/lib/ansible/template/__init__.py @@ -88,6 +88,10 @@ else: from jinja2.utils import concat as j2_concat +JINJA2_BEGIN_TOKENS = frozenset(('variable_begin', 'block_begin', 'comment_begin', 'raw_begin')) +JINJA2_END_TOKENS = frozenset(('variable_end', 'block_end', 'comment_end', 'raw_end')) + + def generate_ansible_template_vars(path, dest_path=None): b_path = to_bytes(path) try: @@ -159,6 +163,39 @@ def _escape_backslashes(data, jinja_env): return data +def is_template(data, jinja_env): + """This function attempts to quickly detect whether a value is a jinja2 + template. To do so, we look for the first 2 matching jinja2 tokens for + start and end delimiters. + """ + found = None + start = True + comment = False + d2 = jinja_env.preprocess(data) + + # This wraps a lot of code, but this is due to lex returing a generator + # so we may get an exception at any part of the loop + try: + for token in jinja_env.lex(d2): + if token[1] in JINJA2_BEGIN_TOKENS: + if start and token[1] == 'comment_begin': + # Comments can wrap other token types + comment = True + start = False + # Example: variable_end -> variable + found = token[1].split('_')[0] + elif token[1] in JINJA2_END_TOKENS: + if token[1].split('_')[0] == found: + return True + elif comment: + continue + return False + except TemplateSyntaxError: + return False + + return False + + def _count_newlines_from_end(in_str): ''' Counts the number of newlines at the end of a string. This is used during @@ -498,7 +535,7 @@ class Templar: if isinstance(variable, string_types): result = variable - if self._contains_vars(variable): + if self.is_template(variable): # Check to see if the string we are trying to render is just referencing a single # var. In this case we don't want to accidentally change the type of the variable # to a string by using the jinja template renderer. We just want to pass it. @@ -596,13 +633,7 @@ class Templar: def is_template(self, data): ''' lets us know if data has a template''' if isinstance(data, string_types): - try: - new = self.do_template(data, fail_on_undefined=True) - except (AnsibleUndefinedVariable, UndefinedError): - return True - except Exception: - return False - return (new != data) + return is_template(data, self.environment) elif isinstance(data, (list, tuple)): for v in data: if self.is_template(v): @@ -613,26 +644,7 @@ class Templar: return True return False - def templatable(self, data): - ''' - returns True if the data can be templated w/o errors - ''' - templatable = True - try: - self.template(data) - except Exception: - templatable = False - return templatable - - def _contains_vars(self, data): - ''' - returns True if the data contains a variable pattern - ''' - if isinstance(data, string_types): - for marker in (self.environment.block_start_string, self.environment.variable_start_string, self.environment.comment_start_string): - if marker in data: - return True - return False + templatable = _contains_vars = is_template def _convert_bare_variable(self, variable): ''' diff --git a/test/units/template/test_templar.py b/test/units/template/test_templar.py index 02022ce7af3..8c74853942c 100644 --- a/test/units/template/test_templar.py +++ b/test/units/template/test_templar.py @@ -104,17 +104,43 @@ class TestTemplarTemplate(BaseTemplar, unittest.TestCase): # self.assertEqual(res['{{ a_keyword }}'], "blip") print(res) - def test_templatable(self): - res = self.templar.templatable('foo') - self.assertTrue(res) + def test_is_template_true(self): + tests = [ + '{{ foo }}', + '{% foo %}', + '{# foo #}', + '{# {{ foo }} #}', + '{# {{ nothing }} {# #}', + '{# {{ nothing }} {# #} #}', + '{% raw %}{{ foo }}{% endraw %}', + ] + for test in tests: + self.assertTrue(self.templar.is_template(test)) - def test_templatable_none(self): - res = self.templar.templatable(None) - self.assertTrue(res) + def test_is_template_false(self): + tests = [ + 'foo', + '{{ foo', + '{% foo', + '{# foo', + '{{ foo %}', + '{{ foo #}', + '{% foo }}', + '{% foo #}', + '{# foo %}', + '{# foo }}', + '{{ foo {{', + '{% raw %}{% foo %}', + ] + for test in tests: + self.assertFalse(self.templar.is_template(test)) - @patch('ansible.template.Templar.template', side_effect=AnsibleError) - def test_templatable_exception(self, mock_template): - res = self.templar.templatable('foo') + def test_is_template_raw_string(self): + res = self.templar.is_template('foo') + self.assertFalse(res) + + def test_is_template_none(self): + res = self.templar.is_template(None) self.assertFalse(res) def test_template_convert_bare_string(self):