fix for setting mtu on ipip and gre interfaces in nmcli.py

[edvinaskairys]

SUMMARY

When trying to edit the mtu parameter on a gre and ipip interface, nmcli always reports that nothing needs to be done, even though the specified mtu value is different than the current mtu for the gre/ipip Interface.

Fixes #9315

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

nmcli.py

[ansibullbot]

cc @alcamie101
click here for bot help

[felixfontein]

Thanks for your contribution! Please note that you need to add a changelog fragment, and that the unit tests are currently failing.

[edvinaskairys]

hello, @felixfontein just added fragments file, could you please explain what to do with failing unit tests ?

[russoz]

Hi @edvinaskairys you have to take a look at the test in tests/unit/plugins/modules/test_nmcli.py, function test_ipip_connection_unchanged(), and understand why changed was False for that test case but now it is True.

[russoz]

And test_gre_connection_unchanged() as well. Given that those are the exact ones this change is about, it is fairly possible that the test code needs to be changed as well.

[edvinaskairys]

can someone help me with failing CI tests ? i'm starting to feel limitations of my programming abilities:)

[russoz]

It's not your limitations, it's a module with almost 3000 lines, and a test module with almost 5000 lines. Its maintainability is... poor, to be extremely kind.

It seems that your change in line makes the mtu option to be automatically added to the command line of... some commands (3k lines, I am not debugging the whole thing now). When the tests ran, even though no mtu is passed in them, the value of the mtu param (in other words, None) seems to be set as an option that goes into the actual nmcli command line. That must certainly change whatever other value was there before (or whatever the heck it does to calculate things in this module), so those module executions in the tests should result, as of before, in not changed, and now they cause some change. But I have no idea of what exactly, much less where.

[edvinaskairys]

strange that this one: #8118 succesfully went through.

[russoz]

Different tests scenarios for those two connections. Focusing on the ipip for the time being (leaving the gre aside): the failing test is test_ipip_connection_unchanged. The output of that failure is:

____________ test_ipip_connection_unchanged[patch_ansible_module0] _____________
[gw3] linux -- Python 3.11.10 /usr/bin/python3.11

mocked_ipip_connection_unchanged = None
capfd = <_pytest.capture.CaptureFixture object at 0x722c4fa59d90>

    @pytest.mark.parametrize('patch_ansible_module', TESTCASE_IPIP, indirect=['patch_ansible_module'])
    def test_ipip_connection_unchanged(mocked_ipip_connection_unchanged, capfd):
        """
        Test : IPIP connection unchanged
        """
        with pytest.raises(SystemExit):
            nmcli.main()
    
        out, err = capfd.readouterr()
        results = json.loads(out)
        assert not results.get('failed')
>       assert not results['changed']
E       assert not True

That is defined in
https://github.com/ansible-collections/community.general/blob/main/tests/unit/plugins/modules/test_nmcli.py#L2900-L2911

Please note the parametrization there uses TESTCASE_IPIP as input. That is defined in
https://github.com/ansible-collections/community.general/blob/main/tests/unit/plugins/modules/test_nmcli.py#L877-L888

TESTCASE_IPIP = [
    {
        'type': 'ipip',
        'conn_name': 'non_existent_nw_device',
        'ifname': 'ipip-existent_nw_device',
        'ip_tunnel_dev': 'non_existent_ipip_device',
        'ip_tunnel_local': '192.168.225.5',
        'ip_tunnel_remote': '192.168.225.6',
        'state': 'present',
        '_ansible_check_mode': False,
    }
]

So, that test calls the module using those parameters - it seems to try and create (state=present) an type=ipip connection using those parameters.
In the module itself, the code block that handles state=present is:
https://github.com/ansible-collections/community.general/blob/main/plugins/modules/nmcli.py#L2725-L2750

I will assume that, in the context of those tests, the connection did not exist before. And, given that the module did not fail (in the test code, the assertion for failed passes, just before the offending assertion for changed), we can narrow it down to:
https://github.com/ansible-collections/community.general/blob/main/plugins/modules/nmcli.py#L2744-L2748

Finally, the changed return value is set at:
https://github.com/ansible-collections/community.general/blob/main/plugins/modules/nmcli.py#L2775-L2778

    if rc is None:
        result['changed'] = False
    else:
        result['changed'] = True

So, basically, the test expected changed to be False, but it is not, implying that rc is None when the test expected it to be 0 (otherwise the module would have failed in https://github.com/ansible-collections/community.general/blob/main/plugins/modules/nmcli.py#L2750 ). So, why does rc is None and not 0 when it comes from nmcli.create_connection()? That's the question that needs answering.

---

That being said, if we search for bond-slave in the test module, all we have is:
https://github.com/ansible-collections/community.general/blob/main/tests/unit/plugins/modules/test_nmcli.py#L18-L48

Which clearly states it tries to state=absent the connection. If you scroll down a bit within that same dict you will see a testcase for ipip being removed as well. That test did not fail. But bond-slave has no test code for state=present.

That explains the difference you see.

---

To run tests locally, you will need a dockerd (or equivalent, like containerd) running, and I suggest using andebox (by yours truly):

https://pypi.org/project/andebox/

You can run this unit test easily in your local machine by simply running (for example):

$ andebox test -- units -v --docker default --python 3.11 tests/unit/plugins/modules/test_nmcli.py

(I used this exact line to replicate the error locally)