-
Notifications
You must be signed in to change notification settings - Fork 1.3k
UEFI Support on CloudStack #3638
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
Conversation
...ns/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
Outdated
Show resolved
Hide resolved
|
@pavanaravapalli first off glad to see this PR! it doesn't seem to compile just now, I marked in the review where it's complaining. Here is a log of |
nvazquez
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 @pavanaravapalli. I left some code review comments. Can you also add documentation? For example for KVM it seems to need some properties set in order to work properly
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java
Outdated
Show resolved
Hide resolved
...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
Outdated
Show resolved
Hide resolved
...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
Outdated
Show resolved
Hide resolved
6f3c77d to
dd98b3b
Compare
|
Hi @nathanejohnson are you planning to test this PR? |
|
@nvazquez I'm going to test on kvm. would be good to get a volunteer for vmware as well |
|
@nathanejohnson sure, I can manually test it on Vmware once the PR is complete |
|
@nvazquez I am working on it. I will address your review comments given in the PR#3643. I have created another PR#3643 for the same branch due to some forking issue. @nathanejohnson @nvazquez |
|
@pavanaravapalli you do not need to create new pull request. |
|
Since PR#3643's forked branch been obsoleted, re-opening this PR. |
f69ee48 to
2f8300d
Compare
Updated Description with KVM related uefi.properties information. |
|
@nvazquez @nathanejohnson |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖centos6 ✖centos7 ✖debian. JID-435 |
...on/src/main/java/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntityImpl.java
Outdated
Show resolved
Hide resolved
9758867 to
59bf8d3
Compare
59bf8d3 to
ac94684
Compare
|
@sureshanaparti, review comments addressed as suggested |
sureshanaparti
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.
Checked the changes for UEFI support. Code changes LGTM.
server/src/main/java/com/cloud/agent/manager/allocator/impl/FirstFitAllocator.java
Show resolved
Hide resolved
server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/cloud/server/ManagementServerImpl.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java
Outdated
Show resolved
Hide resolved
Ok. |
|
Trillian test result (tid-1241)
|
|
3 x Approvals, regression tests went fine. Merging |
| } | ||
|
|
||
| final VirtualMachineProfileImpl vmProfile = new VirtualMachineProfileImpl(vm, template, offering, owner, params); | ||
| s_logger.info(" Uefi params " + "UefiFlag: " + params.get(VirtualMachineProfile.Param.UefiFlag) |
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.
Could this potentially cause any exception? Should this be only logged when custom params are passed; otherwise it may always log as null/null confuse admin/users cc @pavanaravapalli @DaanHoogland @andrijapanicsb
| } | ||
| } | ||
| } | ||
| if(getBootType() != null){ // export to get |
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.
@pavanaravapalli lint issue - add space between if (...
| } | ||
| } | ||
| if(getBootType() != null){ // export to get | ||
| if(getBootType() == ApiConstants.BootType.UEFI) { |
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.
@pavanaravapalli cc @DaanHoogland @andrijapanicsb
There is no null check, write it as someEnum.equals(someVariable or Method()) - this will never throw NPE.
| } | ||
| if(getBootType() != null){ // export to get | ||
| if(getBootType() == ApiConstants.BootType.UEFI) { | ||
| customparameterMap.put(getBootType().toString(), getBootMode().toString()); |
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.
@pavanaravapalli cc @DaanHoogland @andrijapanicsb
What is one of the methods return null?
| detailsCopy.remove("username"); | ||
| detailsCopy.remove("password"); | ||
|
|
||
| if(detailsCopy.containsKey(Host.HOST_UEFI_ENABLE)) { |
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.
@pavanaravapalli space between if (...
| vmProfile.setServiceOffering(_serviceOfferingDao.findByIdIncludingRemoved(vm.getId(), vm.getServiceOfferingId())); | ||
| if (MapUtils.isNotEmpty(vmEntityVO.getDetails()) && | ||
| vmEntityVO.getDetails().containsKey(VirtualMachineProfile.Param.UefiFlag.getName()) && | ||
| "yes".equalsIgnoreCase(vmEntityVO.getDetails().get(VirtualMachineProfile.Param.UefiFlag.getName()))) |
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.
@pavanaravapalli
Can you make this simpler - just put in the key in the details only when it's applicable, then you don't need to compare the uefi value at all.
| guest.setBootType(GuestDef.BootType.UEFI); | ||
| guest.setBootMode(GuestDef.BootMode.LEGACY); | ||
| if (StringUtils.isNotBlank(customParams.get(GuestDef.BootType.UEFI.toString())) && "secure".equalsIgnoreCase(customParams.get(GuestDef.BootType.UEFI.toString()))) { | ||
| guest.setMachineType("q35"); |
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.
@pavanaravapalli cc @DaanHoogland @andrijapanicsb Unless there is a requirement of allowing customisation of UEFI (generally people use whatever libvirt gives), you can simplify this by addressing the change in the libvirt xml. See my changes around support EFI for arm64/raspberrypi (for example https://github.com/apache/cloudstack/pull/3644/files#diff-042ba018afca0d690feae46e22f73301R128)
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.
@rhtyd
UEFI Secure boot does not work with default libvirt xml changes and "pc" as machine type.
There are few other mandatory changes to be handled in case of secure boot guest xml, the same have been captured in the design doc. please have a look in the wiki doc shared with this PR description.
|
@pavanaravapalli please have a look at @rhtyd 's concerns and at #3978 as this PR seems to cause issue. We'll need to fix before release or revert! |
|
cc @nathanejohnson @kiwiflyer , hate to escalate but please see my prior comment. |
@DaanHoogland |
|
@pavanaravapalli could est. when would you be able to do this, preferably before 4.14 RC1? |
@rhtyd it may take time to address all the review comments provided. I am hoping to give fix for NPE #3978 soon. And i need some one to test it as i don't have VMware setup with me present. |
|
@pavanaravapalli I can take care of the test required to validate that #3978 is solved. Please add your update A.S.A.P. |
|
@pavanaravapalli please add your improvements and suggestions as PR to #3983. |
Raised PR #3985 for the fix . @DaanHoogland please verify the fix and approve it. |
|
@pavanaravapalli please review #4088 we need to fix this to be able to release. |
|
@pavanaravapalli Can you update the actual UEFI functionality (with boot mode & type) supported for VMware and KVM, in CloudStack, with this PR changes. |
Updated Description with info. |
Verified code for #4089 , looks good to me. I have not encountered this issue while testing UEFI changes. |
…ypervisor KVM,VMware. enabled boot modes [Legacy,Secure] support for UEFI boot with known caveats. (apache#3638)" This reverts commit d4b537e. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Description
To Enable UEFI Support for Guest VM's deployed on Hypervisors[ KVM, VMware] in Cloud Stack.
Known things.
Refer below doc for more information.
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Enable+UEFI+booting+for+Instance
Types of changes
Screenshots (if appropriate):
KVM Host configuration
How Has This Been Tested?
KVM
template and verified the UEFI boot information.
Note: brctl-utils package is deprecated in RHEL 8. We have force installed epel7 rpm in RHEL 8 and configured KVM agent.
VM Booting with UEFI firmware
Windows Server 2016 VM Secure boot enabled

Windows Server 2016 VM Secure boot disabled

Cent OS 7 Linux : Secure boot enabled

VMware
enabled.
template and verified the UEFI boot information.
Windows Server 2106 VM Secure boot disabled

Windows Server 2106 VM Secure boot enabled

UEFI Enabled Host
Guest VM Uefi Boot Details