tests: Prioritize find link info by permanent MAC address, with fallback to current address #751
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #751 +/- ##
==========================================
+ Coverage 43.11% 43.13% +0.01%
==========================================
Files 12 12
Lines 3124 3125 +1
==========================================
+ Hits 1347 1348 +1
Misses 1777 1777 ☔ View full report in Codecov by Sentry. |
liangwen12year
force-pushed
the
link_info_find_fix
branch
2 times, most recently
from
75a9f57 to
855766f
Compare
| lsr_fail_debug: | ||
| - __network_connections_result | ||
| tags: | ||
| - "tests::mac" |
There was a problem hiding this comment.
@spetrosi I think we'll need to exclude this test from automated testing - we'll need to add tests::mac to the --skip-tags list @liangwen12year I think this test is meant to be run interactively
There was a problem hiding this comment.
Created a PR for that linux-system-roles/tft-tests#65
There was a problem hiding this comment.
I don't follow why the test cannot determine the MAC address itself instead of asking for it in a prompt.
There was a problem hiding this comment.
The test is supposed to operate on an interface which has the permanent address, I do not want to make the test accidentally configured on the interface that is used to bridge the ssh connection between the control node and managed host. Possibly, I can search through all the links in /sys/class/net/ and use the first unconfigured ethernet interface. But such an ethernet interface (unconfigured and also have a permanent mac address) may not exist in the system environment of upstream testing.
There was a problem hiding this comment.
You are right. We can use ethtool -P $iface to determine the mac address of the interface.
There was a problem hiding this comment.
We controll the upstream test envirionment, so it should be clear whether or not such an interface exists and if it does not, there might be an option to create such an interface.
Nevertheless, the prompt also does not specify the requirements for the interface to ensure that it is eligible/the test does not ensure that it actually tests what it is supposed to test.
There was a problem hiding this comment.
Honestly, it turns out that the upstream testing environment does not have the interface with the permanent mac address. I also tried iproute2 , systemd-networkd, and NetworkManager, but I can not create such an interface with the permanent mac address (the permanent mac address can be queried by calling ethtool -P $iface ), I can only create an interface with the current mac address (the current mac address can be queried by calling cat /sys/class/net/$iface/address ).
There was a problem hiding this comment.
I specified the requirements for the interface to ensure that it is eligible.
There was a problem hiding this comment.
How does the upstream environment work?
There was a problem hiding this comment.
Summarized from the slack discussion, the upstream testing uses Testing Farm, which uses AWS machines, we do not run kvm/qemu virtual machines on the AWS machines. According to the doc https://tmt.readthedocs.io/en/stable/spec/hardware.html#network, requesting a machine with extra NICs should be already supported via network.type, we should be able to provision a machine with 2 NICs by specifying the following in the network system role test configuration.
hardware:
network:
- type: eth
- type: eth
855766f to
4538327
Compare
linux-system-roles/network#751 adds a test that must be skipped upstream and will only be tested downstream
liangwen12year
force-pushed
the
link_info_find_fix
branch
9 times, most recently
from
a1bac2e to
9cd64bb
Compare
| hosts: all | ||
| vars_prompt: | ||
| - name: "interface" | ||
| prompt: "The test requires matching an Ethernet interface to its |
There was a problem hiding this comment.
From what I understand, the current MAC address of this interface needs to be different than the permanent MAC address for the new code to actually be tested. And after further discussion, there should probably be a second interface (maybe a virtual one) that has the same MAC address as the permanent MAC address as this was causing an error condition. @liangwen12year told me about this in a meeting, AFAIU, this is not mentioned in the original PR.
There was a problem hiding this comment.
https://issues.redhat.com/browse/RHEL-73442, the issue occurs when both interface name and MAC address are specified in network connections to configure a parent and VLAN connection, and the configuration is applied multiple times. The error is raised because SysUtil._link_infos_fetch() creates a dictionary of link info from /sys/class/net/, comparing the user-specified MAC address against either the "perm-address" or "address." If the user-specified MAC matches only the "address" (current MAC) but not the "perm-address" (permanent MAC), the link info returned may have a different interface name than specified, leading to the error. If a physical interface has permanent MAC A and current MAC B, specifying MAC A in the network connection avoids errors because the VLAN's current MAC matches the parent's, but the VLAN’s perm-address is unset, so its link info isn’t used for comparing the interface name against the user-specified one. However, specifying MAC B causes NetworkManager to fail, as it expects the interface name to match the permanent MAC address, not the current one.
From my understanding, the current mac address of this interface should be the same as the permanent mac, then the created vlan interface will have the same current mac address as the permanent mac address of its parent (a virtual interface like a VLAN typically inherits the current MAC address of its parent interface instead of the permanent MAC.), then without the prior fix (c341683) on the issue, I can reproduce the problem by applying the same network connections multiple times for configuring the parent and the vlan connections.
liangwen12year
force-pushed
the
link_info_find_fix
branch
3 times, most recently
from
50594c8 to
c4ac501
Compare
liangwen12year
force-pushed
the
link_info_find_fix
branch
from
c4ac501 to
1871ed1
Compare
| @@ -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/")): | |||
There was a problem hiding this comment.
Please reduce the commit message to the essence of why this change is useful.
There was a problem hiding this comment.
I rephrased it to capture the essence of the reason for the change.
| @@ -222,31 +222,29 @@ def link_infos(cls, refresh=False): | |||
| return linkinfos | |||
|
|
|||
| @classmethod | |||
| def link_info_find(cls, refresh=False, mac=None, ifname=None): | |||
There was a problem hiding this comment.
The first line of the commit message is unnecessarily long, please make it shorter.
There was a problem hiding this comment.
I rewrote it as refactor: Make SysUtil.link_info_find() more pythonic. Thanks.
library/network_connections.py
Outdated
| if mac == perm_address: | ||
| return linkinfo | ||
| result = None | ||
| null_mac = "00:00:00:00:00:00" |
There was a problem hiding this comment.
please make this a constant (all caps), define it outside the method.
There was a problem hiding this comment.
I defined it as a constant outside the method. Thanks.
library/network_connections.py
Outdated
| if "matched_by_address" in locals(): | ||
| return matched_by_address | ||
| if mac: | ||
| if perm_address != null_mac and mac == perm_address: |
There was a problem hiding this comment.
What's the reason to check that the permanent mac address is not null here?
There was a problem hiding this comment.
null mac is an indication of the absence of the mac address or uninitialized mac address, checking the permanent mac address is not null here can guarantee that the permanent mac address is valid and initialized.
As alluded in https://standards-support.ieee.org/hc/en-us/articles/4888705676564-Guidelines-for-Use-of-Extended-Unique-Identifier-EUI-Organizationally-Unique-Identifier-OUI-and-Company-ID-CID, EUI-48 and EUI-64 identifiers ( Extended Unique Identifier) are most commonly used as globally unique network addresses (sometimes called MAC addresses), and 00-00-00-00-00-00 shall not be used as identifiers.
There was a problem hiding this comment.
The function get_perm_addr() effectively converts the unset/uninitialized MAC into 00:00:00:00:00:00
https://github.com/linux-system-roles/network/blob/main/module_utils/network_lsr/ethtool.py#L39.
liangwen12year
force-pushed
the
link_info_find_fix
branch
3 times, most recently
from
c952ae4 to
01b8e97
Compare
|
[citest] |
liangwen12year
force-pushed
the
link_info_find_fix
branch
from
01b8e97 to
99eeebe
Compare
|
[citest] |
liangwen12year
force-pushed
the
link_info_find_fix
branch
from
99eeebe to
e7d6845
Compare
|
[citest] |
1 similar comment
|
[citest] |
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 <[email protected]>
- 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 <[email protected]>
…t 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 (c341683) 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 <[email protected]>
liangwen12year
force-pushed
the
link_info_find_fix
branch
from
e7d6845 to
e056f6e
Compare
|
[citest] |
Add the integration test
tests_mac_address_match.ymlto verify that the commit "Prioritize find link info by permanent MAC address, with fallback to current address" (c341683) can properly resolve the scenarios where multiple network interfaces share the same current MAC address, leading to potential ambiguity in link matching (for example, Virtual interfaces or VLANs, which often lack a valid perm-address and rely on the parent interface's address).Notice that the test
tests_mac_address_match.ymlwill be skipped in upstream testing and it will only be manually tested downstream since it requires to know the mac address to match when configuring vlan's parent.Enhancement:
Reason:
Result:
Issue Tracker Tickets (Jira or BZ if any):
Resolves: https://issues.redhat.com/browse/RHEL-74211