From 7a1f2e747c1e957f89bc8c67013babce5fc793f9 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 29 Sep 2025 15:49:18 +0530 Subject: [PATCH 1/2] kvm: allow skip forcing disk controller Fixes apache#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 --- .../java/com/cloud/vm/VmDetailConstants.java | 3 + .../resource/LibvirtComputingResource.java | 52 +++++++++++---- .../LibvirtComputingResourceTest.java | 65 +++++++++++++++++-- 3 files changed, 105 insertions(+), 15 deletions(-) diff --git a/api/src/main/java/com/cloud/vm/VmDetailConstants.java b/api/src/main/java/com/cloud/vm/VmDetailConstants.java index a6c9b6eba16b..3d0152b0e438 100644 --- a/api/src/main/java/com/cloud/vm/VmDetailConstants.java +++ b/api/src/main/java/com/cloud/vm/VmDetailConstants.java @@ -54,6 +54,9 @@ public interface VmDetailConstants { String NIC_MULTIQUEUE_NUMBER = "nic.multiqueue.number"; String NIC_PACKED_VIRTQUEUES_ENABLED = "nic.packed.virtqueues.enabled"; + // KVM specific, disk controllers + String KVM_SKIP_FORCE_DISK_CONTROLLER = "skip.force.disk.controller"; + // Mac OSX guest specific (internal) String SMC_PRESENT = "smc.present"; String FIRMWARE = "firmware"; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 87ea55ca7669..b66a838a3a52 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -107,7 +107,6 @@ import org.xml.sax.InputSource; import org.xml.sax.SAXException; - import com.cloud.agent.api.Answer; import com.cloud.agent.api.Command; import com.cloud.agent.api.HostVmStateReportEntry; @@ -188,8 +187,8 @@ import com.cloud.network.Networks.RouterPrivateIpStrategy; import com.cloud.network.Networks.TrafficType; import com.cloud.resource.AgentStatusUpdater; -import com.cloud.resource.ResourceStatusUpdater; import com.cloud.resource.RequestWrapper; +import com.cloud.resource.ResourceStatusUpdater; import com.cloud.resource.ServerResource; import com.cloud.resource.ServerResourceBase; import com.cloud.storage.JavaStorageLayer; @@ -3083,6 +3082,44 @@ public static DiskDef.DiskType getDiskType(KVMPhysicalDisk physicalDisk) { return useBLOCKDiskType(physicalDisk) ? DiskDef.DiskType.BLOCK : DiskDef.DiskType.FILE; } + /** + * Defines the disk configuration for the default pool type based on the provided parameters. + * It determines the appropriate disk settings depending on whether the disk is a data disk, whether + * it's a Windows template, whether UEFI is enabled, and whether secure boot is active. + * + * @param disk The disk definition object that will be configured with the disk settings. + * @param volume The volume (disk) object, containing information about the type of disk. + * @param isWindowsTemplate Flag indicating whether the template is a Windows template. + * @param isUefiEnabled Flag indicating whether UEFI is enabled. + * @param isSecureBoot Flag indicating whether secure boot is enabled. + * @param physicalDisk The physical disk object that contains the path to the disk. + * @param devId The device ID for the disk. + * @param diskBusType The disk bus type to use if not skipping force disk controller. + * @param diskBusTypeData The disk bus type to use for data disks, if applicable. + * @param details A map of VM details containing additional configuration values, such as whether to skip force + * disk controller. + */ + protected void defineDiskForDefaultPoolType(DiskDef disk, DiskTO volume, boolean isWindowsTemplate, + boolean isUefiEnabled, boolean isSecureBoot, KVMPhysicalDisk physicalDisk, int devId, + DiskDef.DiskBus diskBusType, DiskDef.DiskBus diskBusTypeData, Map details) { + boolean skipForceDiskController = MapUtils.getBoolean(details, VmDetailConstants.KVM_SKIP_FORCE_DISK_CONTROLLER, + false); + if (skipForceDiskController) { + disk.defFileBasedDisk(physicalDisk.getPath(), devId, Volume.Type.DATADISK.equals(volume.getType()) ? + diskBusTypeData : diskBusType, DiskDef.DiskFmtType.QCOW2); + return; + } + if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) { + disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2); + } else { + if (isSecureBoot) { + disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate); + } else { + disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2); + } + } + } + public void createVbd(final Connect conn, final VirtualMachineTO vmSpec, final String vmName, final LibvirtVMDef vm) throws InternalErrorException, LibvirtException, URISyntaxException { final Map details = vmSpec.getDetails(); final List disks = Arrays.asList(vmSpec.getDisks()); @@ -3244,15 +3281,8 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { disk.setDiscard(DiscardType.UNMAP); } } else { - if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) { - disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2); - } else { - if (isSecureBoot) { - disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate); - } else { - disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2); - } - } + defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot, + physicalDisk, devId, diskBusType, diskBusTypeData, details); } pool.customizeLibvirtDiskDef(disk); } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index 88e0983b63e6..ed163787b112 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -56,9 +56,6 @@ import javax.xml.xpath.XPathExpressionException; import javax.xml.xpath.XPathFactory; -import com.cloud.utils.net.NetUtils; - -import com.cloud.vm.VmDetailConstants; import org.apache.cloudstack.api.ApiConstants.IoDriverPolicy; import org.apache.cloudstack.storage.command.AttachAnswer; import org.apache.cloudstack.storage.command.AttachCommand; @@ -217,13 +214,15 @@ import com.cloud.template.VirtualMachineTemplate.BootloaderType; import com.cloud.utils.Pair; import com.cloud.utils.exception.CloudRuntimeException; -import com.cloud.utils.script.Script; +import com.cloud.utils.net.NetUtils; import com.cloud.utils.script.OutputInterpreter.OneLineParser; +import com.cloud.utils.script.Script; import com.cloud.utils.ssh.SshHelper; import com.cloud.vm.DiskProfile; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachine.PowerState; import com.cloud.vm.VirtualMachine.Type; +import com.cloud.vm.VmDetailConstants; @RunWith(MockitoJUnitRunner.class) public class LibvirtComputingResourceTest { @@ -240,6 +239,19 @@ public class LibvirtComputingResourceTest { Connect connMock; @Mock LibvirtDomainXMLParser parserMock; + @Mock + private DiskDef diskDef; + @Mock + private DiskTO volume; + @Mock + private KVMPhysicalDisk physicalDisk; + @Mock + private Map details; + + private static final String PHYSICAL_DISK_PATH = "/path/to/disk"; + private static final int DEV_ID = 1; + private static final DiskDef.DiskBus DISK_BUS_TYPE = DiskDef.DiskBus.VIRTIO; + private static final DiskDef.DiskBus DISK_BUS_TYPE_DATA = DiskDef.DiskBus.SCSI; @Spy private LibvirtComputingResource libvirtComputingResourceSpy = Mockito.spy(new LibvirtComputingResource()); @@ -6565,4 +6577,49 @@ public void testCreateTpmDefWithInvalidVersion() { assertEquals(LibvirtVMDef.TpmDef.TpmModel.CRB, tpmDef.getModel()); assertEquals(LibvirtVMDef.TpmDef.TpmVersion.V2_0, tpmDef.getVersion()); } + + @Test + public void defineDiskForDefaultPoolTypeSkipsForceDiskController() { + Map details = new HashMap<>(); + details.put(VmDetailConstants.KVM_SKIP_FORCE_DISK_CONTROLLER, "true"); + Mockito.when(volume.getType()).thenReturn(Volume.Type.DATADISK); + Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH); + libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details); + Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2); + } + + @Test + public void defineDiskForDefaultPoolTypeUsesDiskBusTypeDataForDataDiskWithoutWindowsAndUefi() { + Map details = new HashMap<>(); + Mockito.when(volume.getType()).thenReturn(Volume.Type.DATADISK); + Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH); + libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details); + Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2); + } + + @Test + public void defineDiskForDefaultPoolTypeUsesDiskBusTypeForRootDisk() { + Map details = new HashMap<>(); + Mockito.when(volume.getType()).thenReturn(Volume.Type.ROOT); + Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH); + libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details); + Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE, DiskDef.DiskFmtType.QCOW2); + } + + @Test + public void defineDiskForDefaultPoolTypeUsesSecureBootConfiguration() { + Map details = new HashMap<>(); + Mockito.when(volume.getType()).thenReturn(Volume.Type.ROOT); + Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH); + libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, true, true, true, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details); + Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DiskDef.DiskFmtType.QCOW2, true); + } + + @Test + public void defineDiskForDefaultPoolTypeHandlesNullDetails() { + Mockito.when(volume.getType()).thenReturn(Volume.Type.DATADISK); + Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH); + libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, null); + Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2); + } } From 88d933483bcd1bc609aff586f94068aba60f3376 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 7 Jan 2026 16:43:04 +0530 Subject: [PATCH 2/2] address comment Signed-off-by: Abhishek Kumar --- server/src/main/java/com/cloud/api/query/QueryManagerImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index 8641eb7ffc3c..5833ede550e7 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -5072,6 +5072,7 @@ private void fillVMOrTemplateDetailOptions(final Map> optio options.put(VmDetailConstants.VIRTUAL_TPM_VERSION, Arrays.asList("1.2", "2.0")); options.put(VmDetailConstants.GUEST_CPU_MODE, Arrays.asList("custom", "host-model", "host-passthrough")); options.put(VmDetailConstants.GUEST_CPU_MODEL, Collections.emptyList()); + options.put(VmDetailConstants.KVM_SKIP_FORCE_DISK_CONTROLLER, Arrays.asList("true", "false")); } if (HypervisorType.VMware.equals(hypervisorType)) {