CLOUDSTACK-9738: [Vmware] Optimize vm expunge process for instances with vm snapshots

nvazquez · nvazquez · commit 6ce6cf67f024 · 2017-02-06T23:39:01.000-03:00
diff --git a/api/src/com/cloud/vm/UserVmService.java b/api/src/com/cloud/vm/UserVmService.java
@@ -75,14 +75,14 @@ public interface UserVmService {
     /**
      * Destroys one virtual machine
      *
-     * @param userId
-     *            the id of the user performing the action
      * @param vmId
      *            the id of the virtual machine.
+     * @param expunge
+     *            indicates if vm should be expunged
      * @throws ConcurrentOperationException
      * @throws ResourceUnavailableException
      */
-    UserVm destroyVm(long vmId) throws ResourceUnavailableException, ConcurrentOperationException;
+    UserVm destroyVm(long vmId, boolean expunge) throws ResourceUnavailableException, ConcurrentOperationException;
 
     /**
      * Resets the password of a virtual machine.
diff --git a/api/src/com/cloud/vm/snapshot/VMSnapshotService.java b/api/src/com/cloud/vm/snapshot/VMSnapshotService.java
@@ -46,4 +46,11 @@ UserVm revertToSnapshot(Long vmSnapshotId) throws InsufficientServerCapacityExce
         ConcurrentOperationException;
 
     VirtualMachine getVMBySnapshotId(Long id);
+
+    /**
+     * Delete vm snapshots only from database. Introduced as a Vmware optimization in which vm snapshots are deleted when
+     * the vm gets deleted on hypervisor (no need to delete each vm snapshot before deleting vm, just mark them as deleted on DB)
+     * @param id vm id
+     */
+    boolean deleteVMSnapshotsFromDB(Long vmId);
 }
diff --git a/engine/api/src/com/cloud/vm/VirtualMachineManager.java b/engine/api/src/com/cloud/vm/VirtualMachineManager.java
@@ -110,7 +110,7 @@ void orchestrateStart(String vmUuid, Map<VirtualMachineProfile.Param, Object> pa
 
     void advanceExpunge(String vmUuid) throws ResourceUnavailableException, OperationTimedoutException, ConcurrentOperationException;
 
-    void destroy(String vmUuid) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException;
+    void destroy(String vmUuid, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException;
 
     void migrateAway(String vmUuid, long hostId) throws InsufficientServerCapacityException;
 
diff --git a/engine/api/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntity.java b/engine/api/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntity.java
@@ -129,8 +129,9 @@ String reserve(DeploymentPlanner plannerToUse, @BeanParam DeploymentPlan plan, E
 
     /**
      * Destroys the VM.
+     * @param expunge indicates if vm should be expunged
      */
-    boolean destroy(String caller) throws AgentUnavailableException, CloudException, ConcurrentOperationException;
+    boolean destroy(String caller, boolean expunge) throws AgentUnavailableException, CloudException, ConcurrentOperationException;
 
     /**
      * Duplicate this VM in the database so that it will start new
diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/VMSnapshotStrategy.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/VMSnapshotStrategy.java
@@ -28,4 +28,12 @@ public interface VMSnapshotStrategy {
     boolean revertVMSnapshot(VMSnapshot vmSnapshot);
 
     StrategyPriority canHandle(VMSnapshot vmSnapshot);
+
+    /**
+     * Delete vm snapshot only from database. Introduced as a Vmware optimization in which vm snapshots are deleted when
+     * the vm gets deleted on hypervisor (no need to delete each vm snapshot before deleting vm, just mark them as deleted on DB)
+     * @param vmSnapshot vm snapshot to be marked as deleted.
+     * @return true if vm snapshot removed from DB, false if not.
+     */
+    boolean deleteVMSnapshotFromDB(VMSnapshot vmSnapshot);
 }
diff --git a/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java b/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java
@@ -1679,7 +1679,7 @@ public boolean stateTransitTo(final VirtualMachine vm1, final VirtualMachine.Eve
     }
 
     @Override
-    public void destroy(final String vmUuid) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {
+    public void destroy(final String vmUuid, final boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {
         VMInstanceVO vm = _vmDao.findByUuid(vmUuid);
         if (vm == null || vm.getState() == State.Destroyed || vm.getState() == State.Expunging || vm.getRemoved() != null) {
             if (s_logger.isDebugEnabled()) {
@@ -1689,15 +1689,12 @@ public void destroy(final String vmUuid) throws AgentUnavailableException, Opera
         }
 
         if (s_logger.isDebugEnabled()) {
-            s_logger.debug("Destroying vm " + vm);
+            s_logger.debug("Destroying vm " + vm + ", expunge flag " + (expunge ? "on" : "off"));
         }
 
         advanceStop(vmUuid, VmDestroyForcestop.value());
 
-        if (!_vmSnapshotMgr.deleteAllVMSnapshots(vm.getId(), null)) {
-            s_logger.debug("Unable to delete all snapshots for " + vm);
-            throw new CloudRuntimeException("Unable to delete vm snapshots for " + vm);
-        }
+        deleteVMSnapshots(vm, expunge);
 
         // reload the vm object from db
         vm = _vmDao.findByUuid(vmUuid);
@@ -1712,6 +1709,27 @@ public void destroy(final String vmUuid) throws AgentUnavailableException, Opera
         }
     }
 
+    /**
+     * Delete vm snapshots depending on vm's hypervisor type. For Vmware, vm snapshots removal is delegated to vm cleanup thread
+     * to reduce tasks sent to hypervisor (one tasks to delete vm snapshots and vm itself
+     * instead of one task for each vm snapshot plus another for the vm)
+     * @param vm vm
+     * @param expunge indicates if vm should be expunged
+     */
+    private void deleteVMSnapshots(VMInstanceVO vm, boolean expunge) {
+        if (! vm.getHypervisorType().equals(HypervisorType.VMware)) {
+            if (!_vmSnapshotMgr.deleteAllVMSnapshots(vm.getId(), null)) {
+                s_logger.debug("Unable to delete all snapshots for " + vm);
+                throw new CloudRuntimeException("Unable to delete vm snapshots for " + vm);
+            }
+        }
+        else {
+            if (expunge) {
+                _vmSnapshotMgr.deleteVMSnapshotsFromDB(vm.getId());
+            }
+        }
+    }
+
     protected boolean checkVmOnHost(final VirtualMachine vm, final long hostId) throws AgentUnavailableException, OperationTimedoutException {
         final Answer answer = _agentMgr.send(hostId, new CheckVirtualMachineCommand(vm.getInstanceName()));
         if (answer == null || !answer.getResult()) {
diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/cloud/entity/api/VMEntityManager.java b/engine/orchestration/src/org/apache/cloudstack/engine/cloud/entity/api/VMEntityManager.java
@@ -46,5 +46,5 @@ void deployVirtualMachine(String reservationId, VMEntityVO vmEntityVO, String ca
 
     boolean stopvirtualmachineforced(VMEntityVO vmEntityVO, String caller) throws ResourceUnavailableException;
 
-    boolean destroyVirtualMachine(VMEntityVO vmEntityVO, String caller) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException;
+    boolean destroyVirtualMachine(VMEntityVO vmEntityVO, String caller, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException;
 }
diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/cloud/entity/api/VMEntityManagerImpl.java b/engine/orchestration/src/org/apache/cloudstack/engine/cloud/entity/api/VMEntityManagerImpl.java
@@ -261,10 +261,10 @@ public boolean stopvirtualmachineforced(VMEntityVO vmEntityVO, String caller) th
     }
 
     @Override
-    public boolean destroyVirtualMachine(VMEntityVO vmEntityVO, String caller) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {
+    public boolean destroyVirtualMachine(VMEntityVO vmEntityVO, String caller, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {
 
         VMInstanceVO vm = _vmDao.findByUuid(vmEntityVO.getUuid());
-        _itMgr.destroy(vm.getUuid());
+        _itMgr.destroy(vm.getUuid(), expunge);
         return true;
     }
 
diff --git a/engine/orchestration/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntityImpl.java b/engine/orchestration/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntityImpl.java
@@ -229,8 +229,8 @@ public void cleanup() {
     }
 
     @Override
-    public boolean destroy(String caller) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {
-        return manager.destroyVirtualMachine(this.vmEntityVO, caller);
+    public boolean destroy(String caller, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {
+        return manager.destroyVirtualMachine(this.vmEntityVO, caller, expunge);
     }
 
     @Override
diff --git a/engine/storage/snapshot/src/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java b/engine/storage/snapshot/src/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java
@@ -392,4 +392,15 @@ public boolean revertVMSnapshot(VMSnapshot vmSnapshot) {
     public StrategyPriority canHandle(VMSnapshot vmSnapshot) {
         return StrategyPriority.DEFAULT;
     }
+
+    @Override
+    public boolean deleteVMSnapshotFromDB(VMSnapshot vmSnapshot) {
+        try {
+            vmSnapshotHelper.vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.ExpungeRequested);
+        } catch (NoTransitionException e) {
+            s_logger.debug("Failed to change vm snapshot state with event ExpungeRequested");
+            throw new CloudRuntimeException("Failed to change vm snapshot state with event ExpungeRequested: " + e.getMessage());
+        }
+        return vmSnapshotDao.remove(vmSnapshot.getId());
+    }
 }
diff --git a/plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java b/plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java
@@ -1679,9 +1679,6 @@ public Answer deleteVolume(DeleteCommand cmd) {
                             s_logger.info("Destroy root volume and VM itself. vmName " + vmName);
                         }
 
-                        // Remove all snapshots to consolidate disks for removal
-                        vmMo.removeAllSnapshots();
-
                         VirtualMachineDiskInfo diskInfo = null;
                         if (vol.getChainInfo() != null)
                             diskInfo = _gson.fromJson(vol.getChainInfo(), VirtualMachineDiskInfo.class);
diff --git a/server/src/com/cloud/ha/HighAvailabilityManagerImpl.java b/server/src/com/cloud/ha/HighAvailabilityManagerImpl.java
@@ -674,7 +674,7 @@ protected Long destroyVM(final HaWorkVO work) {
             }
 
             if (vm.getHostId() != null) {
-                _itMgr.destroy(vm.getUuid());
+                _itMgr.destroy(vm.getUuid(), false);
                 s_logger.info("Successfully destroy " + vm);
                 return null;
             } else {
diff --git a/server/src/com/cloud/network/as/AutoScaleManagerImpl.java b/server/src/com/cloud/network/as/AutoScaleManagerImpl.java
@@ -1514,7 +1514,7 @@ public void doScaleDown(final long groupId) {
                     public void run() {
                         try {
 
-                            _userVmManager.destroyVm(vmId);
+                            _userVmManager.destroyVm(vmId, false);
 
                         } catch (ResourceUnavailableException e) {
                             e.printStackTrace();
diff --git a/server/src/com/cloud/resource/ResourceManagerImpl.java b/server/src/com/cloud/resource/ResourceManagerImpl.java
@@ -2165,7 +2165,7 @@ public void deleteRoutingHost(final HostVO host, final boolean isForced, final b
                 final List<VMInstanceVO> vmsOnLocalStorage = _storageMgr.listByStoragePool(storagePool.getId());
                 for (final VMInstanceVO vm : vmsOnLocalStorage) {
                     try {
-                        _vmMgr.destroy(vm.getUuid());
+                        _vmMgr.destroy(vm.getUuid(), false);
                     } catch (final Exception e) {
                         final String errorMsg = "There was an error Destory the vm: " + vm + " as a part of hostDelete id=" + host.getId();
                         s_logger.debug(errorMsg, e);
diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java
@@ -759,7 +759,7 @@ protected boolean cleanupAccount(AccountVO account, long callerUserId, Account c
             for (UserVmVO vm : vms) {
                 if (vm.getState() != VirtualMachine.State.Destroyed && vm.getState() != VirtualMachine.State.Expunging) {
                     try {
-                        _vmMgr.destroyVm(vm.getId());
+                        _vmMgr.destroyVm(vm.getId(), false);
                     } catch (Exception e) {
                         e.printStackTrace();
                         s_logger.warn("Failed destroying instance " + vm.getUuid() + " as part of account deletion.");
diff --git a/server/src/com/cloud/vm/UserVmManagerImpl.java b/server/src/com/cloud/vm/UserVmManagerImpl.java
@@ -2627,7 +2627,7 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C
             throw new PermissionDeniedException("Parameter " + ApiConstants.EXPUNGE + " can be passed by Admin only. Or when the allow.user.expunge.recover.vm key is set.");
         }
 
-        UserVm destroyedVm = destroyVm(vmId);
+        UserVm destroyedVm = destroyVm(vmId, expunge);
         if (expunge) {
             UserVmVO vm = _vmDao.findById(vmId);
             if (!expunge(vm, ctx.getCallingUserId(), ctx.getCallingAccount())) {
@@ -4114,7 +4114,7 @@ public Pair<UserVmVO, Map<VirtualMachineProfile.Param, Object>> startVirtualMach
     }
 
     @Override
-    public UserVm destroyVm(long vmId) throws ResourceUnavailableException, ConcurrentOperationException {
+    public UserVm destroyVm(long vmId, boolean expunge) throws ResourceUnavailableException, ConcurrentOperationException {
         // Account caller = CallContext.current().getCallingAccount();
         // Long userId = CallContext.current().getCallingUserId();
         Long userId = 2L;
@@ -4136,7 +4136,7 @@ public UserVm destroyVm(long vmId) throws ResourceUnavailableException, Concurre
 
         try {
             VirtualMachineEntity vmEntity = _orchSrvc.getVirtualMachine(vm.getUuid());
-            status = vmEntity.destroy(Long.toString(userId));
+            status = vmEntity.destroy(Long.toString(userId), expunge);
         } catch (CloudException e) {
             CloudRuntimeException ex = new CloudRuntimeException("Unable to destroy with specified vmId", e);
             ex.addProxyObject(vm.getUuid(), "vmId");
@@ -4318,6 +4318,8 @@ public UserVm expungeVm(long vmId) throws ResourceUnavailableException, Concurre
             throw new PermissionDeniedException("Expunging a vm can only be done by an Admin. Or when the allow.user.expunge.recover.vm key is set.");
         }
 
+        _vmSnapshotMgr.deleteVMSnapshotsFromDB(vmId);
+
         boolean status;
 
         status = expunge(vm, userId, caller);
diff --git a/server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java b/server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java
@@ -1133,4 +1133,25 @@ private VMSnapshotTO getSnapshotWithParents(VMSnapshotVO snapshot) {
         }
         return result;
     }
+
+    @Override
+    public boolean deleteVMSnapshotsFromDB(Long vmId) {
+        List<VMSnapshotVO> listVmSnapshots = _vmSnapshotDao.findByVm(vmId);
+        if (listVmSnapshots == null || listVmSnapshots.isEmpty()) {
+            return true;
+        }
+        for (VMSnapshotVO snapshot : listVmSnapshots) {
+            try {
+                VMSnapshotStrategy strategy = findVMSnapshotStrategy(snapshot);
+                if (! strategy.deleteVMSnapshotFromDB(snapshot)) {
+                    s_logger.error("Couldn't delete vm snapshot with id " + snapshot.getId());
+                    return false;
+                }
+            }
+            catch (CloudRuntimeException e) {
+                s_logger.error("Couldn't delete vm snapshot due to: " + e.getMessage());
+            }
+        }
+        return true;
+    }
 }
diff --git a/server/test/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java b/server/test/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java
@@ -21,6 +21,7 @@
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyLong;
 import static org.mockito.Mockito.anyString;
+import static org.mockito.Mockito.anyBoolean;
 
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
@@ -118,7 +119,7 @@ protected void defineMocksBehavior()
         VirtualMachineEntity vmEntity = mock(VirtualMachineEntity.class);
 
         when(_orchSrvc.getVirtualMachine(anyString())).thenReturn(vmEntity);
-        when(vmEntity.destroy(anyString())).thenReturn(true);
+        when(vmEntity.destroy(anyString(), anyBoolean())).thenReturn(true);
 
         Mockito.doReturn(vm).when(_vmDao).findById(anyLong());
 

Original file line number	Diff line number	Diff line change
`@@ -46,5 +46,5 @@ void deployVirtualMachine(String reservationId, VMEntityVO vmEntityVO, String ca`
`46`	`46`
`47`	`47`	`boolean stopvirtualmachineforced(VMEntityVO vmEntityVO, String caller) throws ResourceUnavailableException;`
`48`	`48`
`49`		`- boolean destroyVirtualMachine(VMEntityVO vmEntityVO, String caller) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException;`
	`49`	`+ boolean destroyVirtualMachine(VMEntityVO vmEntityVO, String caller, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException;`
`50`	`50`	`}`
Original file line number	Diff line number	Diff line change
`@@ -261,10 +261,10 @@ public boolean stopvirtualmachineforced(VMEntityVO vmEntityVO, String caller) th`
`261`	`261`	`}`
`262`	`262`
`263`	`263`	`@Override`
`264`		`- public boolean destroyVirtualMachine(VMEntityVO vmEntityVO, String caller) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {`
	`264`	`+ public boolean destroyVirtualMachine(VMEntityVO vmEntityVO, String caller, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {`
`265`	`265`
`266`	`266`	`VMInstanceVO vm = _vmDao.findByUuid(vmEntityVO.getUuid());`
`267`		`- _itMgr.destroy(vm.getUuid());`
	`267`	`+ _itMgr.destroy(vm.getUuid(), expunge);`
`268`	`268`	`return true;`
`269`	`269`	`}`
`270`	`270`
Original file line number	Diff line number	Diff line change
`@@ -229,8 +229,8 @@ public void cleanup() {`
`229`	`229`	`}`
`230`	`230`
`231`	`231`	`@Override`
`232`		`- public boolean destroy(String caller) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {`
`233`		`- return manager.destroyVirtualMachine(this.vmEntityVO, caller);`
	`232`	`+ public boolean destroy(String caller, boolean expunge) throws AgentUnavailableException, OperationTimedoutException, ConcurrentOperationException {`
	`233`	`+ return manager.destroyVirtualMachine(this.vmEntityVO, caller, expunge);`
`234`	`234`	`}`
`235`	`235`
`236`	`236`	`@Override`