Logs should have as much of the 5 Ws as possible:
- who
- what
- where
- why
- when
log4j already adds the when.
This log:
if(!haveEnoughSpace) {s_logger.warn("insufficient capacity to allocate all
volumes");break;}
Only specifies the "what". If you add the vm id and other parts of the
deployment plan, then it becomes easier to understand this failure.
Furthermore, are you sure it needs to be a WARN? This certainly seems like
a "normal" situation.
http://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/Level.html#WAR
N
Also at the end of the method, here's the log:
s_logger.debug("Could not find a potential host that has associated
storage pools from the suitable host/pool lists for this VM");
What is "this" vm? Also there is clearly a reason why the method could not
find a potential hosts -- that should be logged as well.
Finally, it should be fairly easy to write a Junit test case for this.
There's a ton of if-then-else /while-break code going on in this method.
If someone comes along and fixes something else, they could easily break
your code.
On 7/26/12 10:18 AM, "Edison Su" <edison...@citrix.com> wrote:
>
>-----------------------------------------------------------
>This is an automatically generated e-mail. To reply, visit:
>https://reviews.apache.org/r/6028/#review9502
>-----------------------------------------------------------
>
>Ship it!
>
>
>Thanks! It's much cleaner now.
>
>- edison su
>
>
>On July 26, 2012, 1:25 p.m., mice xia wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/6028/
>> -----------------------------------------------------------
>>
>> (Updated July 26, 2012, 1:25 p.m.)
>>
>>
>> Review request for cloudstack, Prachi Damle, Nitin Mehta, and edison su.
>>
>>
>> Description
>> -------
>>
>> fix CS-15609 Volumes can be created as a part of VM creation when
>>un-allocated space is insufficient on primary storage
>>
>> check the availability of un-allocated primary storage space during
>>planning stage, for multiple-volume VM creation scenario
>>
>> modification in StorageManagerImpl.java and StorageManager.java:
>> add a new method storagePoolHasEnoughSpace(List<Volumes>, StoragePool),
>>check if storagePool has enough space for all requested volumes
>>
>> modification in FirstfitPlanner.findPotentialDeploymentResources:
>> handle multiple volume case, keep track of allocated volumes for pools
>>and call storagePoolHasEnoughSpace to check space availability
>>
>> modification in AbstractStoragePoolAllocator.java:
>> extract capacity computation logic and make a new method in
>>StorageManagerImpl
>>
>>
>> This addresses bug CS-15609.
>>
>>
>> Diffs
>> -----
>>
>> server/src/com/cloud/deploy/FirstFitPlanner.java eb82c75
>> server/src/com/cloud/storage/StorageManager.java 67ad97c
>> server/src/com/cloud/storage/StorageManagerImpl.java d94bada
>>
>>server/src/com/cloud/storage/allocator/AbstractStoragePoolAllocator.java
>>0a0f66e
>>
>> Diff: https://reviews.apache.org/r/6028/diff/
>>
>>
>> Testing
>> -------
>>
>> perform following tests: (overprovisioning.factor=1)
>> 1) create vm with 20G root + 20G data on one NFS PS, with allocation
>>state 7.82GB/36.72GB, VM creation failed as expected
>> 2) create vm with 20G root + 5G data on one NFS PS, with allocation
>>state 7.82GB/36.72GB, threshold=0.85, VM creation failed as expected
>> 3) create vm with 20G root + 5G data on one NFS PS, with allocation
>>state 7.82GB/36.72GB, threshold=0.99, VM creation succeed as expected
>>
>>
>> Thanks,
>>
>> mice xia
>>
>>
>
