migration_xml_update: set mac to avoid connectivity issue #6684
Conversation
The update_interface_mtu test case has been failing because the IP cache was out of date. Although the VM can be logged into the test failed to continue because the cache tried to access the IP through an old mac address. However, when the test modifies the interface and it doesn't require a mac libvirt will assign some mac address which might be different from the previous one. This leads to the framework's cache to become invalid. Instead, now update the xml so that there's no mismatch between XML and the mac and the connection can be established. This seems to have worked when the test case was submitted recently but I'd attribute this to a coincidence. Recent test runs on s390x and x86_64 all fail due to this failure of connectivity from the test framework with error message """ virttest.virt_vm.VMAddressVerificationError: Could not verify DHCP lease: 52:54:00:8b:b2:59 --> ... """ Signed-off-by: Sebastian Mitterle <smitterl@redhat.com>
|
WalkthroughThe change modifies device creation logic in a migration test file. It restructures the timing of device directory creation by moving it before device removal, and introduces conditional MAC address extraction and injection for interface-type devices. When creating an interface device, the existing MAC address is fetched from the VM XML and injected into the device directory to ensure the newly created interface retains the original MAC address. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
libvirt/tests/src/migration/migration_xml_update.py (1)
29-29: Consider usingast.literal_evalinstead ofevalfor safer parsing.As flagged by static analysis,
eval()can execute arbitrary code, which poses a security risk. While this is test code and the line was simply moved (not newly introduced), consider refactoring to useast.literal_evalfor safer literal evaluation.Apply this diff:
+from ast import literal_eval + def prepare_vm(vm, vm_name, vmxml, params): """ Prepares the VM by adding the device under test :param vm: The VM object :param vm_name: The VM name :param vmxml: The VM XML :param params: The test parameters """ if vm.is_alive(): vm.destroy() dev_type = params.get("remove_all") - dev_dir = eval(params.get("add_device")) + dev_dir = literal_eval(params.get("add_device")) if dev_type == "interface": current_mac = vm_xml.VMXML.get_first_mac_by_name(vm_name) dev_dir['mac_address'] = current_mac
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/migration/migration_xml_update.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.780Z
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.780Z
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.780Z
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/migration/migration_xml_update.py
🪛 Ruff (0.14.5)
libvirt/tests/src/migration/migration_xml_update.py
29-29: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
| dev_dir = eval(params.get("add_device")) | ||
| if dev_type == "interface": | ||
| current_mac = vm_xml.VMXML.get_first_mac_by_name(vm_name) | ||
| dev_dir['mac_address'] = current_mac |
There was a problem hiding this comment.
🧩 Analysis chain
Good fix for MAC preservation, but verify None handling.
The change correctly addresses the connectivity issue by preserving the MAC address. Moving dev_dir creation before device removal and conditionally injecting the current MAC for interface devices ensures the framework's IP cache remains valid.
However, verify that get_first_mac_by_name cannot return None in scenarios where the VM has no interfaces or the call fails. If None is possible, downstream device creation might fail or behave unexpectedly.
Run the following script to check how get_first_mac_by_name handles edge cases:
🏁 Script executed:
#!/bin/bash
# Verify get_first_mac_by_name implementation and None handling
# Search for the implementation of get_first_mac_by_name
ast-grep --pattern $'def get_first_mac_by_name($$$) {
$$$
}'
# Search for existing None checks on get_first_mac_by_name results
rg -nP -C3 'get_first_mac_by_name.*\)' | rg -A5 -B5 'if.*None|is None'Length of output: 886
🏁 Script executed:
#!/bin/bash
# Find the implementation of get_first_mac_by_name
rg -n "def get_first_mac_by_name" --type py
# Check how dev_dir is used after line 32 in the reviewed file
rg -n -A 15 "def prepare_vm" libvirt/tests/src/migration/migration_xml_update.py
# Also search for other None checks on mac assignments to establish pattern
rg -n -B2 -A2 "mac.*is None|is None.*mac" --type py | head -30Length of output: 1382
🏁 Script executed:
#!/bin/bash
# Get info about the VMXML class and get_first_mac_by_name method
# Search for documentation or type hints
rg -n "get_first_mac_by_name" --type py -B2 -A5 | grep -A10 "def get_first_mac_by_name"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the implementation of get_first_mac_by_name and check its return type
rg -n "def get_first_mac_by_name" --type py -A 20
# Check the complete prepare_vm function to see how dev_dir is used after line 32
rg -n "def prepare_vm" libvirt/tests/src/migration/migration_xml_update.py -A 50
# Check if there are any other places where mac_address is checked for None
rg -n "mac_address.*None|None.*mac_address" --type py -B2 -A2Length of output: 2000
🏁 Script executed:
#!/bin/bash
# Search for create_vm_device_by_type to see if it validates mac_address parameter
rg -n "def create_vm_device_by_type" --type py -A 10
# Check if there are test cases that show what happens when mac_address is None
rg -n "mac_address.*=.*None\|mac_address.*=\s*None" --type py -B 2 -A 2Length of output: 0
Add None validation before assigning MAC address.
The code fails to validate that get_first_mac_by_name() returned a MAC address. Elsewhere in the codebase (e.g., libguestfs/tests/guestfs_operated_disk.py:50 and libvirt/tests/src/passthrough/pci/vfio.py:135), the method's return value is explicitly checked for None before use and the test fails if the MAC cannot be retrieved.
At line 31-32 of libvirt/tests/src/migration/migration_xml_update.py, add a None check:
current_mac = vm_xml.VMXML.get_first_mac_by_name(vm_name)
if current_mac is None:
test.fail("Failed to get MAC address for VM %s" % vm_name)
dev_dir['mac_address'] = current_macWithout this check, dev_dir['mac_address'] will be None, causing device creation at line 35 to fail or behave unexpectedly.
🧰 Tools
🪛 Ruff (0.14.5)
29-29: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
In libvirt/tests/src/migration/migration_xml_update.py around lines 29 to 32,
the code assigns current_mac directly from
vm_xml.VMXML.get_first_mac_by_name(vm_name) without validating it; add a None
check immediately after retrieving current_mac and call test.fail with a clear
message if it is None, otherwise proceed to set dev_dir['mac_address'] =
current_mac so you avoid assigning None and causing downstream device-creation
failures.
2 participants
The update_interface_mtu test case has been failing because the IP cache was out of date. Although the VM can be logged into the test failed to continue because the cache tried to access the IP through an old mac address.
However, when the test modifies the interface and it doesn't require a mac libvirt will assign some mac address which might be different from the previous one. This leads to the framework's cache to become invalid. Instead, now update the xml so that there's no mismatch between XML and the mac and the connection can be established.
This seems to have worked when the test case was submitted recently but I'd attribute this to a coincidence. Recent test runs on s390x and x86_64 all fail due to this failure of connectivity from the test framework with error message
"""
virttest.virt_vm.VMAddressVerificationError: Could not verify DHCP lease: 52:54:00:8b:b2:59 --> ...
"""
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.