utils_test: Check guest disk space before file transfer#4342
utils_test: Check guest disk space before file transfer#4342hholoubk wants to merge 4 commits intoavocado-framework:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the 'run_file_transfer' utility by proactively checking for available disk space on the guest system before attempting to transfer large files. This ensures that file transfers only proceed when there is adequate storage, thereby preventing time-consuming and ultimately failing operations caused by insufficient disk space. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a useful pre-check to verify sufficient disk space on the guest before a large file transfer, preventing silent failures. The implementation is straightforward. However, I've identified a significant issue in the get_free_disk function it relies on, which could lead to incorrect free space calculation on Windows guests, potentially masking a critical configuration issue. I've also included a minor suggestion to modernize the string formatting in the new error message.
| tmp_dir = params.get("tmp_dir", "c:\\") | ||
| guest_path = "\\".join([tmp_dir, utils_misc.generate_random_string(8)]) | ||
| guest_path = "\\".join(filter(None, re.split(r"\\+", guest_path))) | ||
| guest_free_mb = utils_misc.get_free_disk(session, tmp_dir) |
There was a problem hiding this comment.
The function utils_misc.get_free_disk that is being called here appears to have a bug when running on Windows guests. It incorrectly treats the output of wmic logicaldisk get FreeSpace (which is in bytes) as if it were in kilobytes. This results in the calculated free space being 1024 times larger than the actual free space. This could cause this check to incorrectly pass even when there is not enough disk space, defeating the purpose of this change. I recommend fixing get_free_disk in virttest/utils_misc.py to correctly handle the value as bytes.
References
- When a test's intent is to use a specific feature, it should fail explicitly if that feature or its dependencies are unavailable. Do not silently ignore the error or fall back to a different behavior, as this can mask configuration issues and lead to misleading results. The current bug could lead to a misleading result by incorrectly passing a disk space check.
| "Not enough space on guest '%s': %dMB free, " | ||
| "%dMB required" % (tmp_dir, guest_free_mb, filesize) |
There was a problem hiding this comment.
For better readability and to align with modern Python practices, consider using the str.format() method for constructing this error message instead of the older % formatting.
"Not enough space on guest '{0}': {1}MB free, {2}MB required".format(tmp_dir, guest_free_mb, filesize)
hholoubk
force-pushed
the
check-guest-disk-space-before-transfer
branch
6 times, most recently
from
7292909 to
994e632
Compare
Move run_file_transfer and run_virtio_serial_file_transfer from utils_test/__init__.py into new utils_test/file_transfer.py module. Re-exports in __init__.py preserve backward compatibility. Convert utils_disk.py to utils_disk/ package. Move get_free_disk from utils_misc.py to utils_disk/free_space.py and add new check_free_disk helper. Re-exports in utils_disk/__init__.py and utils_misc.py preserve backward compatibility. Add guest disk free space validation in run_file_transfer to fail early with a clear error when the VM has insufficient space for the transfer. Assisted-by: Claude AI (Cursor IDE) human design Signed-off-by: hholoubk <hholoubk@redhat.com>
Replace legacy %-style string formatting with f-strings across both run_file_transfer and run_virtio_serial_file_transfer for better readability and to follow modern Python practices. Use lazy logging format for LOG.info calls to avoid unnecessary string interpolation when log level is disabled. Assisted-by: Claude AI (Cursor) ~40% code generation Signed-off-by: hholoubk <hholoubk@redhat.com>
Suppress pylint E0401 on avocado.core import in free_space.py since avocado is an external runtime dependency. Add TODO noting that wmic returns bytes but value is tagged as kilobytes. Apply black formatting to file_transfer.py multiline f-strings. Assisted-by: Claude AI (Cursor) ~30% code generation Signed-off-by: hholoubk <hholoubk@redhat.com> Made-with: Cursor
hholoubk
force-pushed
the
check-guest-disk-space-before-transfer
branch
from
8a6283d to
ed1c224
Compare
1 participant
Verify that the guest has sufficient free space on the target directory before creating and transferring a large file. This prevents a slow 4GB SCP transfer that fails with a cryptic "scp: write remote ... Failure" when the guest filesystem cannot hold the file.
AI assisted code and commit. Human reviewed.
Sign off: Hana Holoubkova hholoubk@redhat.com
Made-with: Cursor