diff --git a/changelogs/fragments/always-create-new-role.yml b/changelogs/fragments/always-create-new-role.yml new file mode 100644 index 00000000000..87209ccc07d --- /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 23bb36b2bf6..ec9f78c651a 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 33ea6cbfef6..b154ca30a37 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 2f04a3f73ff..90604b3791d 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 6f45114b1e2..6d1aa651d8b 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 dc34e097bbe..b2be0a1b108 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 00000000000..6fda5baf5ca --- /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 00000000000..3f375973e9e --- /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 00000000000..bd2b5294994 --- /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 00000000000..a7a5b529d24 --- /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 1e154b795a2..f6902d63441 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 5d47631fe26..fadce8f5dce 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)