From f538865c68860e70d1d06a5e68fae8979f62b573 Mon Sep 17 00:00:00 2001
From: Gris Ge <fge@redhat.com>
Date: Mon, 5 Jul 2021 15:59:10 +0800
Subject: [PATCH] nm: Fix the incorrect change indication when apply the same
 config twice

When applying the same network connections twice, the second apply still
shows `changed: true`.

The root cause:
 * When user never asked about ethtool configuration, network-role
   will generate an all-default `NM.SettingEthtool` and pass it to
   NetworkManager daemon. But NetworkManager discard it when saving to
   ifcfg plugin as ifcfg plugin will not keep empty ethtool option.

 * During second apply, the `NM.SimpleConnection.compare` will return
   False indicating configuration changed because of on-disk connection
   has no ethtool setting while pending connection does.

To fix it, we just remove the all-default `NM.SettingEthtool`.

Signed-off-by: Gris Ge <fge@redhat.com>
---
 library/network_connections.py                | 10 ++++
 .../tests_change_indication_on_repeat_run.yml | 50 +++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 tests/tests_change_indication_on_repeat_run.yml

diff --git a/library/network_connections.py b/library/network_connections.py
index b0508c6..9e489da 100644
--- a/library/network_connections.py
+++ b/library/network_connections.py
@@ -976,6 +976,16 @@ class NMUtil:
                     else:
                         s_ethtool.option_set_uint32(nm_ring, setting)
 
+            # * When users did not specify any ethtool configuration, this module
+            #   will generate an default `NM.SettingEthtool` object and pass it to
+            #   NetworkManager. But NetworkManager cannot serialize this when using the
+            #   ifcfg plugin but treats this as no `NM.SettingEthtool` object.
+            # * The following `NM.SimpleConnection.compare()` will therefore identify a
+            #   difference in the configuration.
+            # * To workaround this, remove the default NM.SettingEthtool object.
+            if s_ethtool.compare(NM.SettingEthtool.new(), NM.SettingCompareFlags.EXACT):
+                con.remove_setting(NM.SettingEthtool)
+
         if connection["mtu"]:
             if connection["type"] == "infiniband":
                 s_infiniband = self.connection_ensure_setting(con, NM.SettingInfiniband)
diff --git a/tests/tests_change_indication_on_repeat_run.yml b/tests/tests_change_indication_on_repeat_run.yml
new file mode 100644
index 0000000..fddcace
--- /dev/null
+++ b/tests/tests_change_indication_on_repeat_run.yml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# This file was generated by ensure_provider_tests.py
+---
+- hosts: all
+  vars:
+    interface: testnic1
+    type: veth
+  name: Test change indication on repeat run
+  tasks:
+    - include_tasks: tasks/manage_test_interface.yml
+      vars:
+        state: present
+    - include_tasks: tasks/assert_device_present.yml
+    - block:
+        - block:
+          - include_role:
+              name: linux-system-roles.network
+            register: __network_connections_result
+          - name: Assert change:true
+            assert:
+              that: __network_connections_result is changed
+          - include_role:
+              name: linux-system-roles.network
+            register: __network_connections_result
+          - name: Assert change:false
+            assert:
+              that: not __network_connections_result is changed
+          vars:
+            network_connections:
+              - name: "{{ interface }}"
+                state: up
+                type: ethernet
+                ip:
+                  address:
+                    - 192.0.2.2/24
+      always:
+        - block:
+            - include_role:
+                name: linux-system-roles.network
+              vars:
+                network_connections:
+                  - name: "{{ interface }}"
+                    persistent_state: absent
+                    state: down
+              ignore_errors: true
+            - include_tasks: tasks/manage_test_interface.yml
+              vars:
+                state: absent
+          tags:
+            - "tests::cleanup"