From 292f0ed0d627a486ec376d649f3ae73f21d27354 Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Wed, 11 May 2016 18:26:39 -0700 Subject: [PATCH] If we can't squash for any reason, then simply do not optimize the items loop. Also add more squashing testcases Fixes #15649 --- lib/ansible/executor/task_executor.py | 111 +++++++++++----------- test/units/executor/test_task_executor.py | 102 +++++++++++++------- 2 files changed, 128 insertions(+), 85 deletions(-) diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index 2a475c2a106..d97fdf41f6a 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -269,59 +269,64 @@ class TaskExecutor: Squash items down to a comma-separated list for certain modules which support it (typically package management modules). ''' - # _task.action could contain templatable strings (via action: and - # local_action:) Template it before comparing. If we don't end up - # optimizing it here, the templatable string might use template vars - # that aren't available until later (it could even use vars from the - # 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): - task_action = templar.template(task_action, fail_on_undefined=False) - - if len(items) > 0 and task_action in self.SQUASH_ACTIONS: - if all(isinstance(o, string_types) for o in items): - final_items = [] - - name = None - for allowed in ['name', 'pkg', 'package']: - name = self._task.args.pop(allowed, None) - if name is not None: - break - - # This gets the information to check whether the name field - # contains a template that we can squash for - template_no_item = template_with_item = None - if name: - if templar._contains_vars(name): - variables[loop_var] = '\0$' - template_no_item = templar.template(name, variables, cache=False) - variables[loop_var] = '\0@' - template_with_item = templar.template(name, variables, cache=False) - del variables[loop_var] - - # Check if the user is doing some operation that doesn't take - # name/pkg or the name/pkg field doesn't have any variables - # and thus the items can't be squashed - if template_no_item != template_with_item: - for item in items: - variables[loop_var] = item - if self._task.evaluate_conditional(templar, variables): - new_item = templar.template(name, cache=False) - final_items.append(new_item) - self._task.args['name'] = final_items - # Wrap this in a list so that the calling function loop - # executes exactly once - return [final_items] - else: - # Restore the name parameter - self._task.args['name'] = name - #elif: - # Right now we only optimize single entries. In the future we - # could optimize more types: - # * lists can be squashed together - # * dicts could squash entries that match in all cases except the - # name or pkg field. + try: + # _task.action could contain templatable strings (via action: and + # local_action:) Template it before comparing. If we don't end up + # optimizing it here, the templatable string might use template vars + # that aren't available until later (it could even use vars from the + # 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): + task_action = templar.template(task_action, fail_on_undefined=False) + + if len(items) > 0 and task_action in self.SQUASH_ACTIONS: + if all(isinstance(o, string_types) for o in items): + final_items = [] + + name = None + for allowed in ['name', 'pkg', 'package']: + name = self._task.args.pop(allowed, None) + if name is not None: + break + + # This gets the information to check whether the name field + # contains a template that we can squash for + template_no_item = template_with_item = None + if name: + if templar._contains_vars(name): + variables[loop_var] = '\0$' + template_no_item = templar.template(name, variables, cache=False) + variables[loop_var] = '\0@' + template_with_item = templar.template(name, variables, cache=False) + del variables[loop_var] + + # Check if the user is doing some operation that doesn't take + # name/pkg or the name/pkg field doesn't have any variables + # and thus the items can't be squashed + if template_no_item != template_with_item: + for item in items: + variables[loop_var] = item + if self._task.evaluate_conditional(templar, variables): + new_item = templar.template(name, cache=False) + final_items.append(new_item) + self._task.args['name'] = final_items + # Wrap this in a list so that the calling function loop + # executes exactly once + return [final_items] + else: + # Restore the name parameter + self._task.args['name'] = name + #elif: + # Right now we only optimize single entries. In the future we + # could optimize more types: + # * lists can be squashed together + # * dicts could squash entries that match in all cases except the + # name or pkg field. + except: + # Squashing is an optimization. If it fails for any reason, + # simply use the unoptimized list of items. + pass return items def _execute(self, variables=None): diff --git a/test/units/executor/test_task_executor.py b/test/units/executor/test_task_executor.py index 8f2c1ed8cc8..87704e21881 100644 --- a/test/units/executor/test_task_executor.py +++ b/test/units/executor/test_task_executor.py @@ -180,8 +180,10 @@ class TestTaskExecutor(unittest.TestCase): mock_host = MagicMock() + loop_var = 'item' + def _evaluate_conditional(templar, variables): - item = variables.get('item') + item = variables.get(loop_var) if item == 'b': return False return True @@ -230,9 +232,31 @@ class TestTaskExecutor(unittest.TestCase): new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) self.assertEqual(new_items, ['a', 'b', 'c']) + mock_task.action = '{{unknown}}' + mock_task.args={'name': '{{item}}'} + new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) + self.assertEqual(new_items, ['a', 'b', 'c']) + + # Maybe should raise an error in this case. The user would have to specify: + # - yum: name="{{ packages[item] }}" + # with_items: + # - ['a', 'b'] + # - ['foo', 'bar'] + # you can't use a list as a dict key so that would probably throw + # an error later. If so, we can throw it now instead. + # Squashing in this case would not be intuitive as the user is being + # explicit in using each list entry as a key. + job_vars = dict(pkg_mgr='yum', packages={ "a": "foo", "b": "bar", "foo": "baz", "bar": "quux" }) + items = [['a', 'b'], ['foo', 'bar']] + mock_task.action = 'yum' + mock_task.args = {'name': '{{ packages[item] }}'} + new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) + self.assertEqual(new_items, items) + # # Replaces # + items = ['a', 'b', 'c'] mock_task.action = 'yum' mock_task.args={'name': '{{item}}'} new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) @@ -243,29 +267,65 @@ class TestTaskExecutor(unittest.TestCase): new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) self.assertEqual(new_items, [['a', 'c']]) + # New loop_var + mock_task.action = 'yum' + mock_task.args = {'name': '{{a_loop_var_item}}'} + mock_task.loop_control = {'loop_var': 'a_loop_var_item'} + loop_var = 'a_loop_var_item' + new_items = te._squash_items(items=items, loop_var='a_loop_var_item', variables=job_vars) + self.assertEqual(new_items, [['a', 'c']]) + loop_var = 'item' + # - # Smoketests -- these won't optimize but make sure that they don't - # traceback either + # These are presently not optimized but could be in the future. + # Expected output if they were optimized is given as a comment + # Please move these to a different section if they are optimized # - mock_task.action = '{{unknown}}' - mock_task.args={'name': '{{item}}'} + + # Squashing lists + job_vars = dict(pkg_mgr='yum') + items = [['a', 'b'], ['foo', 'bar']] + mock_task.action = 'yum' + mock_task.args = {'name': '{{ item }}'} new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) - self.assertEqual(new_items, ['a', 'b', 'c']) + #self.assertEqual(new_items, [['a', 'b', 'foo', 'bar']]) + self.assertEqual(new_items, items) + + # Retrieving from a dict + items = ['a', 'b', 'foo'] + mock_task.action = 'yum' + mock_task.args = {'name': '{{ packages[item] }}'} + new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) + #self.assertEqual(new_items, [['foo', 'baz']]) + self.assertEqual(new_items, items) + + # Another way to retrieve from a dict + job_vars = dict(pkg_mgr='yum') + items = [{'package': 'foo'}, {'package': 'bar'}] + mock_task.action = 'yum' + mock_task.args = {'name': '{{ item["package"] }}'} + new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) + #self.assertEqual(new_items, [['foo', 'bar']]) + self.assertEqual(new_items, items) items = [dict(name='a', state='present'), dict(name='b', state='present'), dict(name='c', state='present')] mock_task.action = 'yum' - mock_task.args={'name': '{{item}}'} + mock_task.args={'name': '{{item.name}}', 'state': '{{item.state}}'} new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) self.assertEqual(new_items, items) + #self.assertEqual(new_items, [dict(name=['a', 'b', 'c'], state='present')]) - job_vars = dict(pkg_mgr='yum') - items = [{'package': 'foo'}, {'package': 'bar'}] + items = [dict(name='a', state='present'), + dict(name='b', state='present'), + dict(name='c', state='absent')] mock_task.action = 'yum' - mock_task.args = {'name': '{{ items["package"] }}'} + mock_task.args={'name': '{{item.name}}', 'state': '{{item.state}}'} new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) self.assertEqual(new_items, items) + #self.assertEqual(new_items, [dict(name=['a', 'b'], state='present'), + # dict(name='c', state='absent')]) # Could do something like this to recover from bad deps in a package job_vars = dict(pkg_mgr='yum', packages=['a', 'b']) @@ -275,28 +335,6 @@ class TestTaskExecutor(unittest.TestCase): new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) self.assertEqual(new_items, items) - # - # These are presently not optimized but could be in the future. - # Expected output if they were optimized is given as a comment - # Please move these to a different section if they are optimized - # - - job_vars = dict(pkg_mgr='yum') - items = [['a', 'b'], ['foo', 'bar']] - mock_task.action = 'yum' - mock_task.args = {'name': '{{ packages[item] }}'} - new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) - self.assertEqual(new_items, items) - #self.assertEqual(new_items, ['a', 'b', 'foo', 'bar']) - - ### Enable this when we've fixed https://github.com/ansible/ansible/issues/15649 - #job_vars = dict(pkg_mgr='yum', packages={ "a": "foo", "b": "bar", "c": "baz" }) - #items = ['a', 'b', 'c'] - #mock_task.action = 'yum' - #mock_task.args = {'name': '{{ packages[item] }}'} - #new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) - #self.assertEqual(new_items, items) - #self.assertEqual(new_items, ['foo', 'bar', 'baz']) def test_task_executor_execute(self): fake_loader = DictDataLoader({})