Fix dummy interface returning changed

[haddystuff]

SUMMARY

connection_options work differently when detecting changes for 802-3-ethernet.mtu parameter. It causes problems with applying mtu setting.
Fixes #3612

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

plugins/modules/net_tools/nmcli.py

ADDITIONAL INFORMATION

After little small discussion(#3615) i came out with this change. Basically it shouldn't break previous behavior, but also should fix #3612.
This how it should work: When we try to run playbook with type "dummy" and no mtu parameter setted ansible won't do any changes and also will display no diff, but after you set(any way possible) mtu to some value except 0, next time you gonna run playbook without mtu it will change it to "auto" and will also show diff.

[ansibullbot]

cc @alcamie101
click here for bot help

[Ajpantuso]

This just skips checking for a change when mtu changes from some value to null and will create a bug for the ethernet and team-slave types. To implement the behavior as you have described it you would need to replace continue with self.mtu = 0.

[haddystuff]

This piece of code skips change if '802-3-ethernet.mtu' parameter doesn't exist and playbook doesn't declare new value for it. But now i can see, that this change can create strange behavior: not setting '802-3-ethernet.mtu' parameter to auto if already existing interface doesn't have it, when declaring "mtu: 0" in playbook. I can fix it by replacing value == 'auto' with self.mtu == None.

If i change continue to self.mtu = 0, i get some strange behavior:
For example: when i run playbook without mtu setted, i get this when rerunning playbook:

-    "802-3-ethernet.mtu": "disabled",
+    "802-3-ethernet.mtu": "auto",

We can avoid all this problems by going in this direction: #3618, but as you mentioned it would change expected behaviour in case of creating new interface without mtu being set. Moreover, i would have to change some tests cases.

[Ajpantuso]

This code doesn't skip change only change reporting which is why it introduces a bug for the ethernet and team-slave types which should be compared against auto even when mtu is unset.

To actually affect the value passed to nmcli you will need to set self.mtu = 0, but this still doesn't address your original bug report. For the dummy type the setting value used for comparison must not be auto if self.mtu is unset.

[haddystuff]

Ok. Now i can see the problem in my change. But my test shows that solution with replacing continue with self.mtu = 0also has some problems.
For example: when no mtu parameter being set in playbook but interface already has "802-3-ethernet.mtu" value(for example 2000) output shows diff as expected, but "802-3-ethernet.mtu" remains the same - 2000(no changes were made).

[haddystuff]

I can also explain how ethernet and team-slave type of connection whould be compared agains 'auto':
"continue" executes only if there is no key(which is 802-3-ethernet.mtu) in conn_info and no self.mtu beeing set. That's the exact situation we have in #3612. It can also effect changed variable in this method, if there is no changes except of that case(mtu not beeing set in playbook and on interface). It will not allow changed to be set True, which cause not no start pointless change(connection_update will not run). It's pointless because there were no parameters to changes except self.mtu which is None. And connection_update method will skip change if option value is None.
Consider this, i can't see how this sitiation will effect ethernet or team-slave type of connections.
I could be wrong but i my tests don't show any problems.

[Ajpantuso]

Is that specific to any interface type?

When I run the following with these changes:

- name: Create dummy connection with MTU of 1500
      community.general.nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.1/32
        state: present
        type: dummy
        mtu: 1500
    - name: Modify dummy connection omitting MTU
      community.general.nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.1/32
        state: present
        type: dummy
    - name: Modify dummy connection omitting MTU (idempotency)
      community.general.nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.1/32
        state: present
        type: dummy
    - name: Retrieve MTU value from nmcli
      ansible.builtin.command:
        cmd: nmcli -f '802-3-ethernet.mtu' conn show dummy_con0
      register: mtu_value
    - name: Output MTU value
      debug:
        msg: "{{ mtu_value.stdout }}"

I get the below:

PLAY [localhost] ************************************************************************************************************************************************************

TASK [Gathering Facts] ******************************************************************************************************************************************************
ok: [localhost]

TASK [Create dummy connection with MTU of 1500] *****************************************************************************************************************************
changed: [localhost] => {"Connection": "Connection dummy_con0 of Type dummy is being added", "changed": true, "conn_name": "dummy_con0", "state": "present", "stdout": "Connection successfully activated (D-Bus active path: /org/freedesktop/NetworkManager/ActiveConnection/27)\n", "stdout_lines": ["Connection successfully activated (D-Bus active path: /org/freedesktop/NetworkManager/ActiveConnection/27)"]}

TASK [Modify dummy connection omitting MTU] *********************************************************************************************************************************
--- before
+++ after
@@ -1,5 +1,5 @@
 {
-    "802-3-ethernet.mtu": "1500",
+    "802-3-ethernet.mtu": "auto",
     "connection.autoconnect": "yes",
     "connection.interface-name": "dummy0",
     "ipv4.addresses": "10.0.0.1/32",

changed: [localhost] => {"Exists": "Connections do exist so we are modifying them", "changed": true, "conn_name": "dummy_con0", "state": "present"}

TASK [Modify dummy connection omitting MTU (idempotency)] *******************************************************************************************************************
ok: [localhost] => {"Exists": "Connections already exist and no changes made", "changed": false, "conn_name": "dummy_con0", "state": "present"}

TASK [Retrieve MTU value from nmcli] ****************************************************************************************************************************************
changed: [localhost] => {"changed": true, "cmd": ["nmcli", "-f", "802-3-ethernet.mtu", "conn", "show", "dummy_con0"], "delta": "0:00:00.021036", "end": "2021-10-28 08:18:31.329219", "msg": "", "rc": 0, "start": "2021-10-28 08:18:31.308183", "stderr": "", "stderr_lines": [], "stdout": "802-3-ethernet.mtu:                     auto", "stdout_lines": ["802-3-ethernet.mtu:                     auto"]}

TASK [Output MTU value] *****************************************************************************************************************************************************
ok: [localhost] => {
    "msg": "802-3-ethernet.mtu:                     auto"
}

PLAY RECAP ******************************************************************************************************************************************************************
localhost                  : ok=6    changed=3    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

[haddystuff]

As you can see i changed continue to current_value = 'auto' and delete self.mtu is None. I think this should do the trick.

[Ajpantuso]

As you can see i changed continue to current_value = 'auto' and delete self.mtu is None. I think this should do the trick.

Setting current_value = 'auto' just falsifies the nmcli output used for comparison and does not actually change the value used to modify the connection.

So with your change the below:

- name: Create dummy connection
      community.general.nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.1/32
        state: present
        type: dummy
    - name: Retrieve MTU value from nmcli
      ansible.builtin.command:
        cmd: nmcli -f '802-3-ethernet.mtu' conn show dummy_con0
      register: mtu_value
    - name: Output MTU value
      debug:
        msg: "{{ mtu_value.stdout }}"
    - name: Modify dummy connection with MTU of 1500
      community.general.nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.1/32
        state: present
        type: dummy
        mtu: 1500
    - name: Retrieve MTU value from nmcli
      ansible.builtin.command:
        cmd: nmcli -f '802-3-ethernet.mtu' conn show dummy_con0
      register: mtu_value
    - name: Output MTU value
      debug:
        msg: "{{ mtu_value.stdout }}"
    - name: Modify dummy connection omitting MTU
      community.general.nmcli:
        conn_name: dummy_con0
        ifname: dummy0
        ip4: 10.0.0.1/32
        state: present
        type: dummy
    - name: Retrieve MTU value from nmcli
      ansible.builtin.command:
        cmd: nmcli -f '802-3-ethernet.mtu' conn show dummy_con0
      register: mtu_value
    - name: Output MTU value
      debug:
        msg: "{{ mtu_value.stdout }}"

Results in the following:

TASK [Create dummy connection] **********************************************************************************************************************************************
changed: [localhost]

TASK [Retrieve MTU value from nmcli] ****************************************************************************************************************************************
changed: [localhost]

TASK [Output MTU value] *****************************************************************************************************************************************************
ok: [localhost] => {
    "msg": ""
}

TASK [Modify dummy connection with MTU of 1500] *****************************************************************************************************************************
--- before
+++ after
@@ -1,5 +1,5 @@
 {
-    "802-3-ethernet.mtu": "auto",
+    "802-3-ethernet.mtu": "1500",
     "connection.autoconnect": "yes",
     "connection.interface-name": "dummy0",
     "ipv4.addresses": "10.0.0.1/32",

changed: [localhost]

TASK [Retrieve MTU value from nmcli] ****************************************************************************************************************************************
changed: [localhost]

TASK [Output MTU value] *****************************************************************************************************************************************************
ok: [localhost] => {
    "msg": "802-3-ethernet.mtu:                     1500"
}

TASK [Modify dummy connection omitting MTU] *********************************************************************************************************************************
--- before
+++ after
@@ -1,5 +1,5 @@
 {
-    "802-3-ethernet.mtu": "1500",
+    "802-3-ethernet.mtu": "auto",
     "connection.autoconnect": "yes",
     "connection.interface-name": "dummy0",
     "ipv4.addresses": "10.0.0.1/32",

changed: [localhost]

TASK [Retrieve MTU value from nmcli] ****************************************************************************************************************************************
changed: [localhost]

TASK [Output MTU value] *****************************************************************************************************************************************************
ok: [localhost] => {
    "msg": "802-3-ethernet.mtu:                     1500"
}

PLAY RECAP ******************************************************************************************************************************************************************
localhost                  : ok=10   changed=6    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0 

[Ajpantuso]

This just needs a changelog fragment and then it's good to go!

[Ajpantuso]

LGTM

[patchback]

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/85085bcd539068a0c02ff48226656bcf3899aac7/pr-3625

Backported as #3687

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/85085bcd539068a0c02ff48226656bcf3899aac7/pr-3625

Backported as #3688

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@haddystuff thanks for fixing this!
@Ajpantuso thanks for reviewing!