From cae682607cdbe710a51ae5181bbc84ee06f3f33e Mon Sep 17 00:00:00 2001 From: James Cammarata Date: Wed, 18 Jan 2017 10:17:10 -0600 Subject: [PATCH] Reworking the way end of role detection is done Rather than trying to enumerate tasks or track an ever changing cur_role flag in PlayIterator, this change simply sets a flag on the last block in the list of blocks returned by Role.compile(). The PlayIterator then checks for that flag when the cur_block number is incremented, and marks the role as complete if the given host had any tasks run in that role. Fixes #20224 --- lib/ansible/executor/play_iterator.py | 104 ++-------------------- lib/ansible/playbook/block.py | 6 ++ lib/ansible/playbook/role/__init__.py | 4 +- test/units/executor/test_play_iterator.py | 41 ++++++++- 4 files changed, 56 insertions(+), 99 deletions(-) diff --git a/lib/ansible/executor/play_iterator.py b/lib/ansible/executor/play_iterator.py index 702dd7a5c7..02052f56b3 100644 --- a/lib/ansible/executor/play_iterator.py +++ b/lib/ansible/executor/play_iterator.py @@ -48,8 +48,6 @@ class HostState: self.cur_regular_task = 0 self.cur_rescue_task = 0 self.cur_always_task = 0 - self.cur_role = None - self.cur_role_task = None self.cur_dep_chain = None self.run_state = PlayIterator.ITERATING_SETUP self.fail_state = PlayIterator.FAILED_NONE @@ -82,12 +80,11 @@ class HostState: ret.append(states[i]) return "|".join(ret) - return "HOST STATE: block=%d, task=%d, rescue=%d, always=%d, role=%s, run_state=%s, fail_state=%s, pending_setup=%s, tasks child state? (%s), rescue child state? (%s), always child state? (%s), did rescue? %s, did start at task? %s" % ( + return "HOST STATE: block=%d, task=%d, rescue=%d, always=%d, run_state=%s, fail_state=%s, pending_setup=%s, tasks child state? (%s), rescue child state? (%s), always child state? (%s), did rescue? %s, did start at task? %s" % ( self.cur_block, self.cur_regular_task, self.cur_rescue_task, self.cur_always_task, - self.cur_role, _run_state_to_string(self.run_state), _failed_state_to_string(self.fail_state), self.pending_setup, @@ -104,7 +101,7 @@ class HostState: for attr in ( '_blocks', 'cur_block', 'cur_regular_task', 'cur_rescue_task', 'cur_always_task', - 'cur_role', 'run_state', 'fail_state', 'pending_setup', 'cur_dep_chain', + 'run_state', 'fail_state', 'pending_setup', 'cur_dep_chain', 'tasks_child_state', 'rescue_child_state', 'always_child_state' ): if getattr(self, attr) != getattr(other, attr): @@ -121,9 +118,6 @@ class HostState: new_state.cur_regular_task = self.cur_regular_task new_state.cur_rescue_task = self.cur_rescue_task new_state.cur_always_task = self.cur_always_task - new_state.cur_role = self.cur_role - if self.cur_role_task: - new_state.cur_role_task = self.cur_role_task[:] new_state.run_state = self.run_state new_state.fail_state = self.fail_state new_state.pending_setup = self.pending_setup @@ -272,92 +266,6 @@ class PlayIterator: old_s = s (s, task) = self._get_next_task_from_state(s, host=host, peek=peek) - def _roles_are_different(ra, rb): - if ra != rb: - return True - else: - return old_s.cur_dep_chain != task.get_dep_chain() - - def _role_is_child(r): - parent = task._parent - while parent: - if hasattr(parent, '_role') and parent._role == r and isinstance(parent, IncludeRole): - return True - parent = parent._parent - return False - - def _get_cur_task(s, depth=0): - res = [s.run_state, depth, s.cur_block, -1] - if s.run_state == self.ITERATING_TASKS: - if s.tasks_child_state: - res[-1] = [s.cur_regular_task, _get_cur_task(s.tasks_child_state, depth=depth+1)] - else: - res[-1] = s.cur_regular_task - elif s.run_state == self.ITERATING_RESCUE: - if s.rescue_child_state: - res[-1] = [s.cur_rescue_task, _get_cur_task(s.rescue_child_state, depth=depth+1)] - else: - res[-1] = s.cur_rescue_task - elif s.run_state == self.ITERATING_ALWAYS: - if s.always_child_state: - res[-1] = [s.cur_always_task, _get_cur_task(s.always_child_state, depth=depth+1)] - else: - res[-1] = s.cur_always_task - return res - - def _do_task_cmp(a, b): - ''' - Does the heavy lifting for _role_task_cmp() of comparing task state objects - returned by _get_cur_task() above. - ''' - res = cmp(a[0], b[0]) - if res == 0: - res = cmp(a[1], b[1]) - if res == 0: - res = cmp(a[2], b[2]) - if res == 0: - # if there were child states, the last value in the list may be - # a list itself representing the current task position plus a new - # list representing the child state. So here we normalize that so - # we can call this method recursively when all else is equal. - if isinstance(a[3], list): - a_i, a_il = a[3] - else: - a_i = a[3] - a_il = [-1, -1, -1, -1] - if isinstance(b[3], list): - b_i, b_il = b[3] - else: - b_i = b[3] - b_il = [-1, -1, -1, -1] - - res = cmp(a_i, b_i) - if res == 0: - res = _do_task_cmp(a_il, b_il) - return res - - def _role_task_cmp(s): - ''' - Compares the given state against the stored state from the previous role task. - ''' - if not s.cur_role_task: - return 1 - cur_task = _get_cur_task(s) - return _do_task_cmp(cur_task, s.cur_role_task) - - if task and task._role: - # if we had a current role, mark that role as completed - if s.cur_role: - role_diff = _roles_are_different(task._role, s.cur_role) - role_child = _role_is_child(s.cur_role) - tasks_cmp = _role_task_cmp(s) - host_done = host.name in s.cur_role._had_task_run - if (role_diff or (not role_diff and tasks_cmp <= 0)) and host_done and not role_child and not peek: - s.cur_role._completed[host.name] = True - s.cur_role = task._role - s.cur_role_task = _get_cur_task(s) - s.cur_dep_chain = task.get_dep_chain() - if not peek: self._host_states[host.name] = s @@ -462,7 +370,6 @@ class PlayIterator: if isinstance(task, Block) or state.tasks_child_state is not None: state.tasks_child_state = HostState(blocks=[task]) state.tasks_child_state.run_state = self.ITERATING_TASKS - state.tasks_child_state.cur_role = state.cur_role # since we've created the child state, clear the task # so we can pick up the child state on the next pass task = None @@ -493,7 +400,6 @@ class PlayIterator: if isinstance(task, Block) or state.rescue_child_state is not None: state.rescue_child_state = HostState(blocks=[task]) state.rescue_child_state.run_state = self.ITERATING_TASKS - state.rescue_child_state.cur_role = state.cur_role task = None state.cur_rescue_task += 1 @@ -525,12 +431,16 @@ class PlayIterator: state.rescue_child_state = None state.always_child_state = None state.did_rescue = False + + # we're advancing blocks, so if this was an end-of-role block we + # mark the current role complete + if block._eor and host.name in block._role._had_task_run: + block._role._completed[host.name] = True else: task = block.always[state.cur_always_task] if isinstance(task, Block) or state.always_child_state is not None: state.always_child_state = HostState(blocks=[task]) state.always_child_state.run_state = self.ITERATING_TASKS - state.always_child_state.cur_role = state.cur_role task = None state.cur_always_task += 1 diff --git a/lib/ansible/playbook/block.py b/lib/ansible/playbook/block.py index bbb287420b..5b9585238c 100644 --- a/lib/ansible/playbook/block.py +++ b/lib/ansible/playbook/block.py @@ -53,6 +53,9 @@ class Block(Base, Become, Conditional, Taggable): self._use_handlers = use_handlers self._implicit = implicit + # end of role flag + self._eor = False + if task_include: self._parent = task_include elif parent_block: @@ -182,6 +185,7 @@ 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._eor = self._eor if self._dep_chain is not None: new_me._dep_chain = self._dep_chain[:] @@ -214,6 +218,7 @@ class Block(Base, Become, Conditional, Taggable): data[attr] = getattr(self, attr) data['dep_chain'] = self.get_dep_chain() + data['eor'] = self._eor if self._role is not None: data['role'] = self._role.serialize() @@ -241,6 +246,7 @@ class Block(Base, Become, Conditional, Taggable): setattr(self, attr, data.get(attr)) self._dep_chain = data.get('dep_chain', None) + self._eor = data.get('eor', False) # if there was a serialized role, unpack it too role_data = data.get('role') diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py index 2fea450428..ab2a657d62 100644 --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py @@ -408,12 +408,14 @@ class Role(Base, Become, Conditional, Taggable): dep_blocks = dep.compile(play=play, dep_chain=new_dep_chain) block_list.extend(dep_blocks) - for task_block in self._task_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._dep_chain = new_dep_chain new_task_block._play = play + if idx == len(self._task_blocks) - 1: + new_task_block._eor = True block_list.append(new_task_block) return block_list diff --git a/test/units/executor/test_play_iterator.py b/test/units/executor/test_play_iterator.py index 671deaf279..e11772031f 100644 --- a/test/units/executor/test_play_iterator.py +++ b/test/units/executor/test_play_iterator.py @@ -87,8 +87,22 @@ class TestPlayIterator(unittest.TestCase): - debug: msg="this is a post_task" """, '/etc/ansible/roles/test_role/tasks/main.yml': """ - - debug: msg="this is a role task" + - name: role task + debug: msg="this is a role task" + - block: + - name: role block task + debug: msg="inside block in role" + always: + - name: role always task + debug: msg="always task in block in role" + - include: foo.yml + - name: role task after include + debug: msg="after include in role" """, + '/etc/ansible/roles/test_role/tasks/foo.yml': """ + - name: role included task + debug: msg="this is task in an include from a role" + """ }) mock_var_manager = MagicMock() @@ -141,6 +155,31 @@ class TestPlayIterator(unittest.TestCase): (host_state, task) = itr.get_next_task_for_host(hosts[0]) self.assertIsNotNone(task) self.assertEqual(task.action, 'debug') + self.assertEqual(task.name, "role task") + self.assertIsNotNone(task._role) + # role block task + (host_state, task) = itr.get_next_task_for_host(hosts[0]) + self.assertIsNotNone(task) + self.assertEqual(task.action, 'debug') + self.assertEqual(task.name, "role block task") + self.assertIsNotNone(task._role) + # role block always task + (host_state, task) = itr.get_next_task_for_host(hosts[0]) + self.assertIsNotNone(task) + self.assertEqual(task.action, 'debug') + self.assertEqual(task.name, "role always task") + self.assertIsNotNone(task._role) + # role include task + #(host_state, task) = itr.get_next_task_for_host(hosts[0]) + #self.assertIsNotNone(task) + #self.assertEqual(task.action, 'debug') + #self.assertEqual(task.name, "role included task") + #self.assertIsNotNone(task._role) + # role task after include + (host_state, task) = itr.get_next_task_for_host(hosts[0]) + self.assertIsNotNone(task) + self.assertEqual(task.action, 'debug') + self.assertEqual(task.name, "role task after include") self.assertIsNotNone(task._role) # regular play task (host_state, task) = itr.get_next_task_for_host(hosts[0])