server,kvm: Avoid device id conflicts between data volumes, CD-ROMs and the config drive ISO

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

@@ -21,13 +21,15 @@
 import java.net.MalformedURLException;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import com.cloud.dc.DataCenter;
 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;
@@ -187,6 +189,22 @@ 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);
+
+    /**
+     * Same as {@link #getReservedDeviceIds(VirtualMachine)}, with the specified usage of config drive instead of the one
+     * of the Instance. It is useful when the networks of the Instance are not known yet, e.g. when importing it.
+     */
+    Set<Long> getReservedDeviceIds(VirtualMachine vm, boolean configDriveUsed);
+
     boolean validateConditionsToReplaceDiskOfferingOfVolume(Volume volume, DiskOffering newDiskOffering, StoragePool destPool);
 
     Volume destroyVolume(long volumeId, Account caller, boolean expunge, boolean forceExpunge, Boolean countDisplayFalseInResourceCount);

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

@@ -17,6 +17,7 @@
 package com.cloud.template;
 
 import java.util.List;
+import java.util.Set;
 
 import org.apache.cloudstack.api.BaseCmd;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
@@ -37,6 +38,7 @@
 import com.cloud.storage.VMTemplateVO;
 import com.cloud.storage.VolumeVO;
 import com.cloud.utils.Pair;
+import com.cloud.vm.VirtualMachine;
 import com.cloud.vm.VirtualMachineProfile;
 
 /**
@@ -183,6 +185,24 @@ public interface TemplateManager {
      */
     void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestination dest);
 
+    /**
+     * Returns the device ids of the CD-ROM devices of the Instance, starting from {@link #CDROM_PRIMARY_DEVICE_SEQ}.
+     * They cannot be used by the volumes of the Instance.
+     * The hypervisor defines all the CD-ROM devices it supports for the Instance (see {@code host.cdrom.max.count}),
+     * whether or not ISOs are attached to them. If the Instance uses config drive, the config drive ISO is one more
+     * CD-ROM: it keeps its device id (4), and the CD-ROMs of the ISOs from that device id are moved to the next ones.
+     *
+     * @param vm the Instance
+     * @return the device ids of the CD-ROM devices, including the one of the config drive ISO if the Instance uses config drive
+     */
+    Set<Long> getCdromDeviceIds(VirtualMachine vm);
+
+    /**
+     * Same as {@link #getCdromDeviceIds(VirtualMachine)}, with the specified usage of config drive instead of the one of
+     * the Instance. It is useful when the networks of the Instance are not known yet, e.g. when importing it.
+     */
+    Set<Long> getCdromDeviceIds(VirtualMachine vm, boolean configDriveUsed);
+
     public static final String MESSAGE_REGISTER_PUBLIC_TEMPLATE_EVENT = "Message.RegisterPublicTemplate.Event";
     public static final String MESSAGE_RESET_TEMPLATE_PERMISSION_EVENT = "Message.ResetTemplatePermission.Event";
 

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

@@ -39,6 +39,7 @@
 
 import com.cloud.agent.properties.AgentProperties;
 import com.cloud.agent.properties.AgentPropertiesFileHandler;
+import com.cloud.template.TemplateManager;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 
@@ -57,8 +58,10 @@
 public class LibvirtVMDef {
     protected static Logger LOGGER = LogManager.getLogger(LibvirtVMDef.class);
 
-    // CD-ROM slot allocation: getDevLabel() maps deviceSeq=3,4 to hdc and hdd on the IDE bus.
-    // Bumping this requires extending getDevLabel() (e.g. to spill onto SATA or a second IDE controller).
+    // Number of CD-ROMs supported by CloudStack per VM (not a limit of the guest OS or QEMU). The CD-ROMs are on the IDE bus,
+    // which has 4 slots: the first two are kept for disks, so the ISOs get hdc and hdd (getDevLabel() maps deviceSeq=3,4).
+    // Going past 2 means putting the CD-ROMs on the SATA bus (or on a second IDE controller): the CD-ROMs with a device id
+    // beyond these slots, e.g. the one following the config drive ISO (which keeps the device id 4), are on the SATA bus.
     public static final int MAX_CDROMS_PER_VM = 2;
 
     private String _hvsType;
@@ -1147,7 +1150,16 @@ public void defISODisk(String volPath, Integer devId, String diskLabel, DiskType
             _bus = DiskBus.IDE;
         }
 
+        private boolean isIdeCdromSlot(Integer devId) {
+            return devId == null || devId < TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + MAX_CDROMS_PER_VM;
+        }
+
         public void defISODisk(String volPath, Integer devId, boolean isSecure, DiskType diskType) {
+            // the IDE bus only has the slots of MAX_CDROMS_PER_VM CD-ROMs (hdc and hdd), the other ones (e.g. the config drive ISO)
+            // are on the SATA bus
+            if (!isSecure && !isIdeCdromSlot(devId)) {
+                isSecure = true;
+            }
             if (!isSecure) {
                 defISODisk(volPath, devId, diskType);
             } else {

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java

@@ -597,4 +597,25 @@ public void testTpmModel() {
                 "<backend type='emulator' version='2.0'/>\n" +
                 "</tpm>\n", tpmDef.toString());
     }
+
+    @Test
+    public void testIsoDiskDefOnIdeBusForTheIdeCdromSlots() {
+        for (int[] slot : new int[][] {{3, 'c'}, {4, 'd'}}) {
+            DiskDef cdrom = new DiskDef();
+            cdrom.defISODisk(null, slot[0], false, DiskDef.DiskType.FILE);
+
+            assertEquals(DiskDef.DiskBus.IDE, cdrom.getBusType());
+            assertEquals("hd" + (char) slot[1], cdrom.getDiskLabel());
+        }
+    }
+
+    @Test
+    public void testIsoDiskDefBeyondTheIdeCdromSlotsIsOnSataBus() {
+        // e.g. the config drive ISO following the ISOs: IDE only has the hdc and hdd slots for CD-ROMs
+        DiskDef cdrom = new DiskDef();
+        cdrom.defISODisk("/mnt/pool/configdrive/i-2-3-VM.iso", 5, false, DiskDef.DiskType.FILE);
+
+        assertEquals(DiskDef.DiskBus.SATA, cdrom.getBusType());
+        assertEquals("sdf", cdrom.getDiskLabel());
+    }
 }

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

@@ -58,6 +58,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import org.apache.cloudstack.utils.bytescale.ByteScaleUtils;
 
 public class KVMGuru extends HypervisorGuruBase implements HypervisorGuru {
@@ -91,30 +92,29 @@ protected KVMGuru() {
      * Get next free DeviceId for a KVM Guest
      */
 
-    protected Long getNextAvailableDeviceId(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));
+        // 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);
+            }
         }
-        devIds.remove("3");
         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);
     }
 
 
@@ -373,6 +373,8 @@ 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 = volumeApiService.getReservedDeviceIds(vm, true);
            for (Backup.VolumeInfo VMVolToRestore : vm.getBackupVolumeList()) {
                VolumeVO volume = _volumeDao.findByUuidIncludingRemoved(VMVolToRestore.getUuid());
                volume.setState(Volume.State.Ready);
@@ -388,9 +390,10 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId,
                    // it here shifts every data disk by one slot on every restore, which then misaligns the
                    // device-id ordering that the restore itself uses to map backup files onto volumes.
                    Long deviceId = VMVolToRestore.getDeviceId();
-                   if (deviceId == null) {
-                       // Backups taken before the device id was recorded in the backup metadata.
-                       deviceId = getNextAvailableDeviceId(_volumeDao.findByInstance(vm.getId()));
+                   if (deviceId == null || reservedDeviceIds.contains(deviceId)) {
+                       // Backups taken before the device id was recorded in the backup metadata, or with a volume on a
+                       // device id which is reserved now for a CD-ROM or the config drive ISO.
+                       deviceId = getNextAvailableDeviceId(reservedDeviceIds, _volumeDao.findByInstance(vm.getId()));
                    }
                    _volumeDao.attachVolume(volume.getId(), vm.getId(), deviceId);
                }
@@ -415,7 +418,7 @@ public boolean attachRestoredVolumeToVirtualMachine(long zoneId, String location
         }
         if (restoredVolume != null) {
             try {
-                _volumeDao.attachVolume(restoredVolume.getId(), vm.getId(), getNextAvailableDeviceId(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/network/element/ConfigDriveNetworkElement.java

@@ -142,7 +142,7 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle
     @Inject
     private HypervisorGuruManager _hvGuruMgr;
 
-    private final static Integer CONFIGDRIVEDISKSEQ = 4;
+    public static final Integer CONFIGDRIVEDISKSEQ = 4;
 
     private boolean canHandle(TrafficType trafficType) {
         return trafficType.equals(TrafficType.Guest);

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

@@ -5357,14 +5357,39 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype
         return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion : minHwVersion;
     }
 
+    @Override
+    public Set<Long> getReservedDeviceIds(VirtualMachine vm) {
+        return _tmpltMgr.getCdromDeviceIds(vm);
+    }
+
+    @Override
+    public Set<Long> getReservedDeviceIds(VirtualMachine vm, boolean configDriveUsed) {
+        return _tmpltMgr.getCdromDeviceIds(vm, configDriveUsed);
+    }
+
+    /**
+     * 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());
         if (deviceId != null) {
-            if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || deviceId.longValue() == 3) {
-                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)) {
@@ -5373,18 +5398,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");
             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;