-
Notifications
You must be signed in to change notification settings - Fork 128
✨ Add ARM suport to VM template #1607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Honza Pokorny <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds ARM (aarch64) architecture support to the VM template system, enabling multi-architecture deployments. The changes introduce a new architecture field to VM flavors and allow cross-architecture VM emulation.
Key changes:
- Add
architecturefield to flavors (defaults tolibvirt_arch) - Implement ARM-specific VM configuration including EFI firmware, CPU model, and video driver
- Enable custom VM template override via
baremetal_vm_templatevariable
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
vm-setup/roles/libvirt/templates/baremetalvm.xml.j2 |
Adds ARM-specific VM configuration with conditional logic for OS setup, CPU model, and device settings based on flavor architecture |
vm-setup/roles/libvirt/tasks/vm_setup_tasks.yml |
Allows consumers to specify a custom VM template via baremetal_vm_template variable |
vm-setup/roles/common/templates/ironic_nodes.json.j2 |
Updates Ironic node properties to use flavor-specific architecture instead of global libvirt_arch |
vm-setup/roles/common/defaults/main.yml |
Adds architecture field to default flavor configuration with libvirt_arch as the default value |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kashifest
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm
Rozzii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution.
Generally looks fine but I am a bit confused related to EFI+ARM.
I have commented the at the relevant section.
|
|
||
| {% if libvirt_arch != 'aarch64' %} | ||
| {% if is_arm %} | ||
| <os firmware='efi'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK EFI + ARM are not a mandatory combination like on x86, many ARM systems and RISC systems (actually all that I have worked with) have no EFI firmware or ACPI or anything akin to the "BIOS/UEFI" that folks are used to on x86/amd64 motherboards.
That said I never emulated arm on libvirt so I am not familiar with how this section should look like, but I would suggest separating is_arm from is_arm_efi or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude came up with checking libvirt_firmware, what do you think?
Assisted-by: claude-sonnet-4.5 Signed-off-by: Honza Pokorny <[email protected]>
Assisted-by: claude-sonnet-4.5 Signed-off-by: Honza Pokorny <[email protected]>
A few improvements in the multi-arch space:
architecturefield to flavors (defaults to libvirt arch)