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/09/24 19:03:04 UTC

[GitHub] [cloudstack] harikrishna-patnala opened a new pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

harikrishna-patnala opened a new pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510


   ### Description
   
   This PR fixes issue #5498 where multiple volumes of a VM are exported to secondary storage upon taking snapshot of a single volume in case of VMware
   <!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   <!-- Fixes: # -->
   Fixes: #5498 
   
   <!--- ********************************************************************************* -->
   <!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
   <!--- PLEASE PUT AN 'X' in only **ONE** box -->
   <!--- ********************************************************************************* -->
   
   ### 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)
   
   ### Feature/Enhancement Scale or Bug Severity
   
   #### Feature/Enhancement Scale
   
   - [x] Major
   - [ ] Minor
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [x] 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 -->
   <!-- see how your change affects other areas of the code, etc. -->
   1. Created a VM with multiple disks
   2. Created snapshot of one disk (also a recurring snapshot). (before creating a snapshot, add some data to the disk)
   3. Check secondary storage snapshot folder to have only one disk
   4. Created a volume from the snapshot above (to check subsequent operations on that snapshot are working fine)
   5. Attached the above volume to a VM and made sure it is the proper disk by checking the data created before in Step 2
   
   <!-- 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

[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @harikrishna-patnala unsupported parameters provided. Supported mgmt server os are: `centos7, centos6, alma8, ubuntu18, suse15, ubuntu20, rocky8, centos8`. Supported hypervisors are: `kvm-centos6, kvm-centos7, kvm-centos8, kvm-rocky8, kvm-alma8, kvm-ubuntu18, kvm-ubuntu20, kvm-suse15, vmware-55u3, vmware-60u2, vmware-65u2, vmware-67u3, vmware-70u1, xenserver-65sp1, xenserver-71, xenserver-74, xcpng74, xcpng76, xcpng80, xcpng81, xcpng82`


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

[GitHub] [cloudstack] nvazquez merged pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   


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

[GitHub] [cloudstack] sureshanaparti commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @blueorangutan test centos7 vmware-67u3


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

[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {

Review comment:
       +1

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {
+            VirtualDisk requiredCloneDisk = null;
+            for (VirtualDisk vmDisk: vmDisks) {
+                if (vmDisk.getKey() == requiredDisk.getKey()) {

Review comment:
       @harikrishna-patnala to be on safer side, can you confirm the disk key when `vmDisks.length == 1` as well




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

[GitHub] [cloudstack] harikrishna-patnala commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-926522815


   @blueorangutan test centos7 vmware-65u2


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

[GitHub] [cloudstack] harikrishna-patnala commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-926519496


   @blueorangutan test centos7 vmware65u2


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

[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
##########
@@ -1849,6 +1854,19 @@ public Answer createTemplateFromSnapshot(CopyCommand cmd) {
         }
     }
 
+    private void checkIfVMHasOnlyRequiredDisk(VirtualMachineMO clonedVm, VirtualDisk requiredDisk) throws Exception {
+
+        s_logger.info(String.format("Checking if Cloned VM %s is created only with required Disk, if not detach the remaining disks", clonedVm.getName()));
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        if (vmDisks.length != 1) {
+            String baseName = VmwareHelper.getDiskDeviceFileName(requiredDisk);
+            s_logger.info(String.format("Detaching all disks for the cloned VM: %s except disk with base name: %s, key=%d", clonedVm.getName(), baseName, requiredDisk.getKey()));
+            clonedVm.detachAllDisksExcept(VmwareHelper.getDiskDeviceFileName(requiredDisk), null);

Review comment:
       Is it possible to include this process, as part of creating full clone with specific disk (which ensures it detaches/destroys the additional disks), instead checking this separately.




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

[GitHub] [cloudstack] rohityadavcloud removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @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

[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   <b>Trillian test result (tid-2243)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 39958 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5510-t2243-vmware-67u3.zip
   Smoke tests completed. 89 look OK, 1 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_03_live_migrate_VM_with_two_data_disks | `Error` | 63.73 | 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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,77 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);

Review comment:
       Is this call required even if Cloned VM is failed?




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

[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,77 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);

Review comment:
       Is this call required even if Cloned VM is failed?

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,77 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
+
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length == 1 && vmDisks[0].getKey() == requiredDisk.getKey()) {
+            s_logger.debug(String.format("VM %s is created only with required Disk", vmName));
+            return;
+        }
+
+        VirtualDisk requiredCloneDisk = null;
+        for (VirtualDisk vmDisk: vmDisks) {
+            if (vmDisk.getKey() == requiredDisk.getKey()) {
+                requiredCloneDisk = vmDisk;
+                break;
+            }
+        }
+        if (requiredCloneDisk == null) {
+            s_logger.error(String.format("Failed to identify required disk in VM %s", vmName));
+            return;
+        }
+
+        String baseName = VmwareHelper.getDiskDeviceFileName(requiredCloneDisk);
+        s_logger.debug(String.format("Detaching all disks for the VM: %s except disk with base name: %s, key=%d", vmName, baseName, requiredCloneDisk.getKey()));
+        clonedVm.detachAllDisksExcept(baseName, null);

Review comment:
       are the disks destroyed after detaching them?




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

[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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






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

[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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






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

[GitHub] [cloudstack] harikrishna-patnala commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-929997997


   @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

[GitHub] [cloudstack] rhtyd commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   This needs a manual test confirmation cc @borisstoyanov @vladimirpetrov @NuxRo thanks


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

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#discussion_r718292114



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,77 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);

Review comment:
       I kept this because deprecated param VirtualMachineConfigSpec might behave differently in vSphere upcoming releases. so as a safer side kept this method call here. But realised it is not required because that will get caught during release testing of that vSphere version.  So removed it here now. Thanks




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

[GitHub] [cloudstack] harikrishna-patnala commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-926449062


   @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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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






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

[GitHub] [cloudstack] rhtyd commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @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

[GitHub] [cloudstack] sureshanaparti commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @blueorangutan test centos7 vmware-67u3


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1442


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

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#discussion_r715559992



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
##########
@@ -1849,6 +1854,19 @@ public Answer createTemplateFromSnapshot(CopyCommand cmd) {
         }
     }
 
+    private void checkIfVMHasOnlyRequiredDisk(VirtualMachineMO clonedVm, VirtualDisk requiredDisk) throws Exception {
+
+        s_logger.info(String.format("Checking if Cloned VM %s is created only with required Disk, if not detach the remaining disks", clonedVm.getName()));
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        if (vmDisks.length != 1) {
+            String baseName = VmwareHelper.getDiskDeviceFileName(requiredDisk);
+            s_logger.info(String.format("Detaching all disks for the cloned VM: %s except disk with base name: %s, key=%d", clonedVm.getName(), baseName, requiredDisk.getKey()));
+            clonedVm.detachAllDisksExcept(VmwareHelper.getDiskDeviceFileName(requiredDisk), null);

Review comment:
       I'll try to keep it in same method. Thanks Suresh




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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   <b>Trillian test result (tid-2202)</b>
   Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 41735 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5510-t2202-vmware-65u2.zip
   Smoke tests completed. 89 look OK, 0 have errors
   Only failed 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

[GitHub] [cloudstack] DaanHoogland commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @nvazquez a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) 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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   <b>Trillian test result (tid-2220)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 37979 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5510-t2220-vmware-67u3.zip
   Smoke tests completed. 89 look OK, 0 have errors
   Only failed 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

[GitHub] [cloudstack] harikrishna-patnala removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala removed a comment on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-926519496


   @blueorangutan test centos7 vmware65u2


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @harikrishna-patnala a Trillian-Jenkins test job (centos7 mgmt + vmware-65u2) 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

[GitHub] [cloudstack] rhtyd commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   <b>Trillian test result (tid-2239)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 36756 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5510-t2239-vmware-67u3.zip
   Smoke tests completed. 89 look OK, 1 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_03_live_migrate_VM_with_two_data_disks | `Error` | 61.81 | 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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] harikrishna-patnala removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala removed a comment on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-926522815


   @blueorangutan test centos7 vmware-65u2


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

[GitHub] [cloudstack] rohityadavcloud removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @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

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#discussion_r718122123



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {

Review comment:
       Changed it as suggested

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {
+            VirtualDisk requiredCloneDisk = null;
+            for (VirtualDisk vmDisk: vmDisks) {
+                if (vmDisk.getKey() == requiredDisk.getKey()) {
+                    requiredCloneDisk = vmDisk;
+                    break;
+                }
+            }
+            if (requiredCloneDisk != null) {

Review comment:
       Changed it as suggested




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

[GitHub] [cloudstack] nvazquez commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @harikrishna-patnala can you please check the test failure? Seems a consistent failure


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] DaanHoogland commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @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

[GitHub] [cloudstack] harikrishna-patnala closed pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala closed pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510


   


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

[GitHub] [cloudstack] nvazquez commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @blueorangutan test centos7 vmware-67u3


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   <b>Trillian test result (tid-2220)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 37979 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5510-t2220-vmware-67u3.zip
   Smoke tests completed. 89 look OK, 0 have errors
   Only failed 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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] rhtyd commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @harikrishna-patnala does the sdk/api behave differently in different vmware versions? 
   @blueorangutan test centos7 vmware-67u3


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1387


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @harikrishna-patnala unsupported parameters provided. Supported mgmt server os are: `centos7, centos6, alma8, ubuntu18, suse15, ubuntu20, rocky8, centos8`. Supported hypervisors are: `kvm-centos6, kvm-centos7, kvm-centos8, kvm-rocky8, kvm-alma8, kvm-ubuntu18, kvm-ubuntu20, kvm-suse15, vmware-55u3, vmware-60u2, vmware-65u2, vmware-67u3, vmware-70u1, xenserver-65sp1, xenserver-71, xenserver-74, xcpng74, xcpng76, xcpng80, xcpng81, xcpng82`


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   <b>Trillian test result (tid-2206)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 38793 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5510-t2206-vmware-67u3.zip
   Smoke tests completed. 89 look OK, 0 have errors
   Only failed 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

[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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






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

[GitHub] [cloudstack] harikrishna-patnala removed a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala removed a comment on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-926449062


   @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

[GitHub] [cloudstack] harikrishna-patnala commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-926577838


   > @harikrishna-patnala does the sdk/api behave differently in different vmware versions?
   
   Ideally it should not differ, I'll test all versions.


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,77 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
+
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length == 1 && vmDisks[0].getKey() == requiredDisk.getKey()) {
+            s_logger.debug(String.format("VM %s is created only with required Disk", vmName));
+            return;
+        }
+
+        VirtualDisk requiredCloneDisk = null;
+        for (VirtualDisk vmDisk: vmDisks) {
+            if (vmDisk.getKey() == requiredDisk.getKey()) {
+                requiredCloneDisk = vmDisk;
+                break;
+            }
+        }
+        if (requiredCloneDisk == null) {
+            s_logger.error(String.format("Failed to identify required disk in VM %s", vmName));
+            return;
+        }
+
+        String baseName = VmwareHelper.getDiskDeviceFileName(requiredCloneDisk);
+        s_logger.debug(String.format("Detaching all disks for the VM: %s except disk with base name: %s, key=%d", vmName, baseName, requiredCloneDisk.getKey()));
+        clonedVm.detachAllDisksExcept(baseName, null);

Review comment:
       are the disks destroyed after detaching them?




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

[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {

Review comment:
       +1




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

[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {
+            VirtualDisk requiredCloneDisk = null;
+            for (VirtualDisk vmDisk: vmDisks) {
+                if (vmDisk.getKey() == requiredDisk.getKey()) {
+                    requiredCloneDisk = vmDisk;
+                    break;
+                }
+            }
+            if (requiredCloneDisk != null) {

Review comment:
       What about inverting the `if` and adding a `return` statement? This way we would reduce indentation and improve readability.

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {

Review comment:
       What about inverting the `if` and adding a `return` statement? This way we would reduce indentation and improve readability.




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

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#discussion_r716580182



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
##########
@@ -1849,6 +1854,19 @@ public Answer createTemplateFromSnapshot(CopyCommand cmd) {
         }
     }
 
+    private void checkIfVMHasOnlyRequiredDisk(VirtualMachineMO clonedVm, VirtualDisk requiredDisk) throws Exception {
+
+        s_logger.info(String.format("Checking if Cloned VM %s is created only with required Disk, if not detach the remaining disks", clonedVm.getName()));
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        if (vmDisks.length != 1) {
+            String baseName = VmwareHelper.getDiskDeviceFileName(requiredDisk);
+            s_logger.info(String.format("Detaching all disks for the cloned VM: %s except disk with base name: %s, key=%d", clonedVm.getName(), baseName, requiredDisk.getKey()));
+            clonedVm.detachAllDisksExcept(VmwareHelper.getDiskDeviceFileName(requiredDisk), null);

Review comment:
       Moved the process to virtualMachineMO class.




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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1440


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   @sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) 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

[GitHub] [cloudstack] rhtyd commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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






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

[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {
+            VirtualDisk requiredCloneDisk = null;
+            for (VirtualDisk vmDisk: vmDisks) {
+                if (vmDisk.getKey() == requiredDisk.getKey()) {
+                    requiredCloneDisk = vmDisk;
+                    break;
+                }
+            }
+            if (requiredCloneDisk != null) {

Review comment:
       What about inverting the `if` and adding a `return` statement? This way we would reduce indentation and improve readability.

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {

Review comment:
       What about inverting the `if` and adding a `return` statement? This way we would reduce indentation and improve readability.




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

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#discussion_r718129291



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {
+            VirtualDisk requiredCloneDisk = null;
+            for (VirtualDisk vmDisk: vmDisks) {
+                if (vmDisk.getKey() == requiredDisk.getKey()) {

Review comment:
       added the key check here




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

[GitHub] [cloudstack] harikrishna-patnala commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-929997997


   @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

[GitHub] [cloudstack] weizhouapache closed pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
weizhouapache closed pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510


   


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_check_mark: suse15. SL-JID 1438


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

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#discussion_r718122123



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {

Review comment:
       Changed it as suggested

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {
+            VirtualDisk requiredCloneDisk = null;
+            for (VirtualDisk vmDisk: vmDisks) {
+                if (vmDisk.getKey() == requiredDisk.getKey()) {
+                    requiredCloneDisk = vmDisk;
+                    break;
+                }
+            }
+            if (requiredCloneDisk != null) {

Review comment:
       Changed it as suggested

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {
+            VirtualDisk requiredCloneDisk = null;
+            for (VirtualDisk vmDisk: vmDisks) {
+                if (vmDisk.getKey() == requiredDisk.getKey()) {

Review comment:
       added the key check here

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,77 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
+
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length == 1 && vmDisks[0].getKey() == requiredDisk.getKey()) {
+            s_logger.debug(String.format("VM %s is created only with required Disk", vmName));
+            return;
+        }
+
+        VirtualDisk requiredCloneDisk = null;
+        for (VirtualDisk vmDisk: vmDisks) {
+            if (vmDisk.getKey() == requiredDisk.getKey()) {
+                requiredCloneDisk = vmDisk;
+                break;
+            }
+        }
+        if (requiredCloneDisk == null) {
+            s_logger.error(String.format("Failed to identify required disk in VM %s", vmName));
+            return;
+        }
+
+        String baseName = VmwareHelper.getDiskDeviceFileName(requiredCloneDisk);
+        s_logger.debug(String.format("Detaching all disks for the VM: %s except disk with base name: %s, key=%d", vmName, baseName, requiredCloneDisk.getKey()));
+        clonedVm.detachAllDisksExcept(baseName, null);

Review comment:
       Thanks for it Suresh, added code for deletion

##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,77 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);

Review comment:
       I kept this because deprecated param VirtualMachineConfigSpec might behave differently in vSphere upcoming releases. so as a safer side kept this method call here. But realised it is not required because that will get caught during release testing of that vSphere version.  So removed it here now. Thanks




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

[GitHub] [cloudstack] harikrishna-patnala closed pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala closed pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510


   


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1435


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

[GitHub] [cloudstack] nvazquez edited a comment on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
nvazquez edited a comment on pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#issuecomment-931351109


   Edit: smoke test failure also seen on the health checks PR, merging this PR - test needs fixing on stabilisation phase


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


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

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

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

[GitHub] [cloudstack] blueorangutan commented on pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_check_mark: suse15. SL-JID 1433


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

[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5510:
URL: https://github.com/apache/cloudstack/pull/5510#discussion_r718289474



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,77 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
+
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length == 1 && vmDisks[0].getKey() == requiredDisk.getKey()) {
+            s_logger.debug(String.format("VM %s is created only with required Disk", vmName));
+            return;
+        }
+
+        VirtualDisk requiredCloneDisk = null;
+        for (VirtualDisk vmDisk: vmDisks) {
+            if (vmDisk.getKey() == requiredDisk.getKey()) {
+                requiredCloneDisk = vmDisk;
+                break;
+            }
+        }
+        if (requiredCloneDisk == null) {
+            s_logger.error(String.format("Failed to identify required disk in VM %s", vmName));
+            return;
+        }
+
+        String baseName = VmwareHelper.getDiskDeviceFileName(requiredCloneDisk);
+        s_logger.debug(String.format("Detaching all disks for the VM: %s except disk with base name: %s, key=%d", vmName, baseName, requiredCloneDisk.getKey()));
+        clonedVm.detachAllDisksExcept(baseName, null);

Review comment:
       Thanks for it Suresh, added code for deletion




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

[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5510: Fix export snapshot and template to secondary storage to export only required disk

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -751,43 +751,74 @@ public boolean hasSnapshot() throws Exception {
         return false;
     }
 
-    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, ManagedObjectReference morDs, Pair<VirtualDisk, String> volumeDeviceInfo)
+    public boolean createFullCloneWithSpecificDisk(String cloneName, ManagedObjectReference morFolder, ManagedObjectReference morResourcePool, VirtualDisk requiredDisk)
             throws Exception {
 
         assert (morFolder != null);
         assert (morResourcePool != null);
-        assert (morDs != null);
-        VirtualDisk requiredDisk = volumeDeviceInfo.first();
 
         VirtualMachineRelocateSpec rSpec = new VirtualMachineRelocateSpec();
-        List<VirtualMachineRelocateSpecDiskLocator> diskLocator = new ArrayList<VirtualMachineRelocateSpecDiskLocator>(1);
-        VirtualMachineRelocateSpecDiskLocator loc = new VirtualMachineRelocateSpecDiskLocator();
-        loc.setDatastore(morDs);
-        loc.setDiskId(requiredDisk.getKey());
-        loc.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        diskLocator.add(loc);
-
-        rSpec.setDiskMoveType(VirtualMachineRelocateDiskMoveOptions.MOVE_ALL_DISK_BACKINGS_AND_DISALLOW_SHARING.value());
-        rSpec.getDisk().addAll(diskLocator);
+
+        VirtualDisk[] vmDisks = getAllDiskDevice();
+        VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec();
+        for (VirtualDisk disk : vmDisks) {
+            if (requiredDisk.getKey() != disk.getKey()) {
+                VirtualDeviceConfigSpec virtualDeviceConfigSpec = new VirtualDeviceConfigSpec();
+                virtualDeviceConfigSpec.setDevice(disk);
+                virtualDeviceConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.REMOVE);
+                vmConfigSpec.getDeviceChange().add(virtualDeviceConfigSpec);
+            }
+        }
         rSpec.setPool(morResourcePool);
 
         VirtualMachineCloneSpec cloneSpec = new VirtualMachineCloneSpec();
         cloneSpec.setPowerOn(false);
         cloneSpec.setTemplate(false);
         cloneSpec.setLocation(rSpec);
+        cloneSpec.setMemory(false);
+        cloneSpec.setConfig(vmConfigSpec);
 
         ManagedObjectReference morTask = _context.getService().cloneVMTask(_mor, morFolder, cloneName, cloneSpec);
 
         boolean result = _context.getVimClient().waitForTask(morTask);
         if (result) {
             _context.waitForTaskProgressDone(morTask);
             s_logger.debug(String.format("Cloned VM: %s as %s", getName(), cloneName));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
             return true;
         } else {
             s_logger.error("VMware cloneVM_Task failed due to " + TaskMO.getTaskFailureInfo(_context, morTask));
+            makeSureVMHasOnlyRequiredDisk(cloneName, requiredDisk);
+            return false;
         }
+    }
 
-        return false;
+    private void makeSureVMHasOnlyRequiredDisk(String vmName, VirtualDisk requiredDisk) throws Exception {
+        VirtualMachineRuntimeInfo runtimeInfo = getRuntimeInfo();
+        HostMO hostMo = new HostMO(_context, runtimeInfo.getHost());
+        DatacenterMO dcMo = new DatacenterMO(_context, hostMo.getHyperHostDatacenter());
+
+        VirtualMachineMO clonedVm = dcMo.findVm(vmName);
+        VirtualDisk[] vmDisks = clonedVm.getAllDiskDevice();
+        s_logger.debug(String.format("Checking if VM %s is created only with required Disk, if not detach the remaining disks", vmName));
+        if (vmDisks.length != 1) {
+            VirtualDisk requiredCloneDisk = null;
+            for (VirtualDisk vmDisk: vmDisks) {
+                if (vmDisk.getKey() == requiredDisk.getKey()) {

Review comment:
       @harikrishna-patnala to be on safer side, can you confirm the disk key when `vmDisks.length == 1` as well




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