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

@@ -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
 

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')

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

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])