GabrielBrascher commented on a change in pull request #4548:
URL: https://github.com/apache/cloudstack/pull/4548#discussion_r544289459



##########
File path: 
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java
##########
@@ -1248,6 +1261,25 @@ public UserVmResponse 
importUnmanagedInstance(ImportUnmanagedInstanceCmd cmd) {
         return cmdList;
     }
 
+    /**
+     * VCenter send a list of disk in lexicographical order, but not in every
+     * case is hard disk one the root disk. Therefore we must check if hd one
+     * the root disk and if not we search it and add it to the top of the
+     * list

Review comment:
       I see some points to improve here and took the liberty to propose the 
following Javadoc block:
   
   ```
   VCenter sends a list of disks in lexicographical order, but not in every 
case the first volume on the list
   is the root volume. Therefore, this method checks if the first element of 
the list is the root disk; if not,
   it searches it and adds it to the top of the list
   ```
   
   Disclaimer 1: I changed from hard disk to "volume" due to the term hard 
disks being related to the technology/device used to store volumes. In many DCs 
Hard Disks are being mixed with (or replaced by) Solid State Disks.
   
   Disclaimer 2: as far as I know, this method is not used only in VCenter 
cases. We have some situations on CloudStack where methods are used only by a 
hypervisor/storage technology, but I think that this is not the case. 
Therefore, maybe the documentation should be enhanced to avoid 
misunderstandings on that matter.
   




----------------------------------------------------------------
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:
[email protected]


Reply via email to