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/14 07:46:21 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9161: fix the quota marvin tes...
GitHub user agneya2001 opened a pull request:
https://github.com/apache/cloudstack/pull/1240
CLOUDSTACK-9161: fix the quota marvin test
1. Create a dummy user, as existing user may already have stale quota
data
2. fix the tests to use the dummy user
3. a boundary condition was revealed and fixed for a new user where
quota service has never run and created bootstrap entries
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shapeblue/cloudstack master-9161
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1240.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 #1240
----
commit b9ff4abadecc44924b0f77ebd2c8cb2d4f9cc253
Author: Abhinandan Prateek <ab...@shapeblue.com>
Date: 2015-12-14T06:40:48Z
CLOUDSTACK-9161: fix the quota marvin test
1. Create a dummy user, as existing user may already have stale quota
data
2. fix the tests to use the dummy user
3. a boundary condition was revealed and fixed for a new user where
quota service has never run and created bootstrap entries
----
---
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-9161: fix the quota marvin tes...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1240
---
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-9161: fix the quota marvin tes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-165062191
@agneya2001 your solution LGTM
@remibergsma will you put it in your regression suite anyway? It does run in the bubble (limitation of warranty; I tested that before the move to test/integration/plugin)
---
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-9161: fix the quota marvin tes...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-175560143
2 PRs, merging 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-9161: fix the quota marvin tes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-164483340
@agneya2001 The definition as I gave you is used in th 'bubble' to create the environment (including settings) before running the test in a deployDatacenter.py run.
---
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-9161: fix the quota marvin tes...
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-164405451
@DaanHoogland Let me add the DC definition, I still need to figure the howto part.
---
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-9161: fix the quota marvin tes...
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-164432291
@DaanHoogland I have put a check that if the quota is not enabled then the tests are skipped. I tried enabling quota from inside the test, but it requires a MS restart so will not work.
Also all we will be testing is API on a empty dataset, the real meat is in quota generation, but that is outside scope of marvin tests.
---
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-9161: fix the quota marvin tes...
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-165001244
Moving the test to test/integration/plugins folder as in most automated environment this will either fail or not run, due to the MS restart required to enable the plugin.
---
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-9161: fix the quota marvin tes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1240#discussion_r47484403
--- Diff: plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java ---
@@ -186,7 +186,7 @@ public int compare(QuotaBalanceVO o1, QuotaBalanceVO o2) {
//check that there is at least one balance entry
for (Iterator<QuotaBalanceVO> it = quotaBalance.iterator(); it.hasNext();) {
QuotaBalanceVO entry = it.next();
- if (entry.getCreditsId() > 0) {
+ if (entry.getCreditsId() == 0) {
--- End diff --
ok, i understand, i think, thanks. At least this code makes sense 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-9161: fix the quota marvin tes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1240#discussion_r47484456
--- Diff: plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java ---
@@ -186,7 +186,7 @@ public int compare(QuotaBalanceVO o1, QuotaBalanceVO o2) {
//check that there is at least one balance entry
for (Iterator<QuotaBalanceVO> it = quotaBalance.iterator(); it.hasNext();) {
QuotaBalanceVO entry = it.next();
- if (entry.getCreditsId() > 0) {
+ if (entry.getCreditsId() == 0) {
--- End diff --
BTW i would factor this out as a boolean checker 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-9161: fix the quota marvin tes...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-170454494
@remibergsma ping, let's merge this before the freeze?
---
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-9161: fix the quota marvin tes...
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1240#discussion_r47481989
--- Diff: plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java ---
@@ -186,7 +186,7 @@ public int compare(QuotaBalanceVO o1, QuotaBalanceVO o2) {
//check that there is at least one balance entry
for (Iterator<QuotaBalanceVO> it = quotaBalance.iterator(); it.hasNext();) {
QuotaBalanceVO entry = it.next();
- if (entry.getCreditsId() > 0) {
+ if (entry.getCreditsId() == 0) {
--- End diff --
@DaanHoogland good that you are probing the logic.
In a system where quota service has never run, there could be credit entries a admin may want to add some credit entries even before quota service has run. So at a point the db can have credit entries without any real balance entry. There could be a situation where there is a balance entry after several credit entries followed by another set of credit entries.
When there is a balance entry it consolidates all the credit entries that happened before it.
So when calculating balance you need to add all the credit entries that happened after a balance entry while ignoring all the credit entries that happened before the first balance entry. You till need to show all the credit entries that happened in between for the balance statement.
When marvin tests were run the db was clean with only one credit entry. This was a special case where you add all the credit entries to show the final balance but you still need to show this credit entry in the balance statement. If quota service has run once it makes a 0 balance entry as part of bootstrapping.
entry.getCreditsId() == 0, looks for real balance entry. A credit id that is non-zero refers to a credit entry with id being the credit id.
---
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-9161: fix the quota marvin tes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1240#discussion_r47480556
--- Diff: plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java ---
@@ -186,7 +186,7 @@ public int compare(QuotaBalanceVO o1, QuotaBalanceVO o2) {
//check that there is at least one balance entry
for (Iterator<QuotaBalanceVO> it = quotaBalance.iterator(); it.hasNext();) {
QuotaBalanceVO entry = it.next();
- if (entry.getCreditsId() > 0) {
+ if (entry.getCreditsId() == 0) {
--- End diff --
So in every set of balance entries between any start and end date there is supposed to be one with creditId == 0?
Also whether one or none are found this loop ends. How does this check work?
---
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-9161: fix the quota marvin tes...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-167042039
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-9161: fix the quota marvin tes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-164406128
i used this file, not all is relevant but the global setting for it are in there.
```
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
{
"zones": [
{
"name": "MCCT-SHARED-1",
"guestcidraddress": "10.1.1.0/24",
"dns1": "8.8.8.8",
"physical_networks": [
{
"broadcastdomainrange": "Zone",
"vlan": "100-200",
"name": "mcct-pnet",
"traffictypes": [
{
"typ": "Guest"
},
{
"typ": "Management"
},
{
"typ": "Public"
}
],
"providers": [
{
"broadcastdomainrange": "ZONE",
"name": "VirtualRouter"
},
{
"broadcastdomainrange": "ZONE",
"name": "VpcVirtualRouter"
},
{
"broadcastdomainrange": "ZONE",
"name": "InternalLbVm"
}
],
"isolationmethods": [
"VLAN"
]
}
],
"ipranges": [
{
"startip": "192.168.23.2",
"endip": "192.168.23.20",
"netmask": "255.255.255.0",
"vlan": "50",
"gateway": "192.168.23.1"
}
],
"networktype": "Advanced",
"pods": [
{
"endip": "192.168.22.150",
"name": "MCCT-POD",
"startip": "192.168.22.130",
"netmask": "255.255.255.0",
"clusters": [
],
"clusters": [
{
"clustername": "MCCT-KVM-1",
"hypervisor": "KVM",
"hosts": [
{
"username": "root",
"url": "http://kvm1",
"password": "password"
},
{
"username": "root",
"url": "http://kvm2",
"password": "password"
}
],
"clustertype": "CloudManaged"
}
],
"gateway": "192.168.22.1"
}
],
"internaldns1": "8.8.4.4",
"secondaryStorages": [
{
"url": "nfs://192.168.22.1:/data/storage/secondary/MCCT-SHARED-1",
"provider" : "NFS"
}
],
"primaryStorages": [
{
"url": "nfs://192.168.22.1:/data/storage/primary/MCCT-KVM-1",
"name": "MCCT-KVM-1-primary",
"hypervisor": "kvm"
}
]
}
],
"dbSvr": {
"dbSvr": "localhost",
"passwd": "cloud",
"db": "cloud",
"port": 3306,
"user": "cloud"
},
"logger":
{
"LogFolderPath": "/tmp/"
},
"globalConfig": [
{
"name": "quota.enable.enforcement",
"value": "true"
},
{
"name": "quota.enable.service",
"value": "true"
},
{
"name": "network.gc.wait",
"value": "60"
},
{
"name": "storage.cleanup.interval",
"value": "300"
},
{
"name": "vm.op.wait.interval",
"value": "5"
},
{
"name": "default.page.size",
"value": "10000"
},
{
"name": "network.gc.interval",
"value": "60"
},
{
"name": "workers",
"value": "10"
},
{
"name": "account.cleanup.interval",
"value": "600"
},
{
"name": "guest.domain.suffix",
"value": "cloud"
},
{
"name": "expunge.delay",
"value": "60"
},
{
"name": "vm.allocation.algorithm",
"value": "firstfitleastconsumed"
},
{
"name": "expunge.interval",
"value": "60"
},
{
"name": "expunge.workers",
"value": "3"
},
{
"name": "check.pod.cidrs",
"value": "true"
},
{
"name": "secstorage.allowed.internal.sites",
"value": "192.168.22.0/24"
},
{
"name": "direct.agent.load.size",
"value": "1000"
}
],
"mgtSvr": [
{
"mgtSvrIp": "localhost",
"passwd": "password",
"user": "root",
"port": 8096,
"hypervisor": "KVM",
"useHttps": "False",
"certCAPath": "NA",
"certPath": "NA"
}
]
}
```
---
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-9161: fix the quota marvin tes...
Posted by agneya2001 <gi...@git.apache.org>.
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-164652898
@DaanHoogland this is what I did:
1. Created a simulator environment
2. Deployed the datacenter using the cfg above,
3. checked the DB and confirmed that the plugin is enabled
4. The tests still fail as to make the quota APIs available the MS needs to be restarted.
5. Restarted management server, all 7 tests ran successfully.
---
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-9161: fix the quota marvin tes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1240#issuecomment-164401474
@agneya2001 code looks good, one question about the logic. will run the test.
As this test requires global settings in the server, it would pay to add a DC definition for marvin with this one.
---
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.
---