Hi Daan, Rajani
I looked at a code of master and 4.4 branch. findById is used at line
3031 of master branch and line 3299 of 4.4 branch, so I think this
leads to an same problem.
The code snippets of master and 4.4 branch are below.
[master] server/src/com/cloud/api/ApiResponseHelper.java
2987 @Override
2988 public UsageRecordResponse createUsageResponse(Usage usageRecord) {
2989 UsageRecordResponse usageRecResponse = new
UsageRecordResponse();
2990
2991 Account account =
ApiDBUtils.findAccountById(usageRecord.getAccountId());
2992 if (account.getType() == Account.ACCOUNT_TYPE_PROJECT) {
2993 //find the project
2994 Project project =
ApiDBUtils.findProjectByProjectAccountId(account.getId());
2995 usageRecResponse.setProjectId(project.getUuid());
2996 usageRecResponse.setProjectName(project.getName());
2997 } else {
2998 usageRecResponse.setAccountId(account.getUuid());
2999 usageRecResponse.setAccountName(account.getAccountName());
3000 }
3001
3002 Domain domain =
ApiDBUtils.findDomainById(usageRecord.getDomainId());
3003 if (domain != null) {
3004 usageRecResponse.setDomainId(domain.getUuid());
3005 }
3006
3007 if (usageRecord.getZoneId() != null) {
3008 DataCenter zone =
ApiDBUtils.findZoneById(usageRecord.getZoneId());
3009 if (zone != null) {
3010 usageRecResponse.setZoneId(zone.getUuid());
3011 }
3012 }
3013 usageRecResponse.setDescription(usageRecord.getDescription());
3014 usageRecResponse.setUsage(usageRecord.getUsageDisplay());
3015 usageRecResponse.setUsageType(usageRecord.getUsageType());
3016 if (usageRecord.getVmInstanceId() != null) {
3017 VMInstanceVO vm =
_entityMgr.findByIdIncludingRemoved(VMInstanceVO.class,
usageRecord.getVmInstanceId());
3018 if (vm != null) {
3019 usageRecResponse.setVirtualMachineId(vm.getUuid());
3020 }
3021 }
3022 usageRecResponse.setVmName(usageRecord.getVmName());
3023 if (usageRecord.getTemplateId() != null) {
3024 VMTemplateVO template =
ApiDBUtils.findTemplateById(usageRecord.getTemplateId());
3025 if (template != null) {
3026 usageRecResponse.setTemplateId(template.getUuid());
3027 }
3028 }
3029
3030 if (usageRecord.getUsageType() ==
UsageTypes.RUNNING_VM || usageRecord.getUsageType() ==
UsageTypes.ALLOCATED_VM) {
3031 ServiceOfferingVO svcOffering =
_entityMgr.findById(ServiceOfferingVO.class,
usageRecord.getOfferingId().toString());
3032 //Service Offering Id
3033 usageRecResponse.setOfferingId(svcOffering.getUuid());
3034 //VM Instance ID
3035 VMInstanceVO vm =
_entityMgr.findByIdIncludingRemoved(VMInstanceVO.class,
usageRecord.getUsageId().toString());
3036 if (vm != null) {
3037 usageRecResponse.setUsageId(vm.getUuid());
3038 }
3039 //Hypervisor Type
3040 usageRecResponse.setType(usageRecord.getType());
3041
[ACS 4.4] server/src/com/cloud/api/ApiResponseHelper.java
3255 @Override
3256 public UsageRecordResponse createUsageResponse(Usage usageRecord) {
3257 UsageRecordResponse usageRecResponse = new
UsageRecordResponse();
3258
3259 Account account =
ApiDBUtils.findAccountById(usageRecord.getAccountId());
3260 if (account.getType() == Account.ACCOUNT_TYPE_PROJECT) {
3261 //find the project
3262 Project project =
ApiDBUtils.findProjectByProjectAccountId(account.getId());
3263 usageRecResponse.setProjectId(project.getUuid());
3264 usageRecResponse.setProjectName(project.getName());
3265 } else {
3266 usageRecResponse.setAccountId(account.getUuid());
3267 usageRecResponse.setAccountName(account.getAccountName());
3268 }
3269
3270 Domain domain =
ApiDBUtils.findDomainById(usageRecord.getDomainId());
3271 if (domain != null) {
3272 usageRecResponse.setDomainId(domain.getUuid());
3273 }
3274
3275 if (usageRecord.getZoneId() != null) {
3276 DataCenter zone =
ApiDBUtils.findZoneById(usageRecord.getZoneId());
3277 if (zone != null) {
3278 usageRecResponse.setZoneId(zone.getUuid());
3279 }
3280 }
3281 usageRecResponse.setDescription(usageRecord.getDescription());
3282 usageRecResponse.setUsage(usageRecord.getUsageDisplay());
3283 usageRecResponse.setUsageType(usageRecord.getUsageType());
3284 if (usageRecord.getVmInstanceId() != null) {
3285 VMInstanceVO vm =
_entityMgr.findByIdIncludingRemoved(VMInstanceVO.class,
usageRecord.getVmInstanceId());
3286 if (vm != null) {
3287 usageRecResponse.setVirtualMachineId(vm.getUuid());
3288 }
3289 }
3290 usageRecResponse.setVmName(usageRecord.getVmName());
3291 if (usageRecord.getTemplateId() != null) {
3292 VMTemplateVO template =
ApiDBUtils.findTemplateById(usageRecord.getTemplateId());
3293 if (template != null) {
3294 usageRecResponse.setTemplateId(template.getUuid());
3295 }
3296 }
3297
3298 if (usageRecord.getUsageType() ==
UsageTypes.RUNNING_VM || usageRecord.getUsageType() ==
UsageTypes.ALLOCATED_VM) {
3299 ServiceOfferingVO svcOffering =
_entityMgr.findById(ServiceOfferingVO.class,
usageRecord.getOfferingId().toString());
3300 //Service Offering Id
3301 usageRecResponse.setOfferingId(svcOffering.getUuid());
3302 //VM Instance ID
3303 VMInstanceVO vm =
_entityMgr.findByIdIncludingRemoved(VMInstanceVO.class,
usageRecord.getUsageId().toString());
3304 if (vm != null) {
3305 usageRecResponse.setUsageId(vm.getUuid());
3306 }
3307 //Hypervisor Type
3308 usageRecResponse.setType(usageRecord.getType());
3309
In addition to this, I'm not sure whether findAccountById,
findProjectByProjectAccountId, findDomainById, findZoneById and
findTemplateById work correctly when resources are destroyed. Does
anyone know these methods are OK?
2014-05-29 18:25 GMT+09:00 sebgoa <[email protected]>:
>
> On May 29, 2014, at 11:20 AM, Rajani Karuturi <[email protected]>
> wrote:
>
>> The fix is already in 4.4
>>
>> commit 3a3457e7132d22f52aa38179d40a6eb9b0b29677
>> Author: Sam Schmit <[email protected]>
>> AuthorDate: Fri May 2 13:07:34 2014 -0500
>> Commit: Daan Hoogland <[email protected]>
>> CommitDate: Tue May 6 17:46:20 2014 +0200
>>
>> CLOUDSTACK-6472 listUsageRecords: Pull information from removed items as
>> well, fixing NPEs/Null UUIDs with usage API calls.
>>
>> M server/src/com/cloud/api/ApiResponseHelper.java
>>
>
> still need to check if this fix in 4.4 is complete. Hiroki's change was to
> 4.3 branch. I did not check 4.4.
>
>>
>> @sebgoa
>> i think it would be better to include bug id in the commit messages as thats
>> the only way to track all the commits for a defect.
>>
>
> yes my bad, did not take time to look up the bug id.
>
>>
>> ~Rajani
>>
>>
>>
>> On 29-May-2014, at 2:25 pm, Daan Hoogland <[email protected]> wrote:
>>
>>> Hiroki,
>>>
>>> Did you have a look at 4.4 or master as well? tell us when it works,
>>> so we can cherry-pick the fix to later versions.
>>>
>>> On Thu, May 29, 2014 at 10:25 AM, Hiroki Ohashi <[email protected]>
>>> wrote:
>>>> sebgoa
>>>>
>>>>
>>>> 2014-05-29 16:52 GMT+09:00 sebgoa <[email protected]>:
>>>>>
>>>>> On May 29, 2014, at 9:26 AM, Hiroki Ohashi <[email protected]> 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 <[email protected]>
>>>>>> 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 <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>> 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
>>>
>>>
>>>
>>> --
>>> Daan
>>
>
Best Regards
--
Hiroki Ohashi
