From 46c3cea7942088628a257d849e840d8d7b877e58 Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Mon, 3 Feb 2025 19:33:31 -0500 Subject: [PATCH 1/3] fix: Sort links before fetching sysfs link infos When fetching link info from sysfs, `os.listdir(path)` returns entries in arbitrary order, which can cause intermittent errors when a VLAN device appears before its parent. This leads to a "no such interface exists" error on repeated configuration application. Sorting the list before processing ensures deterministic behavior, making it consistent and more predictable given the VLAN's device name. Signed-off-by: Wen Liang --- library/network_connections.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/network_connections.py b/library/network_connections.py index fec1fb04..9bfdc3cc 100644 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -173,7 +173,7 @@ def _link_read_permaddress(ifname): @staticmethod def _link_infos_fetch(): links = {} - for ifname in os.listdir("/sys/class/net/"): + for ifname in sorted(os.listdir("/sys/class/net/")): if not os.path.islink("/sys/class/net/" + ifname): # /sys/class/net may contain certain entries # that are not interface names, like From 894645b9990ed04e411dd6ce102ccc932b9530fc Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Mon, 3 Feb 2025 20:11:20 -0500 Subject: [PATCH 2/3] refactor: Make SysUtil.link_info_find() more pythonic - Removed the unnecessary `refresh` argument since it wasn't used. - Used `None` checks more idiomatically with `if mac` instead of `is not None`. - Eliminated redundant variables and conditions to improve readability. - Avoided using `locals()` by explicitly storing fallback results. - Made `ifname` matching take priority before checking MAC addresses. - Ensured that the function returns early when a definitive match is found. Signed-off-by: Wen Liang --- library/network_connections.py | 36 ++++++++++++++++------------------ 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/library/network_connections.py b/library/network_connections.py index 9bfdc3cc..b4a679ac 100644 --- a/library/network_connections.py +++ b/library/network_connections.py @@ -95,6 +95,7 @@ DEFAULT_ACTIVATION_TIMEOUT = 90 DEFAULT_TIMEOUT = 10 +NULL_MAC = "00:00:00:00:00:00" class CheckMode: @@ -222,31 +223,28 @@ def link_infos(cls, refresh=False): return linkinfos @classmethod - def link_info_find(cls, refresh=False, mac=None, ifname=None): - if mac is not None: + def link_info_find(cls, mac=None, ifname=None): + if mac: mac = Util.mac_norm(mac) - for linkinfo in cls.link_infos(refresh).values(): - perm_address = linkinfo.get("perm-address", None) - current_address = linkinfo.get("address", None) - # Match by perm-address (prioritized) - if mac is not None and perm_address not in [None, "00:00:00:00:00:00"]: - if mac == perm_address: - return linkinfo + result = None - # Fallback to match by address - if mac is not None and (perm_address in [None, "00:00:00:00:00:00"]): - if mac == current_address: - matched_by_address = linkinfo # Save for potential fallback + for linkinfo in cls.link_infos().values(): + perm_address = linkinfo.get("perm-address", NULL_MAC) + current_address = linkinfo.get("address", NULL_MAC) - if ifname is not None and ifname == linkinfo.get("ifname", None): - return linkinfo + if ifname and ifname == linkinfo["ifname"]: + result = linkinfo + break - # Return fallback match by address if no perm-address match found - if "matched_by_address" in locals(): - return matched_by_address + if mac: + if perm_address != NULL_MAC and mac == perm_address: + result = linkinfo + break + elif perm_address == NULL_MAC and mac == current_address: + result = linkinfo - return None + return result ############################################################################### From a380ac46acf2753b49bf5cc4c94f7b5f5ee36967 Mon Sep 17 00:00:00 2001 From: Wen Liang Date: Sat, 18 Jan 2025 14:27:45 -0500 Subject: [PATCH 3/3] tests: Prefer permanent MAC for finding link info, fallback to current MAC When a network connection specifies both an interface name and MAC address, and the physical interface has identical permanent and current MACs, applying the configuration multiple times can cause an error: "no such interface exists." The issue occurs because `SysUtil.link_info_find()` may return a link info entry where the user-specified MAC matches only the current ("address") but not the permanent MAC ("perm-address"). In such cases, the returned link info may have a different interface name than the one specified in the network connection, leading to the error. We already implemented a fix (c3416831) that ensures link lookup prioritizes the permanent MAC, only falling back to the current MAC when necessary. The integration test `tests_mac_address_match.yml` verifies this fix by requiring an Ethernet interface where the permanent and current MACs match. Resolves: https://issues.redhat.com/browse/RHEL-74211 Signed-off-by: Wen Liang --- tests/ensure_provider_tests.py | 1 + tests/playbooks/tests_mac_address_match.yml | 75 +++++++++++++++++++ .../assert_network_connections_succeeded.yml | 8 ++ ...cleanup_vlan_and_parent_profile+device.yml | 26 +++++++ tests/tasks/create_mac_address_match.yml | 51 +++++++++++++ tests/tasks/find+remove_profile.yml | 15 ++++ tests/tests_mac_address_match_nm.yml | 23 ++++++ 7 files changed, 199 insertions(+) create mode 100644 tests/playbooks/tests_mac_address_match.yml create mode 100644 tests/tasks/assert_network_connections_succeeded.yml create mode 100644 tests/tasks/cleanup_vlan_and_parent_profile+device.yml create mode 100644 tests/tasks/create_mac_address_match.yml create mode 100644 tests/tasks/find+remove_profile.yml create mode 100644 tests/tests_mac_address_match_nm.yml diff --git a/tests/ensure_provider_tests.py b/tests/ensure_provider_tests.py index 6e0643db..d1d1dc46 100755 --- a/tests/ensure_provider_tests.py +++ b/tests/ensure_provider_tests.py @@ -79,6 +79,7 @@ "playbooks/tests_infiniband.yml": {}, "playbooks/tests_ipv6_disabled.yml": {}, "playbooks/tests_ipv6_dns_search.yml": {}, + "playbooks/tests_mac_address_match.yml": {}, "playbooks/tests_provider.yml": { MINIMUM_VERSION: "'1.20.0'", "comment": "# NetworKmanager 1.20.0 added support for forgetting profiles", diff --git a/tests/playbooks/tests_mac_address_match.yml b/tests/playbooks/tests_mac_address_match.yml new file mode 100644 index 00000000..cc9ca224 --- /dev/null +++ b/tests/playbooks/tests_mac_address_match.yml @@ -0,0 +1,75 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Play for testing MAC address match on device + hosts: all + vars: + # This test requires an Ethernet interface whose permanent + # MAC address matches its current MAC address. Ensure that the + # specified interface meets this condition. + # + # Two VLAN profiles are defined to test deterministic behavior when fetching + # link information from sysfs. The issue being tested arises when `os.listdir(path)` + # returns interfaces in an arbitrary order, potentially listing a VLAN device + # before its parent interface. This can cause intermittent "no such interface + # exists" errors when applying configurations repeatedly. + # + # - `vlan_profile1` (e.g., `eth1.3732`) is named with the VLAN ID appended + # after the parent interface, following the standard `.` format. + # - `vlan_profile2` (e.g., `120-vlan`) has a fixed name, designed to test a scenario + # where lexicographic sorting causes the VLAN to appear before its parent interface. + interface: "{{ lookup('env', 'MAC_ADDR_MATCH_INTERFACE') | default('eth1', true) }}" + profile: "{{ interface }}" + vlan_profile1: "{{ interface }}.3732" + vlan_profile2: "120-vlan" + lsr_fail_debug: + - __network_connections_result + tags: + - "tests::match" + tasks: + - name: Show playbook name + debug: + msg: "this is: playbooks/tests_mac_address_match.yml" + tags: + - always + + - name: Install ethtool (test dependency) + package: + name: ethtool + state: present + use: "{{ (__network_is_ostree | d(false)) | + ternary('ansible.posix.rhel_rpm_ostree', omit) }}" + + - name: Retrieve MAC address using ethtool + command: ethtool -P {{ interface }} + register: mac_address_result + changed_when: false + failed_when: mac_address_result.rc != 0 + + - name: Set the MAC address variable + set_fact: + mac: "{{ mac_address_result.stdout_lines[-1].split(' ')[-1] }}" + + - name: Display the retrieved MAC address + debug: + msg: "Retrieved MAC address for {{ interface }}: {{ mac }}" + + - name: Test the MAC address match + tags: + - tests::match:match + block: + - name: Include the task 'run_test.yml' + include_tasks: tasks/run_test.yml + vars: + lsr_description: Test MAC address match on device + lsr_setup: + - tasks/find+remove_profile.yml + - tasks/assert_profile_absent.yml + lsr_test: + - tasks/create_mac_address_match.yml + - tasks/create_mac_address_match.yml + lsr_assert: + - tasks/assert_profile_present.yml + - tasks/assert_network_connections_succeeded.yml + lsr_cleanup: + - tasks/cleanup_vlan_and_parent_profile+device.yml + - tasks/check_network_dns.yml diff --git a/tests/tasks/assert_network_connections_succeeded.yml b/tests/tasks/assert_network_connections_succeeded.yml new file mode 100644 index 00000000..2bca1492 --- /dev/null +++ b/tests/tasks/assert_network_connections_succeeded.yml @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Assert that configuring network connections is succeeded + assert: + that: + - __network_connections_result.failed == false + msg: Configuring network connections is failed with the error + "{{ __network_connections_result.stderr }}" diff --git a/tests/tasks/cleanup_vlan_and_parent_profile+device.yml b/tests/tasks/cleanup_vlan_and_parent_profile+device.yml new file mode 100644 index 00000000..be5bc904 --- /dev/null +++ b/tests/tasks/cleanup_vlan_and_parent_profile+device.yml @@ -0,0 +1,26 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Clean up the test devices and the connection profiles + tags: + - "tests::cleanup" + block: + - name: Import network role + import_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ profile }}" + persistent_state: absent + state: down + - name: "{{ vlan_profile1 }}" + persistent_state: absent + state: down + - name: "{{ vlan_profile2 }}" + persistent_state: absent + state: down + failed_when: false + - name: Delete the device '{{ interface }}' + command: ip link del {{ interface }} + failed_when: false + changed_when: false +... diff --git a/tests/tasks/create_mac_address_match.yml b/tests/tasks/create_mac_address_match.yml new file mode 100644 index 00000000..412127bb --- /dev/null +++ b/tests/tasks/create_mac_address_match.yml @@ -0,0 +1,51 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Include network role + include_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ interface }}" + state: up + persistent_state: present + autoconnect: true + type: ethernet + interface_name: "{{ interface }}" + mac: "{{ mac }}" + ip: + dhcp4: false + auto6: false + + - name: "{{ vlan_profile1 }}" + state: up + persistent_state: present + type: vlan + parent: "{{ interface }}" + vlan: + id: 3732 + autoconnect: true + ip: + auto_gateway: false + gateway4: 10.10.0.1 + address: 10.10.0.6/24 + dhcp4: false + auto6: false + + - name: "{{ vlan_profile2 }}" + state: up + persistent_state: present + type: vlan + parent: "{{ interface }}" + vlan: + id: 120 + autoconnect: true + ip: + auto_gateway: false + gateway4: 10.10.120.1 + address: 10.10.120.120/24 + dhcp4: false + auto6: false +- name: Show result + debug: + var: __network_connections_result +... diff --git a/tests/tasks/find+remove_profile.yml b/tests/tasks/find+remove_profile.yml new file mode 100644 index 00000000..9adbf20a --- /dev/null +++ b/tests/tasks/find+remove_profile.yml @@ -0,0 +1,15 @@ +# SPDX-License-Identifier: BSD-3-Clause +--- +- name: Get connection profile for '{{ interface }}' + command: "nmcli -g GENERAL.CONNECTION device show {{ interface }}" + register: connection_name + changed_when: false + +- name: Bring down and delete the connection profile for '{{ interface }}' + include_role: + name: linux-system-roles.network + vars: + network_connections: + - name: "{{ connection_name.stdout }}" + persistent_state: absent + state: down diff --git a/tests/tests_mac_address_match_nm.yml b/tests/tests_mac_address_match_nm.yml new file mode 100644 index 00000000..bd7b5275 --- /dev/null +++ b/tests/tests_mac_address_match_nm.yml @@ -0,0 +1,23 @@ +# SPDX-License-Identifier: BSD-3-Clause +# This file was generated by ensure_provider_tests.py +--- +# set network provider and gather facts +# yamllint disable rule:line-length +- name: Run playbook 'playbooks/tests_mac_address_match.yml' with nm as provider + hosts: all + tasks: + - name: Include the task 'el_repo_setup.yml' + include_tasks: tasks/el_repo_setup.yml + - name: Set network provider to 'nm' + set_fact: + network_provider: nm + tags: + - always + + +# The test requires or should run with NetworkManager, therefore it cannot run +# on RHEL/CentOS 6 +- name: Import the playbook 'playbooks/tests_mac_address_match.yml' + import_playbook: playbooks/tests_mac_address_match.yml + when: + - ansible_distribution_major_version != '6'