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 c809201247 Fix: Volumes on lost local storage cannot be removed (#7594)
c809201247 is described below
commit c80920124731c2ff4f12f0178bbe1707b06a94e5
Author: Nicolas Vazquez <[email protected]>
AuthorDate: Fri Jun 23 07:22:15 2023 -0300
Fix: Volumes on lost local storage cannot be removed (#7594)
---
.../main/java/com/cloud/storage/dao/VolumeDao.java | 2 +
.../java/com/cloud/storage/dao/VolumeDaoImpl.java | 11 +++++
.../com/cloud/resource/ResourceManagerImpl.java | 27 ++++++++++++
.../com/cloud/storage/VolumeApiServiceImpl.java | 21 +++++++++
.../cloud/resource/ResourceManagerImplTest.java | 51 ++++++++++++++++++++++
.../cloud/storage/VolumeApiServiceImplTest.java | 18 +++++++-
6 files changed, 129 insertions(+), 1 deletion(-)
diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java
b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java
index 2001cf05ab..3cdaa3b05a 100644
--- a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java
+++ b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java
@@ -147,4 +147,6 @@ public interface VolumeDao extends GenericDao<VolumeVO,
Long>, StateDao<Volume.S
*/
List<VolumeVO> findByDiskOfferingId(long diskOfferingId);
VolumeVO getInstanceRootVolume(long instanceId);
+
+ void updateAndRemoveVolume(VolumeVO volume);
}
diff --git
a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java
b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java
index 3c865bac66..eb3206fc42 100644
--- a/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java
+++ b/engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java
@@ -766,4 +766,15 @@ public class VolumeDaoImpl extends
GenericDaoBase<VolumeVO, Long> implements Vol
sc.setParameters("vType", Volume.Type.ROOT);
return findOneBy(sc);
}
+
+ @Override
+ public void updateAndRemoveVolume(VolumeVO volume) {
+ if (volume.getState() != Volume.State.Destroy) {
+ volume.setState(Volume.State.Destroy);
+ volume.setPoolId(null);
+ volume.setInstanceId(null);
+ update(volume.getId(), volume);
+ remove(volume.getId());
+ }
+ }
}
diff --git a/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
b/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
index 3462908274..d552c12e1a 100755
--- a/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
+++ b/server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
@@ -38,6 +38,9 @@ import javax.naming.ConfigurationException;
import com.cloud.exception.StorageConflictException;
import com.cloud.exception.StorageUnavailableException;
+import com.cloud.storage.Volume;
+import com.cloud.storage.VolumeVO;
+import com.cloud.storage.dao.VolumeDao;
import org.apache.cloudstack.annotation.AnnotationService;
import org.apache.cloudstack.annotation.dao.AnnotationDao;
import org.apache.cloudstack.api.ApiConstants;
@@ -294,6 +297,8 @@ public class ResourceManagerImpl extends ManagerBase
implements ResourceManager,
private UserVmDetailsDao userVmDetailsDao;
@Inject
private AnnotationDao annotationDao;
+ @Inject
+ private VolumeDao volumeDao;
private final long _nodeId = ManagementServerNode.getManagementServerId();
@@ -979,6 +984,7 @@ public class ResourceManagerImpl extends ManagerBase
implements ResourceManager,
final Long poolId = pool.getPoolId();
final StoragePoolVO storagePool =
_storagePoolDao.findById(poolId);
if (storagePool.isLocal() && isForceDeleteStorage) {
+ destroyLocalStoragePoolVolumes(poolId);
storagePool.setUuid(null);
storagePool.setClusterId(null);
_storagePoolDao.update(poolId, storagePool);
@@ -1011,6 +1017,27 @@ public class ResourceManagerImpl extends ManagerBase
implements ResourceManager,
return true;
}
+ private void addVolumesToList(List<VolumeVO> volumes, List<VolumeVO>
volumesToAdd) {
+ if (CollectionUtils.isNotEmpty(volumesToAdd)) {
+ volumes.addAll(volumesToAdd);
+ }
+ }
+
+ protected void destroyLocalStoragePoolVolumes(long poolId) {
+ List<VolumeVO> rootDisks = volumeDao.findByPoolId(poolId);
+ List<VolumeVO> dataVolumes = volumeDao.findByPoolId(poolId,
Volume.Type.DATADISK);
+
+ List<VolumeVO> volumes = new ArrayList<>();
+ addVolumesToList(volumes, rootDisks);
+ addVolumesToList(volumes, dataVolumes);
+
+ if (CollectionUtils.isNotEmpty(volumes)) {
+ for (VolumeVO volume : volumes) {
+ volumeDao.updateAndRemoveVolume(volume);
+ }
+ }
+ }
+
/**
* Returns true if host can be deleted.</br>
* A host can be deleted either if it is in Maintenance or "Degraded"
state.
diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
index 646b81960d..fd234b2479 100644
--- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
+++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
@@ -36,6 +36,7 @@ import javax.inject.Inject;
import org.apache.cloudstack.api.ApiConstants.IoDriverPolicy;
import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.InternalIdentity;
import org.apache.cloudstack.api.ServerApiException;
import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd;
import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd;
@@ -88,6 +89,7 @@ import org.apache.cloudstack.storage.command.AttachAnswer;
import org.apache.cloudstack.storage.command.AttachCommand;
import org.apache.cloudstack.storage.command.DettachCommand;
import org.apache.cloudstack.storage.command.TemplateOrVolumePostUploadCommand;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
@@ -273,6 +275,8 @@ public class VolumeApiServiceImpl extends ManagerBase
implements VolumeApiServic
@Inject
private PrimaryDataStoreDao _storagePoolDao;
@Inject
+ private ImageStoreDao imageStoreDao;
+ @Inject
private DiskOfferingDao _diskOfferingDao;
@Inject
private ServiceOfferingDao _serviceOfferingDao;
@@ -1619,6 +1623,12 @@ public class VolumeApiServiceImpl extends ManagerBase
implements VolumeApiServic
}
private void expungeVolumesInPrimaryOrSecondary(VolumeVO volume,
DataStoreRole role) throws InterruptedException, ExecutionException {
+ if (!canAccessVolumeStore(volume, role)) {
+ s_logger.debug(String.format("Cannot access the storage pool with
role: %s " +
+ "for the volume: %s, skipping expunge from
storage",
+ role.name(), volume.getName()));
+ return;
+ }
VolumeInfo volOnStorage = volFactory.getVolume(volume.getId(), role);
if (volOnStorage != null) {
s_logger.info("Expunging volume " + volume.getId() + " from " +
role + " data store");
@@ -1638,6 +1648,17 @@ public class VolumeApiServiceImpl extends ManagerBase
implements VolumeApiServic
}
}
}
+
+ private boolean canAccessVolumeStore(VolumeVO volume, DataStoreRole role) {
+ if (volume == null) {
+ throw new CloudRuntimeException("No volume given, cannot check
access to volume store");
+ }
+ InternalIdentity pool = role == DataStoreRole.Primary ?
+ _storagePoolDao.findById(volume.getPoolId()) :
+ imageStoreDao.findById(volume.getPoolId());
+ return pool != null;
+ }
+
/**
* Clean volumes cache entries (if they exist).
*/
diff --git
a/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java
b/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java
index 1793456ab9..a7ddd16462 100644
--- a/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java
+++ b/server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java
@@ -34,9 +34,13 @@ import static org.mockito.Mockito.when;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import java.util.UUID;
+import com.cloud.storage.Volume;
+import com.cloud.storage.VolumeVO;
+import com.cloud.storage.dao.VolumeDao;
import org.apache.cloudstack.api.command.admin.host.CancelHostAsDegradedCmd;
import org.apache.cloudstack.api.command.admin.host.DeclareHostAsDegradedCmd;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
@@ -98,6 +102,8 @@ public class ResourceManagerImplTest {
private VMInstanceDao vmInstanceDao;
@Mock
private ConfigurationDao configurationDao;
+ @Mock
+ private VolumeDao volumeDao;
@Spy
@InjectMocks
@@ -119,6 +125,13 @@ public class ResourceManagerImplTest {
@Mock
private GetVncPortCommand getVncPortCommandVm2;
+ @Mock
+ private VolumeVO rootDisk1;
+ @Mock
+ private VolumeVO rootDisk2;
+ @Mock
+ private VolumeVO dataDisk;
+
@Mock
private Connection sshConnection;
@@ -138,6 +151,10 @@ public class ResourceManagerImplTest {
private static String vm2VncAddress = "10.2.2.2";
private static int vm2VncPort = 5901;
+ private static long poolId = 1L;
+ private List<VolumeVO> rootDisks;
+ private List<VolumeVO> dataDisks;
+
@Before
public void setup() throws Exception {
MockitoAnnotations.initMocks(this);
@@ -179,6 +196,11 @@ public class ResourceManagerImplTest {
willReturn(new SSHCmdHelper.SSHCmdResult(0,"",""));
when(configurationDao.getValue(ResourceManager.KvmSshToAgentEnabled.key())).thenReturn("true");
+
+ rootDisks = Arrays.asList(rootDisk1, rootDisk2);
+ dataDisks = Collections.singletonList(dataDisk);
+ when(volumeDao.findByPoolId(poolId)).thenReturn(rootDisks);
+ when(volumeDao.findByPoolId(poolId,
Volume.Type.DATADISK)).thenReturn(dataDisks);
}
@Test
@@ -527,4 +549,33 @@ public class ResourceManagerImplTest {
return new HostVO(1L, "host01", Host.Type.Routing, "192.168.1.1",
"255.255.255.0", null, null, null, null, null, null, null, null, null, null,
UUID.randomUUID().toString(),
hostStatus, "1.0", null, null, 1L, null, 0, 0, null, 0, null);
}
+
+ @Test
+ public void testDestroyLocalStoragePoolVolumesBothRootDisksAndDataDisks() {
+ resourceManager.destroyLocalStoragePoolVolumes(poolId);
+ verify(volumeDao, times(rootDisks.size() + dataDisks.size()))
+ .updateAndRemoveVolume(any(VolumeVO.class));
+ }
+
+ @Test
+ public void testDestroyLocalStoragePoolVolumesOnlyRootDisks() {
+ when(volumeDao.findByPoolId(poolId,
Volume.Type.DATADISK)).thenReturn(null);
+ resourceManager.destroyLocalStoragePoolVolumes(poolId);
+ verify(volumeDao,
times(rootDisks.size())).updateAndRemoveVolume(any(VolumeVO.class));
+ }
+
+ @Test
+ public void testDestroyLocalStoragePoolVolumesOnlyDataDisks() {
+ when(volumeDao.findByPoolId(poolId)).thenReturn(null);
+ resourceManager.destroyLocalStoragePoolVolumes(poolId);
+ verify(volumeDao,
times(dataDisks.size())).updateAndRemoveVolume(any(VolumeVO.class));
+ }
+
+ @Test
+ public void testDestroyLocalStoragePoolVolumesNoDisks() {
+ when(volumeDao.findByPoolId(poolId)).thenReturn(null);
+ when(volumeDao.findByPoolId(poolId,
Volume.Type.DATADISK)).thenReturn(null);
+ resourceManager.destroyLocalStoragePoolVolumes(poolId);
+ verify(volumeDao, never()).updateAndRemoveVolume(any(VolumeVO.class));
+ }
}
diff --git
a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
index 16c7887ef9..e5b85c829e 100644
--- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
+++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
@@ -52,6 +52,8 @@ import
org.apache.cloudstack.framework.jobs.AsyncJobExecutionContext;
import org.apache.cloudstack.framework.jobs.AsyncJobManager;
import org.apache.cloudstack.framework.jobs.dao.AsyncJobJoinMapDao;
import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreDao;
@@ -143,6 +145,12 @@ public class VolumeApiServiceImplTest {
@Mock
private PrimaryDataStoreDao primaryDataStoreDaoMock;
@Mock
+ private ImageStoreDao imageStoreDao;
+ @Mock
+ private ImageStoreVO imageStoreVO;
+ @Mock
+ private StoragePoolVO storagePoolVO;
+ @Mock
private VMSnapshotDao _vmSnapshotDao;
@Mock
private AsyncJobManager _jobMgr;
@@ -214,6 +222,7 @@ public class VolumeApiServiceImplTest {
private long volumeMockId = 12313l;
private long vmInstanceMockId = 1123l;
private long volumeSizeMock = 456789921939l;
+ private static long imageStoreId = 10L;
private String projectMockUuid = "projectUuid";
private long projecMockId = 13801801923810L;
@@ -239,6 +248,10 @@ public class VolumeApiServiceImplTest {
Mockito.when(storagePoolMock.getId()).thenReturn(storagePoolMockId);
+ Mockito.when(volumeVoMock.getPoolId()).thenReturn(storagePoolMockId);
+
Mockito.when(imageStoreDao.findById(imageStoreId)).thenReturn(imageStoreVO);
+
Mockito.when(primaryDataStoreDaoMock.findById(storagePoolMockId)).thenReturn(storagePoolVO);
+
volumeApiServiceImpl._gson = GsonHelper.getGsonLogger();
// mock caller context
@@ -914,7 +927,7 @@ public class VolumeApiServiceImplTest {
@Test
public void
expungeVolumesInSecondaryStorageIfNeededTestVolumeNotFoundInSecondaryStorage()
throws InterruptedException, ExecutionException {
Mockito.lenient().doReturn(asyncCallFutureVolumeapiResultMock).when(volumeServiceMock).expungeVolumeAsync(volumeInfoMock);
-
Mockito.doReturn(null).when(volumeDataFactoryMock).getVolume(volumeMockId,
DataStoreRole.Image);
+
Mockito.lenient().doReturn(null).when(imageStoreDao).findById(imageStoreId);
Mockito.lenient().doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId,
ResourceType.secondary_storage, volumeSizeMock);
Mockito.lenient().doReturn(accountMockId).when(volumeInfoMock).getAccountId();
Mockito.lenient().doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
@@ -933,6 +946,7 @@ public class VolumeApiServiceImplTest {
Mockito.doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId,
ResourceType.secondary_storage, volumeSizeMock);
Mockito.doReturn(accountMockId).when(volumeInfoMock).getAccountId();
Mockito.doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
+ Mockito.doReturn(imageStoreId).when(volumeVoMock).getPoolId();
volumeApiServiceImpl.expungeVolumesInSecondaryStorageIfNeeded(volumeVoMock);
@@ -948,6 +962,7 @@ public class VolumeApiServiceImplTest {
Mockito.lenient().doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId,
ResourceType.secondary_storage, volumeSizeMock);
Mockito.lenient().doReturn(accountMockId).when(volumeInfoMock).getAccountId();
Mockito.lenient().doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
+ Mockito.doReturn(imageStoreId).when(volumeVoMock).getPoolId();
Mockito.doThrow(InterruptedException.class).when(asyncCallFutureVolumeapiResultMock).get();
@@ -962,6 +977,7 @@ public class VolumeApiServiceImplTest {
Mockito.lenient().doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId,
ResourceType.secondary_storage, volumeSizeMock);
Mockito.lenient().doReturn(accountMockId).when(volumeInfoMock).getAccountId();
Mockito.lenient().doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
+ Mockito.doReturn(imageStoreId).when(volumeVoMock).getPoolId();
Mockito.doThrow(ExecutionException.class).when(asyncCallFutureVolumeapiResultMock).get();
