virtual_network: add case for guest reboot or shutdown after detach nic #6711
Conversation
WalkthroughAdds a new Libvirt test configuration at libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg and a new Python test module libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py. The config defines test type, VM lifecycle flags, variants for shutdown/reboot and NIC models. The test module implements run_sub_test (handles guest reboot or shutdown via serial/shell) and run (retrieves VM, parses guest XML to identify the NIC, performs login and connectivity checks, suspend/resume, hot-unplug via virsh.detach_device, and invokes the sub-test after unplug), with support for repeats and timeouts. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
nanli1
changed the title
nanli1
force-pushed
the
add_case_for_guest_reboot_shutdown_after_detach_nic
branch
from
bf7c8c1 to
d1e9375
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/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.py (2)
84-87: Remove commented-out code or add explanation.The commented-out code suggests there was an issue with retrieving the existing MAC address. Either document why this approach was abandoned or remove the commented code entirely.
64-71: Update docstring to reflect actual test flow.The docstring states that shutdown/reboot happens at step 7 after unplugging the interface, but the code can also perform shutdown/reboot after plugging (step 6 in the actual code, line 138). Update the docstring to accurately describe both scenarios.
Example:
Test procedure: 1) Boot up guest without interface 2) Hotplug virtio-net interface through libvirt 3) Check if new interface gets ip address 4) Ping guest new ip from host 5) Pause/resume VM and verify connectivity - 6) Hotunplug the interface - 7) Perform shutdown or reboot operation based on test variant + 6) Optionally perform shutdown or reboot after hotplug + 7) Hotunplug the interface + 8) Optionally perform shutdown or reboot after hotunplug
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.801Z
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.
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
📚 Learning: 2025-11-24T10:44:47.801Z
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.801Z
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/cfg/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.py
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.py (2)
provider/virtual_network/network_base.py (1)
ping_check(98-149)libvirt/tests/src/controller/controller_functional.py (1)
detach_device(563-576)
🪛 Ruff (0.14.7)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.py
79-79: 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.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
| """ | ||
| vm_name = params.get("main_vm") | ||
| vm = env.get_vm(vm_name) | ||
| iface_attrs = eval(params.get("iface_attrs", "{}")) |
There was a problem hiding this comment.
Replace eval() with ast.literal_eval() for security.
Using eval() on external input is a security risk as it can execute arbitrary code. Use ast.literal_eval() instead, which safely evaluates strings containing Python literals.
Apply this diff:
+from ast import literal_eval
+
from virttest import virsh
from virttest import utils_misc, utils_net
from virttest.libvirt_xml import vm_xml- iface_attrs = eval(params.get("iface_attrs", "{}"))
+ iface_attrs = literal_eval(params.get("iface_attrs", "{}"))🧰 Tools
🪛 Ruff (0.14.7)
79-79: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.py
around line 79, replace the unsafe use of eval(params.get("iface_attrs", "{}"))
with ast.literal_eval(params.get("iface_attrs", "{}")) to safely parse only
Python literals; add "import ast" at the top of the file and wrap the
literal_eval call in a try/except to catch ValueError/SyntaxError and fall back
to {} (or re-raise with a clear error) so malformed input is handled gracefully.
| vm_switched_off = run_sub_test(test, vm, params, "after_plug") | ||
| if vm_switched_off: | ||
| return | ||
|
|
||
| test.log.info("TEST_STEP 6: Hot-unplug the virtio-net interface") | ||
| virsh.detach_device(vm_name, iface.xml, wait_for_event=True, **virsh_opt) | ||
| run_sub_test(test, vm, params, "after_unplug") |
There was a problem hiding this comment.
Handle VM shutdown after unplug consistently.
Line 138 checks if the VM was shut down after plug and returns early, but line 144 ignores the return value from run_sub_test after unplug. If the VM is shut down after unplug and repeat_times > 1, the next iteration will fail when trying to operate on a shut-down VM.
Apply this diff to handle shutdown after unplug consistently:
test.log.info("TEST_STEP 6: Hot-unplug the virtio-net interface")
virsh.detach_device(vm_name, iface.xml, wait_for_event=True, **virsh_opt)
- run_sub_test(test, vm, params, "after_unplug")
+ vm_switched_off = run_sub_test(test, vm, params, "after_unplug")
+ if vm_switched_off:
+ return📝 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.
| vm_switched_off = run_sub_test(test, vm, params, "after_plug") | |
| if vm_switched_off: | |
| return | |
| test.log.info("TEST_STEP 6: Hot-unplug the virtio-net interface") | |
| virsh.detach_device(vm_name, iface.xml, wait_for_event=True, **virsh_opt) | |
| run_sub_test(test, vm, params, "after_unplug") | |
| test.log.info("TEST_STEP 6: Hot-unplug the virtio-net interface") | |
| virsh.detach_device(vm_name, iface.xml, wait_for_event=True, **virsh_opt) | |
| vm_switched_off = run_sub_test(test, vm, params, "after_unplug") | |
| if vm_switched_off: | |
| return |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_shutdown_reboot.py
around lines 138 to 144, the code checks and returns if the VM was shut down
after the plug step but ignores the return value from
run_sub_test("after_unplug"), which can lead to operating on a shut-down VM on
subsequent iterations; capture the return value from run_sub_test(test, vm,
params, "after_unplug") into a variable (e.g., vm_switched_off_after_unplug) and
if it is truthy, return immediately to handle shutdown consistently.
c4398a3 to
ed0d3ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.py (2)
9-15: Replaceeval()withast.literal_eval()for iface_attrs.Using
eval()on config data is unnecessary and flagged by Ruff (S307). Safer to parse literals only.Apply this minimal change:
+from ast import literal_eval + from virttest import virsh from virttest import utils_net @@ vm_name = params.get("main_vm") vm = env.get_vm(vm_name) - iface_attrs = eval(params.get("iface_attrs", "{}")) + iface_attrs = literal_eval(params.get("iface_attrs", "{}"))This keeps behavior the same for valid dict literals while avoiding arbitrary code execution from configs. Please re-run static analysis to confirm S307 is cleared.
Also applies to: 71-75
116-123: Honor shutdown after unplug by checkingrun_sub_testreturn value.You already exit early when the VM is shut down in the
after_plugsubtest, but the result ofrun_sub_test(..., "after_unplug")is ignored. If a variant shuts the guest down after unplug andrepeat_times > 1, the next iteration will try to operate on a powered‑off VM.Handle shutdown after unplug consistently:
vm_switched_off = run_sub_test(test, vm, params, "after_plug") if vm_switched_off: return test.log.info("TEST_STEP 5: Hot-unplug the virtio-net interface") virsh.detach_device(vm_name, iface.xml, wait_for_event=True, **virsh_opt) - run_sub_test(test, vm, params, "after_unplug") + vm_switched_off = run_sub_test(test, vm, params, "after_unplug") + if vm_switched_off: + returnThis ensures you don’t attempt further iterations or operations once the guest has been shut down in an
after_unplugsubtest.
🧹 Nitpick comments (2)
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.cfg (1)
16-27: Confirm intended variant matrix (4 subcases vs 2 JIRA scenarios).The nested
variantsunderwith_shutdown/with_rebootcreate four subcases (after_plugandafter_unplugfor both shutdown and reboot), which is slightly broader than the two “after hotunplug” scenarios mentioned in the PR description. If the JIRAs only require post‑unplug flows, you could drop theafter_plugvariants to reduce runtime; otherwise this looks structurally fine.libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.py (1)
30-48: Skip serial setup when no subtest is configured for a given plug_tag.For variants where only one of
sub_type_after_plug/sub_type_after_unplugis set,run_sub_teststill opens a serial console and logs in even thoughsub_type_*is unset, which is wasted work.You can return early when no sub_type is defined:
def run_sub_test(test, vm, params, plug_tag): @@ - sub_type = params.get("sub_type_%s" % plug_tag) - login_timeout = params.get_numeric("login_timeout", 360) + sub_type = params.get(f"sub_type_{plug_tag}") + if not sub_type: + return False + + login_timeout = params.get_numeric("login_timeout", 360) vm.cleanup_serial_console() vm.create_serial_console()This avoids unnecessary serial setup in the
after_plug/after_unplugvariants that don’t define a subtest.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.801Z
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.801Z
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.801Z
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/cfg/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.cfglibvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.py
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.cfglibvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.py (5)
libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)provider/virtual_network/network_base.py (1)
ping_check(98-149)libvirt/tests/src/controller/controller_functional.py (1)
detach_device(563-576)
🪛 Ruff (0.14.7)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_shutdown_reboot.py
73-73: Use of possibly insecure function; consider using ast.literal_eval
(S307)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py
Outdated
Show resolved
Hide resolved
| take_regular_screendumps = no | ||
| create_vm_libvirt = "yes" | ||
| kill_vm_libvirt = "yes" | ||
| outside_ip = "www.google.com" |
There was a problem hiding this comment.
If you'd like to test the IP layer connectivity only, it is better to use IP not hostname. Because hostname relies on the DNS service. Here I recommend using the cloudflare service IP 1.1.1.1 / 1.0.0.1 or google service IP 8.8.8.8 / 8.4.4.8
There was a problem hiding this comment.
Good advice, Updated
| vm = env.get_vm(vm_name) | ||
| iface_attrs = eval(params.get("iface_attrs", "{}")) | ||
| login_timeout = int(params.get("login_timeout", 360)) | ||
| repeat_times = int(params.get("repeat_times", 1)) |
There was a problem hiding this comment.
74-75: use params.get_numeric()
nanli1
force-pushed
the
add_case_for_guest_reboot_shutdown_after_detach_nic
branch
from
ed0d3ff to
ea33e93
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py (3)
73-73: Replaceeval()withast.literal_eval()for security.Using
eval()on external input can execute arbitrary code. Useast.literal_eval()which safely evaluates only Python literals.Apply this diff:
+from ast import literal_eval + from virttest import virsh- iface_attrs = eval(params.get("iface_attrs", "{}")) + iface_attrs = literal_eval(params.get("iface_attrs", "{}"))
78-78: Fix ping pattern to usevm_ping_hostinstead ofguest_ping_host.Looking at
network_base.ping_checkinprovider/virtual_network/network_base.py, the source is determined by splitting on_ping_and checking ifsource == 'vm'to use the session. Withguest_ping_host, the source becomesguest, soping_sessionwill beNoneand the ping won't originate from the VM.- ping_params = {'guest_ping_host': 'pass'} + ping_params = {'vm_ping_host': 'pass'}
120-122: Handle VM shutdown after unplug consistently.The return value from
run_sub_testat line 122 is ignored. If the VM shuts down after unplug andrepeat_times > 1(or if someone modifies the config), the next iteration would fail trying to operate on a shut-down VM. This should be handled consistently with line 116-118.test.log.info("TEST_STEP 5: Hot-unplug the virtio-net interface") virsh.detach_device(vm_name, iface.xml, wait_for_event=True, **virsh_opt) - run_sub_test(test, vm, params, "after_unplug") + vm_switched_off = run_sub_test(test, vm, params, "after_unplug") + if vm_switched_off: + return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.801Z
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.
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
📚 Learning: 2025-11-24T10:44:47.801Z
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.801Z
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/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfglibvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfglibvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py (5)
libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)provider/virtual_network/network_base.py (1)
ping_check(98-149)libvirt/tests/src/controller/controller_functional.py (1)
detach_device(563-576)
🪛 Ruff (0.14.8)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py
73-73: 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.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
🔇 Additional comments (2)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py (1)
1-17: LGTM!Imports and module setup are appropriate for the test functionality.
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg (1)
15-31: LGTM!The variants structure is well-organized, properly defining the shutdown and reboot test scenarios with appropriate sub-type configurations. The
iface_attrsdefinition correctly uses variable substitution formodel_type.
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg
Outdated
Show resolved
Hide resolved
| vm.cleanup_serial_console() | ||
| vm.create_serial_console() | ||
| session = vm.wait_for_serial_login(timeout=login_timeout) | ||
| shutdown_method = params.get("shutdown_method", "shell") | ||
| shutdown_command = params.get("shutdown_command") | ||
| if sub_type == "reboot": | ||
| test.log.info("Running sub test '%s' %s", sub_type, plug_tag) | ||
| if params.get("reboot_method"): | ||
| vm.reboot(session, params["reboot_method"], 0, login_timeout, serial=True) | ||
| elif sub_type == "shutdown": | ||
| test.log.info("Running sub test '%s' %s", sub_type, plug_tag) | ||
| if shutdown_method == "shell": | ||
| # Send a shutdown command to the guest's shell | ||
| session.sendline(shutdown_command) | ||
| return True | ||
|
|
||
| return False |
There was a problem hiding this comment.
Session resource leak: close session before returning.
The session created at line 34 is not closed in any code path. For the shutdown case, the session should be closed before returning. For other paths, the session is also leaked.
Apply this diff to properly close the session:
session = vm.wait_for_serial_login(timeout=login_timeout)
shutdown_method = params.get("shutdown_method", "shell")
shutdown_command = params.get("shutdown_command")
if sub_type == "reboot":
test.log.info("Running sub test '%s' %s", sub_type, plug_tag)
if params.get("reboot_method"):
vm.reboot(session, params["reboot_method"], 0, login_timeout, serial=True)
elif sub_type == "shutdown":
test.log.info("Running sub test '%s' %s", sub_type, plug_tag)
if shutdown_method == "shell":
# Send a shutdown command to the guest's shell
session.sendline(shutdown_command)
+ session.close()
return True
+ session.close()
return False🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py around
lines 32 to 48, the serial session created with vm.wait_for_serial_login(...) is
never closed and is leaked on all code paths; wrap the usage in a try/finally
(or ensure session.close()/session.close_force() is called) so the session is
closed before any return (e.g., close the session in the shutdown branch before
returning True and in the default path before returning False), and handle the
case where session creation fails by guarding the close call.
| reboot_method = shell | ||
| variants: | ||
| - nic_virtio_net: | ||
| model_type = virtio |
There was a problem hiding this comment.
Please help add a variant to support e1000e device.
| variants: | ||
| - with_shutdown: | ||
| variants: | ||
| - after_plug: |
There was a problem hiding this comment.
This variant after_plug duplicate with another PR, please help delete it.
| shutdown_method = shell | ||
| check_image = yes | ||
| - with_reboot: | ||
| sub_type_after_plug = reboot |
| Transferred from tp-qemu nic_hotplug.one_pci.nic_virtio.with_shutdown/with_reboot | ||
| and adapted to tp-libvirt structure and function usage. | ||
|
|
||
| Test procedure: |
There was a problem hiding this comment.
For this case's test steps should be:
1. Boot a guest with nic device
2. check it connectivity
3. Pause/resume vm and verify connectivity again
4. Hot unplug nic device
5. Perform shutdown or reboot
ea33e93 to
591b092
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg (1)
9-13: Makeshutdown_commandOS-aware (Linux vs Windows) or document it as unused for Windows runs.
shutdown_command = "shutdown -h now"(Line 9) is Linux-specific, but the PR description shows Win2025 coverage. If Windows uses a different shutdown path, consider overriding in aWindows:block (or a comment) to avoid future confusion/misconfig.Also applies to: 19-23
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py
🧰 Additional context used
🧠 Learnings (4)
📓 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.801Z
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.
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
📚 Learning: 2025-11-24T10:44:47.801Z
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.801Z
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/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg
⏰ 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 (2)
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg (2)
15-23: Variants look aligned with review feedback (noafter_plug, includese1000e).Good:
with_shutdown/with_rebootfocus on the “after_unplug” scenario, and thenic_e1000e_netvariant is included.Also applies to: 25-28
14-28: No issues identified with variant structure.Multiple
variants:blocks at the same indentation level is standard cartesian config syntax in tp-libvirt and correctly generates the intended cartesian product. This pattern is used throughout the codebase (e.g.,guestfish_file_dir.cfg), confirming the parser handles it properly. Your structure will correctly expand into: (with_shutdown.after_unplug + with_reboot) × (virtio + e1000e).Likely an incorrect or invalid review comment.
xxxx-95889 - [virtual network][virtual-nic-device] guest reboot after hotunplug nic xxxx-95888 - [virtual network][virtual-nic-device] guest shutdown after hotunplug nic Signed-off-by: nanli <[email protected]>
nanli1
force-pushed
the
add_case_for_guest_reboot_shutdown_after_detach_nic
branch
from
591b092 to
c986a94
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py (2)
100-100: Handle VM shutdown after unplug to prevent failures on repeat.If the VM shuts down after unplug and
repeat_times > 1, the next iteration will fail when trying to operate on a shut-down VM.Apply this diff to check the return value and exit the loop if the VM was shut down:
- run_sub_test(test, vm, params, "after_unplug") + vm_switched_off = run_sub_test(test, vm, params, "after_unplug") + if vm_switched_off: + return
21-48: Session resource leak and missing return value.The serial session created at line 35 is never closed in any code path, and the function doesn't return
Trueafter successful shutdown as documented.Apply this diff to close the session properly and return the correct value:
session = vm.wait_for_serial_login(timeout=login_timeout) shutdown_method = params.get("shutdown_method", "shell") shutdown_command = params.get("shutdown_command") if sub_type == "reboot": test.log.info("Running sub test '%s' %s", sub_type, plug_tag) if params.get("reboot_method"): vm.reboot(session, params["reboot_method"], 0, login_timeout, serial=True) elif sub_type == "shutdown": test.log.info("Running sub test '%s' %s", sub_type, plug_tag) if shutdown_method == "shell": session.sendline(shutdown_command) if not utils_misc.wait_for(lambda: vm.state() == "shut off", 120): + session.close() test.fail("VM failed to shutdown within timeout") + session.close() + return True + + session.close() + return FalseBased on learnings, VM cleanup is handled by the teardown method in the libvirt test framework.
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py (2)
89-95: Remove duplicate connectivity check.Step 3 (line 92) already verifies connectivity after resume, making the ping check in step 4 (lines 94-95) redundant.
Apply this diff to remove the duplicate check:
test.log.info("TEST_STEP 3: Pause vm and resume and check connectivity again") virsh.suspend(vm_name, **virsh_opt) virsh.resume(vm_name, **virsh_opt) network_base.ping_check(ping_params, ips, session, force_ipv4=True) - test.log.info("TEST_STEP 4: Ping host from guest after resume") - network_base.ping_check(ping_params, ips, session, force_ipv4=True) session.close() - test.log.info("TEST_STEP 5: Hot-unplug the interface") + test.log.info("TEST_STEP 4: Hot-unplug the interface") virsh.detach_device(vm_name, iface.xml, wait_for_event=True, **virsh_opt) - run_sub_test(test, vm, params, "after_unplug") + test.log.info("TEST_STEP 5: Perform shutdown or reboot operation") + vm_switched_off = run_sub_test(test, vm, params, "after_unplug") + if vm_switched_off: + return
86-96: Consider protecting session with try/finally.If an exception occurs between session creation (line 86) and closure (line 96), the session resource will leak.
Apply this diff to ensure the session is always closed:
test.log.info("TEST_STEP 2: Ping host from guest") session = vm.wait_for_login(timeout=login_timeout) - network_base.ping_check(ping_params, ips, session, force_ipv4=True) - - test.log.info("TEST_STEP 3: Pause vm and resume and check connectivity again") - virsh.suspend(vm_name, **virsh_opt) - virsh.resume(vm_name, **virsh_opt) - network_base.ping_check(ping_params, ips, session, force_ipv4=True) - - test.log.info("TEST_STEP 4: Ping host from guest after resume") - network_base.ping_check(ping_params, ips, session, force_ipv4=True) - session.close() + try: + network_base.ping_check(ping_params, ips, session, force_ipv4=True) + + test.log.info("TEST_STEP 3: Pause vm and resume and check connectivity again") + virsh.suspend(vm_name, **virsh_opt) + virsh.resume(vm_name, **virsh_opt) + network_base.ping_check(ping_params, ips, session, force_ipv4=True) + + test.log.info("TEST_STEP 4: Ping host from guest after resume") + network_base.ping_check(ping_params, ips, session, force_ipv4=True) + finally: + session.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/hotunplug_nic_with_operation.cfg
🧰 Additional context used
🧠 Learnings (5)
📓 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.801Z
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.
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py
📚 Learning: 2025-11-24T10:44:47.801Z
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.801Z
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/virtual_network/qemu/hotunplug_nic_with_operation.py
📚 Learning: 2025-11-18T01:20:50.891Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6654
File: libvirt/tests/src/virtual_network/qemu/boot_with_multiqueue.py:56-58
Timestamp: 2025-11-18T01:20:50.891Z
Learning: In the tp-libvirt project, when calling session command methods and the status code is not needed, prefer using `session.cmd_output()` over `session.cmd_status_output()` to avoid unused variables.
Applied to files:
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py
⏰ 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.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
🔇 Additional comments (2)
libvirt/tests/src/virtual_network/qemu/hotunplug_nic_with_operation.py (2)
1-19: LGTM: Imports and setup are clean.The imports and global configuration are appropriate for a libvirt NIC hotplug test.
50-78: LGTM: Test setup is correct.The test initialization properly retrieves the VM, configures ping parameters, and prepares the test environment.
|
|
||
| def run(test, params, env): | ||
| """ | ||
| Test hotplug of virtio-net NIC devices with shutdown/reboot operations. |
There was a problem hiding this comment.
Please also help update this doc string based on new Test procedure.
yanglei-rh
left a comment
yanglei-rh
left a comment
There was a problem hiding this comment.
Others code are LGTM.
3 participants
xxxx-95889 - [virtual network][virtual-nic-device] guest reboot after hotunplug nic
xxxx-95888 - [virtual network][virtual-nic-device] guest shutdown after hotunplug nic
Windows check pass
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.