This is an automated email from the ASF dual-hosted git repository.
dahn pushed a commit to branch 4.18
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.18 by this push:
new 2d6a0698121 server: improve storage GC to skip expunging possible
duplicate volumes (#7313)
2d6a0698121 is described below
commit 2d6a06981217c8f399aef4ba0a3b41b5655f1a64
Author: Abhishek Kumar <[email protected]>
AuthorDate: Mon Jun 5 13:33:24 2023 +0530
server: improve storage GC to skip expunging possible duplicate volumes
(#7313)
Signed-off-by: Abhishek Kumar <[email protected]>
---
.../java/com/cloud/storage/StorageManagerImpl.java | 38 ++++++++++--
.../com/cloud/storage/StorageManagerImplTest.java | 72 +++++++++++++++++++++-
2 files changed, 104 insertions(+), 6 deletions(-)
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<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.
*
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));
+ }
+
}
