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/13 05:03:10 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9153: When negative credits ar...
GitHub user agneya2001 opened a pull request:
https://github.com/apache/cloudstack/pull/1234
CLOUDSTACK-9153: When negative credits are added to an account the
When negative credits are added to an account the balance credits can become negative for that account. This will fix will lock the account if quota is enforced.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shapeblue/cloudstack master-9153
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1234.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 #1234
----
commit 5025011d3151e9056fed7a9b24f913dc83515fdc
Author: Abhinandan Prateek <ab...@shapeblue.com>
Date: 2015-12-13T03:58:02Z
CLOUDSTACK-9153: When negative credits are added to an account the
balance credits can become negative for that account. This will fix will
lock the account if quota is enforced.
----
---
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-9153: When negative credits ar...
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1234#discussion_r47443252
--- Diff: plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java ---
@@ -383,31 +383,41 @@ public QuotaTariffVO updateQuotaTariffPlan(QuotaTariffUpdateCmd cmd) {
@Override
--- End diff --
There were two "addQuotaCredits" methods here, the first one with only a check. So combined the two. That removed on '@override' but still kept the '@Override' of the removed method.
---
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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164244364
Thanks abhi, I will run it after the regular suite passes and add it to my fork of it, and of course report here.
---
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-9153: When negative credits ar...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164288018
For the sake of it, I run a some generic integration tests and they are happy.
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=true \
component/test_password_server.py \
smoke/test_vpc_redundant.py \
smoke/test_routers_iptables_default_policy.py \
smoke/test_routers_network_ops.py \
smoke/test_vpc_router_nics.py \
smoke/test_router_dhcphosts.py \
smoke/test_loadbalance.py \
smoke/test_internal_lb.py \
smoke/test_ssvm.py \
smoke/test_vpc_vpn.py \
smoke/test_privategw_acl.py \
smoke/test_network.py
```
Result:
```
Check the password file in the Router VM ... === TestName: test_isolate_network_password_server | Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network ... === TestName: test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network and check default routes ... === TestName: test_02_redundant_VPC_default_routes | Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network ... === TestName: test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Status : SUCCESS ===
ok
Test iptables default INPUT/FORWARD policy on RouterVM ... === TestName: test_02_routervm_iptables_policies | Status : SUCCESS ===
ok
Test iptables default INPUT/FORWARD policies on VPC router ... === TestName: test_01_single_VPC_iptables_policies | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_01_isolate_network_FW_PF_default_routes_egress_true | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_02_isolate_network_FW_PF_default_routes_egress_false | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_03_RVR_Network_check_router_state | Status : SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test nics after destroy ... === TestName: test_01_VPC_nics_after_destroy | Status : SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test default routes ... === TestName: test_02_VPC_default_routes | Status : SUCCESS ===
ok
Check that the /etc/dhcphosts.txt doesn't contain duplicate IPs ... === TestName: test_router_dhcphosts | Status : SUCCESS ===
ok
Test to create Load balancing rule with source NAT ... === TestName: test_01_create_lb_rule_src_nat | Status : SUCCESS ===
ok
Test to create Load balancing rule with non source NAT ... === TestName: test_02_create_lb_rule_non_nat | Status : SUCCESS ===
ok
Test for assign & removing load balancing rule ... === TestName: test_assign_and_removal_lb | Status : SUCCESS ===
ok
Test to verify access to loadbalancer haproxy admin stats page ... === TestName: test02_internallb_haproxy_stats_on_all_interfaces | Status : SUCCESS ===
ok
Test create, assign, remove of an Internal LB with roundrobin http traffic to 3 vm's ... === TestName: test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Status : SUCCESS ===
ok
Test SSVM Internals ... === TestName: test_03_ssvm_internals | Status : SUCCESS ===
ok
Test CPVM Internals ... === TestName: test_04_cpvm_internals | Status : SUCCESS ===
ok
Test stop SSVM ... === TestName: test_05_stop_ssvm | Status : SUCCESS ===
ok
Test stop CPVM ... === TestName: test_06_stop_cpvm | Status : SUCCESS ===
ok
Test reboot SSVM ... === TestName: test_07_reboot_ssvm | Status : SUCCESS ===
ok
Test reboot CPVM ... === TestName: test_08_reboot_cpvm | Status : SUCCESS ===
ok
Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : SUCCESS ===
ok
Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : SUCCESS ===
ok
Test Remote Access VPN in VPC ... === TestName: test_vpc_remote_access_vpn | Status : SUCCESS ===
ok
Test VPN in VPC ... === TestName: test_vpc_site2site_vpn | Status : SUCCESS ===
ok
test_01_vpc_privategw_acl (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_01_vpc_privategw_acl | Status : SUCCESS ===
ok
test_02_vpc_privategw_static_routes (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_02_vpc_privategw_static_routes | Status : SUCCESS ===
ok
test_03_rvpc_privategw_static_routes (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_03_rvpc_privategw_static_routes | Status : SUCCESS ===
ok
Test for port forwarding on source NAT ... === TestName: test_01_port_fwd_on_src_nat | Status : SUCCESS ===
ok
Test for port forwarding on non source NAT ... === TestName: test_02_port_fwd_on_non_src_nat | Status : SUCCESS ===
ok
Test for reboot router ... === TestName: test_reboot_router | Status : SUCCESS ===
ok
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_1_static_nat_rule | Status : SUCCESS ===
ok
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_2_nat_rule | Status : SUCCESS ===
ok
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 38 tests in 21982.123s
OK
```
---
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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164269573
thanks @bhaisaab I forgot setting indeed, 5 out of 7 tests now succeed. investigating the other two now
---
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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1234#discussion_r47443410
--- Diff: plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java ---
@@ -383,31 +383,41 @@ public QuotaTariffVO updateQuotaTariffPlan(QuotaTariffUpdateCmd cmd) {
@Override
--- End diff --
Yes I saw, sorry for the confusion. I wasn't paying attention at first.
---
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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164243359
@agneya2001 looks good, I don't understand all of it yet, partly because this is rather new functionality. Any plans on integration tests?
I'll add some questions and comments and run number 1234 through the regression suite. ;)
---
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-9153: When negative credits ar...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1234
---
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-9153: When negative credits ar...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164268405
LGTM
@DaanHoogland to use the plugin and run tests, your zone cfg need to enable the quota plugin global setting and restart mgmt server before the plugin/apis can be used.
---
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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164277189
@remibergsma @bhaisaab @agneya2001 confirmed that the problem with the last two tests is in master as well. I am satisfied nothing breaks in the quota plugin, both this PR and master show the following output:
```
=== TestName: test_01_quota | Status : SUCCESS ===
=== TestName: test_02_quota | Status : SUCCESS ===
=== TestName: test_03_quota | Status : SUCCESS ===
=== TestName: test_04_quota | Status : SUCCESS ===
=== TestName: test_05_quota | Status : SUCCESS ===
=== TestName: test_06_quota | Status : FAILED ===
=== TestName: test_07_quota | Status : FAILED ===
```
---
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-9153: When negative credits ar...
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164243999
@DaanHoogland This does have a smoke test here: test/integration/smoke/test_quota.py. This is very basic, will add more test scenarios to it.
---
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-9153: When negative credits ar...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164271142
@DaanHoogland thanks, I think you're right but only @agneya2001 can confirm 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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164271487
I will try a run without this PR to confirm it is not a problem with 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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164246827
@agneya2001 is this 'only' a major bug? it seems to me to be the purpose of the quota system. blocker may be overdone but I really like this in before 4.7 as regular part of the quota system. ping @remibergsma
---
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-9153: When negative credits ar...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164277430
@DaanHoogland thanks Daan, I'll look into the failing issues with Abhi tomorrow.
---
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-9153: When negative credits ar...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164255558
Ping @bhaisaab any chance to get this reviewed today? ;-)
---
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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164266288
@agneya2001 I ran test_quota.py
```
nosetests --with-marvin --marvin-config=/data/shared/marvin/mct-zone1-kvm1-kvm2-zwps.cfg -s -a tags=advanced,required_hardware=false /data/git/cs1/cloudstack/test/integration/smoke/test_quota.py
```
all tests failed with exceptions like
```
======================================================================
ERROR: test_01_quota (integration.smoke.test_quota.TestQuota)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs1/cloudstack/test/integration/smoke/test_quota.py", line 59, in test_01_quota
response = self.apiclient.quotaTariffList(cmd)
File "/usr/lib/python2.7/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 2972, in quotaTariffList
response = self.connection.marvinRequest(command, response_type=response, method=method)
File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 379, in marvinRequest
raise e
CloudstackAPIException: Execute cmd: error failed, due to: errorCode: 432, errorText:The given command does not exist or it is not available for user
```
might I be missing something?
---
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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1234#discussion_r47442757
--- Diff: plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java ---
@@ -95,7 +95,7 @@
@Inject
private DomainDao _domainDao;
@Inject
- private RegionManager _regionMgr;
+ private AccountManager _accountMgr;
--- End diff --
out of scope: let's get rid of the '_'-prefix convention.
---
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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1234#discussion_r47442747
--- Diff: plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java ---
@@ -383,31 +383,41 @@ public QuotaTariffVO updateQuotaTariffPlan(QuotaTariffUpdateCmd cmd) {
@Override
--- End diff --
it is removed from QuotaResponseBuilder (why?) but still marked as @Override here. any purpose?
---
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-9153: When negative credits ar...
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1234#discussion_r47443258
--- Diff: plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java ---
@@ -95,7 +95,7 @@
@Inject
private DomainDao _domainDao;
@Inject
- private RegionManager _regionMgr;
+ private AccountManager _accountMgr;
--- End diff --
Noted.
---
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-9153: When negative credits ar...
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164248806
Kept it as a major bug as the account with negative quota will still get locked if quota is enforced, by the quota service thread, when it runs next.
While with this fix you can lock the account with the add credit API, instead of waiting for quota service to do that.
---
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-9153: When negative credits ar...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1234#issuecomment-164270791
I think the problem is in the tests as it starts with an empty balance. Can you confirm @bhaisaab , @agneya2001 ?
---
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.
---