add case for ethernet attaching #6557
Conversation
WalkthroughAdds a new Libvirt test and config to hot-plug and remove an Ethernet interface (tap or macvtap). The test creates host device, generates and attaches interface XML, verifies guest XML via XPath and guest connectivity (ping), then detaches and restores VM XML, cleaning up host devices. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Runner as Test Runner
participant Host as Host Device Manager
participant Virsh as virsh / libvirt
participant VM as Guest VM
participant Validator as XPath / Ping Validator
Tester->>Runner: start test (params: tap or macvtap)
Runner->>Host: create host-side device (tap/macvtap)
Runner->>Virsh: ensure VM state (start if required)
Runner->>Virsh: attach-device (generated ethernet XML)
Virsh-->>Runner: attachment result
Runner->>Validator: verify guest XML via XPath (attach)
Validator-->>Runner: XPath result
Runner->>VM: guest pings outside_ip
VM-->>Runner: ping result
alt detach path
Runner->>Virsh: detach-device (generated ethernet XML)
Virsh-->>Runner: detach result
Runner->>Validator: verify guest XML removal via XPath
Validator-->>Runner: XPath result
end
Runner->>Host: remove host-side device
Runner->>Virsh: restore/sync VM XML (backup restore)
Runner-->>Tester: report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Comment |
nanli1
changed the title
![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 (6)
libvirt/tests/cfg/virtual_network/attach_detach_device/attach_ethernet_interface.cfg (2)
8-9: Fix minor syntax: "variants :" → "variants:"Avoid the stray space before the colon to match existing cfg style and prevent brittle parsers.
- variants : + variants:
7-7: Strengthen detach assertions (currently no-op).
expected_xpaths_detach = []combined with the code’s unconditional check provides no XML validation after detach. Ifcheck_guest_xml_by_xpathssupports negative assertions (e.g.,no_element_attrs), add an explicit “not present” XPath for the target dev, or skip the call when empty (see code comment I left).Would you like me to propose concrete detach XPaths supported by your helper?
Also applies to: 21-21
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py (4)
29-33: Use qemu/libvirt user for device ownership; “root” may break attach.Tap/macvtap must be accessible by the qemu process user (often
qemuorlibvirt-qemu). Chowning torootcan cause EPERM on attach. Make the owner configurable and default to a sensible user.@@ - if device_type == "tap": - network_base.create_tap(tap_name, default_br, "root") - elif device_type == "macvtap": - network_base.create_macvtap(macvtap_name, host_iface, "root") + if device_type == "tap": + network_base.create_tap(tap_name, default_br, dev_owner_user) + elif device_type == "macvtap": + network_base.create_macvtap(macvtap_name, host_iface, dev_owner_user) @@ - tap_name = params.get('tap_name', 'mytap0') - macvtap_name = params.get('macvtap_name', 'mymacvtap0') + tap_name = params.get('tap_name', 'mytap0') + macvtap_name = params.get('macvtap_name', 'mymacvtap0') + dev_owner_user = params.get('dev_owner_user', 'qemu')If your environment uses a different user (e.g.,
libvirt-qemu), setdev_owner_userin the cfg.Also applies to: 82-86
59-63: Avoid race on guest NIC removal; wait until interface disappears.Right after detach, the NIC may linger briefly. Poll before failing to reduce flakiness.
- vm_iface_info_after = utils_net.get_linux_iface_info(mac=mac_addr, session=session) - if vm_iface_info_after is not None: - test.fail(f'Interface should be removed from guest: {vm_iface_info_after}') + if not utils_misc.wait_for( + lambda: utils_net.get_linux_iface_info(mac=mac_addr, session=session) is None, + timeout=15): + vm_iface_info_after = utils_net.get_linux_iface_info(mac=mac_addr, session=session) + test.fail(f'Interface should be removed from guest: {vm_iface_info_after}')
54-58: Skip XML check when no detach XPaths provided.Guard the check to avoid a meaningless call (and potential future errors if helpers change behavior on empty input).
- libvirt_vmxml.check_guest_xml_by_xpaths( - vm_xml.VMXML.new_from_dumpxml(vm_name), expected_xpaths_detach) + if expected_xpaths_detach: + libvirt_vmxml.check_guest_xml_by_xpaths( + vm_xml.VMXML.new_from_dumpxml(vm_name), expected_xpaths_detach)
26-26: Fix log typo.“TEST_SETPUP” → “TEST_SETUP”.
- test.log.info("TEST_SETPUP: Setting up VM without interfaces") + test.log.info("TEST_SETUP: Setting up VM without interfaces")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/attach_detach_device/attach_ethernet_interface.cfg(1 hunks)libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py (1)
provider/virtual_network/network_base.py (4)
create_tap(148-161)create_macvtap(164-189)ping_check(94-145)delete_tap(202-208)
🪛 Ruff (0.12.2)
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py
86-86: Use of possibly insecure function; consider using ast.literal_eval
(S307)
87-87: Use of possibly insecure function; consider using ast.literal_eval
(S307)
88-88: 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.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
🔇 Additional comments (1)
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py (1)
36-56: Overall flow looks solid.Clear steps: attach → XML verify → ping → detach. With the above fixes, this should be robust.
| iface_attrs = eval(params.get('iface_attrs', "{}")) | ||
| expected_xpaths_attach = eval(params.get('expected_xpaths_attach')) | ||
| expected_xpaths_detach = eval(params.get('expected_xpaths_detach')) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace eval() with ast.literal_eval for untrusted params.
eval on cfg-driven strings is unsafe and flagged by linters. Use ast.literal_eval with sane defaults.
+import ast
@@
- iface_attrs = eval(params.get('iface_attrs', "{}"))
- expected_xpaths_attach = eval(params.get('expected_xpaths_attach'))
- expected_xpaths_detach = eval(params.get('expected_xpaths_detach'))
+ iface_attrs = ast.literal_eval(params.get('iface_attrs', "{}"))
+ expected_xpaths_attach = ast.literal_eval(params.get('expected_xpaths_attach', "[]"))
+ expected_xpaths_detach = ast.literal_eval(params.get('expected_xpaths_detach', "[]"))Also applies to: 2-2
🧰 Tools
🪛 Ruff (0.12.2)
86-86: Use of possibly insecure function; consider using ast.literal_eval
(S307)
87-87: Use of possibly insecure function; consider using ast.literal_eval
(S307)
88-88: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
In
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py
lines 86-88 (and also at line 2), replace unsafe eval() calls with
ast.literal_eval: import ast at the top, call ast.literal_eval on params.get
values using sensible defaults (e.g., params.get('iface_attrs', '{}') and
params.get('expected_xpaths_attach', '[]') /
params.get('expected_xpaths_detach', '[]')), and wrap in a small try/except to
catch ValueError/SyntaxError and raise or log a clearer error if parsing fails.
| if utils_misc.wait_for( | ||
| lambda: utils_net.get_default_gateway( | ||
| iface_name=True, force_dhcp=True, json=True) is None, timeout=15): | ||
| test.fail("Not find host interface in 15s") | ||
| host_iface = params.get("host_iface") | ||
| host_iface = host_iface if host_iface else utils_net.get_default_gateway( | ||
| iface_name=True, force_dhcp=True).split()[0] |
There was a problem hiding this comment.
Fix inverted wait logic for host iface discovery (can fail immediately).
You currently fail as soon as the default gateway is None, instead of waiting up to 15s for it to appear. Also, avoid .split()[0]; iface_name=True should already return an iface name.
- if utils_misc.wait_for(
- lambda: utils_net.get_default_gateway(
- iface_name=True, force_dhcp=True, json=True) is None, timeout=15):
- test.fail("Not find host interface in 15s")
- host_iface = params.get("host_iface")
- host_iface = host_iface if host_iface else utils_net.get_default_gateway(
- iface_name=True, force_dhcp=True).split()[0]
+ def _get_host_iface():
+ return utils_net.get_default_gateway(iface_name=True, force_dhcp=True)
+ if not utils_misc.wait_for(lambda: _get_host_iface() is not None, timeout=15):
+ test.fail("Host default interface not found within 15s")
+ host_iface = params.get("host_iface") or _get_host_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.
| if utils_misc.wait_for( | |
| lambda: utils_net.get_default_gateway( | |
| iface_name=True, force_dhcp=True, json=True) is None, timeout=15): | |
| test.fail("Not find host interface in 15s") | |
| host_iface = params.get("host_iface") | |
| host_iface = host_iface if host_iface else utils_net.get_default_gateway( | |
| iface_name=True, force_dhcp=True).split()[0] | |
| def _get_host_iface(): | |
| return utils_net.get_default_gateway(iface_name=True, force_dhcp=True) | |
| if not utils_misc.wait_for(lambda: _get_host_iface() is not None, timeout=15): | |
| test.fail("Host default interface not found within 15s") | |
| host_iface = params.get("host_iface") or _get_host_iface() |
🤖 Prompt for AI Agents
In
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py
around lines 89 to 95, the wait_for logic is inverted and the code immediately
fails when the default gateway is None; change the wait predicate to wait until
get_default_gateway(iface_name=True, force_dhcp=True, json=True) is not None
(timeout 15s) and only call test.fail if the wait times out, and when resolving
host_iface use the value returned by get_default_gateway(...) directly (no
.split()[0]) unless params provide host_iface first.
d03c8d5 to
15576d1
Compare
xxxx-296614: [attach-device][ethernet] hotplug ethernet type interface by attach-device Signed-off-by: nanli <[email protected]>
nanli1
force-pushed
the
add_case_for_attach_ethernet_interface
branch
from
15576d1 to
a86978f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py (3)
40-40: Replaceeval()withast.literal_evalfor security.
106-107: Replaceeval()withast.literal_evalfor security.
108-114: Fix inverted wait logic for host interface discovery.
🧹 Nitpick comments (3)
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py (3)
59-59: Remove unnecessary f-string prefix.The f-string doesn't contain any placeholders, so the
fprefix is unnecessary.- test.fail(f'Interface should be in guest') + test.fail('Interface should be in guest')
63-72: Improve exception handling in ping check wrapper.The current broad
except Exceptionis too generic and may hide real issues. Consider handling specific exceptions and logging the actual error.def _ping_check_wrapper(): try: ips = {'outside_ip': params.get('outside_ip')} network_base.ping_check(params, ips, session, force_ipv4=True) return True - except Exception: + except (exceptions.TestError, exceptions.TestFail) as e: + test.log.debug(f"Ping check failed: {e}") return False
84-84: Remove unnecessary f-string prefix.The f-string doesn't contain any placeholders, so the
fprefix is unnecessary.- test.fail(f'Interface should be removed from guest') + test.fail('Interface should be removed from guest')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/attach_detach_device/attach_ethernet_interface.cfg(1 hunks)libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/attach_detach_device/attach_ethernet_interface.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py (1)
provider/virtual_network/network_base.py (4)
create_tap(148-161)create_macvtap(164-189)ping_check(94-145)delete_tap(202-208)
🪛 Ruff (0.12.2)
libvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.py
40-40: Use of possibly insecure function; consider using ast.literal_eval
(S307)
59-59: f-string without any placeholders
Remove extraneous f prefix
(F541)
67-67: Consider moving this statement to an else block
(TRY300)
68-68: Do not catch blind exception: Exception
(BLE001)
84-84: f-string without any placeholders
Remove extraneous f prefix
(F541)
106-106: Use of possibly insecure function; consider using ast.literal_eval
(S307)
107-107: 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). (3)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
xxxx-296614: [attach-device][ethernet] hotplug ethernet type interface by attach-device
Signed-off-by: nanli [email protected]
Summary by CodeRabbit