[VMware] Make disk controller selection on volume attachment consiste…

…nt 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 <[email protected]> * Choose disk controllers after converting osdefault * Rename function --------- Co-authored-by: dahn <[email protected]>
apache · Sep 19, 2024 · 075f981 · 075f981
1 parent 7f2ab63
commit 075f981
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 69 deletions.
diff --git a/...hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java b/...hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
@@ -1975,16 +1975,8 @@ protected void ensureDiskControllers(VirtualMachineMO vmMo, Pair<String, String>
             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<String, String>(rootDiskController, dataDiskController), recommendedDiskController);
+        Pair<String, String> chosenDiskControllers = VmwareHelper.chooseRequiredDiskControllers(controllerInfo, vmMo, null, null);
+        String scsiDiskController = HypervisorHostHelper.getScsiController(chosenDiskControllers);
         if (scsiDiskController == null) {
             return;
         }
@@ -2337,6 +2329,7 @@ protected StartAnswer execute(StartCommand cmd) {
             }
 
             int controllerKey;
+            Pair<String, String> chosenDiskControllers = VmwareHelper.chooseRequiredDiskControllers(controllerInfo,vmMo, null, null);
 
             //
             // Setup ROOT/DATA disk devices
@@ -2361,10 +2354,7 @@ protected StartAnswer execute(StartCommand cmd) {
                 }
 
                 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 @@ private String getGuestOsIdFromVmSpec(VirtualMachineTO vmSpec, boolean deployAsI
     }
 
     private Pair<String, String> 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 @@ private String getDiskController(VirtualMachineMO vmMo, VirtualMachineDiskInfo m
             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/...s/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java b/...s/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java
@@ -2100,15 +2100,18 @@ private Answer attachVolume(Command cmd, DiskTO disk, boolean isAttach, boolean
             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<String, String> 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
@@ -1567,15 +1567,8 @@ public static boolean createBlankVm(VmwareHypervisorHost host, String vmName, St
 
         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<String, String> updatedControllerInfo = new Pair<String, String>(newRootDiskController, newDataDiskController);
-        String scsiDiskController = HypervisorHostHelper.getScsiController(updatedControllerInfo, recommendedController);
+        Pair<String, String> 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 static ManagedObjectReference getHypervisorHostMorFromGuid(String guid) {
         return morHyperHost;
     }
 
-    public static String getScsiController(Pair<String, String> controllerInfo, String recommendedController) {
+    public static String getScsiController(Pair<String, String> 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
@@ -40,11 +40,14 @@
 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 static String getAbsoluteVmdkFile(VirtualDisk disk) {
         }
         return vmdkAbsFile;
     }
+
+    /**
+     * Validates an instance's <code>rootDiskController</code> and <code>dataDiskController</code> details. Throws a
+     * <code>CloudRuntimeException</code> 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 <code>rootDiskController</code> and <code>dataDiskController</code> 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, <code>host</code> and <code>guestOsIdentifier</code> will be ignored.
+     * @param host              host to derive the recommended disk controllers from. Must be provided with <code>guestOsIdentifier</code>.
+     * @param guestOsIdentifier used to derive the recommended disk controllers from the host.
+     */
+    public static Pair<String, String> chooseRequiredDiskControllers(Pair<String, String> 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<DiskControllerType> 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<String, String> 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();
+    }
 }