[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1254


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-206936718
  
Thank you guys.  I will merge this...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[abhinandanprateek]

Github user abhinandanprateek commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-206815106
  
@swill @bhaisaab @DaanHoogland I have done manual testing for the fix as 
expected. This can be merged now as there are enough LGTMs.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[bhaisaab]

Github user bhaisaab commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-206773236
  
LGTM based on test/code, @DaanHoogland Abhi ( @agneya2001 ) can explain in 
much detail; but this fixes a NPE case and uses RoleType comparison that id 
comparison to allow any user account of RoleType.Admin to run quota apis such 
as listing them


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-206755942
  
@swill @bhaisaab we should not accept an LTGM without 
explanation/justification


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-206755664
  
LGTM, @swill based on the code 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-206755482
  
LGTM, @swill 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-206684033
  
We have one LGTM and it is passing CI, is this ready to merge then?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[abhinandanprateek]

Github user abhinandanprateek commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1254#discussion_r58807499
  
--- Diff: 
plugins/database/quota/src/org/apache/cloudstack/api/command/QuotaSummaryCmd.java
 ---
@@ -59,7 +59,7 @@ public QuotaSummaryCmd() {
 public void execute() {
 Account caller = CallContext.current().getCallingAccount();
 List responses;
-if (caller.getAccountId() <= 2) { //non root admin or system
+if (caller.getType() == Account.ACCOUNT_TYPE_ADMIN) { //admin 
account
--- End diff --

Relaxed the condition so that any account with admin type is able to view 
the quota summary.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[swill]

Github user swill commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1254#discussion_r58776967
  
--- Diff: 
plugins/database/quota/src/org/apache/cloudstack/api/command/QuotaSummaryCmd.java
 ---
@@ -59,7 +59,7 @@ public QuotaSummaryCmd() {
 public void execute() {
 Account caller = CallContext.current().getCallingAccount();
 List responses;
-if (caller.getAccountId() <= 2) { //non root admin or system
+if (caller.getType() == Account.ACCOUNT_TYPE_ADMIN) { //admin 
account
--- End diff --

This went from valid entries of 1 or 2 to only 1.  This is an intentional 
change?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[bvbharatk]

Github user bvbharatk commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-206542659
  
### ACS CI BVT Run
 **Sumarry:**
 Build Number 159
 Hypervisor xenserver
 NetworkType Advanced
 Passed=105
 Failed=0
 Skipped=4

_Link to logs Folder (search by build_no):_ 
https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0


**Failed tests:**

**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso

**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_loadbalance.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_non_contigiousvlan.py
test_deploy_vm_iso.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_ssvm.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[bvbharatk]

Github user bvbharatk commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-205035894
  
### ACS CI BVT Run
 **Sumarry:**
 Build Number 158
 Hypervisor xenserver
 NetworkType Advanced
 Passed=106
 Failed=2
 Skipped=4

_Link to logs Folder (search by build_no):_ 
https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0


**Failed tests:**
* :setup Failing since 3 runs

 * ContextSuite context=TestVpcSite2SiteVpn>:setup Failed


**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso

**Passed test suits:**
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData

integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_guest_vlan_range.TestDedicateGuestVlanRange
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
integration.smoke.test_snapshots.TestSnapshotRootDisk

integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_volumes.TestCreateVolume
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[alexandrelimassantana]

Github user alexandrelimassantana commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1254#discussion_r55169349
  
--- Diff: 
framework/quota/src/org/apache/cloudstack/quota/QuotaManagerImpl.java ---
@@ -358,10 +358,11 @@ public QuotaUsageVO updateQuotaDiskUsage(UsageVO 
usageRecord, final BigDecimal a
 BigDecimal rawusage;
 // get service offering details
 ServiceOfferingVO serviceoffering = 
_serviceOfferingDao.findServiceOffering(usageRecord.getVmInstanceId(), 
usageRecord.getOfferingId());
+if (serviceoffering == null) return quotalist;
 rawusage = new BigDecimal(usageRecord.getRawUsage());
 
 QuotaTariffVO tariff = 
_quotaTariffDao.findTariffPlanByUsageType(QuotaTypes.CPU_NUMBER, 
usageRecord.getEndDate());
-if (tariff != null && 
tariff.getCurrencyValue().compareTo(BigDecimal.ZERO) != 0) {
+if (tariff != null && 
tariff.getCurrencyValue().compareTo(BigDecimal.ZERO) != 0 && 
serviceoffering.getCpu() != null) {
--- End diff --

Hello @agneya2001 ,

In the refactor you plan to make, you could also turn this piece of code 
into a method, for reusability:
```Java
tariff != null && tariff.getCurrencyValue().compareTo(BigDecimal.ZERO)
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[bhaisaab]

Github user bhaisaab commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-175562144
  
@agneya2001 okay, we need one more LGTM on this. I'll picked your marvin 
fix from #1240 , can you also share test results if any?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[agneya2001]

Github user agneya2001 commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-175372328
  
@cristofolini it is not that involved a code and not something that i can 
reuse. 
But yes i will take a holistic look at the code and refactor when I get 
back to it. 

@bhaisaab @remibergsma Lets not hold it for this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[cristofolini]

Github user cristofolini commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-174217394
  
@agneya2001 Could you please extract the code at lines 126-135 to a 
separate method? It would make that bit of code much easier to read. :)



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[bhaisaab]

Github user bhaisaab commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-170456162
  
@remibergsma let's test and merge this before the freeze. thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[bhaisaab]

Github user bhaisaab commented on the pull request:

https://github.com/apache/cloudstack/pull/1254#issuecomment-167041889
  
LGTM


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[resmo]

Github user resmo commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1254#discussion_r47931786
  
--- Diff: 
plugins/database/quota/src/org/apache/cloudstack/api/command/QuotaSummaryCmd.java
 ---
@@ -59,7 +59,7 @@ public QuotaSummaryCmd() {
 public void execute() {
 Account caller = CallContext.current().getCallingAccount();
 List responses;
-if (caller.getAccountId() <= 2) { //non root admin or system
+if (caller.getAccountId() <= 2) { // root admin or system
--- End diff --

to assume that an account <=2 has root admin privileges is just naive. 
There must be a solid way to do this. Just my 2 cents..


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[agneya2001]

Github user agneya2001 commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1254#discussion_r47990946
  
--- Diff: 
plugins/database/quota/src/org/apache/cloudstack/api/command/QuotaSummaryCmd.java
 ---
@@ -59,7 +59,7 @@ public QuotaSummaryCmd() {
 public void execute() {
 Account caller = CallContext.current().getCallingAccount();
 List responses;
-if (caller.getAccountId() <= 2) { //non root admin or system
+if (caller.getAccountId() <= 2) { // root admin or system
--- End diff --

rightly pointed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...

[agneya2001]

GitHub user agneya2001 opened a pull request:

https://github.com/apache/cloudstack/pull/1254

CLOUDSTACK-9174: A deleted account results in NPE

When an account is deleted from cloudstack for which quota is still
being calculated and if the quota reaches minimum threshold then
quota service will try to alert the user. This results in NPE and is
fixed by excluding such accounts from alerting and other quota related
mechanisms.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/shapeblue/cloudstack master-9174

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1254.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1254


commit c07720b73cb16cb55dc83047373e22885d47dbc5
Author: Abhinandan Prateek 
Date:   2015-12-17T05:02:33Z

CLOUDSTACK-9174: A deleted account results in NPE

When an account is deleted from cloudstack for which quota is still
being calculated and if the quota reaches minimum threshold then
quota service will try to alert the user. This results in NPE and is
fixed by excluding such accounts from alerting and other quota related
mechanisms.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---