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

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

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

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": {

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

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
+...

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
+...

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
+...

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
+...

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
+...

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
+...

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
+...

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
+...

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"
+...

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

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",
             },