server: Reserve CD-ROM and config drive device ids for volumes

Data volumes could be assigned the same device id as the CD-ROM or the
config drive ISO, which end up with the same libvirt target name (e.g. sde)
when they are on the same bus namespace (SATA with UEFI/Q35).

Add VolumeApiService.getReservedDeviceIds(), which returns the device ids
of the CD-ROM devices and of the config drive ISO when the Instance uses
config drive. The config drive is detected from the network providers
(Dhcp, Dns, UserData), so Instances which have never been started are
covered as well as the ones with the config drive location detail.

The reserved ids are skipped when allocating device ids in:
- attach volume
- restore of volumes and Instances from backup
- import of Instances (unmanaged Instances and VMware to KVM)

The max number of data volumes is preserved when device ids are reserved,
and the error message of an invalid deviceId lists the reserved ids.

Author: weizhouapache

api/src/main/java/com/cloud/storage/VolumeApiService.java

@@ -21,12 +21,14 @@
 import java.net.MalformedURLException;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import com.cloud.exception.ResourceAllocationException;
 import com.cloud.offering.DiskOffering;
 import com.cloud.user.Account;
 import com.cloud.utils.Pair;
 import com.cloud.utils.fsm.NoTransitionException;
+import com.cloud.vm.VirtualMachine;
 
 import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd;
 import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd;
@@ -180,6 +182,16 @@ Volume updateVolume(long volumeId, String path, String state, Long storageId,
      */
     boolean doesStoragePoolSupportDiskOfferingTags(StoragePool destPool, String diskOfferingTags);
 
+    /**
+     * Returns the device ids of the Instance which are used by non-volume devices (the CD-ROM drives and the config drive ISO),
+     * and therefore must not be assigned to its volumes. Otherwise the hypervisor may end up with two devices using the same
+     * target name (e.g. sde), which is rejected.
+     *
+     * @param vm the Instance
+     * @return the reserved device ids
+     */
+    Set<Long> getReservedDeviceIds(VirtualMachine vm);
+
     boolean validateConditionsToReplaceDiskOfferingOfVolume(Volume volume, DiskOffering newDiskOffering, StoragePool destPool);
 
     Volume destroyVolume(long volumeId, Account caller, boolean expunge, boolean forceExpunge);

engine/components-api/src/main/java/com/cloud/template/TemplateManager.java

@@ -67,6 +67,9 @@ public interface TemplateManager {
     static final String VMWARE_TOOLS_ISO = "vmware-tools.iso";
     static final String XS_TOOLS_ISO = "xs-tools.iso";
 
+    // KVM/libvirt maps deviceSeq=3 to hdc (hda/hdb are taken by the root volume on i440fx/IDE).
+    int CDROM_PRIMARY_DEVICE_SEQ = 3;
+
     /**
      * Prepares a template for vm creation for a certain storage pool.
      *

server/src/main/java/com/cloud/hypervisor/KVMGuru.java

@@ -33,6 +33,7 @@
 import com.cloud.storage.GuestOSHypervisorVO;
 import com.cloud.storage.GuestOSVO;
 import com.cloud.storage.Volume;
+import com.cloud.storage.VolumeApiService;
 import com.cloud.storage.VolumeVO;
 import com.cloud.storage.dao.GuestOSDao;
 import com.cloud.storage.dao.GuestOSHypervisorDao;
@@ -43,9 +44,7 @@
 import com.cloud.vm.VMInstanceVO;
 import com.cloud.vm.VirtualMachine;
 import com.cloud.vm.VirtualMachineProfile;
-import com.cloud.vm.VmDetailConstants;
 import com.cloud.vm.dao.VMInstanceDao;
-import com.cloud.vm.dao.VMInstanceDetailsDao;
 import org.apache.cloudstack.backup.Backup;
 import org.apache.cloudstack.storage.command.CopyCommand;
 import org.apache.cloudstack.storage.command.StorageSubSystemCommand;
@@ -55,8 +54,10 @@
 import java.math.BigDecimal;
 import java.math.RoundingMode;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.utils.bytescale.ByteScaleUtils;
 import org.apache.commons.lang3.math.NumberUtils;
@@ -75,7 +76,7 @@ public class KVMGuru extends HypervisorGuruBase implements HypervisorGuru {
     @Inject
     HypervisorCapabilitiesDao _hypervisorCapabilitiesDao;
     @Inject
-    VMInstanceDetailsDao _vmInstanceDetailsDao;
+    VolumeApiService volumeApiService;
 
 
     @Override
@@ -91,33 +92,29 @@ protected KVMGuru() {
      * Get next free DeviceId for a KVM Guest
      */
 
-    protected Long getNextAvailableDeviceId(long vmId, List<VolumeVO> vmVolumes) {
+    protected Long getNextAvailableDeviceId(Set<Long> reservedDeviceIds, List<VolumeVO> vmVolumes) {
 
         int maxDataVolumesSupported;
-        int maxDeviceId;
-        List<String> devIds = new ArrayList<>();
+        List<Long> devIds = new ArrayList<>();
 
         try {
             maxDataVolumesSupported = _hypervisorCapabilitiesDao.getMaxDataVolumesLimit(HypervisorType.KVM,"default");
-            int maxDevices = maxDataVolumesSupported + 2; // add 2 to consider devices root volume and cdrom
-            maxDeviceId = maxDevices - 1;
         } catch (Exception e) {
             throw new RuntimeException("Cannot find maximum number of disk devices that can be attached to the KVM Hypervisor");
         }
-        for (int i = 1; i <= maxDeviceId; i++) {
-            devIds.add(String.valueOf(i));
-        }
-        devIds.remove("3");
-        if (_vmInstanceDetailsDao.findDetail(vmId, VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) {
-            devIds.remove(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.toString());
+        // the device ids reserved for the CD-ROM and config drive devices cannot be used by volumes
+        for (long id = 1; devIds.size() < maxDataVolumesSupported; id++) {
+            if (!reservedDeviceIds.contains(id)) {
+                devIds.add(id);
+            }
         }
         for (VolumeVO vmVolume : vmVolumes) {
-            devIds.remove(vmVolume.getDeviceId().toString().trim());
+            devIds.remove(vmVolume.getDeviceId());
         }
         if (devIds.isEmpty()) {
             throw new RuntimeException("All device Ids are in use.");
         }
-        return Long.parseLong(devIds.iterator().next());
+        return devIds.get(0);
     }
 
 
@@ -369,6 +366,9 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId,
                 vm.setPowerState(VirtualMachine.PowerState.PowerOff);
                 _instanceDao.update(vm.getId(), vm);
             }
+           // the networks of the (possibly expunged) VM may not be available, so always keep the config drive device id free
+           Set<Long> reservedDeviceIds = new HashSet<>(volumeApiService.getReservedDeviceIds(vm));
+           reservedDeviceIds.add(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue());
            for (Backup.VolumeInfo VMVolToRestore : vm.getBackupVolumeList()) {
                VolumeVO volume = _volumeDao.findByUuidIncludingRemoved(VMVolToRestore.getUuid());
                volume.setState(Volume.State.Ready);
@@ -379,7 +379,7 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId,
                } else if (VMVolToRestore.getType() == Volume.Type.DATADISK) {
                    List<VolumeVO> vmVolumes = _volumeDao.findByInstance(vm.getId());
                    _volumeDao.update(volume.getId(), volume);
-                   _volumeDao.attachVolume(volume.getId(), vm.getId(), getNextAvailableDeviceId(vm.getId(), vmVolumes));
+                   _volumeDao.attachVolume(volume.getId(), vm.getId(), getNextAvailableDeviceId(reservedDeviceIds, vmVolumes));
                }
                UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_ATTACH, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(),
                        volume.getDiskOfferingId(), volume.getTemplateId(), volume.getSize(), Volume.class.getName(), volume.getUuid(), vm.getId(), volume.isDisplay());
@@ -397,7 +397,7 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId,
         VolumeVO restoredVolume = _volumeDao.findByUuid(location);
         if (restoredVolume != null) {
             try {
-                _volumeDao.attachVolume(restoredVolume.getId(), vm.getId(), getNextAvailableDeviceId(vm.getId(), vmVolumes));
+                _volumeDao.attachVolume(restoredVolume.getId(), vm.getId(), getNextAvailableDeviceId(volumeApiService.getReservedDeviceIds(vm), vmVolumes));
                 restoredVolume.setState(Volume.State.Ready);
                 _volumeDao.update(restoredVolume.getId(), restoredVolume);
                 UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_ATTACH, restoredVolume.getAccountId(), restoredVolume.getDataCenterId(), restoredVolume.getId(), restoredVolume.getName(),

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

@@ -160,6 +160,9 @@
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.hypervisor.HypervisorCapabilitiesVO;
 import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao;
+import com.cloud.network.Network.Provider;
+import com.cloud.network.Network.Service;
+import com.cloud.network.NetworkModel;
 import com.cloud.network.element.ConfigDriveNetworkElement;
 import com.cloud.offering.DiskOffering;
 import com.cloud.org.Cluster;
@@ -220,6 +223,7 @@
 import com.cloud.utils.fsm.NoTransitionException;
 import com.cloud.utils.fsm.StateMachine2;
 import com.cloud.vm.DiskProfile;
+import com.cloud.vm.NicVO;
 import com.cloud.vm.VMInstanceDetailVO;
 import com.cloud.vm.UserVmManager;
 import com.cloud.vm.UserVmService;
@@ -241,6 +245,7 @@
 import com.cloud.vm.VmWorkResizeVolume;
 import com.cloud.vm.VmWorkSerializer;
 import com.cloud.vm.VmWorkTakeVolumeSnapshot;
+import com.cloud.vm.dao.NicDao;
 import com.cloud.vm.dao.UserVmDao;
 import com.cloud.vm.dao.VMInstanceDetailsDao;
 import com.cloud.vm.dao.VMInstanceDao;
@@ -294,6 +299,10 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
     @Inject
     private VMInstanceDetailsDao vmInstanceDetailsDao;
     @Inject
+    private NicDao _nicDao;
+    @Inject
+    private NetworkModel _networkModel;
+    @Inject
     private UserVmService _userVmService;
     @Inject
     private VolumeDataStoreDao _volumeStoreDao;
@@ -5038,16 +5047,69 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype
         return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion : minHwVersion;
     }
 
+    /**
+     * The number of CD-ROM devices of the Instance. Each CD-ROM device occupies a device id, starting from
+     * {@link TemplateManager#CDROM_PRIMARY_DEVICE_SEQ}.
+     */
+    protected int getVmCdromCount(VirtualMachine vm) {
+        return 1;
+    }
+
+    /**
+     * Checks if a config drive ISO is (or will be) attached to the Instance, which is the case when any of its networks
+     * has ConfigDrive as the provider of the Dhcp, Dns or UserData service. Checking the network providers (instead of
+     * only the config drive location detail, which is added when the ISO is created) also covers Instances which have
+     * never been started.
+     */
+    protected boolean isConfigDriveUsed(VirtualMachine vm) {
+        if (vmInstanceDetailsDao.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) {
+            return true;
+        }
+        for (NicVO nic : _nicDao.listByVmId(vm.getId())) {
+            for (Service service : Arrays.asList(Service.UserData, Service.Dhcp, Service.Dns)) {
+                if (_networkModel.isProviderSupportServiceInNetwork(nic.getNetworkId(), service, Provider.ConfigDrive)) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
+    @Override
+    public Set<Long> getReservedDeviceIds(VirtualMachine vm) {
+        Set<Long> reservedDeviceIds = new HashSet<>();
+        for (int i = 0; i < getVmCdromCount(vm); i++) {
+            reservedDeviceIds.add((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + i);
+        }
+        if (isConfigDriveUsed(vm)) {
+            reservedDeviceIds.add(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue());
+        }
+        return reservedDeviceIds;
+    }
+
+    /**
+     * Returns the first {@code maxDataVolumes} device ids (starting from 1) which are not reserved.
+     */
+    private List<Long> getAvailableDeviceIds(int maxDataVolumes, Set<Long> reservedDeviceIds) {
+        List<Long> availableDeviceIds = new ArrayList<>();
+        for (long id = 1; availableDeviceIds.size() < maxDataVolumes; id++) {
+            if (!reservedDeviceIds.contains(id)) {
+                availableDeviceIds.add(id);
+            }
+        }
+        return availableDeviceIds;
+    }
+
     private Long getDeviceId(UserVmVO vm, Long deviceId) {
         // allocate deviceId
-        int maxDevices = getMaxDataVolumesSupported(vm) + 2; // add 2 to consider devices root volume and cdrom
-        int maxDeviceId = maxDevices - 1;
+        Set<Long> reservedDeviceIds = getReservedDeviceIds(vm);
+        List<Long> availableDeviceIds = getAvailableDeviceIds(getMaxDataVolumesSupported(vm), reservedDeviceIds);
+        long maxDeviceId = availableDeviceIds.get(availableDeviceIds.size() - 1);
         List<VolumeVO> vols = _volsDao.findByInstance(vm.getId());
-        boolean vmHasConfigDrive = vmInstanceDetailsDao.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION) != null;
         if (deviceId != null) {
-            if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || deviceId.longValue() == 3
-                    || (vmHasConfigDrive && deviceId.longValue() == ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ)) {
-                throw new RuntimeException("deviceId should be 0,1,2,4-" + maxDeviceId);
+            if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || reservedDeviceIds.contains(deviceId)) {
+                throw new RuntimeException(String.format("deviceId should be 0 or between 1 and %d, except the ones reserved for CD-ROM/config drive devices: %s",
+                        maxDeviceId, reservedDeviceIds));
             }
             for (VolumeVO vol : vols) {
                 if (vol.getDeviceId().equals(deviceId)) {
@@ -5056,21 +5118,13 @@ private Long getDeviceId(UserVmVO vm, Long deviceId) {
             }
         } else {
             // allocate deviceId here
-            List<String> devIds = new ArrayList<String>();
-            for (int i = 1; i <= maxDeviceId; i++) {
-                devIds.add(String.valueOf(i));
-            }
-            devIds.remove("3");
-            if (vmHasConfigDrive) {
-                devIds.remove(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.toString());
-            }
             for (VolumeVO vol : vols) {
-                devIds.remove(vol.getDeviceId().toString().trim());
+                availableDeviceIds.remove(vol.getDeviceId());
             }
-            if (devIds.isEmpty()) {
+            if (availableDeviceIds.isEmpty()) {
                 throw new RuntimeException(String.format("All device Ids are used by vm %s", vm));
             }
-            deviceId = Long.parseLong(devIds.iterator().next());
+            deviceId = availableDeviceIds.get(0);
         }
 
         return deviceId;

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

@@ -78,6 +78,7 @@
 import com.cloud.network.dao.NetworkDao;
 import com.cloud.network.dao.NetworkVO;
 import com.cloud.network.dao.PhysicalNetworkDao;
+import com.cloud.network.element.ConfigDriveNetworkElement;
 import com.cloud.offering.DiskOffering;
 import com.cloud.offering.NetworkOffering;
 import com.cloud.offering.ServiceOffering;
@@ -847,6 +848,23 @@ private Pair<DiskProfile, StoragePool> importDisk(UnmanagedInstanceTO.Disk disk,
         return new Pair<DiskProfile, StoragePool>(profile, storagePool);
     }
 
+    /**
+     * Returns the device ids which cannot be used by the volumes of the imported Instance: the ones of the CD-ROM devices,
+     * and the one of the config drive ISO (always kept free, as the NICs of the Instance are not imported yet at this point).
+     */
+    private Set<Long> getReservedDeviceIds(VirtualMachine vm) {
+        Set<Long> reservedDeviceIds = new HashSet<>(volumeApiService.getReservedDeviceIds(vm));
+        reservedDeviceIds.add(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue());
+        return reservedDeviceIds;
+    }
+
+    private long getNextDeviceId(long deviceId, Set<Long> reservedDeviceIds) {
+        do {
+            deviceId++;
+        } while (reservedDeviceIds.contains(deviceId));
+        return deviceId;
+    }
+
     private NicProfile importNic(UnmanagedInstanceTO.Nic nic, VirtualMachine vm, Network network, Network.IpAddresses ipAddresses, int deviceId, boolean isDefaultNic, boolean forced) throws InsufficientVirtualNetworkCapacityException, InsufficientAddressCapacityException {
         DataCenterVO dataCenterVO = dataCenterDao.findById(network.getDataCenterId());
         Pair<NicProfile, Integer> result = networkOrchestrationService.importNic(nic.getMacAddress(), deviceId, network, isDefaultNic, vm, ipAddresses, dataCenterVO, forced);
@@ -1165,7 +1183,8 @@ private UserVm importVirtualMachineInternal(final UnmanagedInstanceTO unmanagedI
                 DiskOfferingVO diskOffering = diskOfferingDao.findById(serviceOffering.getDiskOfferingId());
                 diskProfileStoragePoolList.add(importDisk(rootDisk, userVm, cluster, diskOffering, Volume.Type.ROOT, String.format("ROOT-%d", userVm.getId()),
                         rootDisk.getCapacity(), minIops, maxIops, template, owner, null));
-                long deviceId = 1L;
+                Set<Long> reservedDeviceIds = getReservedDeviceIds(userVm);
+                long deviceId = getNextDeviceId(0L, reservedDeviceIds);
                 for (UnmanagedInstanceTO.Disk disk : dataDisks) {
                     if (disk.getCapacity() == null || disk.getCapacity() == 0) {
                         throw new InvalidParameterValueException(String.format("Disk ID: %s size is invalid", rootDisk.getDiskId()));
@@ -1174,7 +1193,7 @@ private UserVm importVirtualMachineInternal(final UnmanagedInstanceTO unmanagedI
                     diskProfileStoragePoolList.add(importDisk(disk, userVm, cluster, offering, Volume.Type.DATADISK, String.format("DATA-%d-%s", userVm.getId(), disk.getDiskId()),
                             disk.getCapacity(), offering.getMinIops(), offering.getMaxIops(),
                             template, owner, deviceId));
-                    deviceId++;
+                    deviceId = getNextDeviceId(deviceId, reservedDeviceIds);
                 }
             } catch (Exception e) {
                 logger.error(String.format("Failed to import volumes while importing vm: %s", displayName), e);
@@ -2789,15 +2808,16 @@ private UserVm importExternalKvmVirtualMachine(final UnmanagedInstanceTO unmanag
                 diskProfileStoragePoolList.add(importExternalDisk(rootDisk, userVm, dest, diskOffering, Volume.Type.ROOT,
                         template, null, remoteUrl, username, password, tmpPath, diskProfile));
 
-                long deviceId = 1L;
+                Set<Long> reservedDeviceIds = getReservedDeviceIds(userVm);
+                long deviceId = getNextDeviceId(0L, reservedDeviceIds);
                 diskSeq = 0;
                 for (UnmanagedInstanceTO.Disk disk : dataDisks) {
                     DiskProfile dataDiskProfile = dataDiskProfiles[diskSeq++];
                     DiskOffering offering = diskOfferingDao.findById(dataDiskOfferingMap.get(disk.getDiskId()));
 
                     diskProfileStoragePoolList.add(importExternalDisk(disk, userVm, dest, offering, Volume.Type.DATADISK,
                             template, deviceId, remoteUrl, username, password, tmpPath, dataDiskProfile));
-                    deviceId++;
+                    deviceId = getNextDeviceId(deviceId, reservedDeviceIds);
                 }
             } catch (Exception e) {
                 logger.error(String.format("Failed to import volumes while importing vm: %s", instanceName), e);