Skip to content

Commit bac8122

Browse files
Damans227Damans227
committed
refactor ISO attachment logic and enhance UI for multi-CDROM management
1 parent 79e8edf commit bac8122

9 files changed

Lines changed: 167 additions & 157 deletions

File tree

‎engine/components-api/src/main/java/com/cloud/template/TemplateManager.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -71,11 +71,8 @@ public interface TemplateManager {
7171
true,
7272
ConfigKey.Scope.Global);
7373

74-
/**
75-
* Device sequence for the bootable / primary cdrom slot. user_vm.iso_id has always pointed at this
76-
* slot; the KVM agent's getDevLabel() maps it to hdc on the IDE bus. Any additional cdrom slots
77-
* (held in vm_iso_map) start at {@code CDROM_PRIMARY_DEVICE_SEQ + 1} (hdd, hde, ...).
78-
*/
74+
// KVM/libvirt maps deviceSeq=3 to hdc (hda/hdb are taken by the root volume on i440fx/IDE).
75+
// user_vm.iso_id has always pointed at this slot; additional cdroms live in vm_iso_map.
7976
int CDROM_PRIMARY_DEVICE_SEQ = 3;
8077

8178
static final String VMWARE_TOOLS_ISO = "vmware-tools.iso";

‎server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import org.apache.cloudstack.api.ApiConstants;
4040
import org.apache.cloudstack.api.ApiConstants.VMDetails;
4141
import org.apache.cloudstack.api.ResponseObject.ResponseView;
42+
import org.apache.cloudstack.api.response.AttachedIsoResponse;
4243
import org.apache.cloudstack.api.response.NicExtraDhcpOptionResponse;
4344
import org.apache.cloudstack.api.response.NicResponse;
4445
import org.apache.cloudstack.api.response.NicSecondaryIpResponse;
@@ -71,6 +72,7 @@
7172
import com.cloud.storage.Storage.TemplateType;
7273
import com.cloud.storage.VMTemplateVO;
7374
import com.cloud.storage.VnfTemplateDetailVO;
75+
import com.cloud.template.TemplateManager;
7476
import com.cloud.storage.VnfTemplateNicVO;
7577
import com.cloud.storage.Volume;
7678
import com.cloud.storage.dao.VMTemplateDao;
@@ -91,10 +93,12 @@
9193
import com.cloud.vm.VMInstanceDetailVO;
9294
import com.cloud.vm.VirtualMachine;
9395
import com.cloud.vm.VirtualMachine.State;
96+
import com.cloud.vm.VmIsoMapVO;
9497
import com.cloud.vm.VmStats;
9598
import com.cloud.vm.dao.NicExtraDhcpOptionDao;
9699
import com.cloud.vm.dao.NicSecondaryIpVO;
97100
import com.cloud.vm.dao.VMInstanceDetailsDao;
101+
import com.cloud.vm.dao.VmIsoMapDao;
98102

99103
@Component
100104
public class UserVmJoinDaoImpl extends GenericDaoBaseWithTagInformation<UserVmJoinVO, UserVmResponse> implements UserVmJoinDao {
@@ -128,7 +132,7 @@ public class UserVmJoinDaoImpl extends GenericDaoBaseWithTagInformation<UserVmJo
128132
@Inject
129133
VMTemplateDao vmTemplateDao;
130134
@Inject
131-
com.cloud.vm.dao.VmIsoMapDao vmIsoMapDao;
135+
VmIsoMapDao vmIsoMapDao;
132136
@Inject
133137
ExtensionHelper extensionHelper;
134138

@@ -247,17 +251,16 @@ public UserVmResponse newUserVmResponse(ResponseView view, String objectName, Us
247251
userVmResponse.setIsoName(userVm.getIsoName());
248252
userVmResponse.setIsoDisplayText(userVm.getIsoDisplayText());
249253

250-
java.util.List<org.apache.cloudstack.api.response.AttachedIsoResponse> attachedIsos = new java.util.ArrayList<>();
254+
List<AttachedIsoResponse> attachedIsos = new ArrayList<>();
251255
if (userVm.getIsoUuid() != null) {
252-
attachedIsos.add(new org.apache.cloudstack.api.response.AttachedIsoResponse(
253-
userVm.getIsoUuid(), userVm.getIsoName(), userVm.getIsoDisplayText(),
254-
com.cloud.template.TemplateManager.CDROM_PRIMARY_DEVICE_SEQ));
256+
attachedIsos.add(new AttachedIsoResponse(userVm.getIsoUuid(), userVm.getIsoName(),
257+
userVm.getIsoDisplayText(), TemplateManager.CDROM_PRIMARY_DEVICE_SEQ));
255258
}
256-
for (com.cloud.vm.VmIsoMapVO row : vmIsoMapDao.listByVmId(userVm.getId())) {
257-
com.cloud.storage.VMTemplateVO tmpl = vmTemplateDao.findById(row.getIsoId());
259+
for (VmIsoMapVO row : vmIsoMapDao.listByVmId(userVm.getId())) {
260+
VMTemplateVO tmpl = vmTemplateDao.findById(row.getIsoId());
258261
if (tmpl != null) {
259-
attachedIsos.add(new org.apache.cloudstack.api.response.AttachedIsoResponse(
260-
tmpl.getUuid(), tmpl.getName(), tmpl.getDisplayText(), row.getDeviceSeq()));
262+
attachedIsos.add(new AttachedIsoResponse(tmpl.getUuid(), tmpl.getName(),
263+
tmpl.getDisplayText(), row.getDeviceSeq()));
261264
}
262265
}
263266
userVmResponse.setIsos(attachedIsos);

‎server/src/main/java/com/cloud/template/TemplateManagerImpl.java‎

Lines changed: 109 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.net.URL;
2222
import java.util.ArrayList;
2323
import java.util.Arrays;
24+
import java.util.Collections;
2425
import java.util.Date;
2526
import java.util.HashMap;
2627
import java.util.List;
@@ -683,54 +684,73 @@ private String extract(Account caller, Long templateId, String url, Long zoneId,
683684
@Override
684685
public void prepareIsoForVmProfile(VirtualMachineProfile profile, DeployDestination dest) {
685686
UserVmVO vm = _userVmDao.findById(profile.getId());
687+
Long poolId = singleStoragePoolId(dest);
688+
Map<Integer, Long> slotToIsoId = loadAttachedIsoSlots(vm);
689+
690+
// Pre-allocate every cdrom slot at boot. QEMU/IDE refuses to hot-add new cdrom drives, so
691+
// runtime attachIso can only media-swap into a slot the domain already owns.
692+
int totalSlots = Math.max(effectiveMaxCdroms(vm), slotsNeededFor(slotToIsoId));
693+
for (int i = 0; i < totalSlots; i++) {
694+
int diskSeq = CDROM_PRIMARY_DEVICE_SEQ + i;
695+
Long isoId = slotToIsoId.get(diskSeq);
696+
profile.addDisk(isoId != null
697+
? buildIsoDisk(profile, vm, dest, poolId, diskSeq, isoId)
698+
: buildEmptyCdromDisk(diskSeq));
699+
}
700+
}
686701

702+
private Long singleStoragePoolId(DeployDestination dest) {
687703
Long poolId = null;
688704
Map<Volume, StoragePool> storageForDisks = dest.getStorageForDisks();
689705
if (MapUtils.isNotEmpty(storageForDisks)) {
690-
for (StoragePool storagePool : storageForDisks.values()) {
691-
if (poolId != null && storagePool.getId() != poolId) {
706+
for (StoragePool pool : storageForDisks.values()) {
707+
if (poolId != null && pool.getId() != poolId) {
692708
throw new CloudRuntimeException("Cannot determine where to download ISO");
693709
}
694-
poolId = storagePool.getId();
710+
poolId = pool.getId();
695711
}
696712
}
713+
return poolId;
714+
}
697715

698-
Map<Integer, Long> slotToIsoId = new HashMap<>();
716+
private Map<Integer, Long> loadAttachedIsoSlots(UserVmVO vm) {
717+
Map<Integer, Long> slots = new HashMap<>();
699718
if (vm.getIsoId() != null) {
700-
slotToIsoId.put(CDROM_PRIMARY_DEVICE_SEQ, vm.getIsoId());
719+
slots.put(CDROM_PRIMARY_DEVICE_SEQ, vm.getIsoId());
701720
}
702721
for (VmIsoMapVO row : _vmIsoMapDao.listByVmId(vm.getId())) {
703-
slotToIsoId.put(row.getDeviceSeq(), row.getIsoId());
722+
slots.put(row.getDeviceSeq(), row.getIsoId());
704723
}
724+
return slots;
725+
}
705726

706-
// Empty ISO DiskTOs pre-allocate cdrom drives at boot so runtime attachIso can media-swap into them.
707-
int cap = effectiveMaxCdroms(vm);
708-
int neededForAttached = slotToIsoId.isEmpty() ? 0
709-
: slotToIsoId.keySet().stream().max(Integer::compare).get() - (CDROM_PRIMARY_DEVICE_SEQ - 1);
710-
int totalSlots = Math.max(cap, neededForAttached);
711-
for (int slot = 0; slot < totalSlots; slot++) {
712-
int diskSeq = CDROM_PRIMARY_DEVICE_SEQ + slot;
713-
Long isoId = slotToIsoId.get(diskSeq);
714-
if (isoId != null) {
715-
TemplateInfo template = prepareIso(isoId, vm.getDataCenterId(), dest.getHost().getId(), poolId);
716-
if (template == null) {
717-
logger.error("Failed to prepare ISO on secondary or cache storage");
718-
throw new CloudRuntimeException("Failed to prepare ISO on secondary or cache storage");
719-
}
720-
if (diskSeq == CDROM_PRIMARY_DEVICE_SEQ && template.isBootable()) {
721-
profile.setBootLoaderType(BootloaderType.CD);
722-
}
723-
GuestOSVO guestOS = _guestOSDao.findById(template.getGuestOSId());
724-
TemplateObjectTO iso = (TemplateObjectTO) template.getTO();
725-
iso.setDirectDownload(template.isDirectDownload());
726-
iso.setGuestOsType(guestOS != null ? guestOS.getDisplayName() : null);
727-
profile.addDisk(new DiskTO(iso, (long) diskSeq, null, Volume.Type.ISO));
728-
} else {
729-
TemplateObjectTO empty = new TemplateObjectTO();
730-
empty.setFormat(ImageFormat.ISO);
731-
profile.addDisk(new DiskTO(empty, (long) diskSeq, null, Volume.Type.ISO));
732-
}
727+
private int slotsNeededFor(Map<Integer, Long> slotToIsoId) {
728+
if (slotToIsoId.isEmpty()) {
729+
return 0;
733730
}
731+
return Collections.max(slotToIsoId.keySet()) - CDROM_PRIMARY_DEVICE_SEQ + 1;
732+
}
733+
734+
private DiskTO buildIsoDisk(VirtualMachineProfile profile, UserVmVO vm, DeployDestination dest, Long poolId, int diskSeq, long isoId) {
735+
TemplateInfo template = prepareIso(isoId, vm.getDataCenterId(), dest.getHost().getId(), poolId);
736+
if (template == null) {
737+
logger.error("Failed to prepare ISO on secondary or cache storage");
738+
throw new CloudRuntimeException("Failed to prepare ISO on secondary or cache storage");
739+
}
740+
if (diskSeq == CDROM_PRIMARY_DEVICE_SEQ && template.isBootable()) {
741+
profile.setBootLoaderType(BootloaderType.CD);
742+
}
743+
GuestOSVO guestOS = _guestOSDao.findById(template.getGuestOSId());
744+
TemplateObjectTO iso = (TemplateObjectTO) template.getTO();
745+
iso.setDirectDownload(template.isDirectDownload());
746+
iso.setGuestOsType(guestOS != null ? guestOS.getDisplayName() : null);
747+
return new DiskTO(iso, (long) diskSeq, null, Volume.Type.ISO);
748+
}
749+
750+
private DiskTO buildEmptyCdromDisk(int diskSeq) {
751+
TemplateObjectTO empty = new TemplateObjectTO();
752+
empty.setFormat(ImageFormat.ISO);
753+
return new DiskTO(empty, (long) diskSeq, null, Volume.Type.ISO);
734754
}
735755

736756
private void prepareTemplateInOneStoragePool(final VMTemplateVO template, final StoragePoolVO pool) {
@@ -1342,16 +1362,7 @@ public boolean attachIso(long isoId, long vmId, Boolean... extraParams) {
13421362
throw new InvalidParameterValueException("Cannot attach VMware tools drivers to incompatible hypervisor " + vm.getHypervisorType());
13431363
}
13441364
if (!isVirtualRouter) {
1345-
Long primaryIsoId = ((UserVm) vm).getIsoId();
1346-
if (isIsoAlreadyAttached(vmId, primaryIsoId, isoId)) {
1347-
throw new InvalidParameterValueException("The specified ISO is already attached to this Instance.");
1348-
}
1349-
int effectiveMax = effectiveMaxCdroms(vm);
1350-
int attached = (primaryIsoId != null ? 1 : 0) + _vmIsoMapDao.listByVmId(vmId).size();
1351-
if (attached >= effectiveMax) {
1352-
throw new InvalidParameterValueException(String.format(
1353-
"Instance has reached the maximum of %d attached CD-ROM(s); detach one before attaching another.", effectiveMax));
1354-
}
1365+
enforceCdromAttachLimits((UserVm) vm, isoId);
13551366
}
13561367
boolean result = attachISOToVM(vmId, userId, isoId, true, forced, isVirtualRouter);
13571368
if (result) {
@@ -1438,45 +1449,54 @@ boolean attachISOToVM(long vmId, long userId, long isoId, boolean attach, boolea
14381449
UserVmVO vm = _userVmDao.findById(vmId);
14391450
VMTemplateVO iso = _tmpltDao.findById(isoId);
14401451

1441-
int targetSlot;
1442-
VmIsoMapVO mapEntry = null;
1452+
int targetSlot = attach ? chooseAttachSlot(vm) : findAttachedSlot(vm, isoId);
1453+
boolean success = attachISOToVM(vmId, isoId, targetSlot, attach, forced, isVirtualRouter);
1454+
if (!success || isVirtualRouter) {
1455+
return success;
1456+
}
14431457
if (attach) {
1444-
VmIsoMapVO highestExtra = highestCdromMapEntry(vmId);
1445-
if (vm.getIsoId() == null) {
1446-
targetSlot = CDROM_PRIMARY_DEVICE_SEQ;
1447-
} else if (highestExtra == null) {
1448-
targetSlot = CDROM_PRIMARY_DEVICE_SEQ + 1;
1449-
} else {
1450-
targetSlot = highestExtra.getDeviceSeq() + 1;
1451-
}
1458+
persistIsoAttachment(vm, iso, targetSlot);
14521459
} else {
1453-
if (vm.getIsoId() != null && vm.getIsoId() == isoId) {
1454-
targetSlot = CDROM_PRIMARY_DEVICE_SEQ;
1455-
} else {
1456-
mapEntry = _vmIsoMapDao.findByVmIdIsoId(vmId, isoId);
1457-
targetSlot = mapEntry != null ? mapEntry.getDeviceSeq() : CDROM_PRIMARY_DEVICE_SEQ;
1458-
}
1460+
persistIsoDetachment(vm, isoId, targetSlot);
14591461
}
1462+
return success;
1463+
}
14601464

1461-
boolean success = attachISOToVM(vmId, isoId, targetSlot, attach, forced, isVirtualRouter);
1465+
private int chooseAttachSlot(UserVmVO vm) {
1466+
if (vm.getIsoId() == null) {
1467+
return CDROM_PRIMARY_DEVICE_SEQ;
1468+
}
1469+
VmIsoMapVO highest = highestCdromMapEntry(vm.getId());
1470+
return highest == null ? CDROM_PRIMARY_DEVICE_SEQ + 1 : highest.getDeviceSeq() + 1;
1471+
}
14621472

1463-
if (success && attach && !isVirtualRouter) {
1464-
if (targetSlot == CDROM_PRIMARY_DEVICE_SEQ) {
1465-
vm.setIsoId(iso.getId());
1466-
_userVmDao.update(vmId, vm);
1467-
} else {
1468-
_vmIsoMapDao.persist(new VmIsoMapVO(vmId, iso.getId(), targetSlot));
1469-
}
1473+
private int findAttachedSlot(UserVmVO vm, long isoId) {
1474+
if (vm.getIsoId() != null && vm.getIsoId() == isoId) {
1475+
return CDROM_PRIMARY_DEVICE_SEQ;
14701476
}
1471-
if (success && !attach && !isVirtualRouter) {
1472-
if (targetSlot == CDROM_PRIMARY_DEVICE_SEQ) {
1473-
vm.setIsoId(null);
1474-
_userVmDao.update(vmId, vm);
1475-
} else if (mapEntry != null) {
1476-
_vmIsoMapDao.remove(mapEntry.getId());
1477-
}
1477+
VmIsoMapVO entry = _vmIsoMapDao.findByVmIdIsoId(vm.getId(), isoId);
1478+
return entry != null ? entry.getDeviceSeq() : CDROM_PRIMARY_DEVICE_SEQ;
1479+
}
1480+
1481+
private void persistIsoAttachment(UserVmVO vm, VMTemplateVO iso, int slot) {
1482+
if (slot == CDROM_PRIMARY_DEVICE_SEQ) {
1483+
vm.setIsoId(iso.getId());
1484+
_userVmDao.update(vm.getId(), vm);
1485+
} else {
1486+
_vmIsoMapDao.persist(new VmIsoMapVO(vm.getId(), iso.getId(), slot));
1487+
}
1488+
}
1489+
1490+
private void persistIsoDetachment(UserVmVO vm, long isoId, int slot) {
1491+
if (slot == CDROM_PRIMARY_DEVICE_SEQ) {
1492+
vm.setIsoId(null);
1493+
_userVmDao.update(vm.getId(), vm);
1494+
return;
1495+
}
1496+
VmIsoMapVO entry = _vmIsoMapDao.findByVmIdIsoId(vm.getId(), isoId);
1497+
if (entry != null) {
1498+
_vmIsoMapDao.remove(entry.getId());
14781499
}
1479-
return success;
14801500
}
14811501

14821502
VmIsoMapVO highestCdromMapEntry(long vmId) {
@@ -1515,9 +1535,22 @@ boolean isIsoAlreadyAttached(long vmId, Long primaryIsoId, long isoId) {
15151535
return _vmIsoMapDao.findByVmIdIsoId(vmId, isoId) != null;
15161536
}
15171537

1538+
private void enforceCdromAttachLimits(UserVm vm, long isoId) {
1539+
Long primaryIsoId = vm.getIsoId();
1540+
if (isIsoAlreadyAttached(vm.getId(), primaryIsoId, isoId)) {
1541+
throw new InvalidParameterValueException("The specified ISO is already attached to this Instance.");
1542+
}
1543+
int effectiveMax = effectiveMaxCdroms(vm);
1544+
int attached = (primaryIsoId != null ? 1 : 0) + _vmIsoMapDao.listByVmId(vm.getId()).size();
1545+
if (attached >= effectiveMax) {
1546+
throw new InvalidParameterValueException(String.format(
1547+
"Instance has reached the maximum of %d attached CD-ROM(s); detach one before attaching another.", effectiveMax));
1548+
}
1549+
}
1550+
15181551
private int effectiveMaxCdroms(VirtualMachine vm) {
15191552
int globalCap = VmCdromMaxCount.value();
1520-
// i440fx/IDE: hda is root, our slot scheme puts cdroms at hdc/hdd → 2 max on KVM.
1553+
// hda is root on i440fx/IDE, leaving hdc/hdd available for cdroms.
15211554
int hypervisorCap = (vm.getHypervisorType() == HypervisorType.KVM) ? 2 : 1;
15221555
return Math.min(globalCap, hypervisorCap);
15231556
}

‎server/src/test/java/com/cloud/api/query/dao/UserVmJoinDaoImplTest.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,12 @@
1616
// under the License.
1717
package com.cloud.api.query.dao;
1818

19+
import static org.mockito.ArgumentMatchers.anyLong;
1920
import static org.mockito.ArgumentMatchers.nullable;
2021
import static org.mockito.MockitoAnnotations.openMocks;
2122

2223
import java.util.Arrays;
24+
import java.util.Collections;
2325
import java.util.EnumSet;
2426

2527
import com.cloud.storage.dao.VMTemplateDao;
@@ -52,6 +54,7 @@
5254
import com.cloud.utils.db.SearchBuilder;
5355
import com.cloud.utils.db.SearchCriteria;
5456
import com.cloud.vm.dao.VMInstanceDetailsDao;
57+
import com.cloud.vm.dao.VmIsoMapDao;
5558

5659
@RunWith(MockitoJUnitRunner.class)
5760
public class UserVmJoinDaoImplTest extends GenericDaoBaseWithTagInformationBaseTest<UserVmJoinVO, UserVmResponse> {
@@ -84,7 +87,7 @@ public class UserVmJoinDaoImplTest extends GenericDaoBaseWithTagInformationBaseT
8487
private VMTemplateDao vmTemplateDao;
8588

8689
@Mock
87-
private com.cloud.vm.dao.VmIsoMapDao vmIsoMapDao;
90+
private VmIsoMapDao vmIsoMapDao;
8891

8992
@Mock
9093
ExtensionHelper extensionHelper;
@@ -106,8 +109,7 @@ public class UserVmJoinDaoImplTest extends GenericDaoBaseWithTagInformationBaseT
106109
@Before
107110
public void setup() {
108111
closeable = openMocks(this);
109-
org.mockito.Mockito.lenient().when(vmIsoMapDao.listByVmId(org.mockito.ArgumentMatchers.anyLong()))
110-
.thenReturn(java.util.Collections.emptyList());
112+
Mockito.lenient().when(vmIsoMapDao.listByVmId(anyLong())).thenReturn(Collections.emptyList());
111113
prepareSetup();
112114
}
113115

0 commit comments

Comments
 (0)