From c685b43fc14210578ba36ea2ae59decdd03e038c Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Mon, 7 Sep 2026 16:07:00 +0200 Subject: [PATCH 1/4] server: Avoid device id collision between config drive ISO and data volumes on KVM ConfigDriveNetworkElement hardcodes disk sequence 4 (CONFIGDRIVEDISKSEQ) for the config drive ISO, but the volume device-id allocators (VolumeApiServiceImpl.getDeviceId, KVMGuru.getNextAvailableDeviceId) only look at persisted VolumeVO rows and have no knowledge that slot 4 is reserved by the config drive. When a VM using ConfigDrive already has data disks on device ids 1 and 2, attaching a 3rd data disk gets assigned device id 4 as well, producing two disks with the same libvirt target device (e.g. "sde") once both end up on the same bus namespace, as happens with Q35/UEFI machine types. Both allocators now skip CONFIGDRIVEDISKSEQ when the VM has a config drive (detected via the existing CONFIG_DRIVE_LOCATION vm detail), the same way they already permanently reserve device id 3 for the CD-ROM slot. --- .../main/java/com/cloud/hypervisor/KVMGuru.java | 14 +++++++++++--- .../network/element/ConfigDriveNetworkElement.java | 2 +- .../com/cloud/storage/VolumeApiServiceImpl.java | 8 +++++++- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java index afb695d78574..4523c9a244e6 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; @@ -42,7 +43,9 @@ import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineProfile; +import com.cloud.vm.VmDetailConstants; import com.cloud.vm.dao.VMInstanceDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; import org.apache.cloudstack.backup.Backup; import org.apache.cloudstack.storage.command.CopyCommand; import org.apache.cloudstack.storage.command.StorageSubSystemCommand; @@ -71,6 +74,8 @@ public class KVMGuru extends HypervisorGuruBase implements HypervisorGuru { VolumeDao _volumeDao; @Inject HypervisorCapabilitiesDao _hypervisorCapabilitiesDao; + @Inject + VMInstanceDetailsDao _vmInstanceDetailsDao; @Override @@ -86,7 +91,7 @@ protected KVMGuru() { * Get next free DeviceId for a KVM Guest */ - protected Long getNextAvailableDeviceId(List vmVolumes) { + protected Long getNextAvailableDeviceId(long vmId, List vmVolumes) { int maxDataVolumesSupported; int maxDeviceId; @@ -103,6 +108,9 @@ protected Long getNextAvailableDeviceId(List vmVolumes) { devIds.add(String.valueOf(i)); } devIds.remove("3"); + if (_vmInstanceDetailsDao.findDetail(vmId, VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) { + devIds.remove(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.toString()); + } for (VolumeVO vmVolume : vmVolumes) { devIds.remove(vmVolume.getDeviceId().toString().trim()); } @@ -371,7 +379,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(vm.getId(), 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()); @@ -389,7 +397,7 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId, VolumeVO restoredVolume = _volumeDao.findByUuid(location); if (restoredVolume != null) { try { - _volumeDao.attachVolume(restoredVolume.getId(), vm.getId(), getNextAvailableDeviceId(vmVolumes)); + _volumeDao.attachVolume(restoredVolume.getId(), vm.getId(), getNextAvailableDeviceId(vm.getId(), 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..a7782bf965f3 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 final static 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 9b2f4f831d3e..5de82483edae 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -160,6 +160,7 @@ import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.hypervisor.HypervisorCapabilitiesVO; import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao; +import com.cloud.network.element.ConfigDriveNetworkElement; import com.cloud.offering.DiskOffering; import com.cloud.org.Cluster; import com.cloud.org.Grouping; @@ -5042,8 +5043,10 @@ private Long getDeviceId(UserVmVO vm, Long deviceId) { int maxDevices = getMaxDataVolumesSupported(vm) + 2; // add 2 to consider devices root volume and cdrom int maxDeviceId = maxDevices - 1; List vols = _volsDao.findByInstance(vm.getId()); + boolean vmHasConfigDrive = vmInstanceDetailsDao.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION) != null; if (deviceId != null) { - if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || deviceId.longValue() == 3) { + if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || deviceId.longValue() == 3 + || (vmHasConfigDrive && deviceId.longValue() == ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ)) { throw new RuntimeException("deviceId should be 0,1,2,4-" + maxDeviceId); } for (VolumeVO vol : vols) { @@ -5058,6 +5061,9 @@ private Long getDeviceId(UserVmVO vm, Long deviceId) { devIds.add(String.valueOf(i)); } devIds.remove("3"); + if (vmHasConfigDrive) { + devIds.remove(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.toString()); + } for (VolumeVO vol : vols) { devIds.remove(vol.getDeviceId().toString().trim()); } From 87da208d97fe85477d758a3af48e9c36c7a7806b Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Tue, 6 Oct 2026 13:56:50 +0200 Subject: [PATCH 2/4] Change CONFIGDRIVEDISKSEQ to static final Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../com/cloud/network/element/ConfigDriveNetworkElement.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a7782bf965f3..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; - public final static Integer CONFIGDRIVEDISKSEQ = 4; + public static final Integer CONFIGDRIVEDISKSEQ = 4; private boolean canHandle(TrafficType trafficType) { return trafficType.equals(TrafficType.Guest); From 594b5063c6cf1996d8c3b7f3f9d11771fa319aea Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Tue, 13 Oct 2026 18:11:37 +0200 Subject: [PATCH 3/4] server: Reserve CD-ROM and config drive device ids for volumes Data volumes could be assigned the same device id as the CD-ROM or the config drive ISO, which end up with the same libvirt target name (e.g. sde) when they are on the same bus namespace (SATA with UEFI/Q35). Add VolumeApiService.getReservedDeviceIds(), which returns the device ids of the CD-ROM devices and of the config drive ISO when the Instance uses config drive. The config drive is detected from the network providers (Dhcp, Dns, UserData), so Instances which have never been started are covered as well as the ones with the config drive location detail. The reserved ids are skipped when allocating device ids in: - attach volume - restore of volumes and Instances from backup - import of Instances (unmanaged Instances and VMware to KVM) The max number of data volumes is preserved when device ids are reserved, and the error message of an invalid deviceId lists the reserved ids. --- .../com/cloud/storage/VolumeApiService.java | 12 ++ .../com/cloud/template/TemplateManager.java | 3 + .../java/com/cloud/hypervisor/KVMGuru.java | 36 ++--- .../cloud/storage/VolumeApiServiceImpl.java | 88 ++++++++-- .../vm/UnmanagedVMsManagerImpl.java | 28 +++- .../com/cloud/hypervisor/KVMGuruTest.java | 46 ++++++ .../storage/VolumeApiServiceImplTest.java | 150 ++++++++++++++++++ .../vm/UnmanagedVMsManagerImplTest.java | 23 +++ 8 files changed, 347 insertions(+), 39 deletions(-) diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java index 19c2ebe455a5..9e272398535e 100644 --- a/api/src/main/java/com/cloud/storage/VolumeApiService.java +++ b/api/src/main/java/com/cloud/storage/VolumeApiService.java @@ -21,12 +21,14 @@ import java.net.MalformedURLException; import java.util.List; import java.util.Map; +import java.util.Set; import com.cloud.exception.ResourceAllocationException; import com.cloud.offering.DiskOffering; import com.cloud.user.Account; import com.cloud.utils.Pair; import com.cloud.utils.fsm.NoTransitionException; +import com.cloud.vm.VirtualMachine; import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd; import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd; @@ -180,6 +182,16 @@ Volume updateVolume(long volumeId, String path, String state, Long storageId, */ boolean doesStoragePoolSupportDiskOfferingTags(StoragePool destPool, String diskOfferingTags); + /** + * Returns the device ids of the Instance which are used by non-volume devices (the CD-ROM drives and the config drive ISO), + * and therefore must not be assigned to its volumes. Otherwise the hypervisor may end up with two devices using the same + * target name (e.g. sde), which is rejected. + * + * @param vm the Instance + * @return the reserved device ids + */ + Set getReservedDeviceIds(VirtualMachine vm); + boolean validateConditionsToReplaceDiskOfferingOfVolume(Volume volume, DiskOffering newDiskOffering, StoragePool destPool); Volume destroyVolume(long volumeId, Account caller, boolean expunge, boolean forceExpunge); 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 f1891c774edd..d873bec6c273 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 @@ -67,6 +67,9 @@ public interface TemplateManager { static final String VMWARE_TOOLS_ISO = "vmware-tools.iso"; static final String XS_TOOLS_ISO = "xs-tools.iso"; + // KVM/libvirt maps deviceSeq=3 to hdc (hda/hdb are taken by the root volume on i440fx/IDE). + int CDROM_PRIMARY_DEVICE_SEQ = 3; + /** * Prepares a template for vm creation for a certain storage pool. * diff --git a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java index 4523c9a244e6..d174824accf6 100644 --- a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java +++ b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java @@ -33,6 +33,7 @@ import com.cloud.storage.GuestOSHypervisorVO; import com.cloud.storage.GuestOSVO; import com.cloud.storage.Volume; +import com.cloud.storage.VolumeApiService; import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.GuestOSDao; import com.cloud.storage.dao.GuestOSHypervisorDao; @@ -43,9 +44,7 @@ import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineProfile; -import com.cloud.vm.VmDetailConstants; import com.cloud.vm.dao.VMInstanceDao; -import com.cloud.vm.dao.VMInstanceDetailsDao; import org.apache.cloudstack.backup.Backup; import org.apache.cloudstack.storage.command.CopyCommand; import org.apache.cloudstack.storage.command.StorageSubSystemCommand; @@ -55,8 +54,10 @@ import java.math.BigDecimal; import java.math.RoundingMode; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.utils.bytescale.ByteScaleUtils; import org.apache.commons.lang3.math.NumberUtils; @@ -75,7 +76,7 @@ public class KVMGuru extends HypervisorGuruBase implements HypervisorGuru { @Inject HypervisorCapabilitiesDao _hypervisorCapabilitiesDao; @Inject - VMInstanceDetailsDao _vmInstanceDetailsDao; + VolumeApiService volumeApiService; @Override @@ -91,33 +92,29 @@ protected KVMGuru() { * Get next free DeviceId for a KVM Guest */ - protected Long getNextAvailableDeviceId(long vmId, List 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)); - } - devIds.remove("3"); - if (_vmInstanceDetailsDao.findDetail(vmId, VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) { - devIds.remove(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.toString()); + // the device ids reserved for the CD-ROM and config drive devices cannot be used by volumes + for (long id = 1; devIds.size() < maxDataVolumesSupported; id++) { + if (!reservedDeviceIds.contains(id)) { + devIds.add(id); + } } for (VolumeVO vmVolume : vmVolumes) { - devIds.remove(vmVolume.getDeviceId().toString().trim()); + devIds.remove(vmVolume.getDeviceId()); } if (devIds.isEmpty()) { throw new RuntimeException("All device Ids are in use."); } - return Long.parseLong(devIds.iterator().next()); + return devIds.get(0); } @@ -369,6 +366,9 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId, vm.setPowerState(VirtualMachine.PowerState.PowerOff); _instanceDao.update(vm.getId(), vm); } + // the networks of the (possibly expunged) VM may not be available, so always keep the config drive device id free + Set reservedDeviceIds = new HashSet<>(volumeApiService.getReservedDeviceIds(vm)); + reservedDeviceIds.add(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.longValue()); for (Backup.VolumeInfo VMVolToRestore : vm.getBackupVolumeList()) { VolumeVO volume = _volumeDao.findByUuidIncludingRemoved(VMVolToRestore.getUuid()); volume.setState(Volume.State.Ready); @@ -379,7 +379,7 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId, } else if (VMVolToRestore.getType() == Volume.Type.DATADISK) { List vmVolumes = _volumeDao.findByInstance(vm.getId()); _volumeDao.update(volume.getId(), volume); - _volumeDao.attachVolume(volume.getId(), vm.getId(), getNextAvailableDeviceId(vm.getId(), vmVolumes)); + _volumeDao.attachVolume(volume.getId(), vm.getId(), getNextAvailableDeviceId(reservedDeviceIds, vmVolumes)); } UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_ATTACH, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(), volume.getDiskOfferingId(), volume.getTemplateId(), volume.getSize(), Volume.class.getName(), volume.getUuid(), vm.getId(), volume.isDisplay()); @@ -397,7 +397,7 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId, VolumeVO restoredVolume = _volumeDao.findByUuid(location); if (restoredVolume != null) { try { - _volumeDao.attachVolume(restoredVolume.getId(), vm.getId(), getNextAvailableDeviceId(vm.getId(), vmVolumes)); + _volumeDao.attachVolume(restoredVolume.getId(), vm.getId(), getNextAvailableDeviceId(volumeApiService.getReservedDeviceIds(vm), vmVolumes)); restoredVolume.setState(Volume.State.Ready); _volumeDao.update(restoredVolume.getId(), restoredVolume); UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_ATTACH, restoredVolume.getAccountId(), restoredVolume.getDataCenterId(), restoredVolume.getId(), restoredVolume.getName(), diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 5de82483edae..7a74fb13d2c9 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -160,6 +160,9 @@ import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.hypervisor.HypervisorCapabilitiesVO; import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao; +import com.cloud.network.Network.Provider; +import com.cloud.network.Network.Service; +import com.cloud.network.NetworkModel; import com.cloud.network.element.ConfigDriveNetworkElement; import com.cloud.offering.DiskOffering; import com.cloud.org.Cluster; @@ -220,6 +223,7 @@ import com.cloud.utils.fsm.NoTransitionException; import com.cloud.utils.fsm.StateMachine2; import com.cloud.vm.DiskProfile; +import com.cloud.vm.NicVO; import com.cloud.vm.VMInstanceDetailVO; import com.cloud.vm.UserVmManager; import com.cloud.vm.UserVmService; @@ -241,6 +245,7 @@ import com.cloud.vm.VmWorkResizeVolume; import com.cloud.vm.VmWorkSerializer; import com.cloud.vm.VmWorkTakeVolumeSnapshot; +import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.dao.VMInstanceDao; @@ -294,6 +299,10 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic @Inject private VMInstanceDetailsDao vmInstanceDetailsDao; @Inject + private NicDao _nicDao; + @Inject + private NetworkModel _networkModel; + @Inject private UserVmService _userVmService; @Inject private VolumeDataStoreDao _volumeStoreDao; @@ -5038,16 +5047,69 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion : minHwVersion; } + /** + * The number of CD-ROM devices of the Instance. Each CD-ROM device occupies a device id, starting from + * {@link TemplateManager#CDROM_PRIMARY_DEVICE_SEQ}. + */ + protected int getVmCdromCount(VirtualMachine vm) { + return 1; + } + + /** + * Checks if a config drive ISO is (or will be) attached to the Instance, which is the case when any of its networks + * has ConfigDrive as the provider of the Dhcp, Dns or UserData service. Checking the network providers (instead of + * only the config drive location detail, which is added when the ISO is created) also covers Instances which have + * never been started. + */ + protected boolean isConfigDriveUsed(VirtualMachine vm) { + if (vmInstanceDetailsDao.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) { + return true; + } + for (NicVO nic : _nicDao.listByVmId(vm.getId())) { + for (Service service : Arrays.asList(Service.UserData, Service.Dhcp, Service.Dns)) { + if (_networkModel.isProviderSupportServiceInNetwork(nic.getNetworkId(), service, Provider.ConfigDrive)) { + return true; + } + } + } + return false; + } + + @Override + public Set 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()); - boolean vmHasConfigDrive = vmInstanceDetailsDao.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION) != null; if (deviceId != null) { - if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || deviceId.longValue() == 3 - || (vmHasConfigDrive && deviceId.longValue() == ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ)) { - throw new RuntimeException("deviceId should be 0,1,2,4-" + maxDeviceId); + if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || reservedDeviceIds.contains(deviceId)) { + throw new RuntimeException(String.format("deviceId should be 0 or between 1 and %d, except the ones reserved for CD-ROM/config drive devices: %s", + maxDeviceId, reservedDeviceIds)); } for (VolumeVO vol : vols) { if (vol.getDeviceId().equals(deviceId)) { @@ -5056,21 +5118,13 @@ private Long getDeviceId(UserVmVO vm, Long deviceId) { } } else { // allocate deviceId here - List devIds = new ArrayList(); - for (int i = 1; i <= maxDeviceId; i++) { - devIds.add(String.valueOf(i)); - } - devIds.remove("3"); - if (vmHasConfigDrive) { - devIds.remove(ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ.toString()); - } for (VolumeVO vol : vols) { - devIds.remove(vol.getDeviceId().toString().trim()); + availableDeviceIds.remove(vol.getDeviceId()); } - if (devIds.isEmpty()) { + if (availableDeviceIds.isEmpty()) { throw new RuntimeException(String.format("All device Ids are used by vm %s", vm)); } - deviceId = Long.parseLong(devIds.iterator().next()); + deviceId = availableDeviceIds.get(0); } return deviceId; 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 585f00d80eb3..d0698a42cff2 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -78,6 +78,7 @@ import com.cloud.network.dao.NetworkDao; import com.cloud.network.dao.NetworkVO; import com.cloud.network.dao.PhysicalNetworkDao; +import com.cloud.network.element.ConfigDriveNetworkElement; import com.cloud.offering.DiskOffering; import com.cloud.offering.NetworkOffering; import com.cloud.offering.ServiceOffering; @@ -871,6 +872,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); @@ -1189,7 +1207,8 @@ private UserVm importVirtualMachineInternal(final UnmanagedInstanceTO unmanagedI DiskOfferingVO diskOffering = diskOfferingDao.findById(serviceOffering.getDiskOfferingId()); diskProfileStoragePoolList.add(importDisk(rootDisk, userVm, cluster, diskOffering, Volume.Type.ROOT, String.format("ROOT-%d", userVm.getId()), rootDisk.getCapacity(), minIops, maxIops, template, owner, null)); - long deviceId = 1L; + Set 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())); @@ -1198,7 +1217,7 @@ private UserVm importVirtualMachineInternal(final UnmanagedInstanceTO unmanagedI diskProfileStoragePoolList.add(importDisk(disk, userVm, cluster, offering, Volume.Type.DATADISK, String.format("DATA-%d-%s", userVm.getId(), disk.getDiskId()), disk.getCapacity(), offering.getMinIops(), offering.getMaxIops(), template, owner, deviceId)); - deviceId++; + deviceId = getNextDeviceId(deviceId, reservedDeviceIds); } } catch (Exception e) { logger.error(String.format("Failed to import volumes while importing vm: %s", displayName), e); @@ -2820,7 +2839,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++]; @@ -2828,7 +2848,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 d94f9db0c99c..f8297ab59bb8 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; @@ -93,6 +98,9 @@ public class KVMGuruTest { @Mock DpdkHelper dpdkHelperMock; + @Mock + HypervisorCapabilitiesDao hypervisorCapabilitiesDaoMock; + @Mock GuestOSVO guestOsVoMock; @@ -506,4 +514,42 @@ public void testGetNullWhenVMThereIsNoInformationOfUsedHosts() { Assert.assertNull(clusterId); } + + 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 b806eb4afe5c..46f5899e6d91 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; @@ -108,6 +110,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.org.Grouping; import com.cloud.projects.Project; import com.cloud.projects.ProjectManager; @@ -125,6 +131,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; @@ -137,12 +144,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; @@ -161,6 +173,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; @@ -2504,4 +2522,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/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java index 30bc769d5d4f..ca6f050aeead 100644 --- a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java @@ -35,9 +35,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; @@ -80,6 +82,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; @@ -1670,4 +1673,24 @@ public void getDetailAsIntegerTestThrowsInvalidParameterValueExceptionWhenValueI details.put("key", "not-a-number"); unmanagedVMsManager.getDetailAsInteger("key", details); } + + @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 a28af1aaab0f9a703fe98f1451432cb3dc74de06 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Fri, 16 Oct 2026 10:35:40 +0200 Subject: [PATCH 4/4] server: Return the device ids of the CD-ROMs from TemplateManager Move the calculation of the device ids reserved for the CD-ROM and the config drive ISO to TemplateManager, as getCdromDeviceIds(). An Instance has one CD-ROM, and the config drive ISO is one more CD-ROM (device id 4) when any network of the Instance uses config drive. VolumeApiService#getReservedDeviceIds() delegates to it, and a variant with the usage of config drive is added for the cases where the networks of the Instance are not known yet (VM import and backup restore). --- .../com/cloud/storage/VolumeApiService.java | 6 ++ .../com/cloud/template/TemplateManager.java | 19 ++++ .../java/com/cloud/hypervisor/KVMGuru.java | 5 +- .../cloud/storage/VolumeApiServiceImpl.java | 50 +--------- .../cloud/template/TemplateManagerImpl.java | 70 ++++++++++++++ .../vm/UnmanagedVMsManagerImpl.java | 5 +- .../storage/VolumeApiServiceImplTest.java | 91 ++++++------------ .../template/TemplateManagerImplTest.java | 92 +++++++++++++++++++ .../vm/UnmanagedVMsManagerImplTest.java | 5 +- 9 files changed, 223 insertions(+), 120 deletions(-) diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java index 9e272398535e..94299afebdda 100644 --- a/api/src/main/java/com/cloud/storage/VolumeApiService.java +++ b/api/src/main/java/com/cloud/storage/VolumeApiService.java @@ -192,6 +192,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); 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 d873bec6c273..dc568815c89b 100644 --- a/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java +++ b/engine/components-api/src/main/java/com/cloud/template/TemplateManager.java @@ -17,6 +17,7 @@ package com.cloud.template; import java.util.List; +import java.util.Set; import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; @@ -37,6 +38,7 @@ import com.cloud.storage.VMTemplateVO; import com.cloud.storage.VolumeVO; import com.cloud.utils.Pair; +import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineProfile; /** @@ -151,6 +153,23 @@ public interface TemplateManager { */ void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestination dest); + /** + * Returns the device ids of the CD-ROM devices of the Instance, starting from {@link #CDROM_PRIMARY_DEVICE_SEQ}. + * They cannot be used by the volumes of the Instance. + * An Instance has one CD-ROM device. If it uses config drive, the config drive ISO is one more CD-ROM, which has the + * device id 4. + * + * @param vm the Instance + * @return the device ids of the CD-ROM devices, including the one of the config drive ISO if the Instance uses config drive + */ + Set getCdromDeviceIds(VirtualMachine vm); + + /** + * Same as {@link #getCdromDeviceIds(VirtualMachine)}, with the specified usage of config drive instead of the one of + * the Instance. It is useful when the networks of the Instance are not known yet, e.g. when importing it. + */ + Set getCdromDeviceIds(VirtualMachine vm, boolean configDriveUsed); + public static final String MESSAGE_REGISTER_PUBLIC_TEMPLATE_EVENT = "Message.RegisterPublicTemplate.Event"; public static final String MESSAGE_RESET_TEMPLATE_PERMISSION_EVENT = "Message.ResetTemplatePermission.Event"; diff --git a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java index d174824accf6..cd669c9eef27 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; @@ -54,7 +53,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; @@ -367,8 +365,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 7a74fb13d2c9..4c1037f4c75a 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -160,10 +160,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; @@ -223,7 +219,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.VMInstanceDetailVO; import com.cloud.vm.UserVmManager; import com.cloud.vm.UserVmService; @@ -245,7 +240,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.VMInstanceDetailsDao; import com.cloud.vm.dao.VMInstanceDao; @@ -299,10 +293,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; @@ -5047,44 +5037,14 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion : minHwVersion; } - /** - * The number of CD-ROM devices of the Instance. Each CD-ROM device occupies a device id, starting from - * {@link TemplateManager#CDROM_PRIMARY_DEVICE_SEQ}. - */ - protected int getVmCdromCount(VirtualMachine vm) { - return 1; - } - - /** - * Checks if a config drive ISO is (or will be) attached to the Instance, which is the case when any of its networks - * has ConfigDrive as the provider of the Dhcp, Dns or UserData service. Checking the network providers (instead of - * only the config drive location detail, which is added when the ISO is created) also covers Instances which have - * never been started. - */ - protected boolean isConfigDriveUsed(VirtualMachine vm) { - if (vmInstanceDetailsDao.findDetail(vm.getId(), VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) { - return true; - } - for (NicVO nic : _nicDao.listByVmId(vm.getId())) { - for (Service service : Arrays.asList(Service.UserData, Service.Dhcp, Service.Dns)) { - if (_networkModel.isProviderSupportServiceInNetwork(nic.getNetworkId(), service, Provider.ConfigDrive)) { - return true; - } - } - } - return false; + @Override + public Set getReservedDeviceIds(VirtualMachine vm) { + return _tmpltMgr.getCdromDeviceIds(vm); } @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; + public Set getReservedDeviceIds(VirtualMachine vm, boolean configDriveUsed) { + 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 13c47e83bfc5..9da2792049c5 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -23,8 +23,10 @@ import java.util.Arrays; 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; @@ -151,6 +153,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; @@ -215,6 +221,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; @@ -223,8 +230,10 @@ import com.cloud.vm.VirtualMachineProfile; import com.cloud.vm.VirtualMachineProfileImpl; import com.cloud.vm.VmDetailConstants; +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.google.gson.Gson; import com.google.gson.GsonBuilder; @@ -324,6 +333,12 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, private HeuristicRuleHelper heuristicRuleHelper; @Inject private UserDataDao userDataDao; + @Inject + private NicDao _nicDao; + @Inject + private NetworkModel _networkModel; + @Inject + private VMInstanceDetailsDao _vmInstanceDetailsDao; private List _adapters; @@ -730,6 +745,61 @@ public void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestinat } } + @Override + public Set getCdromDeviceIds(VirtualMachine vm) { + if (vm.getType() != VirtualMachine.Type.User) { + return getCdromDeviceIds(1); + } + return getCdromDeviceIds(vm, isConfigDriveUsed(vm)); + } + + @Override + public Set getCdromDeviceIds(VirtualMachine vm, boolean configDriveUsed) { + return getCdromDeviceIds(getCdromCount(vm, configDriveUsed)); + } + + private Set getCdromDeviceIds(int cdromCount) { + Set deviceIds = new HashSet<>(); + for (int i = 0; i < cdromCount; i++) { + deviceIds.add((long) CDROM_PRIMARY_DEVICE_SEQ + i); + } + return deviceIds; + } + + private int getCdromCount(VirtualMachine vm, boolean configDriveUsed) { + // an Instance has one CD-ROM device for its ISO + int cdromCount = 1; + // the config drive ISO is one more CD-ROM, which has the device id of ConfigDriveNetworkElement#CONFIGDRIVEDISKSEQ + if (vm.getType() == VirtualMachine.Type.User && configDriveUsed) { + cdromCount += ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ - CDROM_PRIMARY_DEVICE_SEQ; + } + return cdromCount; + } + + /** + * 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 void prepareTemplateInOneStoragePool(final VMTemplateVO template, final StoragePoolVO pool) { logger.info("Schedule to preload Template {} into primary storage {}", template, pool); if (pool.getPoolType() == Storage.StoragePoolType.DatastoreCluster) { 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 d0698a42cff2..714baeac1d4c 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -78,7 +78,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; @@ -877,9 +876,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 46f5899e6d91..3d0693cdba13 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; @@ -110,10 +109,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.org.Grouping; import com.cloud.projects.Project; import com.cloud.projects.ProjectManager; @@ -144,17 +139,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; @@ -173,11 +163,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 @@ -2533,12 +2519,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 mockCdromDeviceIds(UserVmVO vm, Long... deviceIds) { + lenient().when(templateManagerMock.getCdromDeviceIds(vm)).thenReturn(new HashSet<>(Arrays.asList(deviceIds))); } private List mockVmVolumes(UserVmVO vm, long... deviceIds) { @@ -2553,52 +2535,21 @@ private List mockVmVolumes(UserVmVO vm, long... deviceIds) { } @Test - public void getReservedDeviceIdsReturnsOnlyCdromWhenVmDoesNotUseConfigDrive() { + public void getReservedDeviceIdsReturnsTheDeviceIdsOfTheCdroms() { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, false); - Set reserved = volumeApiServiceImpl.getReservedDeviceIds(vm); + mockCdromDeviceIds(vm, 3L); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); - Assert.assertEquals(Collections.singleton((long) TemplateManager.CDROM_PRIMARY_DEVICE_SEQ), reserved); + // the CD-ROM and the config drive ISO + mockCdromDeviceIds(vm, 3L, 4L); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); } @Test - public void getReservedDeviceIdsIncludesConfigDriveWhenNetworkProvidesItBeforeFirstStart() { - // the config drive location detail is only added when the ISO is created, i.e. at the first start + public void getDeviceIdUsesDeviceId4WhenVmHasOneCdrom() throws Exception { 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); + mockCdromDeviceIds(vm, 3L); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, null)); @@ -2606,9 +2557,9 @@ public void getDeviceIdUsesDeviceId4WhenVmDoesNotUseConfigDrive() throws Excepti } @Test - public void getDeviceIdSkipsConfigDriveDeviceIdWhenVmUsesConfigDrive() throws Exception { + public void getDeviceIdSkipsTheDeviceIdOfTheConfigDriveIso() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); + mockCdromDeviceIds(vm, 3L, 4L); mockVmVolumes(vm, 0L, 1L, 2L); Assert.assertEquals(Long.valueOf(5L), invokeGetDeviceId(vm, null)); @@ -2617,7 +2568,7 @@ public void getDeviceIdSkipsConfigDriveDeviceIdWhenVmUsesConfigDrive() throws Ex @Test public void getDeviceIdRejectsReservedDeviceIds() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); + mockCdromDeviceIds(vm, 3L, 4L); mockVmVolumes(vm, 0L); for (long reservedDeviceId : new long[] {3L, 4L}) { @@ -2631,9 +2582,9 @@ public void getDeviceIdRejectsReservedDeviceIds() throws Exception { } @Test - public void getDeviceIdKeepsMaxDataVolumesWhenVmUsesConfigDrive() throws Exception { + public void getDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved() throws Exception { UserVmVO vm = getMockedVm(); - mockVmNetworkProvidesConfigDrive(vm, true); + mockCdromDeviceIds(vm, 3L, 4L); List volumes = mockVmVolumes(vm, 0L); // 6 data volumes are supported by default @@ -2654,4 +2605,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.getCdromDeviceIds(vm, false)).thenReturn(new HashSet<>(Arrays.asList(3L))); + when(templateManagerMock.getCdromDeviceIds(vm, true)).thenReturn(new HashSet<>(Arrays.asList(3L, 4L))); + + Assert.assertEquals(new HashSet<>(Arrays.asList(3L)), volumeApiServiceImpl.getReservedDeviceIds(vm, false)); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)), 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 b1135e07562d..2da25ce10769 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java @@ -30,7 +30,11 @@ import java.io.IOException; 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.BlockingQueue; import java.util.concurrent.ExecutorService; @@ -113,6 +117,9 @@ import com.cloud.host.dao.HostDao; 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; @@ -150,9 +157,16 @@ import com.cloud.utils.UriUtils; import com.cloud.utils.concurrency.NamedThreadFactory; import com.cloud.utils.exception.CloudRuntimeException; +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.VmDetailConstants; +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 junit.framework.TestCase; @@ -230,6 +244,15 @@ public class TemplateManagerImplTest extends TestCase { @Mock private UserDataVO userDataMock; + @Mock + private NicDao nicDaoMock; + + @Mock + private NetworkModel networkModelMock; + + @Mock + private VMInstanceDetailsDao vmInstanceDetailsDaoMock; + public class CustomThreadPoolExecutor extends ThreadPoolExecutor { AtomicInteger ai = new AtomicInteger(0); public CustomThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, @@ -814,6 +837,75 @@ public void verifyHeuristicRulesForZoneTestTemplateNotISOFormatShouldCheckForTem Mockito.verify(heuristicRuleHelperMock, Mockito.times(1)).getImageStoreIfThereIsHeuristicRule(1L, HeuristicType.TEMPLATE, vmTemplateVOMock); } + private Set ids(Long... deviceIds) { + return new HashSet<>(Arrays.asList(deviceIds)); + } + + private UserVmVO mockUserVm() { + UserVmVO vm = Mockito.mock(UserVmVO.class); + Mockito.lenient().when(vm.getId()).thenReturn(1L); + Mockito.lenient().when(vm.getType()).thenReturn(VirtualMachine.Type.User); + return vm; + } + + private void mockVmNetworkProvidesConfigDrive(UserVmVO vm, boolean providesConfigDrive) { + NicVO nic = Mockito.mock(NicVO.class); + Mockito.when(nic.getNetworkId()).thenReturn(100L); + Mockito.when(nicDaoMock.listByVmId(vm.getId())).thenReturn(Collections.singletonList(nic)); + Mockito.lenient().when(networkModelMock.isProviderSupportServiceInNetwork(Mockito.eq(100L), Mockito.any(Service.class), Mockito.eq(Provider.ConfigDrive))).thenReturn(providesConfigDrive); + } + + @Test + public void getCdromDeviceIdsReturnsOnlyThePrimaryCdromForSystemVms() { + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.SecondaryStorageVm); + + Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm)); + Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm, true)); + } + + @Test + public void isConfigDriveUsedReturnsTrueWhenNetworkProvidesConfigDriveBeforeFirstStart() { + UserVmVO vm = mockUserVm(); + mockVmNetworkProvidesConfigDrive(vm, true); + + Assert.assertTrue(templateManager.isConfigDriveUsed(vm)); + } + + @Test + public void isConfigDriveUsedReturnsTrueWhenConfigDriveLocationDetailExists() { + UserVmVO vm = mockUserVm(); + Mockito.when(vmInstanceDetailsDaoMock.findDetail(1L, VmDetailConstants.CONFIG_DRIVE_LOCATION)).thenReturn(Mockito.mock(VMInstanceDetailVO.class)); + + Assert.assertTrue(templateManager.isConfigDriveUsed(vm)); + } + + @Test + public void isConfigDriveUsedReturnsFalseWhenNoNetworkProvidesConfigDrive() { + UserVmVO vm = mockUserVm(); + mockVmNetworkProvidesConfigDrive(vm, false); + + Assert.assertFalse(templateManager.isConfigDriveUsed(vm)); + } + + @Test + public void getCdromDeviceIdsReturnsOnlyThePrimaryCdromWhenVmDoesNotUseConfigDrive() { + UserVmVO vm = mockUserVm(); + mockVmNetworkProvidesConfigDrive(vm, false); + + Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm)); + } + + @Test + public void getCdromDeviceIdsAddsTheCdromOfConfigDrive() { + UserVmVO vm = mockUserVm(); + mockVmNetworkProvidesConfigDrive(vm, true); + + Assert.assertEquals(ids(3L, 4L), templateManager.getCdromDeviceIds(vm)); + Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm, false)); + Assert.assertEquals(ids(3L, 4L), templateManager.getCdromDeviceIds(vm, true)); + } + @Configuration @ComponentScan(basePackageClasses = {TemplateManagerImpl.class}, includeFilters = {@ComponentScan.Filter(value = TestConfiguration.Library.class, type = FilterType.CUSTOM)}, 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 ca6f050aeead..10c9031be73a 100644 --- a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java @@ -1675,9 +1675,10 @@ public void getDetailAsIntegerTestThrowsInvalidParameterValueExceptionWhenValueI } @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);