Network: Add case for detach-interface with --persistent flag #6627
Conversation
WalkthroughAdds Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (4)
208-215: Reduce flakiness: replace fixed sleep with a short poll for XML readiness.Polling avoids race with libvirt XML updates.
Apply this diff:
- # Wait a moment for XML to stabilize - time.sleep(2) - - active_pci = get_interface_pci_address( - vm_name, persistent_mac, True) - inactive_pci = get_interface_pci_address( - vm_name, persistent_mac, False) + # Wait for both active/inactive XML to expose addresses + active_pci = inactive_pci = None + for _ in range(10): # ~10s max + active_pci = get_interface_pci_address(vm_name, persistent_mac, True) + inactive_pci = get_interface_pci_address(vm_name, persistent_mac, False) + if active_pci and inactive_pci: + break + time.sleep(1)
261-271: Reduce flakiness: poll for interface removal instead of fixed sleep.Wait until the interface disappears from both XMLs, then assert.
Apply this diff:
- # Wait a moment for XML to update - time.sleep(2) - - if check_interface_in_xml(vm_name, persistent_mac, True): - test.fail( - "Interface %s still exists in active XML after detach" % persistent_mac) - - if check_interface_in_xml(vm_name, persistent_mac, False): - test.fail( - "Interface %s still exists in inactive XML after detach" % persistent_mac) + # Wait for XMLs to reflect the detach + active_present = inactive_present = True + for _ in range(10): # ~10s max + active_present = check_interface_in_xml(vm_name, persistent_mac, True) + inactive_present = check_interface_in_xml(vm_name, persistent_mac, False) + if not active_present and not inactive_present: + break + time.sleep(1) + + if active_present: + test.fail("Interface %s still exists in active XML after detach" % persistent_mac) + if inactive_present: + test.fail("Interface %s still exists in inactive XML after detach" % persistent_mac)
285-293: Tighten cleanup error handling; avoid broadexcept Exception(Ruff BLE001).With
ignore_status=True, virsh returns a result; checkexit_statusand log details.Apply this diff:
- for mac in attached_macs: - try: - cleanup_options = set_interface_options( - iface_type, None, mac, "", "detach", None) - virsh.detach_interface( - vm_ref, cleanup_options, **virsh_dargs) - logging.info("Cleaned up interface with MAC %s", mac) - except Exception as e: - logging.warning( - "Failed to cleanup interface with MAC %s: %s", mac, e) + for mac in attached_macs: + cleanup_options = set_interface_options( + iface_type, None, mac, "", "detach", None) + res = virsh.detach_interface(vm_ref, cleanup_options, **virsh_dargs) + if res.exit_status != 0: + logging.warning("Cleanup detach failed for %s (rc=%s): %s", + mac, res.exit_status, res.stderr) + else: + logging.info("Cleaned up interface with MAC %s", mac)
11-14: Logger naming: avoid shadowing theloggingmodule.Rename the logger variable (e.g.,
LOGGER = log.getLogger(...)) and useLOGGER.info/debug(...)throughout for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg(1 hunks)libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
291-291: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg (1)
1-10: Config looks consistent with the new test.Parameters match the script defaults and flow; no blockers from config side.
.../src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
Outdated
Show resolved
Hide resolved
|
Test results: |
| # Set VM reference | ||
| if vm_ref == "domname": | ||
| vm_ref = vm_name | ||
| elif vm_ref == "domid": | ||
| vm_ref = vm.get_id() | ||
| elif vm_ref == "domuuid": | ||
| vm_ref = vm.get_uuid() |
There was a problem hiding this comment.
There is no id or uuiid , we just use vm_name is ok , remove useless condition.We should focus on our case , not just see others' case usage.
There was a problem hiding this comment.
okay, remove it ,done
| if start_vm == "yes": | ||
| if not vm.is_alive(): | ||
| vm.start() |
There was a problem hiding this comment.
if start_vm = "yes", avocado-vt would start vm automatically, no need to start again
There was a problem hiding this comment.
OKay. I already update this code, please check it
| if detach_result.exit_status != 0: | ||
| test.fail("Failed to detach persistent interface: %s" % | ||
| detach_result.stderr) |
There was a problem hiding this comment.
We could use libvirt.check_exit_status(ret) as last pr we mentioned
There was a problem hiding this comment.
yes, i see, thanks
| # Wait a moment for XML to update | ||
| time.sleep(2) | ||
|
|
There was a problem hiding this comment.
Please double check if we need this sleep
There was a problem hiding this comment.
Not need, I will remove it
| virsh_dargs = {'ignore_status': True, 'debug': True, 'uri': uri} | ||
|
|
There was a problem hiding this comment.
could you check all virsh_dargs usage in this pr, if all operation needs to be correct, we should set 'ignore_status': False
| # Using as lower capital is not the best way to do, but this is just a | ||
| # workaround to avoid changing the entire file. |
There was a problem hiding this comment.
Please remove this as last pr
| # Cleanup: Detach any remaining attached interfaces | ||
| logging.info("Cleanup: Detaching remaining interfaces") | ||
| for mac in attached_macs: | ||
| try: | ||
| cleanup_options = set_interface_options( | ||
| iface_type, None, mac, "", "detach", None) | ||
| virsh.detach_interface( | ||
| vm_ref, cleanup_options, **virsh_dargs) | ||
| logging.info("Cleaned up interface with MAC %s", mac) | ||
| except Exception as e: | ||
| logging.warning( | ||
| "Failed to cleanup interface with MAC %s: %s", mac, e) |
There was a problem hiding this comment.
no need to detach if we use backup_xml.sync() in L298 ,Please double confirm if I was right.
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
a49ae02 to
a4d782d
Compare
|
Test results: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (1)
202-215: Fix:test.failis called with multiple args, causing a TypeError at runtime
test.fail()expects a single message string. The current call passes the format string plusactive_pcias a second positional argument, which will raise aTypeErrorinstead of failing with the intended message.Apply a fix like this so only one string is passed:
- if active_pci == inactive_pci: - test.fail( - "PCI addresses are the same in active and inactive XML: %s", active_pci) + if active_pci == inactive_pci: + test.fail( + "PCI addresses are the same in active and inactive XML: %s" % active_pci)This matches the style used in the other
test.failcalls in this file.
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (1)
136-137:attached_macslist is effectively unused and can be removed
attached_macsis populated and later haspersistent_macremoved, but its contents are never otherwise read. Now that cleanup relies onbackup_xml.sync()and VM destroy, the list adds complexity without benefit.If you don’t plan to extend cleanup based on this list, consider dropping it:
- # List to store MAC addresses of attached interfaces - attached_macs = [] @@ - attached_macs.append(mac) @@ - attached_macs.append(persistent_mac) @@ - # Remove the persistent MAC from our tracking list since it was detached - attached_macs.remove(persistent_mac) - - logging.info( - "Test completed successfully: All test steps passed") + logging.info( + "Test completed successfully: All test steps passed")This keeps the test logic focused on the behavior under test.
Also applies to: 164-167, 185-187, 252-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg(1 hunks)libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py(1 hunks)
🧰 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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
🔇 Additional comments (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg (1)
1-9: Config entry matches test implementation and vt conventionsThe new test entry’s name, type, and parameters (vm_ref, iface type/source/model,
initial_interface_count) line up with the Python test logic and should integrate cleanly with the virtual_network hotplug suite.libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (1)
14-37: Helper functions and main test flow look consistent and readable
set_interface_options,check_interface_in_xml, andget_interface_pci_addressare clear and match howrun()uses them to exercise attach/detach and XML inspection. The five test steps inrun()align well with the scenario described in the docstring, and use oflibvirt.check_exit_statusplusbackup_xml.sync()infinallygives good safety around failures and state restoration.Also applies to: 40-97, 100-170
| if check_interface_in_xml(vm_name, persistent_mac, True): | ||
| test.fail( | ||
| "Interface %s still exists in active XML after detach" % persistent_mac) | ||
|
|
||
| if check_interface_in_xml(vm_name, persistent_mac, False): | ||
| test.fail( | ||
| "Interface %s still exists in inactive XML after detach" % persistent_mac) | ||
|
|
| logging.info("Detaching persistent interface with MAC %s", | ||
| persistent_mac) |
There was a problem hiding this comment.
I personally think too many logs for this case, also because this case is very simple ,we don't need so many logs.
for example, the two logs are duplicated, Please only left the second one is ok.
Please also check similar issue in the whole case
logging.info("Detaching persistent interface with MAC %s",
persistent_mac)
and
logging.info(
"Successfully detached persistent interface with MAC %s", persistent_mac)
| if vm_ref == "domname": | ||
| vm_ref = vm_name |
There was a problem hiding this comment.
Could I ask why do you 3 parts below to get vm_ref = vm_name, and also no other types in this case. I think it's redundant, we could just set vm_ref = vm_name in cfg is ok
at_detach_iface_vm_ref = "domname"
vm_ref = params.get("at_detach_iface_vm_ref", "domname")
if vm_ref == "domname":
vm_ref = vm_name
There was a problem hiding this comment.
Yes, it's redundant, I have removed it, please check it
| logging.info("Active XML PCI address: %s", active_pci) | ||
| logging.info("Inactive XML PCI address: %s", inactive_pci) |
There was a problem hiding this comment.
Please add these logs in get_interface_pci_address as one log
| if active_pci is None: | ||
| test.fail( | ||
| "Could not find PCI address for interface %s in active XML" % persistent_mac) | ||
|
|
||
| if inactive_pci is None: | ||
| test.fail( | ||
| "Could not find PCI address for interface %s in inactive XML" % persistent_mac) |
There was a problem hiding this comment.
I guess we could combine this to checking by one checking, for example
if active_pci is None or inactive_pci is None:
.....
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
a4d782d to
0e37fe2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (1)
2-2: Remove unused import.The
timemodule is imported but never used in the code.Apply this diff:
-import time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg(1 hunks)libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py(1 hunks)
🧰 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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
🪛 Ruff (0.14.7)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
190-190: Expected :, found name
(invalid-syntax)
190-191: Expected an expression
(invalid-syntax)
191-191: Unexpected indentation
(invalid-syntax)
194-194: Expected except or finally after try block
(invalid-syntax)
240-240: Expected a statement
(invalid-syntax)
240-240: Expected a statement
(invalid-syntax)
240-241: Expected a statement
(invalid-syntax)
242-242: Unexpected indentation
(invalid-syntax)
🔇 Additional comments (10)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg (1)
1-9: LGTM!The test configuration is well-structured and defines appropriate parameters for the detach-interface test scenario. The values align with the test objectives documented in the Python script.
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (9)
14-37: LGTM!The function correctly builds virsh command options with appropriate conditional logic, particularly handling the source parameter only for attach operations.
40-63: LGTM!The function correctly checks for interface presence in both active and inactive VM XML configurations.
66-97: LGTM!The function correctly retrieves and formats PCI addresses from VM XML, handling both active and inactive configurations appropriately.
118-136: LGTM!The test setup properly initializes VM backup, normalizes the connection URI, extracts test parameters, and configures virsh arguments with
ignore_status=Falsefor proper error handling.
138-162: LGTM!Step 1 correctly starts the VM and attaches the initial set of interfaces, properly using
libvirt.check_exit_status()and tracking MAC addresses.
164-180: LGTM!Step 2 correctly attaches the persistent interface with the
--persistentoption and tracks its MAC address.
182-201: Verify PCI address comparison logic.After fixing the syntax error at line 190, the rest of Step 3's logic correctly compares PCI addresses between active and inactive XML. The test appropriately expects them to differ and fails if they're the same.
203-217: LGTM!Step 4 correctly detaches the persistent interface using the
--persistentoption with proper error handling.
219-245: LGTM!Step 5 correctly verifies the interface is removed from both active and inactive XML. The cleanup in the finally block appropriately destroys the VM and restores the original configuration using
backup_xml.sync().
.../src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
Outdated
Show resolved
Hide resolved
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
0e37fe2 to
31b8fc0
Compare
|
Test results: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (2)
135-137: Consider droppingattached_macssince it’s not used functionally
attached_macsis only used to append MACs and then removepersistent_macat the end, but the list never drives any checks or cleanup logic. This can be simplified:- # List to store MAC addresses of attached interfaces - attached_macs = [] @@ - attached_macs.append(mac) @@ - attached_macs.append(persistent_mac) @@ - # Remove the persistent MAC from our tracking list since it was detached - attached_macs.remove(persistent_mac)Removing this avoids confusion about additional cleanup and keeps the test focused on XML-based verification.
Also applies to: 157-159, 176-177, 234-235
182-201: PCI “must differ” assumption may be brittle across libvirt/QEMU versionsThe test currently fails if
active_pci == inactive_pci. That matches the specific bug scenario you’re targeting, but if future libvirt/QEMU behavior changes so that the PCI address is the same in both XMLs, the detach behavior might still be correct while this test starts failing.If the real objective is “detach works when addresses can differ”, consider:
- Treating equal PCI addresses as a
test.cancel()or a logged info+skip rather thantest.fail(), or- Clearly documenting (in a comment) that this test is only valid on environments where attach with
--persistentis known to produce different PCI addresses.This would make the test more robust to future changes in libvirt implementation details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg(1 hunks)libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py(1 hunks)
🧰 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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
🔇 Additional comments (4)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (3)
14-37: Option-string builder for virsh attach/detach looks soundThe
set_interface_options()helper cleanly centralizes option construction for both attach and detach (including--persistentand model/source handling) and matches howvirsh.attach_interface/virsh.detach_interfaceexpect extra arguments. No issues from a correctness standpoint.
40-97: XML helpers correctly encapsulate interface presence and PCI lookup
check_interface_in_xml()andget_interface_pci_address()useVMXML.new_from_dumpxml/new_from_inactive_dumpxmlanddevices.by_device_tag('interface')as expected, and the PCI extraction logic via the<address type='pci'>element is aligned with libvirt’s interface XML. The logging is also precise and will be helpful when debugging failures.
100-245: End‑to‑end flow and cleanup are coherentThe overall
run()flow—starting the VM (if needed), attaching initial interfaces, adding a persistent interface, validating PCI addresses, detaching with--persistent, verifying removal from both active/inactive XML, and finally restoring the original inactive XML viabackup_xml.sync()—is coherent and matches the scenario described in the PR. The use oflibvirt.check_exit_status()withignore_status=Falseis appropriate, and thetry/finallyensures the VM XML is restored even on failure.libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg (1)
1-8: Config wiring matches the new test moduleThe cfg entry cleanly wires
virtual_network.hotplug.detach_interface_persistent_pci_difftotype = detach_interface_persistent_pci_diff, and the interface attributes plusinitial_interface_count = 3are consistent with the Python test’s expectations and defaults.
| logging.info("PCI addresses differ between active (%s) and inactive (%s) XML as expected", | ||
| active_pci, inactive_pci) | ||
|
|
||
| logging.info("Step 3 completed: PCI address verification done") |
| logging.info( | ||
| "Step 4: Detaching persistent interface with --persistent option") | ||
|
|
||
| detach_options = set_interface_options(iface_type, None, persistent_mac, | ||
| "--persistent", "detach", None) | ||
|
|
||
| detach_result = virsh.detach_interface( | ||
| vm_name, detach_options, **virsh_dargs) | ||
| libvirt.check_exit_status(detach_result) | ||
|
|
||
| logging.info( | ||
| "Successfully detached persistent interface with MAC %s", persistent_mac) | ||
| logging.info( | ||
| "Step 4 completed: Persistent interface detached successfully") |
There was a problem hiding this comment.
Please check as we discussed , L204 and L216 are duplicated , no need so many logs, Please check again for the whole file, not just where I added the comment
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
31b8fc0 to
dccfc5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (1)
2-2: Remove unused import.The
timemodule is imported but never used in the code.Apply this diff:
import logging as log -import time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg(1 hunks)libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (1)
provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)
🔇 Additional comments (12)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (12)
11-11: LGTM!Standard avocado logger setup pattern.
14-37: LGTM!The function correctly builds attach/detach interface options, properly handling the difference between attach and detach operations (source only needed for attach).
40-63: LGTM!The function correctly checks for interface existence in both active and inactive XML, with appropriate logging.
66-97: LGTM!The function correctly extracts PCI address information from the XML, handling both active and inactive configurations.
118-136: LGTM!The test setup is correct: backup XML is created, parameters are retrieved with appropriate defaults, and
virsh_dargsis properly configured withignore_status: Falseas required.
140-142: LGTM!The defensive VM start check is harmless and ensures the VM is running and ready regardless of the test configuration.
145-156: LGTM!The initial interface attachment loop is correct: generates unique MACs, properly calls attach-interface, checks exit status, and tracks attached interfaces.
158-167: LGTM!The persistent interface attachment is correct: uses
--persistentflag, checks exit status, and tracks the MAC for later verification.
170-183: LGTM!The PCI address verification correctly checks that addresses differ between active and inactive XML, with proper error handling and clear messages. The test.fail syntax issue from previous reviews has been fixed.
186-192: LGTM!The persistent interface detachment correctly uses
--persistentflag and MAC address, checks exit status, and logs the operation.
195-203: LGTM!The verification correctly ensures the interface is removed from both active and inactive XML after detach with
--persistent, which is the core test objective.
205-209: LGTM!The cleanup correctly destroys the VM if running and restores the original configuration, ensuring a clean test environment.
|
Test result: |
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
dccfc5a to
ccc8262
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (1)
106-107: Consider simplifying: the attached_macs list tracks only one MAC.The
attached_macslist only ever contains one entry (persistent_mac), which is added at line 127 and removed at line 163. The initial interfaces are not tracked. Consider replacing this list with a simple variable likepersistent_macsince you're only tracking a single interface.🔎 Simplified approach without the list:
- # List to store MAC addresses of attached interfaces - attached_macs = []Then remove lines 127 and 163:
- attached_macs.append(persistent_mac) logging.info( "Successfully attached persistent interface with MAC %s", persistent_mac)- # Remove the persistent MAC from our tracking list since it was detached - attached_macs.remove(persistent_mac)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg(1 hunks)libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py(1 hunks)
🧰 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.
📚 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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
🪛 Ruff (0.14.8)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
117-117: Expected ), found name
(invalid-syntax)
118-118: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
121-121: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
122-122: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
124-124: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
125-125: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
127-127: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
128-128: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
132-132: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
134-134: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
136-136: Expected else, found :
(invalid-syntax)
138-138: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
138-140: Invalid annotated assignment target
(invalid-syntax)
140-140: Expected else, found :
(invalid-syntax)
143-143: Expected a statement
(invalid-syntax)
143-143: Expected a statement
(invalid-syntax)
148-148: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
149-149: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
150-150: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
151-151: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
151-155: Invalid annotated assignment target
(invalid-syntax)
155-155: Expected else, found :
(invalid-syntax)
158-158: Expected else, found :
(invalid-syntax)
163-163: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
165-165: Expected a statement
(invalid-syntax)
165-165: Expected a statement
(invalid-syntax)
169-169: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
🔇 Additional comments (5)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (4)
1-9: LGTM!The imports and logging setup are appropriate for this libvirt test module.
12-35: LGTM!The helper function correctly checks for interface presence in both active and inactive XML, with appropriate debug logging.
38-69: LGTM!The helper function correctly retrieves PCI addresses from VM XML, properly handling both active and inactive configurations. The PCI address parsing and formatting logic is accurate.
165-169: LGTM!The cleanup logic correctly destroys the VM if running and restores the original XML configuration, ensuring proper test cleanup.
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg (1)
1-9: LGTM!The test configuration is well-structured and correctly defines all necessary parameters for the detach-interface test case. The initial_interface_count of 3 matches the test documentation, and all interface parameters are appropriate.
.../src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
Outdated
Show resolved
Hide resolved
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
ccc8262 to
f33bba3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (1)
72-169: Excellent implementation! Test logic is correct and well-structured.The test properly implements all steps from the test plan:
- Attaches initial interfaces
- Attaches persistent interface with MAC
- Verifies PCI addresses differ between active/inactive XML
- Detaches with --persistent flag
- Confirms removal from both XMLs
- Restores VM state via backup XML
Previous critical issues (syntax errors, missing operators) have been addressed and the code is now functional and correct.
💡 Optional cleanup suggestions
1. Unused tracking list (lines 106, 127, 163):
The
attached_macslist is created, modified, but never read. Since cleanup is handled bybackup_xml.sync()at line 169, this tracking is unnecessary and can be removed:- # List to store MAC addresses of attached interfaces - attached_macs = [] - try: # Step 1: Start VM and attach 3 initial interfacesattach_result = virsh.attach_interface(vm_name, persistent_options, **virsh_dargs) libvirt.check_exit_status(attach_result) - attached_macs.append(persistent_mac) logging.info( "Successfully attached persistent interface with MAC %s", persistent_mac)if check_interface_in_xml(vm_name, persistent_mac, False): test.fail( f"Interface {persistent_mac} still exists in inactive XML after detach") - - # Remove the persistent MAC from our tracking list since it was detached - attached_macs.remove(persistent_mac) finally:2. Unused loop variable (line 115):
Per Python conventions, use
_for intentionally unused loop variables:- for i in range(initial_interface_count): + for _ in range(initial_interface_count): attach_options = ("%s %s --model %s" % (iface_type, iface_source, iface_model))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg(1 hunks)libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py(1 hunks)
🧰 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.
📚 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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.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/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
🪛 Ruff (0.14.8)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
115-115: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ 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.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
🔇 Additional comments (4)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.cfg (1)
1-9: LGTM! Configuration is clean and complete.The test configuration properly defines all required parameters for the PCI address difference test scenario. The parameter names follow tp-libvirt conventions and the values are appropriate.
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py (3)
1-10: LGTM! Clean imports and standard logging setup.The imports are appropriate for a libvirt hotplug test and the logging setup follows tp-libvirt conventions.
12-35: LGTM! Well-implemented helper function.The function correctly checks for interface presence in both active and inactive XML with appropriate logging.
38-69: LGTM! Solid PCI address extraction logic.The function correctly extracts PCI addresses from XML with proper error handling and default values for optional attributes.
|
Test results: |
.../src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
Outdated
Show resolved
Hide resolved
.../src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
Outdated
Show resolved
Hide resolved
.../src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
Outdated
Show resolved
Hide resolved
.../src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
Outdated
Show resolved
Hide resolved
.../src/virtual_network/hotplug/attach_detach_interface/detach_interface_persistent_pci_diff.py
Outdated
Show resolved
Hide resolved
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
f25aafa to
b52d22e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (1)
215-215: Critical: Fix the typo in method name.The method name should be
get_numeric, notget_numerict. This typo will cause anAttributeErrorat runtime.- login_timeout = params.get_numerict("login_timeout", "360") + login_timeout = params.get_numeric("login_timeout", "360")
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (2)
242-252: Consider refactoring to eliminate code duplication.The attach logic branches between lines 243-248 build similar attach_options strings with minor differences. Per previous review feedback, this can be simplified for better maintainability.
🔎 Proposed refactor to consolidate attach_options building
for i in range(initial_interface_count): - if i < initial_interface_count - 1: - attach_options = ("%s %s --model %s" % (iface_type, iface_source, iface_model)) - else: - # Prepare attach options - attach_options = ("%s %s --model %s --mac %s %s" % (iface_type, iface_source, - iface_model, iface_mac, test_flags)) + # Prepare attach options + base_options = "%s %s --model %s" % (iface_type, iface_source, iface_model) + if i == initial_interface_count - 1: + attach_options = "%s --mac %s %s" % (base_options, iface_mac, test_flags) + else: + attach_options = base_options # Execute attach-interface command attach_result = virsh.attach_interface(vm_name, attach_options, **virsh_dargs) # Check command execution result
267-267: Consider adding status_error parameter for consistency.Line 252 passes
status_errortocheck_exit_status, but this line doesn't. While the detach operation in this scenario likely doesn't expect errors, maintaining consistency with the attach check improves code uniformity.- libvirt.check_exit_status(detach_result) + libvirt.check_exit_status(detach_result, status_error)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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.
📚 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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
status_error(82-84)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
276-276: Avoid specifying long messages outside the exception class
(TRY003)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (5)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (5)
62-93: LGTM! Well-structured PCI address extraction.The helper function correctly extracts PCI addresses from VM XML with proper handling of both active and inactive configurations. The defensive None return and logging are appropriate.
96-117: LGTM! PCI address comparison logic is sound.The function appropriately validates PCI address differences between active and inactive XML states with clear error reporting.
240-240: Good use of configurable timeout.Making the login timeout configurable improves test flexibility for environments with varying performance characteristics.
270-278: LGTM! Detach verification logic is thorough.The code correctly verifies interface removal from both active and inactive XML after detach. The explicit checks and clear error messages are appropriate for test validation.
279-282: Good design: preserving existing test flows.The conditional branching maintains backward compatibility by preserving the comprehensive verification path for existing test scenarios while adding the new PCI-diff-detach functionality.
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
b52d22e to
99cf684
Compare
|
Test results: |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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.
📚 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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
status_error(82-84)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
276-276: Avoid specifying long messages outside the exception class
(TRY003)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (5)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfg (1)
58-64: LGTM - Test scenario configuration is well-defined.The new
persistent_pci_diff_detachvariant appropriately restricts tovm_running, sets the required test parameters, and usesinitial_interface_count = 4to create conditions where PCI address differences can occur between active and inactive XML.libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (4)
62-93: LGTM - Well-structured PCI address extraction.The function correctly parses the interface address element and constructs the PCI address string with sensible defaults.
96-117: LGTM - PCI address difference verification is correct.The function properly handles the three cases: missing addresses, identical addresses, and different addresses (success case).
242-252: LGTM - Attach loop consolidation looks good.The loop correctly attaches multiple interfaces with only the final one including the tracked MAC address and test flags. This addresses the code duplication noted in the previous review.
258-278: LGTM - PCI diff detach scenario handling is correct.The flow properly:
- Checks PCI address difference when enabled
- Detaches with
--persistentflag- Verifies interface removal from both active and inactive XML
The inline exception messages (flagged by static analysis TRY003) are acceptable here as they provide clear, specific context for test failures.
...rtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
Outdated
Show resolved
Hide resolved
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
99cf684 to
0467bde
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (1)
258-282: LGTM! PCI diff/detach test scenario logic is correct.The scenario properly:
- Validates PCI address differences between active/inactive XML
- Detaches the interface with
--persistent- Confirms removal from both XML states
The verification flow ensures the interface is completely removed, which is the expected behavior.
Optional: Address static analysis TRY003 hints
The error messages at lines 276 and 278 could be moved to constants for consistency with Ruff's TRY003 guideline, though this is a minor style consideration for test code:
# At module level INTERFACE_IN_ACTIVE_XML_ERROR = "Interface still exists in active XML after detach" INTERFACE_IN_INACTIVE_XML_ERROR = "Interface still exists in inactive XML after detach" # In the function if active_exists: raise exceptions.TestFail(INTERFACE_IN_ACTIVE_XML_ERROR) if inactive_exists: raise exceptions.TestFail(INTERFACE_IN_INACTIVE_XML_ERROR)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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.
📚 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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfg
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
276-276: Avoid specifying long messages outside the exception class
(TRY003)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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
🔇 Additional comments (6)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (4)
62-93: LGTM! Well-structured PCI address extraction helper.The function correctly retrieves PCI addresses from either active or inactive VM XML, with proper error handling and logging. The PCI address format construction is accurate.
96-117: LGTM! PCI difference validation is sound.The function properly validates that PCI addresses exist in both active and inactive XML and that they differ. The error messages clearly indicate the failure scenarios.
214-218: LGTM! New parameters properly integrated.The new test parameters are correctly retrieved with appropriate defaults:
initial_interface_countdefaults to "1" for backward compatibility ✓login_timeoutusesget_numericcorrectly ✓- Boolean flags use proper "yes" checks ✓
242-252: LGTM! Multi-attach loop refactoring eliminates code duplication.The loop correctly handles both single and multi-interface attachment scenarios:
- First (n-1) interfaces: attached without MAC/flags (live attach)
- Last interface: attached with MAC and test_flags
- Each attach is validated immediately
This creates the intended PCI address difference when using
--persistenton the last interface while the VM has other live-only interfaces.libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfg (2)
3-3: LGTM! Login timeout configured for stability.The 360-second timeout is set globally to ensure VMs have sufficient time to become ready, particularly for the multi-attach scenario that requires a stable VM state.
58-64: LGTM! New test variant properly configured.The
persistent_pci_diff_detachvariant correctly configures the test to:
- Attach 4 interfaces (3 live-only, 1 with
--persistent) to create PCI address differences- Validate PCI addresses differ between active and inactive XML
- Detach with
--persistentand verify complete removalThe variant name clearly describes the test scenario and aligns with the Python implementation.
|
@nanli1 and @qiankehan , Could you help to review this patch? thanks |
The sentence seems incomplete and I cannot get its meanings. Please update the commit msg.
|
| expected_error = params.get("expected_error", "") | ||
| test_scenario = params.get("test_scenario", "") | ||
| login_timeout = params.get_numeric("login_timeout", "360") | ||
| check_pci_diff = "yes" == params.get("check_pci_diff", "no") |
There was a problem hiding this comment.
Use params.get_boolean() and apply it for the next line
There was a problem hiding this comment.
I have updated it, please check it.thanks
| attach_options = ("%s %s --model %s --mac %s %s" % (iface_type, iface_source, | ||
| iface_model, iface_mac, test_flags)) | ||
| # Execute attach-interface command | ||
| attach_result = virsh.attach_interface( |
There was a problem hiding this comment.
You moved virsh.attach_interface() to the loop for i in range(initial_interface_count). However, the loop will not be run the old cases. That's to say, you skip the virsh.attach_interface() for the old cases. Please make sure your update have no effect on the old cases.
There was a problem hiding this comment.
It did not affect the old cases
There was a problem hiding this comment.
It increases the unnecessary complexity when mixing the code for persistent_pci_diff_detach or non-persistent_pci_diff_detach cases together in a for loop. It is better to use if branch to separate them:
if test_scenario == "pci_diff_detach":
...There was a problem hiding this comment.
if you have updated as this comment, Please reply hhan here
| raise exceptions.TestFail("Interface still exists in inactive XML after detach") | ||
| else: | ||
| # Comprehensive verification | ||
| comprehensive_verification(vm_name, vm, iface_type, iface_source, iface_mac, |
There was a problem hiding this comment.
Does comprehensive_verification() work for your cases? If so, it is better to keep it in your cases.
There was a problem hiding this comment.
For this case, Not need it, I have update this code,please check it ,thanks
| inactive_exists = check_interface_xml(vm_name, iface_type, iface_source, | ||
| iface_mac, params, check_active=False) | ||
|
|
||
| if active_exists: |
There was a problem hiding this comment.
Vars active_exists or inactive_exists is only used once. No need to define them.
There was a problem hiding this comment.
Ok, I update it, please check it
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
0467bde to
66923a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In
@libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py:
- Line 215: Update the numeric default values passed to params.get_numeric by
replacing the string defaults with numbers: change
params.get_numeric("login_timeout", "360") to use the numeric default 360 and
change params.get_numeric("initial_interface_count", "1") to use the numeric
default 1; these calls to get_numeric (used in this test script) accept numeric
defaults and should follow the numeric-default convention used elsewhere (e.g.,
boot_with_multiqueue.py).
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (2)
96-117: LGTM! PCI difference validation is correct.The function properly validates that PCI addresses exist and differ between active and inactive XML, raising appropriate exceptions with clear diagnostic information.
Note: Static analysis flags lines 108 and 112 for long exception messages (TRY003). Consider extracting to constants if following strict style guides, though the current implementation is clear and functional.
258-280: Consider verification when detach is skipped in pci_diff_detach scenario.When
test_scenario == "pci_diff_detach"butdetach_interface == False, the attach operation completes and PCI differences may be checked, but no verification of the attach success occurs (comprehensive_verification is skipped).If this is intentional (e.g., separate test variants that only validate PCI diff), document the rationale. Otherwise, consider adding basic attach verification before the detach step.
Example: Add attach verification
if test_scenario == "pci_diff_detach": logging.info("Starting PCI address difference and detach test") + + # Verify attach succeeded before checking PCI diff + if not check_interface_xml(vm_name, iface_type, iface_source, + iface_mac, params, check_active=True): + raise exceptions.TestFail("Interface not found in active XML after attach") + if check_pci_diff: check_pci_address_difference(vm_name, iface_mac)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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.
📚 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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
status_error(82-84)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
272-272: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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
🔇 Additional comments (2)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (2)
62-93: LGTM! Well-structured PCI address retrieval.The helper correctly handles both active and inactive XML, gracefully returns None when the interface or PCI address is not found, and provides useful debug logging.
242-252: LGTM! Loop logic preserves backward compatibility.The attach loop correctly handles both single-attach (default,
initial_interface_count=1) and multi-attach scenarios. For the default case, the loop runs once with MAC and test_flags, matching the original behavior.
...rtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
Outdated
Show resolved
Hide resolved
|
Test results: |
| attach_options = ("%s %s --model %s --mac %s %s" % (iface_type, iface_source, | ||
| iface_model, iface_mac, test_flags)) | ||
| # Execute attach-interface command | ||
| attach_result = virsh.attach_interface( |
There was a problem hiding this comment.
It increases the unnecessary complexity when mixing the code for persistent_pci_diff_detach or non-persistent_pci_diff_detach cases together in a for loop. It is better to use if branch to separate them:
if test_scenario == "pci_diff_detach":
...| test_flags = "--persistent" | ||
| initial_interface_count = 4 | ||
| check_pci_diff = "yes" | ||
| detach_interface = "yes" |
There was a problem hiding this comment.
check_pci_diff or detach_interface is not a general param and only be used in persistent_pci_diff_detach. Since persistent_pci_diff_detach has indicated them, they could be removed in the code
There was a problem hiding this comment.
Yes, remove it,thanks
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
66923a5 to
68c8e2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (1)
258-273: LGTM! Consider extraction for the exception messages.The PCI diff detach verification logic correctly:
- Validates that PCI addresses differ between active and inactive XML
- Performs the detach with
--persistentflag- Verifies complete removal from both XML configurations
Optionally, you could address the TRY003 static analysis hints by extracting the exception messages into constants or using custom exception classes, though this is a minor style improvement and not required given the test framework's patterns.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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.
📚 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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
status_error(82-84)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py
270-270: Avoid specifying long messages outside the exception class
(TRY003)
273-273: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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
🔇 Additional comments (4)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (4)
62-93: LGTM!The
get_interface_pci_addresshelper correctly extracts PCI addresses from either active or inactive VM XML by MAC address. The PCI address formatting follows the standarddomain:bus:slot.functionnotation, and the function appropriately returnsNonewhen the interface is not found.
96-116: LGTM!The
check_pci_address_differencefunction correctly validates that PCI addresses differ between active and inactive XML, which is the core requirement for this test scenario. The error handling appropriately raisesTestFailfor missing or identical addresses.
238-238: LGTM!The VM login now uses the configurable
login_timeoutparameter, which improves test flexibility for scenarios that may require longer VM startup times.
275-278: LGTM!Correctly preserves the existing
comprehensive_verificationfor all non-pci_diff_detachscenarios while allowing the new scenario to use its own specialized verification logic.
yiqianwei
changed the title
|
Test results: |
|
| logging.debug("Interface %s PCI address in %s XML: %s", | ||
| iface_mac, "active" if check_active else "inactive", pci_addr) | ||
| return pci_addr | ||
|
|
||
| logging.debug("Interface %s PCI address not found in %s XML", | ||
| iface_mac, "active" if check_active else "inactive") |
| if check_interface_xml(vm_name, iface_type, iface_source, | ||
| iface_mac, params, check_active=True): | ||
| raise exceptions.TestFail("Interface still exists in active XML after detach") | ||
| if check_interface_xml(vm_name, iface_type, iface_source, | ||
| iface_mac, params, check_active=False): | ||
| raise exceptions.TestFail("Interface still exists in inactive XML after detach") |
There was a problem hiding this comment.
Is there any reason we use raise exceptions.TestFail instead of test.fail
| raise exceptions.TestFail("Interface still exists in inactive XML after detach") | ||
|
|
||
| if test_scenario != "pci_diff_detach": | ||
| # Comprehensive verification |
Test detach-interface behavior when active and inactive XML have different PCI addresses for the same interface Test steps: 1. Attach 3 initial interfaces to running VM 2. Attach target interface with specific MAC and --persistent flags 3. Verify PCI addresses differ between active and inactive XML 4. Detach interface with --persistent flag 5. Verify interface is removed from both active and inactive XML Signed-off-by: Yiqian Wei <yiwei@redhat.com>
yiqianwei
force-pushed
the
detach_interface_persistent_pci_diff
branch
from
68c8e2b to
5ada9ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (3)
235-235: Optional: Simplify the wait_for_login call.The context manager is used but the session is immediately closed. Consider simplifying to:
♻️ Simpler alternative
- vm.wait_for_login(timeout=login_timeout).close() + vm.wait_for_login(timeout=login_timeout)Note: The
wait_for_login()method returns a session object, but if you're just verifying the VM is accessible and don't need to use the session, calling without.close()is cleaner since the session will be garbage-collected. Alternatively, if you want to be explicit about closing:- vm.wait_for_login(timeout=login_timeout).close() + session = vm.wait_for_login(timeout=login_timeout) + session.close()
237-241: Add logging to explain the multi-attach loop.The initial interface attachments create PCI topology differences between active and inactive XML, which is critical to the test's purpose. Adding a log message would improve test clarity and help future maintainers understand this mechanism.
📝 Suggested logging addition
if test_scenario == "pci_diff_detach": + logging.info("Attaching %d initial interfaces to create PCI topology differences", + initial_interface_count) for _ in range(initial_interface_count):
255-271: Consider logging the PCI addresses for debugging.The
check_pci_address_differencefunction returns the active and inactive PCI addresses, but this return value is currently unused. Capturing and logging these addresses would aid in debugging test failures.🐛 Proposed enhancement for debugging
if test_scenario == "pci_diff_detach": logging.info("Starting PCI address difference and detach test") - check_pci_address_difference(vm_name, iface_mac) + active_pci, inactive_pci = check_pci_address_difference(vm_name, iface_mac) + logging.info("Verified PCI address difference - Active: %s, Inactive: %s", + active_pci, inactive_pci) logging.info("Detaching interface with MAC %s using --persistent", iface_mac)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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.
📚 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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.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/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
🔇 Additional comments (6)
libvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfg (2)
3-3: LGTM: Increased login timeout is reasonable.The 360-second timeout provides sufficient time for VM login operations across all test scenarios, which is appropriate given the test may involve multiple interface attach/detach operations.
58-62: LGTM: New test scenario configuration is well-structured.The
persistent_pci_diff_detachvariant correctly:
- Restricts to
vm_runningstate (as PCI address differences require an active VM)- Sets
test_scenarioto identify this specific test path- Uses
--persistentflag to update both active and inactive XML- Configures
initial_interface_count = 3to create PCI topology differencesThe parameters align properly with the Python test implementation.
libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py (4)
62-90: LGTM: PCI address extraction logic is sound.The function correctly:
- Fetches the appropriate XML (active/inactive) based on the parameter
- Iterates through interfaces to locate the target MAC address
- Extracts PCI address components with sensible defaults
- Returns None when the interface or PCI address is not found
The implementation meets the test requirements effectively.
93-113: LGTM: PCI address comparison logic is correct.The function properly:
- Retrieves PCI addresses from both active and inactive XML
- Validates that both addresses are present before comparison
- Raises TestFail with clear error messages when addresses are missing or identical
- Returns both addresses for further use or logging
This is the core validation for the PCI address difference test scenario and is implemented correctly.
211-213: LGTM: New test parameters are properly extracted.The parameters for the new test scenario are correctly retrieved with appropriate defaults:
test_scenario: identifies the test pathlogin_timeout: configurable timeout (defaults to 360s)initial_interface_count: number of initial interfaces to attach (defaults to 3)
272-274: LGTM: Conditional verification logic is appropriate.The test correctly skips
comprehensive_verificationfor thepci_diff_detachscenario since this scenario implements its own specialized verification logic (lines 264-270) that checks interface removal from both active and inactive XML.
|
Test results: It did not affect other cases: |
3 participants
Test detach-interface behavior when active and inactive XML
have different PCI addresses for the same interface
Test steps:
Signed-off-by: Yiqian Wei yiwei@redhat.com
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.