Skip to content

Commit 7a1f2e7

Browse files
shwstpprshwstppr
committed
kvm: 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 <abhishek.mrt22@gmail.com>
1 parent 750290b commit 7a1f2e7

3 files changed

Lines changed: 105 additions & 15 deletions

File tree

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

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

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

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

Lines changed: 41 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,6 @@
107107
import org.xml.sax.InputSource;
108108
import org.xml.sax.SAXException;
109109

110-
111110
import com.cloud.agent.api.Answer;
112111
import com.cloud.agent.api.Command;
113112
import com.cloud.agent.api.HostVmStateReportEntry;
@@ -188,8 +187,8 @@
188187
import com.cloud.network.Networks.RouterPrivateIpStrategy;
189188
import com.cloud.network.Networks.TrafficType;
190189
import com.cloud.resource.AgentStatusUpdater;
191-
import com.cloud.resource.ResourceStatusUpdater;
192190
import com.cloud.resource.RequestWrapper;
191+
import com.cloud.resource.ResourceStatusUpdater;
193192
import com.cloud.resource.ServerResource;
194193
import com.cloud.resource.ServerResourceBase;
195194
import com.cloud.storage.JavaStorageLayer;
@@ -3083,6 +3082,44 @@ public static DiskDef.DiskType getDiskType(KVMPhysicalDisk physicalDisk) {
30833082
return useBLOCKDiskType(physicalDisk) ? DiskDef.DiskType.BLOCK : DiskDef.DiskType.FILE;
30843083
}
30853084

3085+
/**
3086+
* Defines the disk configuration for the default pool type based on the provided parameters.
3087+
* It determines the appropriate disk settings depending on whether the disk is a data disk, whether
3088+
* it's a Windows template, whether UEFI is enabled, and whether secure boot is active.
3089+
*
3090+
* @param disk The disk definition object that will be configured with the disk settings.
3091+
* @param volume The volume (disk) object, containing information about the type of disk.
3092+
* @param isWindowsTemplate Flag indicating whether the template is a Windows template.
3093+
* @param isUefiEnabled Flag indicating whether UEFI is enabled.
3094+
* @param isSecureBoot Flag indicating whether secure boot is enabled.
3095+
* @param physicalDisk The physical disk object that contains the path to the disk.
3096+
* @param devId The device ID for the disk.
3097+
* @param diskBusType The disk bus type to use if not skipping force disk controller.
3098+
* @param diskBusTypeData The disk bus type to use for data disks, if applicable.
3099+
* @param details A map of VM details containing additional configuration values, such as whether to skip force
3100+
* disk controller.
3101+
*/
3102+
protected void defineDiskForDefaultPoolType(DiskDef disk, DiskTO volume, boolean isWindowsTemplate,
3103+
boolean isUefiEnabled, boolean isSecureBoot, KVMPhysicalDisk physicalDisk, int devId,
3104+
DiskDef.DiskBus diskBusType, DiskDef.DiskBus diskBusTypeData, Map<String, String> details) {
3105+
boolean skipForceDiskController = MapUtils.getBoolean(details, VmDetailConstants.KVM_SKIP_FORCE_DISK_CONTROLLER,
3106+
false);
3107+
if (skipForceDiskController) {
3108+
disk.defFileBasedDisk(physicalDisk.getPath(), devId, Volume.Type.DATADISK.equals(volume.getType()) ?
3109+
diskBusTypeData : diskBusType, DiskDef.DiskFmtType.QCOW2);
3110+
return;
3111+
}
3112+
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
3113+
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
3114+
} else {
3115+
if (isSecureBoot) {
3116+
disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
3117+
} else {
3118+
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
3119+
}
3120+
}
3121+
}
3122+
30863123
public void createVbd(final Connect conn, final VirtualMachineTO vmSpec, final String vmName, final LibvirtVMDef vm) throws InternalErrorException, LibvirtException, URISyntaxException {
30873124
final Map<String, String> details = vmSpec.getDetails();
30883125
final List<DiskTO> disks = Arrays.asList(vmSpec.getDisks());
@@ -3244,15 +3281,8 @@ public int compare(final DiskTO arg0, final DiskTO arg1) {
32443281
disk.setDiscard(DiscardType.UNMAP);
32453282
}
32463283
} else {
3247-
if (volume.getType() == Volume.Type.DATADISK && !(isWindowsTemplate && isUefiEnabled)) {
3248-
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusTypeData, DiskDef.DiskFmtType.QCOW2);
3249-
} else {
3250-
if (isSecureBoot) {
3251-
disk.defFileBasedDisk(physicalDisk.getPath(), devId, DiskDef.DiskFmtType.QCOW2, isWindowsTemplate);
3252-
} else {
3253-
disk.defFileBasedDisk(physicalDisk.getPath(), devId, diskBusType, DiskDef.DiskFmtType.QCOW2);
3254-
}
3255-
}
3284+
defineDiskForDefaultPoolType(disk, volume, isWindowsTemplate, isUefiEnabled, isSecureBoot,
3285+
physicalDisk, devId, diskBusType, diskBusTypeData, details);
32563286
}
32573287
pool.customizeLibvirtDiskDef(disk);
32583288
}

‎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());
@@ -6565,4 +6577,49 @@ public void testCreateTpmDefWithInvalidVersion() {
65656577
assertEquals(LibvirtVMDef.TpmDef.TpmModel.CRB, tpmDef.getModel());
65666578
assertEquals(LibvirtVMDef.TpmDef.TpmVersion.V2_0, tpmDef.getVersion());
65676579
}
6580+
6581+
@Test
6582+
public void defineDiskForDefaultPoolTypeSkipsForceDiskController() {
6583+
Map<String, String> details = new HashMap<>();
6584+
details.put(VmDetailConstants.KVM_SKIP_FORCE_DISK_CONTROLLER, "true");
6585+
Mockito.when(volume.getType()).thenReturn(Volume.Type.DATADISK);
6586+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6587+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
6588+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
6589+
}
6590+
6591+
@Test
6592+
public void defineDiskForDefaultPoolTypeUsesDiskBusTypeDataForDataDiskWithoutWindowsAndUefi() {
6593+
Map<String, String> details = new HashMap<>();
6594+
Mockito.when(volume.getType()).thenReturn(Volume.Type.DATADISK);
6595+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6596+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
6597+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
6598+
}
6599+
6600+
@Test
6601+
public void defineDiskForDefaultPoolTypeUsesDiskBusTypeForRootDisk() {
6602+
Map<String, String> details = new HashMap<>();
6603+
Mockito.when(volume.getType()).thenReturn(Volume.Type.ROOT);
6604+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6605+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
6606+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE, DiskDef.DiskFmtType.QCOW2);
6607+
}
6608+
6609+
@Test
6610+
public void defineDiskForDefaultPoolTypeUsesSecureBootConfiguration() {
6611+
Map<String, String> details = new HashMap<>();
6612+
Mockito.when(volume.getType()).thenReturn(Volume.Type.ROOT);
6613+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6614+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, true, true, true, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, details);
6615+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DiskDef.DiskFmtType.QCOW2, true);
6616+
}
6617+
6618+
@Test
6619+
public void defineDiskForDefaultPoolTypeHandlesNullDetails() {
6620+
Mockito.when(volume.getType()).thenReturn(Volume.Type.DATADISK);
6621+
Mockito.when(physicalDisk.getPath()).thenReturn(PHYSICAL_DISK_PATH);
6622+
libvirtComputingResourceSpy.defineDiskForDefaultPoolType(diskDef, volume, false, false, false, physicalDisk, DEV_ID, DISK_BUS_TYPE, DISK_BUS_TYPE_DATA, null);
6623+
Mockito.verify(diskDef).defFileBasedDisk(PHYSICAL_DISK_PATH, DEV_ID, DISK_BUS_TYPE_DATA, DiskDef.DiskFmtType.QCOW2);
6624+
}
65686625
}

0 commit comments

Comments
 (0)