diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java index 372eb0385618..78bc106e00c3 100644 --- a/api/src/main/java/com/cloud/storage/VolumeApiService.java +++ b/api/src/main/java/com/cloud/storage/VolumeApiService.java @@ -21,6 +21,7 @@ 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; @@ -28,6 +29,7 @@ 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 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 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); diff --git a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java index 8c11fe6c93ad..4b5ca4999373 100644 --- a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java +++ b/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 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 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"; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java index 439e4f663416..a3643874a52a 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java +++ b/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; @@ -1138,7 +1141,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 { diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java index 56ad267eac7e..a51719a33264 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java @@ -597,4 +597,25 @@ public void testTpmModel() { "\n" + "\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()); + } } diff --git a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java index 4aa67c5835f0..e40dee25331d 100644 --- a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java +++ b/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 vmVolumes) { + protected Long getNextAvailableDeviceId(Set reservedDeviceIds, List vmVolumes) { int maxDataVolumesSupported; - int maxDeviceId; - List devIds = new ArrayList<>(); + List 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 reservedDeviceIds = volumeApiService.getReservedDeviceIds(vm, true); for (Backup.VolumeInfo VMVolToRestore : vm.getBackupVolumeList()) { VolumeVO volume = _volumeDao.findByUuidIncludingRemoved(VMVolToRestore.getUuid()); volume.setState(Volume.State.Ready); @@ -383,7 +385,7 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId, } else if (VMVolToRestore.getType() == Volume.Type.DATADISK) { List vmVolumes = _volumeDao.findByInstance(vm.getId()); _volumeDao.update(volume.getId(), volume); - _volumeDao.attachVolume(volume.getId(), vm.getId(), getNextAvailableDeviceId(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()); @@ -406,7 +408,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(), diff --git a/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java b/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java index 6cdd6f5753d7..fbc0d106edcf 100644 --- a/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java +++ b/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); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 8674014addd7..88795e7c036a 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/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 getReservedDeviceIds(VirtualMachine vm) { + return _tmpltMgr.getCdromDeviceIds(vm); + } + + @Override + public Set 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 getAvailableDeviceIds(int maxDataVolumes, Set reservedDeviceIds) { + List 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 reservedDeviceIds = getReservedDeviceIds(vm); + List availableDeviceIds = getAvailableDeviceIds(getMaxDataVolumesSupported(vm), reservedDeviceIds); + long maxDeviceId = availableDeviceIds.get(availableDeviceIds.size() - 1); List 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 devIds = new ArrayList(); - 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; diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 873afff01c87..482d0b405d72 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -24,8 +24,10 @@ import java.util.Collections; import java.util.Date; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; @@ -155,6 +157,10 @@ import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.hypervisor.HypervisorGuru; import com.cloud.hypervisor.HypervisorGuruManager; +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.projects.Project; import com.cloud.projects.ProjectManager; import com.cloud.resourcelimit.CheckedReservation; @@ -219,6 +225,7 @@ import com.cloud.utils.db.TransactionCallbackNoReturn; import com.cloud.utils.db.TransactionStatus; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.NicVO; import com.cloud.vm.UserVmManager; import com.cloud.vm.UserVmVO; import com.cloud.vm.VMInstanceVO; @@ -228,9 +235,11 @@ import com.cloud.vm.VirtualMachineProfileImpl; import com.cloud.vm.VmDetailConstants; import com.cloud.vm.VmIsoMapVO; +import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VmIsoMapDao; import com.cloud.vm.dao.VMInstanceDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; import com.google.gson.Gson; import com.google.gson.GsonBuilder; @@ -265,6 +274,12 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, @Inject private UserVmDao _userVmDao; @Inject + private NicDao _nicDao; + @Inject + private NetworkModel _networkModel; + @Inject + private VMInstanceDetailsDao _vmInstanceDetailsDao; + @Inject private VmIsoMapDao _vmIsoMapDao; @Inject private VolumeDao _volumeDao; @@ -705,15 +720,87 @@ public void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestinat // Pre-allocate every cdrom slot at boot. QEMU/IDE refuses to hot-add new cdrom drives, so // runtime attachIso can only media-swap into a slot the domain already owns. int totalSlots = Math.max(effectiveMaxCdroms(vm, dest.getHost().getId()), slotsNeededFor(slotToIsoId)); + boolean configDriveUsed = isConfigDriveUsed(vm); for (int i = 0; i < totalSlots; i++) { - int diskSeq = CDROM_PRIMARY_DEVICE_SEQ + i; - Long isoId = slotToIsoId.get(diskSeq); + int slot = CDROM_PRIMARY_DEVICE_SEQ + i; + int diskSeq = getIsoDeviceSeq(slot, configDriveUsed); + Long isoId = slotToIsoId.get(slot); profile.addDisk(isoId != null ? buildIsoDisk(profile, vm, dest, poolId, diskSeq, isoId) : buildEmptyCdromDisk(diskSeq)); } } + @Override + public Set getCdromDeviceIds(VirtualMachine vm) { + if (vm.getType() != VirtualMachine.Type.User) { + return getCdromDeviceIds(1); + } + return getCdromDeviceIds(vm, isConfigDriveUsed(vm)); + } + + @Override + public Set getCdromDeviceIds(VirtualMachine vm, boolean configDriveUsed) { + return getCdromDeviceIds(getCdromCount(vm, configDriveUsed)); + } + + private Set getCdromDeviceIds(int cdromCount) { + Set deviceIds = new HashSet<>(); + for (int i = 0; i < cdromCount; i++) { + deviceIds.add((long) CDROM_PRIMARY_DEVICE_SEQ + i); + } + return deviceIds; + } + + private int getCdromCount(VirtualMachine vm, boolean configDriveUsed) { + if (vm.getType() != VirtualMachine.Type.User) { + return 1; + } + // all the CD-ROM devices supported by the host are defined for the Instance at boot, even if the configured + // maximum number of ISOs (vm.iso.max.count) is lower or is increased later + int cdromCount = advertisedCdromCap(hostIdForVm(vm)); + UserVmVO userVm = _userVmDao.findById(vm.getId()); + if (userVm != null) { + cdromCount = Math.max(cdromCount, slotsNeededFor(loadAttachedIsoSlots(userVm))); + } + // the config drive ISO is one more CD-ROM, it is not counted in the CD-ROMs supported for the ISOs + return cdromCount + (configDriveUsed ? 1 : 0); + } + + /** + * Returns the device id of the CD-ROM of an ISO slot (the slots are the ones stored for the ISOs of the Instance, + * starting from {@link #CDROM_PRIMARY_DEVICE_SEQ}). The config drive ISO always has the device id + * {@link ConfigDriveNetworkElement#CONFIGDRIVEDISKSEQ}, so the slots from it are moved to the next device ids + * when the Instance uses config drive. + */ + int getIsoDeviceSeq(int slot, boolean configDriveUsed) { + return configDriveUsed && slot >= ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ ? slot + 1 : slot; + } + + /** + * 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. + */ + boolean isConfigDriveUsed(VirtualMachine vm) { + if (_vmInstanceDetailsDao.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) { + return true; + } + List nics = _nicDao.listByVmId(vm.getId()); + if (CollectionUtils.isEmpty(nics)) { + return false; + } + for (NicVO nic : nics) { + for (Service service : Arrays.asList(Service.UserData, Service.Dhcp, Service.Dns)) { + if (_networkModel.isProviderSupportServiceInNetwork(nic.getNetworkId(), service, Provider.ConfigDrive)) { + return true; + } + } + } + return false; + } + private Long singleStoragePoolId(DeployDestination dest) { Long poolId = null; Map storageForDisks = dest.getStorageForDisks(); @@ -1451,7 +1538,8 @@ private boolean attachISOToVM(long vmId, long isoId, int deviceSeq, boolean atta } DataTO isoTO = tmplt.getTO(); - DiskTO disk = new DiskTO(isoTO, (long) deviceSeq, null, Volume.Type.ISO); + int diskSeq = !isVirtualRouter ? getIsoDeviceSeq(deviceSeq, isConfigDriveUsed(vm)) : deviceSeq; + DiskTO disk = new DiskTO(isoTO, (long) diskSeq, null, Volume.Type.ISO); HypervisorGuru hvGuru = _hvGuruMgr.getGuru(vm.getHypervisorType()); VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm); 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 6b437b362271..6a126f512e37 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -898,6 +898,21 @@ private Pair importDisk(UnmanagedInstanceTO.Disk disk, return new Pair(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 getReservedDeviceIds(VirtualMachine vm) { + return volumeApiService.getReservedDeviceIds(vm, true); + } + + private long getNextDeviceId(long deviceId, Set 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 result = networkOrchestrationService.importNic(nic.getMacAddress(), deviceId, network, isDefaultNic, vm, ipAddresses, dataCenterVO, forced); @@ -1227,7 +1242,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, storagePoolId)); - long deviceId = 1L; + Set 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())); @@ -1236,7 +1252,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, storagePoolId)); - deviceId++; + deviceId = getNextDeviceId(deviceId, reservedDeviceIds); } } catch (Exception e) { logger.error(String.format("Failed to import volumes while importing vm: %s", displayName), e); @@ -3390,7 +3406,8 @@ 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 reservedDeviceIds = getReservedDeviceIds(userVm); + long deviceId = getNextDeviceId(0L, reservedDeviceIds); diskSeq = 0; for (UnmanagedInstanceTO.Disk disk : dataDisks) { DiskProfile dataDiskProfile = dataDiskProfiles[diskSeq++]; @@ -3398,7 +3415,7 @@ private UserVm importExternalKvmVirtualMachine(final UnmanagedInstanceTO unmanag 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); diff --git a/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java b/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java index e5239a651230..e06012e35a13 100644 --- a/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java +++ b/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java @@ -19,7 +19,10 @@ import java.io.UnsupportedEncodingException; import java.util.Arrays; import java.util.HashMap; +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.framework.config.ConfigKey; @@ -40,6 +43,7 @@ import com.cloud.configuration.ConfigurationManagerImpl; import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; +import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao; import com.cloud.hypervisor.kvm.dpdk.DpdkHelper; import com.cloud.offering.ServiceOffering; import com.cloud.service.ServiceOfferingDetailsVO; @@ -48,6 +52,7 @@ import com.cloud.service.dao.ServiceOfferingDetailsDao; import com.cloud.storage.GuestOSHypervisorVO; import com.cloud.storage.GuestOSVO; +import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.GuestOSDao; import com.cloud.storage.dao.GuestOSHypervisorDao; import com.cloud.utils.Pair; @@ -97,6 +102,9 @@ public class KVMGuruTest { @Mock DpdkHelper dpdkHelperMock; + @Mock + HypervisorCapabilitiesDao hypervisorCapabilitiesDaoMock; + @Mock GuestOSVO guestOsVoMock; @@ -499,4 +507,42 @@ public void getCpuQuotaPercentageTestReturnNullWhenANumberFormatExceptionIsThrow Double actualQuota = guru.getCpuQuotaPercentage(vmSpeed, hostSpeed); Assert.assertNull(actualQuota); } + + private List mockVolumesWithDeviceIds(long... deviceIds) { + List volumes = new java.util.ArrayList<>(); + for (long deviceId : deviceIds) { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getDeviceId()).thenReturn(deviceId); + volumes.add(volume); + } + return volumes; + } + + @Test + public void getNextAvailableDeviceIdSkipsReservedDeviceIds() { + Mockito.when(hypervisorCapabilitiesDaoMock.getMaxDataVolumesLimit(Hypervisor.HypervisorType.KVM, "default")).thenReturn(6); + List volumes = mockVolumesWithDeviceIds(0L, 1L, 2L); + + Assert.assertEquals(Long.valueOf(4L), guru.getNextAvailableDeviceId(new HashSet<>(Arrays.asList(3L)), volumes)); + Assert.assertEquals(Long.valueOf(5L), guru.getNextAvailableDeviceId(new HashSet<>(Arrays.asList(3L, 4L)), volumes)); + Assert.assertEquals(Long.valueOf(6L), guru.getNextAvailableDeviceId(new HashSet<>(Arrays.asList(3L, 4L, 5L)), volumes)); + } + + @Test + public void getNextAvailableDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved() { + Mockito.when(hypervisorCapabilitiesDaoMock.getMaxDataVolumesLimit(Hypervisor.HypervisorType.KVM, "default")).thenReturn(6); + Set reserved = new HashSet<>(Arrays.asList(3L, 4L)); + // root + 5 data volumes (1, 2, 5, 6, 7): the 6th data volume is still allowed, and gets device id 8 + List volumes = mockVolumesWithDeviceIds(0L, 1L, 2L, 5L, 6L, 7L); + + Assert.assertEquals(Long.valueOf(8L), guru.getNextAvailableDeviceId(reserved, volumes)); + } + + @Test(expected = RuntimeException.class) + public void getNextAvailableDeviceIdThrowsExceptionWhenAllDeviceIdsAreUsed() { + Mockito.when(hypervisorCapabilitiesDaoMock.getMaxDataVolumesLimit(Hypervisor.HypervisorType.KVM, "default")).thenReturn(6); + List volumes = mockVolumesWithDeviceIds(0L, 1L, 2L, 5L, 6L, 7L, 8L); + + guru.getNextAvailableDeviceId(new HashSet<>(Arrays.asList(3L, 4L)), volumes); + } } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 12258e2cc160..334bb3f659fd 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -37,6 +37,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.UUID; import java.util.concurrent.ExecutionException; @@ -128,6 +129,7 @@ import com.cloud.storage.dao.VMTemplateDao; import com.cloud.storage.dao.VolumeDao; import com.cloud.storage.snapshot.SnapshotManager; +import com.cloud.template.TemplateManager; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.AccountVO; @@ -164,6 +166,8 @@ public class VolumeApiServiceImplTest { @Mock private VolumeDataStoreDao volumeDataStoreDaoMock; @Mock + private TemplateManager templateManagerMock; + @Mock private VolumeDao volumeDaoMock; @Mock private BackupDao backupDaoMock; @@ -2896,4 +2900,114 @@ public void testResizeVolumeInternal_VMware_VMRunning_ShouldThrowStateGuardError e.getMessage() != null && e.getMessage().contains("VM should be in")); } } + + private Long invokeGetDeviceId(UserVmVO vm, Long deviceId) throws Exception { + java.lang.reflect.Method method = VolumeApiServiceImpl.class.getDeclaredMethod("getDeviceId", UserVmVO.class, Long.class); + method.setAccessible(true); + try { + return (Long) method.invoke(volumeApiServiceImpl, vm, deviceId); + } catch (InvocationTargetException e) { + throw (Exception) e.getCause(); + } + } + + private void mockCdromDeviceIds(UserVmVO vm, Long... deviceIds) { + lenient().when(templateManagerMock.getCdromDeviceIds(vm)).thenReturn(new HashSet<>(Arrays.asList(deviceIds))); + } + + private List mockVmVolumes(UserVmVO vm, long... deviceIds) { + List volumes = new ArrayList<>(); + for (long deviceId : deviceIds) { + VolumeVO volume = Mockito.mock(VolumeVO.class); + lenient().when(volume.getDeviceId()).thenReturn(deviceId); + volumes.add(volume); + } + lenient().when(volumeDaoMock.findByInstance(vm.getId())).thenReturn(volumes); + return volumes; + } + + @Test + public void getReservedDeviceIdsReturnsTheDeviceIdsOfTheCdroms() { + UserVmVO vm = getMockedVm(); + + mockCdromDeviceIds(vm, 3L); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); + + // e.g. the CD-ROM and the config drive ISO, which is attached as the CD-ROM following the ones of the ISOs + mockCdromDeviceIds(vm, 3L, 4L); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); + + mockCdromDeviceIds(vm, 3L, 4L, 5L); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); + } + + @Test + public void getDeviceIdUsesDeviceId4WhenVmHasOneCdrom() throws Exception { + UserVmVO vm = getMockedVm(); + mockCdromDeviceIds(vm, 3L); + mockVmVolumes(vm, 0L, 1L, 2L); + + Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, null)); + Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, 4L)); + } + + @Test + public void getDeviceIdSkipsDeviceIdsOfAllCdroms() throws Exception { + UserVmVO vm = getMockedVm(); + mockCdromDeviceIds(vm, 3L, 4L); + mockVmVolumes(vm, 0L, 1L, 2L); + + Assert.assertEquals(Long.valueOf(5L), invokeGetDeviceId(vm, null)); + } + + @Test + public void getDeviceIdRejectsReservedDeviceIds() throws Exception { + UserVmVO vm = getMockedVm(); + mockCdromDeviceIds(vm, 3L, 4L); + mockVmVolumes(vm, 0L); + + for (long reservedDeviceId : new long[] {3L, 4L}) { + try { + invokeGetDeviceId(vm, reservedDeviceId); + fail("deviceId " + reservedDeviceId + " should be rejected"); + } catch (RuntimeException e) { + Assert.assertTrue(e.getMessage(), e.getMessage().contains("deviceId should be 0 or between 1 and")); + } + } + } + + @Test + public void getDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved() throws Exception { + UserVmVO vm = getMockedVm(); + mockCdromDeviceIds(vm, 3L, 4L); + List volumes = mockVmVolumes(vm, 0L); + + // 6 data volumes are supported by default + List allocated = new ArrayList<>(); + for (int i = 0; i < 6; i++) { + Long deviceId = invokeGetDeviceId(vm, null); + allocated.add(deviceId); + VolumeVO volume = Mockito.mock(VolumeVO.class); + lenient().when(volume.getDeviceId()).thenReturn(deviceId); + volumes.add(volume); + } + + Assert.assertEquals(Arrays.asList(1L, 2L, 5L, 6L, 7L, 8L), allocated); + try { + invokeGetDeviceId(vm, null); + fail("All device ids are used, an exception is expected"); + } catch (RuntimeException e) { + Assert.assertTrue(e.getMessage(), e.getMessage().contains("All device Ids are used")); + } + } + + @Test + public void getReservedDeviceIdsWithConfigDriveReservesTheCdromOfConfigDrive() { + UserVmVO vm = getMockedVm(); + when(templateManagerMock.getCdromDeviceIds(vm, false)).thenReturn(new HashSet<>(Arrays.asList(3L, 4L))); + when(templateManagerMock.getCdromDeviceIds(vm, true)).thenReturn(new HashSet<>(Arrays.asList(3L, 4L, 5L))); + + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)), volumeApiServiceImpl.getReservedDeviceIds(vm, false)); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)), volumeApiServiceImpl.getReservedDeviceIds(vm, true)); + } } diff --git a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java index 365f66e74115..61407a9d4c2b 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java @@ -65,10 +65,12 @@ import org.springframework.core.type.filter.TypeFilter; import com.cloud.agent.AgentManager; +import com.cloud.agent.api.to.DiskTO; import com.cloud.api.query.dao.SnapshotJoinDao; import com.cloud.api.query.dao.UserVmJoinDao; import com.cloud.api.query.vo.UserVmJoinVO; import com.cloud.dc.dao.DataCenterDao; +import com.cloud.deploy.DeployDestination; import com.cloud.deployasis.dao.TemplateDeployAsIsDetailsDao; import com.cloud.domain.dao.DomainDao; import com.cloud.event.dao.UsageEventDao; @@ -82,6 +84,9 @@ import com.cloud.host.dao.HostDetailsDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.hypervisor.HypervisorGuruManager; +import com.cloud.network.Network.Provider; +import com.cloud.network.Network.Service; +import com.cloud.network.NetworkModel; import com.cloud.projects.ProjectManager; import com.cloud.resourcelimit.CheckedReservation; import com.cloud.storage.DataStoreRole; @@ -120,13 +125,19 @@ import com.cloud.utils.concurrency.NamedThreadFactory; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.uservm.UserVm; +import com.cloud.vm.NicVO; import com.cloud.vm.UserVmVO; +import com.cloud.vm.VMInstanceDetailVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; +import com.cloud.vm.VirtualMachineProfile; import com.cloud.vm.VirtualMachine.State; +import com.cloud.vm.VmDetailConstants; import com.cloud.vm.VmIsoMapVO; +import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.dao.VmIsoMapDao; import junit.framework.TestCase; @@ -249,6 +260,12 @@ public class TemplateManagerImplTest extends TestCase { @Mock HostDetailsDao _hostDetailsDao; + @Mock + NicDao _nicDao; + @Mock + NetworkModel _networkModel; + @Mock + VMInstanceDetailsDao _vmInstanceDetailsDao; @Mock UserVmJoinDao _userVmJoinDao; @@ -1312,4 +1329,162 @@ public boolean match(MetadataReader mdr, MetadataReaderFactory arg1) throws IOEx } } } + + private java.util.Set ids(Long... deviceIds) { + return new java.util.HashSet<>(Arrays.asList(deviceIds)); + } + + private UserVmVO mockUserVmOnHostWithCdromCap(Long advertisedCap) { + UserVmVO vm = Mockito.mock(UserVmVO.class); + Mockito.when(vm.getId()).thenReturn(1L); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.when(vm.getHostId()).thenReturn(7L); + Mockito.when(_userVmDao.findById(1L)).thenReturn(vm); + if (advertisedCap != null) { + DetailVO detail = Mockito.mock(DetailVO.class); + Mockito.when(detail.getValue()).thenReturn(String.valueOf(advertisedCap)); + Mockito.when(_hostDetailsDao.findDetail(7L, Host.HOST_CDROM_MAX_COUNT)).thenReturn(detail); + } + return vm; + } + + @Test + public void getCdromDeviceIdsReturnsOnlyThePrimaryCdromForSystemVms() { + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.SecondaryStorageVm); + + Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm)); + } + + @Test + public void getCdromDeviceIdsReturnsAllCdromsAdvertisedByHostRegardlessOfConfiguredMaximum() { + // vm.iso.max.count defaults to 1, but the host defines all the CD-ROMs it supports + UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); + + Assert.assertEquals(ids(3L, 4L), templateManager.getCdromDeviceIds(vm)); + } + + @Test + public void getCdromDeviceIdsReturnsDefaultWhenHostDoesNotAdvertiseCdromCap() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(null); + + Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm)); + } + + @Test + public void getCdromDeviceIdsCoversAttachedIsosBeyondHostCap() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); + Mockito.when(_vmIsoMapDao.listByVmId(1L)).thenReturn(Arrays.asList( + new VmIsoMapVO(1L, 11L, TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 1), + new VmIsoMapVO(1L, 12L, TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 2))); + + Assert.assertEquals(ids(3L, 4L, 5L), templateManager.getCdromDeviceIds(vm)); + } + + private void mockVmNetworkProvidesConfigDrive(UserVmVO vm, boolean providesConfigDrive) { + NicVO nic = Mockito.mock(NicVO.class); + Mockito.when(nic.getNetworkId()).thenReturn(100L); + Mockito.when(_nicDao.listByVmId(vm.getId())).thenReturn(java.util.Collections.singletonList(nic)); + Mockito.lenient().when(_networkModel.isProviderSupportServiceInNetwork(Mockito.eq(100L), Mockito.any(Service.class), Mockito.eq(Provider.ConfigDrive))).thenReturn(providesConfigDrive); + } + + private void withConfiguredMaxCdroms(String value, Runnable test) { + Object original = org.springframework.test.util.ReflectionTestUtils.getField(TemplateManager.VmIsoMaxCount, "_defaultValue"); + org.springframework.test.util.ReflectionTestUtils.setField(TemplateManager.VmIsoMaxCount, "_defaultValue", value); + try { + test.run(); + } finally { + org.springframework.test.util.ReflectionTestUtils.setField(TemplateManager.VmIsoMaxCount, "_defaultValue", original); + } + } + + @Test + public void isConfigDriveUsedReturnsTrueWhenNetworkProvidesConfigDriveBeforeFirstStart() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(null); + mockVmNetworkProvidesConfigDrive(vm, true); + + Assert.assertTrue(templateManager.isConfigDriveUsed(vm)); + } + + @Test + public void isConfigDriveUsedReturnsTrueWhenConfigDriveLocationDetailExists() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(null); + Mockito.when(_vmInstanceDetailsDao.findDetail(1L, VmDetailConstants.CONFIG_DRIVE_LOCATION)).thenReturn(Mockito.mock(VMInstanceDetailVO.class)); + + Assert.assertTrue(templateManager.isConfigDriveUsed(vm)); + } + + @Test + public void isConfigDriveUsedReturnsFalseWhenNoNetworkProvidesConfigDrive() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(null); + mockVmNetworkProvidesConfigDrive(vm, false); + + Assert.assertFalse(templateManager.isConfigDriveUsed(vm)); + } + + @Test + public void getCdromDeviceIdsAddsTheCdromOfConfigDrive() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); + mockVmNetworkProvidesConfigDrive(vm, true); + + // the config drive ISO is one more CD-ROM than the ones supported for the ISOs + Assert.assertEquals(ids(3L, 4L, 5L), templateManager.getCdromDeviceIds(vm)); + Assert.assertEquals(ids(3L, 4L), templateManager.getCdromDeviceIds(vm, false)); + Assert.assertEquals(ids(3L, 4L, 5L), templateManager.getCdromDeviceIds(vm, true)); + } + + @Test + public void getIsoDeviceSeqKeepsTheDeviceIdOfConfigDrive() { + // without config drive, the CD-ROMs of the ISOs are the ones of the slots + Assert.assertEquals(3, templateManager.getIsoDeviceSeq(3, false)); + Assert.assertEquals(4, templateManager.getIsoDeviceSeq(4, false)); + Assert.assertEquals(5, templateManager.getIsoDeviceSeq(5, false)); + // with config drive (device id 4), the slots from it are moved to the next device ids + Assert.assertEquals(3, templateManager.getIsoDeviceSeq(3, true)); + Assert.assertEquals(5, templateManager.getIsoDeviceSeq(4, true)); + Assert.assertEquals(6, templateManager.getIsoDeviceSeq(5, true)); + } + + private java.util.List prepareIsoDisks(UserVmVO vm) { + // no ISO is attached to the Instance + Mockito.when(vm.getIsoId()).thenReturn(null); + VirtualMachineProfile profile = Mockito.mock(VirtualMachineProfile.class); + Mockito.when(profile.getId()).thenReturn(1L); + DeployDestination dest = Mockito.mock(DeployDestination.class); + Host host = Mockito.mock(Host.class); + Mockito.when(host.getId()).thenReturn(7L); + Mockito.when(dest.getHost()).thenReturn(host); + Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); + + templateManager.prepareIsoForVmProfile(profile, dest); + + org.mockito.ArgumentCaptor disks = org.mockito.ArgumentCaptor.forClass(DiskTO.class); + Mockito.verify(profile, Mockito.atLeast(0)).addDisk(disks.capture()); + return disks.getAllValues(); + } + + @Test + public void prepareIsoForVmProfileSkipsTheDeviceIdOfConfigDrive() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); + mockVmNetworkProvidesConfigDrive(vm, true); + + withConfiguredMaxCdroms("2", () -> { + java.util.List disks = prepareIsoDisks(vm); + + // the config drive ISO has the device id 4 (it is added by the config drive element), so the second ISO is at 5 + Assert.assertEquals(Arrays.asList(3L, 5L), disks.stream().map(DiskTO::getDiskSeq).collect(java.util.stream.Collectors.toList())); + }); + } + + @Test + public void prepareIsoForVmProfileUsesConsecutiveDeviceIdsWithoutConfigDrive() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); + mockVmNetworkProvidesConfigDrive(vm, false); + + withConfiguredMaxCdroms("2", () -> { + java.util.List disks = prepareIsoDisks(vm); + + Assert.assertEquals(Arrays.asList(3L, 4L), disks.stream().map(DiskTO::getDiskSeq).collect(java.util.stream.Collectors.toList())); + }); + } } 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 ec8270a2b79b..65d0fd0e4d88 100644 --- a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java @@ -34,9 +34,11 @@ import java.util.Collections; import java.util.Date; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.UUID; import org.apache.cloudstack.api.ApiConstants; @@ -77,6 +79,7 @@ import org.mockito.MockitoAnnotations; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.test.util.ReflectionTestUtils; import com.cloud.agent.AgentManager; import com.cloud.agent.api.Answer; @@ -2025,4 +2028,25 @@ public void testAutoFillAcceptsBareAddressWithoutPrefix() { Map.of("Network adapter 1", java.util.List.of("10.1.1.151"))); Assert.assertEquals("10.1.1.151", result.get("Network adapter 1").getIp4Address()); } + + @Test + public void getReservedDeviceIdsAlwaysReservesTheCdromOfConfigDrive() { + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + // the networks of the Instance are not imported yet, so the config drive is always assumed + Mockito.when(volumeApiService.getReservedDeviceIds(vm, true)).thenReturn(new HashSet<>(java.util.Arrays.asList(3L, 4L))); + + Set reserved = ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getReservedDeviceIds", vm); + + Assert.assertEquals(new HashSet<>(java.util.Arrays.asList(3L, 4L)), reserved); + } + + @Test + public void getNextDeviceIdSkipsReservedDeviceIds() { + Set reserved = new HashSet<>(java.util.Arrays.asList(3L, 4L)); + + Assert.assertEquals(1L, (long) ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getNextDeviceId", 0L, reserved)); + Assert.assertEquals(2L, (long) ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getNextDeviceId", 1L, reserved)); + Assert.assertEquals(5L, (long) ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getNextDeviceId", 2L, reserved)); + Assert.assertEquals(6L, (long) ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getNextDeviceId", 5L, reserved)); + } }