H Hiroki,
those calls are for ServiceOfferingVO not for VMInstanceVO. So they
are not the same issue but that doesn't mean that we shouldn't those
the be counted when deleted either.
On Thu, May 29, 2014 at 12:11 PM, Hiroki Ohashi <hiroki.s...@gmail.com> wrote:
> 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 <run...@gmail.com>:
>>
>> On May 29, 2014, at 11:20 AM, Rajani Karuturi <rajani.karut...@citrix.com>
>> wrote:
>>
>>> The fix is already in 4.4
>>>
>>> commit 3a3457e7132d22f52aa38179d40a6eb9b0b29677
>>> Author: Sam Schmit <sam.sch...@appcore.com>
>>> AuthorDate: Fri May 2 13:07:34 2014 -0500
>>> Commit: Daan Hoogland <d...@onecht.net>
>>> 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 <daan.hoogl...@gmail.com> 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 <hiroki.s...@gmail.com>
>>>> wrote:
>>>>> 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
>>>>
>>>>
>>>>
>>>> --
>>>> Daan
>>>
>>
>
>
> Best Regards
>
> --
> Hiroki Ohashi
--
Daan
