importVM improvements: RBD support, volume format, and cluster selection

api/src/main/java/org/apache/cloudstack/api/command/admin/vm/ImportVmCmd.java

@@ -92,7 +92,7 @@ public class ImportVmCmd extends ImportUnmanagedInstanceCmd {
 
     @Parameter(name = ApiConstants.DISK_PATH,
             type = CommandType.STRING,
-            description = "path of the disk image")
+            description = "path of the disk image. It is the file name on file based storage pools (NFS, Local, SharedMountPoint), and the image name on RBD storage pools")
     private String diskPath;
 
     @Parameter(name = ApiConstants.IMPORT_SOURCE,

api/src/main/java/org/apache/cloudstack/api/command/admin/volume/ImportVolumeCmd.java

@@ -63,7 +63,7 @@ public class ImportVolumeCmd extends BaseAsyncCmd {
     @Parameter(name = ApiConstants.PATH,
             type = BaseCmd.CommandType.STRING,
             required = true,
-            description = "the path of the volume")
+            description = "the path of the volume. It is the file name on file based storage pools (NFS, Local, SharedMountPoint), and the image name on RBD storage pools")
     private String path;
 
     @Parameter(name = ApiConstants.NAME,

engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/VolumeOrchestrationService.java

@@ -183,10 +183,12 @@ List<DiskProfile> allocateTemplatedVolumes(Type type, String name, DiskOffering
      */
     DiskProfile importVolume(Type type, String name, DiskOffering offering, Long sizeInBytes, Long minIops, Long maxIops,
                              Long zoneId, HypervisorType hypervisorType, VirtualMachine vm, VirtualMachineTemplate template,
-                             Account owner, Long deviceId, Long poolId, Storage.StoragePoolType poolType, String path, String chainInfo);
+                             Account owner, Long deviceId, Long poolId, Storage.StoragePoolType poolType, String path, String chainInfo,
+                             Storage.ImageFormat format);
 
     DiskProfile updateImportedVolume(Type type, DiskOffering offering, VirtualMachine vm, VirtualMachineTemplate template,
-                                     Long deviceId, Long poolId, Storage.StoragePoolType poolType, String path, String chainInfo, DiskProfile diskProfile);
+                                     Long deviceId, Long poolId, Storage.StoragePoolType poolType, String path, String chainInfo, DiskProfile diskProfile,
+                                     Storage.ImageFormat format);
 
     /**
      * Unmanage VM volumes

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java

@@ -2328,7 +2328,8 @@ public void updateVolumeDiskChain(long volumeId, String path, String chainInfo,
     @Override
     public DiskProfile importVolume(Type type, String name, DiskOffering offering, Long sizeInBytes, Long minIops, Long maxIops,
                                     Long zoneId, HypervisorType hypervisorType, VirtualMachine vm, VirtualMachineTemplate template, Account owner,
-                                    Long deviceId, Long poolId, Storage.StoragePoolType poolType, String path, String chainInfo) {
+                                    Long deviceId, Long poolId, Storage.StoragePoolType poolType, String path, String chainInfo,
+                                    ImageFormat format) {
         if (sizeInBytes == null) {
             sizeInBytes = offering.getDiskSize();
         }
@@ -2367,7 +2368,9 @@ public DiskProfile importVolume(Type type, String name, DiskOffering offering, L
             vol.setDisplayVolume(userVm.isDisplayVm());
         }
 
-        vol.setFormat(getSupportedImageFormatForCluster(hypervisorType));
+        // The format the hypervisor actually reported for the existing image wins; pools such as RBD
+        // hold raw images even though QCOW2 is the cluster default for KVM.
+        vol.setFormat(format != null ? format : getSupportedImageFormatForCluster(hypervisorType));
         vol.setPoolId(poolId);
         vol.setPoolType(poolType);
         vol.setPath(path);
@@ -2379,7 +2382,8 @@ public DiskProfile importVolume(Type type, String name, DiskOffering offering, L
 
     @Override
     public DiskProfile updateImportedVolume(Type type, DiskOffering offering, VirtualMachine vm, VirtualMachineTemplate template,
-                                    Long deviceId, Long poolId, Storage.StoragePoolType poolType, String path, String chainInfo, DiskProfile diskProfile) {
+                                    Long deviceId, Long poolId, Storage.StoragePoolType poolType, String path, String chainInfo, DiskProfile diskProfile,
+                                    ImageFormat format) {
 
         VolumeVO vol = _volsDao.findById(diskProfile.getVolumeId());
         if (vm != null) {
@@ -2411,7 +2415,9 @@ public DiskProfile updateImportedVolume(Type type, DiskOffering offering, Virtua
             vol.setDisplayVolume(userVm.isDisplayVm());
         }
 
-        vol.setFormat(getSupportedImageFormatForCluster(vm.getHypervisorType()));
+        // The format the hypervisor actually reported for the existing image wins; pools such as RBD
+        // hold raw images even though QCOW2 is the cluster default for KVM.
+        vol.setFormat(format != null ? format : getSupportedImageFormatForCluster(vm.getHypervisorType()));
         vol.setPoolId(poolId);
         vol.setPoolType(poolType);
         vol.setPath(path);

engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java

@@ -241,7 +241,7 @@ public void testImportVolume() {
 
         volumeOrchestrator.importVolume(volumeType, name, diskOffering, sizeInBytes, null, null,
                 zoneId, hypervisorType, null, null, owner,
-                deviceId, poolId, Storage.StoragePoolType.NetworkFilesystem, path, chainInfo);
+                deviceId, poolId, Storage.StoragePoolType.NetworkFilesystem, path, chainInfo, null);
 
         VolumeVO volume = volumeVOMockedConstructionConstruction.constructed().get(0);
         Mockito.verify(volume, Mockito.never()).setInstanceId(Mockito.anyLong());

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeCommandWrapper.java

@@ -50,7 +50,8 @@ public final class LibvirtCheckVolumeCommandWrapper extends CommandWrapper<Check
     private static final List<Storage.StoragePoolType> STORAGE_POOL_TYPES_SUPPORTED = Arrays.asList(
             Storage.StoragePoolType.Filesystem,
             Storage.StoragePoolType.NetworkFilesystem,
-            Storage.StoragePoolType.SharedMountPoint);
+            Storage.StoragePoolType.SharedMountPoint,
+            Storage.StoragePoolType.RBD);
 
     @Override
     public Answer execute(final CheckVolumeCommand command, final LibvirtComputingResource libvirtComputingResource) {
@@ -64,14 +65,25 @@ public Answer execute(final CheckVolumeCommand command, final LibvirtComputingRe
             if (STORAGE_POOL_TYPES_SUPPORTED.contains(storageFilerTO.getType())) {
                 final KVMPhysicalDisk vol = pool.getPhysicalDisk(srcFile);
                 final String path = vol.getPath();
-                try {
-                    KVMPhysicalDisk.checkQcow2File(path);
-                } catch (final CloudRuntimeException e) {
-                    return new CheckVolumeAnswer(command, false, "", 0, getVolumeDetails(pool, vol));
+                final boolean isRbd = Storage.StoragePoolType.RBD.equals(storageFilerTO.getType());
+
+                Map<VolumeOnStorageTO.Detail, String> volumeDetails = getVolumeDetails(pool, vol);
+                if (MapUtils.isEmpty(volumeDetails)) {
+                    return new Answer(command, false, "Unable to read the volume on the storage pool");
+                }
+
+                if (!isRbd) {
+                    try {
+                        KVMPhysicalDisk.checkQcow2File(path);
+                    } catch (final CloudRuntimeException e) {
+                        return new CheckVolumeAnswer(command, false, "", 0, volumeDetails);
+                    }
                 }
 
-                long size = KVMPhysicalDisk.getVirtualSizeFromFile(path);
-                return new CheckVolumeAnswer(command, true, "", size, getVolumeDetails(pool, vol));
+                // Images on RBD are raw and the path is an image name that qemu-img cannot open
+                // without the rbd: URI, so take the size libvirt already reported for the volume.
+                long size = isRbd ? vol.getVirtualSize() : KVMPhysicalDisk.getVirtualSizeFromFile(path);
+                return new CheckVolumeAnswer(command, true, "", size, volumeDetails);
             } else {
                 return new Answer(command, false, "Unsupported Storage Pool");
             }
@@ -122,6 +134,9 @@ private Map<String, String> getDiskFileInfo(KVMStoragePool pool, KVMPhysicalDisk
         try {
             QemuImg qemu = new QemuImg(0);
             QemuImgFile qemuFile = new QemuImgFile(disk.getPath(), disk.getFormat());
+            if (Storage.StoragePoolType.RBD.equals(pool.getType())) {
+                qemuFile = new QemuImgFile(KVMPhysicalDisk.RBDStringBuilder(pool, disk.getPath()), disk.getFormat());
+            }
             return qemu.info(qemuFile, secure);
         } catch (QemuImgException | LibvirtException ex) {
             logger.error("Failed to get info of disk file: " + ex.getMessage());

server/src/main/java/org/apache/cloudstack/storage/volume/VolumeImportUnmanageManagerImpl.java

@@ -461,10 +461,27 @@ private VolumeVO importVolumeInternal(VolumeOnStorageTO volume, DiskOfferingVO d
                                           Account owner, StoragePoolVO pool, String volumeName) {
         DiskProfile diskProfile = volumeManager.importVolume(Volume.Type.DATADISK, volumeName, diskOffering,
                 volume.getVirtualSize(), null, null, pool.getDataCenterId(), volume.getHypervisorType(), null, null,
-                owner, null, pool.getId(), pool.getPoolType(), volume.getPath(), null);
+                owner, null, pool.getId(), pool.getPoolType(), volume.getPath(), null, getImageFormat(volume.getFormat()));
         return volumeDao.findById(diskProfile.getVolumeId());
     }
 
+    /**
+     * Maps the format the hypervisor reported for the volume on the pool onto an image format, so that
+     * the imported volume records what is actually on the pool (raw on RBD, qcow2 on file based pools)
+     * instead of the cluster default for the hypervisor. Returns null when the format is not recognised.
+     */
+    protected Storage.ImageFormat getImageFormat(String format) {
+        if (StringUtils.isBlank(format)) {
+            return null;
+        }
+        try {
+            return Storage.ImageFormat.valueOf(format.toUpperCase());
+        } catch (IllegalArgumentException e) {
+            logger.warn("Unrecognised image format {} reported for the volume being imported, falling back to the hypervisor default", format);
+            return null;
+        }
+    }
+
     protected void checkResourceLimitForImportVolume(Account owner, VolumeOnStorageTO volume, DiskOfferingVO diskOffering, List<Reserver> reservations) {
         Long volumeSize = volume.getVirtualSize();
         try {

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

@@ -796,14 +796,16 @@ private Pair<DiskProfile, StoragePool> importExternalDisk(UnmanagedInstanceTO.Di
         }
         diskProfile.setSize(copyRemoteVolumeAnswer.getSize());
         DiskProfile profile = volumeManager.updateImportedVolume(type, diskOffering, vm, template, deviceId,
-                storagePool.getId(), storagePool.getPoolType(), copyRemoteVolumeAnswer.getFilename(), chainInfo, diskProfile);
+                storagePool.getId(), storagePool.getPoolType(), copyRemoteVolumeAnswer.getFilename(), chainInfo, diskProfile,
+                getImageFormatFromVolumeDetails(copyRemoteVolumeAnswer.getVolumeDetails()));
 
         return new Pair<>(profile, storagePool);
     }
 
     private Pair<DiskProfile, StoragePool> importKVMLocalDisk(VirtualMachine vm, DiskOffering diskOffering,
                                                               Volume.Type type, VirtualMachineTemplate template,
-                                                              Long deviceId, Long hostId, String diskPath, DiskProfile diskProfile) {
+                                                              Long deviceId, Long hostId, String diskPath, DiskProfile diskProfile,
+                                                              Storage.ImageFormat format) {
         List<StoragePoolVO> storagePools = primaryDataStoreDao.findLocalStoragePoolsByHostAndTags(hostId, null);
         if(storagePools.size() < 1) {
             throw new CloudRuntimeException("Local Storage not found for host");
@@ -812,22 +814,44 @@ private Pair<DiskProfile, StoragePool> importKVMLocalDisk(VirtualMachine vm, Dis
         StoragePool storagePool = storagePools.get(0);
 
         DiskProfile profile = volumeManager.updateImportedVolume(type, diskOffering, vm, template, deviceId,
-                storagePool.getId(), storagePool.getPoolType(), diskPath, null, diskProfile);
+                storagePool.getId(), storagePool.getPoolType(), diskPath, null, diskProfile, format);
 
         return new Pair<>(profile, storagePool);
     }
 
     private Pair<DiskProfile, StoragePool> importKVMSharedDisk(VirtualMachine vm, DiskOffering diskOffering,
                                                               Volume.Type type, VirtualMachineTemplate template,
-                                                              Long deviceId, Long poolId, String diskPath, DiskProfile diskProfile) {
+                                                              Long deviceId, Long poolId, String diskPath, DiskProfile diskProfile,
+                                                              Storage.ImageFormat format) {
         StoragePool storagePool = primaryDataStoreDao.findById(poolId);
 
         DiskProfile profile = volumeManager.updateImportedVolume(type, diskOffering, vm, template, deviceId,
-                poolId, storagePool.getPoolType(), diskPath, null, diskProfile);
+                poolId, storagePool.getPoolType(), diskPath, null, diskProfile, format);
 
         return new Pair<>(profile, storagePool);
     }
 
+    /**
+     * Reads the image format the hypervisor reported for an existing volume, so that the imported
+     * volume records what is actually on the pool (raw on RBD, qcow2 on file based pools) instead of
+     * the cluster default for the hypervisor. Returns null when the agent did not report a format.
+     */
+    protected Storage.ImageFormat getImageFormatFromVolumeDetails(Map<VolumeOnStorageTO.Detail, String> volumeDetails) {
+        if (MapUtils.isEmpty(volumeDetails)) {
+            return null;
+        }
+        String fileFormat = volumeDetails.get(VolumeOnStorageTO.Detail.FILE_FORMAT);
+        if (StringUtils.isBlank(fileFormat)) {
+            return null;
+        }
+        try {
+            return Storage.ImageFormat.valueOf(fileFormat.toUpperCase());
+        } catch (IllegalArgumentException e) {
+            logger.warn("Unrecognised image format {} reported for the volume being imported, falling back to the hypervisor default", fileFormat);
+            return null;
+        }
+    }
+
     private Pair<DiskProfile, StoragePool> importDisk(UnmanagedInstanceTO.Disk disk, VirtualMachine vm, Cluster cluster, DiskOffering diskOffering,
                                                       Volume.Type type, String name, Long diskSize, Long minIops, Long maxIops, VirtualMachineTemplate template,
                                                       Account owner, Long deviceId) {
@@ -842,7 +866,7 @@ private Pair<DiskProfile, StoragePool> importDisk(UnmanagedInstanceTO.Disk disk,
         }
         StoragePool storagePool = getStoragePool(disk, zone, cluster, diskOffering);
         DiskProfile profile = volumeManager.importVolume(type, name, diskOffering, diskSize,
-                minIops, maxIops, vm.getDataCenterId(), vm.getHypervisorType(), vm, template, owner, deviceId, storagePool.getId(), storagePool.getPoolType(), path, chainInfo);
+                minIops, maxIops, vm.getDataCenterId(), vm.getHypervisorType(), vm, template, owner, deviceId, storagePool.getId(), storagePool.getPoolType(), path, chainInfo, null);
 
         return new Pair<DiskProfile, StoragePool>(profile, storagePool);
     }
@@ -2647,6 +2671,13 @@ private UserVmResponse importKvmInstance(ImportVmCmd cmd) {
                 throw new InvalidParameterValueException("Disk image is already in use");
             }
 
+            // A host the planner is pinned to must be able to see the pool, otherwise the volume check
+            // runs on a host that cannot reach the image and reports it as missing.
+            if (ImportSource.SHARED == importSource && hostId != null && storagePoolHostDao.findByPoolHost(poolId, hostId) == null) {
+                throw new InvalidParameterValueException(String.format(
+                        "Specified host does not have access to the storage pool: %s", storagePool.getUuid()));
+            }
+
             DiskOffering diskOffering = diskOfferingDao.findById(serviceOffering.getDiskOfferingId());
 
             if (diskOffering != null && !storagePoolSupportsDiskOffering(storagePool, diskOffering)) {
@@ -2923,7 +2954,13 @@ private UserVm importKvmVirtualMachineFromDisk(final ImportSource importSource,
             ServiceOfferingVO dummyOffering = serviceOfferingDao.findById(userVm.getId(), serviceOffering.getId());
             profile.setServiceOffering(dummyOffering);
             DeploymentPlanner.ExcludeList excludeList = new DeploymentPlanner.ExcludeList();
-            final DataCenterDeployment plan = new DataCenterDeployment(zone.getId(), null, null, hostId, poolId, null);
+            // Confine the plan to the pod and cluster of the pool the caller asked for. Otherwise the
+            // planner is free to pick a host in another cluster that cannot see the pool, and the volume
+            // check then runs against whichever pool that cluster does have. Both are null for a zone
+            // wide pool, which every host can see.
+            StoragePoolVO importStoragePool = primaryDataStoreDao.findById(poolId);
+            final DataCenterDeployment plan = new DataCenterDeployment(zone.getId(), importStoragePool.getPodId(),
+                    importStoragePool.getClusterId(), hostId, poolId, null);
             DeployDestination dest = null;
             try {
                 dest = deploymentPlanningManager.planDeployment(profile, plan, excludeList, null);
@@ -2968,12 +3005,13 @@ private UserVm importKvmVirtualMachineFromDisk(final ImportSource importSource,
             List<Pair<DiskProfile, StoragePool>> diskProfileStoragePoolList = new ArrayList<>();
             try {
                 long deviceId = 1L;
+                Storage.ImageFormat diskFormat = getImageFormatFromVolumeDetails(checkVolumeAnswer.getVolumeDetails());
                 if(ImportSource.SHARED == importSource) {
                     diskProfileStoragePoolList.add(importKVMSharedDisk(userVm, diskOffering, Volume.Type.ROOT,
-                            template, deviceId, poolId, diskPath, diskProfile));
+                            template, deviceId, poolId, diskPath, diskProfile, diskFormat));
                 } else if(ImportSource.LOCAL == importSource) {
                     diskProfileStoragePoolList.add(importKVMLocalDisk(userVm, diskOffering, Volume.Type.ROOT,
-                            template, deviceId, hostId, diskPath, diskProfile));
+                            template, deviceId, hostId, diskPath, diskProfile, diskFormat));
                 }
             } catch (Exception e) {
                 logger.error(String.format("Failed to import volumes while importing vm: %s", instanceName), e);