sebgoa
2014-05-29 16:52 GMT+09:00 sebgoa <run...@gmail.com>:
>
> On May 29, 2014, at 9:26 AM, Hiroki Ohashi <hiroki.s...@gmail.com> wrote:
>
>> Dear guys
>>
>> I found a bug about listUsageRecords API in 4.3 related to a issue
>> CLOUDSTACK-6472 and made a patch. Would you review the patch and
>> correct me if I am wrong?
>>
>> Sam Schmit push a commit about NPEs/Null UUIDs at May 5th but I think
>> his commit is not enough.
>>
>> commit e8047e11db7ef2bdc44bbedfdc47e63c55f080c5
>> Author: Sam Schmit <sam.sch...@appcore.com>
>> Date: Mon May 5 16:38:23 2014 -0500
>>
>> CLOUDSTACK-6472 (4.3 specific) listUsageRecords: Pull
>> information from removed items as well, fixing NPEs/Null UUIDs with
>> usage API calls.
>>
>> Signed-off-by: Sebastien Goasguen <run...@gmail.com>
>>
>> When I called listUsageRecords against CloudStack 4.3 management
>> server that was build at latest 4.3 branch, usage records about
>> destroyed instances of usage type 1 and 2 lacked 'virtualmachineid' as
>> below.
>>
>> {
>> "account": "sgcadm",
>> "accountid": "f9e4e1ca-69fd-4ae3-b70c-15bbcc13406e",
>> "description": "mycluster-front allocated (ServiceOffering: 13)
>> (Template: 203)",
>> "domainid": "84dd635d-fb99-4895-b199-7d777aa144d5",
>> "enddate": "2014-05-20'T'08:59:59+09:00",
>> "name": "mycluster-front",
>> "offeringid": "e5137018-8fca-4e4a-a79e-9e4d1707e079",
>> "rawusage": "0.223611",
>> "startdate": "2014-05-19'T'09:00:00+09:00",
>> "templateid": "1ebdc71e-dcbe-4d3e-82d3-d31897f78d4c",
>> "type": "KVM",
>> "usage": "0.223611 Hrs",
>> "usageid": "09f3d7d6-3c6a-4326-b3d3-fb51506e669d",
>> "usagetype": 2,
>> "zoneid": "c2ba1354-0c15-4284-bdba-4f201767362d"
>> },
>>
>> Hence, it is unable to refer to any destroyed instance id. I patched
>> createUsageResponse method like this.
>>
>> diff --git a/server/src/com/cloud/api/ApiResponseHelper.java
>> b/server/src/com/cloud/api/ApiResponseHelper.java
>> index 7718fc1..cbf3914 100755
>> --- a/server/src/com/cloud/api/ApiResponseHelper.java
>> +++ b/server/src/com/cloud/api/ApiResponseHelper.java
>> @@ -3304,7 +3304,7 @@ public class ApiResponseHelper implements
>> ResponseGenerator {
>> usageRecResponse.setUsage(usageRecord.getUsageDisplay());
>> usageRecResponse.setUsageType(usageRecord.getUsageType());
>> if (usageRecord.getVmInstanceId() != null) {
>> - VMInstanceVO vm =
>> _entityMgr.findById(VMInstanceVO.class,
>> usageRecord.getVmInstanceId());
>> + VMInstanceVO vm =
>> _entityMgr.findByIdIncludingRemoved(VMInstanceVO.class,
>> usageRecord.getVmInstanceId());
>> if (vm != null) {
>> usageRecResponse.setVirtualMachineId(vm.getUuid());
>> }
>>
>
> Looks like an omission to me, I went ahead and patched it:
>
> commit 24e03bed560feb959a61126b76c2d6971e77092e
> Author: Sebastien Goasguen <run...@gmail.com>
> Date: Thu May 29 09:48:46 2014 +0200
>
> Fix usage response, patch by Hiroki Ohashi
>
> We can always revert if people don't agree.
>
> Can you pull the latest 4.3 and try it.
>
Thanks! I will try it.
> Thanks for the patch. you can always attach a patch at
> http://reviews.apache.org
> and follow the instructions at http://cloudstack.apache.org/developers.html
OK, I will follow the instruction next time.
>> After that, listUsageRecords API contains 'virtualmachineid' whether
>> or not instances are destroyed. Here is a result.
>>
>> {
>> "account": "sgcadm",
>> "accountid": "f9e4e1ca-69fd-4ae3-b70c-15bbcc13406e",
>> "description": "mycluster-front allocated (ServiceOffering: 13)
>> (Template: 203)",
>> "domainid": "84dd635d-fb99-4895-b199-7d777aa144d5",
>> "enddate": "2014-05-20'T'08:59:59+09:00",
>> "name": "mycluster-front",
>> "offeringid": "e5137018-8fca-4e4a-a79e-9e4d1707e079",
>> "rawusage": "0.223611",
>> "startdate": "2014-05-19'T'09:00:00+09:00",
>> "templateid": "1ebdc71e-dcbe-4d3e-82d3-d31897f78d4c",
>> "type": "KVM",
>> "usage": "0.223611 Hrs",
>> "usageid": "09f3d7d6-3c6a-4326-b3d3-fb51506e669d",
>> "usagetype": 2,
>> "virtualmachineid": "09f3d7d6-3c6a-4326-b3d3-fb51506e669d",
>> "zoneid": "c2ba1354-0c15-4284-bdba-4f201767362d"
>> },
>>
>> I think this is an expectecd behaviour, so please review and test this
>> patch.
>>
>>
>> Best Regards
>>
>> --
>> Hiroki Ohashi
>
Best Regards
--
Hiroki Ohashi
