server: improve storage GC to skip expunging possible duplicate volumes

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

@@ -19,6 +19,7 @@
 import static com.cloud.utils.NumbersUtil.toHumanReadableSize;
 
 import java.math.BigDecimal;
+import java.math.BigInteger;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.UnknownHostException;
@@ -37,6 +38,7 @@
 import java.util.Map;
 import java.util.Random;
 import java.util.Set;
+import java.util.UUID;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
@@ -45,8 +47,6 @@
 
 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;
@@ -229,11 +229,11 @@
 import com.cloud.utils.db.TransactionStatus;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.vm.DiskProfile;
+import com.cloud.vm.UserVmManager;
 import com.cloud.vm.VMInstanceVO;
 import com.cloud.vm.VirtualMachine.State;
 import com.cloud.vm.dao.VMInstanceDao;
-import java.math.BigInteger;
-import java.util.UUID;
+import com.google.common.collect.Sets;
 
 @Component
 public class StorageManagerImpl extends ManagerBase implements StorageManager, ClusterManagerListener, Configurable {
@@ -1336,7 +1336,10 @@ public void cleanupStorage(boolean recurring) {
                                  continue;
                              }
                         }
-
+                        if (isVolumeSuspectedDestroyDuplicateOfVmVolume(vol)) {
+                            s_logger.warn(String.format("Skipping cleaning up %s as it could be a duplicate for another volume on same pool", vol));
+                            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.
@@ -1468,6 +1471,31 @@ public void cleanupStorage(boolean recurring) {
         }
     }
 
+    protected boolean isVolumeSuspectedDestroyDuplicateOfVmVolume(VolumeVO gcVolume) {
+        if (gcVolume.getPath() == null) {
+            return false;
+        }
+        if (gcVolume.getPoolId() == null) {
+            return false;
+        }
+        Long vmId = gcVolume.getInstanceId();
+        if (vmId == null) {
+            return false;
+        }
+        VMInstanceVO vm = _vmInstanceDao.findById(vmId);
+        if (vm == null) {
+            return false;
+        }
+        List<VolumeVO> vmUsableVolumes = _volumeDao.findUsableVolumesForInstance(vmId);
+        for (VolumeVO vol : vmUsableVolumes) {
+            if (gcVolume.getPoolId().equals(vol.getPoolId()) && gcVolume.getPath().equals(vol.getPath())) {
+                s_logger.debug(String.format("%s meant for garbage collection could a possible duplicate for %s", gcVolume, vol));
+                return true;
+            }
+        }
+        return false;
+    }
+
     /**
      * This method only applies for managed storage.
      *

server/src/test/java/com/cloud/storage/StorageManagerImplTest.java

@@ -16,20 +16,35 @@
 // under the License.
 package com.cloud.storage;
 
+import java.util.ArrayList;
+import java.util.List;
+
 import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.Spy;
-import org.mockito.runners.MockitoJUnitRunner;
+import org.mockito.junit.MockitoJUnitRunner;
 
 import com.cloud.agent.api.StoragePoolInfo;
 import com.cloud.host.Host;
+import com.cloud.storage.dao.VolumeDao;
+import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.dao.VMInstanceDao;
 
 @RunWith(MockitoJUnitRunner.class)
 public class StorageManagerImplTest {
 
+    @Mock
+    VolumeDao _volumeDao;
+
+    @Mock
+    VMInstanceDao vmInstanceDao;
+
     @Spy
+    @InjectMocks
     private StorageManagerImpl storageManagerImpl;
 
     @Test
@@ -58,4 +73,59 @@ private void executeCreateLocalStoragePoolNameForHostName(String hostMockName) {
         String localStoragePoolName = storageManagerImpl.createLocalStoragePoolName(hostMock, storagePoolInfoMock);
         Assert.assertEquals(expectedLocalStorageName, localStoragePoolName);
     }
+
+    private VolumeVO mockVolumeForIsVolumeSuspectedDestroyDuplicateTest() {
+        VolumeVO volumeVO = new VolumeVO("data", 1L, 1L, 1L, 1L, 1L, "data", "data", Storage.ProvisioningType.THIN, 1, null, null, "data", Volume.Type.DATADISK);
+        volumeVO.setPoolId(1L);
+        return volumeVO;
+    }
+
+    @Test
+    public void testIsVolumeSuspectedDestroyDuplicateNoPool() {
+        VolumeVO volume = mockVolumeForIsVolumeSuspectedDestroyDuplicateTest();
+        volume.setPoolId(null);
+        Assert.assertFalse(storageManagerImpl.isVolumeSuspectedDestroyDuplicateOfVmVolume(volume));
+    }
+
+    @Test
+    public void testIsVolumeSuspectedDestroyDuplicateNoPath() {
+        VolumeVO volume = mockVolumeForIsVolumeSuspectedDestroyDuplicateTest();
+        Assert.assertFalse(storageManagerImpl.isVolumeSuspectedDestroyDuplicateOfVmVolume(volume));
+    }
+
+    @Test
+    public void testIsVolumeSuspectedDestroyDuplicateNoVmId() {
+        VolumeVO volume = mockVolumeForIsVolumeSuspectedDestroyDuplicateTest();
+        volume.setInstanceId(null);
+        Assert.assertFalse(storageManagerImpl.isVolumeSuspectedDestroyDuplicateOfVmVolume(volume));
+    }
+
+    @Test
+    public void testIsVolumeSuspectedDestroyDuplicateNoVm() {
+        VolumeVO volume = mockVolumeForIsVolumeSuspectedDestroyDuplicateTest();
+        Assert.assertFalse(storageManagerImpl.isVolumeSuspectedDestroyDuplicateOfVmVolume(volume));
+    }
+
+    @Test
+    public void testIsVolumeSuspectedDestroyDuplicateNoVmVolumes() {
+        VolumeVO volume = mockVolumeForIsVolumeSuspectedDestroyDuplicateTest();
+        Mockito.when(vmInstanceDao.findById(1L)).thenReturn(Mockito.mock(VMInstanceVO.class));
+        Mockito.when(_volumeDao.findUsableVolumesForInstance(1L)).thenReturn(new ArrayList<>());
+        Assert.assertFalse(storageManagerImpl.isVolumeSuspectedDestroyDuplicateOfVmVolume(volume));
+    }
+
+    @Test
+    public void testIsVolumeSuspectedDestroyDuplicateTrue() {
+        Long poolId = 1L;
+        String path = "data";
+        VolumeVO volume = mockVolumeForIsVolumeSuspectedDestroyDuplicateTest();
+        volume.setPoolId(poolId);
+        Mockito.when(vmInstanceDao.findById(1L)).thenReturn(Mockito.mock(VMInstanceVO.class));
+        VolumeVO volumeVO = Mockito.mock(VolumeVO.class);
+        Mockito.when(volumeVO.getPoolId()).thenReturn(poolId);
+        Mockito.when(volumeVO.getPath()).thenReturn(path);
+        Mockito.when(_volumeDao.findUsableVolumesForInstance(1L)).thenReturn(List.of(volumeVO, Mockito.mock(VolumeVO.class)));
+        Assert.assertTrue(storageManagerImpl.isVolumeSuspectedDestroyDuplicateOfVmVolume(volume));
+    }
+
 }