Skip to content

Commit 52c36ca

Browse files
slavkapslavkap
authored
kvm: Fix deploy VM from ISOs with UEFI (#4773)
This PR fixes #4244 deploying of VMs from ISOs and from templates with UEFI boot type deploying of VMs from ISOs and from templates with UEFI boot type with volumes in RAW format
1 parent 7270ca7 commit 52c36ca

9 files changed

Lines changed: 326 additions & 37 deletions

File tree

‎core/src/main/java/org/apache/cloudstack/storage/command/DettachCommand.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919

2020
package org.apache.cloudstack.storage.command;
2121

22+
import java.util.Map;
23+
2224
import com.cloud.agent.api.to.DiskTO;
2325

2426
public class DettachCommand extends StorageSubSystemCommand {
@@ -28,13 +30,21 @@ public class DettachCommand extends StorageSubSystemCommand {
2830
private String _iScsiName;
2931
private String _storageHost;
3032
private int _storagePort;
33+
private Map<String, String> params;
3134

3235
public DettachCommand(final DiskTO disk, final String vmName) {
3336
super();
3437
this.disk = disk;
3538
this.vmName = vmName;
3639
}
3740

41+
public DettachCommand(final DiskTO disk, final String vmName, Map<String, String> params) {
42+
super();
43+
this.disk = disk;
44+
this.vmName = vmName;
45+
this.params = params;
46+
}
47+
3848
@Override
3949
public boolean executeInSequence() {
4050
return false;
@@ -88,6 +98,14 @@ public int getStoragePort() {
8898
return _storagePort;
8999
}
90100

101+
public Map<String, String> getParams() {
102+
return params;
103+
}
104+
105+
public void setParams(Map<String, String> params) {
106+
this.params = params;
107+
}
108+
91109
@Override
92110
public void setExecuteInSequence(final boolean inSeq) {
93111

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java‎

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2405,7 +2405,7 @@ So if getMinSpeed() returns null we fall back to getSpeed().
24052405
DiskDef.DiskBus busT = getDiskModelFromVMDetail(vmTO);
24062406

24072407
if (busT == null) {
2408-
busT = getGuestDiskModel(vmTO.getPlatformEmulator());
2408+
busT = getGuestDiskModel(vmTO.getPlatformEmulator(), isUefiEnabled);
24092409
}
24102410

24112411
// If we're using virtio scsi, then we need to add a virtual scsi controller
@@ -2490,7 +2490,8 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
24902490
}
24912491
});
24922492

2493-
if (MapUtils.isNotEmpty(details) && details.containsKey(GuestDef.BootType.UEFI.toString())) {
2493+
boolean isUefiEnabled = MapUtils.isNotEmpty(details) && details.containsKey(GuestDef.BootType.UEFI.toString());
2494+
if (isUefiEnabled) {
24942495
isSecureBoot = isSecureMode(details.get(GuestDef.BootType.UEFI.toString()));
24952496
}
24962497
if (vmSpec.getOs().toLowerCase().contains("window")) {
@@ -2554,7 +2555,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
25542555
// if params contains a rootDiskController key, use its value (this is what other HVs are doing)
25552556
DiskDef.DiskBus diskBusType = getDiskModelFromVMDetail(vmSpec);
25562557
if (diskBusType == null) {
2557-
diskBusType = getGuestDiskModel(vmSpec.getPlatformEmulator());
2558+
diskBusType = getGuestDiskModel(vmSpec.getPlatformEmulator(), isUefiEnabled);
25582559
}
25592560

25602561
// I'm not sure why previously certain DATADISKs were hard-coded VIRTIO and others not, however this
@@ -2564,16 +2565,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
25642565
final DiskDef disk = new DiskDef();
25652566
int devId = volume.getDiskSeq().intValue();
25662567
if (volume.getType() == Volume.Type.ISO) {
2567-
if (volPath == null) {
2568-
if (isSecureBoot) {
2569-
disk.defISODisk(null, devId,isSecureBoot,isWindowsTemplate);
2570-
} else {
2571-
/* Add iso as placeholder */
2572-
disk.defISODisk(null, devId);
2573-
}
2574-
} else {
2575-
disk.defISODisk(volPath, devId);
2576-
}
2568+
2569+
disk.defISODisk(volPath, devId, isUefiEnabled);
2570+
25772571
if (_guestCpuArch != null && _guestCpuArch.equals("aarch64")) {
25782572
disk.setBusType(DiskDef.DiskBus.SCSI);
25792573
}
@@ -2598,14 +2592,14 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
25982592
disk.defNetworkBasedDisk(glusterVolume + path.replace(mountpoint, ""), pool.getSourceHost(), pool.getSourcePort(), null,
25992593
null, devId, diskBusType, DiskProtocol.GLUSTER, DiskDef.DiskFmtType.QCOW2);
26002594
} else if (pool.getType() == StoragePoolType.CLVM || physicalDisk.getFormat() == PhysicalDiskFormat.RAW) {
2601-
if (volume.getType() == Volume.Type.DATADISK) {
2595+
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
26022596
disk.defBlockBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData);
26032597
}
26042598
else {
26052599
disk.defBlockBasedDisk(physicalDisk.getPath(), devId, diskBusType);
26062600
}
26072601
} else {
2608-
if (volume.getType() == Volume.Type.DATADISK) {
2602+
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
26092603
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
26102604
} else {
26112605
if (isSecureBoot) {
@@ -3410,7 +3404,7 @@ private String getHypervisorPath(final Connect conn) {
34103404
}
34113405

34123406
boolean isGuestPVEnabled(final String guestOSName) {
3413-
DiskDef.DiskBus db = getGuestDiskModel(guestOSName);
3407+
DiskDef.DiskBus db = getGuestDiskModel(guestOSName, false);
34143408
return db != DiskDef.DiskBus.IDE;
34153409
}
34163410

@@ -3445,7 +3439,7 @@ public DiskDef.DiskBus getDiskModelFromVMDetail(final VirtualMachineTO vmTO) {
34453439
return null;
34463440
}
34473441

3448-
private DiskDef.DiskBus getGuestDiskModel(final String platformEmulator) {
3442+
private DiskDef.DiskBus getGuestDiskModel(final String platformEmulator, boolean isUefiEnabled) {
34493443
if (_guestCpuArch != null && _guestCpuArch.equals("aarch64")) {
34503444
return DiskDef.DiskBus.SCSI;
34513445
}
@@ -3454,6 +3448,8 @@ private DiskDef.DiskBus getGuestDiskModel(final String platformEmulator) {
34543448
return DiskDef.DiskBus.IDE;
34553449
} else if (platformEmulator.startsWith("Other PV Virtio-SCSI")) {
34563450
return DiskDef.DiskBus.SCSI;
3451+
} else if (isUefiEnabled && platformEmulator.startsWith("Windows")) {
3452+
return DiskDef.DiskBus.SATA;
34573453
} else if (platformEmulator.contains("Ubuntu") ||
34583454
platformEmulator.startsWith("Fedora") ||
34593455
platformEmulator.startsWith("CentOS") ||

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -716,16 +716,14 @@ private String getDevLabel(int devId, DiskBus bus, boolean forIso) {
716716
} else if (bus == DiskBus.VIRTIO) {
717717
return "vd" + getDevLabelSuffix(devId);
718718
} else if (bus == DiskBus.SATA){
719-
if (!forIso) {
720-
return "sda";
721-
}
719+
return "sd" + getDevLabelSuffix(devId);
722720
}
723721
if (forIso) {
724722
devId --;
725723
} else if(devId >= 2) {
726724
devId += 2;
727725
}
728-
return (DiskBus.SATA == bus) ? "sdb" : "hd" + getDevLabelSuffix(devId);
726+
return "hd" + getDevLabelSuffix(devId);
729727

730728
}
731729

@@ -784,6 +782,16 @@ public void defISODisk(String volPath) {
784782
_bus = DiskBus.IDE;
785783
}
786784

785+
public void defISODisk(String volPath, boolean isUefiEnabled) {
786+
_diskType = DiskType.FILE;
787+
_deviceType = DeviceType.CDROM;
788+
_sourcePath = volPath;
789+
_bus = isUefiEnabled ? DiskBus.SATA : DiskBus.IDE;
790+
_diskLabel = getDevLabel(3, _bus, true);
791+
_diskFmtType = DiskFmtType.RAW;
792+
_diskCacheMode = DiskCacheMode.NONE;
793+
}
794+
787795
public void defISODisk(String volPath, Integer devId) {
788796
if (devId == null) {
789797
defISODisk(volPath);
@@ -798,20 +806,15 @@ public void defISODisk(String volPath, Integer devId) {
798806
}
799807
}
800808

801-
public void defISODisk(String volPath, Integer devId,boolean isSecure, boolean isWindowOs) {
809+
public void defISODisk(String volPath, Integer devId,boolean isSecure) {
802810
if (!isSecure) {
803811
defISODisk(volPath, devId);
804812
} else {
805813
_diskType = DiskType.FILE;
806814
_deviceType = DeviceType.CDROM;
807815
_sourcePath = volPath;
808-
if (isWindowOs) {
809-
_diskLabel = getDevLabel(devId, DiskBus.SATA, true);
810-
_bus = DiskBus.SATA;
811-
} else {
812-
_diskLabel = getDevLabel(devId, DiskBus.SCSI, true);
813-
_bus = DiskBus.SCSI;
814-
}
816+
_diskLabel = getDevLabel(devId, DiskBus.SATA, true);
817+
_bus = DiskBus.SATA;
815818
_diskFmtType = DiskFmtType.RAW;
816819
_diskCacheMode = DiskCacheMode.NONE;
817820

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@
6868
import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
6969
import org.apache.cloudstack.utils.qemu.QemuImgException;
7070
import org.apache.cloudstack.utils.qemu.QemuImgFile;
71+
import org.apache.commons.collections.MapUtils;
7172
import org.apache.commons.io.FileUtils;
7273
import org.apache.log4j.Logger;
7374
import org.libvirt.Connect;
@@ -1055,9 +1056,10 @@ private void deleteSnapshotViaManageSnapshotScript(final String snapshotName, KV
10551056
}
10561057
}
10571058

1058-
protected synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach) throws LibvirtException, URISyntaxException,
1059+
protected synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach, Map<String, String> params) throws LibvirtException, URISyntaxException,
10591060
InternalErrorException {
10601061
String isoXml = null;
1062+
boolean isUefiEnabled = MapUtils.isNotEmpty(params) && params.containsKey("UEFI");
10611063
if (isoPath != null && isAttach) {
10621064
final int index = isoPath.lastIndexOf("/");
10631065
final String path = isoPath.substring(0, index);
@@ -1067,11 +1069,11 @@ protected synchronized String attachOrDetachISO(final Connect conn, final String
10671069
isoPath = isoVol.getPath();
10681070

10691071
final DiskDef iso = new DiskDef();
1070-
iso.defISODisk(isoPath);
1072+
iso.defISODisk(isoPath, isUefiEnabled);
10711073
isoXml = iso.toString();
10721074
} else {
10731075
final DiskDef iso = new DiskDef();
1074-
iso.defISODisk(null);
1076+
iso.defISODisk(null, isUefiEnabled);
10751077
isoXml = iso.toString();
10761078
}
10771079

@@ -1097,7 +1099,7 @@ public Answer attachIso(final AttachCommand cmd) {
10971099
try {
10981100
String dataStoreUrl = getDataStoreUrlFromStore(store);
10991101
final Connect conn = LibvirtConnection.getConnectionByVmName(cmd.getVmName());
1100-
attachOrDetachISO(conn, cmd.getVmName(), dataStoreUrl + File.separator + isoTO.getPath(), true);
1102+
attachOrDetachISO(conn, cmd.getVmName(), dataStoreUrl + File.separator + isoTO.getPath(), true, cmd.getControllerInfo());
11011103
} catch (final LibvirtException e) {
11021104
return new Answer(cmd, false, e.toString());
11031105
} catch (final URISyntaxException e) {
@@ -1120,7 +1122,7 @@ public Answer dettachIso(final DettachCommand cmd) {
11201122
try {
11211123
String dataStoreUrl = getDataStoreUrlFromStore(store);
11221124
final Connect conn = LibvirtConnection.getConnectionByVmName(cmd.getVmName());
1123-
attachOrDetachISO(conn, cmd.getVmName(), dataStoreUrl + File.separator + isoTO.getPath(), false);
1125+
attachOrDetachISO(conn, cmd.getVmName(), dataStoreUrl + File.separator + isoTO.getPath(), false, cmd.getParams());
11241126
} catch (final LibvirtException e) {
11251127
return new Answer(cmd, false, e.toString());
11261128
} catch (final URISyntaxException e) {

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@
112112
import com.cloud.agent.api.to.DatadiskTO;
113113
import com.cloud.agent.api.to.DiskTO;
114114
import com.cloud.agent.api.to.NfsTO;
115+
import com.cloud.agent.api.to.VirtualMachineTO;
115116
import com.cloud.api.ApiDBUtils;
116117
import com.cloud.api.ApiResponseHelper;
117118
import com.cloud.api.query.dao.UserVmJoinDao;
@@ -136,6 +137,8 @@
136137
import com.cloud.host.HostVO;
137138
import com.cloud.host.dao.HostDao;
138139
import com.cloud.hypervisor.Hypervisor;
140+
import com.cloud.hypervisor.HypervisorGuru;
141+
import com.cloud.hypervisor.HypervisorGuruManager;
139142
import com.cloud.hypervisor.Hypervisor.HypervisorType;
140143
import com.cloud.projects.Project;
141144
import com.cloud.projects.ProjectManager;
@@ -199,6 +202,7 @@
199202
import com.cloud.vm.VMInstanceVO;
200203
import com.cloud.vm.VirtualMachine.State;
201204
import com.cloud.vm.VirtualMachineProfile;
205+
import com.cloud.vm.VirtualMachineProfileImpl;
202206
import com.cloud.vm.VmDetailConstants;
203207
import com.cloud.vm.dao.UserVmDao;
204208
import com.cloud.vm.dao.VMInstanceDao;
@@ -283,6 +287,8 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager,
283287
MessageBus _messageBus;
284288
@Inject
285289
private VMTemplateDetailsDao _tmpltDetailsDao;
290+
@Inject
291+
private HypervisorGuruManager _hvGuruMgr;
286292

287293
private boolean _disableExtraction = false;
288294
private List<TemplateAdapter> _adapters;
@@ -1289,11 +1295,16 @@ private boolean attachISOToVM(long vmId, long isoId, boolean attach) {
12891295

12901296
DataTO isoTO = tmplt.getTO();
12911297
DiskTO disk = new DiskTO(isoTO, null, null, Volume.Type.ISO);
1298+
1299+
HypervisorGuru hvGuru = _hvGuruMgr.getGuru(vm.getHypervisorType());
1300+
VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm);
1301+
VirtualMachineTO vmTO = hvGuru.implement(profile);
1302+
12921303
Command cmd = null;
12931304
if (attach) {
1294-
cmd = new AttachCommand(disk, vmName);
1305+
cmd = new AttachCommand(disk, vmName, vmTO.getDetails());
12951306
} else {
1296-
cmd = new DettachCommand(disk, vmName);
1307+
cmd = new DettachCommand(disk, vmName, vmTO.getDetails());
12971308
}
12981309
Answer a = _agentMgr.easySend(vm.getHostId(), cmd);
12991310
return (a != null && a.getResult());

‎server/src/test/java/com/cloud/template/TemplateManagerImplTest.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import com.cloud.host.Status;
3131
import com.cloud.host.dao.HostDao;
3232
import com.cloud.hypervisor.Hypervisor;
33+
import com.cloud.hypervisor.HypervisorGuruManager;
3334
import com.cloud.storage.Storage;
3435
import com.cloud.storage.TemplateProfile;
3536
import com.cloud.projects.ProjectManager;
@@ -180,6 +181,8 @@ public class TemplateManagerImplTest {
180181
@Inject
181182
private VMTemplateDao _tmpltDao;
182183

184+
@Inject
185+
HypervisorGuruManager _hvGuruMgr;
183186

184187
public class CustomThreadPoolExecutor extends ThreadPoolExecutor {
185188
AtomicInteger ai = new AtomicInteger(0);
@@ -697,6 +700,11 @@ public VMTemplateDetailsDao vmTemplateDetailsDao() {
697700
return Mockito.mock(VMTemplateDetailsDao.class);
698701
}
699702

703+
@Bean
704+
public HypervisorGuruManager hypervisorGuruManager() {
705+
return Mockito.mock(HypervisorGuruManager.class);
706+
}
707+
700708
public static class Library implements TypeFilter {
701709
@Override
702710
public boolean match(MetadataReader mdr, MetadataReaderFactory arg1) throws IOException {

0 commit comments

Comments
 (0)