-
Notifications
You must be signed in to change notification settings - Fork 1.2k
kvm: allow skip forcing disk controller #11750
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: 4.19
Are you sure you want to change the base?
kvm: allow skip forcing disk controller #11750
Conversation
Fixes apache#9460 A VM or template detail can be added with key `skip.force.disk.controller` and value `true` to allow skipping forcing disk controllers for the VM especially in case of UEFI VMs. Otherwise, current behaviour of disk controllers depend on the guest OS, UEFI secure boot where Windows VM may always be provisioned with `sata` and other OS VMs with `virtio`. Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 15217 |
@blueorangutan package |
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 15219 |
@blueorangutan package |
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 functionality to allow skipping disk controller forcing for KVM VMs by introducing a new VM detail flag. The enhancement improves flexibility for UEFI VMs that may have specific disk controller requirements.
Key changes:
- Introduces a new VM detail constant
skip.force.disk.controller
to bypass default disk controller logic - Extracts disk definition logic into a separate method for better testability and maintainability
- Adds comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
VmDetailConstants.java | Adds new constant for the skip force disk controller feature |
LibvirtComputingResource.java | Refactors disk definition logic and implements the skip functionality |
LibvirtComputingResourceTest.java | Adds test coverage for the new disk definition method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #11750 +/- ##
=========================================
Coverage 15.18% 15.18%
- Complexity 11370 11373 +3
=========================================
Files 5415 5415
Lines 476082 476091 +9
Branches 58129 58131 +2
=========================================
+ Hits 72288 72304 +16
+ Misses 395702 395694 -8
- Partials 8092 8093 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
clgtm
@blueorangutan package |
@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15246 |
@blueorangutan test |
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
[SF] Trillian test result (tid-14511)
|
Description
Fixes #9460
A VM or template detail can be added with key
skip.force.disk.controller
and valuetrue
to allow skipping forcing disk controllers for the VM especially in case of UEFI VMs.Otherwise, current behaviour of disk controllers depend on the guest OS, UEFI secure boot where Windows VM may always be provisioned with
sata
and other OS VMs withvirtio
.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?