diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index c1d17cc8f70..7bafbcc77b8 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -19,6 +19,7 @@ package com.cloud.storage; 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.List; 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 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; @@ -229,11 +229,11 @@ import com.cloud.utils.db.TransactionLegacy; 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 class StorageManagerImpl extends ManagerBase implements StorageManager, C 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 class StorageManagerImpl extends ManagerBase implements StorageManager, C } } + 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 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. * diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index dc79ac512fa..e7004ba7c5d 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/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 @@ public class StorageManagerImplTest { 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)); + } + }