From e0c7d550a9934b837c45997ce9adef425bce6bcf Mon Sep 17 00:00:00 2001 From: Till Maas Date: Thu, 25 Jun 2020 12:17:24 +0200 Subject: [PATCH] States: Ignore already removed profile for absent When a profile is specified as absent, ignore state requests if the profile is already removed or not completely specified to improve idempotence. Also restructure the states test. This introduces a clear structure for the individual test steps, properly assigns tags for each test from the file and provides a clear error message with a description of the test that failed in case of errors. Support for tests that expect a failure is still missing. --- library/network_connections.py | 23 ++- .../network_lsr/argument_validator.py | 6 +- tests/ensure_provider_tests.py | 1 - tests/playbooks/tests_states.yml | 186 ++++++++++++------ tests/tasks/activate_profile.yml | 9 + tests/tasks/cleanup_profile+device.yml | 9 + tests/tasks/create_bridge_profile.yml | 15 ++ .../create_bridge_profile_no_autoconnect.yml | 16 ++ tests/tasks/delete_interface.yml | 6 + tests/tasks/get_NetworkManager_NVRA.yml | 19 ++ tests/tasks/remove+down_profile.yml | 10 + tests/tasks/remove_profile.yml | 9 + tests/tasks/run_test.yml | 68 +++++++ tests/tests_states_initscripts.yml | 13 ++ tests/unit/test_network_connections.py | 8 +- 15 files changed, 320 insertions(+), 78 deletions(-) create mode 100644 tests/tasks/activate_profile.yml create mode 100644 tests/tasks/cleanup_profile+device.yml create mode 100644 tests/tasks/create_bridge_profile.yml create mode 100644 tests/tasks/create_bridge_profile_no_autoconnect.yml create mode 100644 tests/tasks/delete_interface.yml create mode 100644 tests/tasks/get_NetworkManager_NVRA.yml create mode 100644 tests/tasks/remove+down_profile.yml create mode 100644 tests/tasks/remove_profile.yml create mode 100644 tests/tasks/run_test.yml create mode 100644 tests/tests_states_initscripts.yml diff --git a/library/network_connections.py b/library/network_connections.py index a299e48..bc13856 100644 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -45,6 +45,8 @@ options: Documentation needs to be written. Note that the network_connections ############################################################################### +PERSISTENT_STATE = "persistent_state" +ABSENT_STATE = "absent" DEFAULT_ACTIVATION_TIMEOUT = 90 @@ -2299,11 +2301,13 @@ class Cmd_nm(Cmd): cons = self.nmutil.connection_list(name=connection["name"]) if not changed: - self.log_error( - idx, - "down connection %s failed: connection not found" - % (connection["name"]), + message = "down connection %s failed: connection not found" % ( + connection["name"] ) + if connection[PERSISTENT_STATE] == ABSENT_STATE: + self.log_info(idx, message) + else: + self.log_error(idx, message) ############################################################################### @@ -2436,11 +2440,18 @@ class Cmd_initscripts(Cmd): path = IfcfgUtil.ifcfg_path(name) if not os.path.isfile(path): - if self.check_mode == CheckMode.REAL_RUN: + if ( + self.check_mode == CheckMode.REAL_RUN + and connection.get(PERSISTENT_STATE) != ABSENT_STATE + ): self.log_error(idx, "ifcfg file '%s' does not exist" % (path)) else: + if self.check_mode != CheckMode.REAL_RUN: + in_checkmode = " in check mode" + else: + in_checkmode = "" self.log_info( - idx, "ifcfg file '%s' does not exist in check mode" % (path) + idx, "ifcfg file '%s' does not exist%s" % (path, in_checkmode) ) return diff --git a/module_utils/network_lsr/argument_validator.py b/module_utils/network_lsr/argument_validator.py index 4659950..f005586 100644 --- a/module_utils/network_lsr/argument_validator.py +++ b/module_utils/network_lsr/argument_validator.py @@ -1088,8 +1088,10 @@ class ArgValidator_DictConnection(ArgValidatorDict): actions.append(persistent_state) # If the profile should be absent at the end, it needs to be present in - # the meantime to allow to (de)activate it - if persistent_state == "absent" and state: + # the meantime to allow to (de)activate it. This is only possible if it + # is completely defined, for which `type` needs to be specified. + # Otherwise, downing is happening on a best-effort basis + if persistent_state == "absent" and state and result.get("type"): actions.append("present") # Change the runtime state if necessary diff --git a/tests/ensure_provider_tests.py b/tests/ensure_provider_tests.py index 19122bd..776803e 100755 --- a/tests/ensure_provider_tests.py +++ b/tests/ensure_provider_tests.py @@ -68,7 +68,6 @@ NM_ONLY_TESTS = { "playbooks/tests_802_1x_updated.yml": {}, "playbooks/tests_802_1x.yml": {}, "playbooks/tests_reapply.yml": {}, - "playbooks/tests_states.yml": {}, # mac80211_hwsim (used for tests_wireless) only seems to be available # and working on RHEL/CentOS 7 "playbooks/tests_wireless.yml": { diff --git a/tests/playbooks/tests_states.yml b/tests/playbooks/tests_states.yml index 8edbf8f..5926ad0 100644 --- a/tests/playbooks/tests_states.yml +++ b/tests/playbooks/tests_states.yml @@ -4,74 +4,134 @@ vars: interface: statebr profile: "{{ interface }}" - network_provider: nm + lsr_fail_debug: + - __network_connections_result tasks: - debug: - msg: Inside states tests - - include_tasks: tasks/show_interfaces.yml - - include_tasks: tasks/assert_device_absent.yml + msg: "this is: playbooks/tests_states.yml" + tags: + - always - # create test profile - - include_role: - name: linux-system-roles.network - vars: - network_connections: - - name: statebr - state: up - type: bridge - ip: - dhcp4: false - auto6: false - - include_tasks: tasks/assert_device_present.yml - - include_tasks: tasks/assert_profile_present.yml - # test case (remove profile but keep it up) - # I can remove a profile but keep the configuration active. - - include_role: - name: linux-system-roles.network - vars: - network_connections: - - name: statebr - persistent_state: absent - - include_tasks: tasks/assert_device_present.yml - - include_tasks: tasks/assert_profile_absent.yml - - # test case - # I can set a profile down that is up and absent. - - name: Set down - include_role: - name: linux-system-roles.network - vars: - network_connections: - - name: statebr - state: down - - include_tasks: tasks/assert_device_absent.yml - - include_tasks: tasks/assert_profile_absent.yml - - # test case (downing a non existent profile) - # I can set down and remove a non existent profile - # This will be rescued until issue 188 is fixed - - name: Verify that the play fails with the expected error message - block: - - name: Set down non existent profile - include_role: - name: linux-system-roles.network + - block: + - include_tasks: tasks/run_test.yml vars: - network_connections: - - name: non_existent - state: down - persistent_state: absent + lsr_description: I can create a profile + lsr_setup: + - tasks/delete_interface.yml + - tasks/assert_device_absent.yml + lsr_test: + - tasks/create_bridge_profile.yml + lsr_assert: + - tasks/assert_profile_present.yml + lsr_assert_when: + # Device should be present because of autoconnect: true by + # default for NM (this might be considered a bug) + - what: tasks/assert_device_present.yml + when: "{{ network_provider == 'nm' }}" + lsr_cleanup: + - tasks/cleanup_profile+device.yml + tags: + - tests::states:create - - name: 'UNREACH' - set_fact: - __network_test_failed: false + - block: + - include_tasks: tasks/run_test.yml + vars: + lsr_description: I can create a profile without autoconnect + lsr_setup: + - tasks/delete_interface.yml + - tasks/assert_device_absent.yml + lsr_test: + - tasks/create_bridge_profile_no_autoconnect.yml + lsr_assert: + # Device should be absent because of autoconnect: false + - tasks/assert_device_absent.yml + - tasks/assert_profile_present.yml + lsr_cleanup: + - tasks/cleanup_profile+device.yml + tags: + - tests::states:create_without_autoconnect - rescue: - - name: Role has failed when it should have - set_fact: - __network_test_failed: true + - block: + - include_tasks: tasks/run_test.yml + vars: + lsr_description: I can activate an existing profile + lsr_setup: + - tasks/create_bridge_profile.yml + lsr_test: + - tasks/activate_profile.yml + lsr_assert: + - tasks/assert_device_present.yml + - tasks/assert_profile_present.yml + lsr_cleanup: + - tasks/cleanup_profile+device.yml + tags: + - tests::states:activate - always: - - assert: - that: __network_test_failed - fail_msg: "The role did not fail when it should have" + - block: + - include_tasks: tasks/run_test.yml + vars: + lsr_description: I can remove an existing profile without taking it + down + lsr_setup: + - tasks/create_bridge_profile.yml + - tasks/activate_profile.yml + lsr_test: + - tasks/remove_profile.yml + lsr_assert: + - tasks/assert_device_present.yml + - tasks/assert_profile_absent.yml + lsr_cleanup: + - tasks/cleanup_profile+device.yml + tags: + - tests::states:remove_up + + - block: + - include_tasks: tasks/run_test.yml + vars: + lsr_description: I can take a profile down that is absent + lsr_setup: + - tasks/create_bridge_profile.yml + - tasks/activate_profile.yml + - tasks/remove_profile.yml + lsr_test: + - tasks/remove+down_profile.yml + lsr_assert: + - tasks/assert_profile_absent.yml + lsr_assert_when: + - what: tasks/assert_device_absent.yml + when: "{{ network_provider == 'nm' }}" + lsr_cleanup: + - tasks/cleanup_profile+device.yml + tags: + - tests::states:remove_down + + - block: + - include_tasks: tasks/run_test.yml + vars: + lsr_description: I will not get an error when I try to + remove an absent profile + lsr_setup: + - tasks/create_bridge_profile.yml + - tasks/activate_profile.yml + - tasks/remove+down_profile.yml + lsr_test: + - tasks/remove+down_profile.yml + lsr_assert: + - tasks/assert_profile_absent.yml + # FIXME: This needs to be included before lsr_assert_when but + # after the role ran to ensure that NetworkManager is actually + # installed but it is not an assert. + - tasks/get_NetworkManager_NVRA.yml + lsr_assert_when: + - what: tasks/assert_device_absent.yml + # NetworkManager 1.18.4 from CentOS does not seem to remove the + # virtual interface in this case but it seems to work with + # 1:NetworkManager-1.27.0-26129.d0a2eb8f05.el7.x86_64 + when: "{{ network_provider == 'nm' and + NetworkManager_NVRA != 'NetworkManager-1.18.4-3.el7.x86_64' + }}" + lsr_cleanup: + - tasks/cleanup_profile+device.yml + tags: + - tests::states:remove_down_twice diff --git a/tests/tasks/activate_profile.yml b/tests/tasks/activate_profile.yml new file mode 100644 index 0000000..dea878c --- /dev/null +++ b/tests/tasks/activate_profile.yml @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- include_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ interface }}" + state: up +... diff --git a/tests/tasks/cleanup_profile+device.yml b/tests/tasks/cleanup_profile+device.yml new file mode 100644 index 0000000..92d1eba --- /dev/null +++ b/tests/tasks/cleanup_profile+device.yml @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- shell: | + nmcli con delete {{ interface }} + nmcli con load /etc/sysconfig/network-scripts/ifcfg-{{ interface }} + rm -f /etc/sysconfig/network-scripts/ifcfg-{{ interface }} + ip link del {{ interface }} + ignore_errors: true +... diff --git a/tests/tasks/create_bridge_profile.yml b/tests/tasks/create_bridge_profile.yml new file mode 100644 index 0000000..e846127 --- /dev/null +++ b/tests/tasks/create_bridge_profile.yml @@ -0,0 +1,15 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- include_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ interface }}" + persistent_state: present + type: bridge + ip: + dhcp4: false + auto6: false +- debug: + var: __network_connections_result +... diff --git a/tests/tasks/create_bridge_profile_no_autoconnect.yml b/tests/tasks/create_bridge_profile_no_autoconnect.yml new file mode 100644 index 0000000..308f57d --- /dev/null +++ b/tests/tasks/create_bridge_profile_no_autoconnect.yml @@ -0,0 +1,16 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- include_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ interface }}" + autoconnect: false + persistent_state: present + type: bridge + ip: + dhcp4: false + auto6: false +- debug: + var: __network_connections_result +... diff --git a/tests/tasks/delete_interface.yml b/tests/tasks/delete_interface.yml new file mode 100644 index 0000000..064e17f --- /dev/null +++ b/tests/tasks/delete_interface.yml @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: remove test interface if necessary + command: "ip link del {{ interface }}" + ignore_errors: true +... diff --git a/tests/tasks/get_NetworkManager_NVRA.yml b/tests/tasks/get_NetworkManager_NVRA.yml new file mode 100644 index 0000000..d7b9256 --- /dev/null +++ b/tests/tasks/get_NetworkManager_NVRA.yml @@ -0,0 +1,19 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- block: + - name: Get NetworkManager RPM version + command: + cmd: rpm -qa NetworkManager + warn: false + register: __rpm_qa_NetworkManager + + - name: Store NetworkManager version + set_fact: + NetworkManager_NVRA: "{{ __rpm_qa_NetworkManager.stdout }}" + + - name: Show NetworkManager version + debug: + var: NetworkManager_NVRA + tags: + - always +... diff --git a/tests/tasks/remove+down_profile.yml b/tests/tasks/remove+down_profile.yml new file mode 100644 index 0000000..99c0c8e --- /dev/null +++ b/tests/tasks/remove+down_profile.yml @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- include_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ interface }}" + persistent_state: absent + state: down +... diff --git a/tests/tasks/remove_profile.yml b/tests/tasks/remove_profile.yml new file mode 100644 index 0000000..a7dbc12 --- /dev/null +++ b/tests/tasks/remove_profile.yml @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- include_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ interface }}" + persistent_state: absent +... diff --git a/tests/tasks/run_test.yml b/tests/tasks/run_test.yml new file mode 100644 index 0000000..cc9676e --- /dev/null +++ b/tests/tasks/run_test.yml @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Run test + block: + - name: "TEST: {{ lsr_description }}" + debug: + msg: "########## {{ lsr_description }} ##########" + + - debug: + var: "{{ item }}" + loop: + - lsr_description + - lsr_setup + - lsr_test + - lsr_assert + - lsr_assert_when + - lsr_fail_debug + - lsr_cleanup + + - include_tasks: tasks/show_interfaces.yml + + - name: setup + include_tasks: "{{ item }}" + loop: "{{ lsr_setup }}" + tags: + - "tests::setup" + + - name: test + include_tasks: "{{ item }}" + loop: "{{ lsr_test }}" + tags: + - "tests::test" + + - name: asserts + include_tasks: "{{ item }}" + loop: "{{ lsr_assert }}" + tags: + - "tests::assert" + + - name: conditional asserts + include_tasks: "{{ item['what'] }}" + when: + - "{{ item['when'] }}" + loop: "{{ lsr_assert_when|default([]) }}" + + - name: "Success in test '{{ lsr_description }}'" + debug: + msg: "+++++ Success in test '{{ lsr_description }}' +++++" + + rescue: + - name: "Failure in test '{{ lsr_description }}'" + debug: + msg: "!!!!! Failure in test '{{ lsr_description }}' !!!!!" + + - debug: + var: "{{ item }}" + loop: "{{ lsr_fail_debug | default([]) }}" + + - fail: + msg: "!!!!! Failure in test '{{ lsr_description }}' !!!!!" + + always: + - name: cleanup + include_tasks: "{{ item }}" + loop: "{{ lsr_cleanup }}" + tags: + - "tests::cleanup" +... diff --git a/tests/tests_states_initscripts.yml b/tests/tests_states_initscripts.yml new file mode 100644 index 0000000..3e55a43 --- /dev/null +++ b/tests/tests_states_initscripts.yml @@ -0,0 +1,13 @@ +# SPDX-License-Identifier: BSD-3-Clause +# This file was generated by ensure_provider_tests.py +--- +- hosts: all + name: Run playbook 'playbooks/tests_states.yml' with initscripts as provider + tasks: + - name: Set network provider to 'initscripts' + set_fact: + network_provider: initscripts + tags: + - always + +- import_playbook: playbooks/tests_states.yml diff --git a/tests/unit/test_network_connections.py b/tests/unit/test_network_connections.py index 57b5efc..bc8ff9b 100755 --- a/tests/unit/test_network_connections.py +++ b/tests/unit/test_network_connections.py @@ -2794,11 +2794,7 @@ class TestValidator(unittest.TestCase): def test_state_absent_up_no_type(self): self.check_partial_connection_zero( {"name": "eth0", "persistent_state": "absent", "state": "up"}, - { - "actions": ["present", "up", "absent"], - "persistent_state": "absent", - "state": "up", - }, + {"actions": ["up", "absent"], "persistent_state": "absent", "state": "up"}, ) def test_state_absent_up_type(self): @@ -2822,7 +2818,7 @@ class TestValidator(unittest.TestCase): self.check_partial_connection_zero( {"name": "eth0", "persistent_state": "absent", "state": "down"}, { - "actions": ["present", "down", "absent"], + "actions": ["down", "absent"], "persistent_state": "absent", "state": "down", },