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 <rick@elrod.me>

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."

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:

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

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

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(),