Library/NM: Always rollback checkpoint on failure

library/network_connections.py

@@ -1474,6 +1474,7 @@ def __init__(self):
         RunEnvironment.__init__(self)
         self._run_results = []
         self._log_idx = 0
+        self.on_failure = None
 
         from ansible.module_utils.basic import AnsibleModule
 
@@ -1570,6 +1571,9 @@ def exit_json(self, connections, changed=False, **kwargs):
     def fail_json(
         self, connections, msg, changed=False, warn_traceback=False, **kwargs
     ):
+        if self.on_failure:
+            self.on_failure()
+
         traceback_msg = None
         if warn_traceback:
             traceback_msg = "exception: %s" % (traceback.format_exc())
@@ -1876,6 +1880,10 @@ def rollback_transaction(self, idx, action, error):
             idx, "failure: %s (%s) [[%s]]" % (error, action, traceback.format_exc())
         )
 
+    def on_failure(self):
+        """ Hook to do any cleanup on failure before exiting """
+        pass
+
     def run_action_absent(self, idx):
         raise NotImplementedError()
 
@@ -1943,17 +1951,20 @@ def start_transaction(self):
 
     def rollback_transaction(self, idx, action, error):
         Cmd.rollback_transaction(self, idx, action, error)
+        self.on_failure()
+
+    def finish_transaction(self):
+        Cmd.finish_transaction(self)
         if self._checkpoint:
             try:
-                self.nmutil.rollback_checkpoint(self._checkpoint)
+                self.nmutil.destroy_checkpoint(self._checkpoint)
             finally:
                 self._checkpoint = None
 
-    def finish_transaction(self):
-        Cmd.finish_transaction(self)
+    def on_failure(self):
         if self._checkpoint:
             try:
-                self.nmutil.destroy_checkpoint(self._checkpoint)
+                self.nmutil.rollback_checkpoint(self._checkpoint)
             finally:
                 self._checkpoint = None
 
@@ -2408,6 +2419,7 @@ def main():
             force_state_change=params["force_state_change"],
         )
         connections = cmd.connections
+        run_env_ansible.on_failure = cmd.on_failure
         cmd.run()
     except Exception as e:
         run_env_ansible.fail_json(

tests/ensure_non_running_provider.py

@@ -14,17 +14,18 @@
 OTHER_PROVIDER_SUFFIX = "_other_provider.yml"
 
 IGNORE = [
+    "tests_802_1x_nm.yml",
+    "tests_default_initscripts.yml",
+    "tests_default_nm.yml",
+    "tests_default.yml",
+    "tests_ethtool_features_initscripts.yml",
+    "tests_ethtool_features_nm.yml",
     "tests_helpers-and-asserts.yml",
+    "tests_regression_nm.yml",
     "tests_states.yml",
     "tests_unit.yml",
     "tests_vlan_mtu_initscripts.yml",
     "tests_vlan_mtu_nm.yml",
-    "tests_ethtool_features_initscripts.yml",
-    "tests_ethtool_features_nm.yml",
-    "tests_default_nm.yml",
-    "tests_default_initscripts.yml",
-    "tests_default.yml",
-    "tests_802_1x_nm.yml",
 ]
 
 OTHER_PLAYBOOK = """

tests/playbooks/tests_checkpoint_cleanup.yml

@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# This test is supposed to check that checkpoints are properly cleaned-up after
+# failures in the module. This test currently uses the initscripts provider to
+# mark a device as unmanaged for NM and then tries to activiate it using NM.
+# This failed without removing the checkpoint.
+---
+- hosts: all
+  vars:
+    interface: cptstbr
+    profile: "{{ interface }}"
+    network_provider: nm
+  pre_tasks:
+    - debug:
+        msg: Inside states tests
+    - include_tasks: tasks/show-interfaces.yml
+    - include_tasks: tasks/assert-device_absent.yml
+  roles:
+    - linux-system-roles.network
+  tasks:
+    - block:
+        # create test profile
+        - include_role:
+            name: linux-system-roles.network
+          vars:
+            network_provider: initscripts
+            network_connections:
+              - name: "{{ interface }}"
+                state: up
+                type: bridge
+                ip:
+                  dhcp4: false
+                  auto6: false
+        - include_tasks: tasks/assert-device_present.yml
+        - include_tasks: tasks/assert-profile_present.yml
+        # Use internal module directly for speedup
+        - network_connections:
+            provider: nm
+            connections:
+              - name: "{{ interface }}"
+                state: up
+                type: bridge
+                ip:
+                  dhcp4: false
+                  auto6: false
+          ignore_errors: true
+          register: error_trigger
+        - assert:
+            fail_msg: The module call did not fail. Therefore the test
+              condition was not triggered. This test needs to be adjusted or
+              dropped.
+            that: error_trigger.failed
+        # yamllint disable-line rule:line-length
+        - command: gdbus introspect --system --dest org.freedesktop.NetworkManager --object-path /org/freedesktop/NetworkManager/Checkpoint --recurse
+          register: checkpoints
+        - debug:
+            var: checkpoints
+        - name: Assert that no checkpoints are left
+          assert:
+            fail_msg: Checkpoints not cleaned up
+            that: checkpoints.stdout_lines | length == 2
+      always:
+        - block:
+            # Use internal module directly for speedup
+            - network_connections:
+                provider: nm
+                connections:
+                  - name: "{{ interface }}"
+                    persistent_state: absent
+                    state: down
+            - command: ip link del "{{ interface }}"
+          ignore_errors: true
+          tags:
+            - "tests::cleanup"

tests/tests_regression_nm.yml

@@ -0,0 +1,28 @@
+---
+# set network provider and gather facts
+- hosts: all
+  tasks:
+    - name: Set network provider to 'nm'
+      set_fact:
+        network_provider: nm
+    - name: Install NetworkManager
+      package:
+        name: NetworkManager
+        state: present
+    - name: Get NetworkManager version
+      command: rpm -q --qf "%{version}" NetworkManager
+      args:
+        warn: "no"
+      when: true
+      register: NetworkManager_version
+
+# workaround for: https://github.com/ansible/ansible/issues/27973
+# There is no way in Ansible to abort a playbook hosts with specific OS
+# releases Therefore we include the playbook with the tests only if the hosts
+# would support it.
+# The test requires NetworkManager, therefore it cannot run on RHEL/CentOS 6
+- import_playbook: playbooks/tests_checkpoint_cleanup.yml
+  when:
+    - ansible_distribution_major_version != '6'
+    # The test depends on behavior that is only visible with newer NM
+    - NetworkManager_version.stdout is version('1.22.0', '>=')