[guest os booting] fix image download url issue #6543
Conversation
add disk image url for fix "Unable to download boot image" cases guest_os_booting.boot_order.disk_device.seabios.disk_boot_order.multi_disks_bootable guest_os_booting.boot_order.disk_device.ovmf.disk_boot_order.multi_disks_bootable Signed-off-by: zhentang-tz <[email protected]>
|
avocado run --vt-type libvirt --vt-machine-type q35 guest_os_booting.boot_order.disk_device.seabios.disk_boot_order.multi_disks_bootable guest_os_booting.boot_order.disk_device.ovmf.disk_boot_order.multi_disks_bootable --job-timeout 1200 --vt-connect-uri qemu:///system JOB ID : 4c4644a3c701afdd6d312889006b2dfaec1fd867 |
WalkthroughAdds firmware_type handling in parse_disks_attrs to adjust the disk image URL when using OVMF: if disk1_img is set and download is enabled, the URL’s trailing ".qcow2" is replaced with "-ovmf.qcow2" before download. Other download and validation logic remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Parser as parse_disks_attrs
participant Downloader as Image Downloader
participant Validator as Image Validator
Tester->>Parser: Provide params (includes firmware_type)
alt download_disk1_img && disk1_img set
opt firmware_type == "ovmf"
Note right of Parser: Rewrite URL: *.qcow2 → *-ovmf.qcow2
end
Parser->>Downloader: Download disk1_img_url
Downloader-->>Parser: Download result
Parser->>Validator: Validate image
Validator-->>Parser: Validation result
else
Note over Parser: No download performed
end
Parser-->>Tester: Parsed disk attributes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 (1)
libvirt/tests/src/guest_os_booting/boot_order/boot_from_disk_device.py (1)
32-32: Normalize firmware_type to avoid case/None surprisesUse a default and lowercase once to make the comparison robust.
- firmware_type = params.get("firmware_type") + firmware_type = params.get("firmware_type", "").lower()
| if firmware_type == "ovmf": | ||
| disk1_img_url = disk1_img_url.removesuffix('.qcow2') + '-ovmf.qcow2' | ||
| if not disk1_img_url or not utils_misc.wait_for( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix OVMF URL rewrite: avoid Python 3.9-only API, empty-URL regression, and double "-ovmf"
- str.removesuffix requires Python 3.9+.
- If disk1_img_url is empty, current code produces "-ovmf.qcow2" and bypasses the empty-URL guard.
- If URL already ends with "-ovmf.qcow2", current code yields "...-ovmf-ovmf.qcow2".
- if firmware_type == "ovmf":
- disk1_img_url = disk1_img_url.removesuffix('.qcow2') + '-ovmf.qcow2'
+ if firmware_type == "ovmf" and disk1_img_url:
+ # Avoid double suffix and keep Py<3.9 compatibility
+ if disk1_img_url.endswith("-ovmf.qcow2"):
+ pass # already the OVMF variant
+ elif disk1_img_url.endswith(".qcow2"):
+ disk1_img_url = f"{disk1_img_url[:-6]}-ovmf.qcow2"📝 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.
| if firmware_type == "ovmf": | |
| disk1_img_url = disk1_img_url.removesuffix('.qcow2') + '-ovmf.qcow2' | |
| if not disk1_img_url or not utils_misc.wait_for( | |
| if firmware_type == "ovmf" and disk1_img_url: | |
| # Avoid double suffix and keep Py<3.9 compatibility | |
| if disk1_img_url.endswith("-ovmf.qcow2"): | |
| pass # already the OVMF variant | |
| elif disk1_img_url.endswith(".qcow2"): | |
| disk1_img_url = f"{disk1_img_url[:-6]}-ovmf.qcow2" | |
| if not disk1_img_url or not utils_misc.wait_for( |
🤖 Prompt for AI Agents
In libvirt/tests/src/guest_os_booting/boot_order/boot_from_disk_device.py around
lines 36-38, the OVMF URL rewrite uses str.removesuffix (Py3.9+) and runs even
when disk1_img_url is empty or already ends with "-ovmf.qcow2", causing
empty-URL and double-suffix bugs; fix by first checking disk1_img_url truthiness
before any rewrite, then only rewrite when firmware_type == "ovmf" AND
disk1_img_url is non-empty, and perform a safe suffix adjustment using endswith
checks (if it endswith '.qcow2' and not '-ovmf.qcow2', replace the '.qcow2'
suffix with '-ovmf.qcow2'; otherwise leave the URL unchanged) so the code works
on older Python versions and avoids empty or duplicated suffixes.
add disk image url for fix "Unable to download boot image" cases
guest_os_booting.boot_order.disk_device.seabios.disk_boot_order.multi_disks_bootable
guest_os_booting.boot_order.disk_device.ovmf.disk_boot_order.multi_disks_bootable
Summary by CodeRabbit
Bug Fixes
Tests