You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/03/09 09:25:59 UTC

[GitHub] [cloudstack] slavkap opened a new pull request #4773: Fix deploy VM from ISOs with UEFI

slavkap opened a new pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773


   
   ### Description
   
   This PR fixes #4244 
   deploying of VMs from ISOs and from templates with UEFI boot type
   deploying of VMs from ISOs and from templates with UEFI boot type with
   volumes in RAW format
   
   ### Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [X ] 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)
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [ ] Critical
   - [ X] Major
   - [ ] Minor
   - [ ] Trivial
   
   ### How Has This Been Tested?
   manual tests with CentOS8 and Windows ISOs/templates
   primary storages - NFS/StorPool
   
   I'll appreciate it if somebody could test this with Ceph and another storage plugin that supports RAW image format 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-805546450


   @rhtyd a Jenkins job has been kicked to build packages. 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] shwstppr commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808202058


   @slavkap Trillian which reported the test results above has its own copy of test_data.py which will need updating.
   I've updated the test_data.py there and started a test run from the backend. Let's see how that goes.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r591286440



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2564,16 +2565,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
             final DiskDef disk = new DiskDef();
             int devId = volume.getDiskSeq().intValue();
             if (volume.getType() == Volume.Type.ISO) {
-                if (volPath == null) {
-                    if (isSecureBoot) {
-                        disk.defISODisk(null, devId,isSecureBoot,isWindowsTemplate);
-                    } else {
-                        /* Add iso as placeholder */
-                        disk.defISODisk(null, devId);
-                    }
-                } else {
-                    disk.defISODisk(volPath, devId);
-                }
+
+                disk.defISODisk(volPath, devId, isUefiEnabled);

Review comment:
       third param here is UEFI secure mode or not, please check.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-810049006


   @rhtyd, isn't it easier to list the hosts and check if one of them has "ueficapability" enabled? I'm not sure when I connect to the host for what to look:
   1 - to check that OVMF is installed `rpm -qi OVMF` (this information won't tell us if the agent is set to support UEFI)
   2 - to check if `uefi.properties` file exists
   3 - or to look for something else


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-811695093


   @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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-803808881


   @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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-796585053


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 84


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-810034686


   @slavkap can you add checks in the code to skip the test if the host is not UEFI enabled (you can use ssh client to check on the host)?


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-803809333


   @rhtyd a Jenkins job has been kicked to build packages. 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-804802506


   > I'll have to check. It is a standard centos7 install. If it isn't we will have to update it and rely on your manual tests anyway. You can put it in a separate PR, if you wish.
   
   Yes, it's standard CentOS7 with OVMF installed. I've added in `/etc/cloudstack/agent/uefi.properties` these information:
   
   > guest.nvram.template.secure=/usr/share/OVMF/OVMF_VARS.secboot.fd
   > guest.nvram.template.legacy=/usr/share/OVMF/OVMF_VARS.fd
   > guest.loader.secure=/usr/share/OVMF/OVMF_CODE.secboot.fd
   > guest.nvram.path=/var/lib/libvirt/qemu/nvram/
   > guest.loader.legacy=/usr/share/OVMF/OVMF_CODE.secboot.fd
   
   and into ` /etc/libvirt/qemu.conf`:
   
   > nvram = [ "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd" ]
   
   Also, in the `host_details` table, if it's not a new host, the detail `host.uefi.enable` of the host should be set to true.
   
   I think that it'll be better to add the tests in this PR.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-793980879


   Packaging result: :heavy_multiplication_x: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 58


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-805566253


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 219


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r607614866



##########
File path: tools/marvin/marvin/config/test_data.py
##########
@@ -928,6 +928,16 @@
         "ostype": "CentOS 5.6 (64-bit)",
         "mode": 'HTTP_DOWNLOAD',
     },
+    "iso3": {
+        "displaytext": "Test ISO 3",
+        "name": "ISO 3",
+        "url": "http://people.apache.org/~tsp/dummy.iso",

Review comment:
       @rhtyd, I could add a valid URL, but what is the reason? I've followed the rest of Marvin's test, and no one is using a valid ISO, only dummies




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-797316884


   In your opinion @slavkap, can we automate those tests in a marvin script, using a smaller linux iso?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] shwstppr commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808660438


   @slavkap Looks like Trillian hosts are not having UEFI capability. Do we need to skip UEFI tests in this case?
   
   ```
   2021-03-26 14:08:42,630 DEBUG [c.c.a.m.a.i.FirstFitAllocator] (API-Job-Executor-37:ctx-56e8e114 job-311 ctx-38d5fd62 FirstFitRoutingAllocator) (logid:b61c10e9) Looking for hosts in dc: 1  pod:1  cluster:1
   2021-03-26 14:08:42,631 DEBUG [c.c.a.m.a.i.FirstFitAllocator] (API-Job-Executor-37:ctx-56e8e114 job-311 ctx-38d5fd62 FirstFitRoutingAllocator) (logid:b61c10e9) Hosts with tag 'UEFI' are:[]
   2021-03-26 14:08:42,631 DEBUG [c.c.a.m.a.i.FirstFitAllocator] (API-Job-Executor-37:ctx-56e8e114 job-311 ctx-38d5fd62 FirstFitRoutingAllocator) (logid:b61c10e9) FirstFitAllocator has 0 hosts to check for allocation: []
   2021-03-26 14:08:42,633 DEBUG [c.c.a.m.a.i.FirstFitAllocator] (API-Job-Executor-37:ctx-56e8e114 job-311 ctx-38d5fd62 FirstFitRoutingAllocator) (logid:b61c10e9) Found 0 hosts for allocation after prioritization: []
   2021-03-26 14:08:42,633 DEBUG [c.c.a.m.a.i.FirstFitAllocator] (API-Job-Executor-37:ctx-56e8e114 job-311 ctx-38d5fd62 FirstFitRoutingAllocator) (logid:b61c10e9) Looking for speed=256Mhz, Ram=256 MB
   2021-03-26 14:08:42,633 DEBUG [c.c.a.m.a.i.FirstFitAllocator] (API-Job-Executor-37:ctx-56e8e114 job-311 ctx-38d5fd62 FirstFitRoutingAllocator) (logid:b61c10e9) Host Allocator returning 0 suitable hosts
   2021-03-26 14:08:42,633 DEBUG [c.c.d.DeploymentPlanningManagerImpl] (API-Job-Executor-37:ctx-56e8e114 job-311 ctx-38d5fd62) (logid:b61c10e9) No suitable hosts found
   2021-03-26 14:08:42,633 DEBUG [c.c.d.DeploymentPlanningManagerImpl] (API-Job-Executor-37:ctx-56e8e114 job-311 ctx-38d5fd62) (logid:b61c10e9) No suitable hosts found under this Cluster: 1
   2021-03-26 14:08:42,633 DEBUG [c.c.d.DeploymentPlanningManagerImpl] (API-Job-Executor-37:ctx-56e8e114 job-311 ctx-38d5fd62) (logid:b61c10e9) Could not find suitable Deployment Destination for this VM under any clusters, returning.-
   ```


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] shwstppr commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-810732839


   @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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-803856329


   Packaging result: :heavy_multiplication_x: centos7 :heavy_check_mark: centos8 :heavy_multiplication_x: debian. SL-JID 202


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r607629776



##########
File path: tools/marvin/marvin/config/test_data.py
##########
@@ -928,6 +928,16 @@
         "ostype": "CentOS 5.6 (64-bit)",
         "mode": 'HTTP_DOWNLOAD',
     },
+    "iso3": {
+        "displaytext": "Test ISO 3",
+        "name": "ISO 3",
+        "url": "http://people.apache.org/~tsp/dummy.iso",

Review comment:
       @rhtyd, I tested it with UEFI enabled/disabled, and the test works as expected, but it only checks the dumpxml of the domain, nothing more (that's why I don't see a reason to add a valid ISO). Do I have to open a PR with the changes of test_data.py to your repo?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd merged pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773


   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-799502464


   > @DaanHoogland, I could do the python tests, but the only problem (for me) is the automating of the installation. I should research how and is it possible to automate the installation from ISO. This will take time, and I can't commit how much
   
   I understand, I think the main issue is that a VM with the ISO would be started (in UEFI mode), no? If we can verify that some prompt for installation is there, the rest of the installation is not sifnificant for this functionality. Or am I missing something?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-794235371


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_multiplication_x: debian. SL-JID 67


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-805638550


   @rhtyd, I have to do the Marvin tests. That's why I've put it into a draft until I'm ready


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-813888898


   @sureshanaparti in the last run as I see the hosts are skipped, don't see those failures. The test code also checks if hosts have uefi enabled.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r607692162



##########
File path: tools/marvin/marvin/config/test_data.py
##########
@@ -928,6 +928,16 @@
         "ostype": "CentOS 5.6 (64-bit)",
         "mode": 'HTTP_DOWNLOAD',
     },
+    "iso3": {
+        "displaytext": "Test ISO 3",
+        "name": "ISO 3",
+        "url": "http://people.apache.org/~tsp/dummy.iso",

Review comment:
       Ah okay so the invalid ISO doesn't make any difference. No need since by default Trillian env don't have UEFI enabled hosts, the tests will be skipped. I'll merge the PR based on your remark.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808621696


   <b>Trillian test result (tid-273)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 44904 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4773-t273-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_deploy_vm_iso_uefi.py
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers_iptables_default_policy.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
   Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Smoke tests completed. 84 look OK, 3 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_deploy_vm_from_iso_uefi_secure | `Error` | 1.17 | test_deploy_vm_iso_uefi.py
   test_02_deploy_vm_from_iso_uefi_legacy | `Error` | 1.13 | test_deploy_vm_iso_uefi.py
   test_03_deploy_windows_vm_from_iso_uefi_legacy | `Error` | 1.13 | test_deploy_vm_iso_uefi.py
   test_04_deploy_windows_vm_from_iso_uefi_secure | `Error` | 1.12 | test_deploy_vm_iso_uefi.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 72.08 | test_kubernetes_clusters.py
   test_01_migrate_VM_and_root_volume | `Error` | 67.16 | test_vm_life_cycle.py
   test_02_migrate_VM_with_two_data_disks | `Error` | 50.08 | test_vm_life_cycle.py
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808156958


   <b>Trillian test result (tid-266)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 66498 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4773-t266-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_deploy_vm_iso_uefi.py
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_resource_accounting.py
   Intermittent failure detected: /marvin/tests/smoke/test_router_dhcphosts.py
   Intermittent failure detected: /marvin/tests/smoke/test_router_dns.py
   Intermittent failure detected: /marvin/tests/smoke/test_router_dnsservice.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers_iptables_default_policy.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers.py
   Intermittent failure detected: /marvin/tests/smoke/test_service_offerings.py
   Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
   Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Smoke tests completed. 74 look OK, 13 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   ContextSuite context=TestDeployVMFromISOWithUefi>:setup | `Error` | 0.00 | test_deploy_vm_iso_uefi.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 68.74 | test_kubernetes_clusters.py
   ContextSuite context=TestRAMCPUResourceAccounting>:setup | `Error` | 0.00 | test_resource_accounting.py
   ContextSuite context=TestRouterDHCPHosts>:setup | `Error` | 0.00 | test_router_dhcphosts.py
   ContextSuite context=TestRouterDHCPOpts>:setup | `Error` | 0.00 | test_router_dhcphosts.py
   ContextSuite context=TestRouterDns>:setup | `Error` | 0.00 | test_router_dns.py
   ContextSuite context=TestRouterDnsService>:setup | `Error` | 0.00 | test_router_dnsservice.py
   ContextSuite context=TestRouterIpTablesPolicies>:setup | `Error` | 0.00 | test_routers_iptables_default_policy.py
   ContextSuite context=TestVPCIpTablesPolicies>:setup | `Error` | 0.00 | test_routers_iptables_default_policy.py
   ContextSuite context=TestIsolatedNetworks>:setup | `Error` | 0.00 | test_routers_network_ops.py
   ContextSuite context=TestRedundantIsolateNetworks>:setup | `Error` | 0.00 | test_routers_network_ops.py
   ContextSuite context=TestRouterServices>:setup | `Error` | 0.00 | test_routers.py
   ContextSuite context=TestCpuCapServiceOfferings>:setup | `Error` | 0.00 | test_service_offerings.py
   ContextSuite context=TestServiceOfferings>:setup | `Error` | 0.10 | test_service_offerings.py
   ContextSuite context=TestSnapshotRootDisk>:setup | `Error` | 0.00 | test_snapshots.py
   test_01_migrate_VM_and_root_volume | `Error` | 66.23 | test_vm_life_cycle.py
   test_02_migrate_VM_with_two_data_disks | `Error` | 48.96 | test_vm_life_cycle.py
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 479.20 | test_vpc_redundant.py
   test_05_rvpc_multi_tiers | `Failure` | 434.63 | test_vpc_redundant.py
   test_05_rvpc_multi_tiers | `Error` | 459.43 | test_vpc_redundant.py
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-805546257


   @slavkap is the PR work in progress? If not can you change the draft PR to 'ready for review'? Thanks.
   @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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-806149747


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 248


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-796559298


   @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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-793639073


   @shwstppr a Jenkins job has been kicked to build packages. 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-797413493


   @DaanHoogland, I could do the python tests, but the only problem (for me) is the automating of the installation. I should research how and is it possible to automate the installation from ISO. This will take time, and I can't commit how much


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-810747063


   Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 290


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-794180512


   Packaging result: :heavy_multiplication_x: centos7 :heavy_multiplication_x: centos8 :heavy_check_mark: debian. SL-JID 62


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-793980879






----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r598630234



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
##########
@@ -725,7 +725,7 @@ private String getDevLabel(int devId, DiskBus bus, boolean forIso) {
             } else if(devId >= 2) {
                 devId += 2;
             }
-            return (DiskBus.SATA == bus) ? "sdb" : "hd" + getDevLabelSuffix(devId);
+            return (DiskBus.SATA == bus) ? "sd" + getDevLabelSuffix(devId) : "hd" + getDevLabelSuffix(devId);

Review comment:
       And just saw that the second disk is virtio, and it should be SATA also. I Will add a fix for this, I just need to test it first




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-797366632


   Hi @DaanHoogland, what should be the expectations of Marvin's tests? I mean, do they have to do an automated install (probably with a kickstart for CentOS) and check if UEFI is enabled. Or deploy a VM from ISO with UEFI secure/legacy and check that we have access to it through ssh?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-804711964


   OK, I'll leave the PR as a draft until I'm ready with the tests. Is the test environment for KVM set to support UEFI?


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-810732910


   @shwstppr a Jenkins job has been kicked to build packages. 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r591454081



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -2564,16 +2565,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
             final DiskDef disk = new DiskDef();
             int devId = volume.getDiskSeq().intValue();
             if (volume.getType() == Volume.Type.ISO) {
-                if (volPath == null) {
-                    if (isSecureBoot) {
-                        disk.defISODisk(null, devId,isSecureBoot,isWindowsTemplate);
-                    } else {
-                        /* Add iso as placeholder */
-                        disk.defISODisk(null, devId);
-                    }
-                } else {
-                    disk.defISODisk(volPath, devId);
-                }
+
+                disk.defISODisk(volPath, devId, isUefiEnabled);

Review comment:
       @sureshanaparti here we check if UEFI is enabled because cdrom disk controller should be SATA for q35 machines




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-804066343


   Just to be sure that we are talking about the same thing - are you OK with the tests only to get the dumpxml and verify that UEFI is enabled from there, not to do the installation?
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-795361331


   @sureshanaparti, I have tested both scenarios (UEFI secure/legacy) on CentOS8 and Windows 10 and with RAW/QCOW2 images. But for the RBD storage pool I'm not sure is it going to 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r591289511



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -3454,6 +3448,8 @@ public boolean isCentosHost() {
             return DiskDef.DiskBus.IDE;
         } else if (platformEmulator.startsWith("Other PV Virtio-SCSI")) {
             return DiskDef.DiskBus.SCSI;
+        } else if (isUefiEnabled && platformEmulator.startsWith("Windows")) {
+            return DiskDef.DiskBus.SATA;

Review comment:
       Is this only required in case of UEFI secure mode only?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-797664139


   <b>[S] Trillian test result (tid-100)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 33524 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4773-t100-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
   Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
   Smoke tests completed. 84 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_migrate_VM_and_root_volume | `Error` | 85.53 | test_vm_life_cycle.py
   test_02_migrate_VM_with_two_data_disks | `Error` | 47.07 | test_vm_life_cycle.py
   test_01_secure_vm_migration | `Error` | 1.08 | test_vm_life_cycle.py
   test_02_unsecure_vm_migration | `Error` | 1.07 | test_vm_life_cycle.py
   test_03_secured_to_nonsecured_vm_migration | `Error` | 1.08 | test_vm_life_cycle.py
   test_04_nonsecured_to_secured_vm_migration | `Error` | 0.08 | test_vm_life_cycle.py
   test_08_migrate_vm | `Error` | 4.21 | test_vm_life_cycle.py
   test_hostha_enable_ha_when_host_in_maintenance | `Error` | 303.59 | test_hostha_kvm.py
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r607554381



##########
File path: tools/marvin/marvin/config/test_data.py
##########
@@ -928,6 +928,16 @@
         "ostype": "CentOS 5.6 (64-bit)",
         "mode": 'HTTP_DOWNLOAD',
     },
+    "iso3": {
+        "displaytext": "Test ISO 3",
+        "name": "ISO 3",
+        "url": "http://people.apache.org/~tsp/dummy.iso",

Review comment:
       URL is dummy, is there a valid iso url we can 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-797397490


   > Hi @DaanHoogland, what should be the expectations of Marvin's tests? I mean, do they have to do an automated install (probably with a kickstart for CentOS) and check if UEFI is enabled. Or deploy a VM from ISO with UEFI secure/legacy and check that we have access to it through ssh?
   
   @slavkap, the only expectation is that the verification is automated. This could be with ssh to host or instance, but also with APIs. Both are done a lot in the current set of tests. the problem would probably be with the install from ISO I imagine, but it was a genuine question, You did the testing and probably know best if the manual actions can be automated in python.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-793677257


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2889


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-812989156


   Last test run LGTM, can you review and test @sureshanaparti @Spaceman1984 


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-796559808


   @rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S]


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-795173607


   @slavkap Have you tested both UEFI Legacy and Secure modes ? 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-811695964


   @rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-804759574


   > OK, I'll leave the PR as a draft until I'm ready with the tests. Is the test environment for KVM set to support UEFI?
   
   I'll have to check. It is a standard centos7 install. If it isn't we will have to update it and rely on your manual tests anyway. You can put it in a separate PR, if you wish.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-804019858


   @DaanHoogland, sorry for the late response! I tried few things with python on how to access the VM (because with ssh will not happen). Unfortunately, I couldn't find a way to be prompt that UEFI is enabled before or during the installation. The easiest way is to get the VM's dumpxml and check if the machine type is q35 or if the loader is OVMF. What is your opinion about this?


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r598603409



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
##########
@@ -725,7 +725,7 @@ private String getDevLabel(int devId, DiskBus bus, boolean forIso) {
             } else if(devId >= 2) {
                 devId += 2;
             }
-            return (DiskBus.SATA == bus) ? "sdb" : "hd" + getDevLabelSuffix(devId);
+            return (DiskBus.SATA == bus) ? "sd" + getDevLabelSuffix(devId) : "hd" + getDevLabelSuffix(devId);

Review comment:
       @DaanHoogland actually this is only applied only for SATA disks, and without the change, the problem will appear when we deploy Windows VM from a template with 2 disks.
   
   ```
   <disk  device='disk' type='file'>
   <driver name='qemu' type='qcow2' cache='none' />
   <source file='/mnt/46aa22fc-815b-34cd-96fe-fb1f8fea2754/f28ed907-b9e4-46c2-b543-2b34f0140018'/>
   <target dev='sda' bus='sata'/>
   <serial>f28ed907b9e446c2b543</serial></disk>
   <disk  device='disk' type='file'>
   <driver name='qemu' type='qcow2' cache='none' />
   <source file='/mnt/46aa22fc-815b-34cd-96fe-fb1f8fea2754/bb1c40f8-0867-4197-a1e7-3cef7da7f1e5'/>
   <target dev='vdb' bus='virtio'/>
   <serial>bb1c40f808674197a1e7</serial></disk>
   <disk  device='cdrom' type='file'>
   <driver name='qemu' type='raw' />
   <source file=''/>
   <target dev='sda' bus='sata'/>
   </disk>
   ```
   `org.libvirt.LibvirtException: XML error: target 'sda' duplicated for disk sources '/mnt/46aa22fc-815b-34cd-96fe-fb1f8fea2754/f28ed907-b9e4-46c2-b543-2b34f0140018' and '<null>'`
   
   




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808189157


   @DaanHoogland, may I ask for some help? I've added the element "iso3" to test_data.py, but the UEFI tests failed because it's not in the dictionary. Is Marvin upgraded with the new changes, or did I not put the changes into the right place? I've tested them on my test environment, and it works


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r607619645



##########
File path: tools/marvin/marvin/config/test_data.py
##########
@@ -928,6 +928,16 @@
         "ostype": "CentOS 5.6 (64-bit)",
         "mode": 'HTTP_DOWNLOAD',
     },
+    "iso3": {
+        "displaytext": "Test ISO 3",
+        "name": "ISO 3",
+        "url": "http://people.apache.org/~tsp/dummy.iso",

Review comment:
       @slavkap does your newly added test work with the fix?
   
   Yes, that's a practice we should avoid if/when possible. For running tests with blueorangutan/Trillian, we patch the test_data.py file https://github.com/shapeblue/Trillian/blob/master/Ansible/roles/marvin/templates/test_data.py.j2
   
   So anytime anyone adds invalid data, we manually patch it.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808677858


   @shwstppr, just to inform you that I have a zone with 4 hypervisors in my test environment. If I set up only one hypervisor to support UEFI, everything works fine, but when I set up a second one to support it, CloudStack couldn't find a suitable host to deploy the VM. Unfortunately, I didn't have time to research this problem.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r598571813



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
##########
@@ -725,7 +725,7 @@ private String getDevLabel(int devId, DiskBus bus, boolean forIso) {
             } else if(devId >= 2) {
                 devId += 2;
             }
-            return (DiskBus.SATA == bus) ? "sdb" : "hd" + getDevLabelSuffix(devId);
+            return (DiskBus.SATA == bus) ? "sd" + getDevLabelSuffix(devId) : "hd" + getDevLabelSuffix(devId);

Review comment:
       ```suggestion
               return ((DiskBus.SATA == bus) ? "sd" : "hd") + getDevLabelSuffix(devId);
   ```
   also, also, are those prefixes really really usniversal across OSses?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] shwstppr commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-793637830


   @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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on a change in pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on a change in pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#discussion_r591454980



##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -3454,6 +3448,8 @@ public boolean isCentosHost() {
             return DiskDef.DiskBus.IDE;
         } else if (platformEmulator.startsWith("Other PV Virtio-SCSI")) {
             return DiskDef.DiskBus.SCSI;
+        } else if (isUefiEnabled && platformEmulator.startsWith("Windows")) {
+            return DiskDef.DiskBus.SATA;

Review comment:
       this is required for both secure/legacy UEFI boot type on Windows machines




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-804060532


   > @DaanHoogland, sorry for the late response! I tried few things with python on how to access the VM (because with ssh will not happen). Unfortunately, I couldn't find a way to be prompt that UEFI is enabled before or during the installation. The easiest way is to get the VM's dumpxml and check if the machine type is q35 or if the loader is OVMF. What is your opinion about this?
   
   I gather you are talking testing; yes this can be done as ssh to the host during verification is easy, there are examples around.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808219284


   OK! Thanks again, @shwstppr, for your reply and the help!


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-812084407


   <b>Trillian test result (tid-326)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 38264 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4773-t326-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
   Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
   Smoke tests completed. 85 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 69.84 | test_kubernetes_clusters.py
   test_01_migrate_VM_and_root_volume | `Error` | 66.19 | test_vm_life_cycle.py
   test_02_migrate_VM_with_two_data_disks | `Error` | 49.07 | test_vm_life_cycle.py
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-804700706


   > Just to be sure that we are talking about the same thing - are you OK with the tests only to get the dumpxml and verify that UEFI is enabled from there, not to do the installation?
   
   yes, the purpose of this code is just that is it? there is no assumption that the install actually works and this is out of our hands anyway , *if* i understand correctly.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] shwstppr commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808215603


   Marvin base.py file will get auto updated @slavkap 


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808675925


   @shwstppr, you mean to skip the tests if non of the Trillian hosts doesn't have UEFI? Are you planning to set up at least one host to support UEFI? If the answer is no, maybe those tests aren't needed at all.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] slavkap commented on pull request #4773: Fix deploy VM from ISOs with UEFI

Posted by GitBox <gi...@apache.org>.
slavkap commented on pull request #4773:
URL: https://github.com/apache/cloudstack/pull/4773#issuecomment-808205643


   Thanks, @shwstppr! Does it also have to be updated and the Marvin base.py file (because there are added few changes either)?


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org