diff --git a/changelogs/fragments/69521-free-strategy-include-fix.yml b/changelogs/fragments/69521-free-strategy-include-fix.yml new file mode 100644 index 00000000000..4d92a0675f5 --- /dev/null +++ b/changelogs/fragments/69521-free-strategy-include-fix.yml @@ -0,0 +1,2 @@ +bugfixes: +- Fixed the equality check for IncludedFiles to ensure they are not accidently merged when process_include_results runs. diff --git a/lib/ansible/playbook/included_file.py b/lib/ansible/playbook/included_file.py index 7870d83a6a2..14663673368 100644 --- a/lib/ansible/playbook/included_file.py +++ b/lib/ansible/playbook/included_file.py @@ -51,6 +51,7 @@ class IncludedFile: return (other._filename == self._filename and other._args == self._args and other._vars == self._vars and + other._task._uuid == self._task._uuid and other._task._parent._uuid == self._task._parent._uuid) def __repr__(self): diff --git a/test/integration/targets/includes_race/aliases b/test/integration/targets/includes_race/aliases new file mode 100644 index 00000000000..f71c8117c74 --- /dev/null +++ b/test/integration/targets/includes_race/aliases @@ -0,0 +1,2 @@ +shippable/posix/group4 +skip/aix diff --git a/test/integration/targets/includes_race/inventory b/test/integration/targets/includes_race/inventory new file mode 100644 index 00000000000..878792949fe --- /dev/null +++ b/test/integration/targets/includes_race/inventory @@ -0,0 +1,30 @@ +host001 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host002 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host003 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host004 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host005 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host006 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host007 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host008 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host009 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host010 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host011 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host012 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host013 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host014 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host015 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host016 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host017 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host018 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host019 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host020 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host021 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host022 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host023 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host024 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host025 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host026 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host027 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host028 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host029 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" +host030 ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}" diff --git a/test/integration/targets/includes_race/roles/random_sleep/tasks/main.yml b/test/integration/targets/includes_race/roles/random_sleep/tasks/main.yml new file mode 100644 index 00000000000..cee459a248e --- /dev/null +++ b/test/integration/targets/includes_race/roles/random_sleep/tasks/main.yml @@ -0,0 +1,8 @@ +--- +# tasks file for random_sleep +- name: Generate sleep time + set_fact: + sleep_time: "{{ 3 | random }}" + +- name: Do random sleep + shell: sleep "{{ sleep_time }}" diff --git a/test/integration/targets/includes_race/roles/set_a_fact/tasks/fact1.yml b/test/integration/targets/includes_race/roles/set_a_fact/tasks/fact1.yml new file mode 100644 index 00000000000..36b08dcb787 --- /dev/null +++ b/test/integration/targets/includes_race/roles/set_a_fact/tasks/fact1.yml @@ -0,0 +1,4 @@ +--- +- name: Set fact1 + set_fact: + fact1: yay diff --git a/test/integration/targets/includes_race/roles/set_a_fact/tasks/fact2.yml b/test/integration/targets/includes_race/roles/set_a_fact/tasks/fact2.yml new file mode 100644 index 00000000000..865f130db14 --- /dev/null +++ b/test/integration/targets/includes_race/roles/set_a_fact/tasks/fact2.yml @@ -0,0 +1,4 @@ +--- +- name: Set fact2 + set_fact: + fact2: yay diff --git a/test/integration/targets/includes_race/runme.sh b/test/integration/targets/includes_race/runme.sh new file mode 100755 index 00000000000..2261d2718a0 --- /dev/null +++ b/test/integration/targets/includes_race/runme.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +set -eux + +ansible-playbook test_includes_race.yml -i inventory -v "$@" diff --git a/test/integration/targets/includes_race/test_includes_race.yml b/test/integration/targets/includes_race/test_includes_race.yml new file mode 100644 index 00000000000..20f7dddd8d0 --- /dev/null +++ b/test/integration/targets/includes_race/test_includes_race.yml @@ -0,0 +1,19 @@ +- hosts: all + strategy: free + gather_facts: false + tasks: + - include_role: + name: random_sleep + - block: + - name: set a fact (1) + include_role: + name: set_a_fact + tasks_from: fact1.yml + - name: set a fact (2) + include_role: + name: set_a_fact + tasks_from: fact2.yml + - name: include didn't run + fail: + msg: "set_a_fact didn't run fact1 {{ fact1 | default('not defined')}} fact2: {{ fact2 | default('not defined') }}" + when: (fact1 is not defined or fact2 is not defined) diff --git a/test/units/playbook/test_included_file.py b/test/units/playbook/test_included_file.py index d73415c6986..e6e5244f5ef 100644 --- a/test/units/playbook/test_included_file.py +++ b/test/units/playbook/test_included_file.py @@ -26,8 +26,10 @@ import pytest from units.compat.mock import MagicMock from units.mock.loader import DictDataLoader +from ansible.playbook.block import Block from ansible.playbook.task import Task from ansible.playbook.task_include import TaskInclude +from ansible.playbook.role_include import IncludeRole from ansible.executor import task_result from ansible.playbook.included_file import IncludedFile @@ -48,6 +50,48 @@ def mock_variable_manager(): return mock_variable_manager +def test_equals_ok(): + uuid = '111-111' + parent = MagicMock(name='MockParent') + parent._uuid = uuid + task = MagicMock(name='MockTask') + task._uuid = uuid + task._parent = parent + inc_a = IncludedFile('a.yml', {}, {}, task) + inc_b = IncludedFile('a.yml', {}, {}, task) + assert inc_a == inc_b + + +def test_equals_different_tasks(): + parent = MagicMock(name='MockParent') + parent._uuid = '111-111' + task_a = MagicMock(name='MockTask') + task_a._uuid = '11-11' + task_a._parent = parent + task_b = MagicMock(name='MockTask') + task_b._uuid = '22-22' + task_b._parent = parent + inc_a = IncludedFile('a.yml', {}, {}, task_a) + inc_b = IncludedFile('a.yml', {}, {}, task_b) + assert inc_a != inc_b + + +def test_equals_different_parents(): + parent_a = MagicMock(name='MockParent') + parent_a._uuid = '111-111' + parent_b = MagicMock(name='MockParent') + parent_b._uuid = '222-222' + task_a = MagicMock(name='MockTask') + task_a._uuid = '11-11' + task_a._parent = parent_a + task_b = MagicMock(name='MockTask') + task_b._uuid = '11-11' + task_b._parent = parent_b + inc_a = IncludedFile('a.yml', {}, {}, task_a) + inc_b = IncludedFile('a.yml', {}, {}, task_b) + assert inc_a != inc_b + + def test_included_file_instantiation(): filename = 'somefile.yml' @@ -167,3 +211,100 @@ def test_process_include_simulate_free(mock_iterator, mock_variable_manager): assert res[0]._vars == {} assert res[1]._vars == {} + + +def test_process_include_simulate_free_block_role_tasks(mock_iterator, + mock_variable_manager): + """Test loading the same role returns different included files + + In the case of free, we may end up with included files from roles that + have the same parent but are different tasks. Previously the comparison + for equality did not check if the tasks were the same and only checked + that the parents were the same. This lead to some tasks being run + incorrectly and some tasks being silient dropped.""" + + fake_loader = DictDataLoader({ + 'include_test.yml': "", + '/etc/ansible/roles/foo_role/tasks/task1.yml': """ + - debug: msg=task1 + """, + '/etc/ansible/roles/foo_role/tasks/task2.yml': """ + - debug: msg=task2 + """, + }) + + hostname = "testhost1" + hostname2 = "testhost2" + + role1_ds = { + 'name': 'task1 include', + 'include_role': { + 'name': 'foo_role', + 'tasks_from': 'task1.yml' + } + } + role2_ds = { + 'name': 'task2 include', + 'include_role': { + 'name': 'foo_role', + 'tasks_from': 'task2.yml' + } + } + parent_task_ds = { + 'block': [ + role1_ds, + role2_ds + ] + } + parent_block = Block.load(parent_task_ds, loader=fake_loader) + + parent_block._play = None + + include_role1_ds = { + 'include_args': { + 'name': 'foo_role', + 'tasks_from': 'task1.yml' + } + } + include_role2_ds = { + 'include_args': { + 'name': 'foo_role', + 'tasks_from': 'task2.yml' + } + } + + include_role1 = IncludeRole.load(role1_ds, + block=parent_block, + loader=fake_loader) + include_role2 = IncludeRole.load(role2_ds, + block=parent_block, + loader=fake_loader) + + result1 = task_result.TaskResult(host=hostname, + task=include_role1, + return_data=include_role1_ds) + result2 = task_result.TaskResult(host=hostname2, + task=include_role2, + return_data=include_role2_ds) + results = [result1, result2] + + res = IncludedFile.process_include_results(results, + mock_iterator, + fake_loader, + mock_variable_manager) + assert isinstance(res, list) + # we should get two different includes + assert len(res) == 2 + assert res[0]._filename == 'foo_role' + assert res[1]._filename == 'foo_role' + # with different tasks + assert res[0]._task != res[1]._task + + assert res[0]._hosts == ['testhost1'] + assert res[1]._hosts == ['testhost2'] + + assert res[0]._args == {} + assert res[1]._args == {} + + assert res[0]._vars == {} + assert res[1]._vars == {}