From d6872a7b070d1179e7d76bcda1490bb7003c4574 Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Fri, 20 Apr 2018 12:31:56 -0500 Subject: [PATCH] Backport #38747 for 2.5 - Block copy and reparenting improvements (#38829) * Attempt 4: Prevent reparenting a block with itself (#38747) * More concisely reparent, ensuring we don't go too shallow or too deep in this process. Fixes #38357 * More explicit reparenting, with a short circuit for a common case * We need new_block to have a parent, otherwise we lose context with this approach * Remove duplicate parent assignment * Change callers of Block.copy to not use exclude_parent=True, when including the parent, exclude tasks (cherry picked from commit f474195a3b9f3be7ed7bac4d57deb41372a58850) * Add changelog for #38747 --- .../dyanmic-include-copy-enhancements.yaml | 2 ++ lib/ansible/executor/play_iterator.py | 6 ++-- lib/ansible/playbook/block.py | 29 +++++++++---------- lib/ansible/playbook/role/__init__.py | 4 +-- 4 files changed, 20 insertions(+), 21 deletions(-) create mode 100644 changelogs/fragments/dyanmic-include-copy-enhancements.yaml diff --git a/changelogs/fragments/dyanmic-include-copy-enhancements.yaml b/changelogs/fragments/dyanmic-include-copy-enhancements.yaml new file mode 100644 index 00000000000..e6439f6d56b --- /dev/null +++ b/changelogs/fragments/dyanmic-include-copy-enhancements.yaml @@ -0,0 +1,2 @@ +bugfixes: +- dynamic includes - Improved performance by fixing re-parenting on copy (https://github.com/ansible/ansible/pull/38747) diff --git a/lib/ansible/executor/play_iterator.py b/lib/ansible/executor/play_iterator.py index e9f6a0c47b4..faa54254623 100644 --- a/lib/ansible/executor/play_iterator.py +++ b/lib/ansible/executor/play_iterator.py @@ -524,7 +524,7 @@ class PlayIterator: if state.tasks_child_state: state.tasks_child_state = self._insert_tasks_into_state(state.tasks_child_state, task_list) else: - target_block = state._blocks[state.cur_block].copy(exclude_parent=True) + target_block = state._blocks[state.cur_block].copy() before = target_block.block[:state.cur_regular_task] after = target_block.block[state.cur_regular_task:] target_block.block = before + task_list + after @@ -533,7 +533,7 @@ class PlayIterator: if state.rescue_child_state: state.rescue_child_state = self._insert_tasks_into_state(state.rescue_child_state, task_list) else: - target_block = state._blocks[state.cur_block].copy(exclude_parent=True) + target_block = state._blocks[state.cur_block].copy() before = target_block.rescue[:state.cur_rescue_task] after = target_block.rescue[state.cur_rescue_task:] target_block.rescue = before + task_list + after @@ -542,7 +542,7 @@ class PlayIterator: if state.always_child_state: state.always_child_state = self._insert_tasks_into_state(state.always_child_state, task_list) else: - target_block = state._blocks[state.cur_block].copy(exclude_parent=True) + target_block = state._blocks[state.cur_block].copy() before = target_block.always[:state.cur_always_task] after = target_block.always[state.cur_always_task:] target_block.always = before + task_list + after diff --git a/lib/ansible/playbook/block.py b/lib/ansible/playbook/block.py index 6ceaae1e160..9bbe8883859 100644 --- a/lib/ansible/playbook/block.py +++ b/lib/ansible/playbook/block.py @@ -69,6 +69,10 @@ class Block(Base, Become, Conditional, Taggable): '''object comparison based on _uuid''' return self._uuid == other._uuid + def __ne__(self, other): + '''object comparison based on _uuid''' + return self._uuid != other._uuid + def get_vars(self): ''' Blocks do not store variables directly, however they may be a member @@ -173,21 +177,16 @@ class Block(Base, Become, Conditional, Taggable): new_task = task.copy(exclude_parent=True) if task._parent: new_task._parent = task._parent.copy(exclude_tasks=True) - # go up the parentage tree until we find an - # object without a parent and make this new - # block their parent - cur_obj = new_task - while cur_obj._parent: - if cur_obj._parent: - prev_obj = cur_obj - cur_obj = cur_obj._parent - - # Ensure that we don't make the new_block the parent of itself - if cur_obj != new_block: - cur_obj._parent = new_block + if task._parent == new_block: + # If task._parent is the same as new_block, just replace it + new_task._parent = new_block else: - # prev_obj._parent is cur_obj, to allow for mutability we need to use prev_obj - prev_obj._parent = new_block + # task may not be a direct child of new_block, search for the correct place to insert new_block + cur_obj = new_task._parent + while cur_obj._parent and cur_obj._parent != new_block: + cur_obj = cur_obj._parent + + cur_obj._parent = new_block else: new_task._parent = new_block new_task_list.append(new_task) @@ -203,7 +202,7 @@ class Block(Base, Become, Conditional, Taggable): new_me._parent = None if self._parent and not exclude_parent: - new_me._parent = self._parent.copy(exclude_tasks=exclude_tasks) + new_me._parent = self._parent.copy(exclude_tasks=True) if not exclude_tasks: new_me.block = _dupe_task_list(self.block or [], new_me) diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py index 90d6abb14fe..7f549752e12 100644 --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py @@ -422,9 +422,7 @@ class Role(Base, Become, Conditional, Taggable): block_list.extend(dep_blocks) for idx, task_block in enumerate(self._task_blocks): - new_task_block = task_block.copy(exclude_parent=True) - if task_block._parent: - new_task_block._parent = task_block._parent.copy() + new_task_block = task_block.copy() new_task_block._dep_chain = new_dep_chain new_task_block._play = play if idx == len(self._task_blocks) - 1: