Skip to content

kvm: allow skip forcing disk controller #11750

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shwstppr wants to merge 2 commits into
base: 4.19
Choose a base branch
from
Open

kvm: allow skip forcing disk controller #11750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e34a8bf
ikvm: allow skip forcing disk controller
shwstppr Sep 29, 2025
dfa2832
fix minor space
shwstppr Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/src/main/java/com/cloud/vm/VmDetailConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Contributor

@sureshanaparti sureshanaparti Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shwstppr should it be included for for KVM here:

cloudstack/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

Lines 5060 to 5061 in e90e436

if (HypervisorType.KVM.equals(hypervisorType)) {
options.put(VmDetailConstants.CPU_THREAD_PER_CORE, Collections.emptyList());

also, check if any doc changes needed for this vm detail.


// Mac OSX guest specific (internal)
String SMC_PRESENT = "smc.present";
String FIRMWARE = "firmware";
Expand Down
54 changes: 41 additions & 13 deletions ...rvisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<String, String> details) {
boolean skipForceDiskController = MapUtils.getBoolean(details, VmDetailConstants.KVM_SKIP_FORCE_DISK_CONTROLLER,
false);
if (skipForceDiskController) {
Copy link

@pavanaravapalli pavanaravapalli Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shwstppr
This condition forces the root disk controller to use its configured settings instead of the UEFI workflow settings, even when the VM is deployed with UEFI and this setting "skip.force.disk.controller" is enabled. Consequently, a UEFI-deployed VM may fail to boot because some guest operating systems still depend on SATA disk. Please include a comment or a note detailing the change in behavior."

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<String, String> details = vmSpec.getDetails();
final List<DiskTO> disks = Arrays.asList(vmSpec.getDisks());
Expand Down Expand Up @@ -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) {
Expand Down
65 changes: 61 additions & 4 deletions ...ors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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<String, String> 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());
Expand Down Expand Up @@ -6421,4 +6433,49 @@ public void testGetDiskModelFromVMDetailVirtioBlk() {
DiskDef.DiskBus diskBus = libvirtComputingResourceSpy.getDiskModelFromVMDetail(virtualMachineTO);
assertEquals(DiskDef.DiskBus.VIRTIOBLK, diskBus);
}

@Test
public void defineDiskForDefaultPoolTypeSkipsForceDiskController() {
Map<String, String> 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<String, String> 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<String, String> 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<String, String> 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);
}
}
Loading