This is an automated email from the ASF dual-hosted git repository.
RosiKyu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/main by this push:
new ac65e3ddab0 server,kvm: Avoid device id conflicts between data
volumes, CD-ROMs and the config drive ISO (#14407)
ac65e3ddab0 is described below
commit ac65e3ddab0b200965f83c77d5e960d3c613764b
Author: Wei Zhou <[email protected]>
AuthorDate: Wed Oct 28 10:40:02 2026 +0100
server,kvm: Avoid device id conflicts between data volumes, CD-ROMs and the
config drive ISO (#14407)
* server: Reserve CD-ROM and config drive device ids for volumes
Data volumes could be assigned the same device id as one of the CD-ROM
devices or the config drive ISO, which end up with the same libvirt target
name (e.g. sde) when they are on the same bus namespace (SATA with
UEFI/Q35). With multiple CD-ROM support, the KVM agent defines all the
CD-ROMs supported by the host at boot, so any slot after the primary one
can conflict.
Add VolumeApiService.getReservedDeviceIds(), which returns the device ids
of the CD-ROM devices and of the config drive ISO when the Instance uses
config drive. The number of CD-ROM devices is returned by
TemplateManager.getVmCdromCount(), based on the CD-ROM cap advertised by the
host and the ISOs attached. The config drive is detected from the network
providers (Dhcp, Dns, UserData), so Instances which have never been started
are covered as well as the ones with the config drive location detail.
The reserved ids are skipped when allocating device ids in:
- attach volume
- restore of volumes and Instances from backup
- import of Instances (unmanaged Instances and VMware to KVM)
The max number of data volumes is preserved when device ids are reserved,
and the error message of an invalid deviceId lists the reserved ids.
* server,kvm: Calculate the device id of the config drive ISO
The config drive ISO is attached as a CD-ROM, but its device id was
hardcoded to 4, which is also the device id of the second ISO CD-ROM when
multiple ISOs are supported. With vm.iso.max.count >= 2 the Instance could
get two CD-ROMs on the same device id.
The config drive ISO is now the CD-ROM following the ones of the ISOs, so
its device id is the primary CD-ROM device id plus the number of the CD-ROMs
of the ISOs (TemplateManager.getConfigDriveDeviceId). As the config drive
ISO takes one of the CD-ROMs supported by the host, the maximum number of
ISOs of an Instance using config drive is lowered by one.
The number of CD-ROM devices of an Instance
(TemplateManager.getVmCdromCount)
includes the config drive ISO, so the device ids reserved for the volumes
follow it, and the volume device id allocators do not need to know about the
config drive any more.
The KVM agent finds the config drive CD-ROM by the path of its ISO, and
keeps
its label and bus when refreshing it, instead of using the hardcoded id 4
and
the label hdd (which does not match the SATA CD-ROMs of UEFI Instances).
* server,kvm: Do not count the config drive ISO in vm.iso.max.count
vm.iso.max.count is the maximum number of ISOs of an Instance, so it should
not take the config drive ISO into account: the config drive ISO is attached
as an additional CD-ROM, following the ones of the ISOs.
As the IDE bus only has the slots of the first MAX_CDROMS_PER_VM CD-ROMs
(hdc and hdd), the KVM agent attaches the CD-ROMs beyond these slots (e.g.
the
config drive ISO following the maximum number of ISOs) on the SATA bus.
* Revert "server,kvm: Calculate the device id of the config drive ISO"
The config drive ISO keeps the device id 4, whatever the number of CD-ROMs
of the Instance. The CD-ROMs of the ISOs are attached around it.
This reverts the commits:
- server,kvm: Do not count the config drive ISO in vm.iso.max.count
- server,kvm: Calculate the device id of the config drive ISO
* server,kvm: Skip the device id of the config drive ISO for the ISOs
The config drive ISO keeps the device id 4, whatever the number of ISOs of
the Instance. When an Instance uses config drive, the CD-ROMs of the ISOs
which would use that device id or the next ones are moved to the next device
ids, i.e. the CD-ROMs are, by device id: ISO 1 (3), config drive (4),
ISO 2 (5), ISO 3 (6), ...
The ISO slots stored for the Instance (user_vm.iso_id and vm_iso_map) are
not
changed. They are converted to device ids when the CD-ROMs are defined for
the
Instance, and when an ISO is attached or detached, so Instances which
already
have an ISO in the slot following the first one do not need to be updated.
The number of CD-ROM devices of an Instance
(TemplateManager.getVmCdromCount)
includes the one of the config drive ISO, so the device ids reserved for the
volumes cover all of them. When the networks of an Instance are not known
yet
(import, restore from backup), the config drive is always assumed.
CloudStack supports 2 CD-ROMs per Instance on the IDE bus (hdc and hdd, the
first two slots are kept for the disks). The KVM agent defines the CD-ROMs
beyond these slots on the SATA bus.
* server: Return the device ids of the CD-ROMs instead of their number
TemplateManager exposed the number of CD-ROMs of an Instance, which the only
caller (VolumeApiServiceImpl) converted back to the device ids starting from
the primary CD-ROM device id. Return the device ids directly
(TemplateManager.getCdromDeviceIds), so the callers do not have to assume
that they are consecutive, and the number of CD-ROMs becomes an internal
detail of TemplateManagerImpl.
---
.../java/com/cloud/storage/VolumeApiService.java | 18 +++
.../java/com/cloud/template/TemplateManager.java | 20 +++
.../hypervisor/kvm/resource/LibvirtVMDef.java | 16 +-
.../hypervisor/kvm/resource/LibvirtVMDefTest.java | 21 +++
.../main/java/com/cloud/hypervisor/KVMGuru.java | 31 ++--
.../network/element/ConfigDriveNetworkElement.java | 2 +-
.../com/cloud/storage/VolumeApiServiceImpl.java | 44 ++++--
.../com/cloud/template/TemplateManagerImpl.java | 94 ++++++++++-
.../cloudstack/vm/UnmanagedVMsManagerImpl.java | 25 ++-
.../java/com/cloud/hypervisor/KVMGuruTest.java | 87 +++++++++-
.../cloud/storage/VolumeApiServiceImplTest.java | 114 ++++++++++++++
.../cloud/template/TemplateManagerImplTest.java | 175 +++++++++++++++++++++
.../cloudstack/vm/UnmanagedVMsManagerImplTest.java | 24 +++
13 files changed, 634 insertions(+), 37 deletions(-)
diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java
b/api/src/main/java/com/cloud/storage/VolumeApiService.java
index 372eb038561..78bc106e00c 100644
--- a/api/src/main/java/com/cloud/storage/VolumeApiService.java
+++ b/api/src/main/java/com/cloud/storage/VolumeApiService.java
@@ -21,6 +21,7 @@ package com.cloud.storage;
import java.net.MalformedURLException;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import com.cloud.dc.DataCenter;
import com.cloud.exception.ResourceAllocationException;
@@ -28,6 +29,7 @@ import com.cloud.offering.DiskOffering;
import com.cloud.user.Account;
import com.cloud.utils.Pair;
import com.cloud.utils.fsm.NoTransitionException;
+import com.cloud.vm.VirtualMachine;
import org.apache.cloudstack.api.command.user.volume.AssignVolumeCmd;
import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd;
@@ -187,6 +189,22 @@ public interface VolumeApiService {
*/
boolean doesStoragePoolSupportDiskOfferingTags(StoragePool destPool,
String diskOfferingTags);
+ /**
+ * Returns the device ids of the Instance which are used by non-volume
devices (the CD-ROM drives and the config drive ISO),
+ * and therefore must not be assigned to its volumes. Otherwise the
hypervisor may end up with two devices using the same
+ * target name (e.g. sde), which is rejected.
+ *
+ * @param vm the Instance
+ * @return the reserved device ids
+ */
+ Set<Long> getReservedDeviceIds(VirtualMachine vm);
+
+ /**
+ * Same as {@link #getReservedDeviceIds(VirtualMachine)}, with the
specified usage of config drive instead of the one
+ * of the Instance. It is useful when the networks of the Instance are not
known yet, e.g. when importing it.
+ */
+ Set<Long> getReservedDeviceIds(VirtualMachine vm, boolean configDriveUsed);
+
boolean validateConditionsToReplaceDiskOfferingOfVolume(Volume volume,
DiskOffering newDiskOffering, StoragePool destPool);
Volume destroyVolume(long volumeId, Account caller, boolean expunge,
boolean forceExpunge, Boolean countDisplayFalseInResourceCount);
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 8c11fe6c93a..4b5ca499937 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.VMTemplateStoragePoolVO;
import com.cloud.storage.VMTemplateVO;
import com.cloud.storage.VolumeVO;
import com.cloud.utils.Pair;
+import com.cloud.vm.VirtualMachine;
import com.cloud.vm.VirtualMachineProfile;
/**
@@ -183,6 +185,24 @@ public interface TemplateManager {
*/
void prepareIsoForVmProfile(VirtualMachineProfile profile,
DeployDestination dest);
+ /**
+ * Returns the device ids of the CD-ROM devices of the Instance, starting
from {@link #CDROM_PRIMARY_DEVICE_SEQ}.
+ * They cannot be used by the volumes of the Instance.
+ * The hypervisor defines all the CD-ROM devices it supports for the
Instance (see {@code host.cdrom.max.count}),
+ * whether or not ISOs are attached to them. If the Instance uses config
drive, the config drive ISO is one more
+ * CD-ROM: it keeps its device id (4), and the CD-ROMs of the ISOs from
that device id are moved to the next ones.
+ *
+ * @param vm the Instance
+ * @return the device ids of the CD-ROM devices, including the one of the
config drive ISO if the Instance uses config drive
+ */
+ Set<Long> getCdromDeviceIds(VirtualMachine vm);
+
+ /**
+ * Same as {@link #getCdromDeviceIds(VirtualMachine)}, with the specified
usage of config drive instead of the one of
+ * the Instance. It is useful when the networks of the Instance are not
known yet, e.g. when importing it.
+ */
+ Set<Long> getCdromDeviceIds(VirtualMachine vm, boolean configDriveUsed);
+
public static final String MESSAGE_REGISTER_PUBLIC_TEMPLATE_EVENT =
"Message.RegisterPublicTemplate.Event";
public static final String MESSAGE_RESET_TEMPLATE_PERMISSION_EVENT =
"Message.ResetTemplatePermission.Event";
diff --git
a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
index b6b03bc4e11..4a1aa0ee924 100644
---
a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
+++
b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
@@ -39,6 +39,7 @@ import com.cloud.agent.api.to.VirtualMachineMetadataTO;
import com.cloud.agent.properties.AgentProperties;
import com.cloud.agent.properties.AgentPropertiesFileHandler;
+import com.cloud.template.TemplateManager;
import org.w3c.dom.Document;
import org.w3c.dom.Element;
@@ -57,8 +58,10 @@ import static
java.time.format.DateTimeFormatter.ISO_LOCAL_DATE_TIME;
public class LibvirtVMDef {
protected static Logger LOGGER = LogManager.getLogger(LibvirtVMDef.class);
- // CD-ROM slot allocation: getDevLabel() maps deviceSeq=3,4 to hdc and hdd
on the IDE bus.
- // Bumping this requires extending getDevLabel() (e.g. to spill onto SATA
or a second IDE controller).
+ // Number of CD-ROMs supported by CloudStack per VM (not a limit of the
guest OS or QEMU). The CD-ROMs are on the IDE bus,
+ // which has 4 slots: the first two are kept for disks, so the ISOs get
hdc and hdd (getDevLabel() maps deviceSeq=3,4).
+ // Going past 2 means putting the CD-ROMs on the SATA bus (or on a second
IDE controller): the CD-ROMs with a device id
+ // beyond these slots, e.g. the one following the config drive ISO (which
keeps the device id 4), are on the SATA bus.
public static final int MAX_CDROMS_PER_VM = 2;
private String _hvsType;
@@ -1147,7 +1150,16 @@ public class LibvirtVMDef {
_bus = DiskBus.IDE;
}
+ private boolean isIdeCdromSlot(Integer devId) {
+ return devId == null || devId <
TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + MAX_CDROMS_PER_VM;
+ }
+
public void defISODisk(String volPath, Integer devId, boolean
isSecure, DiskType diskType) {
+ // the IDE bus only has the slots of MAX_CDROMS_PER_VM CD-ROMs
(hdc and hdd), the other ones (e.g. the config drive ISO)
+ // are on the SATA bus
+ if (!isSecure && !isIdeCdromSlot(devId)) {
+ isSecure = true;
+ }
if (!isSecure) {
defISODisk(volPath, devId, diskType);
} else {
diff --git
a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java
b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java
index 56ad267eac7..a51719a3326 100644
---
a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java
+++
b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java
@@ -597,4 +597,25 @@ public class LibvirtVMDefTest extends TestCase {
"<backend type='emulator' version='2.0'/>\n" +
"</tpm>\n", tpmDef.toString());
}
+
+ @Test
+ public void testIsoDiskDefOnIdeBusForTheIdeCdromSlots() {
+ for (int[] slot : new int[][] {{3, 'c'}, {4, 'd'}}) {
+ DiskDef cdrom = new DiskDef();
+ cdrom.defISODisk(null, slot[0], false, DiskDef.DiskType.FILE);
+
+ assertEquals(DiskDef.DiskBus.IDE, cdrom.getBusType());
+ assertEquals("hd" + (char) slot[1], cdrom.getDiskLabel());
+ }
+ }
+
+ @Test
+ public void testIsoDiskDefBeyondTheIdeCdromSlotsIsOnSataBus() {
+ // e.g. the config drive ISO following the ISOs: IDE only has the hdc
and hdd slots for CD-ROMs
+ DiskDef cdrom = new DiskDef();
+ cdrom.defISODisk("/mnt/pool/configdrive/i-2-3-VM.iso", 5, false,
DiskDef.DiskType.FILE);
+
+ assertEquals(DiskDef.DiskBus.SATA, cdrom.getBusType());
+ assertEquals("sdf", cdrom.getDiskLabel());
+ }
}
diff --git a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java
b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java
index bdf52407841..5e3beed769a 100644
--- a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java
+++ b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java
@@ -58,6 +58,7 @@ import java.math.RoundingMode;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import org.apache.cloudstack.utils.bytescale.ByteScaleUtils;
public class KVMGuru extends HypervisorGuruBase implements HypervisorGuru {
@@ -91,30 +92,29 @@ public class KVMGuru extends HypervisorGuruBase implements
HypervisorGuru {
* Get next free DeviceId for a KVM Guest
*/
- protected Long getNextAvailableDeviceId(List<VolumeVO> vmVolumes) {
+ protected Long getNextAvailableDeviceId(Set<Long> reservedDeviceIds,
List<VolumeVO> vmVolumes) {
int maxDataVolumesSupported;
- int maxDeviceId;
- List<String> devIds = new ArrayList<>();
+ List<Long> devIds = new ArrayList<>();
try {
maxDataVolumesSupported =
_hypervisorCapabilitiesDao.getMaxDataVolumesLimit(HypervisorType.KVM,"default");
- int maxDevices = maxDataVolumesSupported + 2; // add 2 to consider
devices root volume and cdrom
- maxDeviceId = maxDevices - 1;
} catch (Exception e) {
throw new RuntimeException("Cannot find maximum number of disk
devices that can be attached to the KVM Hypervisor");
}
- for (int i = 1; i <= maxDeviceId; i++) {
- devIds.add(String.valueOf(i));
+ // the device ids reserved for the CD-ROM and config drive devices
cannot be used by volumes
+ for (long id = 1; devIds.size() < maxDataVolumesSupported; id++) {
+ if (!reservedDeviceIds.contains(id)) {
+ devIds.add(id);
+ }
}
- devIds.remove("3");
for (VolumeVO vmVolume : vmVolumes) {
- devIds.remove(vmVolume.getDeviceId().toString().trim());
+ devIds.remove(vmVolume.getDeviceId());
}
if (devIds.isEmpty()) {
throw new RuntimeException("All device Ids are in use.");
}
- return Long.parseLong(devIds.iterator().next());
+ return devIds.get(0);
}
@@ -373,6 +373,8 @@ public class KVMGuru extends HypervisorGuruBase implements
HypervisorGuru {
vm.setPowerState(VirtualMachine.PowerState.PowerOff);
_instanceDao.update(vm.getId(), vm);
}
+ // the networks of the (possibly expunged) VM may not be available,
so always keep the config drive device id free
+ Set<Long> reservedDeviceIds =
volumeApiService.getReservedDeviceIds(vm, true);
for (Backup.VolumeInfo VMVolToRestore : vm.getBackupVolumeList()) {
VolumeVO volume =
_volumeDao.findByUuidIncludingRemoved(VMVolToRestore.getUuid());
volume.setState(Volume.State.Ready);
@@ -388,9 +390,10 @@ public class KVMGuru extends HypervisorGuruBase implements
HypervisorGuru {
// it here shifts every data disk by one slot on every
restore, which then misaligns the
// device-id ordering that the restore itself uses to map
backup files onto volumes.
Long deviceId = VMVolToRestore.getDeviceId();
- if (deviceId == null) {
- // Backups taken before the device id was recorded in
the backup metadata.
- deviceId =
getNextAvailableDeviceId(_volumeDao.findByInstance(vm.getId()));
+ if (deviceId == null ||
reservedDeviceIds.contains(deviceId)) {
+ // Backups taken before the device id was recorded in
the backup metadata, or with a volume on a
+ // device id which is reserved now for a CD-ROM or the
config drive ISO.
+ deviceId = getNextAvailableDeviceId(reservedDeviceIds,
_volumeDao.findByInstance(vm.getId()));
}
_volumeDao.attachVolume(volume.getId(), vm.getId(),
deviceId);
}
@@ -415,7 +418,7 @@ public class KVMGuru extends HypervisorGuruBase implements
HypervisorGuru {
}
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 6cdd6f5753d..fbc0d106edc 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 e81cc00564e..0702450d85c 100644
--- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
+++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
@@ -5358,14 +5358,39 @@ public class VolumeApiServiceImpl extends ManagerBase
implements VolumeApiServic
return StringUtils.isBlank(minHwVersion) ? defaultHypervisorVersion :
minHwVersion;
}
+ @Override
+ public Set<Long> getReservedDeviceIds(VirtualMachine vm) {
+ return _tmpltMgr.getCdromDeviceIds(vm);
+ }
+
+ @Override
+ public Set<Long> getReservedDeviceIds(VirtualMachine vm, boolean
configDriveUsed) {
+ return _tmpltMgr.getCdromDeviceIds(vm, configDriveUsed);
+ }
+
+ /**
+ * Returns the first {@code maxDataVolumes} device ids (starting from 1)
which are not reserved.
+ */
+ private List<Long> getAvailableDeviceIds(int maxDataVolumes, Set<Long>
reservedDeviceIds) {
+ List<Long> availableDeviceIds = new ArrayList<>();
+ for (long id = 1; availableDeviceIds.size() < maxDataVolumes; id++) {
+ if (!reservedDeviceIds.contains(id)) {
+ availableDeviceIds.add(id);
+ }
+ }
+ return availableDeviceIds;
+ }
+
private Long getDeviceId(UserVmVO vm, Long deviceId) {
// allocate deviceId
- int maxDevices = getMaxDataVolumesSupported(vm) + 2; // add 2 to
consider devices root volume and cdrom
- int maxDeviceId = maxDevices - 1;
+ Set<Long> reservedDeviceIds = getReservedDeviceIds(vm);
+ List<Long> availableDeviceIds =
getAvailableDeviceIds(getMaxDataVolumesSupported(vm), reservedDeviceIds);
+ long maxDeviceId = availableDeviceIds.get(availableDeviceIds.size() -
1);
List<VolumeVO> vols = _volsDao.findByInstance(vm.getId());
if (deviceId != null) {
- if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId
|| deviceId.longValue() == 3) {
- throw new RuntimeException("deviceId should be 0,1,2,4-" +
maxDeviceId);
+ if (deviceId.longValue() < 0 || deviceId.longValue() > maxDeviceId
|| reservedDeviceIds.contains(deviceId)) {
+ throw new RuntimeException(String.format("deviceId should be 0
or between 1 and %d, except the ones reserved for CD-ROM/config drive devices:
%s",
+ maxDeviceId, reservedDeviceIds));
}
for (VolumeVO vol : vols) {
if (vol.getDeviceId().equals(deviceId)) {
@@ -5374,18 +5399,13 @@ public class VolumeApiServiceImpl extends ManagerBase
implements VolumeApiServic
}
} else {
// allocate deviceId here
- List<String> devIds = new ArrayList<String>();
- for (int i = 1; i <= maxDeviceId; i++) {
- devIds.add(String.valueOf(i));
- }
- devIds.remove("3");
for (VolumeVO vol : vols) {
- devIds.remove(vol.getDeviceId().toString().trim());
+ availableDeviceIds.remove(vol.getDeviceId());
}
- if (devIds.isEmpty()) {
+ if (availableDeviceIds.isEmpty()) {
throw new RuntimeException(String.format("All device Ids are
used by vm %s", vm));
}
- deviceId = Long.parseLong(devIds.iterator().next());
+ deviceId = availableDeviceIds.get(0);
}
return deviceId;
diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java
b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java
index 873afff01c8..482d0b405d7 100755
--- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java
+++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java
@@ -24,8 +24,10 @@ import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import java.util.UUID;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
@@ -155,6 +157,10 @@ import com.cloud.hypervisor.Hypervisor;
import com.cloud.hypervisor.Hypervisor.HypervisorType;
import com.cloud.hypervisor.HypervisorGuru;
import com.cloud.hypervisor.HypervisorGuruManager;
+import com.cloud.network.Network.Provider;
+import com.cloud.network.Network.Service;
+import com.cloud.network.NetworkModel;
+import com.cloud.network.element.ConfigDriveNetworkElement;
import com.cloud.projects.Project;
import com.cloud.projects.ProjectManager;
import com.cloud.resourcelimit.CheckedReservation;
@@ -219,6 +225,7 @@ import com.cloud.utils.db.Transaction;
import com.cloud.utils.db.TransactionCallbackNoReturn;
import com.cloud.utils.db.TransactionStatus;
import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.NicVO;
import com.cloud.vm.UserVmManager;
import com.cloud.vm.UserVmVO;
import com.cloud.vm.VMInstanceVO;
@@ -228,9 +235,11 @@ import com.cloud.vm.VirtualMachineProfile;
import com.cloud.vm.VirtualMachineProfileImpl;
import com.cloud.vm.VmDetailConstants;
import com.cloud.vm.VmIsoMapVO;
+import com.cloud.vm.dao.NicDao;
import com.cloud.vm.dao.UserVmDao;
import com.cloud.vm.dao.VmIsoMapDao;
import com.cloud.vm.dao.VMInstanceDao;
+import com.cloud.vm.dao.VMInstanceDetailsDao;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
@@ -265,6 +274,12 @@ public class TemplateManagerImpl extends ManagerBase
implements TemplateManager,
@Inject
private UserVmDao _userVmDao;
@Inject
+ private NicDao _nicDao;
+ @Inject
+ private NetworkModel _networkModel;
+ @Inject
+ private VMInstanceDetailsDao _vmInstanceDetailsDao;
+ @Inject
private VmIsoMapDao _vmIsoMapDao;
@Inject
private VolumeDao _volumeDao;
@@ -705,15 +720,87 @@ public class TemplateManagerImpl extends ManagerBase
implements TemplateManager,
// Pre-allocate every cdrom slot at boot. QEMU/IDE refuses to hot-add
new cdrom drives, so
// runtime attachIso can only media-swap into a slot the domain
already owns.
int totalSlots = Math.max(effectiveMaxCdroms(vm,
dest.getHost().getId()), slotsNeededFor(slotToIsoId));
+ boolean configDriveUsed = isConfigDriveUsed(vm);
for (int i = 0; i < totalSlots; i++) {
- int diskSeq = CDROM_PRIMARY_DEVICE_SEQ + i;
- Long isoId = slotToIsoId.get(diskSeq);
+ int slot = CDROM_PRIMARY_DEVICE_SEQ + i;
+ int diskSeq = getIsoDeviceSeq(slot, configDriveUsed);
+ Long isoId = slotToIsoId.get(slot);
profile.addDisk(isoId != null
? buildIsoDisk(profile, vm, dest, poolId, diskSeq, isoId)
: buildEmptyCdromDisk(diskSeq));
}
}
+ @Override
+ public Set<Long> getCdromDeviceIds(VirtualMachine vm) {
+ if (vm.getType() != VirtualMachine.Type.User) {
+ return getCdromDeviceIds(1);
+ }
+ return getCdromDeviceIds(vm, isConfigDriveUsed(vm));
+ }
+
+ @Override
+ public Set<Long> getCdromDeviceIds(VirtualMachine vm, boolean
configDriveUsed) {
+ return getCdromDeviceIds(getCdromCount(vm, configDriveUsed));
+ }
+
+ private Set<Long> getCdromDeviceIds(int cdromCount) {
+ Set<Long> deviceIds = new HashSet<>();
+ for (int i = 0; i < cdromCount; i++) {
+ deviceIds.add((long) CDROM_PRIMARY_DEVICE_SEQ + i);
+ }
+ return deviceIds;
+ }
+
+ private int getCdromCount(VirtualMachine vm, boolean configDriveUsed) {
+ if (vm.getType() != VirtualMachine.Type.User) {
+ return 1;
+ }
+ // all the CD-ROM devices supported by the host are defined for the
Instance at boot, even if the configured
+ // maximum number of ISOs (vm.iso.max.count) is lower or is increased
later
+ int cdromCount = advertisedCdromCap(hostIdForVm(vm));
+ UserVmVO userVm = _userVmDao.findById(vm.getId());
+ if (userVm != null) {
+ cdromCount = Math.max(cdromCount,
slotsNeededFor(loadAttachedIsoSlots(userVm)));
+ }
+ // the config drive ISO is one more CD-ROM, it is not counted in the
CD-ROMs supported for the ISOs
+ return cdromCount + (configDriveUsed ? 1 : 0);
+ }
+
+ /**
+ * Returns the device id of the CD-ROM of an ISO slot (the slots are the
ones stored for the ISOs of the Instance,
+ * starting from {@link #CDROM_PRIMARY_DEVICE_SEQ}). The config drive ISO
always has the device id
+ * {@link ConfigDriveNetworkElement#CONFIGDRIVEDISKSEQ}, so the slots from
it are moved to the next device ids
+ * when the Instance uses config drive.
+ */
+ int getIsoDeviceSeq(int slot, boolean configDriveUsed) {
+ return configDriveUsed && slot >=
ConfigDriveNetworkElement.CONFIGDRIVEDISKSEQ ? slot + 1 : slot;
+ }
+
+ /**
+ * Checks if a config drive ISO is (or will be) attached to the Instance,
which is the case when any of its networks
+ * has ConfigDrive as the provider of the Dhcp, Dns or UserData service.
Checking the network providers (instead of
+ * only the config drive location detail, which is added when the ISO is
created) also covers Instances which have
+ * never been started.
+ */
+ boolean isConfigDriveUsed(VirtualMachine vm) {
+ if (_vmInstanceDetailsDao.findDetail(vm.getId(),
VmDetailConstants.CONFIG_DRIVE_LOCATION) != null) {
+ return true;
+ }
+ List<NicVO> nics = _nicDao.listByVmId(vm.getId());
+ if (CollectionUtils.isEmpty(nics)) {
+ return false;
+ }
+ for (NicVO nic : nics) {
+ for (Service service : Arrays.asList(Service.UserData,
Service.Dhcp, Service.Dns)) {
+ if
(_networkModel.isProviderSupportServiceInNetwork(nic.getNetworkId(), service,
Provider.ConfigDrive)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
private Long singleStoragePoolId(DeployDestination dest) {
Long poolId = null;
Map<Volume, StoragePool> storageForDisks = dest.getStorageForDisks();
@@ -1451,7 +1538,8 @@ public class TemplateManagerImpl extends ManagerBase
implements TemplateManager,
}
DataTO isoTO = tmplt.getTO();
- DiskTO disk = new DiskTO(isoTO, (long) deviceSeq, null,
Volume.Type.ISO);
+ int diskSeq = !isVirtualRouter ? getIsoDeviceSeq(deviceSeq,
isConfigDriveUsed(vm)) : deviceSeq;
+ DiskTO disk = new DiskTO(isoTO, (long) diskSeq, null, Volume.Type.ISO);
HypervisorGuru hvGuru = _hvGuruMgr.getGuru(vm.getHypervisorType());
VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm);
diff --git
a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
index 233c67d690f..c0e17a064a3 100644
--- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
+++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
@@ -940,6 +940,21 @@ public class UnmanagedVMsManagerImpl implements
UnmanagedVMsManager {
return new Pair<DiskProfile, StoragePool>(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<Long> getReservedDeviceIds(VirtualMachine vm) {
+ return volumeApiService.getReservedDeviceIds(vm, true);
+ }
+
+ private long getNextDeviceId(long deviceId, Set<Long> 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<NicProfile, Integer> result =
networkOrchestrationService.importNic(nic.getMacAddress(), deviceId, network,
isDefaultNic, vm, ipAddresses, dataCenterVO, forced);
@@ -1269,7 +1284,8 @@ public class UnmanagedVMsManagerImpl implements
UnmanagedVMsManager {
DiskOfferingVO diskOffering =
diskOfferingDao.findById(serviceOffering.getDiskOfferingId());
diskProfileStoragePoolList.add(importDisk(rootDisk, userVm,
cluster, diskOffering, Volume.Type.ROOT, String.format("ROOT-%d",
userVm.getId()),
rootDisk.getCapacity(), minIops, maxIops, template,
owner, null, storagePoolId));
- long deviceId = 1L;
+ Set<Long> 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()));
@@ -1278,7 +1294,7 @@ public class UnmanagedVMsManagerImpl implements
UnmanagedVMsManager {
diskProfileStoragePoolList.add(importDisk(disk, userVm,
cluster, offering, Volume.Type.DATADISK, String.format("DATA-%d-%s",
userVm.getId(), disk.getDiskId()),
disk.getCapacity(), offering.getMinIops(),
offering.getMaxIops(),
template, owner, deviceId, storagePoolId));
- deviceId++;
+ deviceId = getNextDeviceId(deviceId, reservedDeviceIds);
}
} catch (Exception e) {
logger.error(String.format("Failed to import volumes while
importing vm: %s", displayName), e);
@@ -3439,7 +3455,8 @@ public class UnmanagedVMsManagerImpl implements
UnmanagedVMsManager {
diskProfileStoragePoolList.add(importExternalDisk(rootDisk,
userVm, dest, diskOffering, Volume.Type.ROOT,
template, null, remoteUrl, username, password,
tmpPath, diskProfile));
- long deviceId = 1L;
+ Set<Long> reservedDeviceIds = getReservedDeviceIds(userVm);
+ long deviceId = getNextDeviceId(0L, reservedDeviceIds);
diskSeq = 0;
for (UnmanagedInstanceTO.Disk disk : dataDisks) {
DiskProfile dataDiskProfile = dataDiskProfiles[diskSeq++];
@@ -3447,7 +3464,7 @@ public class UnmanagedVMsManagerImpl implements
UnmanagedVMsManager {
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 9e85271c388..e42acd90e88 100644
--- a/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java
+++ b/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java
@@ -20,7 +20,9 @@ import java.io.UnsupportedEncodingException;
import java.util.Arrays;
import java.util.List;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Map;
+import java.util.Set;
import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.backup.Backup;
@@ -47,6 +49,7 @@ import com.cloud.configuration.ConfigurationManagerImpl;
import com.cloud.event.UsageEventUtils;
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;
@@ -56,6 +59,7 @@ import com.cloud.service.dao.ServiceOfferingDetailsDao;
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.VolumeDao;
import com.cloud.storage.dao.GuestOSDao;
@@ -113,6 +117,12 @@ public class KVMGuruTest {
@Mock
DpdkHelper dpdkHelperMock;
+ @Mock
+ HypervisorCapabilitiesDao hypervisorCapabilitiesDaoMock;
+
+ @Mock
+ VolumeApiService volumeApiServiceMock;
+
@Mock
GuestOSVO guestOsVoMock;
@@ -519,6 +529,44 @@ public class KVMGuruTest {
Assert.assertNull(actualQuota);
}
+ private List<VolumeVO> mockVolumesWithDeviceIds(long... deviceIds) {
+ List<VolumeVO> 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<VolumeVO> 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<Long> 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<VolumeVO> 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<VolumeVO> volumes = mockVolumesWithDeviceIds(0L, 1L, 2L, 5L, 6L,
7L, 8L);
+
+ guru.getNextAvailableDeviceId(new HashSet<>(Arrays.asList(3L, 4L)),
volumes);
+ }
+
/**
* The restore maps backup files onto volumes by their device id ordering,
so re-importing the
* instance has to put every data disk back on the device id recorded in
the backup. Handing out
@@ -552,6 +600,43 @@ public class KVMGuruTest {
Mockito.verify(_volumeDao).attachVolume(10L, 42L, 0L);
Mockito.verify(_volumeDao).attachVolume(11L, 42L, 5L);
- Mockito.verify(guru,
Mockito.never()).getNextAvailableDeviceId(Mockito.anyList());
+ Mockito.verify(guru,
Mockito.never()).getNextAvailableDeviceId(Mockito.anySet(), Mockito.anyList());
+ }
+
+ @Test
+ public void
testImportVirtualMachineFromBackupDoesNotReinstateReservedDeviceIds() {
+ String vmInternalName = "i-2-42-VM";
+ VMInstanceVO vmInstance = Mockito.mock(VMInstanceVO.class);
+ Mockito.when(vmInstance.getId()).thenReturn(42L);
+ Mockito.when(vmInstance.getRemoved()).thenReturn(null);
+
Mockito.when(_instanceDao.findVMByInstanceNameIncludingRemoved(vmInternalName)).thenReturn(vmInstance);
+ // the CD-ROM and the config drive ISO
+ Mockito.when(volumeApiServiceMock.getReservedDeviceIds(vmInstance,
true)).thenReturn(new HashSet<>(Arrays.asList(3L, 4L)));
+
Mockito.when(hypervisorCapabilitiesDaoMock.getMaxDataVolumesLimit(Hypervisor.HypervisorType.KVM,
"default")).thenReturn(6);
+ List<VolumeVO> currentVolumes = mockVolumesWithDeviceIds(0L, 1L, 2L);
+
Mockito.when(_volumeDao.findByInstance(42L)).thenReturn(currentVolumes);
+
+ Backup.VolumeInfo rootVolume = new Backup.VolumeInfo("root-uuid",
"root-uuid", Volume.Type.ROOT, 1L, 0L, null, null, null);
+ // taken when the config drive device id was not reserved yet
+ Backup.VolumeInfo dataVolume = new Backup.VolumeInfo("data-uuid",
"data-uuid", Volume.Type.DATADISK, 1L, 4L, null, null, null);
+
Mockito.when(vmInstance.getBackupVolumeList()).thenReturn(List.of(rootVolume,
dataVolume));
+
+ VolumeVO rootVolumeVO = Mockito.mock(VolumeVO.class);
+ Mockito.when(rootVolumeVO.getId()).thenReturn(10L);
+ VolumeVO dataVolumeVO = Mockito.mock(VolumeVO.class);
+ Mockito.when(dataVolumeVO.getId()).thenReturn(11L);
+
Mockito.when(_volumeDao.findByUuidIncludingRemoved("root-uuid")).thenReturn(rootVolumeVO);
+
Mockito.when(_volumeDao.findByUuidIncludingRemoved("data-uuid")).thenReturn(dataVolumeVO);
+
Mockito.when(backupProviderMock.getName()).thenReturn("backupProviderMock");
+
+ Backup backup = Mockito.mock(Backup.class);
+
+ try (MockedStatic<UsageEventUtils> ignored =
Mockito.mockStatic(UsageEventUtils.class)) {
+ guru.importVirtualMachineFromBackup(1L, 1L, 1L, 1L,
vmInternalName, backup, backupProviderMock);
+ }
+
+ Mockito.verify(_volumeDao).attachVolume(10L, 42L, 0L);
+ // device id 4 is reserved, so the volume gets the next free one
+ Mockito.verify(_volumeDao).attachVolume(11L, 42L, 5L);
}
}
diff --git
a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
index 12258e2cc16..334bb3f659f 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.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
+import java.util.HashSet;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.ExecutionException;
@@ -128,6 +129,7 @@ import com.cloud.storage.dao.StoragePoolTagsDao;
import com.cloud.storage.dao.VMTemplateDao;
import com.cloud.storage.dao.VolumeDao;
import com.cloud.storage.snapshot.SnapshotManager;
+import com.cloud.template.TemplateManager;
import com.cloud.user.Account;
import com.cloud.user.AccountManager;
import com.cloud.user.AccountVO;
@@ -164,6 +166,8 @@ public class VolumeApiServiceImplTest {
@Mock
private VolumeDataStoreDao volumeDataStoreDaoMock;
@Mock
+ private TemplateManager templateManagerMock;
+ @Mock
private VolumeDao volumeDaoMock;
@Mock
private BackupDao backupDaoMock;
@@ -2896,4 +2900,114 @@ public class VolumeApiServiceImplTest {
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<VolumeVO> mockVmVolumes(UserVmVO vm, long... deviceIds) {
+ List<VolumeVO> volumes = new ArrayList<>();
+ for (long deviceId : deviceIds) {
+ VolumeVO volume = Mockito.mock(VolumeVO.class);
+ lenient().when(volume.getDeviceId()).thenReturn(deviceId);
+ volumes.add(volume);
+ }
+
lenient().when(volumeDaoMock.findByInstance(vm.getId())).thenReturn(volumes);
+ return volumes;
+ }
+
+ @Test
+ public void getReservedDeviceIdsReturnsTheDeviceIdsOfTheCdroms() {
+ UserVmVO vm = getMockedVm();
+
+ mockCdromDeviceIds(vm, 3L);
+ Assert.assertEquals(new HashSet<>(Arrays.asList(3L)),
volumeApiServiceImpl.getReservedDeviceIds(vm));
+
+ // e.g. the CD-ROM and the config drive ISO, which is attached as the
CD-ROM following the ones of the ISOs
+ mockCdromDeviceIds(vm, 3L, 4L);
+ Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)),
volumeApiServiceImpl.getReservedDeviceIds(vm));
+
+ mockCdromDeviceIds(vm, 3L, 4L, 5L);
+ Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)),
volumeApiServiceImpl.getReservedDeviceIds(vm));
+ }
+
+ @Test
+ public void getDeviceIdUsesDeviceId4WhenVmHasOneCdrom() throws Exception {
+ UserVmVO vm = getMockedVm();
+ mockCdromDeviceIds(vm, 3L);
+ mockVmVolumes(vm, 0L, 1L, 2L);
+
+ Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, null));
+ Assert.assertEquals(Long.valueOf(4L), invokeGetDeviceId(vm, 4L));
+ }
+
+ @Test
+ public void getDeviceIdSkipsDeviceIdsOfAllCdroms() throws Exception {
+ UserVmVO vm = getMockedVm();
+ mockCdromDeviceIds(vm, 3L, 4L);
+ mockVmVolumes(vm, 0L, 1L, 2L);
+
+ Assert.assertEquals(Long.valueOf(5L), invokeGetDeviceId(vm, null));
+ }
+
+ @Test
+ public void getDeviceIdRejectsReservedDeviceIds() throws Exception {
+ UserVmVO vm = getMockedVm();
+ mockCdromDeviceIds(vm, 3L, 4L);
+ mockVmVolumes(vm, 0L);
+
+ for (long reservedDeviceId : new long[] {3L, 4L}) {
+ try {
+ invokeGetDeviceId(vm, reservedDeviceId);
+ fail("deviceId " + reservedDeviceId + " should be rejected");
+ } catch (RuntimeException e) {
+ Assert.assertTrue(e.getMessage(),
e.getMessage().contains("deviceId should be 0 or between 1 and"));
+ }
+ }
+ }
+
+ @Test
+ public void getDeviceIdKeepsMaxDataVolumesWhenDeviceIdsAreReserved()
throws Exception {
+ UserVmVO vm = getMockedVm();
+ mockCdromDeviceIds(vm, 3L, 4L);
+ List<VolumeVO> volumes = mockVmVolumes(vm, 0L);
+
+ // 6 data volumes are supported by default
+ List<Long> allocated = new ArrayList<>();
+ for (int i = 0; i < 6; i++) {
+ Long deviceId = invokeGetDeviceId(vm, null);
+ allocated.add(deviceId);
+ VolumeVO volume = Mockito.mock(VolumeVO.class);
+ lenient().when(volume.getDeviceId()).thenReturn(deviceId);
+ volumes.add(volume);
+ }
+
+ Assert.assertEquals(Arrays.asList(1L, 2L, 5L, 6L, 7L, 8L), allocated);
+ try {
+ invokeGetDeviceId(vm, null);
+ fail("All device ids are used, an exception is expected");
+ } catch (RuntimeException e) {
+ Assert.assertTrue(e.getMessage(), e.getMessage().contains("All
device Ids are used"));
+ }
+ }
+
+ @Test
+ public void
getReservedDeviceIdsWithConfigDriveReservesTheCdromOfConfigDrive() {
+ UserVmVO vm = getMockedVm();
+ when(templateManagerMock.getCdromDeviceIds(vm, false)).thenReturn(new
HashSet<>(Arrays.asList(3L, 4L)));
+ when(templateManagerMock.getCdromDeviceIds(vm, true)).thenReturn(new
HashSet<>(Arrays.asList(3L, 4L, 5L)));
+
+ Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L)),
volumeApiServiceImpl.getReservedDeviceIds(vm, false));
+ Assert.assertEquals(new HashSet<>(Arrays.asList(3L, 4L, 5L)),
volumeApiServiceImpl.getReservedDeviceIds(vm, true));
+ }
}
diff --git
a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java
b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java
index 365f66e7411..61407a9d4c2 100755
--- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java
+++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java
@@ -65,10 +65,12 @@ import
org.springframework.core.type.classreading.MetadataReaderFactory;
import org.springframework.core.type.filter.TypeFilter;
import com.cloud.agent.AgentManager;
+import com.cloud.agent.api.to.DiskTO;
import com.cloud.api.query.dao.SnapshotJoinDao;
import com.cloud.api.query.dao.UserVmJoinDao;
import com.cloud.api.query.vo.UserVmJoinVO;
import com.cloud.dc.dao.DataCenterDao;
+import com.cloud.deploy.DeployDestination;
import com.cloud.deployasis.dao.TemplateDeployAsIsDetailsDao;
import com.cloud.domain.dao.DomainDao;
import com.cloud.event.dao.UsageEventDao;
@@ -82,6 +84,9 @@ import com.cloud.host.dao.HostDao;
import com.cloud.host.dao.HostDetailsDao;
import com.cloud.hypervisor.Hypervisor;
import com.cloud.hypervisor.HypervisorGuruManager;
+import com.cloud.network.Network.Provider;
+import com.cloud.network.Network.Service;
+import com.cloud.network.NetworkModel;
import com.cloud.projects.ProjectManager;
import com.cloud.resourcelimit.CheckedReservation;
import com.cloud.storage.DataStoreRole;
@@ -120,13 +125,19 @@ import com.cloud.utils.UriUtils;
import com.cloud.utils.concurrency.NamedThreadFactory;
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.uservm.UserVm;
+import com.cloud.vm.NicVO;
import com.cloud.vm.UserVmVO;
+import com.cloud.vm.VMInstanceDetailVO;
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.VirtualMachineProfile;
import com.cloud.vm.VirtualMachine.State;
+import com.cloud.vm.VmDetailConstants;
import com.cloud.vm.VmIsoMapVO;
+import com.cloud.vm.dao.NicDao;
import com.cloud.vm.dao.UserVmDao;
import com.cloud.vm.dao.VMInstanceDao;
+import com.cloud.vm.dao.VMInstanceDetailsDao;
import com.cloud.vm.dao.VmIsoMapDao;
import junit.framework.TestCase;
@@ -249,6 +260,12 @@ public class TemplateManagerImplTest extends TestCase {
@Mock
HostDetailsDao _hostDetailsDao;
+ @Mock
+ NicDao _nicDao;
+ @Mock
+ NetworkModel _networkModel;
+ @Mock
+ VMInstanceDetailsDao _vmInstanceDetailsDao;
@Mock
UserVmJoinDao _userVmJoinDao;
@@ -1312,4 +1329,162 @@ public class TemplateManagerImplTest extends TestCase {
}
}
}
+
+ private java.util.Set<Long> ids(Long... deviceIds) {
+ return new java.util.HashSet<>(Arrays.asList(deviceIds));
+ }
+
+ private UserVmVO mockUserVmOnHostWithCdromCap(Long advertisedCap) {
+ UserVmVO vm = Mockito.mock(UserVmVO.class);
+ Mockito.when(vm.getId()).thenReturn(1L);
+ Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.User);
+ Mockito.when(vm.getHostId()).thenReturn(7L);
+ Mockito.when(_userVmDao.findById(1L)).thenReturn(vm);
+ if (advertisedCap != null) {
+ DetailVO detail = Mockito.mock(DetailVO.class);
+
Mockito.when(detail.getValue()).thenReturn(String.valueOf(advertisedCap));
+ Mockito.when(_hostDetailsDao.findDetail(7L,
Host.HOST_CDROM_MAX_COUNT)).thenReturn(detail);
+ }
+ return vm;
+ }
+
+ @Test
+ public void getCdromDeviceIdsReturnsOnlyThePrimaryCdromForSystemVms() {
+ VirtualMachine vm = Mockito.mock(VirtualMachine.class);
+
Mockito.when(vm.getType()).thenReturn(VirtualMachine.Type.SecondaryStorageVm);
+
+ Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm));
+ }
+
+ @Test
+ public void
getCdromDeviceIdsReturnsAllCdromsAdvertisedByHostRegardlessOfConfiguredMaximum()
{
+ // vm.iso.max.count defaults to 1, but the host defines all the
CD-ROMs it supports
+ UserVmVO vm = mockUserVmOnHostWithCdromCap(2L);
+
+ Assert.assertEquals(ids(3L, 4L),
templateManager.getCdromDeviceIds(vm));
+ }
+
+ @Test
+ public void
getCdromDeviceIdsReturnsDefaultWhenHostDoesNotAdvertiseCdromCap() {
+ UserVmVO vm = mockUserVmOnHostWithCdromCap(null);
+
+ Assert.assertEquals(ids(3L), templateManager.getCdromDeviceIds(vm));
+ }
+
+ @Test
+ public void getCdromDeviceIdsCoversAttachedIsosBeyondHostCap() {
+ UserVmVO vm = mockUserVmOnHostWithCdromCap(2L);
+ Mockito.when(_vmIsoMapDao.listByVmId(1L)).thenReturn(Arrays.asList(
+ new VmIsoMapVO(1L, 11L,
TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 1),
+ new VmIsoMapVO(1L, 12L,
TemplateManager.CDROM_PRIMARY_DEVICE_SEQ + 2)));
+
+ Assert.assertEquals(ids(3L, 4L, 5L),
templateManager.getCdromDeviceIds(vm));
+ }
+
+ private void mockVmNetworkProvidesConfigDrive(UserVmVO vm, boolean
providesConfigDrive) {
+ NicVO nic = Mockito.mock(NicVO.class);
+ Mockito.when(nic.getNetworkId()).thenReturn(100L);
+
Mockito.when(_nicDao.listByVmId(vm.getId())).thenReturn(java.util.Collections.singletonList(nic));
+
Mockito.lenient().when(_networkModel.isProviderSupportServiceInNetwork(Mockito.eq(100L),
Mockito.any(Service.class),
Mockito.eq(Provider.ConfigDrive))).thenReturn(providesConfigDrive);
+ }
+
+ private void withConfiguredMaxCdroms(String value, Runnable test) {
+ Object original =
org.springframework.test.util.ReflectionTestUtils.getField(TemplateManager.VmIsoMaxCount,
"_defaultValue");
+
org.springframework.test.util.ReflectionTestUtils.setField(TemplateManager.VmIsoMaxCount,
"_defaultValue", value);
+ try {
+ test.run();
+ } finally {
+
org.springframework.test.util.ReflectionTestUtils.setField(TemplateManager.VmIsoMaxCount,
"_defaultValue", original);
+ }
+ }
+
+ @Test
+ public void
isConfigDriveUsedReturnsTrueWhenNetworkProvidesConfigDriveBeforeFirstStart() {
+ UserVmVO vm = mockUserVmOnHostWithCdromCap(null);
+ mockVmNetworkProvidesConfigDrive(vm, true);
+
+ Assert.assertTrue(templateManager.isConfigDriveUsed(vm));
+ }
+
+ @Test
+ public void
isConfigDriveUsedReturnsTrueWhenConfigDriveLocationDetailExists() {
+ UserVmVO vm = mockUserVmOnHostWithCdromCap(null);
+ Mockito.when(_vmInstanceDetailsDao.findDetail(1L,
VmDetailConstants.CONFIG_DRIVE_LOCATION)).thenReturn(Mockito.mock(VMInstanceDetailVO.class));
+
+ Assert.assertTrue(templateManager.isConfigDriveUsed(vm));
+ }
+
+ @Test
+ public void
isConfigDriveUsedReturnsFalseWhenNoNetworkProvidesConfigDrive() {
+ UserVmVO vm = mockUserVmOnHostWithCdromCap(null);
+ mockVmNetworkProvidesConfigDrive(vm, false);
+
+ Assert.assertFalse(templateManager.isConfigDriveUsed(vm));
+ }
+
+ @Test
+ public void getCdromDeviceIdsAddsTheCdromOfConfigDrive() {
+ UserVmVO vm = mockUserVmOnHostWithCdromCap(2L);
+ mockVmNetworkProvidesConfigDrive(vm, true);
+
+ // the config drive ISO is one more CD-ROM than the ones supported for
the ISOs
+ Assert.assertEquals(ids(3L, 4L, 5L),
templateManager.getCdromDeviceIds(vm));
+ Assert.assertEquals(ids(3L, 4L), templateManager.getCdromDeviceIds(vm,
false));
+ Assert.assertEquals(ids(3L, 4L, 5L),
templateManager.getCdromDeviceIds(vm, true));
+ }
+
+ @Test
+ public void getIsoDeviceSeqKeepsTheDeviceIdOfConfigDrive() {
+ // without config drive, the CD-ROMs of the ISOs are the ones of the
slots
+ Assert.assertEquals(3, templateManager.getIsoDeviceSeq(3, false));
+ Assert.assertEquals(4, templateManager.getIsoDeviceSeq(4, false));
+ Assert.assertEquals(5, templateManager.getIsoDeviceSeq(5, false));
+ // with config drive (device id 4), the slots from it are moved to the
next device ids
+ Assert.assertEquals(3, templateManager.getIsoDeviceSeq(3, true));
+ Assert.assertEquals(5, templateManager.getIsoDeviceSeq(4, true));
+ Assert.assertEquals(6, templateManager.getIsoDeviceSeq(5, true));
+ }
+
+ private java.util.List<DiskTO> prepareIsoDisks(UserVmVO vm) {
+ // no ISO is attached to the Instance
+ Mockito.when(vm.getIsoId()).thenReturn(null);
+ VirtualMachineProfile profile =
Mockito.mock(VirtualMachineProfile.class);
+ Mockito.when(profile.getId()).thenReturn(1L);
+ DeployDestination dest = Mockito.mock(DeployDestination.class);
+ Host host = Mockito.mock(Host.class);
+ Mockito.when(host.getId()).thenReturn(7L);
+ Mockito.when(dest.getHost()).thenReturn(host);
+
Mockito.when(_hostDao.findById(7L)).thenReturn(Mockito.mock(HostVO.class));
+
+ templateManager.prepareIsoForVmProfile(profile, dest);
+
+ org.mockito.ArgumentCaptor<DiskTO> disks =
org.mockito.ArgumentCaptor.forClass(DiskTO.class);
+ Mockito.verify(profile, Mockito.atLeast(0)).addDisk(disks.capture());
+ return disks.getAllValues();
+ }
+
+ @Test
+ public void prepareIsoForVmProfileSkipsTheDeviceIdOfConfigDrive() {
+ UserVmVO vm = mockUserVmOnHostWithCdromCap(2L);
+ mockVmNetworkProvidesConfigDrive(vm, true);
+
+ withConfiguredMaxCdroms("2", () -> {
+ java.util.List<DiskTO> disks = prepareIsoDisks(vm);
+
+ // the config drive ISO has the device id 4 (it is added by the
config drive element), so the second ISO is at 5
+ Assert.assertEquals(Arrays.asList(3L, 5L),
disks.stream().map(DiskTO::getDiskSeq).collect(java.util.stream.Collectors.toList()));
+ });
+ }
+
+ @Test
+ public void
prepareIsoForVmProfileUsesConsecutiveDeviceIdsWithoutConfigDrive() {
+ UserVmVO vm = mockUserVmOnHostWithCdromCap(2L);
+ mockVmNetworkProvidesConfigDrive(vm, false);
+
+ withConfiguredMaxCdroms("2", () -> {
+ java.util.List<DiskTO> disks = prepareIsoDisks(vm);
+
+ Assert.assertEquals(Arrays.asList(3L, 4L),
disks.stream().map(DiskTO::getDiskSeq).collect(java.util.stream.Collectors.toList()));
+ });
+ }
}
diff --git
a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java
b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java
index 0e8c67d5127..c5267033dfb 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.ArrayList;
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;
@@ -81,6 +83,7 @@ import org.mockito.Mockito;
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;
@@ -2094,4 +2097,25 @@ public class UnmanagedVMsManagerImplTest {
Map.of("Network adapter 1", java.util.List.of("10.1.1.151")));
Assert.assertEquals("10.1.1.151", result.get("Network adapter
1").getIp4Address());
}
+
+ @Test
+ public void getReservedDeviceIdsAlwaysReservesTheCdromOfConfigDrive() {
+ VirtualMachine vm = Mockito.mock(VirtualMachine.class);
+ // the networks of the Instance are not imported yet, so the config
drive is always assumed
+ Mockito.when(volumeApiService.getReservedDeviceIds(vm,
true)).thenReturn(new HashSet<>(java.util.Arrays.asList(3L, 4L)));
+
+ Set<Long> reserved =
ReflectionTestUtils.invokeMethod(unmanagedVMsManager, "getReservedDeviceIds",
vm);
+
+ Assert.assertEquals(new HashSet<>(java.util.Arrays.asList(3L, 4L)),
reserved);
+ }
+
+ @Test
+ public void getNextDeviceIdSkipsReservedDeviceIds() {
+ Set<Long> 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));
+ }
}
