diff --git a/api/src/com/cloud/vm/UserVmService.java b/api/src/com/cloud/vm/UserVmService.java index 54206ed2395..db9ded89a98 100644 --- 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 index 0d4d22c7cb1..e376265acfa 100644 --- a/api/src/com/cloud/vm/snapshot/VMSnapshotService.java +++ b/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); } diff --git a/engine/api/src/com/cloud/vm/VirtualMachineManager.java b/engine/api/src/com/cloud/vm/VirtualMachineManager.java index b205415c0c6..4b61caaf921 100644 --- a/engine/api/src/com/cloud/vm/VirtualMachineManager.java +++ b/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; 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 index 1c3af621f7d..de91ea79dfb 100644 --- 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 @@ 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 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 index 239ba87e040..38f633aefb3 100644 --- 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 old mode 100644 new mode 100755 index b1c69b2c22a..d19aaf372cc --- a/engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/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()) { 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 index 0c357ca0313..7e3edc69243 100644 --- 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 @@ 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; } 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 index a944b937342..1911645f5a9 100644 --- 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 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; } 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 index 2f4de36b05a..7a8f624d325 100644 --- 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 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 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 index 13fd54cf8f7..71a5e104d31 100644 --- 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 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()); + } } diff --git a/plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java b/plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java index 8ce65c42f60..e790d47303b 100644 --- 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 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); diff --git a/server/src/com/cloud/ha/HighAvailabilityManagerImpl.java b/server/src/com/cloud/ha/HighAvailabilityManagerImpl.java index 53d79f71079..fe6b7171968 100644 --- a/server/src/com/cloud/ha/HighAvailabilityManagerImpl.java +++ b/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 { diff --git a/server/src/com/cloud/network/as/AutoScaleManagerImpl.java b/server/src/com/cloud/network/as/AutoScaleManagerImpl.java index 657d4d4b88e..d9113607772 100644 --- a/server/src/com/cloud/network/as/AutoScaleManagerImpl.java +++ b/server/src/com/cloud/network/as/AutoScaleManagerImpl.java @@ -1514,7 +1514,7 @@ public class AutoScaleManagerImpl extends ManagerBase implements AutoScale 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 index 2bb1596ed1c..2efce066b0c 100644 --- a/server/src/com/cloud/resource/ResourceManagerImpl.java +++ b/server/src/com/cloud/resource/ResourceManagerImpl.java @@ -2165,7 +2165,7 @@ public class ResourceManagerImpl extends ManagerBase implements ResourceManager, final List 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 index c7a6ef66c34..1a3607f1f3f 100644 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/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."); diff --git a/server/src/com/cloud/vm/UserVmManagerImpl.java b/server/src/com/cloud/vm/UserVmManagerImpl.java old mode 100644 new mode 100755 index ec7a6f6ff62..dedeab18db2 --- a/server/src/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/com/cloud/vm/UserVmManagerImpl.java @@ -2627,7 +2627,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())) { @@ -4114,7 +4114,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; @@ -4136,7 +4136,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"); @@ -4318,6 +4318,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); diff --git a/server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java b/server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java index d155186c1db..5f9115f2b7d 100644 --- a/server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java +++ b/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 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 index 43c8cfcacce..239771361f7 100644 --- 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.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());