Always create new role (#78661)

Don't use role cache for determining whether to create a new instance of role

12 files changed, 126 insertions, 68 deletions

diff --git a/changelogs/fragments/always-create-new-role.yml b/changelogs/fragments/always-create-new-role.yml
new file mode 100644
index 0000000000..87209ccc07
--- /dev/null
+++ b/changelogs/fragments/always-create-new-role.yml
@@ -0,0 +1,5 @@
+bugfixes:
+- role deduplication - Always create new role object, regardless of
+  deduplication. Deduplication will only affect whether a duplicate call to a
+  role will execute, as opposed to re-using the same object.
+  (https://github.com/ansible/ansible/pull/78661)
diff --git a/lib/ansible/playbook/play.py b/lib/ansible/playbook/play.py
index 23bb36b2bf..ec9f78c651 100644
--- a/lib/ansible/playbook/play.py
+++ b/lib/ansible/playbook/play.py
@@ -30,7 +30,7 @@ from ansible.playbook.base import Base
 from ansible.playbook.block import Block
 from ansible.playbook.collectionsearch import CollectionSearch
 from ansible.playbook.helpers import load_list_of_blocks, load_list_of_roles
-from ansible.playbook.role import Role
+from ansible.playbook.role import Role, hash_params
 from ansible.playbook.task import Task
 from ansible.playbook.taggable import Taggable
 from ansible.vars.manager import preprocess_vars
@@ -93,7 +93,7 @@ class Play(Base, Taggable, CollectionSearch):
         self._included_conditional = None
         self._included_path = None
         self._removed_hosts = []
-        self.ROLE_CACHE = {}
+        self.role_cache = {}
 
         self.only_tags = set(context.CLIARGS.get('tags', [])) or frozenset(('all',))
         self.skip_tags = set(context.CLIARGS.get('skip_tags', []))
@@ -104,6 +104,22 @@ class Play(Base, Taggable, CollectionSearch):
     def __repr__(self):
         return self.get_name()
 
+    @property
+    def ROLE_CACHE(self):
+        """Backwards compat for custom strategies using ``play.ROLE_CACHE``
+        """
+        display.deprecated(
+            'Play.ROLE_CACHE is deprecated in favor of Play.role_cache, or StrategyBase._get_cached_role',
+            version='2.18',
+        )
+        cache = {}
+        for path, roles in self.role_cache.items():
+            for role in roles:
+                name = role.get_name()
+                hashed_params = hash_params(role._get_hash_dict())
+                cache.setdefault(name, {})[hashed_params] = role
+        return cache
+
     def _validate_hosts(self, attribute, name, value):
         # Only validate 'hosts' if a value was passed in to original data set.
         if 'hosts' in self._ds:
@@ -393,7 +409,7 @@ class Play(Base, Taggable, CollectionSearch):
 
     def copy(self):
         new_me = super(Play, self).copy()
-        new_me.ROLE_CACHE = self.ROLE_CACHE.copy()
+        new_me.role_cache = self.role_cache.copy()
         new_me._included_conditional = self._included_conditional
         new_me._included_path = self._included_path
         new_me._action_groups = self._action_groups
diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py
index 33ea6cbfef..b154ca30a3 100644
--- a/lib/ansible/playbook/role/__init__.py
+++ b/lib/ansible/playbook/role/__init__.py
@@ -22,6 +22,7 @@ __metaclass__ = type
 import os
 
 from collections.abc import Container, Mapping, Set, Sequence
+from types import MappingProxyType
 
 from ansible import constants as C
 from ansible.errors import AnsibleError, AnsibleParserError, AnsibleAssertionError
@@ -110,7 +111,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
         self.public = public
         self.static = True
 
-        self._metadata = None
+        self._metadata = RoleMetadata()
         self._play = play
         self._parents = []
         self._dependencies = []
@@ -130,6 +131,8 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
         # Indicates whether this role was included via include/import_role
         self.from_include = from_include
 
+        self._hash = None
+
         super(Role, self).__init__()
 
     def __repr__(self):
@@ -140,36 +143,38 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
             return '.'.join(x for x in (self._role_collection, self._role_name) if x)
         return self._role_name
 
+    def get_role_path(self):
+        # Purposefully using realpath for canonical path
+        return os.path.realpath(self._role_path)
+
+    def _get_hash_dict(self):
+        if self._hash:
+            return self._hash
+        self._hash = MappingProxyType(
+            {
+                'name': self.get_name(),
+                'path': self.get_role_path(),
+                'params': MappingProxyType(self.get_role_params()),
+                'when': self.when,
+                'tags': self.tags,
+                'from_files': MappingProxyType(self._from_files),
+                'vars': MappingProxyType(self.vars),
+                'from_include': self.from_include,
+            }
+        )
+        return self._hash
+
+    def __eq__(self, other):
+        if not isinstance(other, Role):
+            return False
+
+        return self._get_hash_dict() == other._get_hash_dict()
+
     @staticmethod
     def load(role_include, play, parent_role=None, from_files=None, from_include=False, validate=True, public=True):
         if from_files is None:
             from_files = {}
         try:
-            # The ROLE_CACHE is a dictionary of role names, with each entry
-            # containing another dictionary corresponding to a set of parameters
-            # specified for a role as the key and the Role() object itself.
-            # We use frozenset to make the dictionary hashable.
-
-            params = role_include.get_role_params()
-            if role_include.when is not None:
-                params['when'] = role_include.when
-            if role_include.tags is not None:
-                params['tags'] = role_include.tags
-            if from_files is not None:
-                params['from_files'] = from_files
-            if role_include.vars:
-                params['vars'] = role_include.vars
-
-            params['from_include'] = from_include
-
-            hashed_params = hash_params(params)
-            if role_include.get_name() in play.ROLE_CACHE:
-                for (entry, role_obj) in play.ROLE_CACHE[role_include.get_name()].items():
-                    if hashed_params == entry:
-                        if parent_role:
-                            role_obj.add_parent(parent_role)
-                        return role_obj
-
             # TODO: need to fix cycle detection in role load (maybe use an empty dict
             #  for the in-flight in role cache as a sentinel that we're already trying to load
             #  that role?)
@@ -177,11 +182,15 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
             r = Role(play=play, from_files=from_files, from_include=from_include, validate=validate, public=public)
             r._load_role_data(role_include, parent_role=parent_role)
 
-            if role_include.get_name() not in play.ROLE_CACHE:
-                play.ROLE_CACHE[role_include.get_name()] = dict()
+            role_path = r.get_role_path()
+            if role_path not in play.role_cache:
+                play.role_cache[role_path] = []
+
+            # Using the role path as a cache key is done to improve performance when a large number of roles
+            # are in use in the play
+            if r not in play.role_cache[role_path]:
+                play.role_cache[role_path].append(r)
 
-            # FIXME: how to handle cache keys for collection-based roles, since they're technically adjustable per task?
-            play.ROLE_CACHE[role_include.get_name()][hashed_params] = r
             return r
 
         except RuntimeError:
@@ -222,8 +231,6 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
         if metadata:
             self._metadata = RoleMetadata.load(metadata, owner=self, variable_manager=self._variable_manager, loader=self._loader)
             self._dependencies = self._load_dependencies()
-        else:
-            self._metadata = RoleMetadata()
 
         # reset collections list; roles do not inherit collections from parents, just use the defaults
         # FUTURE: use a private config default for this so we can allow it to be overridden later
@@ -422,10 +429,9 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
         '''
 
         deps = []
-        if self._metadata:
-            for role_include in self._metadata.dependencies:
-                r = Role.load(role_include, play=self._play, parent_role=self)
-                deps.append(r)
+        for role_include in self._metadata.dependencies:
+            r = Role.load(role_include, play=self._play, parent_role=self)
+            deps.append(r)
 
         return deps
 
@@ -493,14 +499,14 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
         all_vars = self.get_inherited_vars(dep_chain, only_exports=only_exports)
 
         # get exported variables from meta/dependencies
-        seen = set()
+        seen = []
         for dep in self.get_all_dependencies():
             # Avoid reruning dupe deps since they can have vars from previous invocations and they accumulate in deps
             # TODO: re-examine dep loading to see if we are somehow improperly adding the same dep too many times
             if dep not in seen:
                 # only take 'exportable' vars from deps
                 all_vars = combine_vars(all_vars, dep.get_vars(include_params=False, only_exports=True))
-                seen.add(dep)
+                seen.append(dep)
 
         # role_vars come from vars/ in a role
         all_vars = combine_vars(all_vars, self._role_vars)
@@ -634,8 +640,7 @@ class Role(Base, Conditional, Taggable, CollectionSearch):
         res['_had_task_run'] = self._had_task_run.copy()
         res['_completed'] = self._completed.copy()
 
-        if self._metadata:
-            res['_metadata'] = self._metadata.serialize()
+        res['_metadata'] = self._metadata.serialize()
 
         if include_deps:
             deps = []
diff --git a/lib/ansible/plugins/strategy/__init__.py b/lib/ansible/plugins/strategy/__init__.py
index 2f04a3f73f..90604b3791 100644
--- a/lib/ansible/plugins/strategy/__init__.py
+++ b/lib/ansible/plugins/strategy/__init__.py
@@ -764,11 +764,10 @@ class StrategyBase:
             # If this is a role task, mark the parent role as being run (if
             # the task was ok or failed, but not skipped or unreachable)
             if original_task._role is not None and role_ran:  # TODO:  and original_task.action not in C._ACTION_INCLUDE_ROLE:?
-                # lookup the role in the ROLE_CACHE to make sure we're dealing
+                # lookup the role in the role cache to make sure we're dealing
                 # with the correct object and mark it as executed
-                for (entry, role_obj) in iterator._play.ROLE_CACHE[original_task._role.get_name()].items():
-                    if role_obj._uuid == original_task._role._uuid:
-                        role_obj._had_task_run[original_host.name] = True
+                role_obj = self._get_cached_role(original_task, iterator._play)
+                role_obj._had_task_run[original_host.name] = True
 
             ret_results.append(task_result)
 
@@ -995,9 +994,9 @@ class StrategyBase:
             # Allow users to use this in a play as reported in https://github.com/ansible/ansible/issues/22286?
             # How would this work with allow_duplicates??
             if task.implicit:
-                if target_host.name in task._role._had_task_run:
-                    task._role._completed[target_host.name] = True
-                    msg = 'role_complete for %s' % target_host.name
+                role_obj = self._get_cached_role(task, iterator._play)
+                role_obj._completed[target_host.name] = True
+                msg = 'role_complete for %s' % target_host.name
         elif meta_action == 'reset_connection':
             all_vars = self._variable_manager.get_vars(play=iterator._play, host=target_host, task=task,
                                                        _hosts=self._hosts_cache, _hosts_all=self._hosts_cache_all)
@@ -1061,6 +1060,15 @@ class StrategyBase:
             self._tqm.send_callback('v2_runner_on_skipped', res)
         return [res]
 
+    def _get_cached_role(self, task, play):
+        role_path = task._role.get_role_path()
+        role_cache = play.role_cache[role_path]
+        try:
+            idx = role_cache.index(task._role)
+            return role_cache[idx]
+        except ValueError:
+            raise AnsibleError(f'Cannot locate {task._role.get_name()} in role cache')
+
     def get_hosts_left(self, iterator):
         ''' returns list of available hosts for this iterator by filtering out unreachables '''
 
diff --git a/lib/ansible/plugins/strategy/free.py b/lib/ansible/plugins/strategy/free.py
index 6f45114b1e..6d1aa651d8 100644
--- a/lib/ansible/plugins/strategy/free.py
+++ b/lib/ansible/plugins/strategy/free.py
@@ -173,10 +173,9 @@ class StrategyModule(StrategyBase):
 
                         # check to see if this task should be skipped, due to it being a member of a
                         # role which has already run (and whether that role allows duplicate execution)
-                        if not isinstance(task, Handler) and task._role and task._role.has_run(host):
-                            # If there is no metadata, the default behavior is to not allow duplicates,
-                            # if there is metadata, check to see if the allow_duplicates flag was set to true
-                            if task._role._metadata is None or task._role._metadata and not task._role._metadata.allow_duplicates:
+                        if not isinstance(task, Handler) and task._role:
+                            role_obj = self._get_cached_role(task, iterator._play)
+                            if role_obj.has_run(host) and role_obj._metadata.allow_duplicates is False:
                                 display.debug("'%s' skipped because role has already run" % task, host=host_name)
                                 del self._blocked_hosts[host_name]
                                 continue
diff --git a/lib/ansible/plugins/strategy/linear.py b/lib/ansible/plugins/strategy/linear.py
index dc34e097bb..b2be0a1b10 100644
--- a/lib/ansible/plugins/strategy/linear.py
+++ b/lib/ansible/plugins/strategy/linear.py
@@ -170,10 +170,9 @@ class StrategyModule(StrategyBase):
 
                     # check to see if this task should be skipped, due to it being a member of a
                     # role which has already run (and whether that role allows duplicate execution)
-                    if not isinstance(task, Handler) and task._role and task._role.has_run(host):
-                        # If there is no metadata, the default behavior is to not allow duplicates,
-                        # if there is metadata, check to see if the allow_duplicates flag was set to true
-                        if task._role._metadata is None or task._role._metadata and not task._role._metadata.allow_duplicates:
+                    if not isinstance(task, Handler) and task._role:
+                        role_obj = self._get_cached_role(task, iterator._play)
+                        if role_obj.has_run(host) and role_obj._metadata.allow_duplicates is False:
                             display.debug("'%s' skipped because role has already run" % task)
                             continue
 
diff --git a/test/integration/targets/roles/dupe_inheritance.yml b/test/integration/targets/roles/dupe_inheritance.yml
new file mode 100644
index 0000000000..6fda5baf5c
--- /dev/null
+++ b/test/integration/targets/roles/dupe_inheritance.yml
@@ -0,0 +1,10 @@
+- name: Test
+  hosts: testhost
+  gather_facts: false
+  roles:
+    - role: top
+      info: First definition
+      testvar: abc
+
+    - role: top
+      info: Second definition
diff --git a/test/integration/targets/roles/roles/bottom/tasks/main.yml b/test/integration/targets/roles/roles/bottom/tasks/main.yml
new file mode 100644
index 0000000000..3f375973e9
--- /dev/null
+++ b/test/integration/targets/roles/roles/bottom/tasks/main.yml
@@ -0,0 +1,3 @@
+- name: "{{ info }} - {{ role_name }}: testvar content"
+  debug:
+    msg: '{{ testvar | default("Not specified") }}'
diff --git a/test/integration/targets/roles/roles/middle/tasks/main.yml b/test/integration/targets/roles/roles/middle/tasks/main.yml
new file mode 100644
index 0000000000..bd2b529499
--- /dev/null
+++ b/test/integration/targets/roles/roles/middle/tasks/main.yml
@@ -0,0 +1,6 @@
+- name: "{{ info }} - {{ role_name }}: testvar content"
+  debug:
+    msg: '{{ testvar | default("Not specified") }}'
+
+- include_role:
+    name: bottom
diff --git a/test/integration/targets/roles/roles/top/tasks/main.yml b/test/integration/targets/roles/roles/top/tasks/main.yml
new file mode 100644
index 0000000000..a7a5b529d2
--- /dev/null
+++ b/test/integration/targets/roles/roles/top/tasks/main.yml
@@ -0,0 +1,6 @@
+- name: "{{ info }} - {{ role_name }}: testvar content"
+  debug:
+    msg: '{{ testvar | default("Not specified") }}'
+
+- include_role:
+    name: middle
diff --git a/test/integration/targets/roles/runme.sh b/test/integration/targets/roles/runme.sh
index 1e154b795a..f6902d6344 100755
--- a/test/integration/targets/roles/runme.sh
+++ b/test/integration/targets/roles/runme.sh
@@ -14,6 +14,7 @@ set -eux
 [ "$(ansible-playbook allowed_dupes.yml -i ../../inventory --tags importrole "$@" | grep -c '"msg": "A"')" = "2" ]
 [ "$(ansible-playbook allowed_dupes.yml -i ../../inventory --tags includerole "$@" | grep -c '"msg": "A"')" = "2" ]
 
+[ "$(ansible-playbook dupe_inheritance.yml -i ../../inventory "$@" | grep -c '"msg": "abc"')" = "3" ]
 
 # ensure role data is merged correctly
 ansible-playbook data_integrity.yml -i ../../inventory "$@"
diff --git a/test/units/playbook/role/test_role.py b/test/units/playbook/role/test_role.py
index 5d47631fe2..fadce8f5dc 100644
--- a/test/units/playbook/role/test_role.py
+++ b/test/units/playbook/role/test_role.py
@@ -177,7 +177,7 @@ class TestRole(unittest.TestCase):
         })
 
         mock_play = MagicMock()
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load('foo_tasks', play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play)
@@ -199,7 +199,7 @@ class TestRole(unittest.TestCase):
         })
 
         mock_play = MagicMock()
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load('foo_tasks', play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play, from_files=dict(tasks='custom_main'))
@@ -217,7 +217,7 @@ class TestRole(unittest.TestCase):
         })
 
         mock_play = MagicMock()
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load('foo_handlers', play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play)
@@ -238,7 +238,7 @@ class TestRole(unittest.TestCase):
         })
 
         mock_play = MagicMock()
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load('foo_vars', play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play)
@@ -259,7 +259,7 @@ class TestRole(unittest.TestCase):
         })
 
         mock_play = MagicMock()
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load('foo_vars', play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play)
@@ -280,7 +280,7 @@ class TestRole(unittest.TestCase):
         })
 
         mock_play = MagicMock()
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load('foo_vars', play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play)
@@ -303,7 +303,7 @@ class TestRole(unittest.TestCase):
         })
 
         mock_play = MagicMock()
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load('foo_vars', play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play)
@@ -323,7 +323,7 @@ class TestRole(unittest.TestCase):
         })
 
         mock_play = MagicMock()
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load('foo_vars', play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play)
@@ -370,7 +370,7 @@ class TestRole(unittest.TestCase):
 
         mock_play = MagicMock()
         mock_play.collections = None
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load('foo_metadata', play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play)
@@ -415,7 +415,7 @@ class TestRole(unittest.TestCase):
         })
 
         mock_play = MagicMock()
-        mock_play.ROLE_CACHE = {}
+        mock_play.role_cache = {}
 
         i = RoleInclude.load(dict(role='foo_complex'), play=mock_play, loader=fake_loader)
         r = Role.load(i, play=mock_play)