From 275728e0f6487153be939049b83a2d3dcd5fdff2 Mon Sep 17 00:00:00 2001 From: James Cammarata Date: Wed, 10 Feb 2016 21:27:14 -0500 Subject: [PATCH] Fixing bugs related to nested blocks inside roles * Make sure dep chains are checked recursively for nested blocks * Fixing iterator is_failed() check to make sure we're not in a rescue block before returning True * Use is_failed() to test whether a host should be added to the TQM failed_hosts list * Use is_failed() when compiling the list of hosts left to iterate over in both the linear and free strategies Fixes #14222 --- lib/ansible/executor/play_iterator.py | 12 ++++--- lib/ansible/playbook/block.py | 43 +++++++++++++++--------- lib/ansible/playbook/role/__init__.py | 9 ++--- lib/ansible/plugins/strategy/__init__.py | 6 ++-- lib/ansible/plugins/strategy/free.py | 2 +- lib/ansible/plugins/strategy/linear.py | 2 +- 6 files changed, 46 insertions(+), 28 deletions(-) diff --git a/lib/ansible/executor/play_iterator.py b/lib/ansible/executor/play_iterator.py index 2341c679c2..bd36b5a417 100644 --- a/lib/ansible/executor/play_iterator.py +++ b/lib/ansible/executor/play_iterator.py @@ -198,7 +198,7 @@ class PlayIterator: task = None if s.run_state == self.ITERATING_COMPLETE: display.debug("host %s is done iterating, returning" % host.name) - return (None, None) + return (s, None) old_s = s (s, task) = self._get_next_task_from_state(s, host=host, peek=peek) @@ -207,14 +207,14 @@ class PlayIterator: if ra != rb: return True else: - return old_s.cur_dep_chain != task._block._dep_chain + return old_s.cur_dep_chain != task._block.get_dep_chain() if task and task._role: # if we had a current role, mark that role as completed if s.cur_role and _roles_are_different(task._role, s.cur_role) and host.name in s.cur_role._had_task_run and not peek: s.cur_role._completed[host.name] = True s.cur_role = task._role - s.cur_dep_chain = task._block._dep_chain + s.cur_dep_chain = task._block.get_dep_chain() if not peek: self._host_states[host.name] = s @@ -417,7 +417,11 @@ class PlayIterator: else: return True elif state.run_state == self.ITERATING_TASKS and self._check_failed_state(state.tasks_child_state): - return True + cur_block = self._blocks[state.cur_block] + if len(cur_block.rescue) > 0 and state.fail_state & self.FAILED_RESCUE == 0: + return False + else: + return True elif state.run_state == self.ITERATING_RESCUE and self._check_failed_state(state.rescue_child_state): return True elif state.run_state == self.ITERATING_ALWAYS and self._check_failed_state(state.always_child_state): diff --git a/lib/ansible/playbook/block.py b/lib/ansible/playbook/block.py index 095e6b338d..b16316ab2e 100644 --- a/lib/ansible/playbook/block.py +++ b/lib/ansible/playbook/block.py @@ -46,6 +46,7 @@ class Block(Base, Become, Conditional, Taggable): self._role = role self._task_include = None self._parent_block = None + self._dep_chain = None self._use_handlers = use_handlers self._implicit = implicit @@ -54,11 +55,6 @@ class Block(Base, Become, Conditional, Taggable): elif parent_block: self._parent_block = parent_block - if parent_block: - self._dep_chain = parent_block._dep_chain[:] - else: - self._dep_chain = [] - super(Block, self).__init__() def get_vars(self): @@ -153,6 +149,15 @@ class Block(Base, Become, Conditional, Taggable): except AssertionError: raise AnsibleParserError("A malformed block was encountered.", obj=self._ds) + def get_dep_chain(self): + if self._dep_chain is None: + if self._parent_block: + return self._parent_block.get_dep_chain() + else: + return None + else: + return self._dep_chain[:] + def copy(self, exclude_parent=False, exclude_tasks=False): def _dupe_task_list(task_list, new_block): new_task_list = [] @@ -169,7 +174,9 @@ class Block(Base, Become, Conditional, Taggable): new_me = super(Block, self).copy() new_me._play = self._play new_me._use_handlers = self._use_handlers - new_me._dep_chain = self._dep_chain[:] + + if self._dep_chain: + new_me._dep_chain = self._dep_chain[:] if not exclude_tasks: new_me.block = _dupe_task_list(self.block or [], new_me) @@ -201,7 +208,7 @@ class Block(Base, Become, Conditional, Taggable): if attr not in ('block', 'rescue', 'always'): data[attr] = getattr(self, attr) - data['dep_chain'] = self._dep_chain + data['dep_chain'] = self.get_dep_chain() if self._role is not None: data['role'] = self._role.serialize() @@ -226,7 +233,7 @@ class Block(Base, Become, Conditional, Taggable): if attr in data and attr not in ('block', 'rescue', 'always'): setattr(self, attr, data.get(attr)) - self._dep_chain = data.get('dep_chain', []) + self._dep_chain = data.get('dep_chain', None) # if there was a serialized role, unpack it too role_data = data.get('role') @@ -247,10 +254,12 @@ class Block(Base, Become, Conditional, Taggable): pb = Block() pb.deserialize(pb_data) self._parent_block = pb + self._dep_chain = self._parent_block.get_dep_chain() def evaluate_conditional(self, templar, all_vars): - if len(self._dep_chain): - for dep in self._dep_chain: + dep_chain = self.get_dep_chain() + if dep_chain: + for dep in dep_chain: if not dep.evaluate_conditional(templar, all_vars): return False if self._task_include is not None: @@ -274,8 +283,10 @@ class Block(Base, Become, Conditional, Taggable): if self._task_include: self._task_include.set_loader(loader) - for dep in self._dep_chain: - dep.set_loader(loader) + dep_chain = self.get_dep_chain() + if dep_chain: + for dep in dep_chain: + dep.set_loader(loader) def _get_parent_attribute(self, attr, extend=False): ''' @@ -305,10 +316,10 @@ class Block(Base, Become, Conditional, Taggable): else: value = parent_value - if len(self._dep_chain) and (value is None or extend): - reverse_dep_chain = self._dep_chain[:] - reverse_dep_chain.reverse() - for dep in reverse_dep_chain: + dep_chain = self.get_dep_chain() + if dep_chain and (value is None or extend): + dep_chain.reverse() + for dep in dep_chain: dep_value = getattr(dep, attr, None) if extend: value = self._extend_value(value, dep_value) diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py index f192ea6c94..9b406ae7ba 100644 --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py @@ -262,10 +262,11 @@ class Role(Base, Become, Conditional, Taggable): def get_inherited_vars(self, dep_chain=[], include_params=True): inherited_vars = dict() - for parent in dep_chain: - inherited_vars = combine_vars(inherited_vars, parent._role_vars) - if include_params: - inherited_vars = combine_vars(inherited_vars, parent._role_params) + if dep_chain: + for parent in dep_chain: + inherited_vars = combine_vars(inherited_vars, parent._role_vars) + if include_params: + inherited_vars = combine_vars(inherited_vars, parent._role_params) return inherited_vars def get_role_params(self): diff --git a/lib/ansible/plugins/strategy/__init__.py b/lib/ansible/plugins/strategy/__init__.py index 34db52a77d..49c1bdfb35 100644 --- a/lib/ansible/plugins/strategy/__init__.py +++ b/lib/ansible/plugins/strategy/__init__.py @@ -210,8 +210,10 @@ class StrategyBase: [iterator.mark_host_failed(h) for h in self._inventory.get_hosts(iterator._play.hosts) if h.name not in self._tqm._unreachable_hosts] else: iterator.mark_host_failed(host) - (state, tmp_task) = iterator.get_next_task_for_host(host, peek=True) - if not state or state.run_state != PlayIterator.ITERATING_RESCUE: + + # only add the host to the failed list officially if it has + # been failed by the iterator + if iterator.is_failed(host): self._tqm._failed_hosts[host.name] = True self._tqm._stats.increment('failures', host.name) else: diff --git a/lib/ansible/plugins/strategy/free.py b/lib/ansible/plugins/strategy/free.py index 5431f96f46..74ac9743b7 100644 --- a/lib/ansible/plugins/strategy/free.py +++ b/lib/ansible/plugins/strategy/free.py @@ -58,7 +58,7 @@ class StrategyModule(StrategyBase): work_to_do = True while work_to_do and not self._tqm._terminated: - hosts_left = [host for host in self._inventory.get_hosts(iterator._play.hosts) if host.name not in self._tqm._unreachable_hosts] + hosts_left = [host for host in self._inventory.get_hosts(iterator._play.hosts) if host.name not in self._tqm._unreachable_hosts and not iterator.is_failed(host)] if len(hosts_left) == 0: self._tqm.send_callback('v2_playbook_on_no_hosts_remaining') result = False diff --git a/lib/ansible/plugins/strategy/linear.py b/lib/ansible/plugins/strategy/linear.py index 846c3d0cf3..0d4faf6149 100644 --- a/lib/ansible/plugins/strategy/linear.py +++ b/lib/ansible/plugins/strategy/linear.py @@ -162,7 +162,7 @@ class StrategyModule(StrategyBase): try: display.debug("getting the remaining hosts for this loop") - hosts_left = [host for host in self._inventory.get_hosts(iterator._play.hosts) if host.name not in self._tqm._unreachable_hosts] + hosts_left = [host for host in self._inventory.get_hosts(iterator._play.hosts) if host.name not in self._tqm._unreachable_hosts and not iterator.is_failed(host)] display.debug("done getting the remaining hosts for this loop") # queue up this task for each host in the inventory