From 247e43b25244720f81e41febf0993f4b435fcb3d Mon Sep 17 00:00:00 2001 From: Alex Schultz Date: Thu, 4 Jun 2020 09:41:46 -0600 Subject: [PATCH] Fix IncludedFile equality check (#69524) In the case of a free style strategy, it is possible to end up with multiple hosts trying to include from the same role, however the tasks being included may be different with the use of tasks_from. Previously if you had two hosts that were included the same role when the process_include_results function tries to determine if a included needs to be run on a specific host, it would end up merging two different tasks into which ever one was processed first. This change updates the equality check to also check if the task uuid associated with the IncludedFile is the same. The previous check only checked if the task's parent uuid was the same. This breaks down when both includes have the same parent. - 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) Closes #69521 --- .../69521-free-strategy-include-fix.yml | 2 + lib/ansible/playbook/included_file.py | 1 + .../integration/targets/includes_race/aliases | 2 + .../targets/includes_race/inventory | 30 ++++ .../roles/random_sleep/tasks/main.yml | 8 + .../roles/set_a_fact/tasks/fact1.yml | 4 + .../roles/set_a_fact/tasks/fact2.yml | 4 + .../targets/includes_race/runme.sh | 5 + .../includes_race/test_includes_race.yml | 19 +++ test/units/playbook/test_included_file.py | 141 ++++++++++++++++++ 10 files changed, 216 insertions(+) create mode 100644 changelogs/fragments/69521-free-strategy-include-fix.yml create mode 100644 test/integration/targets/includes_race/aliases create mode 100644 test/integration/targets/includes_race/inventory create mode 100644 test/integration/targets/includes_race/roles/random_sleep/tasks/main.yml create mode 100644 test/integration/targets/includes_race/roles/set_a_fact/tasks/fact1.yml create mode 100644 test/integration/targets/includes_race/roles/set_a_fact/tasks/fact2.yml create mode 100755 test/integration/targets/includes_race/runme.sh create mode 100644 test/integration/targets/includes_race/test_includes_race.yml 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..fff62d9f20f --- /dev/null +++ b/test/integration/targets/includes_race/aliases @@ -0,0 +1,2 @@ +shippable/posix/group5 +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 511f3169e63..f143acb93a1 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 @@ -49,6 +51,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' @@ -170,6 +214,103 @@ def test_process_include_simulate_free(mock_iterator, mock_variable_manager): 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 == {} + + def test_empty_raw_params(): parent_task_ds = {'debug': 'msg=foo'} parent_task = Task.load(parent_task_ds)