Skip to content

Commit e34a8bf

Browse files
shwstpprshwstppr
committed
ikvm: allow skip forcing disk controller
Fixes #9460 A VM or template detail can be added with key `skip.force.disk.controller` and value `true` to allow skipping forcing disk controllers for the VM especially in case of UEFI VMs. Otherwise, current behaviour of disk controllers depend on the guest OS, UEFI secure boot where Windows VM may always be provisioned with `sata` and other OS VMs with `virtio`. Signed-off-by: Abhishek Kumar <[email protected]>
1 parent 2e113e5 commit e34a8bf

File tree

3 files changed

+105
-17
lines changed

3 files changed

+105
-17
lines changed

‎api/src/main/java/com/cloud/vm/VmDetailConstants.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,9 @@ public interface VmDetailConstants {
5353
String NIC_MULTIQUEUE_NUMBER = "nic.multiqueue.number";
5454
String NIC_PACKED_VIRTQUEUES_ENABLED = "nic.packed.virtqueues.enabled";
5555

56+
// KVM specific, disk controllers
57+
String KVM_SKIP_FORCE_DISK_CONTROLLER = "skip.force.disk.controller";
58+
5659
// Mac OSX guest specific (internal)
5760
String SMC_PRESENT = "smc.present";
5861
String FIRMWARE = "firmware";

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

Lines changed: 41 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,6 @@
100100
import org.xml.sax.InputSource;
101101
import org.xml.sax.SAXException;
102102

103-
104103
import com.cloud.agent.api.Answer;
105104
import com.cloud.agent.api.Command;
106105
import com.cloud.agent.api.HostVmStateReportEntry;
@@ -180,8 +179,8 @@
180179
import com.cloud.network.Networks.RouterPrivateIpStrategy;
181180
import com.cloud.network.Networks.TrafficType;
182181
import com.cloud.resource.AgentStatusUpdater;
183-
import com.cloud.resource.ResourceStatusUpdater;
184182
import com.cloud.resource.RequestWrapper;
183+
import com.cloud.resource.ResourceStatusUpdater;
185184
import com.cloud.resource.ServerResource;
186185
import com.cloud.resource.ServerResourceBase;
187186
import com.cloud.storage.JavaStorageLayer;
@@ -3010,6 +3009,44 @@ public static DiskDef.DiskType getDiskType(KVMPhysicalDisk physicalDisk) {
30103009
return useBLOCKDiskType(physicalDisk) ? DiskDef.DiskType.BLOCK : DiskDef.DiskType.FILE;
30113010
}
30123011

3012+
/**
3013+
* Defines the disk configuration for the default pool type based on the provided parameters.
3014+
* It determines the appropriate disk settings depending on whether the disk is a data disk, whether
3015+
* it's a Windows template, whether UEFI is enabled, and whether secure boot is active.
3016+
*
3017+
* @param disk The disk definition object that will be configured with the disk settings.
3018+
* @param volume The volume (disk) object, containing information about the type of disk.
3019+
* @param isWindowsTemplate Flag indicating whether the template is a Windows template.
3020+
* @param isUefiEnabled Flag indicating whether UEFI is enabled.
3021+
* @param isSecureBoot Flag indicating whether secure boot is enabled.
3022+
* @param physicalDisk The physical disk object that contains the path to the disk.
3023+
* @param devId The device ID for the disk.
3024+
* @param diskBusType The disk bus type to use if not skipping force disk controller.
3025+
* @param diskBusTypeData The disk bus type to use for data disks, if applicable.
3026+
* @param details A map of VM details containing additional configuration values, such as whether to skip force
3027+
* disk controller.
3028+
*/
3029+
protected void defineDiskForDefaultPoolType(DiskDef disk, DiskTO volume, boolean isWindowsTemplate,
3030+
boolean isUefiEnabled, boolean isSecureBoot, KVMPhysicalDisk physicalDisk, int devId,
3031+
DiskDef.DiskBus diskBusType, DiskDef.DiskBus diskBusTypeData, Map<String, String> details) {
3032+
boolean skipForceDiskController = MapUtils.getBoolean(details, VmDetailConstants.KVM_SKIP_FORCE_DISK_CONTROLLER,
3033+
false);
3034+
if (skipForceDiskController) {
3035+
disk.defFileBasedDisk(physicalDisk.getPath(), devId, Volume.Type.DATADISK.equals(volume.getType()) ?
3036+
diskBusTypeData : diskBusType, DiskDef.DiskFmtType.QCOW2);
3037+
return;
3038+
}
3039+
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
3040+
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
3041+
} else {
3042+
if (isSecureBoot) {
3043+
disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
3044+
} else {
3045+
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
3046+
}
3047+
}
3048+
}
3049+
30133050
public void createVbd(final Connect conn, final VirtualMachineTO vmSpec, final String vmName, final LibvirtVMDef vm) throws InternalErrorException, LibvirtException, URISyntaxException {
30143051
final Map<String, String> details = vmSpec.getDetails();
30153052
final List<DiskTO> disks = Arrays.asList(vmSpec.getDisks());
@@ -3161,18 +3198,9 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
31613198
disk.setDiscard(DiscardType.UNMAP);
31623199
}
31633200
} else {
3164-
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
3165-
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
3166-
} else {
3167-
if (isSecureBoot) {
3168-
disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
3169-
} else {
3170-
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
3171-
}
3172-
}
3173-
3201+
defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
3202+
physicalDisk,devId, diskBusType, diskBusTypeData, details);
31743203
}
3175-
31763204
}
31773205

31783206
if (data instanceof VolumeObjectTO) {

‎plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java‎

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,6 @@
5656
import javax.xml.xpath.XPathExpressionException;
5757
import javax.xml.xpath.XPathFactory;
5858

59-
import com.cloud.utils.net.NetUtils;
60-
61-
import com.cloud.vm.VmDetailConstants;
6259
import org.apache.cloudstack.api.ApiConstants.IoDriverPolicy;
6360
import org.apache.cloudstack.storage.command.AttachAnswer;
6461
import org.apache.cloudstack.storage.command.AttachCommand;
@@ -217,13 +214,15 @@
217214
import com.cloud.template.VirtualMachineTemplate.BootloaderType;
218215
import com.cloud.utils.Pair;
219216
import com.cloud.utils.exception.CloudRuntimeException;
220-
import com.cloud.utils.script.Script;
217+
import com.cloud.utils.net.NetUtils;
221218
import com.cloud.utils.script.OutputInterpreter.OneLineParser;
219+
import com.cloud.utils.script.Script;
222220
import com.cloud.utils.ssh.SshHelper;
223221
import com.cloud.vm.DiskProfile;
224222
import com.cloud.vm.VirtualMachine;
225223
import com.cloud.vm.VirtualMachine.PowerState;
226224
import com.cloud.vm.VirtualMachine.Type;
225+
import com.cloud.vm.VmDetailConstants;
227226

228227
@RunWith(MockitoJUnitRunner.class)
229228
public class LibvirtComputingResourceTest {
@@ -240,6 +239,19 @@ public class LibvirtComputingResourceTest {
240239
Connect connMock;
241240
@Mock
242241
LibvirtDomainXMLParser parserMock;
242+
@Mock
243+
private DiskDef diskDef;
244+
@Mock
245+
private DiskTO volume;
246+
@Mock
247+
private KVMPhysicalDisk physicalDisk;
248+
@Mock
249+
private Map<String, String> details;
250+
251+
private static final String PHYSICAL_DISK_PATH = "/path/to/disk";
252+
private static final int DEV_ID = 1;
253+
private static final DiskDef.DiskBus DISK_BUS_TYPE = DiskDef.DiskBus.VIRTIO;
254+
private static final DiskDef.DiskBus DISK_BUS_TYPE_DATA = DiskDef.DiskBus.SCSI;
243255

244256
@Spy
245257
private LibvirtComputingResource libvirtComputingResourceSpy = Mockito.spy(new LibvirtComputingResource());
@@ -6421,4 +6433,49 @@ public void testGetDiskModelFromVMDetailVirtioBlk() {
64216433
DiskDef.DiskBus diskBus = libvirtComputingResourceSpy.getDiskModelFromVMDetail(virtualMachineTO);
64226434
assertEquals(DiskDef.DiskBus.VIRTIOBLK, diskBus);
64236435
}
6436+
6437+
@Test
6438+
public void defineDiskForDefaultPoolTypeSkipsForceDiskController() {
6439+
Map<String, String> details = new HashMap<>();
6440+
details.put(VmDetailConstants.KVM_SKIP_FORCE_DISK_CONTROLLER, "true");
6441+
Mockito.when(volume.getType()).thenReturn(Volume.Type.DATADISK);
6442+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6443+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
6444+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
6445+
}
6446+
6447+
@Test
6448+
public void defineDiskForDefaultPoolTypeUsesDiskBusTypeDataForDataDiskWithoutWindowsAndUefi() {
6449+
Map<String, String> details = new HashMap<>();
6450+
Mockito.when(volume.getType()).thenReturn(Volume.Type.DATADISK);
6451+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6452+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
6453+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
6454+
}
6455+
6456+
@Test
6457+
public void defineDiskForDefaultPoolTypeUsesDiskBusTypeForRootDisk() {
6458+
Map<String, String> details = new HashMap<>();
6459+
Mockito.when(volume.getType()).thenReturn(Volume.Type.ROOT);
6460+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6461+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
6462+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE, DiskDef.DiskFmtType.QCOW2);
6463+
}
6464+
6465+
@Test
6466+
public void defineDiskForDefaultPoolTypeUsesSecureBootConfiguration() {
6467+
Map<String, String> details = new HashMap<>();
6468+
Mockito.when(volume.getType()).thenReturn(Volume.Type.ROOT);
6469+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6470+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, true, true, true, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
6471+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DiskDef.DiskFmtType.QCOW2, true);
6472+
}
6473+
6474+
@Test
6475+
public void defineDiskForDefaultPoolTypeHandlesNullDetails() {
6476+
Mockito.when(volume.getType()).thenReturn(Volume.Type.DATADISK);
6477+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6478+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, null);
6479+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
6480+
}
64246481
}

0 commit comments

Comments
 (0)