search

dmacvicar / terraform-provider-libvirt

Terraform provider to provision infrastructure with Linux's KVM using libvirt
Apache License 2.0
1.59k stars 458 forks source link

Set firmware attribute to efi for aarch64 #816

Closed Prashanth684 closed 3 years ago

Prashanth684 commented 3 years ago

This new firmare attribute was added to newer versions of libvirt-go-xml. This enables to specify the firmware as 'bios' or 'efi' and automatically fills in the loader and NVRAM elements without having to specify them manually. Refer to https://libvirt.org/formatdomain.html#bios-bootloader for this. As part of this change, libvirt-go and libvirt-go-xml needs to be updated. The old functionality of specifying the loader and NVRAM is also left as such so those can continue to be specified if we do not want automatic selection

Setting this automatically for aarch64 as it is a EFI architecture by default.

Please make sure you read the contributor documentation before opening a Pull Request.

Prashanth684 commented 3 years ago

would be nice to expose this as a terraform variable..but there is already a firmware variable which actually refers to the firmware file path for the loader. should that be changed to some other name ?

dmacvicar commented 3 years ago

Thanks for the PR!

The issue you describe is common. We try to expose an attribute in HCL without exposing the whole XML sub-block. In this case https://github.com/dmacvicar/terraform-provider-libvirt/pull/50 choice of name firmware was not good.

I am not convinced where to put this. Pehaps the default domain definition should handle this already? ie. set it to EFI for ARM when returning the initial empty definition?

Prashanth684 commented 3 years ago

Thanks for the PR!

The issue you describe is common. We try to expose an attribute in HCL without exposing the whole XML sub-block. In this case #50 choice of name firmware was not good.

I am not convinced where to put this. Pehaps the default domain definition should handle this already? ie. set it to EFI for ARM when returning the initial empty definition?

Thanks for the feedback. That makes sense. I added it to the default definition but had to pass in arch as a parameter to set it for ARM. just used "x86_64" in places where it was just used for go tests.

Prashanth684 commented 3 years ago

Thanks for the PR! The issue you describe is common. We try to expose an attribute in HCL without exposing the whole XML sub-block. In this case #50 choice of name firmware was not good. I am not convinced where to put this. Pehaps the default domain definition should handle this already? ie. set it to EFI for ARM when returning the initial empty definition?

Thanks for the feedback. That makes sense. I added it to the default definition but had to pass in arch as a parameter to set it for ARM. just used "x86_64" in places where it was just used for go tests.

@dmacvicar - does this change look ok ?

MalloZup commented 3 years ago

thx for PR ! I have added a comment for discussion.

Prashanth684 commented 3 years ago

updated PR per comments.

Prashanth684 commented 3 years ago

I really like where the code is right now, but as I mentioned newDomainDefForConnection is executed on Read which happens on an explicit Read or at the end of Create, which means after the XML is injected into libvirt.

Are you sure the code working and producing the desired effect?

newDomainDefForConnection is called in at the beginning of create also: https://github.com/dmacvicar/terraform-provider-libvirt/blob/master/libvirt/resource_libvirt_domain.go#L443. Yes, i tested and it works

dmacvicar commented 3 years ago

Thanks for the contribution @Prashanth684

Prashanth684 commented 3 years ago

Thanks for the contribution @Prashanth684

Thank you for helping to get this in!