diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java index 19c2ebe455a5..94299afebdda 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,22 @@ Volume updateVolume(long volumeId, String path, String state, Long storageId, */ boolean doesStoragePoolSupportDiskOfferingTags(StoragePool destPool, String diskOfferingTags); + /** + * Returns the device ids of the Instance which are used by non-volume devices (the CD-ROM drives and the config drive ISO), + * and therefore must not be assigned to its volumes. Otherwise the hypervisor may end up with two devices using the same + * target name (e.g. sde), which is rejected. + * + * @param vm the Instance + * @return the reserved device ids + */ + Set getReservedDeviceIds(VirtualMachine vm); + + /** + * Same as {@link #getReservedDeviceIds(VirtualMachine)}, with the specified usage of config drive instead of the one + * of the Instance. It is useful when the networks of the Instance are not known yet, e.g. when importing it. + */ + Set getReservedDeviceIds(VirtualMachine vm, boolean configDriveUsed); + boolean validateConditionsToReplaceDiskOfferingOfVolume(Volume volume, DiskOffering newDiskOffering, StoragePool destPool); Volume destroyVolume(long volumeId, Account caller, boolean expunge, boolean forceExpunge); 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..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; /** @@ -67,6 +69,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. * @@ -148,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 afb695d78574..cd669c9eef27 100644 --- a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java +++ b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java @@ -32,6 +32,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; @@ -54,6 +55,7 @@ import java.util.ArrayList; 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; @@ -71,6 +73,8 @@ public class KVMGuru extends HypervisorGuruBase implements HypervisorGuru { VolumeDao _volumeDao; @Inject HypervisorCapabilitiesDao _hypervisorCapabilitiesDao; + @Inject + VolumeApiService volumeApiService; @Override @@ -86,30 +90,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); } @@ -361,6 +364,8 @@ public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId, vm.setPowerState(VirtualMachine.PowerState.PowerOff); _instanceDao.update(vm.getId(), vm); } + // the networks of the (possibly expunged) VM may not be available, so always keep the config drive device id free + Set reservedDeviceIds = volumeApiService.getReservedDeviceIds(vm, true); for (Backup.VolumeInfo VMVolToRestore : vm.getBackupVolumeList()) { VolumeVO volume = _volumeDao.findByUuidIncludingRemoved(VMVolToRestore.getUuid()); volume.setState(Volume.State.Ready); @@ -371,7 +376,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()); @@ -389,7 +394,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(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 9b2f4f831d3e..4c1037f4c75a 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -5037,14 +5037,39 @@ protected String getMinimumHypervisorVersionInDatacenter(long datacenterId, Hype return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion : minHwVersion; } + @Override + public Set getReservedDeviceIds(VirtualMachine vm) { + return _tmpltMgr.getCdromDeviceIds(vm); + } + + @Override + public Set getReservedDeviceIds(VirtualMachine vm, boolean configDriveUsed) { + return _tmpltMgr.getCdromDeviceIds(vm, configDriveUsed); + } + + /** + * Returns the first {@code maxDataVolumes} device ids (starting from 1) which are not reserved. + */ + private List getAvailableDeviceIds(int maxDataVolumes, Set reservedDeviceIds) { + List availableDeviceIds = new ArrayList<>(); + for (long id = 1; availableDeviceIds.size() < maxDataVolumes; id++) { + if (!reservedDeviceIds.contains(id)) { + availableDeviceIds.add(id); + } + } + return availableDeviceIds; + } + private Long getDeviceId(UserVmVO vm, Long deviceId) { // allocate deviceId - int maxDevices = getMaxDataVolumesSupported(vm) + 2; // add 2 to consider devices root volume and cdrom - int maxDeviceId = maxDevices - 1; + Set reservedDeviceIds = getReservedDeviceIds(vm); + List availableDeviceIds = getAvailableDeviceIds(getMaxDataVolumesSupported(vm), reservedDeviceIds); + long maxDeviceId = availableDeviceIds.get(availableDeviceIds.size() - 1); List vols = _volsDao.findByInstance(vm.getId()); if (deviceId != null) { - if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || deviceId.longValue() == 3) { - throw new RuntimeException("deviceId should be 0,1,2,4-" + maxDeviceId); + if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId || reservedDeviceIds.contains(deviceId)) { + throw new RuntimeException(String.format("deviceId should be 0 or between 1 and %d, except the ones reserved for CD-ROM/config drive devices: %s", + maxDeviceId, reservedDeviceIds)); } for (VolumeVO vol : vols) { if (vol.getDeviceId().equals(deviceId)) { @@ -5053,18 +5078,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 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 585f00d80eb3..714baeac1d4c 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -871,6 +871,21 @@ private Pair importDisk(UnmanagedInstanceTO.Disk disk, return new Pair(profile, storagePool); } + /** + * Returns the device ids which cannot be used by the volumes of the imported Instance: the ones of the CD-ROM devices, + * and the one of the config drive ISO (always kept free, as the NICs of the Instance are not imported yet at this point). + */ + private Set getReservedDeviceIds(VirtualMachine vm) { + return volumeApiService.getReservedDeviceIds(vm, true); + } + + private long getNextDeviceId(long deviceId, Set reservedDeviceIds) { + do { + deviceId++; + } while (reservedDeviceIds.contains(deviceId)); + return deviceId; + } + private NicProfile importNic(UnmanagedInstanceTO.Nic nic, VirtualMachine vm, Network network, Network.IpAddresses ipAddresses, int deviceId, boolean isDefaultNic, boolean forced) throws InsufficientVirtualNetworkCapacityException, InsufficientAddressCapacityException { DataCenterVO dataCenterVO = dataCenterDao.findById(network.getDataCenterId()); Pair result = networkOrchestrationService.importNic(nic.getMacAddress(), deviceId, network, isDefaultNic, vm, ipAddresses, dataCenterVO, forced); @@ -1189,7 +1204,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 +1214,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 +2836,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 +2845,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..3d0693cdba13 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -37,6 +37,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.UUID; import java.util.concurrent.ExecutionException; @@ -125,6 +126,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; @@ -161,6 +163,8 @@ public class VolumeApiServiceImplTest { @Mock private VolumeDataStoreDao volumeDataStoreDaoMock; @Mock + private TemplateManager templateManagerMock; + @Mock private VolumeDao volumeDaoMock; @Mock private BackupDao backupDaoMock; @@ -2504,4 +2508,111 @@ public void testResizeVolumeInternal_VMware_VMRunning_ShouldThrowStateGuardError e.getMessage() != null && e.getMessage().contains("VM should be in")); } } + + private Long invokeGetDeviceId(UserVmVO vm, Long deviceId) throws Exception { + java.lang.reflect.Method method = VolumeApiServiceImpl.class.getDeclaredMethod("getDeviceId", UserVmVO.class, Long.class); + method.setAccessible(true); + try { + return (Long) method.invoke(volumeApiServiceImpl, vm, deviceId); + } catch (InvocationTargetException e) { + throw (Exception) e.getCause(); + } + } + + private void mockCdromDeviceIds(UserVmVO vm, Long... deviceIds) { + lenient().when(templateManagerMock.getCdromDeviceIds(vm)).thenReturn(new HashSet<>(Arrays.asList(deviceIds))); + } + + private List mockVmVolumes(UserVmVO vm, long... deviceIds) { + List volumes = new ArrayList<>(); + for (long deviceId : deviceIds) { + VolumeVO volume = Mockito.mock(VolumeVO.class); + lenient().when(volume.getDeviceId()).thenReturn(deviceId); + volumes.add(volume); + } + lenient().when(volumeDaoMock.findByInstance(vm.getId())).thenReturn(volumes); + return volumes; + } + + @Test + public void getReservedDeviceIdsReturnsTheDeviceIdsOfTheCdroms() { + UserVmVO vm = getMockedVm(); + + mockCdromDeviceIds(vm, 3L); + Assert.assertEquals(new HashSet<>(Arrays.asList(3L)), volumeApiServiceImpl.getReservedDeviceIds(vm)); + + // 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 getDeviceIdUsesDeviceId4WhenVmHasOneCdrom() throws Exception { + UserVmVO vm = getMockedVm(); + mockCdromDeviceIds(vm, 3L); + mockVmVolumes(vm, 0L, 1L, 2L); + + Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, null)); + Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, 4L)); + } + + @Test + public void getDeviceIdSkipsTheDeviceIdOfTheConfigDriveIso() throws Exception { + UserVmVO vm = getMockedVm(); + mockCdromDeviceIds(vm, 3L, 4L); + mockVmVolumes(vm, 0L, 1L, 2L); + + Assert.assertEquals(Long.valueOf(5L), invokeGetDeviceId(vm, null)); + } + + @Test + public void getDeviceIdRejectsReservedDeviceIds() throws Exception { + UserVmVO vm = getMockedVm(); + mockCdromDeviceIds(vm, 3L, 4L); + mockVmVolumes(vm, 0L); + + for (long reservedDeviceId : new long[] {3L, 4L}) { + try { + invokeGetDeviceId(vm, reservedDeviceId); + fail("deviceId " + reservedDeviceId + " should be rejected"); + } catch (RuntimeException e) { + Assert.assertTrue(e.getMessage(), e.getMessage().contains("deviceId should be 0 or between 1 and")); + } + } + } + + @Test + public void getDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved() throws Exception { + UserVmVO vm = getMockedVm(); + mockCdromDeviceIds(vm, 3L, 4L); + List volumes = mockVmVolumes(vm, 0L); + + // 6 data volumes are supported by default + List allocated = new ArrayList<>(); + for (int i = 0; i < 6; i++) { + Long deviceId = invokeGetDeviceId(vm, null); + allocated.add(deviceId); + VolumeVO volume = Mockito.mock(VolumeVO.class); + lenient().when(volume.getDeviceId()).thenReturn(deviceId); + volumes.add(volume); + } + + Assert.assertEquals(Arrays.asList(1L, 2L, 5L, 6L, 7L, 8L), allocated); + try { + invokeGetDeviceId(vm, null); + fail("All device ids are used, an exception is expected"); + } catch (RuntimeException e) { + Assert.assertTrue(e.getMessage(), e.getMessage().contains("All device Ids are used")); + } + } + + @Test + public void getReservedDeviceIdsWithConfigDriveReservesTheCdromOfConfigDrive() { + UserVmVO vm = getMockedVm(); + when(templateManagerMock.getCdromDeviceIds(vm, false)).thenReturn(new HashSet<>(Arrays.asList(3L))); + 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 30bc769d5d4f..10c9031be73a 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,25 @@ public void getDetailAsIntegerTestThrowsInvalidParameterValueExceptionWhenValueI details.put("key", "not-a-number"); unmanagedVMsManager.getDetailAsInteger("key", details); } + + @Test + public void getReservedDeviceIdsAlwaysReservesTheCdromOfConfigDrive() { + VirtualMachine vm = Mockito.mock(VirtualMachine.class); + // the networks of the Instance are not imported yet, so the config drive is always assumed + Mockito.when(volumeApiService.getReservedDeviceIds(vm, true)).thenReturn(new HashSet<>(java.util.Arrays.asList(3L, 4L))); + + Set reserved = ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getReservedDeviceIds", vm); + + Assert.assertEquals(new HashSet<>(java.util.Arrays.asList(3L, 4L)), reserved); + } + + @Test + public void getNextDeviceIdSkipsReservedDeviceIds() { + Set reserved = new HashSet<>(java.util.Arrays.asList(3L, 4L)); + + Assert.assertEquals(1L, (long) ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getNextDeviceId", 0L, reserved)); + Assert.assertEquals(2L, (long) ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getNextDeviceId", 1L, reserved)); + Assert.assertEquals(5L, (long) ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getNextDeviceId", 2L, reserved)); + Assert.assertEquals(6L, (long) ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getNextDeviceId", 5L, reserved)); + } }