You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "shwstppr (via GitHub)" <gi...@apache.org> on 2025/01/20 10:45:59 UTC
[PR] kvm: fix disk controller for secure boot vm [cloudstack]
shwstppr opened a new pull request, #10213:
URL: https://github.com/apache/cloudstack/pull/10213
### Description
Fixes #9460
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] build/CI
- [ ] test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2872387115
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#discussion_r2366199705
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -3161,18 +3196,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
disk.setDiscard(DiscardType.UNMAP);
}
} else {
- if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
- } else {
- if (isSecureBoot) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
- } else {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
- }
- }
-
+ defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
Review Comment:
@pavanaravapalli probably not for all guest OSes, but should work for newer ones, as #9460 reports VM works fine with virtio. Also, as this is optional, it is up to the operator to check and use
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2602181550
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/10213?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 15.85%. Comparing base [(`9967bb3`)](https://app.codecov.io/gh/apache/cloudstack/commit/9967bb3fe899f82ce5fc83da5eba0d278a2df70a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`edd537f`)](https://app.codecov.io/gh/apache/cloudstack/commit/edd537f8f53954ca269fe4f4512ec229825cace8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files with missing lines](https://app.codecov.io/gh/apache/cloudstack/pull/10213?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ervisor/kvm/resource/LibvirtComputingResource.java](https://app.codecov.io/gh/apache/cloudstack/pull/10213?src=pr&el=tree&filepath=plugins%2Fhypervisors%2Fkvm%2Fsrc%2Fmain%2Fjava%2Fcom%2Fcloud%2Fhypervisor%2Fkvm%2Fresource%2FLibvirtComputingResource.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9oeXBlcnZpc29ycy9rdm0vc3JjL21haW4vamF2YS9jb20vY2xvdWQvaHlwZXJ2aXNvci9rdm0vcmVzb3VyY2UvTGlidmlydENvbXB1dGluZ1Jlc291cmNlLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/10213?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.19 #10213 +/- ##
============================================
+ Coverage 15.13% 15.85% +0.71%
+ Complexity 11274 11270 -4
============================================
Files 5408 5042 -366
Lines 473974 444402 -29572
Branches 57813 52632 -5181
============================================
- Hits 71730 70440 -1290
+ Misses 394227 366092 -28135
+ Partials 8017 7870 -147
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/10213/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/10213/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/10213/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.85% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/10213?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3301213096
<b>[SF] Trillian test result (tid-14360)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 47868 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10213-t14360-kvm-ol8.zip
Smoke tests completed. 133 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3336825379
I'm closing this for now, as we don't have a consensus and a proper solution
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3298877906
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2827003383
Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 13161
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3263484995
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "pavanaravapalli (via GitHub)" <gi...@apache.org>.
pavanaravapalli commented on code in PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#discussion_r2362170524
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -3161,18 +3196,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
disk.setDiscard(DiscardType.UNMAP);
}
} else {
- if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
- } else {
- if (isSecureBoot) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
- } else {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
- }
- }
-
+ defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
Review Comment:
@shwstppr If a user forces the disk type with this template flag, will Secure Boot still work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3218120055
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "pavanaravapalli (via GitHub)" <gi...@apache.org>.
pavanaravapalli commented on code in PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#discussion_r1922711472
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -3149,11 +3149,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
} else {
- if (isSecureBoot) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
- } else {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
- }
+ disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
Review Comment:
@shwstppr
When the uefi functionality implemented there is a issue with (Windows Guest VM + Secure Boot )Root Disk controller for boot disk using virtio, hence it's been set for windows guest vm -> sata, Linux guest vm -> virtio as disk controller.
these code changes will break the uefi functionality for windows guest vm with secure boot.
right now I don't have setup to test the functionality, it will be good If you can test the scenario and confirm that this won't break windows guest vm along with secure boot use-case .
And this behavior always over-rides the os configuration setting, no matter what controller configured in the OS details. Which needs to be addressed if the above use-case is not broken. Other wise we need to find a mid-way to fix both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2602082075
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2827024528
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2827241355
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13164
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2872384241
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2602214492
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12127
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#discussion_r1922333323
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -3149,11 +3149,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
} else {
- if (isSecureBoot) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
- } else {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
- }
+ disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
Review Comment:
for DATADISK, should it use `diskBusTypeData` instead ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "pavanaravapalli (via GitHub)" <gi...@apache.org>.
pavanaravapalli commented on code in PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#discussion_r1922711472
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -3149,11 +3149,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
} else {
- if (isSecureBoot) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
- } else {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
- }
+ disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
Review Comment:
@shwstppr
When the uefi functionality implemented there is a issue with (Windows Guest VM + Secure Boot ) Disk controller for disk using virtio, hence it's been set for windows guest vm -> sata, Linux guest vm -> virtio as disk controller.
these code changes will break the uefi functionality for windows guest vm with secure boot.
right now I don't have setup to test the functionality, it will be good If you can test the scenario and confirm that this won't break windows guest vm along with secure boot use-case .
And this behavior always over-rides the os configuration setting, no matter what controller configured in the OS details. Which needs to be addressed if the above use-case is not broken. Other wise we need to find a mid-way to fix both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2872642961
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13345
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2826781577
@pavanaravapalli @weizhouapache cc @rajujith @vladimirpetrov I've made the behaviour configurable using VM (or template) detail. User/operator can add a VM/template detail - win.skip.force.disk.controller = true to make KVM plugin not force SATA controller for Windows VM with secured boot
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3263524706
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 14884
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2602077290
This would fix the issue but not sure if could cause the issue of disks not being identified when virtio drivers are not installed. Maybe we can add some documentation. ping @pavanaravapalli, if you've any advise
@blurorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3218119533
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3310930092
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr closed pull request #10213: kvm: fix disk controller for secure boot vm
URL: https://github.com/apache/cloudstack/pull/10213
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2827018374
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2826731728
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3298874398
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "pavanaravapalli (via GitHub)" <gi...@apache.org>.
pavanaravapalli commented on code in PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#discussion_r1922711472
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -3149,11 +3149,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
} else {
- if (isSecureBoot) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
- } else {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
- }
+ disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
Review Comment:
@shwstppr
When the uefi functionality implemented there is a issue with (Windows Guest VM + Secure Boot ) Disk controller for boot disk using virtio, hence it's been set for windows guest vm -> sata, Linux guest vm -> virtio as disk controller.
these code changes will break the uefi functionality for windows guest vm with secure boot.
right now I don't have setup to test the functionality, it will be good If you can test the scenario and confirm that this won't break windows guest vm along with secure boot use-case .
And this behavior always over-rides the os configuration setting, no matter what controller configured in the OS details. Which needs to be addressed if the above use-case is not broken. Other wise we need to find a mid-way to fix both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#discussion_r2368873264
##########
api/src/main/java/com/cloud/vm/VmDetailConstants.java:
##########
@@ -53,6 +53,9 @@ public interface VmDetailConstants {
String NIC_MULTIQUEUE_NUMBER = "nic.multiqueue.number";
String NIC_PACKED_VIRTQUEUES_ENABLED = "nic.packed.virtqueues.enabled";
+ // KVM specific, disk controllers
+ String KVM_WIN_SKIP_FORCE_DISK_CONTROLLER = "win.skip.force.disk.controller";
Review Comment:
In my opinion, the root disk and data disk controller should not be changed when vm is stopped and started, in regardless of the vm settings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-2826786153
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on code in PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#discussion_r2387493660
##########
api/src/main/java/com/cloud/vm/VmDetailConstants.java:
##########
@@ -53,6 +53,9 @@ public interface VmDetailConstants {
String NIC_MULTIQUEUE_NUMBER = "nic.multiqueue.number";
String NIC_PACKED_VIRTQUEUES_ENABLED = "nic.packed.virtqueues.enabled";
+ // KVM specific, disk controllers
+ String KVM_WIN_SKIP_FORCE_DISK_CONTROLLER = "win.skip.force.disk.controller";
Review Comment:
@weizhouapache In my test they are not really changed. Even at the deployment time, Windows VM with secure boot was always getting `sata` while other (Linux, etc) VMs were getting `virtio`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3346259107
Recreated here, https://github.com/apache/cloudstack/pull/11750
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "pavanaravapalli (via GitHub)" <gi...@apache.org>.
pavanaravapalli commented on code in PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#discussion_r2371640547
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -3161,18 +3196,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
disk.setDiscard(DiscardType.UNMAP);
}
} else {
- if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
- } else {
- if (isSecureBoot) {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
- } else {
- disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
- }
- }
-
+ defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
Review Comment:
@shwstppr Thanks for the response. We should mention potential UEFI workflow issues in the flag description since guest OS support varies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3263484094
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] kvm: fix disk controller for secure boot vm [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10213:
URL: https://github.com/apache/cloudstack/pull/10213#issuecomment-3218166407
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14717
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org