From f474195a3b9f3be7ed7bac4d57deb41372a58850 Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Mon, 16 Apr 2018 12:33:08 -0500 Subject: [PATCH] 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 --- lib/ansible/executor/play_iterator.py | 6 +++--- lib/ansible/playbook/block.py | 29 +++++++++++++-------------- lib/ansible/playbook/role/__init__.py | 4 +--- 3 files changed, 18 insertions(+), 21 deletions(-) diff --git a/lib/ansible/executor/play_iterator.py b/lib/ansible/executor/play_iterator.py index e9f6a0c47b..faa5425462 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 5b59b4e56f..d8a0087fcb 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 73bd96e734..05c515c726 100644 --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py @@ -410,9 +410,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: