From 7c61d8aeaff7ecd496ec6e7f4a6d2885a3dfe174 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Jandre?= <48719461+JoaoJandre@users.noreply.github.com> Date: Tue, 6 Dec 2022 17:48:35 -0300 Subject: [PATCH] Set root volume as destroyed when destroying a VM (#6868) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Set root volume as destroyed when destroying a VM * Address review * Address review Co-authored-by: João Jandre --- .../java/com/cloud/storage/dao/VolumeDao.java | 1 + .../com/cloud/storage/dao/VolumeDaoImpl.java | 7 +++++ .../com/cloud/storage/StorageManagerImpl.java | 12 ++++++++ .../main/java/com/cloud/vm/UserVmManager.java | 6 ++++ .../java/com/cloud/vm/UserVmManagerImpl.java | 30 +++++++++++++++++-- .../com/cloud/vm/UserVmManagerImplTest.java | 18 +++++++++++ 6 files changed, 71 insertions(+), 3 deletions(-) diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java index 64151d60687..2001cf05ab9 100644 --- a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java +++ b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java @@ -146,4 +146,5 @@ public interface VolumeDao extends GenericDao, StateDao findByDiskOfferingId(long diskOfferingId); + VolumeVO getInstanceRootVolume(long instanceId); } diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java index d27faa3a78a..3c865bac663 100644 --- a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java @@ -759,4 +759,11 @@ public class VolumeDaoImpl extends GenericDaoBase implements Vol throw new CloudRuntimeException(e); } } + @Override + public VolumeVO getInstanceRootVolume(long instanceId) { + SearchCriteria sc = RootDiskStateSearch.create(); + sc.setParameters("instanceId", instanceId); + sc.setParameters("vType", Volume.Type.ROOT); + return findOneBy(sc); + } } diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 00a15e6e32c..0018cc49d11 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -46,6 +46,7 @@ import java.util.stream.Collectors; import javax.inject.Inject; import com.google.common.collect.Sets; +import com.cloud.vm.UserVmManager; import org.apache.cloudstack.annotation.AnnotationService; import org.apache.cloudstack.annotation.dao.AnnotationDao; import org.apache.cloudstack.api.ApiConstants; @@ -344,6 +345,8 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C @Inject private AnnotationDao annotationDao; + @Inject + protected UserVmManager userVmManager; protected List _discoverers; public List getDiscoverers() { @@ -1325,6 +1328,15 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C List vols = _volsDao.listVolumesToBeDestroyed(new Date(System.currentTimeMillis() - ((long)StorageCleanupDelay.value() << 10))); for (VolumeVO vol : vols) { + if (Type.ROOT.equals(vol.getVolumeType())) { + VMInstanceVO vmInstanceVO = _vmInstanceDao.findById(vol.getInstanceId()); + if (vmInstanceVO != null && vmInstanceVO.getState() == State.Destroyed) { + s_logger.debug(String.format("ROOT volume [%s] will not be expunged because the VM is [%s], therefore this volume will be expunged with the VM" + + " cleanup job.", vol.getUuid(), vmInstanceVO.getState())); + continue; + } + } + try { // If this fails, just log a warning. It's ideal if we clean up the host-side clustered file // system, but not necessary. diff --git a/server/src/main/java/com/cloud/vm/UserVmManager.java b/server/src/main/java/com/cloud/vm/UserVmManager.java index 082c36f518e..5029615b241 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManager.java +++ b/server/src/main/java/com/cloud/vm/UserVmManager.java @@ -57,6 +57,10 @@ public interface UserVmManager extends UserVmService { ConfigKey DisplayVMOVFProperties = new ConfigKey("Advanced", Boolean.class, "vm.display.ovf.properties", "false", "Set display of VMs OVF properties as part of VM details", true); + ConfigKey DestroyRootVolumeOnVmDestruction = new ConfigKey("Advanced", Boolean.class, "destroy.root.volume.on.vm.destruction", "false", + "Destroys the VM's root volume when the VM is destroyed.", + true, ConfigKey.Scope.Domain); + static final int MAX_USER_DATA_LENGTH_BYTES = 2048; public static final String CKS_NODE = "cksnode"; @@ -136,4 +140,6 @@ public interface UserVmManager extends UserVmService { boolean checkIfDynamicScalingCanBeEnabled(VirtualMachine vm, ServiceOffering offering, VirtualMachineTemplate template, Long zoneId); + Boolean getDestroyRootVolumeOnVmDestruction(Long domainId); + } diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 4e4f38e1af0..caa2459f5a9 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -596,6 +596,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir private int _scaleRetry; private Map vmIdCountMap = new ConcurrentHashMap<>(); + protected static long ROOT_DEVICE_ID = 0; + private static final int MAX_HTTP_GET_LENGTH = 2 * MAX_USER_DATA_LENGTH_BYTES; private static final int NUM_OF_2K_BLOCKS = 512; private static final int MAX_HTTP_POST_LENGTH = NUM_OF_2K_BLOCKS * MAX_USER_DATA_LENGTH_BYTES; @@ -2333,8 +2335,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir List volumes = _volsDao.findByInstance(vmId); for (VolumeVO volume : volumes) { if (volume.getVolumeType().equals(Volume.Type.ROOT)) { - // Create an event - _volumeService.publishVolumeCreationUsageEvent(volume); + recoverRootVolume(volume, vmId); + break; } } @@ -2346,6 +2348,15 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir return _vmDao.findById(vmId); } + protected void recoverRootVolume(VolumeVO volume, Long vmId) { + if (Volume.State.Destroy.equals(volume.getState())) { + _volumeService.recoverVolume(volume.getId()); + _volsDao.attachVolume(volume.getId(), vmId, ROOT_DEVICE_ID); + } else { + _volumeService.publishVolumeCreationUsageEvent(volume); + } + } + @Override public boolean configure(String name, Map params) throws ConfigurationException { _name = name; @@ -3330,6 +3341,15 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir deleteVolumesFromVm(volumesToBeDeleted, expunge); + if (getDestroyRootVolumeOnVmDestruction(vm.getDomainId())) { + VolumeVO rootVolume = _volsDao.getInstanceRootVolume(vm.getId()); + if (rootVolume != null) { + _volService.destroyVolume(rootVolume.getId()); + } else { + s_logger.warn(String.format("Tried to destroy ROOT volume for VM [%s], but couldn't retrieve it.", vm.getUuid())); + } + } + return destroyedVm; } @@ -8004,7 +8024,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir public ConfigKey[] getConfigKeys() { return new ConfigKey[] {EnableDynamicallyScaleVm, AllowDiskOfferingChangeDuringScaleVm, AllowUserExpungeRecoverVm, VmIpFetchWaitInterval, VmIpFetchTrialMax, VmIpFetchThreadPoolMax, VmIpFetchTaskWorkers, AllowDeployVmIfGivenHostFails, EnableAdditionalVmConfig, DisplayVMOVFProperties, - KvmAdditionalConfigAllowList, XenServerAdditionalConfigAllowList, VmwareAdditionalConfigAllowList}; + KvmAdditionalConfigAllowList, XenServerAdditionalConfigAllowList, VmwareAdditionalConfigAllowList, DestroyRootVolumeOnVmDestruction}; } @Override @@ -8334,4 +8354,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir s_logger.warn(String.format("Skip collecting vm %s disk and network statistics as the expected vm state is %s but actual state is %s", vm, expectedState, vm.getState())); } } + + public Boolean getDestroyRootVolumeOnVmDestruction(Long domainId){ + return DestroyRootVolumeOnVmDestruction.valueIn(domainId); + } } diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 2e6f72e67b3..06a56e45f12 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -16,6 +16,8 @@ // under the License. package com.cloud.vm; +import com.cloud.storage.Volume; +import com.cloud.storage.dao.VolumeDao; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -166,6 +168,12 @@ public class UserVmManagerImplTest { @Mock UserDataDao userDataDao; + @Mock + private VolumeVO volumeVOMock; + + @Mock + private VolumeDao volumeDaoMock; + private long vmId = 1l; private static final long GiB_TO_BYTES = 1024 * 1024 * 1024; @@ -856,4 +864,14 @@ public class UserVmManagerImplTest { Assert.assertEquals("testUserdata", userVmVO.getUserData()); Assert.assertEquals(1L, (long)userVmVO.getUserDataId()); } + + @Test + public void recoverRootVolumeTestDestroyState() { + Mockito.doReturn(Volume.State.Destroy).when(volumeVOMock).getState(); + + userVmManagerImpl.recoverRootVolume(volumeVOMock, vmId); + + Mockito.verify(volumeApiService).recoverVolume(volumeVOMock.getId()); + Mockito.verify(volumeDaoMock).attachVolume(volumeVOMock.getId(), vmId, UserVmManagerImpl.ROOT_DEVICE_ID); + } }