From 075f9811c4b1f785f268df00271cdb7b46d73d1d Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Thu, 19 Sep 2024 14:54:30 -0300 Subject: [PATCH] [VMware] Make disk controller selection on volume attachment consistent with VM creation and start (#9636) * Make volume attachment disk controller selection consistent with VM creation and start * Update vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java Co-authored-by: dahn * Choose disk controllers after converting osdefault * Rename function --------- Co-authored-by: dahn --- .../vmware/resource/VmwareResource.java | 53 +++---------- .../resource/VmwareStorageProcessor.java | 15 ++-- .../vmware/mo/HypervisorHostHelper.java | 23 +----- .../hypervisor/vmware/util/VmwareHelper.java | 75 +++++++++++++++++++ 4 files changed, 97 insertions(+), 69 deletions(-) diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java index 732d40da307..92821c7e26d 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java @@ -1975,16 +1975,8 @@ public class VmwareResource extends ServerResourceBase implements StoragePoolRes return; } - String msg; - String rootDiskController = controllerInfo.first(); - String dataDiskController = controllerInfo.second(); - String scsiDiskController; - String recommendedDiskController = null; - - if (VmwareHelper.isControllerOsRecommended(dataDiskController) || VmwareHelper.isControllerOsRecommended(rootDiskController)) { - recommendedDiskController = vmMo.getRecommendedDiskController(null); - } - scsiDiskController = HypervisorHostHelper.getScsiController(new Pair(rootDiskController, dataDiskController), recommendedDiskController); + Pair chosenDiskControllers = VmwareHelper.chooseRequiredDiskControllers(controllerInfo, vmMo, null, null); + String scsiDiskController = HypervisorHostHelper.getScsiController(chosenDiskControllers); if (scsiDiskController == null) { return; } @@ -2337,6 +2329,7 @@ public class VmwareResource extends ServerResourceBase implements StoragePoolRes } int controllerKey; + Pair chosenDiskControllers = VmwareHelper.chooseRequiredDiskControllers(controllerInfo,vmMo, null, null); // // Setup ROOT/DATA disk devices @@ -2361,10 +2354,7 @@ public class VmwareResource extends ServerResourceBase implements StoragePoolRes } VirtualMachineDiskInfo matchingExistingDisk = getMatchingExistingDisk(diskInfoBuilder, vol, hyperHost, context); - String diskController = getDiskController(vmMo, matchingExistingDisk, vol, controllerInfo, deployAsIs); - if (DiskControllerType.getType(diskController) == DiskControllerType.osdefault) { - diskController = vmMo.getRecommendedDiskController(null); - } + String diskController = getDiskController(vmMo, matchingExistingDisk, vol, chosenDiskControllers, deployAsIs); if (DiskControllerType.getType(diskController) == DiskControllerType.ide) { controllerKey = vmMo.getIDEControllerKey(ideUnitNumber); if (vol.getType() == Volume.Type.DATADISK) { @@ -2847,27 +2837,10 @@ public class VmwareResource extends ServerResourceBase implements StoragePoolRes } private Pair getControllerInfoFromVmSpec(VirtualMachineTO vmSpec) throws CloudRuntimeException { - String dataDiskController = vmSpec.getDetails().get(VmDetailConstants.DATA_DISK_CONTROLLER); - String rootDiskController = vmSpec.getDetails().get(VmDetailConstants.ROOT_DISK_CONTROLLER); - - // If root disk controller is scsi, then data disk controller would also be scsi instead of using 'osdefault' - // This helps avoid mix of different scsi subtype controllers in instance. - if (DiskControllerType.osdefault == DiskControllerType.getType(dataDiskController) && DiskControllerType.lsilogic == DiskControllerType.getType(rootDiskController)) { - dataDiskController = DiskControllerType.scsi.toString(); - } - - // Validate the controller types - dataDiskController = DiskControllerType.getType(dataDiskController).toString(); - rootDiskController = DiskControllerType.getType(rootDiskController).toString(); - - if (DiskControllerType.getType(rootDiskController) == DiskControllerType.none) { - throw new CloudRuntimeException("Invalid root disk controller detected : " + rootDiskController); - } - if (DiskControllerType.getType(dataDiskController) == DiskControllerType.none) { - throw new CloudRuntimeException("Invalid data disk controller detected : " + dataDiskController); - } - - return new Pair<>(rootDiskController, dataDiskController); + String rootDiskControllerDetail = vmSpec.getDetails().get(VmDetailConstants.ROOT_DISK_CONTROLLER); + String dataDiskControllerDetail = vmSpec.getDetails().get(VmDetailConstants.DATA_DISK_CONTROLLER); + VmwareHelper.validateDiskControllerDetails(rootDiskControllerDetail, dataDiskControllerDetail); + return new Pair<>(rootDiskControllerDetail, dataDiskControllerDetail); } private String getBootModeFromVmSpec(VirtualMachineTO vmSpec, boolean deployAsIs) { @@ -3615,15 +3588,7 @@ public class VmwareResource extends ServerResourceBase implements StoragePoolRes return controllerType.toString(); } - if (vol.getType() == Volume.Type.ROOT) { - s_logger.info("Chose disk controller for vol " + vol.getType() + " -> " + controllerInfo.first() - + ", based on root disk controller settings at global configuration setting."); - return controllerInfo.first(); - } else { - s_logger.info("Chose disk controller for vol " + vol.getType() + " -> " + controllerInfo.second() - + ", based on default data disk controller setting i.e. Operating system recommended."); // Need to bring in global configuration setting & template level setting. - return controllerInfo.second(); - } + return VmwareHelper.getControllerBasedOnDiskType(controllerInfo, vol); } private void postDiskConfigBeforeStart(VirtualMachineMO vmMo, VirtualMachineTO vmSpec, DiskTO[] sortedDisks, int ideControllerKey, diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java index 57522a678f8..40ebc4cb02a 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java @@ -2100,15 +2100,18 @@ public class VmwareStorageProcessor implements StorageProcessor { AttachAnswer answer = new AttachAnswer(disk); if (isAttach) { - String diskController = getLegacyVmDataDiskController(); - + String rootDiskControllerDetail = DiskControllerType.ide.toString(); + if (controllerInfo != null && StringUtils.isNotEmpty(controllerInfo.get(VmDetailConstants.ROOT_DISK_CONTROLLER))) { + rootDiskControllerDetail = controllerInfo.get(VmDetailConstants.ROOT_DISK_CONTROLLER); + } + String dataDiskControllerDetail = getLegacyVmDataDiskController(); if (controllerInfo != null && StringUtils.isNotEmpty(controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER))) { - diskController = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER); + dataDiskControllerDetail = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER); } - if (DiskControllerType.getType(diskController) == DiskControllerType.osdefault) { - diskController = vmMo.getRecommendedDiskController(null); - } + VmwareHelper.validateDiskControllerDetails(rootDiskControllerDetail, dataDiskControllerDetail); + Pair chosenDiskControllers = VmwareHelper.chooseRequiredDiskControllers(new Pair<>(rootDiskControllerDetail, dataDiskControllerDetail), vmMo, null, null); + String diskController = VmwareHelper.getControllerBasedOnDiskType(chosenDiskControllers, disk); vmMo.attachDisk(new String[] { datastoreVolumePath }, morDs, diskController, storagePolicyId, volumeTO.getIopsReadRate() + volumeTO.getIopsWriteRate()); VirtualMachineDiskInfoBuilder diskInfoBuilder = vmMo.getDiskInfoBuilder(); diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java index 12ef462ec8a..44965e9321b 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java @@ -1567,15 +1567,8 @@ public class HypervisorHostHelper { VmwareHelper.setBasicVmConfig(vmConfig, cpuCount, cpuSpeedMHz, cpuReservedMHz, memoryMB, memoryReserveMB, guestOsIdentifier, limitCpuUse, false); - String newRootDiskController = controllerInfo.first(); - String newDataDiskController = controllerInfo.second(); - String recommendedController = null; - if (VmwareHelper.isControllerOsRecommended(newRootDiskController) || VmwareHelper.isControllerOsRecommended(newDataDiskController)) { - recommendedController = host.getRecommendedDiskController(guestOsIdentifier); - } - - Pair updatedControllerInfo = new Pair(newRootDiskController, newDataDiskController); - String scsiDiskController = HypervisorHostHelper.getScsiController(updatedControllerInfo, recommendedController); + Pair chosenDiskControllers = VmwareHelper.chooseRequiredDiskControllers(controllerInfo, null, host, guestOsIdentifier); + String scsiDiskController = HypervisorHostHelper.getScsiController(chosenDiskControllers); // If there is requirement for a SCSI controller, ensure to create those. if (scsiDiskController != null) { int busNum = 0; @@ -2249,19 +2242,11 @@ public class HypervisorHostHelper { return morHyperHost; } - public static String getScsiController(Pair controllerInfo, String recommendedController) { + public static String getScsiController(Pair controllerInfo) { String rootDiskController = controllerInfo.first(); String dataDiskController = controllerInfo.second(); - // If "osdefault" is specified as controller type, then translate to actual recommended controller. - if (VmwareHelper.isControllerOsRecommended(rootDiskController)) { - rootDiskController = recommendedController; - } - if (VmwareHelper.isControllerOsRecommended(dataDiskController)) { - dataDiskController = recommendedController; - } - - String scsiDiskController = null; //If any of the controller provided is SCSI then return it's sub-type. + String scsiDiskController; //If any of the controller provided is SCSI then return it's sub-type. if (isIdeController(rootDiskController) && isIdeController(dataDiskController)) { //Default controllers would exist return null; diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java index 8a637721506..96b176df5f6 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java @@ -40,11 +40,14 @@ import javax.xml.datatype.DatatypeConfigurationException; import javax.xml.datatype.DatatypeFactory; import javax.xml.datatype.XMLGregorianCalendar; +import com.cloud.agent.api.to.DiskTO; import com.cloud.hypervisor.vmware.mo.ClusterMO; import com.cloud.hypervisor.vmware.mo.DatastoreFile; import com.cloud.hypervisor.vmware.mo.DistributedVirtualSwitchMO; import com.cloud.hypervisor.vmware.mo.HypervisorHostHelper; import com.cloud.serializer.GsonHelper; +import com.cloud.storage.Volume; +import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.NetUtils; import com.vmware.vim25.DatastoreInfo; import com.vmware.vim25.DistributedVirtualPort; @@ -1063,4 +1066,76 @@ public class VmwareHelper { } return vmdkAbsFile; } + + /** + * Validates an instance's rootDiskController and dataDiskController details. Throws a + * CloudRuntimeException if they are invalid. + */ + public static void validateDiskControllerDetails(String rootDiskControllerDetail, String dataDiskControllerDetail) { + rootDiskControllerDetail = DiskControllerType.getType(rootDiskControllerDetail).toString(); + if (DiskControllerType.getType(rootDiskControllerDetail) == DiskControllerType.none) { + throw new CloudRuntimeException(String.format("[%s] is not a valid root disk controller", rootDiskControllerDetail)); + } + dataDiskControllerDetail = DiskControllerType.getType(dataDiskControllerDetail).toString(); + if (DiskControllerType.getType(dataDiskControllerDetail) == DiskControllerType.none) { + throw new CloudRuntimeException(String.format("[%s] is not a valid data disk controller", dataDiskControllerDetail)); + } + } + + /** + * Based on an instance's rootDiskController and dataDiskController details, returns a pair + * containing the disk controllers that should be used for root disk and the data disks, respectively. + * + * @param controllerInfo pair containing the root disk and data disk controllers, respectively. + * @param vmMo virtual machine to derive the recommended disk controllers from. If not null, host and guestOsIdentifier will be ignored. + * @param host host to derive the recommended disk controllers from. Must be provided with guestOsIdentifier. + * @param guestOsIdentifier used to derive the recommended disk controllers from the host. + */ + public static Pair chooseRequiredDiskControllers(Pair controllerInfo, VirtualMachineMO vmMo, + VmwareHypervisorHost host, String guestOsIdentifier) throws Exception { + String recommendedDiskControllerClassName = vmMo != null ? vmMo.getRecommendedDiskController(null) : host.getRecommendedDiskController(guestOsIdentifier); + String recommendedDiskController = DiskControllerType.getType(recommendedDiskControllerClassName).toString(); + + String convertedRootDiskController = controllerInfo.first(); + if (isControllerOsRecommended(convertedRootDiskController)) { + convertedRootDiskController = recommendedDiskController; + } + + String convertedDataDiskController = controllerInfo.second(); + if (isControllerOsRecommended(convertedDataDiskController)) { + convertedDataDiskController = recommendedDiskController; + } + + if (diskControllersShareTheSameBusType(convertedRootDiskController, convertedDataDiskController)) { + s_logger.debug("Root and data disk controllers share the same bus type; therefore, we will only use the controllers specified for the root disk."); + return new Pair<>(convertedRootDiskController, convertedRootDiskController); + } + + return new Pair<>(convertedRootDiskController, convertedDataDiskController); + } + + protected static boolean diskControllersShareTheSameBusType(String rootDiskController, String dataDiskController) { + DiskControllerType rootDiskControllerType = DiskControllerType.getType(rootDiskController); + DiskControllerType dataDiskControllerType = DiskControllerType.getType(dataDiskController); + if (rootDiskControllerType.equals(dataDiskControllerType)) { + return true; + } + List scsiDiskControllers = List.of(DiskControllerType.scsi, DiskControllerType.lsilogic, DiskControllerType.lsisas1068, + DiskControllerType.buslogic ,DiskControllerType.pvscsi); + return scsiDiskControllers.contains(rootDiskControllerType) && scsiDiskControllers.contains(dataDiskControllerType); + } + + /** + * Identifies whether the disk is a root or data disk, and returns the controller from the provided pair that should + * be used for the disk. + * @param controllerInfo pair containing the root disk and data disk controllers, respectively. + */ + public static String getControllerBasedOnDiskType(Pair controllerInfo, DiskTO disk) { + if (disk.getType() == Volume.Type.ROOT || disk.getDiskSeq() == 0) { + s_logger.debug(String.format("Choosing disk controller [%s] for the root disk.", controllerInfo.first())); + return controllerInfo.first(); + } + s_logger.debug(String.format("Choosing disk controller [%s] for the data disks.", controllerInfo.second())); + return controllerInfo.second(); + } }