From 9cfc76a221701440100c799dcd2497ddb6524521 Mon Sep 17 00:00:00 2001 From: Rick Elrod Date: Tue, 23 Jun 2020 10:53:25 -0500 Subject: [PATCH] Nuke with_ squashing, deprecated for 2.11 (#70209) Change: Removes with_* loop squashing and tests for 2.11 Test Plan: CI, and grepped for with_items in package manager integration targets. There might be some test cases in collections which need to stop testing this behavior. Signed-off-by: Rick Elrod --- .../deprecation-taskexecutor-squash.yml | 2 + lib/ansible/config/base.yml | 17 -- lib/ansible/executor/task_executor.py | 91 --------- test/sanity/ignore.txt | 1 - test/units/executor/test_task_executor.py | 183 ------------------ 5 files changed, 2 insertions(+), 292 deletions(-) create mode 100644 changelogs/fragments/deprecation-taskexecutor-squash.yml diff --git a/changelogs/fragments/deprecation-taskexecutor-squash.yml b/changelogs/fragments/deprecation-taskexecutor-squash.yml new file mode 100644 index 00000000000..43475830208 --- /dev/null +++ b/changelogs/fragments/deprecation-taskexecutor-squash.yml @@ -0,0 +1,2 @@ +removed_features: + - "`with_*` loops are no longer optimized for modules whose `name` parameters can take lists (mostly package managers). Use `name` instead of looping over individual names with `with_items` and friends." diff --git a/lib/ansible/config/base.yml b/lib/ansible/config/base.yml index 26158b32074..438f35eaec9 100644 --- a/lib/ansible/config/base.yml +++ b/lib/ansible/config/base.yml @@ -1046,23 +1046,6 @@ DEFAULT_SFTP_BATCH_MODE: - {key: sftp_batch_mode, section: ssh_connection} type: boolean yaml: {key: ssh_connection.sftp_batch_mode} -DEFAULT_SQUASH_ACTIONS: - name: Squashable actions - default: apk, apt, dnf, homebrew, openbsd_pkg, pacman, pip, pkgng, yum, zypper - description: - - Ansible can optimise actions that call modules that support list parameters when using ``with_`` looping. - Instead of calling the module once for each item, the module is called once with the full list. - - The default value for this setting is only for certain package managers, but it can be used for any module. - - Currently, this is only supported for modules that have a name or pkg parameter, and only when the item is the only thing being passed to the parameter. - env: [{name: ANSIBLE_SQUASH_ACTIONS}] - ini: - - {key: squash_actions, section: defaults} - type: list - version_added: "2.0" - deprecated: - why: Loop squashing is deprecated and this configuration will no longer be used - version: "2.11" - alternatives: a list directly with the module argument DEFAULT_SSH_TRANSFER_METHOD: # TODO: move to ssh plugin default: diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index 203cfbca81e..0cb03d71414 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -80,10 +80,6 @@ class TaskExecutor: class. ''' - # Modules that we optimize by squashing loop items into a single call to - # the module - SQUASH_ACTIONS = frozenset(C.DEFAULT_SQUASH_ACTIONS) - def __init__(self, host, task, job_vars, play_context, new_stdin, loader, shared_loader_obj, final_q): self._host = host self._task = task @@ -295,9 +291,6 @@ class TaskExecutor: u" to something else to avoid variable collisions and unexpected behavior." % loop_var) ran_once = False - if self._task.loop_with: - # Only squash with 'with_:' not with the 'loop:', 'magic' squashing can be removed once with_ loops are - items = self._squash_items(items, loop_var, task_vars) no_log = False items_len = len(items) @@ -411,90 +404,6 @@ class TaskExecutor: return results - def _squash_items(self, items, loop_var, variables): - ''' - Squash items down to a comma-separated list for certain modules which support it - (typically package management modules). - ''' - name = None - 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.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: - if all(isinstance(o, string_types) for o in items): - final_items = [] - - found = None - for allowed in ['name', 'pkg', 'package']: - name = self._task.args.pop(allowed, None) - if name is not None: - found = allowed - 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.is_template(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: - if self._task.loop_with and self._task.loop_with not in ('items', 'list'): - value_text = "\"{{ query('%s', %r) }}\"" % (self._task.loop_with, self._task.loop) - else: - value_text = '%r' % self._task.loop - # Without knowing the data structure well, it's easiest to strip python2 unicode - # literals after stringifying - value_text = re.sub(r"\bu'", "'", value_text) - - display.deprecated( - 'Invoking "%s" only once while using a loop via squash_actions is deprecated. ' - 'Instead of using a loop to supply multiple items and specifying `%s: "%s"`, ' - 'please use `%s: %s` and remove the loop' % (self._task.action, found, name, found, value_text), - version='2.11', collection_name='ansible.builtin' - ) - 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 Exception: - # Squashing is an optimization. If it fails for any reason, - # simply use the unoptimized list of items. - - # Restore the name parameter - if name is not None: - self._task.args['name'] = name - return items - def _execute(self, variables=None): ''' The primary workhorse of the executor system, this runs the task diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index c927abfee0b..417dd93e3c8 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -39,7 +39,6 @@ lib/ansible/executor/playbook_executor.py pylint:blacklisted-name lib/ansible/executor/powershell/async_watchdog.ps1 pslint:PSCustomUseLiteralPath lib/ansible/executor/powershell/async_wrapper.ps1 pslint:PSCustomUseLiteralPath lib/ansible/executor/powershell/exec_wrapper.ps1 pslint:PSCustomUseLiteralPath -lib/ansible/executor/task_executor.py pylint:ansible-deprecated-version lib/ansible/executor/task_queue_manager.py pylint:blacklisted-name lib/ansible/galaxy/collection.py compile-2.6!skip # 'ansible-galaxy collection' requires 2.7+ lib/ansible/module_utils/compat/_selectors2.py future-import-boilerplate # ignore bundled diff --git a/test/units/executor/test_task_executor.py b/test/units/executor/test_task_executor.py index 0ae7995a8db..300fd339d90 100644 --- a/test/units/executor/test_task_executor.py +++ b/test/units/executor/test_task_executor.py @@ -187,194 +187,11 @@ class TestTaskExecutor(unittest.TestCase): def _execute(variables): return dict(item=variables.get('item')) - te._squash_items = MagicMock(return_value=items) te._execute = MagicMock(side_effect=_execute) res = te._run_loop(items) self.assertEqual(len(res), 3) - def test_task_executor_squash_items(self): - items = ['a', 'b', 'c'] - - fake_loader = DictDataLoader({}) - - mock_host = MagicMock() - - loop_var = 'item' - - def _evaluate_conditional(templar, variables): - item = variables.get(loop_var) - if item == 'b': - return False - return True - - mock_task = MagicMock() - mock_task.evaluate_conditional.side_effect = _evaluate_conditional - - mock_play_context = MagicMock() - - mock_shared_loader = None - mock_queue = MagicMock() - - new_stdin = None - job_vars = dict(pkg_mgr='yum') - - te = TaskExecutor( - host=mock_host, - task=mock_task, - job_vars=job_vars, - play_context=mock_play_context, - new_stdin=new_stdin, - loader=fake_loader, - shared_loader_obj=mock_shared_loader, - final_q=mock_queue, - ) - - # No replacement - mock_task.action = 'yum' - new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) - self.assertEqual(new_items, ['a', 'b', 'c']) - self.assertIsInstance(mock_task.args, MagicMock) - - mock_task.action = 'foo' - 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(mock_task.args, {'name': '{{item}}'}) - - mock_task.action = 'yum' - mock_task.args = {'name': 'static'} - new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) - self.assertEqual(new_items, ['a', 'b', 'c']) - self.assertEqual(mock_task.args, {'name': 'static'}) - - mock_task.action = 'yum' - mock_task.args = {'name': '{{pkg_mgr}}'} - new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) - self.assertEqual(new_items, ['a', 'b', 'c']) - self.assertEqual(mock_task.args, {'name': '{{pkg_mgr}}'}) - - 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']) - self.assertEqual(mock_task.args, {'name': '{{item}}'}) - - # Could do something like this to recover from bad deps in a package - job_vars = dict(pkg_mgr='yum', packages=['a', 'b']) - items = ['absent', 'latest'] - mock_task.action = 'yum' - mock_task.args = {'name': '{{ packages }}', 'state': '{{ item }}'} - new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) - self.assertEqual(new_items, items) - self.assertEqual(mock_task.args, {'name': '{{ packages }}', 'state': '{{ item }}'}) - - # 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) - self.assertEqual(mock_task.args, {'name': '{{ packages[item] }}'}) - - # 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) - self.assertEqual(new_items, [['a', 'c']]) - self.assertEqual(mock_task.args, {'name': ['a', 'c']}) - - mock_task.action = '{{pkg_mgr}}' - mock_task.args = {'name': '{{item}}'} - new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) - self.assertEqual(new_items, [['a', 'c']]) - self.assertEqual(mock_task.args, {'name': ['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']]) - self.assertEqual(mock_task.args, {'name': ['a', 'c']}) - loop_var = 'item' - - # - # 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 - # - - # 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', 'foo', 'bar']]) - # self.assertEqual(mock_task.args, {'name': ['a', 'b', 'foo', 'bar']}) - self.assertEqual(new_items, items) - self.assertEqual(mock_task.args, {'name': '{{ item }}'}) - - # 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(mock_task.args, {'name': ['foo', 'baz']}) - self.assertEqual(new_items, items) - self.assertEqual(mock_task.args, {'name': '{{ packages[item] }}'}) - - # 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(mock_task.args, {'name': ['foo', 'bar']}) - self.assertEqual(new_items, items) - self.assertEqual(mock_task.args, {'name': '{{ item["package"] }}'}) - - 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.name}}', 'state': '{{item.state}}'} - new_items = te._squash_items(items=items, loop_var='item', variables=job_vars) - # self.assertEqual(new_items, [dict(name=['a', 'b', 'c'], state='present')]) - # self.assertEqual(mock_task.args, {'name': ['a', 'b', 'c'], 'state': 'present'}) - self.assertEqual(new_items, items) - self.assertEqual(mock_task.args, {'name': '{{item.name}}', 'state': '{{item.state}}'}) - - items = [ - dict(name='a', state='present'), - dict(name='b', state='present'), - dict(name='c', state='absent'), - ] - mock_task.action = 'yum' - 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, [dict(name=['a', 'b'], state='present'), - # dict(name='c', state='absent')]) - # self.assertEqual(mock_task.args, {'name': '{{item.name}}', 'state': '{{item.state}}'}) - self.assertEqual(new_items, items) - self.assertEqual(mock_task.args, {'name': '{{item.name}}', 'state': '{{item.state}}'}) - def test_task_executor_get_action_handler(self): te = TaskExecutor( host=MagicMock(),