You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by agneya2001 <gi...@git.apache.org> on 2015/12/17 06:08:49 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
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 <ab...@shapeblue.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by resmo <gi...@git.apache.org>.
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<QuotaSummaryResponse> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by bhaisaab <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by swill <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by agneya2001 <gi...@git.apache.org>.
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<QuotaSummaryResponse> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by swill <gi...@git.apache.org>.
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<QuotaSummaryResponse> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by DaanHoogland <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by bvbharatk <gi...@git.apache.org>.
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:**
* <nose.suite
* ContextSuite context=TestVpcRemoteAccessVpn>: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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by bhaisaab <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by abhinandanprateek <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by bhaisaab <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by abhinandanprateek <gi...@git.apache.org>.
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<QuotaSummaryResponse> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by bhaisaab <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by bvbharatk <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by alexandrelimassantana <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by agneya2001 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by DaanHoogland <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by DaanHoogland <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by cristofolini <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9174: A deleted account result...
Posted by swill <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---