Set root volume as destroyed when destroying a VM (#6868)

* Set root volume as destroyed when destroying a VM

* Address review

* Address review

Co-authored-by: João Jandre <joao@scclouds.com.br>

Author: JoaoJandre

engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java

@@ -146,4 +146,5 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S
      * @return the list of volumes that uses that disk offering.
      */
     List<VolumeVO> findByDiskOfferingId(long diskOfferingId);
+    VolumeVO getInstanceRootVolume(long instanceId);
 }

engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java

@@ -759,4 +759,11 @@ public void updateDiskOffering(long volumeId, long diskOfferingId) {
             throw new CloudRuntimeException(e);
         }
     }
+    @Override
+    public VolumeVO getInstanceRootVolume(long instanceId) {
+        SearchCriteria<VolumeVO> sc = RootDiskStateSearch.create();
+        sc.setParameters("instanceId", instanceId);
+        sc.setParameters("vType", Volume.Type.ROOT);
+        return findOneBy(sc);
+    }
 }

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

@@ -46,6 +46,7 @@
 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<StoragePoolDiscoverer> _discoverers;
 
     public List<StoragePoolDiscoverer> getDiscoverers() {
@@ -1325,6 +1328,15 @@ public void cleanupStorage(boolean recurring) {
 
                     List<VolumeVO> 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.

server/src/main/java/com/cloud/vm/UserVmManager.java

@@ -57,6 +57,10 @@ public interface UserVmManager extends UserVmService {
     ConfigKey<Boolean> DisplayVMOVFProperties = new ConfigKey<Boolean>("Advanced", Boolean.class, "vm.display.ovf.properties", "false",
             "Set display of VMs OVF properties as part of VM details", true);
 
+    ConfigKey<Boolean> DestroyRootVolumeOnVmDestruction = new ConfigKey<Boolean>("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 @@ UserVm updateVirtualMachine(long id, String displayName, String group, Boolean h
 
     boolean checkIfDynamicScalingCanBeEnabled(VirtualMachine vm, ServiceOffering offering, VirtualMachineTemplate template, Long zoneId);
 
+    Boolean getDestroyRootVolumeOnVmDestruction(Long domainId);
+
 }

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<Long, VmAndCountDetails> 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 UserVm recoverVirtualMachine(RecoverVMCmd cmd) throws ResourceAllocationE
                 List<VolumeVO> 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 UserVm recoverVirtualMachine(RecoverVMCmd cmd) throws ResourceAllocationE
         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<String, Object> params) throws ConfigurationException {
         _name = name;
@@ -3330,6 +3341,15 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C
 
         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 String getConfigComponentName() {
     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 @@ private void collectVmDiskAndNetworkStatistics(UserVm vm, State expectedState) {
             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);
+    }
 }

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 void testResetVMUserDataSuccessResetWithUserdataId() {
         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);
+    }
 }