From: Prachi Damle Sent: Wednesday, July 25, 2012 2:44 PM To: Edison Su; Mice Xia Cc: cloudstack-dev@incubator.apache.org; Nitin Mehta Subject: RE: Review Request: CS-15609 Volumes can be created as a part of VM creation when un-allocated space is insufficient on primary storage
From: Prachi Damle Sent: Wednesday, July 25, 2012 1:34 PM To: Edison Su; Mice Xia Cc: cloudstack-dev@incubator.apache.org<mailto:cloudstack-dev@incubator.apache.org>; Nitin Mehta Subject: RE: Review Request: CS-15609 Volumes can be created as a part of VM creation when un-allocated space is insufficient on primary storage From: Prachi Damle Sent: Wednesday, July 25, 2012 11:17 AM To: Mice Xia; Edison Su Cc: cloudstack-dev@incubator.apache.org<mailto:cloudstack-dev@incubator.apache.org>; Nitin Mehta Subject: RE: Review Request: CS-15609 Volumes can be created as a part of VM creation when un-allocated space is insufficient on primary storage Hi Mice, I think option#2 is better because it will avoid unnecessary iterations and return when first fit satisfying both volume allocations and host linkage is found. >>I just think the logic: volume size against storage pool is already in >>AbstractStoragePoolAllocator->checkpool, better to find a way to reuse the >>code, instead of duplicate in other place. Edison, it's not just the volume size against pool check. That check is already done by allocator when it returns that pool as suitable. The check that Mice is adding is checking if all volumes can co-exist in case same pool gets selected. This needs the knowledge of sizes of all other volumes of that VM. [Edison] In checkpool, the code will check how many storage is available on the specified pool, then add the asksize, then get the used percentage, if the percentage is lower than a threshold, return false, or return true. For me, it's the almost the same code as Mice did. The only difference is, Mice is checking on a bunch of volumes against a storage pool, while checkpool is on one volume. if there is any computation that is common to AbstractStoragePoolAllocator->checkpool, then we can reuse that by placing it under some utility method may be. [Prachi]: Sure. As I said earlier, common computation can be reused, Or changed to work on bunch of volumes. But this will still get invoked from findPotentialDeploymentResources in planner, rather than by allocator. Allocator will work on finding pools given a volume. [Edison] Agree, called in findPotentialDeploymentResources or findSuitablePoolsForVolumes, both are ok. How about put the common computation code in storagemanagemerImpl? However I think it would be good to do all the size computation in a separate method for readability. Also following needs to be fixed in your logic: 1) haveEnoughSpace should be initialized to false for every Volume iteration 2) If for some volume none of the pool seems to have enough space (due to other prior volumes allocated), we need not continue - error out there. Thus add the check if(!haveEnoughSpace) { //error/ } inside the l 3) storageUsedMap should be initialized for every host iteration, otherwise while trying the second host 'requestedSpaceSize' might be wrong. 4) Before iterating over the volumes, I think we should sort the list in descending order of volume size - so that volume needing the largest chunk of space gets mapped to the pool first. This will avoid the situation like this: volA 4G, volb 6G SP1 6G, SP2 4G Allocator could return volA -> SP1, SP2 and volB -> SP1. If you choose the pool for VolA first, then VolB might not find a fit. But if we sort by size and choose pool for VolB first, we might find a destination. Thanks, Prachi From: Mice Xia [mailto:mice_...@tcloudcomputing.com]<mailto:[mailto:mice_...@tcloudcomputing.com]> Sent: Wednesday, July 25, 2012 6:43 AM To: Edison Su Cc: cloudstack-dev@incubator.apache.org<mailto:cloudstack-dev@incubator.apache.org>; Prachi Damle; Nitin Mehta Subject: Re: Review Request: CS-15609 Volumes can be created as a part of VM creation when un-allocated space is insufficient on primary storage Hi Edison, after some further investigation, i found it is non-trivial to put this logic in findSuitablePoolsForVolumes. method findSuitablePoolsForVolumes is essentially used for listing all storage pools for one individual volume.(it even gets a parameter 'returnUpTo'). if it has to deal with more then one volumes, the computation there will be exponential, it is basically a tree recursive search for all possible volume allocation on storagepools, think about following example. volA 20G , volB 30G SP1 40G, sp2 60G there are three possible allocation: volA->SP1 volB->SP2 volA->SP2 volB->SP2 volA->SP2 volB->SP1 this seems not very efficient and the return signature may need a change. back to this multiple volumes capacity check issue, i would like to suggest following two approaches: 1) put special check logic in findSuitablePoolsForVolumes, but, instead of recursive exploring, return immediately when the first possible allocation is found. 2) or keep it in findPotentialDeploymentResources, while findSuitablePoolsForVolumes is responsbile for providing pools for single volume, findPotentialDeploymentResources will check them as a whole and it will return immediately after find a proper allocation. please adivce, thanks Regards Mice
