Copilot commented on code in PR #10454: URL: https://github.com/apache/cloudstack/pull/10454#discussion_r3008887357
########## engine/schema/src/main/java/org/apache/cloudstack/storage/DiskControllerMappingVO.java: ########## @@ -0,0 +1,205 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package org.apache.cloudstack.storage; + +import com.cloud.hypervisor.Hypervisor.HypervisorType; +import com.cloud.utils.db.GenericDao; +import org.apache.cloudstack.util.HypervisorTypeConverter; +import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; + +import javax.persistence.Column; +import javax.persistence.Convert; +import javax.persistence.Entity; +import javax.persistence.GeneratedValue; +import javax.persistence.GenerationType; +import javax.persistence.Id; +import javax.persistence.Table; +import javax.persistence.Temporal; +import javax.persistence.TemporalType; +import java.util.Date; +import java.util.UUID; + +@Entity +@Table(name = "disk_controller_mapping") +public class DiskControllerMappingVO implements DiskControllerMapping { + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + @Column(name = "id", nullable = false) + private Long id; + + @Column(name = "uuid", nullable = false) + private String uuid = UUID.randomUUID().toString(); + + @Column(name = "name", nullable = false) + private String name; + + @Column(name = "controller_reference", nullable = false) + private String controllerReference; + + @Column(name = "bus_name", nullable = false) + private String busName; + + @Column(name = "hypervisor", nullable = false) + @Convert(converter = HypervisorTypeConverter.class) + private HypervisorType hypervisor; + + @Column(name = "max_device_count") + private Integer maxDeviceCount = null; + + @Column(name = "max_controller_count") + private Integer maxControllerCount = null; + + @Column(name = "vmdk_adapter_type") + private String vmdkAdapterType = null; + + @Column(name = "min_hardware_version") + private String minHardwareVersion = null; + + @Column(name = GenericDao.CREATED_COLUMN, nullable = false) + @Temporal(value = TemporalType.TIMESTAMP) + private Date created; + + @Column(name = GenericDao.REMOVED_COLUMN) + @Temporal(value = TemporalType.TIMESTAMP) + private Date removed = null; + + public DiskControllerMappingVO() { + } + + @Override + public String getName() { + return name; + } + + @Override + public String getControllerReference() { + return controllerReference; + } + + @Override + public String getBusName() { + return busName; + } + + @Override + public HypervisorType getHypervisor() { + return hypervisor; + } + + @Override + public Integer getMaxDeviceCount() { + return maxDeviceCount; + } + + @Override + public Integer getMaxControllerCount() { + return maxControllerCount; + } + + @Override + public String getVmdkAdapterType() { + return vmdkAdapterType; + } + + @Override + public String getMinHardwareVersion() { + return minHardwareVersion; + } + + @Override + public Date getRemoved() { + return removed; + } + + @Override + public Date getCreated() { + return created; + } + + @Override + public String getUuid() { + return uuid; + } + + @Override + public long getId() { + return id; + } + + public void setId(Long id) { + this.id = id; + } + + public void setUuid(String uuid) { + this.uuid = uuid; + } + + public void setName(String name) { + this.name = name; + } + + public void setControllerReference(String controllerReference) { + this.controllerReference = controllerReference; + } + + public void setBusName(String busName) { + this.busName = busName; + } + + public void setHypervisor(HypervisorType hypervisor) { + this.hypervisor = hypervisor; + } + + public void setMaxDeviceCount(Integer maxDeviceCount) { + this.maxDeviceCount = maxDeviceCount; + } + + public void setMaxControllerCount(Integer maxControllerCount) { + this.maxControllerCount = maxControllerCount; + } + + public void setVmdkAdapterType(String vmdkAdapterType) { + this.vmdkAdapterType = vmdkAdapterType; + } + + public void setMinHardwareVersion(String minHardwareVersion) { + this.minHardwareVersion = minHardwareVersion; + } + + public void setCreated(Date created) { + this.created = created; + } + + public void setRemoved(Date removed) { + this.removed = removed; + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof DiskControllerMappingVO)) { + return false; + } + DiskControllerMappingVO that = (DiskControllerMappingVO) obj; + return controllerReference.equals(that.getControllerReference()); + } + + @Override Review Comment: `equals()` is overridden but `hashCode()` is not. This breaks the general contract for hash-based collections (e.g., `HashSet`/`HashMap`) and can cause duplicate mappings or failed lookups when mappings are compared/collected. Add a `hashCode()` implementation consistent with `equals()` (or switch to `Objects.hash(controllerReference)`). ```suggestion @Override public int hashCode() { return controllerReference != null ? controllerReference.hashCode() : 0; } @Override ``` ########## vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java: ########## @@ -1097,74 +1119,168 @@ public static String getAbsoluteVmdkFile(VirtualDisk disk) { } /** - * Validates an instance's <code>rootDiskController</code> and <code>dataDiskController</code> details. Throws a - * <code>CloudRuntimeException</code> if they are invalid. + * Returns a disk controller mapping that has the provided name or the provided controller reference. + * @throws CloudRuntimeException if there is no configured disk controller matching the provided params. */ - 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)); + public static DiskControllerMappingVO getDiskControllerMapping(String name, String controllerReference) { + for (DiskControllerMappingVO mapping : getAllSupportedDiskControllerMappings()) { + if (mapping.getName().equals(name) || mapping.getControllerReference().equals(controllerReference)) { + return mapping; + } } - 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)); + LOGGER.debug("No corresponding disk controller mapping found for name [{}] and controller reference [{}].", name, controllerReference); + throw new CloudRuntimeException("Specified disk controller is invalid."); + } + + public static List<DiskControllerMappingVO> getAllSupportedDiskControllerMappings() { + return supportedDiskControllers; + } + + public static List<DiskControllerMappingVO> getAllSupportedDiskControllerMappingsExceptOsDefault() { + return getAllSupportedDiskControllerMappings().stream() + .filter(c -> !isControllerOsRecommended(c)) + .collect(Collectors.toList()); + } + + /** + * Returns a set containing the disk controllers an instance should have based on the provided params. + * @param controllerInfo pair containing the root disk and data disk controllers, respectively. + * @param isSystemVm if true, only the root disk controller is required; otherwise, both controllers are required. + */ + public static Set<DiskControllerMappingVO> getRequiredDiskControllers(Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo, + boolean isSystemVm) { + Set<DiskControllerMappingVO> requiredDiskControllers = new HashSet<>(); + requiredDiskControllers.add(controllerInfo.first()); + if (!isSystemVm) { + requiredDiskControllers.add(controllerInfo.second()); } + return requiredDiskControllers; + } + + /** + * Instructs the provided <code>VirtualMachineConfigSpec</code> to create the disk controllers contained in <code>requiredDiskControllers</code>. + * @param isSystemVm if true, only one disk controller of each type will be created; otherwise, the maximum amount of each controller will be created. + * @throws Exception if a disk controller mapping has an invalid controller reference. + */ + public static void addDiskControllersToVmConfigSpec(VirtualMachineConfigSpec vmConfigSpec, Set<DiskControllerMappingVO> requiredDiskControllers, + boolean isSystemVm) throws Exception { + int currentKey = -1; + + for (DiskControllerMappingVO diskController : requiredDiskControllers) { + Class<?> controllerClass = Class.forName(diskController.getControllerReference()); + if (controllerClass == VirtualIDEController.class) { + continue; + } + for (int bus = 0; bus < diskController.getMaxControllerCount(); bus++) { + VirtualController controller = (VirtualController) controllerClass.newInstance(); Review Comment: `Class#newInstance()` is deprecated and can fail in surprising ways (e.g., with non-public constructors). Use `getDeclaredConstructor().newInstance()` instead, and consider surfacing a clearer exception if instantiation fails for a configured mapping. ```suggestion VirtualController controller = (VirtualController) controllerClass.getDeclaredConstructor().newInstance(); ``` ########## plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java: ########## @@ -2102,18 +2102,19 @@ private Answer attachVolume(Command cmd, DiskTO disk, boolean isAttach, boolean AttachAnswer answer = new AttachAnswer(disk); if (isAttach) { - 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))) { - dataDiskControllerDetail = controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER); - } - - VmwareHelper.validateDiskControllerDetails(rootDiskControllerDetail, dataDiskControllerDetail); - Pair<String, String> chosenDiskControllers = VmwareHelper.chooseRequiredDiskControllers(new Pair<>(rootDiskControllerDetail, dataDiskControllerDetail), vmMo, null, null); - String diskController = VmwareHelper.getControllerBasedOnDiskType(chosenDiskControllers, disk); + // Let's first find which disk controller should be used for the volume being attached. + // + // `controllerInfo` can not be null here. It is always defined when creating the `AttachComand` in Review Comment: Typo in comment: `AttachComand` should be `AttachCommand`. ```suggestion // `controllerInfo` can not be null here. It is always defined when creating the `AttachCommand` in ``` ########## vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java: ########## @@ -108,6 +171,304 @@ public void testGetUnmanageInstanceDisks() { Assert.assertEquals(dataStoreName, disk.getDatastoreName()); } + @Test + public void isControllerOsRecommendedTestControllerIsOsRecommendedReturnsTrue() { + DiskControllerMappingVO mapping = new DiskControllerMappingVO(); + mapping.setName("osdefault"); + + boolean result = VmwareHelper.isControllerOsRecommended(mapping); + + Assert.assertTrue(result); + } + + @Test + public void isControllerOsRecommendedTestControllerIsNotOsRecommendedReturnsFalse() { + DiskControllerMappingVO mapping = new DiskControllerMappingVO(); + mapping.setName("lsilogic"); + + boolean result = VmwareHelper.isControllerOsRecommended(mapping); + + Assert.assertFalse(result); + } + + @Test + public void isControllerScsiTestControllerIsScsiReturnsTrue() { + DiskControllerMappingVO mapping = new DiskControllerMappingVO(); + mapping.setBusName("scsi"); + + boolean result = VmwareHelper.isControllerScsi(mapping); + + Assert.assertTrue(result); + } + + @Test + public void isControllerScsiTestControllerIsNotScsiReturnsFalse() { + DiskControllerMappingVO mapping = new DiskControllerMappingVO(); + mapping.setBusName("nvme"); + + boolean result = VmwareHelper.isControllerScsi(mapping); + + Assert.assertFalse(result); + } + + @Test + public void getDiskControllerMappingTestSearchByExistingNameReturnsObject() { + String name = "lsilogic"; + + DiskControllerMappingVO mapping = VmwareHelper.getDiskControllerMapping(name, null); + + Assert.assertEquals(name, mapping.getName()); + } + + @Test + public void getDiskControllerMappingTestSearchByExistingControllerReferenceReturnsObject() { + String classpath = VirtualLsiLogicController.class.getName(); + + DiskControllerMappingVO mapping = VmwareHelper.getDiskControllerMapping(null, classpath); + + Assert.assertEquals(classpath, mapping.getControllerReference()); + } + + @Test(expected = CloudRuntimeException.class) + public void getDiskControllerMappingTestThrowsExceptionWhenNoMatches() { + VmwareHelper.getDiskControllerMapping("invalid", "invalid"); + } + + @Test + public void getAllDiskControllerMappingsExceptOsDefaultTestReturnDoesNotContainOsDefaultMapping() { + List<DiskControllerMappingVO> result = VmwareHelper.getAllSupportedDiskControllerMappingsExceptOsDefault(); + + DiskControllerMappingVO osdefaultMapping = VmwareHelper.getDiskControllerMapping("osdefault", null); + Assert.assertFalse(result.contains(osdefaultMapping)); + Assert.assertFalse(result.isEmpty()); + } + + @Test + public void getRequiredDiskControllersTestRequiresAllControllersWhenInstanceIsNotSystemVm() { + Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo = new Pair<>(new DiskControllerMappingVO(), new DiskControllerMappingVO()); + + Set<DiskControllerMappingVO> result = VmwareHelper.getRequiredDiskControllers(controllerInfo, false); + + Assert.assertTrue(result.contains(controllerInfo.first())); + Assert.assertTrue(result.contains(controllerInfo.second())); + } + + @Test + public void getRequiredDiskControllersTestRequiresOnlyRootDiskControllerWhenInstanceIsSystemVm() { + Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo = new Pair<>(new DiskControllerMappingVO(), new DiskControllerMappingVO()); + + Set<DiskControllerMappingVO> result = VmwareHelper.getRequiredDiskControllers(controllerInfo, true); + + Assert.assertTrue(result.contains(controllerInfo.first())); + Assert.assertFalse(result.contains(controllerInfo.second())); + } + + @Test + public void chooseDiskControllersDiskControllersTestControllersAreNotOsRecommendedReturnsProvidedControllers() throws Exception { + DiskControllerMappingVO nvmeMapping = VmwareHelper.getDiskControllerMapping("nvme", null); + DiskControllerMappingVO sataMapping = VmwareHelper.getDiskControllerMapping("sata", null); + Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo = new Pair<>(nvmeMapping, sataMapping); + Mockito.doReturn("VirtualLsiLogicController").when(virtualMachineMO).getRecommendedDiskController(null); + + Pair<DiskControllerMappingVO, DiskControllerMappingVO> result = VmwareHelper.chooseDiskControllers(controllerInfo, virtualMachineMO, null, null); + + Assert.assertEquals(nvmeMapping, result.first()); + Assert.assertEquals(sataMapping, result.second()); + } + + @Test + public void chooseDiskControllersTestControllersAreOsRecommendedAndVmMoIsProvidedReturnsConvertedControllersBasedOnVmMo() throws Exception { + DiskControllerMappingVO osdefaultMapping = VmwareHelper.getDiskControllerMapping("osdefault", null); + Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo = new Pair<>(osdefaultMapping, osdefaultMapping); + Mockito.doReturn("VirtualLsiLogicController").when(virtualMachineMO).getRecommendedDiskController(null); + + Pair<DiskControllerMappingVO, DiskControllerMappingVO> result = VmwareHelper.chooseDiskControllers(controllerInfo, virtualMachineMO, null, null); + + DiskControllerMappingVO lsilogicMapping = VmwareHelper.getDiskControllerMapping("lsilogic", null); + Assert.assertEquals(lsilogicMapping, result.first()); + Assert.assertEquals(lsilogicMapping, result.second()); + } + + @Test + public void chooseDiskControllersTestControllersAreOsRecommendedAndHostIsProvidedReturnsConvertedControllersBasedOnHost() throws Exception { + DiskControllerMappingVO osdefaultMapping = VmwareHelper.getDiskControllerMapping("osdefault", null); + Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo = new Pair<>(osdefaultMapping, osdefaultMapping); + String guestOsId = "guestOsId"; + Mockito.doReturn("VirtualLsiLogicController").when(vmwareHypervisorHostMock).getRecommendedDiskController(guestOsId); + + Pair<DiskControllerMappingVO, DiskControllerMappingVO> result = VmwareHelper.chooseDiskControllers(controllerInfo, null, vmwareHypervisorHostMock, guestOsId); + + DiskControllerMappingVO lsilogicMapping = VmwareHelper.getDiskControllerMapping("lsilogic", null); + Assert.assertEquals(lsilogicMapping, result.first()); + Assert.assertEquals(lsilogicMapping, result.second()); + } + + @Test + public void chooseDiskControllersTestControllersShareTheSameBusTypeReturnsRootDiskController() throws Exception { + DiskControllerMappingVO osdefaultMapping = VmwareHelper.getDiskControllerMapping("osdefault", null); + DiskControllerMappingVO pvscsiMapping = VmwareHelper.getDiskControllerMapping("pvscsi", null); + Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo = new Pair<>(osdefaultMapping, pvscsiMapping); + Mockito.doReturn("VirtualLsiLogicController").when(virtualMachineMO).getRecommendedDiskController(null); + + Pair<DiskControllerMappingVO, DiskControllerMappingVO> result = VmwareHelper.chooseDiskControllers(controllerInfo, virtualMachineMO, null, null); + + DiskControllerMappingVO lsilogicMapping = VmwareHelper.getDiskControllerMapping("lsilogic", null); + Assert.assertEquals(lsilogicMapping, result.first()); + Assert.assertEquals(lsilogicMapping, result.second()); + } + + @Test + public void addDiskControllersToVmConfigSpecTestDoesNotAddIdeControllers() throws Exception { + DiskControllerMappingVO ideMapping = VmwareHelper.getDiskControllerMapping("ide", null); + Set<DiskControllerMappingVO> requiredControllers = new HashSet<>(); + requiredControllers.add(ideMapping); + + VmwareHelper.addDiskControllersToVmConfigSpec(virtualMachineConfigSpecMock, requiredControllers, false); + + Assert.assertEquals(0, virtualMachineConfigSpecMock.getDeviceChange().size()); + } + + @Test + public void addDiskControllersToVmConfigSpecTestMaximumAmmountOfControllersIsAdded() throws Exception { + DiskControllerMappingVO nvmeMapping = VmwareHelper.getDiskControllerMapping("nvme", null); + DiskControllerMappingVO sataMapping = VmwareHelper.getDiskControllerMapping("sata", null); + Set<DiskControllerMappingVO> requiredControllers = new HashSet<>(); + requiredControllers.add(nvmeMapping); + requiredControllers.add(sataMapping); + + VmwareHelper.addDiskControllersToVmConfigSpec(virtualMachineConfigSpecMock, requiredControllers, false); + + int expectedControllerAmmount = nvmeMapping.getMaxControllerCount() + sataMapping.getMaxControllerCount(); + Assert.assertEquals(expectedControllerAmmount, virtualMachineConfigSpecMock.getDeviceChange().size()); Review Comment: Typo in test/variable naming: "Ammount" should be "Amount" (e.g., `expectedControllerAmount`, `...MaximumAmount...`). Renaming improves readability and avoids propagating the typo to future tests. ########## vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java: ########## @@ -1097,74 +1119,168 @@ public static String getAbsoluteVmdkFile(VirtualDisk disk) { } /** - * Validates an instance's <code>rootDiskController</code> and <code>dataDiskController</code> details. Throws a - * <code>CloudRuntimeException</code> if they are invalid. + * Returns a disk controller mapping that has the provided name or the provided controller reference. + * @throws CloudRuntimeException if there is no configured disk controller matching the provided params. */ - 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)); + public static DiskControllerMappingVO getDiskControllerMapping(String name, String controllerReference) { + for (DiskControllerMappingVO mapping : getAllSupportedDiskControllerMappings()) { + if (mapping.getName().equals(name) || mapping.getControllerReference().equals(controllerReference)) { + return mapping; + } } - 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)); + LOGGER.debug("No corresponding disk controller mapping found for name [{}] and controller reference [{}].", name, controllerReference); + throw new CloudRuntimeException("Specified disk controller is invalid."); + } + + public static List<DiskControllerMappingVO> getAllSupportedDiskControllerMappings() { + return supportedDiskControllers; + } + + public static List<DiskControllerMappingVO> getAllSupportedDiskControllerMappingsExceptOsDefault() { + return getAllSupportedDiskControllerMappings().stream() + .filter(c -> !isControllerOsRecommended(c)) + .collect(Collectors.toList()); + } + + /** + * Returns a set containing the disk controllers an instance should have based on the provided params. + * @param controllerInfo pair containing the root disk and data disk controllers, respectively. + * @param isSystemVm if true, only the root disk controller is required; otherwise, both controllers are required. + */ + public static Set<DiskControllerMappingVO> getRequiredDiskControllers(Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo, + boolean isSystemVm) { + Set<DiskControllerMappingVO> requiredDiskControllers = new HashSet<>(); + requiredDiskControllers.add(controllerInfo.first()); + if (!isSystemVm) { + requiredDiskControllers.add(controllerInfo.second()); } + return requiredDiskControllers; + } + + /** + * Instructs the provided <code>VirtualMachineConfigSpec</code> to create the disk controllers contained in <code>requiredDiskControllers</code>. + * @param isSystemVm if true, only one disk controller of each type will be created; otherwise, the maximum amount of each controller will be created. + * @throws Exception if a disk controller mapping has an invalid controller reference. + */ + public static void addDiskControllersToVmConfigSpec(VirtualMachineConfigSpec vmConfigSpec, Set<DiskControllerMappingVO> requiredDiskControllers, + boolean isSystemVm) throws Exception { + int currentKey = -1; + + for (DiskControllerMappingVO diskController : requiredDiskControllers) { + Class<?> controllerClass = Class.forName(diskController.getControllerReference()); + if (controllerClass == VirtualIDEController.class) { + continue; + } + for (int bus = 0; bus < diskController.getMaxControllerCount(); bus++) { + VirtualController controller = (VirtualController) controllerClass.newInstance(); + controller.setBusNumber(bus); + controller.setKey(currentKey); + currentKey--; + + if (controller instanceof VirtualSCSIController) { + ((VirtualSCSIController) controller).setSharedBus(VirtualSCSISharing.NO_SHARING); + } + + VirtualDeviceConfigSpec controllerConfigSpec = new VirtualDeviceConfigSpec(); + controllerConfigSpec.setDevice(controller); + controllerConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.ADD); + vmConfigSpec.getDeviceChange().add(controllerConfigSpec); + + if (isSystemVm) { + break; + } + } + } + } + + /** + * Based on an instance's <code>rootDiskController</code> and <code>dataDiskController</code> details, returns a pair + * containing the disk controller mappings that should be used for root disk and the data disks, respectively. + * @param isSystemVm if true, the root disk controller detail will be ignored, and the chosen root disk controller will be <code>lsilogic</code>. + */ + public static Pair<DiskControllerMappingVO, DiskControllerMappingVO> getDiskControllersFromVmSettings(String rootDiskControllerDetail, + String dataDiskControllerDetail, + boolean isSystemVm) { + String updatedRootDiskControllerDetail = isSystemVm ? DiskControllerType.lsilogic.toString() : ObjectUtils.defaultIfNull(rootDiskControllerDetail, DiskControllerType.osdefault.toString()); + String updatedDataDiskControllerDetail = ObjectUtils.defaultIfNull(dataDiskControllerDetail, updatedRootDiskControllerDetail); + + DiskControllerMappingVO specifiedRootDiskController = getDiskControllerMapping(updatedRootDiskControllerDetail, null); + DiskControllerMappingVO specifiedDataDiskController = getDiskControllerMapping(updatedDataDiskControllerDetail, null); + + return new Pair<>(specifiedRootDiskController, specifiedDataDiskController); } /** * 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 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. + * @throws Exception if no disk controller mapping matching the recommended disk controller exists. */ - public static Pair<String, String> chooseRequiredDiskControllers(Pair<String, String> controllerInfo, VirtualMachineMO vmMo, - VmwareHypervisorHost host, String guestOsIdentifier) throws Exception { + public static Pair<DiskControllerMappingVO, DiskControllerMappingVO> chooseDiskControllers(Pair<DiskControllerMappingVO, DiskControllerMappingVO> 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(); + DiskControllerMappingVO recommendedDiskController = getAllSupportedDiskControllerMappingsExceptOsDefault().stream() + .filter(c -> c.getControllerReference().contains(recommendedDiskControllerClassName)) + .findFirst() + .orElseThrow(() -> new CloudRuntimeException("Recommended disk controller is not mapped.")); Review Comment: Selecting the recommended controller mapping via `controllerReference.contains(recommendedDiskControllerClassName)` + `findFirst()` is ambiguous when multiple mappings share the same controller reference (e.g., the default DB mappings include both `scsi` and `lsilogic` for `VirtualLsiLogicController`). This can make the chosen mapping depend on DB/list ordering. Prefer an unambiguous match (e.g., exact class name match) and/or a deterministic tie-breaker (e.g., prefer `lsilogic` over the legacy alias) to keep behavior stable. ########## vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java: ########## @@ -1097,74 +1119,168 @@ public static String getAbsoluteVmdkFile(VirtualDisk disk) { } /** - * Validates an instance's <code>rootDiskController</code> and <code>dataDiskController</code> details. Throws a - * <code>CloudRuntimeException</code> if they are invalid. + * Returns a disk controller mapping that has the provided name or the provided controller reference. + * @throws CloudRuntimeException if there is no configured disk controller matching the provided params. */ - 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)); + public static DiskControllerMappingVO getDiskControllerMapping(String name, String controllerReference) { + for (DiskControllerMappingVO mapping : getAllSupportedDiskControllerMappings()) { + if (mapping.getName().equals(name) || mapping.getControllerReference().equals(controllerReference)) { + return mapping; + } } - 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)); + LOGGER.debug("No corresponding disk controller mapping found for name [{}] and controller reference [{}].", name, controllerReference); + throw new CloudRuntimeException("Specified disk controller is invalid."); + } + + public static List<DiskControllerMappingVO> getAllSupportedDiskControllerMappings() { + return supportedDiskControllers; + } + + public static List<DiskControllerMappingVO> getAllSupportedDiskControllerMappingsExceptOsDefault() { + return getAllSupportedDiskControllerMappings().stream() + .filter(c -> !isControllerOsRecommended(c)) + .collect(Collectors.toList()); + } + + /** + * Returns a set containing the disk controllers an instance should have based on the provided params. + * @param controllerInfo pair containing the root disk and data disk controllers, respectively. + * @param isSystemVm if true, only the root disk controller is required; otherwise, both controllers are required. + */ + public static Set<DiskControllerMappingVO> getRequiredDiskControllers(Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo, + boolean isSystemVm) { + Set<DiskControllerMappingVO> requiredDiskControllers = new HashSet<>(); + requiredDiskControllers.add(controllerInfo.first()); + if (!isSystemVm) { + requiredDiskControllers.add(controllerInfo.second()); } + return requiredDiskControllers; + } + + /** + * Instructs the provided <code>VirtualMachineConfigSpec</code> to create the disk controllers contained in <code>requiredDiskControllers</code>. + * @param isSystemVm if true, only one disk controller of each type will be created; otherwise, the maximum amount of each controller will be created. + * @throws Exception if a disk controller mapping has an invalid controller reference. + */ + public static void addDiskControllersToVmConfigSpec(VirtualMachineConfigSpec vmConfigSpec, Set<DiskControllerMappingVO> requiredDiskControllers, + boolean isSystemVm) throws Exception { + int currentKey = -1; + + for (DiskControllerMappingVO diskController : requiredDiskControllers) { + Class<?> controllerClass = Class.forName(diskController.getControllerReference()); + if (controllerClass == VirtualIDEController.class) { + continue; + } + for (int bus = 0; bus < diskController.getMaxControllerCount(); bus++) { + VirtualController controller = (VirtualController) controllerClass.newInstance(); + controller.setBusNumber(bus); + controller.setKey(currentKey); + currentKey--; + + if (controller instanceof VirtualSCSIController) { + ((VirtualSCSIController) controller).setSharedBus(VirtualSCSISharing.NO_SHARING); + } + + VirtualDeviceConfigSpec controllerConfigSpec = new VirtualDeviceConfigSpec(); + controllerConfigSpec.setDevice(controller); + controllerConfigSpec.setOperation(VirtualDeviceConfigSpecOperation.ADD); + vmConfigSpec.getDeviceChange().add(controllerConfigSpec); + + if (isSystemVm) { + break; + } + } + } + } + + /** + * Based on an instance's <code>rootDiskController</code> and <code>dataDiskController</code> details, returns a pair + * containing the disk controller mappings that should be used for root disk and the data disks, respectively. + * @param isSystemVm if true, the root disk controller detail will be ignored, and the chosen root disk controller will be <code>lsilogic</code>. + */ + public static Pair<DiskControllerMappingVO, DiskControllerMappingVO> getDiskControllersFromVmSettings(String rootDiskControllerDetail, + String dataDiskControllerDetail, + boolean isSystemVm) { + String updatedRootDiskControllerDetail = isSystemVm ? DiskControllerType.lsilogic.toString() : ObjectUtils.defaultIfNull(rootDiskControllerDetail, DiskControllerType.osdefault.toString()); + String updatedDataDiskControllerDetail = ObjectUtils.defaultIfNull(dataDiskControllerDetail, updatedRootDiskControllerDetail); + + DiskControllerMappingVO specifiedRootDiskController = getDiskControllerMapping(updatedRootDiskControllerDetail, null); + DiskControllerMappingVO specifiedDataDiskController = getDiskControllerMapping(updatedDataDiskControllerDetail, null); + + return new Pair<>(specifiedRootDiskController, specifiedDataDiskController); } /** * 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 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. + * @throws Exception if no disk controller mapping matching the recommended disk controller exists. */ - public static Pair<String, String> chooseRequiredDiskControllers(Pair<String, String> controllerInfo, VirtualMachineMO vmMo, - VmwareHypervisorHost host, String guestOsIdentifier) throws Exception { + public static Pair<DiskControllerMappingVO, DiskControllerMappingVO> chooseDiskControllers(Pair<DiskControllerMappingVO, DiskControllerMappingVO> 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(); + DiskControllerMappingVO recommendedDiskController = getAllSupportedDiskControllerMappingsExceptOsDefault().stream() + .filter(c -> c.getControllerReference().contains(recommendedDiskControllerClassName)) + .findFirst() + .orElseThrow(() -> new CloudRuntimeException("Recommended disk controller is not mapped.")); - String convertedRootDiskController = controllerInfo.first(); + DiskControllerMappingVO convertedRootDiskController = controllerInfo.first(); if (isControllerOsRecommended(convertedRootDiskController)) { convertedRootDiskController = recommendedDiskController; } - String convertedDataDiskController = controllerInfo.second(); + DiskControllerMappingVO convertedDataDiskController = controllerInfo.second(); if (isControllerOsRecommended(convertedDataDiskController)) { convertedDataDiskController = recommendedDiskController; } - if (diskControllersShareTheSameBusType(convertedRootDiskController, convertedDataDiskController)) { + if (convertedRootDiskController.getBusName().equals(convertedDataDiskController.getBusName())) { 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) { + public static DiskControllerMappingVO getControllerBasedOnDiskType(Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo, + DiskTO disk) { if (disk.getType() == Volume.Type.ROOT || disk.getDiskSeq() == 0) { - LOGGER.debug(String.format("Choosing disk controller [%s] for the root disk.", controllerInfo.first())); + LOGGER.debug("Choosing disk controller [{}] for the root disk.", controllerInfo.first()); return controllerInfo.first(); } - LOGGER.debug(String.format("Choosing disk controller [%s] for the data disks.", controllerInfo.second())); + LOGGER.debug("Choosing disk controller [{}] for the data disks.", controllerInfo.second()); return controllerInfo.second(); } + + public static void configureDiskControllerMappingsInVmwareBaseModule(List<DiskControllerMappingVO> mappings) { + List<DiskControllerMappingVO> validMappings = new ArrayList<>(); + + for (DiskControllerMappingVO mapping : mappings) { + try { + if (!DiskControllerType.osdefault.toString().equals(mapping.getName())) { + Class.forName(mapping.getControllerReference()); + } + LOGGER.debug("Adding disk controller mapping with name [{}] and controller reference [{}] to the list of available disk controllers.", + mapping.getName(), mapping.getControllerReference()); + validMappings.add(mapping); Review Comment: `configureDiskControllerMappingsInVmwareBaseModule` only validates that the controller class exists, but later logic assumes non-`osdefault` mappings have non-null `busName`, `maxDeviceCount`, `maxControllerCount`, and `vmdkAdapterType`. Since the DB schema allows these columns to be NULL, a partially configured mapping can later trigger NPEs or incorrect behavior. Consider validating required fields here (and skipping invalid rows) so the runtime surface area is safer. ########## plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java: ########## @@ -1973,61 +1974,157 @@ protected ScaleVmAnswer execute(ScaleVmCommand cmd) { return new ScaleVmAnswer(cmd, true, null); } - protected void ensureDiskControllers(VirtualMachineMO vmMo, Pair<String, String> controllerInfo) throws Exception { - if (vmMo == null) { + /** + * Validates whether the instance has the required disk controllers, and creates them if it does not. + * @param controllerInfo pair containing the root disk and data disk controllers, respectively. + * @throws CloudRuntimeException if the instance does not have the required controllers, but this method is unable to create them. + */ + protected void ensureDiskControllers(VirtualMachineMO vmMo, Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo, + boolean isSystemVm) throws Exception { + Pair<DiskControllerMappingVO, DiskControllerMappingVO> chosenDiskControllers = VmwareHelper.chooseDiskControllers(controllerInfo, vmMo, null, null); + Set<DiskControllerMappingVO> requiredDiskControllers = VmwareHelper.getRequiredDiskControllers(chosenDiskControllers, isSystemVm); + + if (vmHasRequiredControllers(vmMo, requiredDiskControllers)) { return; } - Pair<String, String> chosenDiskControllers = VmwareHelper.chooseRequiredDiskControllers(controllerInfo, vmMo, null, null); - String scsiDiskController = HypervisorHostHelper.getScsiController(chosenDiskControllers); - if (scsiDiskController == null) { - return; + validateRequiredVirtualHardwareVersionForNewDiskControllers(vmMo, requiredDiskControllers); + validateNewDiskControllersSupportExistingDisks(vmMo, chosenDiskControllers); + teardownAllDiskControllers(vmMo); + + VirtualMachineConfigSpec vmConfigSpec = new VirtualMachineConfigSpec(); + VmwareHelper.addDiskControllersToVmConfigSpec(vmConfigSpec, requiredDiskControllers, isSystemVm); + if (!vmMo.configureVm(vmConfigSpec)) { + throw new CloudRuntimeException("Unable to configure virtual machine's disk controllers."); } + logger.info("Successfully added virtual machine [{}]'s required disk controllers [{}].", vmMo, requiredDiskControllers); + } - vmMo.getScsiDeviceControllerKeyNoException(); - // This VM needs SCSI controllers. - // Get count of existing scsi controllers. Helps not to attempt to create more than the maximum allowed 4 - // Get maximum among the bus numbers in use by scsi controllers. Safe to pick maximum, because we always go sequential allocating bus numbers. - Ternary<Integer, Integer, DiskControllerType> scsiControllerInfo = vmMo.getScsiControllerInfo(); - int requiredNumScsiControllers = VmwareHelper.MAX_SCSI_CONTROLLER_COUNT - scsiControllerInfo.first(); - int availableBusNum = scsiControllerInfo.second() + 1; // method returned current max. bus number + protected boolean vmHasRequiredControllers(VirtualMachineMO vmMo, Set<DiskControllerMappingVO> requiredDiskControllers) throws Exception { + Set<String> requiredDiskControllerClasspaths = requiredDiskControllers.stream() + .map(DiskControllerMappingVO::getControllerReference) + .collect(Collectors.toSet()); - if (DiskControllerType.getType(scsiDiskController) != scsiControllerInfo.third()) { - logger.debug(String.format("Change controller type from: %s to: %s", scsiControllerInfo.third().toString(), - scsiDiskController)); - vmMo.tearDownDevices(new Class<?>[]{VirtualSCSIController.class}); - vmMo.addScsiDeviceControllers(DiskControllerType.getType(scsiDiskController)); - return; + List<VirtualController> existingControllers = vmMo.getControllers(); + for (VirtualController controller : existingControllers) { + String classpath = controller.getClass().getName(); + requiredDiskControllerClasspaths.remove(classpath); } - if (requiredNumScsiControllers == 0) { - return; + if (requiredDiskControllerClasspaths.isEmpty()) { + logger.debug("Virtual machine [{}] has all the required controllers [{}].", vmMo, requiredDiskControllers); + return true; + } + logger.debug("Virtual machine [{}] does not have the following required controllers: [{}].", vmMo, requiredDiskControllerClasspaths); + return false; + } + + /*** + * Validates if the provided <code>VirtualMachineMO</code>'s virtual hardware version supports the required disk controllers. + * @throws CloudRuntimeException if one of the disk controllers requires a version greater than the instance's virtual + * hardware version. + */ + protected void validateRequiredVirtualHardwareVersionForNewDiskControllers(VirtualMachineMO vmMo, Set<DiskControllerMappingVO> requiredDiskControllers) throws Exception { + int hardwareVersion = vmMo.getVirtualHardwareVersion(); + List<DiskControllerMappingVO> unsupportedDiskControllers = new ArrayList<>(); + + for (DiskControllerMappingVO diskController : requiredDiskControllers) { + if (diskController.getMinHardwareVersion() == null) { + continue; + } + Integer requiredVersion = Integer.parseInt(diskController.getMinHardwareVersion()); + if (hardwareVersion < requiredVersion) { + unsupportedDiskControllers.add(diskController); + } + } + + if (!unsupportedDiskControllers.isEmpty()) { + String names = unsupportedDiskControllers.stream().map(DiskControllerMappingVO::getName).collect(Collectors.joining(", ")); + String requiredVersions = unsupportedDiskControllers.stream().map(DiskControllerMappingVO::getMinHardwareVersion).collect(Collectors.joining(", ")); + logger.debug("Virtual machine [{}] does not support disk controllers [{}], as its virtual hardware version is [{}] but the controllers require, respectfully, versions [{}].", Review Comment: Typo in log message: "respectfully" should be "respectively" (the sentence is describing required versions, not expressing respect). ```suggestion logger.debug("Virtual machine [{}] does not support disk controllers [{}], as its virtual hardware version is [{}] but the controllers require, respectively, versions [{}].", ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
