Fix interface discovery, serial console and ping issues! - #6913
Fix interface discovery, serial console and ping issues!#6913Anushree-Mathur wants to merge 1 commit into
Conversation
Three related fixes to make NIC passthrough testing work on pSeries (POWER10, ppc64le) with VFIO-passthrough BCM5719 NICs. 1. Interface discovery via uevent fallback get_interface_from_pci_id() uses ethtool -i bus-info to match the PCI address. On pSeries, the host PCI domain (0018:xx) differs from the guest-visible PCI domain (0001:xx) because the hypervisor re-numbers PCI domains inside the guest. The match never succeeds and the function returns None. Fix: when get_interface_from_pci_id() returns None, fall back to reading /sys/class/net/<iface>/device/uevent PCI_SLOT_NAME which contains the guest-visible PCI address and always matches correctly. 2. Serial console port already occupied on pSeries virsh console allows only one active connection at a time on pSeries. The avocado-vt framework opens a serial console at VM boot and holds it. Any subsequent wait_for_serial_login() call blocks until timeout because the port is already occupied. Fix: call cleanup_serial_console() + create_serial_console() before wait_for_serial_login() to release the occupied port and open a fresh connection. All NIC operations (ip config, ping) run on serial_session. 3. Ping via NIC name fails on VFIO passthrough The passthrough NIC (enP1p0s1) connects to a physical switch with no L2 path to virbr0 (192.168.122.1). Using ping -I enP1p0s1 forces packets out the physical port where ARP never resolves. Fix: pass source IP as interface parameter instead of NIC name. The kernel routes via the virtio NIC (enp0s1) -> virbr0 -> gateway. Signed-off-by: Anushree-Mathur <anushree.mathur@linux.ibm.com>
WalkthroughThe change modifies the check_device_status function used in NIC passthrough IP configuration and ping testing. When the primary PCI-to-interface lookup fails, a fallback resolves the interface by scanning ip -o link show output and matching PCI_SLOT_NAME from sysfs uevent data. Interface configuration and ping verification are then moved to a newly created serial-console session instead of the original session, with netmask converted to CIDR, IP verification added, and the serial session closed in a finally block. Estimated code review effort: 3 (Moderate) | ~20 minutes Compact metadata:
Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough.py`:
- Around line 226-230: The fallback PCI matching in the PCI passthrough test
still relies on full-address substring checks, so renumbered domains won’t match
correctly. Update the comparison in the PCI address matching block to first try
exact full PCI BDF equality, then fall back to uniquely matching on the
bus:slot.function portion when the domain differs, using the existing pci_addr,
pci_normalized, and val_normalized logic around nic_name assignment.
- Around line 240-241: The NIC configuration path in libvirt_pci_passthrough.py
calls netmask_to_cidr, but that helper is undefined here, so the setup will fail
before the interface is configured. Add or import the correct netmask_to_cidr
implementation before the code that computes cidr_mask, and make sure the symbol
is available in the passthrough flow where the NIC address is built.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7cb6c6f-cf52-4d3b-a17a-54810ab85782
📒 Files selected for processing (1)
libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough.py
| # Normalize for comparison (case-insensitive) | ||
| pci_normalized = pci_addr.strip().lower() | ||
| val_normalized = val.lower() | ||
| if pci_normalized in val_normalized or val_normalized in pci_normalized: | ||
| nic_name = iface |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the PCI BDF when domains are renumbered.
The fallback still compares full PCI addresses, so domain-only differences like 0000:01:00.0 vs 0001:01:00.0 will not match. Compare exact full address first, then uniquely match on bus:slot.function.
🐛 Proposed fix
- # Normalize for comparison (case-insensitive)
- pci_normalized = pci_addr.strip().lower()
- val_normalized = val.lower()
- if pci_normalized in val_normalized or val_normalized in pci_normalized:
+ pci_normalized = pci_addr.strip().lower()
+ val_normalized = val.lower()
+ pci_bdf = pci_normalized.split(":", 1)[-1]
+ val_bdf = val_normalized.split(":", 1)[-1]
+ if pci_normalized == val_normalized or pci_bdf == val_bdf:
nic_name = iface📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Normalize for comparison (case-insensitive) | |
| pci_normalized = pci_addr.strip().lower() | |
| val_normalized = val.lower() | |
| if pci_normalized in val_normalized or val_normalized in pci_normalized: | |
| nic_name = iface | |
| pci_normalized = pci_addr.strip().lower() | |
| val_normalized = val.lower() | |
| pci_bdf = pci_normalized.split(":", 1)[-1] | |
| val_bdf = val_normalized.split(":", 1)[-1] | |
| if pci_normalized == val_normalized or pci_bdf == val_bdf: | |
| nic_name = iface |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough.py` around lines
226 - 230, The fallback PCI matching in the PCI passthrough test still relies on
full-address substring checks, so renumbered domains won’t match correctly.
Update the comparison in the PCI address matching block to first try exact full
PCI BDF equality, then fall back to uniquely matching on the bus:slot.function
portion when the domain differs, using the existing pci_addr, pci_normalized,
and val_normalized logic around nic_name assignment.
| # Convert netmask to CIDR | ||
| cidr_mask = netmask_to_cidr(netmask) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Define the netmask conversion before using it.
netmask_to_cidr is undefined, so this path will raise before configuring the NIC.
🐛 Proposed fix
- # Convert netmask to CIDR
- cidr_mask = netmask_to_cidr(netmask)
+ # Convert dotted netmask or prefix length to CIDR
+ cidr_mask = ipaddress.IPv4Network("0.0.0.0/%s" % netmask).prefixlen📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Convert netmask to CIDR | |
| cidr_mask = netmask_to_cidr(netmask) | |
| # Convert dotted netmask or prefix length to CIDR | |
| cidr_mask = ipaddress.IPv4Network("0.0.0.0/%s" % netmask).prefixlen |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 241-241: Undefined name netmask_to_cidr
(F821)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough.py` around lines
240 - 241, The NIC configuration path in libvirt_pci_passthrough.py calls
netmask_to_cidr, but that helper is undefined here, so the setup will fail
before the interface is configured. Add or import the correct netmask_to_cidr
implementation before the code that computes cidr_mask, and make sure the symbol
is available in the passthrough flow where the NIC address is built.
Source: Linters/SAST tools
|
For the CI failure fix and the proper sequence, I request maintainers to merge the following PR first: Thank you |
|
perhaps this might be useful for your dependent PR: https://docs.github.com/en/pull-requests/get-started/stacked-prs-quickstart |
hholoubk
left a comment
hholoubk
left a comment
There was a problem hiding this comment.
Please consider also things suggested by CodeRabbit.
I have also some doubts about introducing some new code where I believe there should be some already existing way to get the fall back .. but didnt have time to deepdive for it.
| # Clear console port and open a fresh serial session | ||
| # virsh console only allows one connection at a time on pSeries | ||
| logging.info("Clearing serial console before login") | ||
| vm.cleanup_serial_console() |
There was a problem hiding this comment.
Hi @Anushree-Mathur,
Please be aware, that the wait_for_serial_login avocado-vt method was refactored slightly and the serial console cleanup can be called as part of it .. so instead of the rows 246-250 you can use this
I am not sure, why the time.sleep(2) there? Is there any meaningful reason?
serial_session = vm.wait_for_serial_login(timeout=60, recreate_serial_console=True)
| # Bring interface up | ||
| logging.info("Bringing up interface %s", nic_name) | ||
| serial_session.cmd("ip link set %s up" % nic_name, timeout=30) | ||
| time.sleep(2) |
There was a problem hiding this comment.
I don't see any reason for the time.sleep(2) here ... do you have any?
| test.fail(err_msg % o_ping) | ||
| # Fallback: If get_interface_from_pci_id returns None, use uevent file | ||
| if nic_name == "None" or not nic_name: | ||
| logging.warning("get_interface_from_pci_id returned None for %s, trying uevent method", val) |
There was a problem hiding this comment.
I would suggest to use an f-string? Python's f-strings (f"{val}") are preferred over % formatting because they evaluate expressions directly inside the string, making complex logs and test failure messages much easier to scan.
At least in the new code we should consider the modern ways.
logging.warning("get_interface_from_pci_id returned None for {val}, trying uevent method")
| if iface in ["lo", "sit0"]: # Skip loopback | ||
| continue | ||
| # Read PCI address from uevent file | ||
| pci_cmd = "cat /sys/class/net/{}/device/uevent 2>/dev/null | grep PCI_SLOT_NAME | cut -d= -f2".format(iface) |
There was a problem hiding this comment.
suggested f-string
pci_cmd = f"cat /sys/class/net/{iface}/device/uevent 2>/dev/null | grep PCI_SLOT_NAME | cut -d= -f2"
|
One more thing. Please check the checks, that were not successfull. |
3 participants
Three related fixes to make NIC passthrough testing work on pSeries (POWER10, ppc64le) with VFIO-passthrough BCM5719 NICs.
get_interface_from_pci_id() uses ethtool -i bus-info to match the PCI address. On pSeries, the host PCI domain (0018:xx) differs from the guest-visible PCI domain (0001:xx) because the hypervisor re-numbers PCI domains inside the guest. The match never succeeds and the function returns None.
Fix: when get_interface_from_pci_id() returns None, fall back to reading /sys/class/net//device/uevent PCI_SLOT_NAME which contains the guest-visible PCI address and always matches correctly.
virsh console allows only one active connection at a time on pSeries. The avocado-vt framework opens a serial console at VM boot and holds it. Any subsequent wait_for_serial_login() call blocks until timeout because the port is already occupied.
Fix: call cleanup_serial_console() + create_serial_console() before wait_for_serial_login() to release the occupied port and open a fresh connection. All NIC operations (ip config, ping) run on serial_session.
The passthrough NIC (enP1p0s1) connects to a physical switch with no L2 path to virbr0 (192.168.122.1). Using ping -I enP1p0s1 forces packets out the physical port where ARP never resolves.
Fix: pass source IP as interface parameter instead of NIC name. The kernel routes via the virtio NIC (enp0s1) -> virbr0 -> gateway.
Signed-off-by: Anushree-Mathur anushree.mathur@linux.ibm.com
Summary by CodeRabbit