Add case to support hotplug/hot-unplug nic device in a loop #6745
Conversation
WalkthroughAdds a new libvirt test configuration block virtual_network.qemu_test.hotplug_nic_with_repetition and a new Python test module implementing repeated NIC hot‑plug cycles (default 100). Configuration sets create/kill VM via libvirt, login_timeout=360, hotplug_cycles=100, dhcp_cmd default "dhcpcd -n %s" with RHEL.7/8/9 override "dhclient -r && dhclient %s", RHEL.7 sets make_change=yes, and ovmf adds kill_vm_libvirt_options=--nvram. The module implements domif_setlink, connectivity and IP renewal helpers, attach/detach interface functions, and a run routine coordinating hot‑plug, IP acquisition/renewal, connectivity checks, suspend/resume, and cleanup. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (4)
37-58: Unusedtestparameter.The
testparameter is declared but never used. Consider adding logging calls usingtest.logfor consistency with other functions, or remove the parameter if logging isn't needed here.
124-134: Redundant exit status check.With
ignore_status=False,vm.attach_interfaceraises an exception on failure, making theelsebranch on lines 133-134 unreachable. Either remove the redundant check or useignore_status=Trueif you want to handle errors manually.🔎 Proposed simplification
result = vm.attach_interface( option=attach_options, ignore_status=False, debug=True ) - - if result.exit_status == 0: - test.log.info(f"Hotplug succeed: {new_mac}") - return new_mac, iface_type - else: - raise Exception(f"Hotplug failed: {result.stderr_text}") + test.log.info(f"Hotplug succeed: {new_mac}") + return new_mac, iface_type
144-154: Same redundant exit status check pattern.Same issue as
attach_hotplug_interface- withignore_status=False, theelsebranch is unreachable.
161-161: Fullwidth characters in strings.Line 161 uses a fullwidth colon
：in the docstring and line 217 uses a fullwidth comma，. Replace with ASCII equivalents for consistency.🔎 Proposed fix
- Test Steps： + Test Steps:- test.log.info("First time failed，try to renew ip address...") + test.log.info("First time failed, try to renew ip address...")Also applies to: 217-217
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.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/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.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_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
📚 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/hotplug_nic_with_repetition.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (3)
provider/virtual_network/network_base.py (1)
ping_check(101-152)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
31-31: Consider moving this statement to an else block
(TRY300)
33-33: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
37-37: Unused function argument: test
(ARG001)
75-75: Do not catch blind exception: Exception
(BLE001)
78-78: Consider moving this statement to an else block
(TRY300)
85-85: Do not catch blind exception: Exception
(BLE001)
134-134: Create your own exception
(TRY002)
134-134: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Create your own exception
(TRY002)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
161-161: Docstring contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF002)
217-217: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
223-223: Do not catch blind exception: Exception
(BLE001)
243-243: Do not catch blind exception: Exception
(BLE001)
258-258: 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.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
🔇 Additional comments (5)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfg (1)
1-15: LGTM!The configuration is well-structured with appropriate OS-specific overrides. The RHEL.7
make_change = "yes"setting correctly handles legacy network-scripts configuration, while newer RHEL versions use nmcli. The 360-second login timeout and 100 hotplug cycles provide reasonable test parameters.libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (4)
14-23: LGTM!Clean wrapper function for
virsh.domif_setlink.
26-34: LGTM!The connectivity check logic is correct. The function properly propagates exceptions after logging.
186-194: Verify systemctl restart impact on other tests.The workaround to restart libvirtd is documented, but consider whether this could impact parallel test execution or leave the service in an unexpected state. Based on retrieved learnings, teardown handles VM cleanup, but verify that libvirtd state is properly restored if the test fails mid-execution.
196-273: Main test loop structure looks good.The test loop correctly implements the documented sequence: setlink down → hotplug → verify connectivity → suspend/resume → verify again → setlink up → unplug. The retry logic with IP renewal and VM reboot provides reasonable resilience. Based on retrieved learnings, VM cleanup is handled by the framework's teardown method.
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
yanglei-rh
force-pushed
the
LIBVIRTAT-22225
branch
from
b89d5b5 to
368af6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (1)
234-238: Remove unreachable code after test.fail().Line 236 calls
test.fail(), which raises aTestFailexception and terminates execution. Lines 237-238 are therefore unreachable. The log statement should be moved to anelsebranch or the redundantifshould be removed.🔎 Proposed fix
- if not hotnic_ip: - session.close() - test.fail(f"The {iteration + 1} round: Hotplug nic still can not obtain ip after reboot vm") - if hotnic_ip: - test.log.info(f"Obtain the hotplug nic ip: {hotnic_ip}") + if not hotnic_ip: + session.close() + test.fail(f"The {iteration + 1} round: Hotplug nic still can not obtain ip after reboot vm") + + test.log.info(f"Obtain the hotplug nic ip: {hotnic_ip}")
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (1)
52-52: Verify ARP cleanup pattern matches all relevant entries.The awk pattern
/^[1-9]/will only match lines starting with digits 1-9, potentially missing ARP entries for addresses beginning with 0 (though rare in practice). Confirm this pattern captures all intended entries, or consider/^[0-9]/for completeness.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfg
🧰 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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
79-79: unindent does not match any outer indentation level
(invalid-syntax)
79-79: Expected a statement
(invalid-syntax)
79-79: Expected a statement
(invalid-syntax)
79-80: Expected an expression
(invalid-syntax)
80-80: Unexpected indentation
(invalid-syntax)
87-87: Expected except or finally after try block
(invalid-syntax)
⏰ 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.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
🔇 Additional comments (2)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (2)
196-271: LGTM: Robust IP acquisition and connectivity verification.The test loop implements a comprehensive retry strategy with multiple fallback mechanisms:
- Initial IP acquisition attempt
- IP renewal retry on failure
- VM reboot as final fallback
The suspend/resume cycle with connectivity re-verification provides good coverage for the hotplug reliability scenario.
186-193: Restart is a documented workaround for domif-setlink support with virt-install-created VMs.The libvirtd restart on line 188 is explicitly documented in the code comment as a necessary workaround to enable domif-setlink management for virt-install-created VMs. The implementation properly handles side effects by cleaning up and recreating the serial console after the restart. This pattern is used consistently in other tests across the codebase (iface_network.py, libvirtd.py, unix_sock.py), indicating it's an accepted practice within the tp-libvirt framework. The restart occurs once during test setup rather than repeatedly, and test isolation is managed by the framework's lifecycle methods (env.get_vm).
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
yanglei-rh
force-pushed
the
LIBVIRTAT-22225
branch
from
368af6f to
1269455
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (2)
233-237: Remove unreachable code aftertest.fail().Lines 236-237 are unreachable because
test.fail()on line 235 raises aTestFailexception. The logging statement should be moved to anelseblock or the logic restructured to avoid dead code.🔎 Proposed fix
if not hotnic_ip: session.close() test.fail(f"The {iteration + 1} round: Hotplug nic still can't obtain ip after reboot vm") - if hotnic_ip: - test.log.info(f"Obtain the hotplug nic ip: {hotnic_ip}") + else: + test.log.info(f"Obtain the hotplug nic ip: {hotnic_ip}")
98-132: Conditionally include--sourceonly when non-empty.Line 115 unconditionally includes
--source {source}in the attach options. When the original interface type is neither 'network' nor 'bridge' (lines 107-110),sourceremains an empty string, resulting in--sourcebeing passed to virsh, which may cause unexpected behavior.Apply the same conditional pattern used for
model(lines 116-117):🔎 Proposed fix
- attach_options = f"--type {iface_type} --source {source} --mac {new_mac}" + attach_options = f"--type {iface_type} --mac {new_mac}" + if source: + attach_options += f" --source {source}" if model: attach_options += f" --model {model}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.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/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.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_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
📚 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/hotplug_nic_with_repetition.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (3)
provider/virtual_network/network_base.py (1)
ping_check(101-152)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
30-30: Consider moving this statement to an else block
(TRY300)
32-32: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
36-36: Unused function argument: test
(ARG001)
74-74: Do not catch blind exception: Exception
(BLE001)
77-77: Consider moving this statement to an else block
(TRY300)
78-78: Do not catch blind exception: Exception
(BLE001)
82-82: Do not catch blind exception: Exception
(BLE001)
132-132: Create your own exception
(TRY002)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Create your own exception
(TRY002)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Docstring contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF002)
215-215: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
221-221: Do not catch blind exception: Exception
(BLE001)
241-241: Do not catch blind exception: Exception
(BLE001)
256-256: 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.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (8)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfg (1)
1-15: Configuration looks well-designed for NIC hotplug stress testing.The configuration appropriately sets 100 hotplug cycles, adequate login timeout, and OS-specific DHCP commands. The RHEL.7-specific
make_change = "yes"aligns with the ifcfg-based network configuration used in the Python module'srenew_ip_addressfunction. The OVMF nvram cleanup option is also correctly specified.libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (7)
1-11: Imports and configuration look appropriate.The module imports the necessary libvirt testing utilities and sets reasonable virsh defaults for debugging and error handling.
13-22: Wrapper function is appropriate.This provides a consistent interface for domain interface link state management with debugging enabled.
25-33: Connectivity check wrapper is correct.The function appropriately wraps the ping check utility and propagates failures to the caller. The explicit
return Trueon successful ping is clear and appropriate.
36-57: IP renewal logic correctly handles both Windows and Linux guests.The function appropriately uses different approaches for Windows (network restart by MAC) and Linux (ifcfg config for RHEL.7 or nmcli for newer systems), aligning with the
make_changeparameter from the configuration file.
60-95: IP acquisition logic with retry/renew is well-implemented.The function appropriately tries multiple methods (address cache, guest interface query) with retry logic and IP renewal on failure. The detailed logging of ARP table on failure aids debugging.
135-152: Interface detachment logic is correct.The function properly constructs detach options with MAC and type, using a sensible default for the type parameter.
239-270: Main test loop logic correctly implements the test steps.The loop appropriately performs all required steps: disabling the original interface, hot-plugging a new NIC, verifying connectivity, testing suspend/resume resilience, and cleaning up. Session management and error handling are correctly implemented, with proper cleanup on failures.
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
yanglei-rh
force-pushed
the
LIBVIRTAT-22225
branch
from
1269455 to
1c7c7b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (2)
115-115: Conditionally include the--sourceargument.When the interface type is neither 'network' nor 'bridge',
sourceremains an empty string (line 103), resulting in--source ""being passed to virsh. This can cause unexpected behavior. The--sourceargument should be added conditionally, similar to howmodelis handled.🔎 Proposed fix
attach_options = f"--type {iface_type} --source {source} --mac {new_mac}" + attach_options = f"--type {iface_type} --mac {new_mac}" + if source: + attach_options += f" --source {source}" if model: attach_options += f" --model {model}"Or set a default source value when it's empty:
+ if not source: + source = "default" # or handle based on interface type requirements attach_options = f"--type {iface_type} --source {source} --mac {new_mac}"Based on past review comments, this issue was previously identified but appears not fully resolved.
233-237: Remove unreachable code aftertest.fail().Lines 236-237 are unreachable because
test.fail()on line 235 raises aTestFailexception. The checkif hotnic_ip:and the subsequent log statement will never execute.🔎 Proposed fix
if not hotnic_ip: session.close() test.fail(f"The {iteration + 1} round: Hotplug nic still can't obtain ip after reboot vm") - if hotnic_ip: - test.log.info(f"Obtain the hotplug nic ip: {hotnic_ip}") + test.log.info(f"Obtain the hotplug nic ip: {hotnic_ip}")
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (2)
36-36: Remove unusedtestparameter.The
testparameter is declared but never used in the function body. Consider removing it to clean up the function signature.🔎 Proposed fix
-def renew_ip_address(test, session, mac, is_linux_guest, params): +def renew_ip_address(session, mac, is_linux_guest, params):Note: You'll need to update all call sites to remove the
testargument (lines 81, 217).
215-215: Add missing space after comma.The log message is missing a space after the comma.
🔎 Proposed fix
- test.log.info("First time failed,try to renew ip address...") + test.log.info("First time failed, try to renew ip address...")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfg
🧰 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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (1)
provider/virtual_network/network_base.py (1)
ping_check(101-152)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
30-30: Consider moving this statement to an else block
(TRY300)
32-32: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
36-36: Unused function argument: test
(ARG001)
74-74: Do not catch blind exception: Exception
(BLE001)
77-77: Consider moving this statement to an else block
(TRY300)
78-78: Do not catch blind exception: Exception
(BLE001)
82-82: Do not catch blind exception: Exception
(BLE001)
132-132: Create your own exception
(TRY002)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Create your own exception
(TRY002)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
221-221: Do not catch blind exception: Exception
(BLE001)
241-241: Do not catch blind exception: Exception
(BLE001)
256-256: 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
yanglei-rh
force-pushed
the
LIBVIRTAT-22225
branch
from
1c7c7b5 to
79f0239
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (4)
13-22: Rename parametervmtovm_namefor clarity.The parameter is named
vmbut the docstring and usage indicate it represents a domain name string, not a VM object. Renaming it tovm_namewould improve code clarity and match the actual usage at line 201.🔎 Proposed fix
-def domif_setlink(vm, device, operation, options): +def domif_setlink(vm_name, device, operation, options): """ Set the domain link state - :param vm : domain name + :param vm_name : domain name :param device : domain virtual interface :param operation : domain virtual interface state :param options : some options like --config """ - return virsh.domif_setlink(vm, device, operation, options, debug=True) + return virsh.domif_setlink(vm_name, device, operation, options, debug=True)
25-33: Remove unused return value.The function returns
Trueon success, but this return value is never used by callers (lines 241, 256). Since the function raises an exception on failure, the return value provides no additional information. Consider removing it for simplicity.🔎 Proposed fix
def check_connectivity(test, session, ping_params, ips): - try: network_base.ping_check(ping_params, ips, session, force_ipv4=True) test.log.info("Ping succeeded") - return True except Exception as e: test.log.error("Ping failed with: %s", str(e)) raise
74-77: Remove redundantpassstatement.Line 76 contains a
passstatement after logging the exception, which is immediately followed byreturn None. Thepassis redundant and can be removed.🔎 Proposed fix
try: result = session.cmd_output_safe(ip_cmd) if result.strip(): return result.strip() except Exception as e: test.log.debug(f"Failed to get IP from guest interface: {e}") - pass return None
186-194: Clarify the workaround rationale.Lines 186-187 mention a workaround to restart libvirtd for virt-install compatibility with domif-setlink, but don't explain the root cause or provide a reference. Consider adding a bug ID, ticket number, or more detailed explanation to help future maintainers understand why this workaround is necessary.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfg
🧰 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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (2)
provider/virtual_network/network_base.py (1)
ping_check(101-152)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
30-30: Consider moving this statement to an else block
(TRY300)
32-32: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
74-74: Do not catch blind exception: Exception
(BLE001)
77-77: Consider moving this statement to an else block
(TRY300)
78-78: Do not catch blind exception: Exception
(BLE001)
82-82: Do not catch blind exception: Exception
(BLE001)
134-134: Create your own exception
(TRY002)
134-134: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Create your own exception
(TRY002)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
223-223: Do not catch blind exception: Exception
(BLE001)
242-242: Do not catch blind exception: Exception
(BLE001)
257-257: 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.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
🔇 Additional comments (3)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (3)
98-134: LGTM!The function correctly derives interface parameters from the original interface and handles the conditional source parameter appropriately. The logic is sound and the implementation is clear.
137-154: LGTM!The function provides a clean interface for detaching hotplugged NICs with appropriate defaults and error handling.
196-272: LGTM with appreciation for comprehensive error handling!The main test loop demonstrates thorough validation with:
- IP acquisition retry logic with explicit renewal and VM reboot fallback
- Connectivity verification before and after suspend/resume cycles
- Proper cleanup of sessions and device state at each iteration
The 100-cycle stress test effectively validates NIC hotplug resilience.
|
Hi @nanli1 Could you please help review this patch? Thanks a lot. |
yanglei-rh
force-pushed
the
LIBVIRTAT-22225
branch
from
79f0239 to
83f8975
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (1)
52-53: Critical: dhcp_cmd validation is still missing.Despite being flagged and marked as addressed in previous reviews, lines 52-53 still lack validation of the
dhcp_cmdparameter. Ifparams.get("dhcp_cmd")returnsNoneor a malformed string without a%splaceholder, line 53 will raiseTypeErrororValueError.🔎 Proposed fix
session.cmd_output_safe("ip link set dev %s up" % ifname) dhcp_cmd = params.get("dhcp_cmd") + if not dhcp_cmd: + raise ValueError("dhcp_cmd parameter is required but not provided") + if '%s' not in dhcp_cmd: + raise ValueError(f"dhcp_cmd must contain %s placeholder, got: {dhcp_cmd}") session.cmd_output_safe(dhcp_cmd % ifname, timeout=240)
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (2)
137-137: Consider requiring expected_type parameter.Line 137 defaults to
"network"whenexpected_typeis not provided, but this may fail if the hotplugged interface is a different type (e.g., bridge, user). Since the caller (attach_hotplug_interface) returns the actual interface type, consider either:
- Making
expected_typea required parameter- Documenting that the default only works for network-type interfaces
🔎 Option 1: Make expected_type required
-def detach_hotplug_interface(test, vm, mac_address, expected_type=None): +def detach_hotplug_interface(test, vm, mac_address, expected_type): test.log.info(f"Hot unplug interface: {mac_address}") - iface_type = expected_type or "network" + iface_type = expected_type
179-179: Add defensive check for interface existence.Line 179 accesses
[0]on the result oflibvirt.get_vm_device()without verifying the list is non-empty. While running VMs typically have at least one interface, adding a defensive check would prevent potentialIndexErrorin edge cases.🔎 Proposed defensive check
vmxml = vm_xml.VMXML.new_from_dumpxml(vm_name) - original_iface = libvirt.get_vm_device(vmxml, "interface", index=-1)[0] + ifaces = libvirt.get_vm_device(vmxml, "interface", index=-1) + if not ifaces: + test.error("VM has no network interfaces") + original_iface = ifaces[0] original_mac = original_iface.mac_address
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfg
🧰 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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (3)
provider/virtual_network/network_base.py (1)
ping_check(101-152)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
30-30: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
72-72: Do not catch blind exception: Exception
(BLE001)
74-74: Consider moving this statement to an else block
(TRY300)
75-75: Do not catch blind exception: Exception
(BLE001)
79-79: Do not catch blind exception: Exception
(BLE001)
131-131: Create your own exception
(TRY002)
131-131: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Create your own exception
(TRY002)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
220-220: Do not catch blind exception: Exception
(BLE001)
239-239: Do not catch blind exception: Exception
(BLE001)
254-254: 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.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
|
@yanglei-rh hi lei , Let's add test result |
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
|
Test pass:
|
nanli1
left a comment
nanli1
left a comment
There was a problem hiding this comment.
Some small update, Others LGTM
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
| if is_linux_guest: | ||
| ifname = utils_net.get_linux_ifname(session, hotplug_mac) | ||
| ip_cmd = f"ip addr show {ifname} | grep 'inet ' | awk '{{print $2}}' | cut -d'/' -f1" | ||
| try: | ||
| result = session.cmd_output_safe(ip_cmd) | ||
| if result.strip(): | ||
| return result.strip() | ||
| except Exception as e: | ||
| test.log.debug(f"Failed to get IP from guest interface: {e}") | ||
| return None |
There was a problem hiding this comment.
this could be reused by
session = vm.wait_for_login(login_nic_index, timeout=120, serial=True)
iface_ip = network_base.get_vm_ip(session, net_mac)
There was a problem hiding this comment.
Hi @nanli1 if I understand correct, this comment it means these two lines can replaced get_hotplug_nic_ip fucntion, right? If yes. I'd like to say we can not assume guest always can got ip address after hotplug. We test hotplug/unplug 100 times, and we need a diversity approach.
Please feel free to add your comments, thanks.
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
| if not hotnic_ip: | ||
| test.log.info("First time failed, try to renew ip address...") | ||
| try: | ||
| renew_ip_address(session, hotplug_mac, guest_is_linux, params) | ||
| hotnic_ip = get_hotplug_nic_ip( | ||
| test, vm, hotplug_mac, session, guest_is_linux, params | ||
| ) | ||
| except Exception as e: | ||
| test.log.warning(f"Still failed after renew ip address: {e}") | ||
|
|
||
| if not hotnic_ip: | ||
| test.log.info("Rebooting vm after renew ip address failed...") | ||
| session = vm.reboot(session=session, serial=True) | ||
| vm.verify_alive() | ||
|
|
||
| hotnic_ip = get_hotplug_nic_ip( | ||
| test, vm, hotplug_mac, session, guest_is_linux, params | ||
| ) | ||
|
|
||
| if not hotnic_ip: | ||
| session.close() | ||
| test.fail(f"The {iteration + 1} round: Hotplug nic still can't obtain ip after reboot vm") | ||
| test.log.info(f"Obtain the hotplug nic ip: {hotnic_ip}") |
There was a problem hiding this comment.
Do we need to check 3 times if not hotnic_ip: We could use only one , what do you think
There was a problem hiding this comment.
Good question. In fact, these three checks form a progressive process. This is done to ensure that when a problem is encountered, it can be more directly identified as a product issue rather than an automation problem.
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
yanglei-rh
force-pushed
the
LIBVIRTAT-22225
branch
from
83f8975 to
cfe8ffb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (3)
120-153: Remove unused variable assignment.Line 146 assigns
resultfromvm.attach_interface()but never uses it. Sinceignore_status=Falseis set, the call will raise an exception on failure, making the explicit check unnecessary.🔎 Proposed fix
- result = vm.attach_interface( + vm.attach_interface( option=attach_options, ignore_status=False, debug=True )
156-175: Remove unused variable assignment.Line 166 assigns
resultfromvm.detach_interface()but never uses it. Sinceignore_status=Falseis set, the call will raise an exception on failure.🔎 Proposed fix
- result = vm.detach_interface( + vm.detach_interface( option=detach_options, ignore_status=False, debug=True, wait_for_event=wait_for_event, event_type=event_type )
178-296: Remove unused variable.Line 199 assigns
ping_paramsbut it's never used in the test. This can be safely removed.🔎 Proposed fix
guest_is_linux = "linux" == params.get("os_type", "") host_ip_addr = utils_net.get_host_ip_address(params) - ping_params = {'vm_ping_host': 'pass'} ips = {'host_ip': host_ip_addr}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfg
🧰 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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/hotplug_nic_with_repetition.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (3)
libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
41-41: Create your own exception
(TRY002)
41-41: Avoid specifying long messages outside the exception class
(TRY003)
52-52: Create your own exception
(TRY002)
52-52: Avoid specifying long messages outside the exception class
(TRY003)
97-97: Do not catch blind exception: Exception
(BLE001)
99-99: Consider moving this statement to an else block
(TRY300)
100-100: Do not catch blind exception: Exception
(BLE001)
104-104: Do not catch blind exception: Exception
(BLE001)
146-146: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
166-166: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
199-199: Local variable ping_params is assigned to but never used
Remove assignment to unused variable ping_params
(F841)
210-210: Undefined name utils_libvirtd
(F821)
243-243: Do not catch blind exception: Exception
(BLE001)
264-264: Do not catch blind exception: Exception
(BLE001)
281-281: 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.9
- GitHub Check: Python 3.12
🔇 Additional comments (6)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (6)
12-12: LGTM!Good practice to define common virsh options as a module-level constant for reuse.
15-24: LGTM!This wrapper function properly delegates to
virsh.domif_setlinkwith appropriate debug logging.
28-54: LGTM!The connectivity check includes good fallback logic - if the initial ping fails on Linux guests, it attempts to add a temporary route to work around potential routing issues before retrying. This defensive approach increases test reliability.
57-80: LGTM!The function appropriately handles both Windows and Linux guests, with version-specific logic for Linux (ifcfg-based for RHEL7, nmcli for RHEL8+). The comment at lines 64-65 correctly explains the stability reasoning for the dual approach.
83-117: LGTM!The function implements a robust multi-tiered approach to IP acquisition: trying the address cache first, then shell commands for Linux guests, and finally attempting IP renewal if both fail. The 360-second timeout with debug logging provides good observability for troubleshooting.
236-258: LGTM!The progressive three-tier IP acquisition strategy (normal attempt → renewal → reboot) provides good diagnostics to distinguish between transient issues and product bugs. This defensive approach is well-suited for a 100-iteration stability test.
yanglei-rh
force-pushed
the
LIBVIRTAT-22225
branch
from
cfe8ffb to
d1381c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (1)
14-23: Usevirsh_optfor consistent error handling.The function currently hardcodes
debug=Truebut doesn't passignore_status=False, which means errors won't automatically raise exceptions and require manualexit_statuschecks. This creates inconsistency (line 221 doesn't check errors while line 287 does).As suggested in a previous review, pass
**virsh_optto ensure consistent error handling across all virsh calls.🔎 Proposed fix
def domif_setlink(vm_name, device, operation, options): """ Set the domain link state :param vm_name : domain name :param device : domain virtual interface :param operation : domain virtual interface state :param options : some options like --config """ - return virsh.domif_setlink(vm_name, device, operation, options, debug=True) + return virsh.domif_setlink(vm_name, device, operation, options, **virsh_opt)Based on previous review discussion, this change was acknowledged but not yet implemented.
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (2)
144-148: Optional: Suppress unused variable warning.The
resultvariable is assigned but never used. Sinceignore_status=Falseis passed, the call will raise an exception on failure, making the return value unnecessary. To clean up the linter warning, either omit capturing the return value or use_to indicate intentional discard:🔎 Proposed fix
- result = vm.attach_interface( + vm.attach_interface( option=attach_options, ignore_status=False, debug=True )Or alternatively:
- result = vm.attach_interface( + _ = vm.attach_interface( option=attach_options, ignore_status=False, debug=True )
164-170: Optional: Suppress unused variable warning.Same as above—the
resultvariable is assigned but never used sinceignore_status=Falsewill raise on failure.🔎 Proposed fix
- result = vm.detach_interface( + vm.detach_interface( option=detach_options, ignore_status=False, debug=True, wait_for_event=wait_for_event, event_type=event_type )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfg
🧰 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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/hotplug_nic_with_repetition.py
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
39-39: Create your own exception
(TRY002)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Create your own exception
(TRY002)
50-50: Avoid specifying long messages outside the exception class
(TRY003)
95-95: Do not catch blind exception: Exception
(BLE001)
97-97: Consider moving this statement to an else block
(TRY300)
98-98: Do not catch blind exception: Exception
(BLE001)
102-102: Do not catch blind exception: Exception
(BLE001)
144-144: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
164-164: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
197-197: Local variable ping_params is assigned to but never used
Remove assignment to unused variable ping_params
(F841)
241-241: Do not catch blind exception: Exception
(BLE001)
262-262: Do not catch blind exception: Exception
(BLE001)
279-279: 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.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
Outdated
Show resolved
Hide resolved
yanglei-rh
force-pushed
the
LIBVIRTAT-22225
branch
from
d1381c6 to
3809383
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (5)
14-23: Consider using**virsh_optfor consistent error handling.The function currently passes
debug=Trueexplicitly, but the globalvirsh_optdictionary (line 11) already defines bothdebug=Trueandignore_status=False. Using**virsh_optwould provide consistent error handling across all virsh calls and automatically raise exceptions on failure.🔎 Proposed refactor
def domif_setlink(vm_name, device, operation, options): """ Set the domain link state :param vm_name : domain name :param device : domain virtual interface :param operation : domain virtual interface state :param options : some options like --config """ - return virsh.domif_setlink(vm_name, device, operation, options, debug=True) + return virsh.domif_setlink(vm_name, device, operation, options, **virsh_opt)
144-148: Clean up unusedresultvariable.The
resultvariable is assigned but never used. Sinceignore_status=Falseis set, the function will raise an exception on failure, so the result doesn't need to be captured.🔎 Proposed cleanup
- result = vm.attach_interface( + vm.attach_interface( option=attach_options, ignore_status=False, debug=True )
164-170: Clean up unusedresultvariable.Similar to the
attach_hotplug_interfacefunction, theresultvariable is assigned but never used sinceignore_status=Falsecauses exceptions on failure.🔎 Proposed cleanup
- result = vm.detach_interface( + vm.detach_interface( option=detach_options, ignore_status=False, debug=True, wait_for_event=wait_for_event, event_type=event_type )
220-221: Clean up unusedresultvariable.The
resultfromdomif_setlinkis assigned but never used. Oncedomif_setlinkis updated to use**virsh_opt(per earlier suggestion), errors will be raised automatically, making this variable unnecessary.🔎 Proposed cleanup
- result = domif_setlink(vm_name, original_mac, "down", "") + domif_setlink(vm_name, original_mac, "down", "") test.log.info("setlink down succeed")
285-286: Clean up unusedresultvariable.Same as the earlier setlink operation, the
resultvariable is unused.🔎 Proposed cleanup
- result = domif_setlink(vm_name, original_mac, "up", "") + domif_setlink(vm_name, original_mac, "up", "") test.log.info("setlink up succeed")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🧰 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-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_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.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_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
📚 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/hotplug_nic_with_repetition.cfg
📚 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/hotplug_nic_with_repetition.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (3)
libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
39-39: Create your own exception
(TRY002)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Create your own exception
(TRY002)
50-50: Avoid specifying long messages outside the exception class
(TRY003)
95-95: Do not catch blind exception: Exception
(BLE001)
97-97: Consider moving this statement to an else block
(TRY300)
98-98: Do not catch blind exception: Exception
(BLE001)
102-102: Do not catch blind exception: Exception
(BLE001)
144-144: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
164-164: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
240-240: Do not catch blind exception: Exception
(BLE001)
261-261: Do not catch blind exception: Exception
(BLE001)
278-278: Do not catch blind exception: Exception
(BLE001)
285-285: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
⏰ 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.12
- GitHub Check: Python 3.11
Automate VIRT-99272 hotplug/hot-unplug nic device in a loop Signed-off-by: Lei Yang <leiyang@redhat.com>
yanglei-rh
force-pushed
the
LIBVIRTAT-22225
branch
from
3809383 to
7842725
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (1)
26-52: Good connectivity check with platform-specific handling.The function appropriately handles both Windows and Linux guests, with a sensible retry mechanism for Linux that adds a temporary route. The use of
utils_test.pingis confirmed valid per the framework.For slightly better Python style, consider defining a custom exception class (e.g.,
ConnectivityCheckError) instead of genericExceptionat lines 39 and 50, which would align with PEP 8 conventions and static analysis recommendations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_repetition.cfg
🧰 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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/src/virtual_network/qemu/hotplug_nic_with_repetition.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/hotplug_nic_with_repetition.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (3)
libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py
39-39: Create your own exception
(TRY002)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Create your own exception
(TRY002)
50-50: Avoid specifying long messages outside the exception class
(TRY003)
95-95: Do not catch blind exception: Exception
(BLE001)
97-97: Consider moving this statement to an else block
(TRY300)
98-98: Do not catch blind exception: Exception
(BLE001)
102-102: Do not catch blind exception: Exception
(BLE001)
240-240: Do not catch blind exception: Exception
(BLE001)
261-261: Do not catch blind exception: Exception
(BLE001)
278-278: 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.8
- GitHub Check: Python 3.12
🔇 Additional comments (9)
libvirt/tests/src/virtual_network/qemu/hotplug_nic_with_repetition.py (9)
1-11: Well-organized imports and consistent virsh options.The imports are complete (including
utils_libvirtdwhich addresses a past review concern), and thevirsh_optdictionary provides consistent error handling across virsh operations withignore_status=Falseensuring exceptions are raised on failures.
14-23: Clean wrapper with consistent error handling.The
domif_setlinkwrapper correctly applies**virsh_opt, ensuring that failures automatically raise exceptions without requiring manual exit status checks at each call site.
55-78: Well-structured IP renewal with RHEL version awareness.The function properly handles both Windows and Linux guests, with the comment at lines 62-63 explaining the RHEL7-specific approach. The dual-path Linux handling (ifcfg vs nmcli) aligns with the evolution of RHEL network management, addressing past review feedback about documenting this distinction.
Based on past review discussion confirming
dhcp_cmdis always provided in the configuration file.
81-115: Resilient IP acquisition with progressive fallback.The function employs a well-designed strategy: check cache → query guest → attempt renewal, with appropriate timeout via
utils_misc.wait_for. The broad exception handling (flagged by static analysis) is intentional and appropriate here for test stability, allowing graceful degradation through multiple recovery attempts.
118-151: Proper interface cloning with conditional parameter handling.The function correctly derives the interface configuration from the original device and conditionally includes the
--sourceparameter only when present (lines 136-137), addressing past review feedback. The use ofignore_status=Falseensures failures are caught immediately.
154-173: Good detach implementation with event synchronization.The function properly uses
wait_for_eventwithevent_type="device-removed"to handle asynchronous device removal, addressing the concern about QEMU's asynchronous detachment behavior. The comment acknowledges this pattern should eventually move to the framework, per past review discussions.
176-214: Thorough test setup with documented workaround.The initialization properly retrieves test parameters, verifies VM state, and extracts the original interface configuration. The libvirtd restart at lines 207-208 addresses virt-install compatibility (as documented in the comment) and correctly uses
utils_libvirtd.Libvirtd()which handles RHEL version differences transparently. The serial console cleanup and recreation (lines 209-213) is necessary after the daemon restart.Based on past review feedback confirming this approach for daemon restart and console management.
215-255: Excellent progressive IP acquisition with clear failure path.The IP acquisition logic employs a well-designed three-tier retry strategy:
- Initial attempt via
get_hotplug_nic_ip(lines 229-231)- Manual renewal if first attempt fails (lines 233-239)
- VM reboot as last resort (lines 243-250)
Each step is clearly logged with iteration context, and the session is properly closed before
test.fail()at line 253. This progressive approach maximizes test reliability while providing clear diagnostics when failures do occur.
256-291: Complete test flow with proper resource management.The remainder of the loop implements a thorough validation sequence:
- Initial connectivity check (lines 257-263)
- Suspend/resume cycle to verify device persistence (lines 267-269)
- Post-resume connectivity verification (lines 271-280)
- Original interface restoration and hotplug device cleanup (lines 284-290)
Sessions are consistently closed before
test.fail()calls, and each step includes clear iteration-numbered error messages for easy diagnosis. The suspend/resume validation is particularly valuable for catching state-management issues.
|
Hi @nanli1 Please help review this patch again, I already replied all the comments. There are two comments I haven't closed ; we can discuss them further if there are any issues. |
3 participants
Automate VIRT-99272 hotplug/hot-unplug nic device in a loop
ID:LIBVIRTAT-22225
Signed-off-by: Lei Yang leiyang@redhat.com
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.