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 |
1 participant
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