chore(tp-libvirt): Handle network interfaces in UNKNOWN state for connection validation #6753
Conversation
WalkthroughThe pull request adds fallback interface discovery logic to two network test files. When no UP network interfaces are available, the code now retrieves interfaces in UNKNOWN state and attempts to ping google.com on each. The first interface that successfully responds to the ping is added to the UP interface list, allowing subsequent source-device validation to proceed. This introduces a new conditional path in the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom Pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @libvirt/tests/src/virtual_network/iface_network.py:
- Around line 128-137: The fallback ping loop using utils_net.get_net_if and
utils_net.ping duplicated in iface_network.py and iface_options.py should be
extracted into a shared helper (e.g., get_usable_network_interface) that accepts
params for prefer_state/fallback_state, ping_target, ping_count and
ping_timeout; replace the inline loop that appends to net_ifs with a call to
this helper, reduce ping timeout to ~10s and ping_count to 2, add logging
warnings/info/errors when falling back, and make the ping_target configurable
(or optional) so tests can use local gateway or disable external checks to avoid
external DNS/internet dependencies.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/src/virtual_network/iface_network.pylibvirt/tests/src/virtual_network/iface_options.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/src/virtual_network/iface_network.pylibvirt/tests/src/virtual_network/iface_options.py
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
libvirt/tests/src/virtual_network/iface_network.py
🧬 Code graph analysis (2)
libvirt/tests/src/virtual_network/iface_network.py (1)
libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
libvirt/tests/src/virtual_network/iface_options.py (1)
libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
| # if no interfaces are available in UP state, | ||
| # iterate through all interfaces with UNKNOWN | ||
| # state and check if they can be pinged to the | ||
| # internet through. | ||
| if len(net_ifs) == 0: | ||
| unknown_net_ifs = utils_net.get_net_if(state="UNKNOWN") | ||
| for interface in unknown_net_ifs: | ||
| if utils_net.ping(dest="google.com", interface=interface, count=5, timeout=120)[0] == 0: | ||
| net_ifs.append(interface) | ||
| break |
There was a problem hiding this comment.
Extract duplicated fallback logic and reduce external dependencies.
This fallback mechanism is duplicated in both iface_network.py and iface_options.py. Additionally, relying on google.com connectivity introduces several issues:
- Test fragility: Tests become dependent on external DNS resolution, internet connectivity, and firewall rules—any of which can cause non-deterministic failures in isolated or restricted environments.
- Long timeout: The 120-second timeout per interface can significantly slow down test execution when multiple UNKNOWN interfaces exist.
- Silent failure: If no UNKNOWN interface is reachable,
net_ifsremains empty, potentially causing downstream failures without clear diagnostics. - No observability: The fallback path is not logged, making debugging harder.
Recommendations
-
Extract to a helper function in a shared module to eliminate duplication:
def get_usable_network_interface(prefer_state="UP", fallback_state="UNKNOWN", ping_target="google.com", ping_count=2, ping_timeout=10): """Get a usable network interface, with fallback if needed.""" net_ifs = utils_net.get_net_if(state=prefer_state) if not net_ifs: logging.warning("No %s interfaces found, checking %s interfaces", prefer_state, fallback_state) unknown_ifs = utils_net.get_net_if(state=fallback_state) for iface in unknown_ifs: status, _ = utils_net.ping(dest=ping_target, interface=iface, count=ping_count, timeout=ping_timeout) if status == 0: logging.info("Found reachable %s interface: %s", fallback_state, iface) net_ifs.append(iface) break if not net_ifs: logging.error("No reachable interfaces found") return net_ifs
-
Make ping target configurable via test parameters to support different environments.
-
Reduce timeout from 120s to something more reasonable (e.g., 10-20s) to avoid test slowness.
-
Consider alternative approaches:
- Check interface carrier status via
sys/class/net/{iface}/carrier - Ping a local gateway instead of the internet
- Make external connectivity check optional via test configuration
- Check interface carrier status via
🤖 Prompt for AI Agents
In @libvirt/tests/src/virtual_network/iface_network.py around lines 128 - 137,
The fallback ping loop using utils_net.get_net_if and utils_net.ping duplicated
in iface_network.py and iface_options.py should be extracted into a shared
helper (e.g., get_usable_network_interface) that accepts params for
prefer_state/fallback_state, ping_target, ping_count and ping_timeout; replace
the inline loop that appends to net_ifs with a call to this helper, reduce ping
timeout to ~10s and ping_count to 2, add logging warnings/info/errors when
falling back, and make the ping_target configurable (or optional) so tests can
use local gateway or disable external checks to avoid external DNS/internet
dependencies.
| net_ifs = utils_net.get_net_if(state="UP") | ||
| # if no interfaces are available in UP state, | ||
| # iterate through all interfaces with UNKNOWN | ||
| # state and check if they can be pinged to the | ||
| # internet through. | ||
| if len(net_ifs) == 0: | ||
| unknown_net_ifs = utils_net.get_net_if(state="UNKNOWN") | ||
| for interface in unknown_net_ifs: | ||
| if utils_net.ping(dest="google.com", interface=interface, count=5, timeout=120)[0] == 0: | ||
| net_ifs.append(interface) | ||
| break |
There was a problem hiding this comment.
Duplicate code: same concerns as in iface_network.py.
This code block is identical to the fallback logic in libvirt/tests/src/virtual_network/iface_network.py (lines 128-137). All concerns raised there apply here as well: the hardcoded google.com dependency, lack of error handling, excessive timeout, and missing logging.
Consolidating this logic into a shared helper function would address the duplication and make it easier to improve the implementation once rather than maintaining it in multiple places.
The current implementation is looking for interfaces in the UP state, while ignoring interfaces in the UNKNOWN state, which was causing tests to fail on hosts where no interfaces are in the UP state. Added logic to iterate through interfaces in UNKNOWN state, and check which one can be used to ping to the outside world, then using the same interface for the purpose of the tests where it is required.
aniket-sahu-ibmx
force-pushed
the
KVMCI
branch
from
7c12b3b to
54b2fae
Compare
aniket-sahu-ibmx
changed the title
aniket-sahu-ibmx
changed the title
1 participant
The current implementation is looking for interfaces in the UP state, while ignoring interfaces in the UNKNOWN state, which was causing tests to fail on hosts where no interfaces are in the UP state. Added logic to iterate through interfaces in UNKNOWN state, and check which one can be used to ping to the outside world, then using the same interface for the purpose of the tests where it is required.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.