diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java index 259dfaa6b1fa..d2f0286b2914 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java @@ -42,7 +42,10 @@ import com.cloud.uservm.UserVm; import com.cloud.utils.db.EntityManager; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.DiskProfile; +import com.cloud.vm.UserVmVO; import com.cloud.vm.VirtualMachine; +import com.cloud.vm.dao.UserVmDao; import com.cloud.utils.Pair; import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; @@ -67,6 +70,7 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.MockedConstruction; @@ -108,6 +112,9 @@ public class VolumeOrchestratorTest { @Mock private ImageStoreDao imageStoreDaoMock; + @Mock + private UserVmDao userVmDao; + @Spy @InjectMocks @@ -255,6 +262,37 @@ public void testImportVolume() { Mockito.verify(volume, Mockito.times(1)).setState(Volume.State.Ready); } + @Test + public void testUpdateImportedVolumeGivesRootDeviceIdZeroWhenNoneIsGiven() { + updateImportedVolumeAndAssertSavedDeviceId(Type.ROOT, null, 0L); + } + + @Test + public void testUpdateImportedVolumeKeepsExplicitDeviceId() { + updateImportedVolumeAndAssertSavedDeviceId(Type.DATADISK, 2L, 2L); + } + + private void updateImportedVolumeAndAssertSavedDeviceId(Type type, Long requestedDeviceId, long expectedDeviceId) { + // behaves like a real VolumeVO, but is not created through its constructor, so that the VolumeVO + // construction mock opened (and never closed) by testImportVolume cannot replace it + VolumeVO volume = Mockito.mock(VolumeVO.class, Mockito.CALLS_REAL_METHODS); + volume.setDeviceId(1L); + DiskProfile diskProfile = Mockito.mock(DiskProfile.class); + Mockito.when(diskProfile.getVolumeId()).thenReturn(10L); + Mockito.when(volumeDao.findById(10L)).thenReturn(volume); + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Mockito.when(vm.getId()).thenReturn(MOCK_VM_ID); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(userVmDao.findById(MOCK_VM_ID)).thenReturn(Mockito.mock(UserVmVO.class)); + + volumeOrchestrator.updateImportedVolume(type, Mockito.mock(DiskOffering.class), vm, null, requestedDeviceId, + MOCK_POOL_ID, Storage.StoragePoolType.NetworkFilesystem, "/mnt/pool/disk.qcow2", null, diskProfile, Storage.ImageFormat.QCOW2); + + ArgumentCaptor savedVolume = ArgumentCaptor.forClass(VolumeVO.class); + Mockito.verify(volumeDao).update(Mockito.anyLong(), savedVolume.capture()); + Assert.assertEquals(Long.valueOf(expectedDeviceId), savedVolume.getValue().getDeviceId()); + } + @Test public void testAllocateDuplicateVolumeVOBasic() { Volume oldVol = Mockito.mock(Volume.class); diff --git a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java index 355457d7c8ae..92bd523e816d 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -3014,17 +3014,16 @@ private UserVm importKvmVirtualMachineFromDisk(final ImportSource importSource, List> diskProfileStoragePoolList = new ArrayList<>(); try { - long deviceId = 1L; Storage.ImageFormat diskFormat = getImageFormatFromVolumeDetails(volumeDetails); if (storagePool.getPoolType() == Storage.StoragePoolType.StorPool) { diskFormat = Storage.ImageFormat.QCOW2; } if(ImportSource.SHARED == importSource) { diskProfileStoragePoolList.add(importKVMSharedDisk(userVm, diskOffering, Volume.Type.ROOT, - template, deviceId, poolId, diskPath, diskProfile, diskFormat)); + template, null, poolId, diskPath, diskProfile, diskFormat)); } else if(ImportSource.LOCAL == importSource) { diskProfileStoragePoolList.add(importKVMLocalDisk(userVm, diskOffering, Volume.Type.ROOT, - template, deviceId, hostId, diskPath, diskProfile, diskFormat)); + template, null, hostId, diskPath, diskProfile, diskFormat)); } } catch (Exception e) { logger.error(String.format("Failed to import volumes while importing vm: %s", instanceName), e); diff --git a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java index b6376201c738..5a9e7619b06d 100644 --- a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java @@ -1018,7 +1018,7 @@ private void importFromDisk(String source, Storage.StoragePoolType poolType, Str DiskProfile diskProfile = Mockito.mock(DiskProfile.class); when(volumeManager.allocateRawVolume(any(), any(), any(), any(), any(), any(), any(), any(), any(), any(), anyBoolean())) .thenReturn(diskProfile); - when(volumeManager.updateImportedVolume(any(), any(), any(), any(), anyLong(), anyLong(), any(), anyString(), any(), any(), any())) + when(volumeManager.updateImportedVolume(any(), any(), any(), any(), nullable(Long.class), anyLong(), any(), anyString(), any(), any(), any())) .thenReturn(diskProfile); Map storage = new HashMap<>(); VolumeVO volume = Mockito.mock(VolumeVO.class); @@ -1046,8 +1046,9 @@ private void importFromDisk(String source, Storage.StoragePoolType poolType, Str unmanagedVMsManager.importVm(cmd); } // the imported volume must record the format the agent reported for the image on the pool, - // not the hypervisor default, so an RBD image is stored as RAW - verify(volumeManager).updateImportedVolume(any(), any(), any(), any(), anyLong(), anyLong(), Mockito.eq(poolType), + // not the hypervisor default, so an RBD image is stored as RAW; it is the ROOT volume and passes + // no device id, so the volume manager keeps the root default (0) + verify(volumeManager).updateImportedVolume(Mockito.eq(Volume.Type.ROOT), any(), any(), any(), Mockito.isNull(), anyLong(), Mockito.eq(poolType), Mockito.eq(diskPath), Mockito.isNull(), any(), Mockito.eq(expectedFormat)); // the storage driver must be told the volume is now imported, so e.g. StorPool can tag it