Set firmware attribute in libvirt domain for aarch64 #214
Conversation
For aarch64, we need to specify the firmware to be efi so the loader and the nvram is correctly populated with the appropriate files for machine creation. This actually started in openshift#213 similar to what was done in the terraform provider, but the newer libvirt-go-xml libraries support setting of this attribute making things easier
openshift-ci-robot
added
the
do-not-merge/work-in-progress
| // reference: https://libvirt.org/formatdomain.html#bios-bootloader | ||
| if arch == "aarch64" { | ||
| domainDef.OS.Firmware = "efi" | ||
| } |
There was a problem hiding this comment.
I am ok with hardcoding this for aarch64, but do we want to expose this through the machine provider api so it can be set through the installer and this code can be arch agnostic?
There was a problem hiding this comment.
ok..on second thought .. probably not...this is a basic property of aarch64...better to leave it as is and keep things simple
There was a problem hiding this comment.
It must not be so basic, otherwise it would be the libvirt default ;) I agree with leaving this as is for now, as I don't think cluster-api-provider-libvirt should support arbitrary VM configurations. This is one case where it should be able to pick up the correct value needed by openshift. For x86_64, the efi switch would happen when we start using the q35 machine type.
Prashanth684
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
| github.com/libvirt/libvirt-go-xml v4.6.0+incompatible h1:dL2JDzkEtsiZkPh42KHvLt208wgNfO40IsBjHtaXohs= | ||
| github.com/libvirt/libvirt-go-xml v4.6.0+incompatible/go.mod h1:oBlgD3xOA01ihiK5stbhFzvieyW+jVS6kbbsMVF623A= | ||
| github.com/libvirt/libvirt-go v5.10.0+incompatible h1:01fwkdUHH2hk4YyFNCr48OvSGqXYLzp9cofUpeyeLNc= | ||
| github.com/libvirt/libvirt-go v5.10.0+incompatible/go.mod h1:34zsnB4iGeOv7Byj6qotuW8Ya4v4Tr43ttjz/F0wjLE= |
There was a problem hiding this comment.
I don't think it's required to bump libvirt-go, only libvirt-go-xml should be needed? If you bump the libvirt-go dependency, this can cause unexpected failures to run the installer (if you build on a system where libvirt 5.10 is installed and you then try to run it on a system with libvirt 4.6 installed). I'd prefer to keep the libvirt-go dependency unchanged if possible.
There was a problem hiding this comment.
i changed libvirt-go and libvirt-go-xml to match that of the openshift installer. I thought it would be good to keep both in sync? in any case the installer uses the same versions so should be fine ?
There was a problem hiding this comment.
I think the installer upgraded to a newer version indirectly (it updated another module which needed a newer version). This caused issues fwiw openshift/installer#4339
Since we already bit the bullet with the installer, we can try changing it here as well :)
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest |
4 participants
For aarch64, we need to specify the firmware to be efi so the loader and the nvram is correctly
populated with the appropriate files for machine creation.
This actually started in #213 similar to what was done in the terraform provider, but the newer
libvirt-go-xml libraries support setting of this attribute making things easier