Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: fix pod retrieval during volume attach #10324

Draft
wants to merge 2 commits into
base: 4.19
Choose a base branch
from
Draft

server: fix pod retrieval during volume attach #10324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25f7494
server: fix pod retrieval during volume attach
shwstppr Feb 4, 2025
daa379e
changes for attaching allocated vol to a stopped vm
shwstppr Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2426,10 +2426,10 @@ protected VolumeVO getVmExistingVolumeForVolumeAttach(UserVmVO vm, VolumeInfo vo
return existingVolumeOfVm;
}

protected StoragePool getPoolForAllocatedOrUploadedVolumeForAttach(final VolumeInfo volumeToAttach, final UserVmVO vm) {
protected StoragePool getSuitablePoolForAllocatedOrUploadedVolumeForAttach(final VolumeInfo volumeToAttach, final UserVmVO vm) {
DataCenter zone = _dcDao.findById(vm.getDataCenterId());
Pair<Long, Long> clusterHostId = virtualMachineManager.findClusterAndHostIdForVm(vm, false);
long podId = vm.getPodIdToDeployIn();
Long podId = vm.getPodIdToDeployIn();
if (clusterHostId.first() != null) {
Cluster cluster = clusterDao.findById(clusterHostId.first());
podId = cluster.getPodId();
Expand All @@ -2441,12 +2441,8 @@ protected StoragePool getPoolForAllocatedOrUploadedVolumeForAttach(final VolumeI
offering.isUseLocalStorage(), offering.isRecreatable(),
volumeToAttach.getTemplateId());
diskProfile.setHyperType(vm.getHypervisorType());
StoragePool pool = _volumeMgr.findStoragePool(diskProfile, zone, pod, clusterHostId.first(),
return _volumeMgr.findStoragePool(diskProfile, zone, pod, clusterHostId.first(),
clusterHostId.second(), vm, Collections.emptySet());
if (pool == null) {
throw new CloudRuntimeException(String.format("Failed to find a primary storage for volume in state: %s", volumeToAttach.getState()));
}
return pool;
}

protected VolumeInfo createVolumeOnPrimaryForAttachIfNeeded(final VolumeInfo volumeToAttach, final UserVmVO vm, VolumeVO existingVolumeOfVm) {
Expand All @@ -2464,7 +2460,13 @@ protected VolumeInfo createVolumeOnPrimaryForAttachIfNeeded(final VolumeInfo vol
}
}
if (destPrimaryStorage == null) {
destPrimaryStorage = getPoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
destPrimaryStorage = getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
if (destPrimaryStorage == null) {
if (Volume.State.Allocated.equals(volumeToAttach.getState()) && State.Stopped.equals(vm.getState())) {
return newVolumeOnPrimaryStorage;
}
throw new CloudRuntimeException(String.format("Failed to find a primary storage for volume in state: %s", volumeToAttach.getState()));
}
}
try {
if (volumeOnSecondary && Storage.StoragePoolType.PowerFlex.equals(destPrimaryStorage.getPoolType())) {
Expand Down
51 changes: 41 additions & 10 deletions server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1938,13 +1938,13 @@ public void testGetPoolForAllocatedOrUploadedVolumeForAttach_Success() {
when(volumeToAttach.getDiskOfferingId()).thenReturn(1L);
when(volumeOrchestrationService.findStoragePool(any(DiskProfile.class), eq(zone), eq(pod), eq(1L), eq(2L), eq(vm), eq(Collections.emptySet())))
.thenReturn(pool);
StoragePool result = volumeApiServiceImpl.getPoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
StoragePool result = volumeApiServiceImpl.getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
Assert.assertNotNull(result);
Assert.assertEquals(pool, result);
}

@Test(expected = CloudRuntimeException.class)
public void testGetPoolForAllocatedOrUploadedVolumeForAttach_NoPoolFound_ThrowsException() {
@Test
public void testGetPoolForAllocatedOrUploadedVolumeForAttach_NoSuitablePoolFound_ReturnsNull() {
VolumeInfo volumeToAttach = Mockito.mock(VolumeInfo.class);
UserVmVO vm = Mockito.mock(UserVmVO.class);
DataCenterVO zone = mockZone();
Expand All @@ -1959,11 +1959,11 @@ public void testGetPoolForAllocatedOrUploadedVolumeForAttach_NoPoolFound_ThrowsE
when(volumeToAttach.getDiskOfferingId()).thenReturn(1L);
when(volumeOrchestrationService.findStoragePool(any(DiskProfile.class), eq(zone), eq(pod), eq(1L), eq(2L), eq(vm), eq(Collections.emptySet())))
.thenReturn(null);
volumeApiServiceImpl.getPoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
Assert.assertNull(volumeApiServiceImpl.getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm));
}

@Test
public void testGetPoolForAllocatedOrUploadedVolumeForAttach_NoCluster() {
public void testGetSuitablePoolForAllocatedOrUploadedVolumeForAttach_NoCluster() {
VolumeInfo volumeToAttach = Mockito.mock(VolumeInfo.class);
UserVmVO vm = Mockito.mock(UserVmVO.class);
DataCenterVO zone = mockZone();
Expand All @@ -1977,7 +1977,7 @@ public void testGetPoolForAllocatedOrUploadedVolumeForAttach_NoCluster() {
when(volumeToAttach.getDiskOfferingId()).thenReturn(1L);
when(volumeOrchestrationService.findStoragePool(any(DiskProfile.class), eq(zone), eq(pod), eq(null), eq(2L), eq(vm), eq(Collections.emptySet())))
.thenReturn(pool);
StoragePool result = volumeApiServiceImpl.getPoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
StoragePool result = volumeApiServiceImpl.getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
Assert.assertNotNull(result);
Assert.assertEquals(pool, result);
}
Expand Down Expand Up @@ -2023,7 +2023,7 @@ public void testCreateVolumeOnPrimaryForAttachIfNeeded_UsesGetPoolForAttach() {
UserVmVO vm = Mockito.mock(UserVmVO.class);
StoragePool destPrimaryStorage = Mockito.mock(StoragePool.class);
Mockito.doReturn(destPrimaryStorage).when(volumeApiServiceImpl)
.getPoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
.getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
VolumeInfo newVolumeOnPrimaryStorage = Mockito.mock(VolumeInfo.class);
try {
Mockito.when(volumeOrchestrationService.createVolumeOnPrimaryStorage(
Expand All @@ -2034,7 +2034,7 @@ public void testCreateVolumeOnPrimaryForAttachIfNeeded_UsesGetPoolForAttach() {
}
VolumeInfo result = volumeApiServiceImpl.createVolumeOnPrimaryForAttachIfNeeded(volumeToAttach, vm, null);
Assert.assertSame(newVolumeOnPrimaryStorage, result);
verify(volumeApiServiceImpl).getPoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
verify(volumeApiServiceImpl).getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
}

@Test(expected = InvalidParameterValueException.class)
Expand All @@ -2045,7 +2045,7 @@ public void testCreateVolumeOnPrimaryForAttachIfNeeded_UnsupportedPoolType_Throw
StoragePool destPrimaryStorage = Mockito.mock(StoragePool.class);
when(destPrimaryStorage.getPoolType()).thenReturn(Storage.StoragePoolType.PowerFlex);
Mockito.doReturn(destPrimaryStorage).when(volumeApiServiceImpl)
.getPoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
.getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
volumeApiServiceImpl.createVolumeOnPrimaryForAttachIfNeeded(volumeToAttach, vm, null);
}

Expand All @@ -2057,7 +2057,7 @@ public void testCreateVolumeOnSecondaryForAttachIfNeeded_CreateVolumeFails_Throw
StoragePool destPrimaryStorage = Mockito.mock(StoragePool.class);
Mockito.when(destPrimaryStorage.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem);
Mockito.doReturn(destPrimaryStorage).when(volumeApiServiceImpl)
.getPoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
.getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
try {
Mockito.when(volumeOrchestrationService.createVolumeOnPrimaryStorage(vm, volumeToAttach, vm.getHypervisorType(), destPrimaryStorage))
.thenThrow(new NoTransitionException("Mocked exception"));
Expand All @@ -2069,4 +2069,35 @@ public void testCreateVolumeOnSecondaryForAttachIfNeeded_CreateVolumeFails_Throw
);
Assert.assertTrue(exception.getMessage().contains("Failed to create volume on primary storage"));
}

@Test
public void testCreateVolumeOnSecondaryForAttachIfNeeded_NoSuitablePool_ThrowsException() {
VolumeInfo volumeToAttach = Mockito.mock(VolumeInfo.class);
Mockito.when(volumeToAttach.getState()).thenReturn(Volume.State.Uploaded);
UserVmVO vm = Mockito.mock(UserVmVO.class);
Mockito.doReturn(null).when(volumeApiServiceImpl)
.getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
CloudRuntimeException exception = Assert.assertThrows(CloudRuntimeException.class, () ->
volumeApiServiceImpl.createVolumeOnPrimaryForAttachIfNeeded(volumeToAttach, vm, null)
);
Assert.assertTrue(exception.getMessage().startsWith("Failed to find a primary storage for volume"));
}

@Test
public void testCreateVolumeOnSecondaryForAttachIfNeeded_NoSuitablePool_ReturnSameVolumeInfo() {
VolumeInfo volumeToAttach = Mockito.mock(VolumeInfo.class);
Mockito.when(volumeToAttach.getState()).thenReturn(Volume.State.Allocated);
UserVmVO vm = Mockito.mock(UserVmVO.class);
Mockito.when(vm.getState()).thenReturn(State.Stopped);
Mockito.doReturn(null).when(volumeApiServiceImpl)
.getSuitablePoolForAllocatedOrUploadedVolumeForAttach(volumeToAttach, vm);
VolumeInfo result = volumeApiServiceImpl.createVolumeOnPrimaryForAttachIfNeeded(volumeToAttach, vm, null);
Assert.assertSame(volumeToAttach, result);
try {
Mockito.verify(volumeOrchestrationService, Mockito.never()).createVolumeOnPrimaryStorage(Mockito.any(),
Mockito.any(), Mockito.any(), Mockito.any());
} catch (NoTransitionException e) {
Assert.fail();
}
}
}
Loading