KVM: assign device ID 0 to root volumes imported from shared/local storage

importVm with importsource=shared or importsource=local allocated the root
volume with device ID 0, then overwrote it with an explicit ID 1 when
finalizing the imported volume. Pass no device ID so the volume manager keeps
the root default (0), as the other import paths do.

Fixes #13771

Author: andrijapanicsb

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<VolumeVO> 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);

server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java

@@ -3014,17 +3014,16 @@ private UserVm importKvmVirtualMachineFromDisk(final ImportSource importSource,
 
             List<Pair<DiskProfile, StoragePool>> 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);

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<Volume, StoragePool> 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