Skip to content

Fix interface discovery, serial console and ping issues! #6913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Anushree-Mathur wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix interface discovery, serial console and ping issues! #6913

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 77 additions & 13 deletions libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,19 +207,83 @@ def check_device_status(net_ip, server_ip, netmask):
# ping to server from each function
for val in bus_info:
nic_name = str(utils_misc.get_interface_from_pci_id(val, session))
session.cmd("ip addr flush dev %s" % nic_name)
session.cmd("ip addr add %s/%s dev %s"
% (net_ip, netmask, nic_name))
session.cmd("ip link set %s up" % nic_name)
# Pinging using nic_name is having issue,
# hence replaced with IPAddress
s_ping, o_ping = utils_test.ping(server_ip, count=5,
interface=net_ip, timeout=30,
session=session)
logging.info(o_ping)
if s_ping != 0:
err_msg = "Ping test fails, error info: '%s'"
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)

@hholoubk hholoubk Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

try:
# Get all network interfaces
ifaces_output = session.cmd_output("ip -o link show | awk -F': ' '{print $2}'")
interfaces = [iface.strip() for iface in ifaces_output.strip().split("\n") if iface.strip()]

# Find interface matching PCI address using uevent file
for iface in interfaces:
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)

@hholoubk hholoubk Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggested f-string

pci_cmd = f"cat /sys/class/net/{iface}/device/uevent 2>/dev/null | grep PCI_SLOT_NAME | cut -d= -f2"

status, pci_addr = session.cmd_status_output(pci_cmd)
if status == 0 and pci_addr.strip():
# 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
Comment on lines +226 to +230

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
# 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.

logging.info("Found interface %s for PCI %s using uevent method", nic_name, val)
break
except Exception as e:
logging.error("Failed to find interface for PCI %s: %s", val, str(e))

if nic_name == "None" or not nic_name:
test.error("Could not determine interface name for PCI device {}".format(val))
continue

# Convert netmask to CIDR
cidr_mask = netmask_to_cidr(netmask)
Comment on lines +240 to +241

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
# 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


# 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()

@hholoubk hholoubk Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

vm.create_serial_console()
time.sleep(2)

serial_session = vm.wait_for_serial_login(timeout=60)
try:
# 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)

@hholoubk hholoubk Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any reason for the time.sleep(2) here ... do you have any?


# Configure IP
logging.info("Configuring IP %s/%s on %s", net_ip, cidr_mask, nic_name)
serial_session.cmd("ip addr flush dev %s" % nic_name, timeout=30)
serial_session.cmd("ip addr add %s/%s dev %s" % (net_ip, cidr_mask, nic_name), timeout=30)

# Verify IP configuration
ip_check = serial_session.cmd_output(
"ip -4 addr show dev %s" % nic_name, timeout=30)
if "inet %s" % net_ip not in ip_check:
test.fail("Failed to configure IP %s on %s" % (net_ip, nic_name))
logging.info("IP configured successfully on %s", nic_name)

# Ping via source IP (not -I nic_name).
# -I nic_name forces packets out the physical port which has no path
# to virbr0. Using source IP lets kernel route via enp0s1 -> virbr0.
logging.info("Pinging %s from source IP %s", server_ip, net_ip)
s_ping, o_ping = utils_test.ping(
server_ip,
count=5,
interface=str(net_ip),
timeout=30,
session=serial_session)
logging.info(o_ping)
if s_ping != 0:
test.fail("Ping to %s failed, error info: '%s'" % (server_ip, o_ping))
logging.info("Ping to %s successful", server_ip)

finally:
serial_session.close()

# Each interface should have unique IP
# For ppc64 arch let's test using one ip only
if arch != "ppc64le":
Expand Down
Loading