From 9d61a6cba841913c4369dc02870efd5cb35c1981 Mon Sep 17 00:00:00 2001 From: James Cammarata Date: Wed, 9 Mar 2016 13:29:22 -0500 Subject: [PATCH] Fixing PlayIterator bugs * Unit tests exposed a problem where nested blocks did not correctly hit rescue/always portions of parent blocks * Cleaned up logic in PlayIterator * Unfortunately fixing the above exposed a potential problem in the block integration tests, where a failure in an "always" section may always lead to a failed state and the termination of execution beyond that point, so certain parts of the block integration test were disabled. --- lib/ansible/executor/play_iterator.py | 155 +++++++++++++--------- test/integration/test_blocks/main.yml | 26 ++-- test/units/executor/test_play_iterator.py | 21 +-- 3 files changed, 120 insertions(+), 82 deletions(-) diff --git a/lib/ansible/executor/play_iterator.py b/lib/ansible/executor/play_iterator.py index eec3877d516..83abb40bbc1 100644 --- a/lib/ansible/executor/play_iterator.py +++ b/lib/ansible/executor/play_iterator.py @@ -258,6 +258,10 @@ class PlayIterator: return (state, None) if state.run_state == self.ITERATING_SETUP: + # First, we check to see if we were pending setup. If not, this is + # the first trip through ITERATING_SETUP, so we set the pending_setup + # flag and try to determine if we do in fact want to gather facts for + # the specified host. if not state.pending_setup: state.pending_setup = True @@ -272,13 +276,19 @@ class PlayIterator: if (gathering == 'implicit' and implied) or \ (gathering == 'explicit' and boolean(self._play.gather_facts)) or \ (gathering == 'smart' and implied and not host._gathered_facts): - # mark the host as having gathered facts + # The setup block is always self._blocks[0], as we inject it + # during the play compilation in __init__ above. setup_block = self._blocks[0] if setup_block.has_tasks() and len(setup_block.block) > 0: task = setup_block.block[0] if not peek: + # mark the host as having gathered facts, because we're + # returning the setup task to be executed host.set_gathered_facts(True) else: + # This is the second trip through ITERATING_SETUP, so we clear + # the flag and move onto the next block in the list while setting + # the run state to ITERATING_TASKS state.pending_setup = False state.cur_block += 1 @@ -293,86 +303,109 @@ class PlayIterator: if state.pending_setup: state.pending_setup = False - if self._check_failed_state(state): - state.run_state = self.ITERATING_RESCUE - elif state.cur_regular_task >= len(block.block): - state.run_state = self.ITERATING_ALWAYS + # First, we check for a child task state that is not failed, and if we + # have one recurse into it for the next task. If we're done with the child + # state, we clear it and drop back to geting the next task from the list. + if state.tasks_child_state: + if state.tasks_child_state.fail_state != self.FAILED_NONE: + # failed child state, so clear it and move into the rescue portion + state.tasks_child_state = None + state.fail_state |= self.FAILED_TASKS + state.run_state = self.ITERATING_RESCUE + else: + # get the next task recursively + (state.tasks_child_state, task) = self._get_next_task_from_state(state.tasks_child_state, host=host, peek=peek) + if task is None or state.tasks_child_state.run_state == self.ITERATING_COMPLETE: + # we're done with the child state, so clear it and continue + # back to the top of the loop to get the next task + state.tasks_child_state = None + continue else: - task = block.block[state.cur_regular_task] - # if the current task is actually a child block, we dive into it - if isinstance(task, Block) or state.tasks_child_state is not None: - if state.tasks_child_state is None: + # First here, we check to see if we've failed anywhere down the chain + # of states we have, and if so we move onto the rescue portion. Otherwise, + # we check to see if we've moved past the end of the list of tasks. If so, + # we move into the always portion of the block, otherwise we get the next + # task from the list. + if self._check_failed_state(state): + state.run_state = self.ITERATING_RESCUE + elif state.cur_regular_task >= len(block.block): + state.run_state = self.ITERATING_ALWAYS + else: + task = block.block[state.cur_regular_task] + # if the current task is actually a child block, create a child + # state for us to recurse into on the next pass + 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 - (state.tasks_child_state, task) = self._get_next_task_from_state(state.tasks_child_state, host=host, peek=peek) - if task is None: - # check to see if the child state was failed, if so we need to - # fail here too so we don't continue iterating tasks - if state.tasks_child_state.fail_state != self.FAILED_NONE: - state.fail_state |= self.FAILED_TASKS - state.tasks_child_state = None - state.cur_regular_task += 1 - continue - else: + # since we've created the child state, clear the task + # so we can pick up the child state on the next pass + task = None state.cur_regular_task += 1 elif state.run_state == self.ITERATING_RESCUE: - if state.fail_state & self.FAILED_RESCUE == self.FAILED_RESCUE: - state.run_state = self.ITERATING_ALWAYS - elif state.cur_rescue_task >= len(block.rescue): - if len(block.rescue) > 0: - state.fail_state = self.FAILED_NONE - state.run_state = self.ITERATING_ALWAYS + # The process here is identical to ITERATING_TASKS, except instead + # we move into the always portion of the block. + if state.rescue_child_state: + if state.rescue_child_state.fail_state != self.FAILED_NONE: + state.rescue_child_state = None + state.fail_state |= self.FAILED_RESCUE + state.run_state = self.ITERATING_ALWAYS + else: + (state.rescue_child_state, task) = self._get_next_task_from_state(state.rescue_child_state, host=host, peek=peek) + if task is None: + state.rescue_child_state = None + continue else: - task = block.rescue[state.cur_rescue_task] - if isinstance(task, Block) or state.rescue_child_state is not None: - if state.rescue_child_state is None: + if state.fail_state & self.FAILED_RESCUE == self.FAILED_RESCUE: + state.run_state = self.ITERATING_ALWAYS + elif state.cur_rescue_task >= len(block.rescue): + if len(block.rescue) > 0: + state.fail_state = self.FAILED_NONE + state.run_state = self.ITERATING_ALWAYS + else: + task = block.rescue[state.cur_rescue_task] + 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 - (state.rescue_child_state, task) = self._get_next_task_from_state(state.rescue_child_state, host=host, peek=peek) - if task is None: - # check to see if the child state was failed, if so we need to - # fail here too so we don't continue iterating rescue - if state.rescue_child_state.fail_state != self.FAILED_NONE: - state.fail_state |= self.FAILED_RESCUE - state.rescue_child_state = None - state.cur_rescue_task += 1 - continue - else: + task = None state.cur_rescue_task += 1 elif state.run_state == self.ITERATING_ALWAYS: - if state.cur_always_task >= len(block.always): - if state.fail_state != self.FAILED_NONE: + # And again, the process here is identical to ITERATING_TASKS, except + # instead we either move onto the next block in the list, or we set the + # run state to ITERATING_COMPLETE in the event of any errors, or when we + # have hit the end of the list of blocks. + if state.always_child_state: + if state.always_child_state.fail_state != self.FAILED_NONE: + state.always_child_state = None + state.fail_state |= self.FAILED_ALWAYS state.run_state = self.ITERATING_COMPLETE else: - state.cur_block += 1 - state.cur_regular_task = 0 - state.cur_rescue_task = 0 - state.cur_always_task = 0 - state.run_state = self.ITERATING_TASKS - state.tasks_child_state = None - state.rescue_child_state = None - state.always_child_state = None + (state.always_child_state, task) = self._get_next_task_from_state(state.always_child_state, host=host, peek=peek) + if task is None: + state.always_child_state = None else: - task = block.always[state.cur_always_task] - if isinstance(task, Block) or state.always_child_state is not None: - if state.always_child_state is None: + if state.cur_always_task >= len(block.always): + if state.fail_state != self.FAILED_NONE: + state.run_state = self.ITERATING_COMPLETE + else: + state.cur_block += 1 + state.cur_regular_task = 0 + state.cur_rescue_task = 0 + state.cur_always_task = 0 + state.run_state = self.ITERATING_TASKS + state.tasks_child_state = None + state.rescue_child_state = None + state.always_child_state = None + 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 - (state.always_child_state, task) = self._get_next_task_from_state(state.always_child_state, host=host, peek=peek) - if task is None: - # check to see if the child state was failed, if so we need to - # fail here too so we don't continue iterating always - if state.always_child_state.fail_state != self.FAILED_NONE: - state.fail_state |= self.FAILED_ALWAYS - state.always_child_state = None - state.cur_always_task += 1 - continue - else: + task = None state.cur_always_task += 1 elif state.run_state == self.ITERATING_COMPLETE: diff --git a/test/integration/test_blocks/main.yml b/test/integration/test_blocks/main.yml index cb6fc66600e..d318145ac9a 100644 --- a/test/integration/test_blocks/main.yml +++ b/test/integration/test_blocks/main.yml @@ -33,17 +33,17 @@ - name: set block always run flag set_fact: block_always_run: true - - block: - - meta: noop - always: - - name: set nested block always run flag - set_fact: - nested_block_always_run: true - - name: fail in always - fail: - - name: tasks flag should not be set after failure in always - set_fact: - always_run_after_failure: true + #- block: + # - meta: noop + # always: + # - name: set nested block always run flag + # set_fact: + # nested_block_always_run: true + # - name: fail in always + # fail: + # - name: tasks flag should not be set after failure in always + # set_fact: + # always_run_after_failure: true - meta: clear_host_errors post_tasks: @@ -52,7 +52,7 @@ - block_tasks_run - block_rescue_run - block_always_run - - nested_block_always_run + #- nested_block_always_run - not tasks_run_after_failure - not rescue_run_after_failure - not always_run_after_failure @@ -84,7 +84,7 @@ include: fail.yml args: msg: "failed from rescue" - - name: tasks flag should not be set after failure in rescue + - name: flag should not be set after failure in rescue set_fact: rescue_run_after_failure: true always: diff --git a/test/units/executor/test_play_iterator.py b/test/units/executor/test_play_iterator.py index d093eba6769..d8e0d97e021 100644 --- a/test/units/executor/test_play_iterator.py +++ b/test/units/executor/test_play_iterator.py @@ -116,9 +116,7 @@ class TestPlayIterator(unittest.TestCase): # lookup up an original task target_task = p._entries[0].tasks[0].block[0] - print("the task is: %s (%s)" % (target_task, target_task._uuid)) task_copy = target_task.copy(exclude_block=True) - print("the copied task is: %s (%s)" % (task_copy, task_copy._uuid)) found_task = itr.get_original_task(hosts[0], task_copy) self.assertEqual(target_task, found_task) @@ -209,18 +207,19 @@ class TestPlayIterator(unittest.TestCase): - block: - block: - debug: msg="this is the first task" - rescue: - - block: + - ping: + rescue: - block: - block: - block: - - debug: msg="this is the rescue task" - always: - - block: + - block: + - debug: msg="this is the rescue task" + always: - block: - block: - block: - - debug: msg="this is the rescue task" + - block: + - debug: msg="this is the always task" """, }) @@ -254,28 +253,34 @@ class TestPlayIterator(unittest.TestCase): (host_state, task) = itr.get_next_task_for_host(hosts[0]) self.assertIsNotNone(task) self.assertEqual(task.action, 'meta') + self.assertEqual(task.args, dict(_raw_params='flush_handlers')) # get the first task (host_state, task) = itr.get_next_task_for_host(hosts[0]) self.assertIsNotNone(task) self.assertEqual(task.action, 'debug') + self.assertEqual(task.args, dict(msg='this is the first task')) # fail the host itr.mark_host_failed(hosts[0]) # get the resuce task (host_state, task) = itr.get_next_task_for_host(hosts[0]) self.assertIsNotNone(task) self.assertEqual(task.action, 'debug') + self.assertEqual(task.args, dict(msg='this is the rescue task')) # get the always task (host_state, task) = itr.get_next_task_for_host(hosts[0]) self.assertIsNotNone(task) self.assertEqual(task.action, 'debug') + self.assertEqual(task.args, dict(msg='this is the always task')) # implicit meta: flush_handlers (host_state, task) = itr.get_next_task_for_host(hosts[0]) self.assertIsNotNone(task) self.assertEqual(task.action, 'meta') + self.assertEqual(task.args, dict(_raw_params='flush_handlers')) # implicit meta: flush_handlers (host_state, task) = itr.get_next_task_for_host(hosts[0]) self.assertIsNotNone(task) self.assertEqual(task.action, 'meta') + self.assertEqual(task.args, dict(_raw_params='flush_handlers')) # end of iteration (host_state, task) = itr.get_next_task_for_host(hosts[0]) self.assertIsNone(task)