Skip to content

Commit c809201

Browse files
nvazqueznvazquez
authored
Fix: Volumes on lost local storage cannot be removed (#7594)
1 parent 0acc66f commit c809201

File tree

6 files changed

+129
-1
lines changed

6 files changed

+129
-1
lines changed

‎engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,4 +147,6 @@ public interface VolumeDao extends GenericDao<VolumeVO, Long>, StateDao<Volume.S
147147
*/
148148
List<VolumeVO> findByDiskOfferingId(long diskOfferingId);
149149
VolumeVO getInstanceRootVolume(long instanceId);
150+
151+
void updateAndRemoveVolume(VolumeVO volume);
150152
}

‎engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -766,4 +766,15 @@ public VolumeVO getInstanceRootVolume(long instanceId) {
766766
sc.setParameters("vType", Volume.Type.ROOT);
767767
return findOneBy(sc);
768768
}
769+
770+
@Override
771+
public void updateAndRemoveVolume(VolumeVO volume) {
772+
if (volume.getState() != Volume.State.Destroy) {
773+
volume.setState(Volume.State.Destroy);
774+
volume.setPoolId(null);
775+
volume.setInstanceId(null);
776+
update(volume.getId(), volume);
777+
remove(volume.getId());
778+
}
779+
}
769780
}

‎server/src/main/java/com/cloud/resource/ResourceManagerImpl.java‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@
3838

3939
import com.cloud.exception.StorageConflictException;
4040
import com.cloud.exception.StorageUnavailableException;
41+
import com.cloud.storage.Volume;
42+
import com.cloud.storage.VolumeVO;
43+
import com.cloud.storage.dao.VolumeDao;
4144
import org.apache.cloudstack.annotation.AnnotationService;
4245
import org.apache.cloudstack.annotation.dao.AnnotationDao;
4346
import org.apache.cloudstack.api.ApiConstants;
@@ -294,6 +297,8 @@ public void setDiscoverers(final List<? extends Discoverer> discoverers) {
294297
private UserVmDetailsDao userVmDetailsDao;
295298
@Inject
296299
private AnnotationDao annotationDao;
300+
@Inject
301+
private VolumeDao volumeDao;
297302

298303
private final long _nodeId = ManagementServerNode.getManagementServerId();
299304

@@ -979,6 +984,7 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
979984
final Long poolId = pool.getPoolId();
980985
final StoragePoolVO storagePool = _storagePoolDao.findById(poolId);
981986
if (storagePool.isLocal() && isForceDeleteStorage) {
987+
destroyLocalStoragePoolVolumes(poolId);
982988
storagePool.setUuid(null);
983989
storagePool.setClusterId(null);
984990
_storagePoolDao.update(poolId, storagePool);
@@ -1011,6 +1017,27 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
10111017
return true;
10121018
}
10131019

1020+
private void addVolumesToList(List<VolumeVO> volumes, List<VolumeVO> volumesToAdd) {
1021+
if (CollectionUtils.isNotEmpty(volumesToAdd)) {
1022+
volumes.addAll(volumesToAdd);
1023+
}
1024+
}
1025+
1026+
protected void destroyLocalStoragePoolVolumes(long poolId) {
1027+
List<VolumeVO> rootDisks = volumeDao.findByPoolId(poolId);
1028+
List<VolumeVO> dataVolumes = volumeDao.findByPoolId(poolId, Volume.Type.DATADISK);
1029+
1030+
List<VolumeVO> volumes = new ArrayList<>();
1031+
addVolumesToList(volumes, rootDisks);
1032+
addVolumesToList(volumes, dataVolumes);
1033+
1034+
if (CollectionUtils.isNotEmpty(volumes)) {
1035+
for (VolumeVO volume : volumes) {
1036+
volumeDao.updateAndRemoveVolume(volume);
1037+
}
1038+
}
1039+
}
1040+
10141041
/**
10151042
* Returns true if host can be deleted.</br>
10161043
* A host can be deleted either if it is in Maintenance or "Degraded" state.

‎server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636

3737
import org.apache.cloudstack.api.ApiConstants.IoDriverPolicy;
3838
import org.apache.cloudstack.api.ApiErrorCode;
39+
import org.apache.cloudstack.api.InternalIdentity;
3940
import org.apache.cloudstack.api.ServerApiException;
4041
import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd;
4142
import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd;
@@ -88,6 +89,7 @@
8889
import org.apache.cloudstack.storage.command.AttachCommand;
8990
import org.apache.cloudstack.storage.command.DettachCommand;
9091
import org.apache.cloudstack.storage.command.TemplateOrVolumePostUploadCommand;
92+
import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
9193
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
9294
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
9395
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
@@ -273,6 +275,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
273275
@Inject
274276
private PrimaryDataStoreDao _storagePoolDao;
275277
@Inject
278+
private ImageStoreDao imageStoreDao;
279+
@Inject
276280
private DiskOfferingDao _diskOfferingDao;
277281
@Inject
278282
private ServiceOfferingDao _serviceOfferingDao;
@@ -1619,6 +1623,12 @@ protected void expungeVolumesInSecondaryStorageIfNeeded(VolumeVO volume) throws
16191623
}
16201624

16211625
private void expungeVolumesInPrimaryOrSecondary(VolumeVO volume, DataStoreRole role) throws InterruptedException, ExecutionException {
1626+
if (!canAccessVolumeStore(volume, role)) {
1627+
s_logger.debug(String.format("Cannot access the storage pool with role: %s " +
1628+
"for the volume: %s, skipping expunge from storage",
1629+
role.name(), volume.getName()));
1630+
return;
1631+
}
16221632
VolumeInfo volOnStorage = volFactory.getVolume(volume.getId(), role);
16231633
if (volOnStorage != null) {
16241634
s_logger.info("Expunging volume " + volume.getId() + " from " + role + " data store");
@@ -1638,6 +1648,17 @@ private void expungeVolumesInPrimaryOrSecondary(VolumeVO volume, DataStoreRole r
16381648
}
16391649
}
16401650
}
1651+
1652+
private boolean canAccessVolumeStore(VolumeVO volume, DataStoreRole role) {
1653+
if (volume == null) {
1654+
throw new CloudRuntimeException("No volume given, cannot check access to volume store");
1655+
}
1656+
InternalIdentity pool = role == DataStoreRole.Primary ?
1657+
_storagePoolDao.findById(volume.getPoolId()) :
1658+
imageStoreDao.findById(volume.getPoolId());
1659+
return pool != null;
1660+
}
1661+
16411662
/**
16421663
* Clean volumes cache entries (if they exist).
16431664
*/

‎server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,13 @@
3434

3535
import java.util.ArrayList;
3636
import java.util.Arrays;
37+
import java.util.Collections;
3738
import java.util.List;
3839
import java.util.UUID;
3940

41+
import com.cloud.storage.Volume;
42+
import com.cloud.storage.VolumeVO;
43+
import com.cloud.storage.dao.VolumeDao;
4044
import org.apache.cloudstack.api.command.admin.host.CancelHostAsDegradedCmd;
4145
import org.apache.cloudstack.api.command.admin.host.DeclareHostAsDegradedCmd;
4246
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
@@ -98,6 +102,8 @@ public class ResourceManagerImplTest {
98102
private VMInstanceDao vmInstanceDao;
99103
@Mock
100104
private ConfigurationDao configurationDao;
105+
@Mock
106+
private VolumeDao volumeDao;
101107

102108
@Spy
103109
@InjectMocks
@@ -119,6 +125,13 @@ public class ResourceManagerImplTest {
119125
@Mock
120126
private GetVncPortCommand getVncPortCommandVm2;
121127

128+
@Mock
129+
private VolumeVO rootDisk1;
130+
@Mock
131+
private VolumeVO rootDisk2;
132+
@Mock
133+
private VolumeVO dataDisk;
134+
122135
@Mock
123136
private Connection sshConnection;
124137

@@ -138,6 +151,10 @@ public class ResourceManagerImplTest {
138151
private static String vm2VncAddress = "10.2.2.2";
139152
private static int vm2VncPort = 5901;
140153

154+
private static long poolId = 1L;
155+
private List<VolumeVO> rootDisks;
156+
private List<VolumeVO> dataDisks;
157+
141158
@Before
142159
public void setup() throws Exception {
143160
MockitoAnnotations.initMocks(this);
@@ -179,6 +196,11 @@ public void setup() throws Exception {
179196
willReturn(new SSHCmdHelper.SSHCmdResult(0,"",""));
180197

181198
when(configurationDao.getValue(ResourceManager.KvmSshToAgentEnabled.key())).thenReturn("true");
199+
200+
rootDisks = Arrays.asList(rootDisk1, rootDisk2);
201+
dataDisks = Collections.singletonList(dataDisk);
202+
when(volumeDao.findByPoolId(poolId)).thenReturn(rootDisks);
203+
when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(dataDisks);
182204
}
183205

184206
@Test
@@ -527,4 +549,33 @@ private HostVO createDummyHost(Status hostStatus) {
527549
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(),
528550
hostStatus, "1.0", null, null, 1L, null, 0, 0, null, 0, null);
529551
}
552+
553+
@Test
554+
public void testDestroyLocalStoragePoolVolumesBothRootDisksAndDataDisks() {
555+
resourceManager.destroyLocalStoragePoolVolumes(poolId);
556+
verify(volumeDao, times(rootDisks.size() + dataDisks.size()))
557+
.updateAndRemoveVolume(any(VolumeVO.class));
558+
}
559+
560+
@Test
561+
public void testDestroyLocalStoragePoolVolumesOnlyRootDisks() {
562+
when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(null);
563+
resourceManager.destroyLocalStoragePoolVolumes(poolId);
564+
verify(volumeDao, times(rootDisks.size())).updateAndRemoveVolume(any(VolumeVO.class));
565+
}
566+
567+
@Test
568+
public void testDestroyLocalStoragePoolVolumesOnlyDataDisks() {
569+
when(volumeDao.findByPoolId(poolId)).thenReturn(null);
570+
resourceManager.destroyLocalStoragePoolVolumes(poolId);
571+
verify(volumeDao, times(dataDisks.size())).updateAndRemoveVolume(any(VolumeVO.class));
572+
}
573+
574+
@Test
575+
public void testDestroyLocalStoragePoolVolumesNoDisks() {
576+
when(volumeDao.findByPoolId(poolId)).thenReturn(null);
577+
when(volumeDao.findByPoolId(poolId, Volume.Type.DATADISK)).thenReturn(null);
578+
resourceManager.destroyLocalStoragePoolVolumes(poolId);
579+
verify(volumeDao, never()).updateAndRemoveVolume(any(VolumeVO.class));
580+
}
530581
}

‎server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@
5252
import org.apache.cloudstack.framework.jobs.AsyncJobManager;
5353
import org.apache.cloudstack.framework.jobs.dao.AsyncJobJoinMapDao;
5454
import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO;
55+
import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
56+
import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
5557
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
5658
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
5759
import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreDao;
@@ -143,6 +145,12 @@ public class VolumeApiServiceImplTest {
143145
@Mock
144146
private PrimaryDataStoreDao primaryDataStoreDaoMock;
145147
@Mock
148+
private ImageStoreDao imageStoreDao;
149+
@Mock
150+
private ImageStoreVO imageStoreVO;
151+
@Mock
152+
private StoragePoolVO storagePoolVO;
153+
@Mock
146154
private VMSnapshotDao _vmSnapshotDao;
147155
@Mock
148156
private AsyncJobManager _jobMgr;
@@ -214,6 +222,7 @@ public class VolumeApiServiceImplTest {
214222
private long volumeMockId = 12313l;
215223
private long vmInstanceMockId = 1123l;
216224
private long volumeSizeMock = 456789921939l;
225+
private static long imageStoreId = 10L;
217226

218227
private String projectMockUuid = "projectUuid";
219228
private long projecMockId = 13801801923810L;
@@ -239,6 +248,10 @@ public void setup() throws InterruptedException, ExecutionException {
239248

240249
Mockito.when(storagePoolMock.getId()).thenReturn(storagePoolMockId);
241250

251+
Mockito.when(volumeVoMock.getPoolId()).thenReturn(storagePoolMockId);
252+
Mockito.when(imageStoreDao.findById(imageStoreId)).thenReturn(imageStoreVO);
253+
Mockito.when(primaryDataStoreDaoMock.findById(storagePoolMockId)).thenReturn(storagePoolVO);
254+
242255
volumeApiServiceImpl._gson = GsonHelper.getGsonLogger();
243256

244257
// mock caller context
@@ -914,7 +927,7 @@ public void expungeVolumesInPrimaryStorageIfNeededTestThrowingExecutionException
914927
@Test
915928
public void expungeVolumesInSecondaryStorageIfNeededTestVolumeNotFoundInSecondaryStorage() throws InterruptedException, ExecutionException {
916929
Mockito.lenient().doReturn(asyncCallFutureVolumeapiResultMock).when(volumeServiceMock).expungeVolumeAsync(volumeInfoMock);
917-
Mockito.doReturn(null).when(volumeDataFactoryMock).getVolume(volumeMockId, DataStoreRole.Image);
930+
Mockito.lenient().doReturn(null).when(imageStoreDao).findById(imageStoreId);
918931
Mockito.lenient().doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId, ResourceType.secondary_storage, volumeSizeMock);
919932
Mockito.lenient().doReturn(accountMockId).when(volumeInfoMock).getAccountId();
920933
Mockito.lenient().doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
@@ -933,6 +946,7 @@ public void expungeVolumesInSecondaryStorageIfNeededTestVolumeFoundInSecondarySt
933946
Mockito.doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId, ResourceType.secondary_storage, volumeSizeMock);
934947
Mockito.doReturn(accountMockId).when(volumeInfoMock).getAccountId();
935948
Mockito.doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
949+
Mockito.doReturn(imageStoreId).when(volumeVoMock).getPoolId();
936950

937951
volumeApiServiceImpl.expungeVolumesInSecondaryStorageIfNeeded(volumeVoMock);
938952

@@ -948,6 +962,7 @@ public void expungeVolumesInSecondaryStorageIfNeededTestThrowinInterruptedExcept
948962
Mockito.lenient().doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId, ResourceType.secondary_storage, volumeSizeMock);
949963
Mockito.lenient().doReturn(accountMockId).when(volumeInfoMock).getAccountId();
950964
Mockito.lenient().doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
965+
Mockito.doReturn(imageStoreId).when(volumeVoMock).getPoolId();
951966

952967
Mockito.doThrow(InterruptedException.class).when(asyncCallFutureVolumeapiResultMock).get();
953968

@@ -962,6 +977,7 @@ public void expungeVolumesInSecondaryStorageIfNeededTestThrowingExecutionExcepti
962977
Mockito.lenient().doNothing().when(resourceLimitServiceMock).decrementResourceCount(accountMockId, ResourceType.secondary_storage, volumeSizeMock);
963978
Mockito.lenient().doReturn(accountMockId).when(volumeInfoMock).getAccountId();
964979
Mockito.lenient().doReturn(volumeSizeMock).when(volumeInfoMock).getSize();
980+
Mockito.doReturn(imageStoreId).when(volumeVoMock).getPoolId();
965981

966982
Mockito.doThrow(ExecutionException.class).when(asyncCallFutureVolumeapiResultMock).get();
967983

0 commit comments

Comments
 (0)