Skip to content

Commit

Permalink
Fix: Volumes on lost local storage cannot be removed (#7594)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nvazquez
nvazquez committed Jun 23, 2023
1 parent 0acc66f commit c809201
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 1 deletion.
2 changes: 2 additions & 0 deletions engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
11 changes: 11 additions & 0 deletions engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -766,4 +766,15 @@ public VolumeVO getInstanceRootVolume(long instanceId) {
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());
}
}
}
27 changes: 27 additions & 0 deletions server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@

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;
Expand Down Expand Up @@ -294,6 +297,8 @@ public void setDiscoverers(final List<? extends Discoverer> discoverers) {
private UserVmDetailsDao userVmDetailsDao;
@Inject
private AnnotationDao annotationDao;
@Inject
private VolumeDao volumeDao;

private final long _nodeId = ManagementServerNode.getManagementServerId();

Expand Down Expand Up @@ -979,6 +984,7 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
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);
Expand Down Expand Up @@ -1011,6 +1017,27 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
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.
Expand Down
21 changes: 21 additions & 0 deletions server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

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;
Expand Down Expand Up @@ -88,6 +89,7 @@
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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1619,6 +1623,12 @@ protected void expungeVolumesInSecondaryStorageIfNeeded(VolumeVO volume) throws
}

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");
Expand All @@ -1638,6 +1648,17 @@ private void expungeVolumesInPrimaryOrSecondary(VolumeVO volume, DataStoreRole r
}
}
}

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).
*/
Expand Down
51 changes: 51 additions & 0 deletions server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,13 @@

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;
Expand Down Expand Up @@ -98,6 +102,8 @@ public class ResourceManagerImplTest {
private VMInstanceDao vmInstanceDao;
@Mock
private ConfigurationDao configurationDao;
@Mock
private VolumeDao volumeDao;

@Spy
@InjectMocks
Expand All @@ -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;

Expand All @@ -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);
Expand Down Expand Up @@ -179,6 +196,11 @@ public void setup() throws Exception {
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
Expand Down Expand Up @@ -527,4 +549,33 @@ private HostVO createDummyHost(Status hostStatus) {
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));
}
}
18 changes: 17 additions & 1 deletion server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@
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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -239,6 +248,10 @@ public void setup() throws InterruptedException, ExecutionException {

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
Expand Down Expand Up @@ -914,7 +927,7 @@ public void expungeVolumesInPrimaryStorageIfNeededTestThrowingExecutionException
@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();
Expand All @@ -933,6 +946,7 @@ public void expungeVolumesInSecondaryStorageIfNeededTestVolumeFoundInSecondarySt
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);

Expand All @@ -948,6 +962,7 @@ public void expungeVolumesInSecondaryStorageIfNeededTestThrowinInterruptedExcept
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();

Expand All @@ -962,6 +977,7 @@ public void expungeVolumesInSecondaryStorageIfNeededTestThrowingExecutionExcepti
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();

Expand Down

0 comments on commit c809201

Please sign in to comment.