From 1dddc59ad7ebd1544f710bfa40ab5ffb7edf0f7a Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Tue, 13 Oct 2026 18:11:44 +0200 Subject: [PATCH 1/6] server: Reserve CD-ROM and config drive device ids for volumes Data volumes could be assigned the same device id as one of the CD-ROM devices 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). With multiple CD-ROM support, the KVM agent defines all the CD-ROMs supported by the host at boot, so any slot after the primary one can conflict. 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 number of CD-ROM devices is returned by TemplateManager.getVmCdromCount(), based on the CD-ROM cap advertised by the host and the ISOs attached. 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. --- .../com/cloud/storage/VolumeApiService.java | 12 ++ .../com/cloud/template/TemplateManager.java | 12 ++ .../java/com/cloud/hypervisor/KVMGuru.java | 29 ++-- .../element/ConfigDriveNetworkElement.java | 2 +- .../cloud/storage/VolumeApiServiceImpl.java | 84 ++++++++-- .../cloud/template/TemplateManagerImpl.java | 15 ++ .../vm/UnmanagedVMsManagerImpl.java | 28 +++- .../com/cloud/hypervisor/KVMGuruTest.java | 46 ++++++ .../storage/VolumeApiServiceImplTest.java | 150 ++++++++++++++++++ .../template/TemplateManagerImplTest.java | 47 ++++++ .../vm/UnmanagedVMsManagerImplTest.java | 23 +++ 11 files changed, 419 insertions(+), 29 deletions(-) diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java index 372eb0385618..e1cd60952e29 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,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 getReservedDeviceIds(VirtualMachine vm); + 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..7943f4c57d2e 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 @@ -37,6 +37,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 +184,17 @@ public interface TemplateManager { */ void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestination dest); + /** + * Returns the number of CD-ROM devices of the Instance. The CD-ROM devices occupy the device ids starting from + * {@link #CDROM_PRIMARY_DEVICE_SEQ}, which therefore 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. + * + * @param vm the Instance + * @return the number of CD-ROM devices + */ + int getVmCdromCount(VirtualMachine vm); + 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/server/src/main/java/com/cloud/hypervisor/KVMGuru.java b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java index 4aa67c5835f0..e44802200f4b 100644 --- a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java +++ b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java @@ -27,6 +27,7 @@ import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao; import com.cloud.hypervisor.kvm.dpdk.DpdkHelper; +import com.cloud.network.element.ConfigDriveNetworkElement; import com.cloud.service.ServiceOfferingVO; import com.cloud.storage.DataStoreRole; import com.cloud.storage.GuestOSHypervisorVO; @@ -56,8 +57,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.utils.bytescale.ByteScaleUtils; public class KVMGuru extends HypervisorGuruBase implements HypervisorGuru { @@ -91,30 +94,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 +375,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 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); @@ -383,7 +388,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 +411,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..9cd397b742e4 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -165,6 +165,10 @@ 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; import com.cloud.org.Grouping; @@ -224,6 +228,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.UserVmManager; import com.cloud.vm.UserVmService; import com.cloud.vm.UserVmVO; @@ -245,6 +250,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.VMInstanceDao; import com.cloud.vm.dao.VMInstanceDetailsDao; @@ -297,6 +303,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; @@ -5357,14 +5367,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 _tmpltMgr.getVmCdromCount(vm); + } + + /** + * 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 getReservedDeviceIds(VirtualMachine vm) { + Set 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 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 +5438,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..da19381525ab 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -714,6 +714,21 @@ public void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestinat } } + @Override + public int getVmCdromCount(VirtualMachine vm) { + 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))); + } + return cdromCount; + } + private Long singleStoragePoolId(DeployDestination dest) { Long poolId = null; Map storageForDisks = dest.getStorageForDisks(); 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..a884361eaa79 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -80,6 +80,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; @@ -898,6 +899,23 @@ 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) { + Set reservedDeviceIds = new HashSet<>(volumeApiService.getReservedDeviceIds(vm)); + reservedDeviceIds.add(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue()); + return reservedDeviceIds; + } + + 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 +1245,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 +1255,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 +3409,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 +3418,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..a597b8bb3010 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -37,7 +37,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutionException; @@ -109,6 +111,10 @@ import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor.HypervisorType; +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.Grouping; import com.cloud.projects.Project; @@ -128,6 +134,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; @@ -140,12 +147,17 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.fsm.NoTransitionException; import com.cloud.vm.DiskProfile; +import com.cloud.vm.NicVO; import com.cloud.vm.UserVmManager; import com.cloud.vm.UserVmVO; +import com.cloud.vm.VMInstanceDetailVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.State; +import com.cloud.vm.VmDetailConstants; import com.cloud.vm.VirtualMachineManager; +import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.dao.VMInstanceDao; import com.cloud.vm.snapshot.VMSnapshot; import com.cloud.vm.snapshot.VMSnapshotDetailsVO; @@ -164,6 +176,12 @@ public class VolumeApiServiceImplTest { @Mock private VolumeDataStoreDao volumeDataStoreDaoMock; @Mock + private VMInstanceDetailsDao vmInstanceDetailsDaoMock; + @Mock + private NicDao nicDaoMock; + @Mock + private NetworkModel networkModelMock; + @Mock private VolumeDao volumeDaoMock; @Mock private BackupDao backupDaoMock; @@ -2896,4 +2914,136 @@ 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 mockVmNetworkProvidesConfigDrive(UserVmVO vm, boolean providesConfigDrive) { + NicVO nic = Mockito.mock(NicVO.class); + lenient().when(nic.getNetworkId()).thenReturn(100L); + lenient().when(nicDaoMock.listByVmId(vm.getId())).thenReturn(Collections.singletonList(nic)); + lenient().when(networkModelMock.isProviderSupportServiceInNetwork(eq(100L), eq(Service.UserData), eq(Provider.ConfigDrive))).thenReturn(providesConfigDrive); + lenient().doReturn(1).when(volumeApiServiceImpl).getVmCdromCount(vm); + } + + 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 getReservedDeviceIdsReturnsOnlyCdromWhenVmDoesNotUseConfigDrive() { + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, false); + + Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); + + Assert.assertEquals(Collections.singleton((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ), reserved); + } + + @Test + public void getReservedDeviceIdsIncludesConfigDriveWhenNetworkProvidesItBeforeFirstStart() { + // the config drive location detail is only added when the ISO is created, i.e. at the first start + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, true); + + Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); + + Assert.assertEquals(new HashSet<>(Arrays.asList((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ, ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue())), reserved); + } + + @Test + public void getReservedDeviceIdsIncludesConfigDriveWhenConfigDriveLocationDetailExists() { + UserVmVO vm = getMockedVm(); + when(vmInstanceDetailsDaoMock.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION)).thenReturn(mock(VMInstanceDetailVO.class)); + doReturn(1).when(volumeApiServiceImpl).getVmCdromCount(vm); + + Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); + + Assert.assertTrue(reserved.contains(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue())); + } + + @Test + public void getReservedDeviceIdsReservesOneDeviceIdPerCdrom() { + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, false); + doReturn(3).when(volumeApiServiceImpl).getVmCdromCount(vm); + + Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); + + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)), reserved); + } + + @Test + public void getDeviceIdUsesDeviceId4WhenVmDoesNotUseConfigDrive() throws Exception { + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, false); + mockVmVolumes(vm, 0L, 1L, 2L); + + Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, null)); + Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, 4L)); + } + + @Test + public void getDeviceIdSkipsConfigDriveDeviceIdWhenVmUsesConfigDrive() throws Exception { + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, true); + mockVmVolumes(vm, 0L, 1L, 2L); + + Assert.assertEquals(Long.valueOf(5L), invokeGetDeviceId(vm, null)); + } + + @Test + public void getDeviceIdRejectsReservedDeviceIds() throws Exception { + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, true); + 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 getDeviceIdKeepsMaxDataVolumesWhenVmUsesConfigDrive() throws Exception { + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, true); + 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")); + } + } } diff --git a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java index 365f66e74115..a75939d2bbc6 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java @@ -1312,4 +1312,51 @@ public boolean match(MetadataReader mdr, MetadataReaderFactory arg1) throws IOEx } } } + + 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 getVmCdromCountReturnsOneForSystemVms() { + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.SecondaryStorageVm); + + Assert.assertEquals(1, templateManager.getVmCdromCount(vm)); + } + + @Test + public void getVmCdromCountReturnsAllCdromsAdvertisedByHostRegardlessOfConfiguredMaximum() { + // vm.iso.max.count defaults to 1, but the host defines all the CD-ROMs it supports + UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); + + Assert.assertEquals(2, templateManager.getVmCdromCount(vm)); + } + + @Test + public void getVmCdromCountReturnsDefaultWhenHostDoesNotAdvertiseCdromCap() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(null); + + Assert.assertEquals(TemplateManager.DEFAULT_CDROM_MAX_PER_VM, templateManager.getVmCdromCount(vm)); + } + + @Test + public void getVmCdromCountCoversAttachedIsosBeyondHostCap() { + 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(3, templateManager.getVmCdromCount(vm)); + } } 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..29448e961250 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,24 @@ 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 getReservedDeviceIdsAlwaysIncludesConfigDriveDeviceId() { + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Mockito.when(volumeApiService.getReservedDeviceIds(vm)).thenReturn(Collections.singleton(3L)); + + 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)); + } } From 30764893a9d72d5fe1f1526585bbf1c9fc588088 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Tue, 13 Oct 2026 19:46:38 +0200 Subject: [PATCH 2/6] server,kvm: Calculate the device id of the config drive ISO The config drive ISO is attached as a CD-ROM, but its device id was hardcoded to 4, which is also the device id of the second ISO CD-ROM when multiple ISOs are supported. With vm.iso.max.count >= 2 the Instance could get two CD-ROMs on the same device id. The config drive ISO is now the CD-ROM following the ones of the ISOs, so its device id is the primary CD-ROM device id plus the number of the CD-ROMs of the ISOs (TemplateManager.getConfigDriveDeviceId). As the config drive ISO takes one of the CD-ROMs supported by the host, the maximum number of ISOs of an Instance using config drive is lowered by one. The number of CD-ROM devices of an Instance (TemplateManager.getVmCdromCount) includes the config drive ISO, so the device ids reserved for the volumes follow it, and the volume device id allocators do not need to know about the config drive any more. The KVM agent finds the config drive CD-ROM by the path of its ISO, and keeps its label and bus when refreshing it, instead of using the hardcoded id 4 and the label hdd (which does not match the SATA CD-ROMs of UEFI Instances). --- .../com/cloud/template/TemplateManager.java | 12 +- .../resource/LibvirtComputingResource.java | 32 ++++- .../LibvirtComputingResourceTest.java | 32 +++++ .../element/ConfigDriveNetworkElement.java | 15 ++- .../cloud/storage/VolumeApiServiceImpl.java | 37 +----- .../cloud/template/TemplateManagerImpl.java | 75 +++++++++++- .../ConfigDriveNetworkElementTest.java | 8 ++ .../storage/VolumeApiServiceImplTest.java | 83 +++---------- .../template/TemplateManagerImplTest.java | 112 ++++++++++++++++++ 9 files changed, 287 insertions(+), 119 deletions(-) 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 7943f4c57d2e..cdc1165ac5f9 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 @@ -191,10 +191,20 @@ public interface TemplateManager { * whether or not ISOs are attached to them. * * @param vm the Instance - * @return the number of CD-ROM devices + * @return the number of CD-ROM devices, including the one of the config drive ISO if the Instance uses config drive */ int getVmCdromCount(VirtualMachine vm); + /** + * Returns the device id of the config drive ISO of the Instance. The config drive ISO is attached as the CD-ROM + * following the ones of the ISOs, so it is {@link #CDROM_PRIMARY_DEVICE_SEQ} plus the number of the CD-ROMs of the ISOs. + * + * @param vm the Instance + * @param hostId the host the Instance is (to be) deployed on + * @return the device id of the config drive ISO + */ + int getConfigDriveDeviceId(VirtualMachine vm, Long hostId); + 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/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 7c3f826be171..bec819705431 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -570,8 +570,6 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv protected List localStoragePaths = new ArrayList<>(); protected List localStorageUUIDs = new ArrayList<>(); - private static final String CONFIG_DRIVE_ISO_DISK_LABEL = "hdd"; - private static final int CONFIG_DRIVE_ISO_DEVICE_ID = 4; protected File qemuSocketsPath; private final String qemuGuestAgentSocketName = "org.qemu.guest_agent.0"; @@ -4163,24 +4161,33 @@ protected KVMStoragePoolManager getPoolManager() { return storagePoolManager; } + /** + * Checks if the CD-ROM is the one of the config drive ISO of the VM. The device id (and so the label) of the config + * drive ISO depends on the number of the CD-ROMs of the VM, so it is found by the path of the ISO. + */ + protected boolean isConfigDriveCdrom(final DiskDef disk, final String vmName) { + return disk.getDeviceType() == DiskDef.DeviceType.CDROM && disk.getDiskPath() != null + && disk.getDiskPath().endsWith("/" + ConfigDrive.createConfigDrivePath(vmName)); + } + public void detachAndAttachConfigDriveISO(final Connect conn, final String vmName) { // detach and re-attach configdrive ISO List disks = getDisks(conn, vmName); DiskDef configdrive = null; for (DiskDef disk : disks) { - if (disk.getDeviceType() == DiskDef.DeviceType.CDROM && CONFIG_DRIVE_ISO_DISK_LABEL.equals(disk.getDiskLabel())) { + if (isConfigDriveCdrom(disk, vmName)) { configdrive = disk; } } if (configdrive != null) { try { LOGGER.debug(String.format("Detaching ConfigDrive ISO of the VM %s, at path %s", vmName, configdrive.getDiskPath())); - String result = attachOrDetachISO(conn, vmName, configdrive.getDiskPath(), false, CONFIG_DRIVE_ISO_DEVICE_ID); + String result = attachOrDetachISO(conn, vmName, configdrive.getDiskPath(), false, null, configdrive); if (result != null) { LOGGER.warn("Detach ConfigDrive ISO of the VM {}, at path {} with result: {}", vmName, configdrive.getDiskPath(), result); } LOGGER.debug(String.format("Attaching ConfigDrive ISO of the VM %s, at path %s", vmName, configdrive.getDiskPath())); - result = attachOrDetachISO(conn, vmName, configdrive.getDiskPath(), true, CONFIG_DRIVE_ISO_DEVICE_ID); + result = attachOrDetachISO(conn, vmName, configdrive.getDiskPath(), true, null, configdrive); if (result != null) { LOGGER.warn("Attach ConfigDrive ISO of the VM {}, at path {} with result: {}", vmName, configdrive.getDiskPath(), result); } @@ -4193,6 +4200,15 @@ public void detachAndAttachConfigDriveISO(final Connect conn, final String vmNam public synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, final Integer diskSeq) throws LibvirtException, URISyntaxException, InternalErrorException { + return attachOrDetachISO(conn, vmName, isoPath, isAttach, diskSeq, null); + } + + /** + * Attaches or detaches (ejects) an ISO. If {@code cdrom} is specified, the ISO is attached to that CD-ROM device + * (the label and bus of the device are kept), instead of the one of the device id. + */ + private synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, final Integer diskSeq, final DiskDef cdrom) throws LibvirtException, URISyntaxException, + InternalErrorException { final DiskDef iso = new DiskDef(); if (isAttach && StringUtils.isNotBlank(isoPath)) { if (isoPath.startsWith(getConfigPath() + "/" + ConfigDrive.CONFIGDRIVEDIR) || isoPath.contains(vmName)) { @@ -4211,13 +4227,17 @@ public synchronized String attachOrDetachISO(final Connect conn, final String vm } else { iso.defISODisk(null, diskSeq, DiskDef.DiskType.FILE); } + if (cdrom != null) { + iso.setDiskLabel(cdrom.getDiskLabel()); + iso.setBusType(cdrom.getBusType()); + } final String result = attachOrDetachDevice(conn, true, vmName, iso.toString()); if (result == null && !isAttach) { final List disks = getDisks(conn, vmName); for (final DiskDef disk : disks) { if (disk.getDeviceType() == DiskDef.DeviceType.CDROM - && (diskSeq == null || disk.getDiskLabel().equals(iso.getDiskLabel()))) { + && ((diskSeq == null && cdrom == null) || disk.getDiskLabel().equals(iso.getDiskLabel()))) { cleanupDisk(disk); } } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index 33fb377bc90e..71166af9bc86 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -7730,4 +7730,36 @@ public void getPeriodAndQuotaTestPeriodIsEqualToMaximumWhenRequired() { Pair result = libvirtComputingResourceSpy.getPeriodAndQuota(cpuQuotaPercentage); Assert.assertEquals(expectedResult, result); } + + private DiskDef createCdromWithSource(String path, String label, DiskDef.DiskBus bus) { + DiskDef cdrom = new DiskDef(); + cdrom.defISODisk(path, null, label, DiskDef.DiskType.FILE); + cdrom.setBusType(bus); + return cdrom; + } + + @Test + public void isConfigDriveCdromTestFindsConfigDriveIsoWhateverTheDeviceLabel() { + String vmName = "i-2-3-VM"; + // the config drive ISO is the CD-ROM following the ones of the ISOs, so its label depends on the number of the CD-ROMs + Assert.assertTrue(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/mnt/pool/configdrive/i-2-3-VM.iso", "hdd", DiskDef.DiskBus.IDE), vmName)); + Assert.assertTrue(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/mnt/pool/configdrive/i-2-3-VM.iso", "sde", DiskDef.DiskBus.SATA), vmName)); + Assert.assertTrue(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/var/cache/cloud/configdrive/i-2-3-VM.iso", "sdf", DiskDef.DiskBus.SATA), vmName)); + } + + @Test + public void isConfigDriveCdromTestIgnoresOtherCdroms() { + String vmName = "i-2-3-VM"; + Assert.assertFalse(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/mnt/pool/some-ubuntu.iso", "hdc", DiskDef.DiskBus.IDE), vmName)); + Assert.assertFalse(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/mnt/pool/configdrive/i-2-30-VM.iso", "hdd", DiskDef.DiskBus.IDE), vmName)); + Assert.assertFalse(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource(null, "hdc", DiskDef.DiskBus.IDE), vmName)); + } + + @Test + public void isConfigDriveCdromTestIgnoresDisks() { + DiskDef disk = new DiskDef(); + disk.defFileBasedDisk("/mnt/pool/configdrive/i-2-3-VM.iso", 1, DiskDef.DiskBus.VIRTIO, DiskDef.DiskFmtType.RAW); + + Assert.assertFalse(libvirtComputingResourceSpy.isConfigDriveCdrom(disk, "i-2-3-VM")); + } } 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 fbc0d106edcf..efbce2e38e49 100644 --- a/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java +++ b/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java @@ -73,6 +73,7 @@ import com.cloud.storage.dao.GuestOSCategoryDao; import com.cloud.storage.dao.GuestOSDao; import com.cloud.storage.dao.VolumeDao; +import com.cloud.template.TemplateManager; import com.cloud.utils.Pair; import com.cloud.utils.StringUtils; import com.cloud.utils.component.AdapterBase; @@ -114,6 +115,8 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle @Inject NetworkModel _networkModel; @Inject + TemplateManager _templateMgr; + @Inject NetworkOrchestrationService _networkOrchestrationService; @Inject GuestOSCategoryDao _guestOSCategoryDao; @@ -142,7 +145,9 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle @Inject private HypervisorGuruManager _hvGuruMgr; - public static final Integer CONFIGDRIVEDISKSEQ = 4; + // Device id of the config drive ISO when the Instance has a single ISO CD-ROM (the config drive ISO is the CD-ROM + // following the ones of the ISOs, see TemplateManager#getConfigDriveDeviceId). Used where the Instance is not known. + public static final Integer CONFIGDRIVEDISKSEQ = TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 1; private boolean canHandle(TrafficType trafficType) { return trafficType.equals(TrafficType.Guest); @@ -591,7 +596,7 @@ private boolean createConfigDriveIsoOnHostCache(NicProfile nic, VirtualMachinePr profile.setConfigDriveLocation(answer.getConfigDriveLocation()); updateConfigDriveLocationInVMDetails(profile.getId(), answer.getConfigDriveLocation()); - addConfigDriveDisk(profile, null); + addConfigDriveDisk(profile, null, host.getId()); return true; } @@ -676,7 +681,7 @@ public boolean createConfigDriveIso(NicProfile nic, VirtualMachineProfile profil } profile.setConfigDriveLocation(answer.getConfigDriveLocation()); updateConfigDriveLocationInVMDetails(profile.getId(), answer.getConfigDriveLocation()); - addConfigDriveDisk(profile, dataStore); + addConfigDriveDisk(profile, dataStore, dest != null && dest.getHost() != null ? dest.getHost().getId() : null); return true; } @@ -776,7 +781,7 @@ private boolean deleteConfigDriveIso(final VirtualMachine vm) throws ResourceUna return true; } - private void addConfigDriveDisk(final VirtualMachineProfile profile, final DataStore dataStore) throws ResourceUnavailableException { + private void addConfigDriveDisk(final VirtualMachineProfile profile, final DataStore dataStore, final Long hostId) throws ResourceUnavailableException { boolean isoAvailable = false; final String isoPath = ConfigDrive.createConfigDrivePath(profile.getInstanceName()); for (DiskTO dataTo : profile.getDisks()) { @@ -798,7 +803,7 @@ private void addConfigDriveDisk(final VirtualMachineProfile profile, final DataS dataTO.setPath(isoPath); dataTO.setFormat(Storage.ImageFormat.ISO); - profile.addDisk(new DiskTO(dataTO, CONFIGDRIVEDISKSEQ.longValue(), isoPath, Volume.Type.ISO)); + profile.addDisk(new DiskTO(dataTO, (long) _templateMgr.getConfigDriveDeviceId(profile.getVirtualMachine(), hostId), isoPath, Volume.Type.ISO)); } else { logger.warn("Config drive ISO already is in Instance profile."); } diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 9cd397b742e4..0c0f7af51e36 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -165,10 +165,6 @@ 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; import com.cloud.org.Grouping; @@ -228,7 +224,6 @@ 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.UserVmManager; import com.cloud.vm.UserVmService; import com.cloud.vm.UserVmVO; @@ -250,7 +245,6 @@ 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.VMInstanceDao; import com.cloud.vm.dao.VMInstanceDetailsDao; @@ -303,10 +297,6 @@ 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; @@ -5368,42 +5358,19 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype } /** - * 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}. + * The number of CD-ROM devices of the Instance (including the one of the config drive ISO, if any). Each CD-ROM + * device occupies a device id, starting from {@link TemplateManager#CDROM_PRIMARY_DEVICE_SEQ}. */ protected int getVmCdromCount(VirtualMachine vm) { return _tmpltMgr.getVmCdromCount(vm); } - /** - * 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 getReservedDeviceIds(VirtualMachine vm) { Set 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; } diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index da19381525ab..abce0e4d1714 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -155,6 +155,9 @@ 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.projects.Project; import com.cloud.projects.ProjectManager; import com.cloud.resourcelimit.CheckedReservation; @@ -219,6 +222,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 +232,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 +271,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; @@ -719,14 +731,53 @@ public int getVmCdromCount(VirtualMachine vm) { if (vm.getType() != VirtualMachine.Type.User) { return 1; } + Long hostId = hostIdForVm(vm); + boolean configDriveUsed = isConfigDriveUsed(vm); // 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)); + int cdromCount = advertisedCdromCap(hostId); + int isoSlots = getIsoSlotCount(vm, hostId, configDriveUsed); + // the config drive ISO is attached as the CD-ROM following the ones of the ISOs + return Math.max(cdromCount, isoSlots + (configDriveUsed ? 1 : 0)); + } + + @Override + public int getConfigDriveDeviceId(VirtualMachine vm, Long hostId) { + // the config drive ISO is attached as the CD-ROM following the ones of the ISOs + return CDROM_PRIMARY_DEVICE_SEQ + getIsoSlotCount(vm, hostId, true); + } + + /** + * The number of CD-ROM devices of the Instance which are used for ISOs, i.e. the ones allocated at boot. + */ + private int getIsoSlotCount(VirtualMachine vm, Long hostId, boolean configDriveUsed) { UserVmVO userVm = _userVmDao.findById(vm.getId()); - if (userVm != null) { - cdromCount = Math.max(cdromCount, slotsNeededFor(loadAttachedIsoSlots(userVm))); + int slotsNeeded = userVm != null ? slotsNeededFor(loadAttachedIsoSlots(userVm)) : 0; + return Math.max(effectiveMaxCdroms(vm, hostId, configDriveUsed), slotsNeeded); + } + + /** + * 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 cdromCount; + return false; } private Long singleStoragePoolId(DeployDestination dest) { @@ -1588,16 +1639,28 @@ void enforceCdromAttachLimits(long vmId, UserVm vm, long isoId) { } int effectiveMaxCdroms(VirtualMachine vm, Long hostId) { + return effectiveMaxCdroms(vm, hostId, isConfigDriveUsed(vm)); + } + + /** + * The maximum number of ISOs which can be attached to the Instance. The config drive ISO, if any, is attached as a + * CD-ROM as well, and therefore takes one of the CD-ROM devices supported by the host. + */ + int effectiveMaxCdroms(VirtualMachine vm, Long hostId, boolean configDriveUsed) { HostVO host = hostId != null ? _hostDao.findById(hostId) : null; Long clusterId = host != null ? host.getClusterId() : null; int configuredCap = VmIsoMaxCount.valueIn(clusterId); int hypervisorCap = advertisedCdromCap(hostId); + int effectiveMax = configuredCap; if (configuredCap > hypervisorCap) { logger.warn("{} is set to {} but the placement host supports a maximum of {} CD-ROM(s) per Instance. Clamping to {}.", VmIsoMaxCount.key(), configuredCap, hypervisorCap, hypervisorCap); - return hypervisorCap; + effectiveMax = hypervisorCap; + } + if (configDriveUsed && hypervisorCap > 1) { + effectiveMax = Math.min(effectiveMax, hypervisorCap - 1); } - return configuredCap; + return effectiveMax; } int advertisedCdromCap(Long hostId) { diff --git a/server/src/test/java/com/cloud/network/element/ConfigDriveNetworkElementTest.java b/server/src/test/java/com/cloud/network/element/ConfigDriveNetworkElementTest.java index 336f9990c12b..e5c2e8ec67e2 100644 --- a/server/src/test/java/com/cloud/network/element/ConfigDriveNetworkElementTest.java +++ b/server/src/test/java/com/cloud/network/element/ConfigDriveNetworkElementTest.java @@ -49,6 +49,7 @@ import com.cloud.utils.fsm.StateListener; import com.cloud.utils.fsm.StateMachine2; import com.cloud.utils.net.Ip; +import com.cloud.template.TemplateManager; import com.cloud.vm.Nic; import com.cloud.vm.NicProfile; import com.cloud.vm.VMInstanceDetailVO; @@ -92,6 +93,7 @@ import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -128,6 +130,7 @@ public class ConfigDriveNetworkElementTest { @Mock private HostDao _hostDao; @Mock private ServiceOfferingDao _serviceOfferingDao; @Mock private UserVmDao _vmDao; + @Mock private TemplateManager _templateMgr; @Mock private VMInstanceDao _vmInstanceDao; @Mock private VMInstanceDetailsDao _vmInstanceDetailsDao; @Mock private NetworkDao _networkDao; @@ -306,6 +309,7 @@ public void testCreateConfigDriveIso() throws Exception { when(answer.getResult()).thenReturn(true); when(answer.getConfigDriveLocation()).thenReturn(NetworkElement.Location.PRIMARY); when(virtualMachine.getUuid()).thenReturn("vm-uuid"); + when(_templateMgr.getConfigDriveDeviceId(Mockito.any(), Mockito.any())).thenReturn(5); Map parms = Maps.newHashMap(); parms.put(VirtualMachineProfile.Param.VmPassword, PASSWORD); @@ -315,6 +319,10 @@ public void testCreateConfigDriveIso() throws Exception { profile.setVmData(Collections.emptyList()); assertTrue(_configDrivesNetworkElement.createConfigDriveIso(nicp, profile, deployDestination, null)); + // the device id of the config drive ISO is the one calculated from the number of the CD-ROMs of the Instance + assertEquals(1, profile.getDisks().size()); + assertEquals(Long.valueOf(5L), profile.getDisks().get(0).getDiskSeq()); + ArgumentCaptor commandCaptor = ArgumentCaptor.forClass(HandleConfigDriveIsoCommand.class); verify(agentManager, times(1)).easySend(Mockito.anyLong(), commandCaptor.capture()); HandleConfigDriveIsoCommand createCommand = commandCaptor.getValue(); diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index a597b8bb3010..029160a754a5 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -39,7 +39,6 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutionException; @@ -111,10 +110,6 @@ import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor.HypervisorType; -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.Grouping; import com.cloud.projects.Project; @@ -134,7 +129,6 @@ 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; @@ -147,17 +141,12 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.fsm.NoTransitionException; import com.cloud.vm.DiskProfile; -import com.cloud.vm.NicVO; import com.cloud.vm.UserVmManager; import com.cloud.vm.UserVmVO; -import com.cloud.vm.VMInstanceDetailVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.State; -import com.cloud.vm.VmDetailConstants; import com.cloud.vm.VirtualMachineManager; -import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; -import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.dao.VMInstanceDao; import com.cloud.vm.snapshot.VMSnapshot; import com.cloud.vm.snapshot.VMSnapshotDetailsVO; @@ -176,12 +165,6 @@ public class VolumeApiServiceImplTest { @Mock private VolumeDataStoreDao volumeDataStoreDaoMock; @Mock - private VMInstanceDetailsDao vmInstanceDetailsDaoMock; - @Mock - private NicDao nicDaoMock; - @Mock - private NetworkModel networkModelMock; - @Mock private VolumeDao volumeDaoMock; @Mock private BackupDao backupDaoMock; @@ -2925,12 +2908,8 @@ private Long invokeGetDeviceId(UserVmVO vm, Long deviceId) throws Exception { } } - private void mockVmNetworkProvidesConfigDrive(UserVmVO vm, boolean providesConfigDrive) { - NicVO nic = Mockito.mock(NicVO.class); - lenient().when(nic.getNetworkId()).thenReturn(100L); - lenient().when(nicDaoMock.listByVmId(vm.getId())).thenReturn(Collections.singletonList(nic)); - lenient().when(networkModelMock.isProviderSupportServiceInNetwork(eq(100L), eq(Service.UserData), eq(Provider.ConfigDrive))).thenReturn(providesConfigDrive); - lenient().doReturn(1).when(volumeApiServiceImpl).getVmCdromCount(vm); + private void mockVmCdromCount(UserVmVO vm, int cdromCount) { + lenient().doReturn(cdromCount).when(volumeApiServiceImpl).getVmCdromCount(vm); } private List mockVmVolumes(UserVmVO vm, long... deviceIds) { @@ -2944,53 +2923,25 @@ private List mockVmVolumes(UserVmVO vm, long... deviceIds) { return volumes; } - @Test - public void getReservedDeviceIdsReturnsOnlyCdromWhenVmDoesNotUseConfigDrive() { - UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, false); - - Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); - - Assert.assertEquals(Collections.singleton((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ), reserved); - } - - @Test - public void getReservedDeviceIdsIncludesConfigDriveWhenNetworkProvidesItBeforeFirstStart() { - // the config drive location detail is only added when the ISO is created, i.e. at the first start - UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); - - Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); - - Assert.assertEquals(new HashSet<>(Arrays.asList((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ, ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue())), reserved); - } - - @Test - public void getReservedDeviceIdsIncludesConfigDriveWhenConfigDriveLocationDetailExists() { - UserVmVO vm = getMockedVm(); - when(vmInstanceDetailsDaoMock.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION)).thenReturn(mock(VMInstanceDetailVO.class)); - doReturn(1).when(volumeApiServiceImpl).getVmCdromCount(vm); - - Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); - - Assert.assertTrue(reserved.contains(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue())); - } - @Test public void getReservedDeviceIdsReservesOneDeviceIdPerCdrom() { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, false); - doReturn(3).when(volumeApiServiceImpl).getVmCdromCount(vm); - Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); + mockVmCdromCount(vm, 1); + 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 + mockVmCdromCount(vm, 2); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); - Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)), reserved); + mockVmCdromCount(vm, 3); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); } @Test - public void getDeviceIdUsesDeviceId4WhenVmDoesNotUseConfigDrive() throws Exception { + public void getDeviceIdUsesDeviceId4WhenVmHasOneCdrom() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, false); + mockVmCdromCount(vm, 1); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, null)); @@ -2998,9 +2949,9 @@ public void getDeviceIdUsesDeviceId4WhenVmDoesNotUseConfigDrive() throws Excepti } @Test - public void getDeviceIdSkipsConfigDriveDeviceIdWhenVmUsesConfigDrive() throws Exception { + public void getDeviceIdSkipsDeviceIdsOfAllCdroms() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); + mockVmCdromCount(vm, 2); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(5L), invokeGetDeviceId(vm, null)); @@ -3009,7 +2960,7 @@ public void getDeviceIdSkipsConfigDriveDeviceIdWhenVmUsesConfigDrive() throws Ex @Test public void getDeviceIdRejectsReservedDeviceIds() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); + mockVmCdromCount(vm, 2); mockVmVolumes(vm, 0L); for (long reservedDeviceId : new long[] {3L, 4L}) { @@ -3023,9 +2974,9 @@ public void getDeviceIdRejectsReservedDeviceIds() throws Exception { } @Test - public void getDeviceIdKeepsMaxDataVolumesWhenVmUsesConfigDrive() throws Exception { + public void getDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); + mockVmCdromCount(vm, 2); List volumes = mockVmVolumes(vm, 0L); // 6 data volumes are supported by default diff --git a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java index a75939d2bbc6..16ec41ec5978 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java @@ -82,6 +82,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,12 +123,17 @@ 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.VMInstanceDetailVO; import com.cloud.vm.UserVmVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.State; import com.cloud.vm.VmIsoMapVO; +import com.cloud.vm.VmDetailConstants; +import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.dao.VMInstanceDao; import com.cloud.vm.dao.VmIsoMapDao; @@ -249,6 +257,12 @@ public class TemplateManagerImplTest extends TestCase { @Mock HostDetailsDao _hostDetailsDao; + @Mock + NicDao _nicDao; + @Mock + NetworkModel _networkModel; + @Mock + VMInstanceDetailsDao _vmInstanceDetailsDao; @Mock UserVmJoinDao _userVmJoinDao; @@ -1359,4 +1373,102 @@ public void getVmCdromCountCoversAttachedIsosBeyondHostCap() { Assert.assertEquals(3, templateManager.getVmCdromCount(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 effectiveMaxCdromsLeavesACdromForConfigDrive() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); + Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); + + withConfiguredMaxCdroms("2", () -> { + Assert.assertEquals(2, templateManager.effectiveMaxCdroms(vm, 7L, false)); + Assert.assertEquals(1, templateManager.effectiveMaxCdroms(vm, 7L, true)); + }); + } + + @Test + public void effectiveMaxCdromsKeepsOneCdromWhenHostDoesNotAdvertiseCdromCap() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(null); + Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); + + Assert.assertEquals(1, templateManager.effectiveMaxCdroms(vm, 7L, true)); + } + + @Test + public void getVmCdromCountIncludesConfigDriveCdrom() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(null); + mockVmNetworkProvidesConfigDrive(vm, true); + + // the host does not advertise the number of CD-ROMs: one CD-ROM for ISOs, and one for the config drive ISO + Assert.assertEquals(2, templateManager.getVmCdromCount(vm)); + } + + @Test + public void getVmCdromCountDoesNotExceedHostCapWhenConfigDriveTakesACdrom() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); + Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); + mockVmNetworkProvidesConfigDrive(vm, true); + + withConfiguredMaxCdroms("2", () -> Assert.assertEquals(2, templateManager.getVmCdromCount(vm))); + } + + @Test + public void getConfigDriveDeviceIdIsTheCdromFollowingTheOnesOfIsos() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(3L); + Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); + + // vm.iso.max.count is 1 by default + Assert.assertEquals(TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 1, templateManager.getConfigDriveDeviceId(vm, 7L)); + withConfiguredMaxCdroms("3", () -> Assert.assertEquals(TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 2, templateManager.getConfigDriveDeviceId(vm, 7L))); + } + + @Test + public void getConfigDriveDeviceIdFollowsAttachedIsosBeyondConfiguredMaximum() { + UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); + Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); + Mockito.when(_vmIsoMapDao.listByVmId(1L)).thenReturn(Arrays.asList( + new VmIsoMapVO(1L, 11L, TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 1))); + + Assert.assertEquals(TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 2, templateManager.getConfigDriveDeviceId(vm, 7L)); + } } From 2baaf551b10dcd993b6224a07f4ce552a5f016de Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Thu, 15 Oct 2026 13:51:49 +0200 Subject: [PATCH 3/6] server,kvm: Do not count the config drive ISO in vm.iso.max.count vm.iso.max.count is the maximum number of ISOs of an Instance, so it should not take the config drive ISO into account: the config drive ISO is attached as an additional CD-ROM, following the ones of the ISOs. As the IDE bus only has the slots of the first MAX_CDROMS_PER_VM CD-ROMs (hdc and hdd), the KVM agent attaches the CD-ROMs beyond these slots (e.g. the config drive ISO following the maximum number of ISOs) on the SATA bus. --- .../hypervisor/kvm/resource/LibvirtVMDef.java | 12 +++++++- .../kvm/resource/LibvirtVMDefTest.java | 21 +++++++++++++ .../cloud/template/TemplateManagerImpl.java | 30 +++++++------------ .../template/TemplateManagerImplTest.java | 24 +++++---------- 4 files changed, 50 insertions(+), 37 deletions(-) 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..d51c48416950 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; @@ -58,7 +59,7 @@ 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). + // The CD-ROMs beyond these slots (e.g. the config drive ISO following the ISOs) are on the SATA bus. public static final int MAX_CDROMS_PER_VM = 2; private String _hvsType; @@ -1138,7 +1139,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/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index abce0e4d1714..84183c67b3bc 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -732,28 +732,27 @@ public int getVmCdromCount(VirtualMachine vm) { return 1; } Long hostId = hostIdForVm(vm); - boolean configDriveUsed = isConfigDriveUsed(vm); // 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(hostId); - int isoSlots = getIsoSlotCount(vm, hostId, configDriveUsed); - // the config drive ISO is attached as the CD-ROM following the ones of the ISOs - return Math.max(cdromCount, isoSlots + (configDriveUsed ? 1 : 0)); + int isoSlots = getIsoSlotCount(vm, hostId); + // the config drive ISO is attached as the CD-ROM following the ones of the ISOs (it is not counted in vm.iso.max.count) + return Math.max(cdromCount, isoSlots + (isConfigDriveUsed(vm) ? 1 : 0)); } @Override public int getConfigDriveDeviceId(VirtualMachine vm, Long hostId) { // the config drive ISO is attached as the CD-ROM following the ones of the ISOs - return CDROM_PRIMARY_DEVICE_SEQ + getIsoSlotCount(vm, hostId, true); + return CDROM_PRIMARY_DEVICE_SEQ + getIsoSlotCount(vm, hostId); } /** * The number of CD-ROM devices of the Instance which are used for ISOs, i.e. the ones allocated at boot. */ - private int getIsoSlotCount(VirtualMachine vm, Long hostId, boolean configDriveUsed) { + private int getIsoSlotCount(VirtualMachine vm, Long hostId) { UserVmVO userVm = _userVmDao.findById(vm.getId()); int slotsNeeded = userVm != null ? slotsNeededFor(loadAttachedIsoSlots(userVm)) : 0; - return Math.max(effectiveMaxCdroms(vm, hostId, configDriveUsed), slotsNeeded); + return Math.max(effectiveMaxCdroms(vm, hostId), slotsNeeded); } /** @@ -1638,29 +1637,20 @@ void enforceCdromAttachLimits(long vmId, UserVm vm, long isoId) { } } - int effectiveMaxCdroms(VirtualMachine vm, Long hostId) { - return effectiveMaxCdroms(vm, hostId, isConfigDriveUsed(vm)); - } - /** - * The maximum number of ISOs which can be attached to the Instance. The config drive ISO, if any, is attached as a - * CD-ROM as well, and therefore takes one of the CD-ROM devices supported by the host. + * The maximum number of ISOs which can be attached to the Instance. The config drive ISO is not counted. */ - int effectiveMaxCdroms(VirtualMachine vm, Long hostId, boolean configDriveUsed) { + int effectiveMaxCdroms(VirtualMachine vm, Long hostId) { HostVO host = hostId != null ? _hostDao.findById(hostId) : null; Long clusterId = host != null ? host.getClusterId() : null; int configuredCap = VmIsoMaxCount.valueIn(clusterId); int hypervisorCap = advertisedCdromCap(hostId); - int effectiveMax = configuredCap; if (configuredCap > hypervisorCap) { logger.warn("{} is set to {} but the placement host supports a maximum of {} CD-ROM(s) per Instance. Clamping to {}.", VmIsoMaxCount.key(), configuredCap, hypervisorCap, hypervisorCap); - effectiveMax = hypervisorCap; - } - if (configDriveUsed && hypervisorCap > 1) { - effectiveMax = Math.min(effectiveMax, hypervisorCap - 1); + return hypervisorCap; } - return effectiveMax; + return configuredCap; } int advertisedCdromCap(Long hostId) { diff --git a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java index 16ec41ec5978..3d4e76188e30 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java @@ -1416,22 +1416,13 @@ public void isConfigDriveUsedReturnsFalseWhenNoNetworkProvidesConfigDrive() { } @Test - public void effectiveMaxCdromsLeavesACdromForConfigDrive() { + public void effectiveMaxCdromsDoesNotCountConfigDrive() { UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); + mockVmNetworkProvidesConfigDrive(vm, true); - withConfiguredMaxCdroms("2", () -> { - Assert.assertEquals(2, templateManager.effectiveMaxCdroms(vm, 7L, false)); - Assert.assertEquals(1, templateManager.effectiveMaxCdroms(vm, 7L, true)); - }); - } - - @Test - public void effectiveMaxCdromsKeepsOneCdromWhenHostDoesNotAdvertiseCdromCap() { - UserVmVO vm = mockUserVmOnHostWithCdromCap(null); - Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); - - Assert.assertEquals(1, templateManager.effectiveMaxCdroms(vm, 7L, true)); + // the config drive ISO is attached as an additional CD-ROM, it does not take one of the ISOs + withConfiguredMaxCdroms("2", () -> Assert.assertEquals(2, templateManager.effectiveMaxCdroms(vm, 7L))); } @Test @@ -1444,12 +1435,13 @@ public void getVmCdromCountIncludesConfigDriveCdrom() { } @Test - public void getVmCdromCountDoesNotExceedHostCapWhenConfigDriveTakesACdrom() { + public void getVmCdromCountAddsTheCdromOfConfigDriveToTheOnesOfIsos() { UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); mockVmNetworkProvidesConfigDrive(vm, true); - withConfiguredMaxCdroms("2", () -> Assert.assertEquals(2, templateManager.getVmCdromCount(vm))); + // 2 CD-ROMs for ISOs (vm.iso.max.count), and 1 for the config drive ISO + withConfiguredMaxCdroms("2", () -> Assert.assertEquals(3, templateManager.getVmCdromCount(vm))); } @Test @@ -1459,7 +1451,7 @@ public void getConfigDriveDeviceIdIsTheCdromFollowingTheOnesOfIsos() { // vm.iso.max.count is 1 by default Assert.assertEquals(TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 1, templateManager.getConfigDriveDeviceId(vm, 7L)); - withConfiguredMaxCdroms("3", () -> Assert.assertEquals(TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 2, templateManager.getConfigDriveDeviceId(vm, 7L))); + withConfiguredMaxCdroms("3", () -> Assert.assertEquals(TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 3, templateManager.getConfigDriveDeviceId(vm, 7L))); } @Test From 4642ad1ee10e27e0f4175005cf3d88c7096bda5d Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Thu, 15 Oct 2026 16:31:26 +0200 Subject: [PATCH 4/6] Revert "server,kvm: Calculate the device id of the config drive ISO" The config drive ISO keeps the device id 4, whatever the number of CD-ROMs of the Instance. The CD-ROMs of the ISOs are attached around it. This reverts the commits: - server,kvm: Do not count the config drive ISO in vm.iso.max.count - server,kvm: Calculate the device id of the config drive ISO --- .../com/cloud/template/TemplateManager.java | 12 +- .../resource/LibvirtComputingResource.java | 32 +----- .../hypervisor/kvm/resource/LibvirtVMDef.java | 12 +- .../LibvirtComputingResourceTest.java | 32 ------ .../kvm/resource/LibvirtVMDefTest.java | 21 ---- .../element/ConfigDriveNetworkElement.java | 15 +-- .../cloud/storage/VolumeApiServiceImpl.java | 37 ++++++- .../cloud/template/TemplateManagerImpl.java | 61 +--------- .../ConfigDriveNetworkElementTest.java | 8 -- .../storage/VolumeApiServiceImplTest.java | 83 +++++++++++--- .../template/TemplateManagerImplTest.java | 104 ------------------ 11 files changed, 118 insertions(+), 299 deletions(-) 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 cdc1165ac5f9..7943f4c57d2e 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 @@ -191,20 +191,10 @@ public interface TemplateManager { * whether or not ISOs are attached to them. * * @param vm the Instance - * @return the number of CD-ROM devices, including the one of the config drive ISO if the Instance uses config drive + * @return the number of CD-ROM devices */ int getVmCdromCount(VirtualMachine vm); - /** - * Returns the device id of the config drive ISO of the Instance. The config drive ISO is attached as the CD-ROM - * following the ones of the ISOs, so it is {@link #CDROM_PRIMARY_DEVICE_SEQ} plus the number of the CD-ROMs of the ISOs. - * - * @param vm the Instance - * @param hostId the host the Instance is (to be) deployed on - * @return the device id of the config drive ISO - */ - int getConfigDriveDeviceId(VirtualMachine vm, Long hostId); - 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/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index bec819705431..7c3f826be171 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -570,6 +570,8 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv protected List localStoragePaths = new ArrayList<>(); protected List localStorageUUIDs = new ArrayList<>(); + private static final String CONFIG_DRIVE_ISO_DISK_LABEL = "hdd"; + private static final int CONFIG_DRIVE_ISO_DEVICE_ID = 4; protected File qemuSocketsPath; private final String qemuGuestAgentSocketName = "org.qemu.guest_agent.0"; @@ -4161,33 +4163,24 @@ protected KVMStoragePoolManager getPoolManager() { return storagePoolManager; } - /** - * Checks if the CD-ROM is the one of the config drive ISO of the VM. The device id (and so the label) of the config - * drive ISO depends on the number of the CD-ROMs of the VM, so it is found by the path of the ISO. - */ - protected boolean isConfigDriveCdrom(final DiskDef disk, final String vmName) { - return disk.getDeviceType() == DiskDef.DeviceType.CDROM && disk.getDiskPath() != null - && disk.getDiskPath().endsWith("/" + ConfigDrive.createConfigDrivePath(vmName)); - } - public void detachAndAttachConfigDriveISO(final Connect conn, final String vmName) { // detach and re-attach configdrive ISO List disks = getDisks(conn, vmName); DiskDef configdrive = null; for (DiskDef disk : disks) { - if (isConfigDriveCdrom(disk, vmName)) { + if (disk.getDeviceType() == DiskDef.DeviceType.CDROM && CONFIG_DRIVE_ISO_DISK_LABEL.equals(disk.getDiskLabel())) { configdrive = disk; } } if (configdrive != null) { try { LOGGER.debug(String.format("Detaching ConfigDrive ISO of the VM %s, at path %s", vmName, configdrive.getDiskPath())); - String result = attachOrDetachISO(conn, vmName, configdrive.getDiskPath(), false, null, configdrive); + String result = attachOrDetachISO(conn, vmName, configdrive.getDiskPath(), false, CONFIG_DRIVE_ISO_DEVICE_ID); if (result != null) { LOGGER.warn("Detach ConfigDrive ISO of the VM {}, at path {} with result: {}", vmName, configdrive.getDiskPath(), result); } LOGGER.debug(String.format("Attaching ConfigDrive ISO of the VM %s, at path %s", vmName, configdrive.getDiskPath())); - result = attachOrDetachISO(conn, vmName, configdrive.getDiskPath(), true, null, configdrive); + result = attachOrDetachISO(conn, vmName, configdrive.getDiskPath(), true, CONFIG_DRIVE_ISO_DEVICE_ID); if (result != null) { LOGGER.warn("Attach ConfigDrive ISO of the VM {}, at path {} with result: {}", vmName, configdrive.getDiskPath(), result); } @@ -4200,15 +4193,6 @@ public void detachAndAttachConfigDriveISO(final Connect conn, final String vmNam public synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, final Integer diskSeq) throws LibvirtException, URISyntaxException, InternalErrorException { - return attachOrDetachISO(conn, vmName, isoPath, isAttach, diskSeq, null); - } - - /** - * Attaches or detaches (ejects) an ISO. If {@code cdrom} is specified, the ISO is attached to that CD-ROM device - * (the label and bus of the device are kept), instead of the one of the device id. - */ - private synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, final Integer diskSeq, final DiskDef cdrom) throws LibvirtException, URISyntaxException, - InternalErrorException { final DiskDef iso = new DiskDef(); if (isAttach && StringUtils.isNotBlank(isoPath)) { if (isoPath.startsWith(getConfigPath() + "/" + ConfigDrive.CONFIGDRIVEDIR) || isoPath.contains(vmName)) { @@ -4227,17 +4211,13 @@ private synchronized String attachOrDetachISO(final Connect conn, final String v } else { iso.defISODisk(null, diskSeq, DiskDef.DiskType.FILE); } - if (cdrom != null) { - iso.setDiskLabel(cdrom.getDiskLabel()); - iso.setBusType(cdrom.getBusType()); - } final String result = attachOrDetachDevice(conn, true, vmName, iso.toString()); if (result == null && !isAttach) { final List disks = getDisks(conn, vmName); for (final DiskDef disk : disks) { if (disk.getDeviceType() == DiskDef.DeviceType.CDROM - && ((diskSeq == null && cdrom == null) || disk.getDiskLabel().equals(iso.getDiskLabel()))) { + && (diskSeq == null || disk.getDiskLabel().equals(iso.getDiskLabel()))) { cleanupDisk(disk); } } 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 d51c48416950..439e4f663416 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,7 +39,6 @@ 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; @@ -59,7 +58,7 @@ 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. - // The CD-ROMs beyond these slots (e.g. the config drive ISO following the ISOs) are on the SATA bus. + // Bumping this requires extending getDevLabel() (e.g. to spill onto SATA or a second IDE controller). public static final int MAX_CDROMS_PER_VM = 2; private String _hvsType; @@ -1139,16 +1138,7 @@ 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/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index 71166af9bc86..33fb377bc90e 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -7730,36 +7730,4 @@ public void getPeriodAndQuotaTestPeriodIsEqualToMaximumWhenRequired() { Pair result = libvirtComputingResourceSpy.getPeriodAndQuota(cpuQuotaPercentage); Assert.assertEquals(expectedResult, result); } - - private DiskDef createCdromWithSource(String path, String label, DiskDef.DiskBus bus) { - DiskDef cdrom = new DiskDef(); - cdrom.defISODisk(path, null, label, DiskDef.DiskType.FILE); - cdrom.setBusType(bus); - return cdrom; - } - - @Test - public void isConfigDriveCdromTestFindsConfigDriveIsoWhateverTheDeviceLabel() { - String vmName = "i-2-3-VM"; - // the config drive ISO is the CD-ROM following the ones of the ISOs, so its label depends on the number of the CD-ROMs - Assert.assertTrue(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/mnt/pool/configdrive/i-2-3-VM.iso", "hdd", DiskDef.DiskBus.IDE), vmName)); - Assert.assertTrue(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/mnt/pool/configdrive/i-2-3-VM.iso", "sde", DiskDef.DiskBus.SATA), vmName)); - Assert.assertTrue(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/var/cache/cloud/configdrive/i-2-3-VM.iso", "sdf", DiskDef.DiskBus.SATA), vmName)); - } - - @Test - public void isConfigDriveCdromTestIgnoresOtherCdroms() { - String vmName = "i-2-3-VM"; - Assert.assertFalse(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/mnt/pool/some-ubuntu.iso", "hdc", DiskDef.DiskBus.IDE), vmName)); - Assert.assertFalse(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource("/mnt/pool/configdrive/i-2-30-VM.iso", "hdd", DiskDef.DiskBus.IDE), vmName)); - Assert.assertFalse(libvirtComputingResourceSpy.isConfigDriveCdrom(createCdromWithSource(null, "hdc", DiskDef.DiskBus.IDE), vmName)); - } - - @Test - public void isConfigDriveCdromTestIgnoresDisks() { - DiskDef disk = new DiskDef(); - disk.defFileBasedDisk("/mnt/pool/configdrive/i-2-3-VM.iso", 1, DiskDef.DiskBus.VIRTIO, DiskDef.DiskFmtType.RAW); - - Assert.assertFalse(libvirtComputingResourceSpy.isConfigDriveCdrom(disk, "i-2-3-VM")); - } } 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 a51719a33264..56ad267eac7e 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,25 +597,4 @@ 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/network/element/ConfigDriveNetworkElement.java b/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java index efbce2e38e49..fbc0d106edcf 100644 --- a/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java +++ b/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java @@ -73,7 +73,6 @@ import com.cloud.storage.dao.GuestOSCategoryDao; import com.cloud.storage.dao.GuestOSDao; import com.cloud.storage.dao.VolumeDao; -import com.cloud.template.TemplateManager; import com.cloud.utils.Pair; import com.cloud.utils.StringUtils; import com.cloud.utils.component.AdapterBase; @@ -115,8 +114,6 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle @Inject NetworkModel _networkModel; @Inject - TemplateManager _templateMgr; - @Inject NetworkOrchestrationService _networkOrchestrationService; @Inject GuestOSCategoryDao _guestOSCategoryDao; @@ -145,9 +142,7 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle @Inject private HypervisorGuruManager _hvGuruMgr; - // Device id of the config drive ISO when the Instance has a single ISO CD-ROM (the config drive ISO is the CD-ROM - // following the ones of the ISOs, see TemplateManager#getConfigDriveDeviceId). Used where the Instance is not known. - public static final Integer CONFIGDRIVEDISKSEQ = TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 1; + public static final Integer CONFIGDRIVEDISKSEQ = 4; private boolean canHandle(TrafficType trafficType) { return trafficType.equals(TrafficType.Guest); @@ -596,7 +591,7 @@ private boolean createConfigDriveIsoOnHostCache(NicProfile nic, VirtualMachinePr profile.setConfigDriveLocation(answer.getConfigDriveLocation()); updateConfigDriveLocationInVMDetails(profile.getId(), answer.getConfigDriveLocation()); - addConfigDriveDisk(profile, null, host.getId()); + addConfigDriveDisk(profile, null); return true; } @@ -681,7 +676,7 @@ public boolean createConfigDriveIso(NicProfile nic, VirtualMachineProfile profil } profile.setConfigDriveLocation(answer.getConfigDriveLocation()); updateConfigDriveLocationInVMDetails(profile.getId(), answer.getConfigDriveLocation()); - addConfigDriveDisk(profile, dataStore, dest != null && dest.getHost() != null ? dest.getHost().getId() : null); + addConfigDriveDisk(profile, dataStore); return true; } @@ -781,7 +776,7 @@ private boolean deleteConfigDriveIso(final VirtualMachine vm) throws ResourceUna return true; } - private void addConfigDriveDisk(final VirtualMachineProfile profile, final DataStore dataStore, final Long hostId) throws ResourceUnavailableException { + private void addConfigDriveDisk(final VirtualMachineProfile profile, final DataStore dataStore) throws ResourceUnavailableException { boolean isoAvailable = false; final String isoPath = ConfigDrive.createConfigDrivePath(profile.getInstanceName()); for (DiskTO dataTo : profile.getDisks()) { @@ -803,7 +798,7 @@ private void addConfigDriveDisk(final VirtualMachineProfile profile, final DataS dataTO.setPath(isoPath); dataTO.setFormat(Storage.ImageFormat.ISO); - profile.addDisk(new DiskTO(dataTO, (long) _templateMgr.getConfigDriveDeviceId(profile.getVirtualMachine(), hostId), isoPath, Volume.Type.ISO)); + profile.addDisk(new DiskTO(dataTO, CONFIGDRIVEDISKSEQ.longValue(), isoPath, Volume.Type.ISO)); } else { logger.warn("Config drive ISO already is in Instance profile."); } diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 0c0f7af51e36..9cd397b742e4 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -165,6 +165,10 @@ 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; import com.cloud.org.Grouping; @@ -224,6 +228,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.UserVmManager; import com.cloud.vm.UserVmService; import com.cloud.vm.UserVmVO; @@ -245,6 +250,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.VMInstanceDao; import com.cloud.vm.dao.VMInstanceDetailsDao; @@ -297,6 +303,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; @@ -5358,19 +5368,42 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype } /** - * The number of CD-ROM devices of the Instance (including the one of the config drive ISO, if any). Each CD-ROM - * device occupies a device id, starting from {@link TemplateManager#CDROM_PRIMARY_DEVICE_SEQ}. + * 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 _tmpltMgr.getVmCdromCount(vm); } + /** + * 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 getReservedDeviceIds(VirtualMachine vm) { Set 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; } diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 84183c67b3bc..da19381525ab 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -155,9 +155,6 @@ 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.projects.Project; import com.cloud.projects.ProjectManager; import com.cloud.resourcelimit.CheckedReservation; @@ -222,7 +219,6 @@ 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; @@ -232,11 +228,9 @@ 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; @@ -271,12 +265,6 @@ 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; @@ -731,52 +719,14 @@ public int getVmCdromCount(VirtualMachine vm) { if (vm.getType() != VirtualMachine.Type.User) { return 1; } - Long hostId = hostIdForVm(vm); // 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(hostId); - int isoSlots = getIsoSlotCount(vm, hostId); - // the config drive ISO is attached as the CD-ROM following the ones of the ISOs (it is not counted in vm.iso.max.count) - return Math.max(cdromCount, isoSlots + (isConfigDriveUsed(vm) ? 1 : 0)); - } - - @Override - public int getConfigDriveDeviceId(VirtualMachine vm, Long hostId) { - // the config drive ISO is attached as the CD-ROM following the ones of the ISOs - return CDROM_PRIMARY_DEVICE_SEQ + getIsoSlotCount(vm, hostId); - } - - /** - * The number of CD-ROM devices of the Instance which are used for ISOs, i.e. the ones allocated at boot. - */ - private int getIsoSlotCount(VirtualMachine vm, Long hostId) { + int cdromCount = advertisedCdromCap(hostIdForVm(vm)); UserVmVO userVm = _userVmDao.findById(vm.getId()); - int slotsNeeded = userVm != null ? slotsNeededFor(loadAttachedIsoSlots(userVm)) : 0; - return Math.max(effectiveMaxCdroms(vm, hostId), slotsNeeded); - } - - /** - * 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; + if (userVm != null) { + cdromCount = Math.max(cdromCount, slotsNeededFor(loadAttachedIsoSlots(userVm))); } - 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; + return cdromCount; } private Long singleStoragePoolId(DeployDestination dest) { @@ -1637,9 +1587,6 @@ void enforceCdromAttachLimits(long vmId, UserVm vm, long isoId) { } } - /** - * The maximum number of ISOs which can be attached to the Instance. The config drive ISO is not counted. - */ int effectiveMaxCdroms(VirtualMachine vm, Long hostId) { HostVO host = hostId != null ? _hostDao.findById(hostId) : null; Long clusterId = host != null ? host.getClusterId() : null; diff --git a/server/src/test/java/com/cloud/network/element/ConfigDriveNetworkElementTest.java b/server/src/test/java/com/cloud/network/element/ConfigDriveNetworkElementTest.java index e5c2e8ec67e2..336f9990c12b 100644 --- a/server/src/test/java/com/cloud/network/element/ConfigDriveNetworkElementTest.java +++ b/server/src/test/java/com/cloud/network/element/ConfigDriveNetworkElementTest.java @@ -49,7 +49,6 @@ import com.cloud.utils.fsm.StateListener; import com.cloud.utils.fsm.StateMachine2; import com.cloud.utils.net.Ip; -import com.cloud.template.TemplateManager; import com.cloud.vm.Nic; import com.cloud.vm.NicProfile; import com.cloud.vm.VMInstanceDetailVO; @@ -93,7 +92,6 @@ import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.is; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -130,7 +128,6 @@ public class ConfigDriveNetworkElementTest { @Mock private HostDao _hostDao; @Mock private ServiceOfferingDao _serviceOfferingDao; @Mock private UserVmDao _vmDao; - @Mock private TemplateManager _templateMgr; @Mock private VMInstanceDao _vmInstanceDao; @Mock private VMInstanceDetailsDao _vmInstanceDetailsDao; @Mock private NetworkDao _networkDao; @@ -309,7 +306,6 @@ public void testCreateConfigDriveIso() throws Exception { when(answer.getResult()).thenReturn(true); when(answer.getConfigDriveLocation()).thenReturn(NetworkElement.Location.PRIMARY); when(virtualMachine.getUuid()).thenReturn("vm-uuid"); - when(_templateMgr.getConfigDriveDeviceId(Mockito.any(), Mockito.any())).thenReturn(5); Map parms = Maps.newHashMap(); parms.put(VirtualMachineProfile.Param.VmPassword, PASSWORD); @@ -319,10 +315,6 @@ public void testCreateConfigDriveIso() throws Exception { profile.setVmData(Collections.emptyList()); assertTrue(_configDrivesNetworkElement.createConfigDriveIso(nicp, profile, deployDestination, null)); - // the device id of the config drive ISO is the one calculated from the number of the CD-ROMs of the Instance - assertEquals(1, profile.getDisks().size()); - assertEquals(Long.valueOf(5L), profile.getDisks().get(0).getDiskSeq()); - ArgumentCaptor commandCaptor = ArgumentCaptor.forClass(HandleConfigDriveIsoCommand.class); verify(agentManager, times(1)).easySend(Mockito.anyLong(), commandCaptor.capture()); HandleConfigDriveIsoCommand createCommand = commandCaptor.getValue(); diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 029160a754a5..a597b8bb3010 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -39,6 +39,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutionException; @@ -110,6 +111,10 @@ import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor.HypervisorType; +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.Grouping; import com.cloud.projects.Project; @@ -129,6 +134,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; @@ -141,12 +147,17 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.fsm.NoTransitionException; import com.cloud.vm.DiskProfile; +import com.cloud.vm.NicVO; import com.cloud.vm.UserVmManager; import com.cloud.vm.UserVmVO; +import com.cloud.vm.VMInstanceDetailVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.State; +import com.cloud.vm.VmDetailConstants; import com.cloud.vm.VirtualMachineManager; +import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.dao.VMInstanceDao; import com.cloud.vm.snapshot.VMSnapshot; import com.cloud.vm.snapshot.VMSnapshotDetailsVO; @@ -165,6 +176,12 @@ public class VolumeApiServiceImplTest { @Mock private VolumeDataStoreDao volumeDataStoreDaoMock; @Mock + private VMInstanceDetailsDao vmInstanceDetailsDaoMock; + @Mock + private NicDao nicDaoMock; + @Mock + private NetworkModel networkModelMock; + @Mock private VolumeDao volumeDaoMock; @Mock private BackupDao backupDaoMock; @@ -2908,8 +2925,12 @@ private Long invokeGetDeviceId(UserVmVO vm, Long deviceId) throws Exception { } } - private void mockVmCdromCount(UserVmVO vm, int cdromCount) { - lenient().doReturn(cdromCount).when(volumeApiServiceImpl).getVmCdromCount(vm); + private void mockVmNetworkProvidesConfigDrive(UserVmVO vm, boolean providesConfigDrive) { + NicVO nic = Mockito.mock(NicVO.class); + lenient().when(nic.getNetworkId()).thenReturn(100L); + lenient().when(nicDaoMock.listByVmId(vm.getId())).thenReturn(Collections.singletonList(nic)); + lenient().when(networkModelMock.isProviderSupportServiceInNetwork(eq(100L), eq(Service.UserData), eq(Provider.ConfigDrive))).thenReturn(providesConfigDrive); + lenient().doReturn(1).when(volumeApiServiceImpl).getVmCdromCount(vm); } private List mockVmVolumes(UserVmVO vm, long... deviceIds) { @@ -2924,24 +2945,52 @@ private List mockVmVolumes(UserVmVO vm, long... deviceIds) { } @Test - public void getReservedDeviceIdsReservesOneDeviceIdPerCdrom() { + public void getReservedDeviceIdsReturnsOnlyCdromWhenVmDoesNotUseConfigDrive() { + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, false); + + Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); + + Assert.assertEquals(Collections.singleton((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ), reserved); + } + + @Test + public void getReservedDeviceIdsIncludesConfigDriveWhenNetworkProvidesItBeforeFirstStart() { + // the config drive location detail is only added when the ISO is created, i.e. at the first start + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, true); + + Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); + + Assert.assertEquals(new HashSet<>(Arrays.asList((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ, ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue())), reserved); + } + + @Test + public void getReservedDeviceIdsIncludesConfigDriveWhenConfigDriveLocationDetailExists() { UserVmVO vm = getMockedVm(); + when(vmInstanceDetailsDaoMock.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION)).thenReturn(mock(VMInstanceDetailVO.class)); + doReturn(1).when(volumeApiServiceImpl).getVmCdromCount(vm); + + Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); - mockVmCdromCount(vm, 1); - Assert.assertEquals(new HashSet<>(Arrays.asList(3L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); + Assert.assertTrue(reserved.contains(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue())); + } + + @Test + public void getReservedDeviceIdsReservesOneDeviceIdPerCdrom() { + UserVmVO vm = getMockedVm(); + mockVmNetworkProvidesConfigDrive(vm, false); + doReturn(3).when(volumeApiServiceImpl).getVmCdromCount(vm); - // e.g. the CD-ROM and the config drive ISO, which is attached as the CD-ROM following the ones of the ISOs - mockVmCdromCount(vm, 2); - Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); + Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); - mockVmCdromCount(vm, 3); - Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)), reserved); } @Test - public void getDeviceIdUsesDeviceId4WhenVmHasOneCdrom() throws Exception { + public void getDeviceIdUsesDeviceId4WhenVmDoesNotUseConfigDrive() throws Exception { UserVmVO vm = getMockedVm(); - mockVmCdromCount(vm, 1); + mockVmNetworkProvidesConfigDrive(vm, false); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, null)); @@ -2949,9 +2998,9 @@ public void getDeviceIdUsesDeviceId4WhenVmHasOneCdrom() throws Exception { } @Test - public void getDeviceIdSkipsDeviceIdsOfAllCdroms() throws Exception { + public void getDeviceIdSkipsConfigDriveDeviceIdWhenVmUsesConfigDrive() throws Exception { UserVmVO vm = getMockedVm(); - mockVmCdromCount(vm, 2); + mockVmNetworkProvidesConfigDrive(vm, true); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(5L), invokeGetDeviceId(vm, null)); @@ -2960,7 +3009,7 @@ public void getDeviceIdSkipsDeviceIdsOfAllCdroms() throws Exception { @Test public void getDeviceIdRejectsReservedDeviceIds() throws Exception { UserVmVO vm = getMockedVm(); - mockVmCdromCount(vm, 2); + mockVmNetworkProvidesConfigDrive(vm, true); mockVmVolumes(vm, 0L); for (long reservedDeviceId : new long[] {3L, 4L}) { @@ -2974,9 +3023,9 @@ public void getDeviceIdRejectsReservedDeviceIds() throws Exception { } @Test - public void getDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved() throws Exception { + public void getDeviceIdKeepsMaxDataVolumesWhenVmUsesConfigDrive() throws Exception { UserVmVO vm = getMockedVm(); - mockVmCdromCount(vm, 2); + mockVmNetworkProvidesConfigDrive(vm, true); List volumes = mockVmVolumes(vm, 0L); // 6 data volumes are supported by default diff --git a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java index 3d4e76188e30..a75939d2bbc6 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java @@ -82,9 +82,6 @@ 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; @@ -123,17 +120,12 @@ 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.VMInstanceDetailVO; import com.cloud.vm.UserVmVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.State; import com.cloud.vm.VmIsoMapVO; -import com.cloud.vm.VmDetailConstants; -import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; -import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.dao.VMInstanceDao; import com.cloud.vm.dao.VmIsoMapDao; @@ -257,12 +249,6 @@ public class TemplateManagerImplTest extends TestCase { @Mock HostDetailsDao _hostDetailsDao; - @Mock - NicDao _nicDao; - @Mock - NetworkModel _networkModel; - @Mock - VMInstanceDetailsDao _vmInstanceDetailsDao; @Mock UserVmJoinDao _userVmJoinDao; @@ -1373,94 +1359,4 @@ public void getVmCdromCountCoversAttachedIsosBeyondHostCap() { Assert.assertEquals(3, templateManager.getVmCdromCount(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 effectiveMaxCdromsDoesNotCountConfigDrive() { - UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); - Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); - mockVmNetworkProvidesConfigDrive(vm, true); - - // the config drive ISO is attached as an additional CD-ROM, it does not take one of the ISOs - withConfiguredMaxCdroms("2", () -> Assert.assertEquals(2, templateManager.effectiveMaxCdroms(vm, 7L))); - } - - @Test - public void getVmCdromCountIncludesConfigDriveCdrom() { - UserVmVO vm = mockUserVmOnHostWithCdromCap(null); - mockVmNetworkProvidesConfigDrive(vm, true); - - // the host does not advertise the number of CD-ROMs: one CD-ROM for ISOs, and one for the config drive ISO - Assert.assertEquals(2, templateManager.getVmCdromCount(vm)); - } - - @Test - public void getVmCdromCountAddsTheCdromOfConfigDriveToTheOnesOfIsos() { - UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); - Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); - mockVmNetworkProvidesConfigDrive(vm, true); - - // 2 CD-ROMs for ISOs (vm.iso.max.count), and 1 for the config drive ISO - withConfiguredMaxCdroms("2", () -> Assert.assertEquals(3, templateManager.getVmCdromCount(vm))); - } - - @Test - public void getConfigDriveDeviceIdIsTheCdromFollowingTheOnesOfIsos() { - UserVmVO vm = mockUserVmOnHostWithCdromCap(3L); - Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); - - // vm.iso.max.count is 1 by default - Assert.assertEquals(TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 1, templateManager.getConfigDriveDeviceId(vm, 7L)); - withConfiguredMaxCdroms("3", () -> Assert.assertEquals(TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 3, templateManager.getConfigDriveDeviceId(vm, 7L))); - } - - @Test - public void getConfigDriveDeviceIdFollowsAttachedIsosBeyondConfiguredMaximum() { - UserVmVO vm = mockUserVmOnHostWithCdromCap(2L); - Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class)); - Mockito.when(_vmIsoMapDao.listByVmId(1L)).thenReturn(Arrays.asList( - new VmIsoMapVO(1L, 11L, TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 1))); - - Assert.assertEquals(TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 2, templateManager.getConfigDriveDeviceId(vm, 7L)); - } } From 62bde9658974846eff494d9c8dff4dd040afb86e Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Thu, 15 Oct 2026 16:45:30 +0200 Subject: [PATCH 5/6] server,kvm: Skip the device id of the config drive ISO for the ISOs The config drive ISO keeps the device id 4, whatever the number of ISOs of the Instance. When an Instance uses config drive, the CD-ROMs of the ISOs which would use that device id or the next ones are moved to the next device ids, i.e. the CD-ROMs are, by device id: ISO 1 (3), config drive (4), ISO 2 (5), ISO 3 (6), ... The ISO slots stored for the Instance (user_vm.iso_id and vm_iso_map) are not changed. They are converted to device ids when the CD-ROMs are defined for the Instance, and when an ISO is attached or detached, so Instances which already have an ISO in the slot following the first one do not need to be updated. The number of CD-ROM devices of an Instance (TemplateManager.getVmCdromCount) includes the one of the config drive ISO, so the device ids reserved for the volumes cover all of them. When the networks of an Instance are not known yet (import, restore from backup), the config drive is always assumed. CloudStack supports 2 CD-ROMs per Instance on the IDE bus (hdc and hdd, the first two slots are kept for the disks). The KVM agent defines the CD-ROMs beyond these slots on the SATA bus. --- .../com/cloud/storage/VolumeApiService.java | 6 + .../com/cloud/template/TemplateManager.java | 11 +- .../hypervisor/kvm/resource/LibvirtVMDef.java | 16 ++- .../kvm/resource/LibvirtVMDefTest.java | 21 +++ .../java/com/cloud/hypervisor/KVMGuru.java | 5 +- .../cloud/storage/VolumeApiServiceImpl.java | 46 ++----- .../cloud/template/TemplateManagerImpl.java | 67 +++++++++- .../vm/UnmanagedVMsManagerImpl.java | 5 +- .../storage/VolumeApiServiceImplTest.java | 92 ++++--------- .../template/TemplateManagerImplTest.java | 124 ++++++++++++++++++ .../vm/UnmanagedVMsManagerImplTest.java | 5 +- 11 files changed, 281 insertions(+), 117 deletions(-) diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java index e1cd60952e29..78bc106e00c3 100644 --- a/api/src/main/java/com/cloud/storage/VolumeApiService.java +++ b/api/src/main/java/com/cloud/storage/VolumeApiService.java @@ -199,6 +199,12 @@ Volume updateVolume(long volumeId, String path, String state, Long storageId, */ 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 7943f4c57d2e..7adb66012b75 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 @@ -188,13 +188,20 @@ public interface TemplateManager { * Returns the number of CD-ROM devices of the Instance. The CD-ROM devices occupy the device ids starting from * {@link #CDROM_PRIMARY_DEVICE_SEQ}, which therefore 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. + * 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 number of CD-ROM devices + * @return the number of CD-ROM devices, including the one of the config drive ISO if the Instance uses config drive */ int getVmCdromCount(VirtualMachine vm); + /** + * Same as {@link #getVmCdromCount(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. + */ + int getVmCdromCount(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 e44802200f4b..e40dee25331d 100644 --- a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java +++ b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java @@ -27,7 +27,6 @@ import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao; import com.cloud.hypervisor.kvm.dpdk.DpdkHelper; -import com.cloud.network.element.ConfigDriveNetworkElement; import com.cloud.service.ServiceOfferingVO; import com.cloud.storage.DataStoreRole; import com.cloud.storage.GuestOSHypervisorVO; @@ -57,7 +56,6 @@ 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; @@ -376,8 +374,7 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId, _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 = new HashSet<>(volumeApiService.getReservedDeviceIds(vm)); - reservedDeviceIds.add(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue()); + Set reservedDeviceIds = volumeApiService.getReservedDeviceIds(vm, true); for (Backup.VolumeInfo VMVolToRestore : vm.getBackupVolumeList()) { VolumeVO volume = _volumeDao.findByUuidIncludingRemoved(VMVolToRestore.getUuid()); volume.setState(Volume.State.Ready); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 9cd397b742e4..ef5fb4e9fbb8 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -165,10 +165,6 @@ 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; import com.cloud.org.Grouping; @@ -228,7 +224,6 @@ 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.UserVmManager; import com.cloud.vm.UserVmService; import com.cloud.vm.UserVmVO; @@ -250,7 +245,6 @@ 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.VMInstanceDao; import com.cloud.vm.dao.VMInstanceDetailsDao; @@ -303,10 +297,6 @@ 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; @@ -5368,42 +5358,28 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype } /** - * 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}. + * The number of CD-ROM devices of the Instance (including the one of the config drive ISO, if any). Each CD-ROM + * device occupies a device id, starting from {@link TemplateManager#CDROM_PRIMARY_DEVICE_SEQ}. */ protected int getVmCdromCount(VirtualMachine vm) { return _tmpltMgr.getVmCdromCount(vm); } - /** - * 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 getReservedDeviceIds(VirtualMachine vm) { + return getReservedDeviceIds(vm, getVmCdromCount(vm)); } @Override - public Set getReservedDeviceIds(VirtualMachine vm) { + public Set getReservedDeviceIds(VirtualMachine vm, boolean configDriveUsed) { + return getReservedDeviceIds(vm, _tmpltMgr.getVmCdromCount(vm, configDriveUsed)); + } + + private Set getReservedDeviceIds(VirtualMachine vm, int cdromCount) { Set reservedDeviceIds = new HashSet<>(); - for (int i = 0; i < getVmCdromCount(vm); i++) { + for (int i = 0; i < cdromCount; i++) { reservedDeviceIds.add((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + i); } - if (isConfigDriveUsed(vm)) { - reservedDeviceIds.add(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue()); - } return reservedDeviceIds; } diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index da19381525ab..68f1d4e23775 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -155,6 +155,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 +223,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 +233,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 +272,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,9 +718,11 @@ 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)); @@ -716,6 +731,14 @@ public void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestinat @Override public int getVmCdromCount(VirtualMachine vm) { + if (vm.getType() != VirtualMachine.Type.User) { + return 1; + } + return getVmCdromCount(vm, isConfigDriveUsed(vm)); + } + + @Override + public int getVmCdromCount(VirtualMachine vm, boolean configDriveUsed) { if (vm.getType() != VirtualMachine.Type.User) { return 1; } @@ -726,7 +749,42 @@ public int getVmCdromCount(VirtualMachine vm) { if (userVm != null) { cdromCount = Math.max(cdromCount, slotsNeededFor(loadAttachedIsoSlots(userVm))); } - return cdromCount; + // 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) { @@ -1466,7 +1524,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 a884361eaa79..6a126f512e37 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -80,7 +80,6 @@ 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; @@ -904,9 +903,7 @@ private Pair importDisk(UnmanagedInstanceTO.Disk disk, * 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) { - Set reservedDeviceIds = new HashSet<>(volumeApiService.getReservedDeviceIds(vm)); - reservedDeviceIds.add(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue()); - return reservedDeviceIds; + return volumeApiService.getReservedDeviceIds(vm, true); } private long getNextDeviceId(long deviceId, Set reservedDeviceIds) { diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index a597b8bb3010..184e7883c156 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -39,7 +39,6 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutionException; @@ -111,10 +110,6 @@ import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor.HypervisorType; -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.Grouping; import com.cloud.projects.Project; @@ -147,17 +142,12 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.fsm.NoTransitionException; import com.cloud.vm.DiskProfile; -import com.cloud.vm.NicVO; import com.cloud.vm.UserVmManager; import com.cloud.vm.UserVmVO; -import com.cloud.vm.VMInstanceDetailVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.State; -import com.cloud.vm.VmDetailConstants; import com.cloud.vm.VirtualMachineManager; -import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; -import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.dao.VMInstanceDao; import com.cloud.vm.snapshot.VMSnapshot; import com.cloud.vm.snapshot.VMSnapshotDetailsVO; @@ -176,11 +166,7 @@ public class VolumeApiServiceImplTest { @Mock private VolumeDataStoreDao volumeDataStoreDaoMock; @Mock - private VMInstanceDetailsDao vmInstanceDetailsDaoMock; - @Mock - private NicDao nicDaoMock; - @Mock - private NetworkModel networkModelMock; + private TemplateManager templateManagerMock; @Mock private VolumeDao volumeDaoMock; @Mock @@ -2925,12 +2911,8 @@ private Long invokeGetDeviceId(UserVmVO vm, Long deviceId) throws Exception { } } - private void mockVmNetworkProvidesConfigDrive(UserVmVO vm, boolean providesConfigDrive) { - NicVO nic = Mockito.mock(NicVO.class); - lenient().when(nic.getNetworkId()).thenReturn(100L); - lenient().when(nicDaoMock.listByVmId(vm.getId())).thenReturn(Collections.singletonList(nic)); - lenient().when(networkModelMock.isProviderSupportServiceInNetwork(eq(100L), eq(Service.UserData), eq(Provider.ConfigDrive))).thenReturn(providesConfigDrive); - lenient().doReturn(1).when(volumeApiServiceImpl).getVmCdromCount(vm); + private void mockVmCdromCount(UserVmVO vm, int cdromCount) { + lenient().doReturn(cdromCount).when(volumeApiServiceImpl).getVmCdromCount(vm); } private List mockVmVolumes(UserVmVO vm, long... deviceIds) { @@ -2944,53 +2926,25 @@ private List mockVmVolumes(UserVmVO vm, long... deviceIds) { return volumes; } - @Test - public void getReservedDeviceIdsReturnsOnlyCdromWhenVmDoesNotUseConfigDrive() { - UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, false); - - Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); - - Assert.assertEquals(Collections.singleton((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ), reserved); - } - - @Test - public void getReservedDeviceIdsIncludesConfigDriveWhenNetworkProvidesItBeforeFirstStart() { - // the config drive location detail is only added when the ISO is created, i.e. at the first start - UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); - - Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); - - Assert.assertEquals(new HashSet<>(Arrays.asList((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ, ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue())), reserved); - } - - @Test - public void getReservedDeviceIdsIncludesConfigDriveWhenConfigDriveLocationDetailExists() { - UserVmVO vm = getMockedVm(); - when(vmInstanceDetailsDaoMock.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION)).thenReturn(mock(VMInstanceDetailVO.class)); - doReturn(1).when(volumeApiServiceImpl).getVmCdromCount(vm); - - Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); - - Assert.assertTrue(reserved.contains(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue())); - } - @Test public void getReservedDeviceIdsReservesOneDeviceIdPerCdrom() { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, false); - doReturn(3).when(volumeApiServiceImpl).getVmCdromCount(vm); - Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); + mockVmCdromCount(vm, 1); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); - Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)), reserved); + // e.g. the CD-ROM and the config drive ISO, which is attached as the CD-ROM following the ones of the ISOs + mockVmCdromCount(vm, 2); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); + + mockVmCdromCount(vm, 3); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); } @Test - public void getDeviceIdUsesDeviceId4WhenVmDoesNotUseConfigDrive() throws Exception { + public void getDeviceIdUsesDeviceId4WhenVmHasOneCdrom() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, false); + mockVmCdromCount(vm, 1); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, null)); @@ -2998,9 +2952,9 @@ public void getDeviceIdUsesDeviceId4WhenVmDoesNotUseConfigDrive() throws Excepti } @Test - public void getDeviceIdSkipsConfigDriveDeviceIdWhenVmUsesConfigDrive() throws Exception { + public void getDeviceIdSkipsDeviceIdsOfAllCdroms() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); + mockVmCdromCount(vm, 2); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(5L), invokeGetDeviceId(vm, null)); @@ -3009,7 +2963,7 @@ public void getDeviceIdSkipsConfigDriveDeviceIdWhenVmUsesConfigDrive() throws Ex @Test public void getDeviceIdRejectsReservedDeviceIds() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); + mockVmCdromCount(vm, 2); mockVmVolumes(vm, 0L); for (long reservedDeviceId : new long[] {3L, 4L}) { @@ -3023,9 +2977,9 @@ public void getDeviceIdRejectsReservedDeviceIds() throws Exception { } @Test - public void getDeviceIdKeepsMaxDataVolumesWhenVmUsesConfigDrive() throws Exception { + public void getDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); + mockVmCdromCount(vm, 2); List volumes = mockVmVolumes(vm, 0L); // 6 data volumes are supported by default @@ -3046,4 +3000,14 @@ public void getDeviceIdKeepsMaxDataVolumesWhenVmUsesConfigDrive() throws Excepti Assert.assertTrue(e.getMessage(), e.getMessage().contains("All device Ids are used")); } } + + @Test + public void getReservedDeviceIdsWithConfigDriveReservesTheCdromOfConfigDrive() { + UserVmVO vm = getMockedVm(); + when(templateManagerMock.getVmCdromCount(vm, false)).thenReturn(2); + when(templateManagerMock.getVmCdromCount(vm, true)).thenReturn(3); + + 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 a75939d2bbc6..88cfa2a84859 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; @@ -1359,4 +1376,111 @@ public void getVmCdromCountCoversAttachedIsosBeyondHostCap() { Assert.assertEquals(3, templateManager.getVmCdromCount(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 getVmCdromCountAddsTheCdromOfConfigDrive() { + 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(3, templateManager.getVmCdromCount(vm)); + Assert.assertEquals(2, templateManager.getVmCdromCount(vm, false)); + Assert.assertEquals(3, templateManager.getVmCdromCount(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 29448e961250..65d0fd0e4d88 100644 --- a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java @@ -2030,9 +2030,10 @@ public void testAutoFillAcceptsBareAddressWithoutPrefix() { } @Test - public void getReservedDeviceIdsAlwaysIncludesConfigDriveDeviceId() { + public void getReservedDeviceIdsAlwaysReservesTheCdromOfConfigDrive() { VirtualMachine vm = Mockito.mock(VirtualMachine.class); - Mockito.when(volumeApiService.getReservedDeviceIds(vm)).thenReturn(Collections.singleton(3L)); + // 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); From de91dd4417a04516884a21af7bfbe4b44aa4177c Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Thu, 15 Oct 2026 18:14:04 +0200 Subject: [PATCH 6/6] server: Return the device ids of the CD-ROMs instead of their number TemplateManager exposed the number of CD-ROMs of an Instance, which the only caller (VolumeApiServiceImpl) converted back to the device ids starting from the primary CD-ROM device id. Return the device ids directly (TemplateManager.getCdromDeviceIds), so the callers do not have to assume that they are consecutive, and the number of CD-ROMs becomes an internal detail of TemplateManagerImpl. --- .../com/cloud/template/TemplateManager.java | 13 +++++---- .../cloud/storage/VolumeApiServiceImpl.java | 20 ++----------- .../cloud/template/TemplateManagerImpl.java | 22 ++++++++++++--- .../storage/VolumeApiServiceImplTest.java | 24 ++++++++-------- .../template/TemplateManagerImplTest.java | 28 +++++++++++-------- 5 files changed, 55 insertions(+), 52 deletions(-) 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 7adb66012b75..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; @@ -185,22 +186,22 @@ public interface TemplateManager { void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestination dest); /** - * Returns the number of CD-ROM devices of the Instance. The CD-ROM devices occupy the device ids starting from - * {@link #CDROM_PRIMARY_DEVICE_SEQ}, which therefore cannot be used by the volumes of the Instance. + * 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 number of CD-ROM devices, including the one of the config drive ISO if the Instance uses config drive + * @return the device ids of the CD-ROM devices, including the one of the config drive ISO if the Instance uses config drive */ - int getVmCdromCount(VirtualMachine vm); + Set getCdromDeviceIds(VirtualMachine vm); /** - * Same as {@link #getVmCdromCount(VirtualMachine)}, with the specified usage of config drive instead of the one of + * 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. */ - int getVmCdromCount(VirtualMachine vm, boolean configDriveUsed); + 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/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index ef5fb4e9fbb8..88795e7c036a 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -5357,30 +5357,14 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion : minHwVersion; } - /** - * The number of CD-ROM devices of the Instance (including the one of the config drive ISO, if any). Each CD-ROM - * device occupies a device id, starting from {@link TemplateManager#CDROM_PRIMARY_DEVICE_SEQ}. - */ - protected int getVmCdromCount(VirtualMachine vm) { - return _tmpltMgr.getVmCdromCount(vm); - } - @Override public Set getReservedDeviceIds(VirtualMachine vm) { - return getReservedDeviceIds(vm, getVmCdromCount(vm)); + return _tmpltMgr.getCdromDeviceIds(vm); } @Override public Set getReservedDeviceIds(VirtualMachine vm, boolean configDriveUsed) { - return getReservedDeviceIds(vm, _tmpltMgr.getVmCdromCount(vm, configDriveUsed)); - } - - private Set getReservedDeviceIds(VirtualMachine vm, int cdromCount) { - Set reservedDeviceIds = new HashSet<>(); - for (int i = 0; i < cdromCount; i++) { - reservedDeviceIds.add((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + i); - } - return reservedDeviceIds; + return _tmpltMgr.getCdromDeviceIds(vm, configDriveUsed); } /** diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 68f1d4e23775..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; @@ -730,15 +732,27 @@ public void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestinat } @Override - public int getVmCdromCount(VirtualMachine vm) { + public Set getCdromDeviceIds(VirtualMachine vm) { if (vm.getType() != VirtualMachine.Type.User) { - return 1; + return getCdromDeviceIds(1); } - return getVmCdromCount(vm, isConfigDriveUsed(vm)); + return getCdromDeviceIds(vm, isConfigDriveUsed(vm)); } @Override - public int getVmCdromCount(VirtualMachine vm, boolean configDriveUsed) { + 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; } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 184e7883c156..334bb3f659fd 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -2911,8 +2911,8 @@ private Long invokeGetDeviceId(UserVmVO vm, Long deviceId) throws Exception { } } - private void mockVmCdromCount(UserVmVO vm, int cdromCount) { - lenient().doReturn(cdromCount).when(volumeApiServiceImpl).getVmCdromCount(vm); + 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) { @@ -2927,24 +2927,24 @@ private List mockVmVolumes(UserVmVO vm, long... deviceIds) { } @Test - public void getReservedDeviceIdsReservesOneDeviceIdPerCdrom() { + public void getReservedDeviceIdsReturnsTheDeviceIdsOfTheCdroms() { UserVmVO vm = getMockedVm(); - mockVmCdromCount(vm, 1); + 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 - mockVmCdromCount(vm, 2); + mockCdromDeviceIds(vm, 3L, 4L); Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); - mockVmCdromCount(vm, 3); + 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(); - mockVmCdromCount(vm, 1); + mockCdromDeviceIds(vm, 3L); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, null)); @@ -2954,7 +2954,7 @@ public void getDeviceIdUsesDeviceId4WhenVmHasOneCdrom() throws Exception { @Test public void getDeviceIdSkipsDeviceIdsOfAllCdroms() throws Exception { UserVmVO vm = getMockedVm(); - mockVmCdromCount(vm, 2); + mockCdromDeviceIds(vm, 3L, 4L); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(5L), invokeGetDeviceId(vm, null)); @@ -2963,7 +2963,7 @@ public void getDeviceIdSkipsDeviceIdsOfAllCdroms() throws Exception { @Test public void getDeviceIdRejectsReservedDeviceIds() throws Exception { UserVmVO vm = getMockedVm(); - mockVmCdromCount(vm, 2); + mockCdromDeviceIds(vm, 3L, 4L); mockVmVolumes(vm, 0L); for (long reservedDeviceId : new long[] {3L, 4L}) { @@ -2979,7 +2979,7 @@ public void getDeviceIdRejectsReservedDeviceIds() throws Exception { @Test public void getDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved() throws Exception { UserVmVO vm = getMockedVm(); - mockVmCdromCount(vm, 2); + mockCdromDeviceIds(vm, 3L, 4L); List volumes = mockVmVolumes(vm, 0L); // 6 data volumes are supported by default @@ -3004,8 +3004,8 @@ public void getDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved() throws Exce @Test public void getReservedDeviceIdsWithConfigDriveReservesTheCdromOfConfigDrive() { UserVmVO vm = getMockedVm(); - when(templateManagerMock.getVmCdromCount(vm, false)).thenReturn(2); - when(templateManagerMock.getVmCdromCount(vm, true)).thenReturn(3); + 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 88cfa2a84859..61407a9d4c2b 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java @@ -1330,6 +1330,10 @@ 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); @@ -1345,36 +1349,36 @@ private UserVmVO mockUserVmOnHostWithCdromCap(Long advertisedCap) { } @Test - public void getVmCdromCountReturnsOneForSystemVms() { + public void getCdromDeviceIdsReturnsOnlyThePrimaryCdromForSystemVms() { VirtualMachine vm = Mockito.mock(VirtualMachine.class); Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.SecondaryStorageVm); - Assert.assertEquals(1, templateManager.getVmCdromCount(vm)); + Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm)); } @Test - public void getVmCdromCountReturnsAllCdromsAdvertisedByHostRegardlessOfConfiguredMaximum() { + 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(2, templateManager.getVmCdromCount(vm)); + Assert.assertEquals(ids(3L, 4L), templateManager.getCdromDeviceIds(vm)); } @Test - public void getVmCdromCountReturnsDefaultWhenHostDoesNotAdvertiseCdromCap() { + public void getCdromDeviceIdsReturnsDefaultWhenHostDoesNotAdvertiseCdromCap() { UserVmVO vm = mockUserVmOnHostWithCdromCap(null); - Assert.assertEquals(TemplateManager.DEFAULT_CDROM_MAX_PER_VM, templateManager.getVmCdromCount(vm)); + Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm)); } @Test - public void getVmCdromCountCoversAttachedIsosBeyondHostCap() { + 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(3, templateManager.getVmCdromCount(vm)); + Assert.assertEquals(ids(3L, 4L, 5L), templateManager.getCdromDeviceIds(vm)); } private void mockVmNetworkProvidesConfigDrive(UserVmVO vm, boolean providesConfigDrive) { @@ -1419,14 +1423,14 @@ public void isConfigDriveUsedReturnsFalseWhenNoNetworkProvidesConfigDrive() { } @Test - public void getVmCdromCountAddsTheCdromOfConfigDrive() { + 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(3, templateManager.getVmCdromCount(vm)); - Assert.assertEquals(2, templateManager.getVmCdromCount(vm, false)); - Assert.assertEquals(3, templateManager.getVmCdromCount(vm, true)); + 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