Merge pull request #1905 from nvazquez/expungeVmRefactor

CLOUDSTACK-9738: [Vmware] Optimize vm expunge process for instances with vm snapshots## Description
It was noticed that expunging instances with many vm snapshots took a look of time, as hypervisor received as many tasks as vm snapshots instance had, apart from the delete vm task. We propose a way to optimize this process for instances with vm snapshots by sending only one delete task to hypervisor, which will delete vm and its snapshots

## Use cases

1. deleteVMsnapohsot-> no changes to current behavior
2. destroyVM with expunge=false ->  no actions to VMsnaphsot is performed at the moment. When VM cleanup thread is executed it will perform the same sequence as (3). If instance is recovered before expunged by the cleanup thread it will remain intact with VMSnapshot chain present
3. destroyVM with expunge=true:
   * Vmsnaphsot is marked with removed timestamp and state = Expunging in DB
   * VM is deleted in HW

* pr/1905:
  CLOUDSTACK-9738: [Vmware] Optimize vm expunge process for instances with vm snapshots

Signed-off-by: Rajani Karuturi <rajani.karuturi@accelerite.com>

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.

api/src/com/cloud/vm/snapshot/VMSnapshotService.java

@@ -46,4 +46,11 @@ public interface VMSnapshotService {
         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);
 }

engine/api/src/com/cloud/vm/VirtualMachineManager.java

@@ -110,7 +110,7 @@ public interface VirtualMachineManager extends Manager {
 
     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;
 

engine/api/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntity.java

@@ -129,8 +129,9 @@ public interface VirtualMachineEntity extends CloudStackEntity {
 
     /**
      * 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

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);
 }

engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java

@@ -1679,7 +1679,7 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
     }
 
     @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 class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         }
 
         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 class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
         }
     }
 
+    /**
+     * 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()) {

engine/orchestration/src/org/apache/cloudstack/engine/cloud/entity/api/VMEntityManager.java

@@ -46,5 +46,5 @@ public interface VMEntityManager {
 
     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;
 }

engine/orchestration/src/org/apache/cloudstack/engine/cloud/entity/api/VMEntityManagerImpl.java

@@ -261,10 +261,10 @@ public class VMEntityManagerImpl implements VMEntityManager {
     }
 
     @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;
     }
 

engine/orchestration/src/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntityImpl.java

@@ -229,8 +229,8 @@ public class VirtualMachineEntityImpl implements VirtualMachineEntity {
     }
 
     @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

engine/storage/snapshot/src/org/apache/cloudstack/storage/vmsnapshot/DefaultVMSnapshotStrategy.java

@@ -392,4 +392,15 @@ public class DefaultVMSnapshotStrategy extends ManagerBase implements 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());
+    }
 }

plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java

@@ -1679,9 +1679,6 @@ public class VmwareStorageProcessor implements StorageProcessor {
                             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);

server/src/com/cloud/ha/HighAvailabilityManagerImpl.java

@@ -674,7 +674,7 @@ public class HighAvailabilityManagerImpl extends ManagerBase implements HighAvai
             }
 
             if (vm.getHostId() != null) {
-                _itMgr.destroy(vm.getUuid());
+                _itMgr.destroy(vm.getUuid(), false);
                 s_logger.info("Successfully destroy " + vm);
                 return null;
             } else {

server/src/com/cloud/network/as/AutoScaleManagerImpl.java

@@ -1514,7 +1514,7 @@ public class AutoScaleManagerImpl<Type> extends ManagerBase implements AutoScale
                     public void run() {
                         try {
 
-                            _userVmManager.destroyVm(vmId);
+                            _userVmManager.destroyVm(vmId, false);
 
                         } catch (ResourceUnavailableException e) {
                             e.printStackTrace();

server/src/com/cloud/resource/ResourceManagerImpl.java

@@ -2165,7 +2165,7 @@ public class ResourceManagerImpl extends ManagerBase implements ResourceManager,
                 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);

server/src/com/cloud/user/AccountManagerImpl.java

@@ -759,7 +759,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
             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.");

server/src/com/cloud/vm/UserVmManagerImpl.java

@@ -2625,7 +2625,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
             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())) {
@@ -4112,7 +4112,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
     }
 
     @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;
@@ -4134,7 +4134,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
 
         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");
@@ -4316,6 +4316,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
             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);

server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java

@@ -1133,4 +1133,25 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme
         }
         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;
+    }
 }

server/test/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java

@@ -21,6 +21,7 @@ import static org.mockito.Mockito.mock;
 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 @@ public class AccountManagerImplVolumeDeleteEventTest extends AccountManagetImplT
         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());