Fix/guest os booting aarch64 ci#6872
Conversation
On UEFI virt guests, boot order appears on the disk device (<boot order='1'/>) instead of <os><boot dev='hd'/>. Map the legacy xpath when the host or params indicate AArch64/arm64, including arm64-mmio machine types and nested os_xpath structures. Use a regex for xpath matching, walk dict values, and log the adapted os_xpath for debugging. Committer: Bolatbek Issakh <bissakh@redhat.com>
Params often point custom_loader_path under /usr/share/edk2/ovmf/, which does not exist on AArch64 hosts. Resolve to a writable path under the Avocado data directory or /tmp, rewrite loader_dict paths to match, makedirs before copy2, and surface OSError text on failure. Fixes positive_test.customize_loader on arm64-mmio. Committer: Bolatbek Issakh <bissakh@redhat.com>
Try multiple qcow2 URLs (arch-specific first on aarch64), optional disk1_img_url_extra and longer download timeout. When os_attrs_boots is set, clear conflicting per-disk boot/loadparm after os boots. Extend serial login wait on aarch64 (default 480s, overridable via serial_login_timeout). Committer: Bolatbek Issakh <bissakh@redhat.com>
WalkthroughThis PR introduces AArch64 boot and firmware support enhancements across three test modules. It adds architecture detection to determine if a guest is ARM64-like, conditionally rewrites legacy OS boot XPath predicates to UEFI disk boot format, and generates multiple OVMF/AArch64 candidate URLs with retry-across-all logic. Boot configuration is cleaned to prevent conflicts, serial login timeouts are made platform-specific, and OVMF loader paths are validated with fallback to alternative writable directories when needed. All three modules also gain improved error reporting and logging. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libvirt/tests/src/guest_os_booting/boot_order/boot_from_disk_device.py`:
- Around line 46-51: Replace uses of platform.machine() in this test with the
guest-arch parameters like params.get("arch"), params.get("vm_arch_name"),
params.get("machine_type") or params.get("shortname") (same pattern as
_guest_is_aarch64_arm64() in without_default_os_attributes.py); specifically,
change the image URL selection block (currently branching on platform.machine())
to detect the guest architecture from params and prefer aarch64 variants only
when the guest is aarch64, and also update the login timeout logic that uses
platform.machine() (around the later login timeout lines) to use the same
params-based guest-arch check so timeouts and image ordering reflect the guest,
not the host.
In
`@libvirt/tests/src/guest_os_booting/os_configuration/without_default_os_attributes.py`:
- Around line 80-81: Replace the insecure eval() call used to parse
params.get("os_xpath") with ast.literal_eval() to safely evaluate only Python
literal structures; update the os_xpath assignment passed to
_adapt_os_xpath_uefi_disk_boot by calling
ast.literal_eval(params.get("os_xpath")) and add/import the ast module at the
top of the file if missing, keeping the rest of the call (params, host_arch)
unchanged.
In `@libvirt/tests/src/guest_os_booting/ovmf_firmware/ovmf_loader.py`:
- Around line 52-69: The fallback destinations for custom_loader_path use shared
basenames causing race/overwrite; change the branch that builds the alt
candidates to create a unique per-run path (e.g. use tempfile.mkdtemp() to make
a unique temp directory or tempfile.NamedTemporaryFile(delete=False) /
tempfile.mkstemp() to create a unique temp file) and join the basename into that
unique path before calling _replace_path_in_obj and returning; update the alt
variable used in the os.access check and loader_dict replacement so the
copy/cleanup uses this unique per-test path (referencing alt,
custom_loader_path, loader_dict, and _replace_path_in_obj).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9cf29d2-ed73-49b1-a6cf-104ad4189821
📒 Files selected for processing (3)
libvirt/tests/src/guest_os_booting/boot_order/boot_from_disk_device.pylibvirt/tests/src/guest_os_booting/os_configuration/without_default_os_attributes.pylibvirt/tests/src/guest_os_booting/ovmf_firmware/ovmf_loader.py
| if platform.machine() == "aarch64": | ||
| # Prefer arch-named images first: generic URL may be x86-only | ||
| # and still HTTP-200, then guest never reaches login. | ||
| urls = aarch64_variants + [disk1_img_url, ovmf_url] | ||
| else: | ||
| urls = [ovmf_url, disk1_img_url] + aarch64_variants |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the file mentioned in the review
find . -name "boot_from_disk_device.py" -type fRepository: autotest/tp-libvirt
Length of output: 136
🏁 Script executed:
# Also find the reference file
find . -name "without_default_os_attributes.py" -type fRepository: autotest/tp-libvirt
Length of output: 150
🏁 Script executed:
# Get an overview of the repository structure
git ls-files | head -20Repository: autotest/tp-libvirt
Length of output: 560
🏁 Script executed:
wc -l ./libvirt/tests/src/guest_os_booting/boot_order/boot_from_disk_device.pyRepository: autotest/tp-libvirt
Length of output: 140
🏁 Script executed:
cat -n ./libvirt/tests/src/guest_os_booting/boot_order/boot_from_disk_device.pyRepository: autotest/tp-libvirt
Length of output: 7333
🏁 Script executed:
wc -l ./libvirt/tests/src/guest_os_booting/os_configuration/without_default_os_attributes.pyRepository: autotest/tp-libvirt
Length of output: 154
🏁 Script executed:
cat -n ./libvirt/tests/src/guest_os_booting/os_configuration/without_default_os_attributes.py | head -80Repository: autotest/tp-libvirt
Length of output: 2990
🏁 Script executed:
# Check what parameters are typically passed to these test functions
grep -r "arch\|machine_type\|shortname" ./libvirt/tests/src/guest_os_booting/boot_order/ | head -30Repository: autotest/tp-libvirt
Length of output: 1935
🏁 Script executed:
# Look for how params are used in the guest_os_booting_base module
find . -name "guest_os_booting_base*" -type fRepository: autotest/tp-libvirt
Length of output: 116
🏁 Script executed:
# Check if there are other files in the boot_order directory that might use similar logic
ls -la ./libvirt/tests/src/guest_os_booting/boot_order/Repository: autotest/tp-libvirt
Length of output: 810
🏁 Script executed:
cat -n ./provider/guest_os_booting/guest_os_booting_base.py | head -100Repository: autotest/tp-libvirt
Length of output: 3667
🏁 Script executed:
# Also check if there are any test configuration files that show how params work
find ./libvirt/tests -name "*.py" -type f -path "*guest_os_booting*" | xargs grep -l "arch\|machine_type" | head -5Repository: autotest/tp-libvirt
Length of output: 437
🏁 Script executed:
# Let's also look for how params typically contains the guest arch info
grep -r "params.get.*arch\|params.get.*machine" ./libvirt/tests/src/guest_os_booting/ | head -10Repository: autotest/tp-libvirt
Length of output: 509
Use guest architecture from params, not platform.machine().
The code currently uses host architecture for both image URL selection (lines 46-51) and login timeout (lines 152-153), which fails when the host and guest architectures differ. For example, on an x86_64 host running an aarch64 guest, the x86-only generic image would be downloaded first and cause boot/login failures.
The same test directory already has a working solution: _guest_is_aarch64_arm64() in without_default_os_attributes.py demonstrates the correct pattern using params.get("arch"), params.get("vm_arch_name"), params.get("machine_type"), and params.get("shortname"). Reuse that approach here to properly detect guest architecture.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 49-49: Consider [*aarch64_variants, disk1_img_url, ovmf_url] instead of concatenation
Replace with [*aarch64_variants, disk1_img_url, ovmf_url]
(RUF005)
[warning] 51-51: Consider [ovmf_url, disk1_img_url, *aarch64_variants] instead of concatenation
Replace with [ovmf_url, disk1_img_url, *aarch64_variants]
(RUF005)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/guest_os_booting/boot_order/boot_from_disk_device.py`
around lines 46 - 51, Replace uses of platform.machine() in this test with the
guest-arch parameters like params.get("arch"), params.get("vm_arch_name"),
params.get("machine_type") or params.get("shortname") (same pattern as
_guest_is_aarch64_arm64() in without_default_os_attributes.py); specifically,
change the image URL selection block (currently branching on platform.machine())
to detect the guest architecture from params and prefer aarch64 variants only
when the guest is aarch64, and also update the login timeout logic that uses
platform.machine() (around the later login timeout lines) to use the same
params-based guest-arch check so timeouts and image ordering reflect the guest,
not the host.
| os_xpath = _adapt_os_xpath_uefi_disk_boot( | ||
| eval(params.get("os_xpath")), params, host_arch) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the file
fd -t f "without_default_os_attributes.py" libvirt/tests/src/Repository: autotest/tp-libvirt
Length of output: 148
🏁 Script executed:
# Once found, look at lines around 80-81 with context
cat -n libvirt/tests/src/guest_os_booting/os_configuration/without_default_os_attributes.py | sed -n '70,95p'Repository: autotest/tp-libvirt
Length of output: 1101
🏁 Script executed:
# Look for the _adapt_os_xpath_uefi_disk_boot function to understand what it expects
rg -n "_adapt_os_xpath_uefi_disk_boot" libvirt/tests/src/Repository: autotest/tp-libvirt
Length of output: 351
🏁 Script executed:
# Look at the _adapt_os_xpath_uefi_disk_boot function definition
cat -n libvirt/tests/src/guest_os_booting/os_configuration/without_default_os_attributes.py | sed -n '41,70p'Repository: autotest/tp-libvirt
Length of output: 1126
🏁 Script executed:
# Check imports at the top of the file
cat -n libvirt/tests/src/guest_os_booting/os_configuration/without_default_os_attributes.py | head -20Repository: autotest/tp-libvirt
Length of output: 712
🏁 Script executed:
# Look for test parameters/config to see what os_xpath values look like
rg -n "os_xpath" libvirt/tests/src/guest_os_booting/os_configuration/ -A 2 -B 2Repository: autotest/tp-libvirt
Length of output: 3734
Replace eval() with ast.literal_eval() to safely parse os_xpath parameter.
The current code executes arbitrary Python from configuration data, which is a security risk. Since _adapt_os_xpath_uefi_disk_boot() only processes literal Python values (strings, dicts, lists, tuples), ast.literal_eval() is the appropriate safe alternative.
Proposed fix
+import ast
import platform
import re
@@
- os_xpath = _adapt_os_xpath_uefi_disk_boot(
- eval(params.get("os_xpath")), params, host_arch)
+ os_xpath = _adapt_os_xpath_uefi_disk_boot(
+ ast.literal_eval(params.get("os_xpath")), params, host_arch)📝 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.
| os_xpath = _adapt_os_xpath_uefi_disk_boot( | |
| eval(params.get("os_xpath")), params, host_arch) | |
| import ast | |
| import platform | |
| import re | |
| ... | |
| os_xpath = _adapt_os_xpath_uefi_disk_boot( | |
| ast.literal_eval(params.get("os_xpath")), params, host_arch) |
🧰 Tools
🪛 Ruff (0.15.12)
[error] 81-81: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@libvirt/tests/src/guest_os_booting/os_configuration/without_default_os_attributes.py`
around lines 80 - 81, Replace the insecure eval() call used to parse
params.get("os_xpath") with ast.literal_eval() to safely evaluate only Python
literal structures; update the os_xpath assignment passed to
_adapt_os_xpath_uefi_disk_boot by calling
ast.literal_eval(params.get("os_xpath")) and add/import the ast module at the
top of the file if missing, keeping the rest of the call (params, host_arch)
unchanged.
| for alt in ( | ||
| os.path.join(data_dir.get_data_dir(), "images", os.path.basename(custom_loader_path)), | ||
| os.path.join(tempfile.gettempdir(), os.path.basename(custom_loader_path)), | ||
| ): | ||
| parent = os.path.dirname(alt) | ||
| try: | ||
| os.makedirs(parent, exist_ok=True) | ||
| except OSError: | ||
| continue | ||
| if os.access(parent, os.W_OK): | ||
| test.log.info( | ||
| "custom_loader_path %r not usable (dir=%r); using %s instead", | ||
| custom_loader_path, | ||
| dest_dir, | ||
| alt, | ||
| ) | ||
| loader_dict = _replace_path_in_obj(loader_dict, custom_loader_path, alt) | ||
| return alt, loader_dict |
There was a problem hiding this comment.
Make the fallback loader path unique per test run.
These two candidates use shared filenames under images/ and /tmp/. Parallel jobs with the same custom_loader_path basename can overwrite each other here, and the unconditional cleanup later can delete another test's copied loader. Please switch this fallback to a per-run temp file or temp directory before rewriting loader_dict.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/guest_os_booting/ovmf_firmware/ovmf_loader.py` around lines
52 - 69, The fallback destinations for custom_loader_path use shared basenames
causing race/overwrite; change the branch that builds the alt candidates to
create a unique per-run path (e.g. use tempfile.mkdtemp() to make a unique temp
directory or tempfile.NamedTemporaryFile(delete=False) / tempfile.mkstemp() to
create a unique temp file) and join the basename into that unique path before
calling _replace_path_in_obj and returning; update the alt variable used in the
os.access check and loader_dict replacement so the copy/cleanup uses this unique
per-test path (referencing alt, custom_loader_path, loader_dict, and
_replace_path_in_obj).
|
Closing this PR due to current team constraints. This is part of a broader effort to triage all in-flight work across our upstream repos. If this work is still needed, please feel free to reopen and it will be picked up. Apologies for any inconvenience. |
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements