From fdace3850191424eb442f39adbcff71eff2f3fa6 Mon Sep 17 00:00:00 2001 From: Asif Draxi <47986843+AsifAd@users.noreply.github.com> Date: Sat, 23 May 2026 17:05:40 +0530 Subject: [PATCH] nmcli: fix check/diff for bond arp_interval and arp_ip_target (#11588) (#12085) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * nmcli: bond ARP options stop lying in check/diff (#11588) Align arp_interval/arp_ip_target keys with bond.options parsing, route ARP settings via +bond.options, and fix bond MTU false positives. * Changelog: nmcli fragment gets PR links and clearer diff wording Address reviewer feedback on #12085 — both entries now cite the PR URL and the MTU entry says "incorrectly reports diff" instead of "false positives". --------- Co-authored-by: Asif Draxi --- .../fragments/11588-nmcli-bond-arp-diff.yml | 3 + plugins/modules/nmcli.py | 13 +- tests/unit/plugins/modules/test_nmcli.py | 268 +++++++++++++++++- 3 files changed, 275 insertions(+), 9 deletions(-) create mode 100644 changelogs/fragments/11588-nmcli-bond-arp-diff.yml diff --git a/changelogs/fragments/11588-nmcli-bond-arp-diff.yml b/changelogs/fragments/11588-nmcli-bond-arp-diff.yml new file mode 100644 index 0000000000..22c0898b1f --- /dev/null +++ b/changelogs/fragments/11588-nmcli-bond-arp-diff.yml @@ -0,0 +1,3 @@ +bugfixes: + - nmcli - fix check/diff reporting changes for bond ``arp_interval`` and ``arp_ip_target`` options when they are already configured (https://github.com/ansible-collections/community.general/issues/11588, https://github.com/ansible-collections/community.general/pull/12085). + - nmcli - fix incorrectly reports diff for bond connections when ``mtu`` is unset and NetworkManager reports no explicit MTU value (https://github.com/ansible-collections/community.general/pull/12085). diff --git a/plugins/modules/nmcli.py b/plugins/modules/nmcli.py index 189b0b77f8..4c8862f7f2 100644 --- a/plugins/modules/nmcli.py +++ b/plugins/modules/nmcli.py @@ -1941,8 +1941,8 @@ class Nmcli: if self.type == "bond": options.update( { - "arp-interval": self.arp_interval, - "arp-ip-target": self.arp_ip_target, + "arp_interval": self.arp_interval, + "arp_ip_target": self.arp_ip_target, "downdelay": self.downdelay, "miimon": self.miimon, "mode": self.mode, @@ -2474,11 +2474,8 @@ class Nmcli: if key in self.SECRET_OPTIONS: self.edit_commands += [f"set {key} {value}"] continue - if key == "xmit_hash_policy": - cmd.extend(["+bond.options", f"xmit_hash_policy={value}"]) - continue - if key == "fail_over_mac": - cmd.extend(["+bond.options", f"fail_over_mac={value}"]) + if key in ("xmit_hash_policy", "fail_over_mac", "arp_interval", "arp_ip_target"): + cmd.extend(["+bond.options", f"{key}={value}"]) continue cmd.extend([key, value]) @@ -2680,7 +2677,7 @@ class Nmcli: elif all( [ key == self.mtu_setting, - self.type == "dummy", + self.type in ("bond", "bond-slave", "dummy"), current_value is None, value == "auto", self.mtu is None, diff --git a/tests/unit/plugins/modules/test_nmcli.py b/tests/unit/plugins/modules/test_nmcli.py index e31ddb6a2f..c73af9ab15 100644 --- a/tests/unit/plugins/modules/test_nmcli.py +++ b/tests/unit/plugins/modules/test_nmcli.py @@ -617,9 +617,107 @@ ipv4.may-fail: yes ipv6.method: auto ipv6.ignore-auto-dns: no ipv6.ignore-auto-routes: no +802-3-ethernet.mtu: auto bond.options: mode=active-backup,primary=non_existent_primary,xmit_hash_policy=layer3+4 """ +TESTCASE_BOND_ARP = [ + { + "type": "bond", + "conn_name": "non_existent_nw_device", + "ifname": "bond_non_existant", + "mode": "active-backup", + "arp_interval": 100, + "arp_ip_target": "192.168.1.1", + "state": "present", + "_ansible_check_mode": False, + } +] + +TESTCASE_BOND_ARP_SHOW_OUTPUT = """\ +connection.id: non_existent_nw_device +connection.interface-name: bond_non_existant +connection.autoconnect: yes +ipv4.method: auto +ipv4.ignore-auto-dns: no +ipv4.ignore-auto-routes: no +ipv4.never-default: no +ipv4.may-fail: yes +ipv6.method: auto +ipv6.ignore-auto-dns: no +ipv6.ignore-auto-routes: no +802-3-ethernet.mtu: auto +bond.options: mode=active-backup,arp_interval=100,arp_ip_target=192.168.1.1 +""" + +TESTCASE_BOND_ARP_MODIFY = [ + { + "type": "bond", + "conn_name": "non_existent_nw_device", + "ifname": "bond_non_existant", + "mode": "active-backup", + "arp_interval": 200, + "arp_ip_target": "192.168.1.1", + "state": "present", + "_ansible_check_mode": False, + } +] + +TESTCASE_BOND_ARP_TARGET_MODIFY = [ + { + "type": "bond", + "conn_name": "non_existent_nw_device", + "ifname": "bond_non_existant", + "mode": "active-backup", + "arp_interval": 100, + "arp_ip_target": "192.168.1.2", + "state": "present", + "_ansible_check_mode": False, + } +] + +TESTCASE_BOND_ARP_CHECK_MODE = [ + { + "type": "bond", + "conn_name": "non_existent_nw_device", + "ifname": "bond_non_existant", + "mode": "active-backup", + "arp_interval": 100, + "arp_ip_target": "192.168.1.1", + "state": "present", + "_ansible_check_mode": True, + } +] + +TESTCASE_BOND_ARP_ZERO = [ + { + "type": "bond", + "conn_name": "non_existent_nw_device", + "ifname": "bond_non_existant", + "mode": "active-backup", + "arp_interval": 0, + "arp_ip_target": "192.168.1.1", + "state": "present", + "_ansible_check_mode": False, + } +] + +TESTCASE_BOND_ARP_ZERO_SHOW_OUTPUT = """\ +connection.id: non_existent_nw_device +connection.interface-name: bond_non_existant +connection.autoconnect: yes +ipv4.method: auto +ipv4.ignore-auto-dns: no +ipv4.ignore-auto-routes: no +ipv4.never-default: no +ipv4.may-fail: yes +ipv6.method: auto +ipv6.ignore-auto-dns: no +ipv6.ignore-auto-routes: no +802-3-ethernet.mtu: auto +bond.options: mode=active-backup,arp_interval=0,arp_ip_target=192.168.1.1 +""" + TESTCASE_BRIDGE = [ { "type": "bridge", @@ -1719,6 +1817,29 @@ def mocked_bond_connection_unchanged(mocker): mocker_set(mocker, connection_exists=True, execute_return=(0, TESTCASE_BOND_SHOW_OUTPUT, "")) +@pytest.fixture +def mocked_bond_arp_connection_unchanged(mocker): + mocker_set(mocker, connection_exists=True, execute_return=(0, TESTCASE_BOND_ARP_SHOW_OUTPUT, "")) + + +@pytest.fixture +def mocked_bond_arp_zero_connection_unchanged(mocker): + mocker_set(mocker, connection_exists=True, execute_return=(0, TESTCASE_BOND_ARP_ZERO_SHOW_OUTPUT, "")) + + +@pytest.fixture +def mocked_bond_arp_connection_modify(mocker): + mocker_set( + mocker, + connection_exists=True, + execute_return=None, + execute_side_effect=( + (0, TESTCASE_BOND_ARP_SHOW_OUTPUT, ""), + (0, "", ""), + ), + ) + + @pytest.fixture def mocked_bridge_connection_unchanged(mocker): mocker_set(mocker, connection_exists=True, execute_return=(0, TESTCASE_BRIDGE_SHOW_OUTPUT, "")) @@ -2142,7 +2263,6 @@ def test_bond_connection_create(mocked_generic_connection_create, capfd): assert results["changed"] -@pytest.mark.skip(reason="Currently broken") # TODO: fix me! @pytest.mark.parametrize("patch_ansible_module", TESTCASE_BOND, indirect=["patch_ansible_module"]) def test_bond_connection_unchanged(mocked_bond_connection_unchanged, capfd): """ @@ -2157,6 +2277,152 @@ def test_bond_connection_unchanged(mocked_bond_connection_unchanged, capfd): assert not results["changed"] +@pytest.mark.parametrize("patch_ansible_module", TESTCASE_BOND_ARP, indirect=["patch_ansible_module"]) +def test_bond_arp_connection_unchanged(mocked_bond_arp_connection_unchanged): + """ + Regression test for #11588: + Bond ARP options must not be reported as changed when already configured. + """ + module = nmcli.create_module() + nmcli_module = nmcli.Nmcli(module) + + changed, diff = nmcli_module.is_connection_changed() + + mismatches = { + key: (diff["before"][key], diff["after"][key]) + for key in diff["before"] + if diff["before"][key] != diff["after"][key] + } + assert not changed, f"unexpected changes: {mismatches}" + assert diff["before"]["arp_interval"] == "100" + assert diff["after"]["arp_interval"] == "100" + assert diff["before"]["arp_ip_target"] == "192.168.1.1" + assert diff["after"]["arp_ip_target"] == "192.168.1.1" + + +@pytest.mark.parametrize("patch_ansible_module", TESTCASE_BOND_ARP, indirect=["patch_ansible_module"]) +def test_bond_arp_connection_unchanged_main(mocked_bond_arp_connection_unchanged, capfd): + """ + Regression test for #11588 via main(): + Second run of a bond with ARP options must report changed=false. + """ + with pytest.raises(SystemExit): + nmcli.main() + + out, err = capfd.readouterr() + results = json.loads(out) + assert not results.get("failed") + assert not results["changed"] + + +@pytest.mark.parametrize("patch_ansible_module", TESTCASE_BOND_ARP_ZERO, indirect=["patch_ansible_module"]) +def test_bond_arp_interval_zero_unchanged(mocked_bond_arp_zero_connection_unchanged): + """ + arp_interval=0 is a valid value and must not be treated as unset. + """ + module = nmcli.create_module() + nmcli_module = nmcli.Nmcli(module) + + changed, diff = nmcli_module.is_connection_changed() + + assert not changed + assert diff["before"]["arp_interval"] == "0" + assert diff["after"]["arp_interval"] == "0" + + +@pytest.mark.parametrize("patch_ansible_module", TESTCASE_BOND_ARP_MODIFY, indirect=["patch_ansible_module"]) +def test_bond_arp_connection_changed(mocked_bond_arp_connection_unchanged): + """ + When ARP interval differs from the existing connection, a change must be reported. + """ + module = nmcli.create_module() + nmcli_module = nmcli.Nmcli(module) + + changed, diff = nmcli_module.is_connection_changed() + + assert changed + assert diff["before"]["arp_interval"] == "100" + assert diff["after"]["arp_interval"] == "200" + assert diff["before"]["arp_ip_target"] == diff["after"]["arp_ip_target"] == "192.168.1.1" + + +@pytest.mark.parametrize("patch_ansible_module", TESTCASE_BOND_ARP_TARGET_MODIFY, indirect=["patch_ansible_module"]) +def test_bond_arp_ip_target_changed(mocked_bond_arp_connection_unchanged): + """ + When only arp_ip_target differs from the existing connection, a change must be reported. + """ + module = nmcli.create_module() + nmcli_module = nmcli.Nmcli(module) + + changed, diff = nmcli_module.is_connection_changed() + + assert changed + assert diff["before"]["arp_interval"] == diff["after"]["arp_interval"] == "100" + assert diff["before"]["arp_ip_target"] == "192.168.1.1" + assert diff["after"]["arp_ip_target"] == "192.168.1.2" + + +@pytest.mark.parametrize("patch_ansible_module", TESTCASE_BOND_ARP_CHECK_MODE, indirect=["patch_ansible_module"]) +def test_bond_arp_connection_unchanged_check_mode(mocked_bond_arp_connection_unchanged, capfd): + """ + Check mode must not report changed when bond ARP options already match. + """ + with pytest.raises(SystemExit): + nmcli.main() + + assert nmcli.Nmcli.execute_command.call_count == 1 + out, err = capfd.readouterr() + results = json.loads(out) + assert not results.get("failed") + assert not results["changed"] + + +@pytest.mark.parametrize("patch_ansible_module", TESTCASE_BOND_ARP_MODIFY, indirect=["patch_ansible_module"]) +def test_bond_arp_connection_modify(mocked_bond_arp_connection_modify, capfd): + """ + Test : Bond connection modify uses bond.options syntax for ARP options + """ + with pytest.raises(SystemExit): + nmcli.main() + + assert nmcli.Nmcli.execute_command.call_count == 2 + arg_list = nmcli.Nmcli.execute_command.call_args_list + modify_args, modify_kw = arg_list[1] + + modify_args_text = [to_text(x) for x in modify_args[0]] + assert modify_args[0][2] == "modify" + assert "+bond.options" in modify_args_text + assert "arp_interval=200" in modify_args_text + + out, err = capfd.readouterr() + results = json.loads(out) + assert not results.get("failed") + assert results["changed"] + + +@pytest.mark.parametrize("patch_ansible_module", TESTCASE_BOND_ARP, indirect=["patch_ansible_module"]) +def test_bond_arp_connection_create(mocked_generic_connection_create, capfd): + """ + Test : Bond connection with ARP options created via bond.options syntax + """ + with pytest.raises(SystemExit): + nmcli.main() + + assert nmcli.Nmcli.execute_command.call_count == 1 + arg_list = nmcli.Nmcli.execute_command.call_args_list + args, kwargs = arg_list[0] + + args_text = [to_text(x) for x in args[0]] + assert "+bond.options" in args_text + assert "arp_interval=100" in args_text + assert "arp_ip_target=192.168.1.1" in args_text + + out, err = capfd.readouterr() + results = json.loads(out) + assert not results.get("failed") + assert results["changed"] + + @pytest.mark.parametrize("patch_ansible_module", TESTCASE_GENERIC, indirect=["patch_ansible_module"]) def test_generic_connection_create(mocked_generic_connection_create, capfd): """