shwstppr commented on a change in pull request #4548:
URL: https://github.com/apache/cloudstack/pull/4548#discussion_r547231847
##########
File path:
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -7239,7 +7239,9 @@ private static File fetchSystemVmKeyFile() {
instanceDisk.setController(DiskControllerType.getType(device.getClass().getSimpleName()).toString());
instanceDisk.setControllerUnit(((VirtualSCSIController) device).getBusNumber());
} else {
-
instanceDisk.setController(DiskControllerType.none.toString());
+ String msg = String.format("Controller of
disk %s is unsupported", instanceDisk.getLabel());
+ s_logger.error(msg);
+ throw new Exception(msg);
Review comment:
@rhtyd @DK101010 same method is used when listUnmanagedINstances API is
called so I'm not if we would want the complete API to raise an exception when
disks for one particular VM is not found. In my opinion, we can log the error
here and return the list of disks for the VM as empty. With an empty list of
disks VM import with automatically fail with existing check
https://github.com/apache/cloudstack/blob/master/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java#L942-L944
If we want to go with the above approach of returning empty disks list, we
can clear the `instanceDisks` list variable in the catch block
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
