fix virtual network drive packed time issue #6762
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Need some waitting time to make case more stable Signed-off-by: nanli <[email protected]>
WalkthroughThis change modifies a test file to implement a retry mechanism when locating a virtio features file. Instead of directly reading the file path, the code now waits up to 30 seconds with 2-second intervals for the find command to succeed, then retrieves the file status and path. If the file is not located within this timeout period, the test fails. An additional utility import was added to support this functionality. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In @libvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.py:
- Around line 38-45: The status variable returned by session.cmd_status_output
is unused; replace it with _ (e.g., use _, virtio_features_file =
session.cmd_status_output(find_cmd)) and strip trailing whitespace/newlines from
virtio_features_file before building packed_bit_cmd (use
virtio_features_file.strip() when constructing packed_bit_cmd) so the cat
command does not receive a path with a newline.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6754
File: libvirt/tests/cfg/virtual_network/qemu/boot_with_multiqueue.cfg:19-21
Timestamp: 2026-01-10T16:53:10.863Z
Learning: In tp-libvirt test configurations for Windows guests, the avocado-vt/virttest framework automatically provides the `cdrom_virtio` parameter for accessing virtio-win ISO, even when not explicitly defined in the cfg file. The Python test code can reference `cdrom_virtio` without requiring it to be set via `cdroms += " virtio"` in the configuration.
📚 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/elements_and_attributes/driver_packed.py
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
libvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.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/elements_and_attributes/driver_packed.py
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
Applied to files:
libvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.py
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.py
41-41: Unpacked variable status is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
🔇 Additional comments (2)
libvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.py (2)
5-5: LGTM!The import of
utils_miscis correctly added to support thewait_forpolling mechanism.
16-66: Retry mechanism approach looks good.The use of
utils_misc.wait_forwith a 30-second timeout and 2-second polling interval is a reasonable approach to handle the timing issue where the virtio features file may not be immediately available after VM boot. This addresses the race condition described in the PR objectives.
Comment on lines
+38
to
45
| if not utils_misc.wait_for(lambda: session.cmd_status_output(find_cmd)[0] == 0, | ||
| timeout=30, step=2): | ||
| test.fail(f"Virtio features file not found for PCI address {pci_addr}") | ||
| status, virtio_features_file = session.cmd_status_output(find_cmd) | ||
|
|
||
| test.log.debug(f"Using virtio features file: {virtio_features_file}") | ||
|
|
||
| packed_bit_cmd = f"cat {virtio_features_file}" |
There was a problem hiding this comment.
Unused variable and missing strip() on command output.
Two issues:
- The
statusvariable on line 41 is unused (per static analysis hint). Use_to indicate it's intentionally discarded. - The output from
cmd_status_outputtypically includes a trailing newline. When used directly in thecatcommand on line 45, this could cause unexpected behavior.
Proposed fix
if not utils_misc.wait_for(lambda: session.cmd_status_output(find_cmd)[0] == 0,
timeout=30, step=2):
test.fail(f"Virtio features file not found for PCI address {pci_addr}")
- status, virtio_features_file = session.cmd_status_output(find_cmd)
-
+ _, virtio_features_file = session.cmd_status_output(find_cmd)
+ virtio_features_file = virtio_features_file.strip()
test.log.debug(f"Using virtio features file: {virtio_features_file}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not utils_misc.wait_for(lambda: session.cmd_status_output(find_cmd)[0] == 0, | |
| timeout=30, step=2): | |
| test.fail(f"Virtio features file not found for PCI address {pci_addr}") | |
| status, virtio_features_file = session.cmd_status_output(find_cmd) | |
| test.log.debug(f"Using virtio features file: {virtio_features_file}") | |
| packed_bit_cmd = f"cat {virtio_features_file}" | |
| if not utils_misc.wait_for(lambda: session.cmd_status_output(find_cmd)[0] == 0, | |
| timeout=30, step=2): | |
| test.fail(f"Virtio features file not found for PCI address {pci_addr}") | |
| _, virtio_features_file = session.cmd_status_output(find_cmd) | |
| virtio_features_file = virtio_features_file.strip() | |
| test.log.debug(f"Using virtio features file: {virtio_features_file}") | |
| packed_bit_cmd = f"cat {virtio_features_file}" |
🧰 Tools
🪛 Ruff (0.14.10)
41-41: Unpacked variable status is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In @libvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.py
around lines 38 - 45, The status variable returned by session.cmd_status_output
is unused; replace it with _ (e.g., use _, virtio_features_file =
session.cmd_status_output(find_cmd)) and strip trailing whitespace/newlines from
virtio_features_file before building packed_bit_cmd (use
virtio_features_file.strip() when constructing packed_bit_cmd) so the cat
command does not receive a path with a newline.
Contributor
Author
|
@qiankehan hi hhan, Could help review |
qiankehan
suggested changes
Jan 21, 2026
| if not utils_misc.wait_for(lambda: session.cmd_status_output(find_cmd)[0] == 0, | ||
| timeout=30, step=2): | ||
| test.fail(f"Virtio features file not found for PCI address {pci_addr}") | ||
| status, virtio_features_file = session.cmd_status_output(find_cmd) |
Contributor
There was a problem hiding this comment.
status is not used. Please use cmd_output()
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Need some waiting time to make case more stable
Before fix
(1/1) type_specific.io-github-autotest-libvirt.virtual_network.elements_and_attributes.driver_packed: FAIL: Expected packed bit 1, but got c (49.14 s)After fix
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.