Network: Add case for attach interface with various options for vm #6621
Conversation
xxx-296603: Attach-interface with --live --current --persistent --config options Signed-off-by: Yiqian Wei <[email protected]>
WalkthroughThis pull request introduces a comprehensive test suite for libvirt's virsh attach-interface command with various option combinations and VM states. It includes a test configuration file defining test parameters and expected outcomes, plus a Python implementation with verification logic. Changes
Sequence DiagramsequenceDiagram
participant Test
participant VM
participant Libvirt
participant Guest
Test->>VM: Backup original XML
Test->>VM: Set VM state (running/shutoff)
Test->>Test: Construct attach-interface options
Test->>Libvirt: Execute virsh attach-interface
alt Success
Libvirt-->>Test: Interface attached
else Expected Failure
Libvirt-->>Test: Error (mutual exclusivity/unsupported)
Test->>Test: Validate error message
end
alt VM running
rect rgba(100, 150, 200, 0.3)
Note over Test,Guest: Active XML Verification
Test->>Libvirt: Query active domain XML
Test->>Test: Verify interface in active XML
end
rect rgba(100, 200, 150, 0.3)
Note over Test,Guest: VM-side Interface Verification
Test->>Guest: Login to VM
Test->>Guest: Execute "ip l" to find interface
Guest-->>Test: Interface details
Test->>Test: Validate MAC/interface presence
end
else VM shutoff
rect rgba(100, 150, 200, 0.3)
Note over Test,Libvirt: Inactive XML Verification
Test->>Libvirt: Query inactive domain XML
Test->>Test: Verify interface in inactive XML
end
end
Test->>Test: Aggregate verification results
Test->>VM: Restore original XML
Test->>VM: Restore original VM state
Test->>Test: Report pass/fail
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The implementation introduces a multi-layer test harness with several functions handling option composition, XML verification against both active and inactive domain states, VM-side network interface validation, error handling with configurable expectations, and comprehensive state management. The control flow spans setup, execution, conditional verification paths, guest login procedures, and cleanup—requiring careful review of logic density and interaction correctness. Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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
🧹 Nitpick comments (8)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.cfg (2)
39-44: Add --current --config mutual-exclusion cases.Consider adding variants to assert --current and --config are mutually exclusive (both vm_running and vm_shutoff), mirroring current_live_option.
Proposed snippet (vm_running):
+ - current_config_option: + test_flags = "--current --config" + status_error = "yes" + expected_error = "are mutually exclusive"And similarly under vm_shutoff.
Also applies to: 75-81
17-17: Tiny nit: trailing whitespace.There's a trailing space after "no". Not harmful, but easy to clean.
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.py (6)
72-83: Make MAC compare case-insensitive and relax source attr check.Avoid false negatives from MAC letter casing and support different source attrs.
- if (iface.type_name == iface_type and - iface.mac_address == iface_mac): + mac_equal = ((iface.mac_address or '').lower() == + (iface_mac or '').lower()) + if (iface.type_name == iface_type and mac_equal): if iface_source is not None: - if (iface.xmltreefile.find('source') is not None and - iface.source.get('network') == iface_source): + src_elem = iface.xmltreefile.find('source') + if (src_elem is not None and ( + src_elem.get('network') == iface_source or + src_elem.get('bridge') == iface_source or + src_elem.get('dev') == iface_source)): logging.info("Found interface %s in %s XML", iface_mac, xml_type) return True else: logging.info("Found interface %s in %s XML", iface_mac, xml_type) return True
87-89: Log stack on XML check failure.Use logger.exception to retain traceback; keep return False behavior.
- except Exception as e: - logging.error("Failed to check %s XML: %s", xml_type, e) - return False + except Exception: + logging.exception("Failed to check %s XML", xml_type) + return FalseNote: If feasible, narrow the exception types to avoid a blind catch. [Based on hints]
214-214: Silence unused arg warning fortest.Framework requires the signature; explicitly discard to appease linters.
def run(test, params, env): @@ - vm_name = params.get("main_vm") + # Avocado plugs `test` but it's unused here + del test + vm_name = params.get("main_vm")
328-329: Log cleanup exceptions with traceback.Switch to logger.exception.
- except Exception as e: - logging.error("Cleanup failed: %s", e) + except Exception: + logging.exception("Cleanup failed")Also consider narrowing the except if you can.
5-15: Prune unused imports.process, utils_misc (if you don’t adopt wait_for), and utils_test.libvirt look unused.
-from avocado.utils import process @@ -from virttest import utils_misc @@ -from virttest.utils_test import libvirtIf you switch to utils_misc.wait_for in the VM check, keep utils_misc.
290-301: Error substring can be fragile across locales/versions.You already use substring matching; consider allowing multiple acceptable fragments (e.g., "are mutually exclusive", "--current and --live") to reduce brittleness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.cfg(1 hunks)libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
🪛 Ruff (0.14.0)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.py
86-86: Consider moving this statement to an else block
(TRY300)
87-87: Do not catch blind exception: Exception
(BLE001)
88-88: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
133-133: Consider moving this statement to an else block
(TRY300)
135-135: Do not catch blind exception: Exception
(BLE001)
214-214: Unused function argument: test
(ARG001)
257-258: Avoid specifying long messages outside the exception class
(TRY003)
296-297: Avoid specifying long messages outside the exception class
(TRY003)
328-328: Do not catch blind exception: Exception
(BLE001)
329-329: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (3)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.cfg (3)
12-55: Good coverage of vm_running flag combinations.Matrix looks correct for live/config/current/persistent, and mutual exclusion for --current --live is captured.
51-54: Mutual exclusivity covered well.--current and --live should fail; substring match on "are mutually exclusive" is reasonable.
Please confirm the exact stderr substring on your CI/libvirt version to avoid brittle assertions.
55-66: Review comment is valid; test configuration requires verification across supported libvirt versions.The web search confirms your concern: when omitting flags, virsh uses the "legacy API" whose behavior depends on the hypervisor/driver; on some platforms it will update the persistent XML for an offline domain, on others it may error.
This means the test's expectation of an error ("domain is not running") may not hold consistently across different libvirt versions or drivers. You should manually verify the
no_optioncase against all supported libvirt versions in your test matrix to prevent spurious failures where the command succeeds by updating inactive XML instead of erroring.
| try: | ||
| session = vm.wait_for_login() | ||
| # Wait for interface to be available | ||
| time.sleep(5) | ||
|
|
||
| logging.info("checking VM interface using 'ip l' command") | ||
| # Use 'ip l' command as specifically requested | ||
| status, output = session.cmd_status_output("ip l") | ||
| session.close() | ||
|
|
||
| if status != 0: | ||
| return (1, "Failed to execute 'ip l' command in VM", None) |
There was a problem hiding this comment.
Ensure session is closed on all paths; reduce flakiness.
Close the VM session in a finally block; optional: poll instead of fixed sleep.
- try:
- session = vm.wait_for_login()
- # Wait for interface to be available
- time.sleep(5)
+ session = None
+ try:
+ session = vm.wait_for_login()
+ # Wait briefly for interface; consider replacing with utils_misc.wait_for
+ time.sleep(5)
logging.info("checking VM interface using 'ip l' command")
# Use 'ip l' command as specifically requested
status, output = session.cmd_status_output("ip l")
- session.close()
+ # session closed in finally
@@
- except Exception as detail:
- return (1, "Failed to login to VM or execute 'ip l': %s" % detail, None)
+ except Exception as detail:
+ return (1, "Failed to login to VM or execute 'ip l': %s" % detail, None)
+ finally:
+ if session:
+ session.close()Optional (less flaky): replace time.sleep with utils_misc.wait_for to poll for the MAC for up to, e.g., 30s. I can provide that patch if you want.
Also applies to: 108-111, 135-136
🤖 Prompt for AI Agents
In
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.py
around lines 100 to 111 (and also apply same change to the logic at lines
108-111 and 135-136), the VM session is only closed on the happy path causing
resource leaks and flaky tests; wrap the session usage in a try/finally that
always calls session.close(), and replace the fixed time.sleep(5) with a polling
wait (e.g., utils_misc.wait_for or equivalent) that checks for the interface/MAC
up to ~30s to reduce flakiness so the session is always closed and the test
waits deterministically for the interface to appear.
| attach_result = virsh.attach_interface( | ||
| vm_name, attach_options, **virsh_dargs) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Confirm virsh wrapper signature for options.
Ensure virttest.virsh.attach_interface accepts the options string as the second positional argument (some wrappers use keyword options/extra).
🏁 Script executed:
#!/bin/bash
# Verify attach_interface signature and typical call sites
rg -nP -C2 'def\s+attach_interface\s*\(' virttest/virsh.py virttest | sed -n '1,200p'
rg -nP -C2 'virsh\.attach_interface\s*\(' -g '!**/venv/**'Length of output: 26187
🏁 Script executed:
#!/bin/bash
# Find virsh.py location in the repository
fd -type f -name "virsh.py"
# Search for attach_interface function definition
rg -nP 'def\s+attach_interface\s*\(' -A 5 -g '**/*.py'Length of output: 170
🏁 Script executed:
#!/bin/bash
# Also check the imports in the test file to understand where virsh comes from
rg -nP '^import|^from' libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.py | head -20Length of output: 381
🏁 Script executed:
#!/bin/bash
# Find virsh.py without --type filter
fd -name "virsh.py"
# Also search directly for the function definition
rg 'def attach_interface' -A 8Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check if virttest is a local package
find . -type d -name "virttest" 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search PyPI for virttest package info
pip search virttest 2>/dev/null || pip index versions virttest 2>/dev/null || curl -s "https://pypi.org/pypi/virttest/json" | head -100Length of output: 24
🌐 Web query:
virttest virsh attach_interface documentation signature
💡 Result:
The virttest VM method signature is:
virttest.libvirt_vm.VM.attach_interface(option='', ignore_status=False, debug=False)
(Attaches a NIC to the VM; see documentation.) [1]
Source: virttest API docs. [1]
🌐 Web query:
virttest virsh attach_interface positional argument second parameter
💡 Result:
In virttest.libvirt_vm.VM.attach_interface(option='', ignore_status=False, debug=False) the second positional parameter is ignore_status (a boolean). [1]
Source: virttest API docs. [1]
Use keyword argument option= to match virsh wrapper signature and codebase convention.
The codebase predominantly uses virsh.attach_interface(vm_name, option=..., **kwargs) with option as a keyword argument. The code at lines 290-291 passes attach_options as a positional argument, which is inconsistent. Change to:
attach_result = virsh.attach_interface(
vm_name, option=attach_options, **virsh_dargs)🤖 Prompt for AI Agents
In
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_vm_status.py
around lines 290 to 292, the call to virsh.attach_interface passes
attach_options as a positional argument; change it to use the keyword argument
`option=` to match the virsh wrapper signature and codebase convention (e.g.,
call virsh.attach_interface(vm_name, option=attach_options, **virsh_dargs)),
ensuring other kwargs remain unchanged.
|
Test results: |
| uri = libvirt_vm.normalize_connect_uri( | ||
| params.get("connect_uri", "default")) |
There was a problem hiding this comment.
Could I ask what this var uri used for the case checking
uri = libvirt_vm.normalize_connect_uri(
params.get("connect_uri", "default"))
If not necessary, we could remove it
| if (("--live" in test_flags or "--current" in test_flags) and | ||
| not libvirt_version.version_compare(1, 0, 5)): | ||
| raise exceptions.TestSkipError( | ||
| "Libvirt version doesn't support --live/--current") |
There was a problem hiding this comment.
Version checking is not mentioned in polarion case, we do not need to add case if not mentioned, especially this is a very low libvirt version
| if status_error: | ||
| if attach_result.exit_status == 0: | ||
| raise exceptions.TestFail( | ||
| "Expected attach command to fail but it succeeded") | ||
| else: | ||
| logging.info("Attach command failed as expected: %s", | ||
| attach_result.stderr) | ||
| if expected_error and expected_error not in attach_result.stderr: | ||
| raise exceptions.TestFail("Expected error '%s' not found in: %s" % | ||
| (expected_error, attach_result.stderr)) | ||
| return | ||
| else: | ||
| if attach_result.exit_status != 0: | ||
| raise exceptions.TestFail("Attach command failed unexpectedly: %s" % | ||
| attach_result.stderr) | ||
|
|
There was a problem hiding this comment.
Could you please check if these lines can be replaced by checking method below
result = virsh.define(vmxml.xml, debug=True)
libvirt.check_exit_status(result, status_error)
if err_msg:
libvirt.check_result(result, err_msg)
| # Wait for operation to take effect | ||
| time.sleep(3) |
There was a problem hiding this comment.
Could you please double confirm if we really need this sleep
| time.sleep(5) | ||
|
|
||
| logging.info("checking VM interface using 'ip l' command") | ||
| # Use 'ip l' command as specifically requested | ||
| status, output = session.cmd_status_output("ip l") |
There was a problem hiding this comment.
Could you check if we can reuse utils_misc.wait_for() to check here, we should always try to not use hard code sleeping
| :param iface_type: interface type | ||
| :param iface_source: interface source | ||
| :param iface_mac: interface MAC address |
There was a problem hiding this comment.
Same as the other comment , I didn't see source ,model and type in polarion case ,Could you double check?
| # Do not check active XML when VM is shutoff | ||
| if vm_state == "shutoff" and expected_active_xml: | ||
| logging.info( | ||
| "VM is shutoff, skipping active XML check even though expected_active_xml=yes") | ||
| logging.info( | ||
| "For shutoff VM with --current flag, interface should be in inactive XML instead") |
There was a problem hiding this comment.
If no checking here, We could remove it
| - no_option: | ||
| test_flags = "" | ||
| status_error = "no" | ||
| expected_active_xml = "yes" | ||
| expected_inactive_xml = "no" | ||
| expected_vm_interface = "yes" | ||
| - live_option: | ||
| test_flags = "--live" | ||
| status_error = "no" | ||
| expected_active_xml = "yes" | ||
| expected_inactive_xml = "no" | ||
| expected_vm_interface = "yes" | ||
| - config_option: | ||
| test_flags = "--config" | ||
| status_error = "no" | ||
| expected_active_xml = "no" | ||
| expected_inactive_xml = "yes" | ||
| expected_vm_interface = "no" | ||
| - persistent_option: | ||
| test_flags = "--persistent" | ||
| status_error = "no" | ||
| expected_active_xml = "yes" | ||
| expected_inactive_xml = "yes" | ||
| expected_vm_interface = "yes" | ||
| - current_option: | ||
| test_flags = "--current" | ||
| status_error = "no" | ||
| expected_active_xml = "yes" | ||
| expected_inactive_xml = "no" | ||
| expected_vm_interface = "yes" | ||
| - config_live_option: | ||
| test_flags = "--config --live" | ||
| status_error = "no" | ||
| expected_active_xml = "yes" | ||
| expected_inactive_xml = "yes" | ||
| expected_vm_interface = "yes" | ||
| - current_live_option: | ||
| test_flags = "--current --live" | ||
| status_error = "yes" | ||
| expected_error = "are mutually exclusive" |
There was a problem hiding this comment.
Could you extract expected_active_xml = "yes" under
- vm_running:
vm_state = "running"
to remove duplication
Also status_error = "no" , Please also check if other vars could be extracted to remove duplication
| expected_inactive_xml = "yes" == params.get("expected_inactive_xml", "no") | ||
| inactive_exists = check_interface_xml(vm_name, iface_type, iface_source, | ||
| iface_mac, check_active=False) | ||
| if expected_inactive_xml and not inactive_exists: | ||
| errors.append( | ||
| "Interface not found in inactive XML expected with flags: %s" % flags) | ||
| elif not expected_inactive_xml and inactive_exists: | ||
| errors.append( | ||
| "Interface found in inactive XML but should not exist with flags: %s" % flags) | ||
| else: | ||
| result_msg = "found" if inactive_exists else "not found" | ||
| logging.info( | ||
| "Interface %s in inactive XML as expected with flags: %s" % (result_msg, flags)) |
There was a problem hiding this comment.
As we talked before, I remembered you misunderstanding the meaning of expected_inactive_xml. Me too , I also feel a little confused about its' meaning, Do you think expected_inactive_xml should be expected_existed_in_inactive_xml or some name else?
| status, msg, iface_name = check_interface_in_vm_with_ip_l( | ||
| vm, iface_mac) | ||
| if status != 0: | ||
| errors.append("VM interface check via 'ip l': %s" % msg) | ||
| else: | ||
| logging.info("Interface found in VM (name: %s)", iface_name) | ||
|
|
||
| if errors: | ||
| raise exceptions.TestFail( | ||
| "verification failed: %s" % "; ".join(errors)) | ||
|
|
||
| logging.info("All verifications passed successfully") | ||
|
|
There was a problem hiding this comment.
Just a thought, Do you think no return for check_interface_in_vm_with_ip_l and move these checking in check_interface_in_vm_with_ip_l is good?
|
Could you add test result for this pr |
2 participants
xxx-296603: Attach-interface with --live --current --persistent --config options
Summary by CodeRabbit