Copilot commented on code in PR #10454:
URL: https://github.com/apache/cloudstack/pull/10454#discussion_r2135323465
##########
plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java:
##########
@@ -2101,18 +2101,11 @@ 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);
+ String rootDiskControllerDetail =
controllerInfo.get(VmDetailConstants.ROOT_DISK_CONTROLLER);
+ String dataDiskControllerDetail =
controllerInfo.get(VmDetailConstants.DATA_DISK_CONTROLLER);
Review Comment:
There’s no null-check for `controllerInfo` or its keys before calling
`get(...)`. Add a guard to avoid NPEs or provide a sensible default when
`controllerInfo` is missing or incomplete.
```suggestion
String rootDiskControllerDetail = null;
String dataDiskControllerDetail = null;
if (controllerInfo != null) {
rootDiskControllerDetail =
controllerInfo.getOrDefault(VmDetailConstants.ROOT_DISK_CONTROLLER, null);
dataDiskControllerDetail =
controllerInfo.getOrDefault(VmDetailConstants.DATA_DISK_CONTROLLER, null);
}
```
##########
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:
##########
@@ -5320,8 +5325,9 @@ private void fillVMOrTemplateDetailOptions(final
Map<String, List<String>> optio
if (HypervisorType.VMware.equals(hypervisorType)) {
options.put(VmDetailConstants.NIC_ADAPTER, Arrays.asList("E1000",
"PCNet32", "Vmxnet2", "Vmxnet3"));
- options.put(VmDetailConstants.ROOT_DISK_CONTROLLER,
Arrays.asList("osdefault", "ide", "scsi", "lsilogic", "lsisas1068", "buslogic",
"pvscsi"));
- options.put(VmDetailConstants.DATA_DISK_CONTROLLER,
Arrays.asList("osdefault", "ide", "scsi", "lsilogic", "lsisas1068", "buslogic",
"pvscsi"));
+ List<String> availableDiskControllers =
diskControllerMappingDao.listForHypervisor(HypervisorType.VMware).stream().map(DiskControllerMappingVO::getName).collect(Collectors.toList());
Review Comment:
[nitpick] The list of controller names is not sorted, which can lead to
inconsistent option ordering in the UI. Consider sorting alphabetically to
ensure a predictable user experience.
```suggestion
List<String> availableDiskControllers =
diskControllerMappingDao.listForHypervisor(HypervisorType.VMware).stream()
.map(DiskControllerMappingVO::getName)
.sorted()
.collect(Collectors.toList());
```
##########
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java:
##########
@@ -3654,23 +3752,29 @@ private VirtualMachineDiskInfo
getMatchingExistingDisk(VirtualMachineDiskInfoBui
return getMatchingExistingDiskWithVolumeDetails(diskInfoBuilder,
volume.getPath(), chainInfo, isManaged, iScsiName, datastoreUUID, hyperHost,
context);
}
- private String getDiskController(VirtualMachineMO vmMo,
VirtualMachineDiskInfo matchingExistingDisk, DiskTO vol, Pair<String, String>
controllerInfo, boolean deployAsIs) throws Exception {
- DiskControllerType controllerType = DiskControllerType.none;
+ /**
+ * Returns the disk controller mapping that should be used for the disk.
If the instance uses a deploy-as-is template
+ * and the disk already exists, tries to choose based on the current bus
name first, but chooses any available disk controller
+ * if unable to choose a type based on the bus name; if the instance does
not use a deploy-as-is template or the disk
+ * does not exist, chooses based on <code>controllerInfo</code>.
+ * @param controllerInfo pair containing the root disk and data disk
controllers, respectively. Ignored if the instance uses a deploy-as-is template
and the disk already exists.
+ * @throws CloudRuntimeException if the instance uses a deploy-as-is
template, but no disk controller is available.
+ */
+ protected DiskControllerMappingVO getControllerForDisk(VirtualMachineMO
vmMo, VirtualMachineDiskInfo matchingExistingDisk,
+ DiskTO vol,
Pair<DiskControllerMappingVO, DiskControllerMappingVO> controllerInfo,
+ boolean deployAsIs)
throws Exception {
if (deployAsIs && matchingExistingDisk != null) {
String currentBusName =
matchingExistingDisk.getDiskDeviceBusName();
if (currentBusName != null) {
- logger.info("Chose disk controller based on existing
information: " + currentBusName);
- if (currentBusName.startsWith("ide")) {
- controllerType = DiskControllerType.ide;
- } else if (currentBusName.startsWith("scsi")) {
- controllerType = DiskControllerType.scsi;
+ Set<DiskControllerMappingVO>
mappingsForExistingDiskControllers =
vmMo.getMappingsForExistingDiskControllers();
+ for (DiskControllerMappingVO mapping :
mappingsForExistingDiskControllers) {
+ if (currentBusName.startsWith(mapping.getBusName())) {
+ logger.debug("Choosing disk controller [{}] for
virtual machine [{}] based on current bus name [{}].", mapping.getName(), vmMo,
currentBusName);
+ return mapping;
+ }
}
}
Review Comment:
[nitpick] Falling back silently to an arbitrary existing controller may be
confusing in failure scenarios. Add a debug or warning log here to record which
controller was chosen and why the bus name lookup failed.
```suggestion
}
logger.debug("Falling back to an arbitrary existing disk
controller for virtual machine [{}] as no matching controller was found based
on the current bus name.", vmMo);
```
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
