add case for attaching interface with target option for nat and direc… #6620
Conversation
WalkthroughAdds variant-driven target-name validation to attach/detach interface tests: new config blocks define expected target names; test code adds helper checks (VM XML, domiflist, guest OS) and integrates them into attach/detach flow when enabled. No other functional changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Lib as Test Logic
participant QEMU as VM / Guest
participant Virsh as virsh / domiflist
Runner->>Lib: start attach test (test_target_option enabled)
Lib->>QEMU: attach-interface (configure)
Lib->>Lib: check_target_option_behavior(vm, mac, expected)
Note right of Lib: reads VM XML, compares target
Lib->>Virsh: check_domiflist_target(vm, mac, expected)
Note right of Virsh: domiflist lookup
Lib->>QEMU: verify guest interface exists (optional)
Runner->>Lib: start detach
Lib->>QEMU: detach-interface
Lib->>Lib: check_guest_interface_exists(vm, mac, expected_existed=False)
Lib->>Virsh: check_domiflist_target(vm, mac, expected, existed=False)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
nanli1
force-pushed
the
add_case_for_attach_interface_with_target_option_for_nat_and_direct
branch
2 times, most recently
from
b412b7f to
445d443
Compare
![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
🧹 Nitpick comments (2)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py (2)
251-273: Consider usingnext(iter(...))for cleaner iteration.At line 266, using
next(iter(iflist_info.keys()))is more idiomatic than creating a list and indexing.Apply this diff:
- target_names = list(iflist_info.keys())[0] + target_names = next(iter(iflist_info.keys()))
293-293: Consider narrowing exception handling.Catching bare
Exceptionis too broad. Consider catching specific exceptions like login or session errors.- except Exception as e: - test.fail("Failed to check guest interface: %s" % str(e)) + except (remote.LoginError, aexpect.ShellError) as e: + test.fail("Failed to check guest interface: %s" % str(e))Note: You may need to add the relevant imports at the top of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virsh_cmd/domain/virsh_attach_detach_interface.cfg(2 hunks)libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
🪛 Ruff (0.14.0)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py
266-266: Prefer next(iter(iflist_info.keys())) over single element slice
Replace with next(iter(iflist_info.keys()))
(RUF015)
286-286: Unpacked variable status is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
293-293: Do not catch blind exception: Exception
(BLE001)
⏰ 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.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
🔇 Additional comments (6)
libvirt/tests/cfg/virsh_cmd/domain/virsh_attach_detach_interface.cfg (2)
57-83: LGTM! Well-structured test matrix for network interface type.The test variant structure clearly defines the expected behavior for different target name patterns. The configuration properly enables target option testing and sets appropriate check flags.
132-160: LGTM! Appropriate override for direct interface type.The explicit reset of
at_detach_iface_targetat line 141 correctly overrides the parent direct section's default value, and the comment clearly documents this intent. The expected target names (macvtap0 vs vnet1) properly reflect the behavioral differences between direct and network interface types.libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py (4)
16-16: LGTM! Import is used in the new helper functions.The
libvirt_miscimport is correctly added and utilized incheck_domiflist_target()at line 265.
354-360: LGTM! Parameter handling for target option tests.The new parameters properly enable target option testing. Setting
iface_targetto None whennone_targetis true correctly reflects the configuration intent, and downstream code at line 559 updates it appropriately before use.
553-559: LGTM! Target option validation properly integrated after attach.The validation flow correctly checks target behavior in VM XML and domiflist, then updates
iface_targetto the expected value for subsequent checks.
610-613: LGTM! Post-detach validation properly resets test state.The code correctly verifies interface removal from guest OS and domiflist, then resets
fail_flagto allow the test to pass when detach succeeds.
| def check_target_option_behavior(test, vm_name, iface_mac, expected_target_name): | ||
| """ | ||
| Check target option behavior according to test matrix | ||
| :param test: Test object for test.fail() calls | ||
| :param vm_name: Name of the virtual machine | ||
| :param iface_mac: MAC address of the interface to check | ||
| :param expected_target_name: Expected exact target name for "keep" behavior | ||
| """ | ||
|
|
||
| # Get actual target from XML using fetch_attrs | ||
| vmxml = vm_xml.VMXML.new_from_dumpxml(vm_name) | ||
| interfaces = vmxml.devices.by_device_tag('interface') | ||
| actual_target = None | ||
| for iface in interfaces: | ||
| if iface.mac_address == iface_mac: | ||
| actual_target = iface.fetch_attrs().get('target', {}).get('dev') | ||
| break | ||
| if not re.findall(expected_target_name, actual_target): | ||
| test.fail("Expected target '%s', got '%s'" % (expected_target_name, actual_target)) | ||
| test.log.debug("PASS: Expected target '%s' in vm xml for MAC %s", expected_target_name, iface_mac) |
There was a problem hiding this comment.
Handle None value for actual_target before pattern matching.
If the interface has no target attribute, actual_target will be None at line 244, causing re.findall() at line 246 to raise a TypeError.
Apply this diff to add None check and simplify the pattern matching:
for iface in interfaces:
if iface.mac_address == iface_mac:
actual_target = iface.fetch_attrs().get('target', {}).get('dev')
break
+
+ if actual_target is None:
+ test.fail("No target attribute found for interface with MAC %s" % iface_mac)
+
- if not re.findall(expected_target_name, actual_target):
+ if expected_target_name not in actual_target:
test.fail("Expected target '%s', got '%s'" % (expected_target_name, actual_target))🤖 Prompt for AI Agents
In libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py around
lines 228-248, actual_target can be None when the interface has no target
attribute which will make re.findall() raise a TypeError; add a guard that fails
the test with a clear message if actual_target is None, then replace
re.findall(expected_target_name, actual_target) with a simpler check such as
re.search(expected_target_name, actual_target) (or direct equality if exact
match is required) to do the pattern match safely, and keep the existing debug
log after the successful check.
| def check_guest_interface_exists(test, vm, iface_mac): | ||
| """ | ||
| Check if interface exists in guest OS | ||
| :param test: Test object for test.fail() calls | ||
| :param vm: Virtual machine object. | ||
| :param iface_mac: MAC address of the interface to verify in guest. | ||
| """ | ||
| try: | ||
| session = vm.wait_for_login() | ||
| status, output = session.cmd_status_output("ip -4 -o link list") | ||
| session.close() | ||
|
|
||
| if re.search(iface_mac.lower(), output.lower()): | ||
| test.fail("Interface with MAC %s was found in guest: %s" % (iface_mac, output)) | ||
| logging.info("PASS: Interface with MAC %s found in guest", iface_mac) | ||
|
|
||
| except Exception as e: | ||
| test.fail("Failed to check guest interface: %s" % str(e)) |
There was a problem hiding this comment.
Fix misleading function name and incorrect log message.
The function is called after detach (line 611) to verify the interface was removed, but:
- The function name
check_guest_interface_existsimplies checking that the interface EXISTS, not that it's absent - Line 291 logs "PASS: Interface with MAC %s found in guest" but this line only executes when the interface is NOT found
Apply this diff to fix the function name and log message:
-def check_guest_interface_exists(test, vm, iface_mac):
+def check_guest_interface_removed(test, vm, iface_mac):
"""
- Check if interface exists in guest OS
+ Check if interface has been removed from guest OS
:param test: Test object for test.fail() calls
:param vm: Virtual machine object.
- :param iface_mac: MAC address of the interface to verify in guest.
+ :param iface_mac: MAC address of the interface to verify is absent in guest.
"""
try:
session = vm.wait_for_login()
- status, output = session.cmd_status_output("ip -4 -o link list")
+ _, output = session.cmd_status_output("ip -4 -o link list")
session.close()
if re.search(iface_mac.lower(), output.lower()):
test.fail("Interface with MAC %s was found in guest: %s" % (iface_mac, output))
- logging.info("PASS: Interface with MAC %s found in guest", iface_mac)
+ logging.info("PASS: Interface with MAC %s removed from guest", iface_mac)
except Exception as e:
test.fail("Failed to check guest interface: %s" % str(e))Also update the call site at line 611:
- check_guest_interface_exists(test, vm, iface_mac)
+ check_guest_interface_removed(test, vm, iface_mac)🧰 Tools
🪛 Ruff (0.14.0)
286-286: Unpacked variable status is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
293-293: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py around
lines 276 to 294, the function name and success log are misleading for a
post-detach check: rename the function from check_guest_interface_exists to
check_guest_interface_removed and update its success message to indicate the
interface was not found (e.g., "PASS: Interface with MAC %s not found in
guest"), while keeping the failure path when the interface is still present;
also update the call site at line 611 to use check_guest_interface_removed(test,
vm, iface_mac).
There was a problem hiding this comment.
There is indeed a logical error in L289-L291. Please modify them according to the actual desired results.
There was a problem hiding this comment.
@meinaLi, you're right! The log message on line 291 is incorrect. Here's the corrected version of the function with the logical error fixed:
def check_guest_interface_removed(test, vm, iface_mac):
"""
Check if interface has been removed from guest OS
:param test: Test object for test.fail() calls
:param vm: Virtual machine object.
:param iface_mac: MAC address of the interface to verify is absent in guest.
"""
try:
session = vm.wait_for_login()
_, output = session.cmd_status_output("ip -4 -o link list")
session.close()
if re.search(iface_mac.lower(), output.lower()):
test.fail("Interface with MAC %s was found in guest: %s" % (iface_mac, output))
logging.info("PASS: Interface with MAC %s not found in guest (removed successfully)", iface_mac)
except Exception as e:
test.fail("Failed to check guest interface: %s" % str(e))Key changes:
- Function name:
check_guest_interface_exists→check_guest_interface_removed(reflects actual purpose) - Line 291 log message: "found in guest" → "not found in guest (removed successfully)" (corrects the logic)
- Docstring: Updated to clarify it checks for interface removal, not existence
Also update the call site at line 611:
check_guest_interface_removed(test, vm, iface_mac)There was a problem hiding this comment.
hi meina, Thanks your patience, That's so great.
The logic is correct , because this function is checking after vm detaching the device and device should not exist.
But you are still right, since the function name is so confusing , So I updated it to be more clear.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@meinaLi hi dear, do you mind reviewing this one : D |
| def check_guest_interface_exists(test, vm, iface_mac): | ||
| """ | ||
| Check if interface exists in guest OS | ||
| :param test: Test object for test.fail() calls | ||
| :param vm: Virtual machine object. | ||
| :param iface_mac: MAC address of the interface to verify in guest. | ||
| """ | ||
| try: | ||
| session = vm.wait_for_login() | ||
| status, output = session.cmd_status_output("ip -4 -o link list") | ||
| session.close() | ||
|
|
||
| if re.search(iface_mac.lower(), output.lower()): | ||
| test.fail("Interface with MAC %s was found in guest: %s" % (iface_mac, output)) | ||
| logging.info("PASS: Interface with MAC %s found in guest", iface_mac) | ||
|
|
||
| except Exception as e: | ||
| test.fail("Failed to check guest interface: %s" % str(e)) |
There was a problem hiding this comment.
There is indeed a logical error in L289-L291. Please modify them according to the actual desired results.
| # Test attach-interface with --target option for network interface type | ||
| # Test matrix based on target name behavior for network type | ||
| only vm_running..domname | ||
| check_xml_result = "no" |
There was a problem hiding this comment.
This variant is not used in python file.
There was a problem hiding this comment.
hi meina, Thanks your patience , this is a history variable and it's used in
L350 validate_xml_result = "yes" == params.get("check_xml_result", "no")
| # Test attach-interface with --target option for direct interface type | ||
| # Test matrix based on target name behavior for direct type | ||
| only vm_running..domname | ||
| check_xml_result = "no" |
There was a problem hiding this comment.
Same with above comment, this variant is not used in python file.
There was a problem hiding this comment.
hi meina, Thanks your patience , this is a history variable and it's used in
L350 validate_xml_result = "yes" == params.get("check_xml_result", "no") s
There was a problem hiding this comment.
Yes, you're right. I forgot to check the existing parameters; I assumed they were newly added.
…ypes This commit introduces test cases for attaching network interfaces with the --target option specifically for NAT and direct network types. Reference: xxxx-296606 Signed-off-by: nanli <[email protected]>
nanli1
force-pushed
the
add_case_for_attach_interface_with_target_option_for_nat_and_direct
branch
from
445d443 to
7fef28a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py (2)
276-298: Address static analysis warnings.Two minor cleanup items flagged by static analysis:
- Line 287: Unused
statusvariable should be prefixed with underscore- Line 297: Catching bare
Exceptionis too broadApply this diff:
try: session = vm.wait_for_login() - status, output = session.cmd_status_output("ip -4 -o link list") + _, output = session.cmd_status_output("ip -4 -o link list") session.close() if expected_existed: if not re.search(iface_mac.lower(), output.lower()): test.fail("Interface with MAC %s was not found in guest: %s" % (iface_mac, output)) else: if re.search(iface_mac.lower(), output.lower()): test.fail("Interface with MAC %s was found in guest: %s" % (iface_mac, output)) logging.info("PASS checking for the Interface with MAC %s in guest", iface_mac) - except Exception as e: + except (aexpect.ShellError, remote.LoginError) as e: test.fail("Failed to check guest interface: %s" % str(e))Note: You'll need to import the specific exception types if not already imported. Based on past review comments and static analysis hints.
228-248: Critical: Handle None value for actual_target before pattern matching.If the interface has no target attribute,
actual_targetwill be None at line 244, causingre.findall()at line 246 to raise a TypeError.Apply this diff to add None check:
for iface in interfaces: if iface.mac_address == iface_mac: actual_target = iface.fetch_attrs().get('target', {}).get('dev') break + + if actual_target is None: + test.fail("No target attribute found for interface with MAC %s" % iface_mac) + - if not re.findall(expected_target_name, actual_target): + if not re.search(expected_target_name, actual_target): test.fail("Expected target '%s', got '%s'" % (expected_target_name, actual_target))Based on past review comments.
🧹 Nitpick comments (1)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py (1)
266-266: Prefernext(iter())over single element slice.For better clarity and performance, use
next(iter(iflist_info.keys()))instead of creating an intermediate list.Apply this diff:
- target_names = list(iflist_info.keys())[0] + target_names = next(iter(iflist_info.keys()))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virsh_cmd/domain/virsh_attach_detach_interface.cfg(2 hunks)libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
🪛 Ruff (0.14.2)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py
266-266: Prefer next(iter(iflist_info.keys())) over single element slice
Replace with next(iter(iflist_info.keys()))
(RUF015)
287-287: Unpacked variable status is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
297-297: Do not catch blind exception: Exception
(BLE001)
⏰ 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.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
🔇 Additional comments (7)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py (5)
16-16: LGTM - Import is appropriate.The
libvirt_miscimport is needed forconvert_to_dictused in the newcheck_domiflist_targetfunction.
358-364: LGTM - Parameter handling is correct.The target option test parameters are properly initialized, and the
none_targethandling correctly setsiface_targetto None when needed.
557-563: LGTM - Post-attach validation is well-structured.The target option checks are properly integrated after attach. The
iface_targetupdate at line 563 ensures subsequent checks use the actual created target name.
614-617: Clarify the rationale for resetting fail_flag.At line 617,
fail_flagis unconditionally set to 0 after successful detach verification. This seems to discard any failures that occurred during attach (if they didn't cause immediate test exit).Could you clarify the intended behavior here? If there were attach-phase failures tracked in
fail_flag(that didn't trigger immediate exit at line 494), should they still be reported, or is the intent that successful detach validation overrides them for target_option_test?Consider either:
- Removing line 617 if attach failures should always be reported, or
- Adding a comment explaining why detach success resets attach failures for this test type
566-575: Verify iface_target is set before direct type check.For direct interface types, line 567 uses
iface_targetin the command. While the target option test correctly updatesiface_targetat line 563, ensure non-target-option-test scenarios also haveiface_targetdefined (which they do via line 110 in the config settingat_detach_iface_target = "test").The logic is sound, but worth noting the dependency on config defaults.
libvirt/tests/cfg/virsh_cmd/domain/virsh_attach_detach_interface.cfg (2)
57-83: LGTM - Network target option test matrix is well-defined.The test variants comprehensively cover different target name patterns for network interfaces:
- Explicit target names (vnet90, macvtap90, vif90, macvlan90, testname)
- Unset target (none_target = "yes")
The
expected_target_namevalues correctly reflect libvirt's target name behavior for network interfaces, where only certain prefixes (vif, testname) are preserved, while others default to vnet1.
132-160: LGTM - Direct interface target option test matrix is comprehensive.The test variants mirror the network type structure with appropriate direct-interface semantics:
- Line 141 correctly overrides the parent's
at_detach_iface_target = "test"with empty string as default- Per-variant target values are properly defined
expected_target_namevalues reflect direct interface behavior (defaulting to macvtap0, preserving vif/testname)The test matrix aligns with the PR objectives of validating target option behavior for direct interfaces.
| def check_domiflist_target(test, vm_name, iface_mac, expected_target_name, existed=True): | ||
| """ | ||
| Check target device name from virsh domiflist. | ||
| :param test: Test object for test.fail() calls | ||
| :param vm_name: Name of the virtual machine | ||
| :param iface_mac: MAC address of the interface to find | ||
| :param expected_target_name: Expected target device name pattern | ||
| :param existed: Whether the target should exist (True) or not exist (False) | ||
| :raises: test.fail() if domiflist command fails or MAC not found | ||
| """ | ||
| result = virsh.domiflist(vm_name, ignore_status=True, debug=True) | ||
| for line in result.stdout.strip().split('\n'): | ||
| if iface_mac.lower() in line.lower(): | ||
| iflist_info = libvirt_misc.convert_to_dict(line, pattern=r'(\S+) +(.*)') | ||
| target_names = list(iflist_info.keys())[0] | ||
| if existed: | ||
| if not re.findall(expected_target_name, target_names): | ||
| test.fail("Expected '%s' is existed, but got '%s' in domiflist" % (expected_target_name, target_names)) | ||
| else: | ||
| if re.findall(expected_target_name, target_names): | ||
| test.fail("Expected '%s' is not existed, but got '%s' in domiflist" % (expected_target_name, target_names)) | ||
| test.log.debug("Checking PASS: Interface target '%s' with domiflist for MAC %s", expected_target_name, iface_mac) |
There was a problem hiding this comment.
Major: Add validation when MAC address is not found with existed=True.
When existed=True (expecting interface to be present), if the MAC address is not found in domiflist output, the function silently succeeds at line 273. This should fail since we're expecting the interface to exist.
Apply this diff to add MAC not found validation:
+ mac_found = False
for line in result.stdout.strip().split('\n'):
if iface_mac.lower() in line.lower():
+ mac_found = True
iflist_info = libvirt_misc.convert_to_dict(line, pattern=r'(\S+) +(.*)')
target_names = list(iflist_info.keys())[0]
if existed:
if not re.findall(expected_target_name, target_names):
test.fail("Expected '%s' is existed, but got '%s' in domiflist" % (expected_target_name, target_names))
else:
if re.findall(expected_target_name, target_names):
test.fail("Expected '%s' is not existed, but got '%s' in domiflist" % (expected_target_name, target_names))
+
+ if existed and not mac_found:
+ test.fail("Interface with MAC %s not found in domiflist output" % iface_mac)
test.log.debug("Checking PASS: Interface target '%s' with domiflist for MAC %s", expected_target_name, iface_mac)🧰 Tools
🪛 Ruff (0.14.2)
266-266: Prefer next(iter(iflist_info.keys())) over single element slice
Replace with next(iter(iflist_info.keys()))
(RUF015)
🤖 Prompt for AI Agents
In libvirt/tests/src/virsh_cmd/domain/virsh_attach_detach_interface.py around
lines 251 to 273, the function currently does not fail if the expected MAC is
missing when existed=True; add a boolean flag (e.g., found=False) before
iterating, set found=True when a matching MAC line is processed, and after the
loop check: if existed and not found then call test.fail with a clear message
that the MAC/interface was not found in domiflist; keep existing target-name
checks unchanged so they still run when the MAC is found.
| # Test attach-interface with --target option for direct interface type | ||
| # Test matrix based on target name behavior for direct type | ||
| only vm_running..domname | ||
| check_xml_result = "no" |
There was a problem hiding this comment.
Yes, you're right. I forgot to check the existing parameters; I assumed they were newly added.
2 participants
…t type
xxxx-296606:Attach interface with --target for nat and direct interface
Signed-off-by: nanli [email protected]
Other related cases
Summary by CodeRabbit