From e34a8bfa82f5bb25101022f5be297cb2e0284830 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 29 Sep 2025 15:49:18 +0530 Subject: [PATCH 1/2] ikvm: 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 | 54 +++++++++++---- .../LibvirtComputingResourceTest.java | 65 +++++++++++++++++-- 3 files changed, 105 insertions(+), 17 deletions(-) diff --git a/api/src/main/java/com/cloud/vm/VmDetailConstants.java b/api/src/main/java/com/cloud/vm/VmDetailConstants.java index f7d87ab18f1e..158dc949f0da 100644 --- a/api/src/main/java/com/cloud/vm/VmDetailConstants.java +++ b/api/src/main/java/com/cloud/vm/VmDetailConstants.java @@ -53,6 +53,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 b43c7dced0d8..dcee94996912 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 @@ -100,7 +100,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; @@ -180,8 +179,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; @@ -3010,6 +3009,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()); @@ -3161,18 +3198,9 @@ 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); } - } if (data instanceof VolumeObjectTO) { 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 9ef96deb6915..d301adf8ebd7 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()); @@ -6421,4 +6433,49 @@ public void testGetDiskModelFromVMDetailVirtioBlk() { DiskDef.DiskBus diskBus = libvirtComputingResourceSpy.getDiskModelFromVMDetail(virtualMachineTO); assertEquals(DiskDef.DiskBus.VIRTIOBLK, diskBus); } + + @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 dfa2832eb7833e49d27069474eea614acf670367 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 1 Oct 2025 09:25:54 +0530 Subject: [PATCH 2/2] fix minor space --- .../cloud/hypervisor/kvm/resource/LibvirtComputingResource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 dcee94996912..8025bbbe2400 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 @@ -3199,7 +3199,7 @@ public int compare(final DiskTO arg0, final DiskTO arg1) { } } else { defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot, - physicalDisk,devId, diskBusType, diskBusTypeData, details); + physicalDisk, devId, diskBusType, diskBusTypeData, details); } }