From 1b4a4567d28fc39b4a524cb688102d2aa4a99643 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 6 Mar 2023 14:26:59 +0530 Subject: [PATCH 1/2] server: improve storage GC to not expunge volumes which could be duplicate Signed-off-by: Abhishek Kumar --- .../com/cloud/storage/StorageManagerImpl.java | 34 ++++++++++++++++--- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index c1d17cc8f70b..4383dd45c816 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 @@ 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; @@ -232,8 +232,7 @@ 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 +1335,10 @@ public void cleanupStorage(boolean recurring) { continue; } } - + if (isVolumeSuspectedDestroyDuplicate(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 +1470,28 @@ public void cleanupStorage(boolean recurring) { } } + private boolean isVolumeSuspectedDestroyDuplicate(VolumeVO gcVolume) { + if (Volume.State.Destroy.equals(gcVolume.getState())) { + 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() != null && 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. * From 8a0564d8b56d882edbd95d682394c30d98877388 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Tue, 23 May 2023 14:18:24 +0530 Subject: [PATCH 2/2] fix Signed-off-by: Abhishek Kumar --- .../com/cloud/storage/StorageManagerImpl.java | 12 ++-- .../cloud/storage/StorageManagerImplTest.java | 72 ++++++++++++++++++- 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 4383dd45c816..7bafbcc77b81 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -229,6 +229,7 @@ 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; @@ -1335,7 +1336,7 @@ public void cleanupStorage(boolean recurring) { continue; } } - if (isVolumeSuspectedDestroyDuplicate(vol)) { + 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; } @@ -1470,8 +1471,11 @@ public void cleanupStorage(boolean recurring) { } } - private boolean isVolumeSuspectedDestroyDuplicate(VolumeVO gcVolume) { - if (Volume.State.Destroy.equals(gcVolume.getState())) { + protected boolean isVolumeSuspectedDestroyDuplicateOfVmVolume(VolumeVO gcVolume) { + if (gcVolume.getPath() == null) { + return false; + } + if (gcVolume.getPoolId() == null) { return false; } Long vmId = gcVolume.getInstanceId(); @@ -1484,7 +1488,7 @@ private boolean isVolumeSuspectedDestroyDuplicate(VolumeVO gcVolume) { } List vmUsableVolumes = _volumeDao.findUsableVolumesForInstance(vmId); for (VolumeVO vol : vmUsableVolumes) { - if (gcVolume.getPoolId().equals(vol.getPoolId()) && gcVolume.getPath() != null && gcVolume.getPath().equals(vol.getPath())) { + 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; } diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index dc79ac512fa5..e7004ba7c5d3 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 @@ 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)); + } + }