You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Hugo Trippaers <HT...@schubergphilis.com> on 2013/01/15 10:32:48 UTC
[MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
Hey all,
I would like to merge my branch with master. This branch deals with the items reported in tickets CLOUDSTACK-934, CLOUDSTACK-727, CLOUDSTACK-101. Basically the patch introduces support for dealing with bridges and interfaces on KVM hypervisors running openvswitch. Support is included for both vlan and Nicira NVP based networks.
During development some bugs were discovered, following the commits for those fixes
- 1ceecc92c88c84b99c99547f5c086657ec26f8d7 Fix logic error in ConfigurationServerImpl.java
- 87fe64695305d6a2c505f757ab7607b765c313b7 NPE in KVMStoragePoolManager
Testing done
- mvn clean test
- setup clean CloudStack installation with KVM server using vlan based isolation. Deploy systemvms and several instances on isolated networks and test connectivity.
- reuse existing CloudStack installation with Nicira NVP. Add cluster with KVM hosts. Deploy several instances and networks and test connectivity.
Testing not done
- basic networking based setup, I figured that if advanced networks work, basic networks should work as well.
Risk
- minimal risk, anyone willing to use this feature needs to explicitly enable this by making changes to agent.properties for the cloud-agent. By default the current bridge drivers will be used.
Database changes
- none
Documentation tracked in a separate ticket: CLOUDSTACK-977
Cheers,
Hugo
RE: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
Posted by Sudha Ponnaganti <su...@citrix.com>.
Hi Hugo,
Would it be possible to have test cases and test results to be posted to cwiki/QA for the following stories. Seems like you have done some validation. If there is anyone available from your org that can do some sanity validation and submit results on ACS Master that would be great.
Also do you have any thoughts around regression for future changes that you would be making. I would like to call on community to see if anyone can take up QA for these features. I am a little concern as this is lot of code and Wido indicates that he would not be able to actually test the logic.
All stories are related to Nicira integration
CLOUDSTACK-934,
CLOUDSTACK - 726,
CLOUDSTACK-727,
CLOUDSTACK-101
Thanks
/sudha
-----Original Message-----
From: Wido den Hollander [mailto:wido@widodh.nl]
Sent: Thursday, January 17, 2013 3:47 AM
To: cloudstack-dev@incubator.apache.org
Subject: Re: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
On 01/15/2013 10:32 AM, Hugo Trippaers wrote:
> Hey all,
>
> I would like to merge my branch with master. This branch deals with the items reported in tickets CLOUDSTACK-934, CLOUDSTACK-727, CLOUDSTACK-101. Basically the patch introduces support for dealing with bridges and interfaces on KVM hypervisors running openvswitch. Support is included for both vlan and Nicira NVP based networks.
>
> During development some bugs were discovered, following the commits
> for those fixes
> - 1ceecc92c88c84b99c99547f5c086657ec26f8d7 Fix logic error in
> ConfigurationServerImpl.java
> - 87fe64695305d6a2c505f757ab7607b765c313b7 NPE in
> KVMStoragePoolManager
>
> Testing done
> - mvn clean test
> - setup clean CloudStack installation with KVM server using vlan based isolation. Deploy systemvms and several instances on isolated networks and test connectivity.
> - reuse existing CloudStack installation with Nicira NVP. Add cluster with KVM hosts. Deploy several instances and networks and test connectivity.
>
> Testing not done
> - basic networking based setup, I figured that if advanced networks work, basic networks should work as well.
>
> Risk
> - minimal risk, anyone willing to use this feature needs to explicitly enable this by making changes to agent.properties for the cloud-agent. By default the current bridge drivers will be used.
>
> Database changes
> - none
>
> Documentation tracked in a separate ticket: CLOUDSTACK-977
>
> Cheers,
>
I've been reviewing this, but I haven't been able to test the code since I don't have anything for this running.
I did find some things which I report in random order :) I basically went from commit to commit in the branch and checked it.
Globally I've found some indentation things, like:
switch (_bridgeType) {
case OPENVSWITCH:
getOvsPifs();
break;
case NATIVE:
default:
getPifs();
break;
}
Isn't the indentation missing there for "case"?
Further in for example OvsVifDriver there are some blank lines with spaces or trailing spaces.
You also seem to write if/else-contitions in various ways, like:
if (....) {
}
else {
}
I thought our code convention said we should use:
if (....) {
} else {
}
Not a very big deal, but just to prevent other people from taking over the "habbits".
In OvsVifDriver there is a method called: "deleteExitingLinkLocalRoutTable"
Isn't that a typo? Shouldn't 'Rout' be 'Route'?
What I don't get in LibvirtVMDef is this:
if (_virtualPortType != null) {
netBuilder.append("<virtualport type='" + _virtualPortType + "'>\n");
if (_virtualPortInterfaceId != null) {
netBuilder.append("<parameters interfaceid='" + _virtualPortInterfaceId + "'/>\n");
}
netBuilder.append("</virtualport>\n");
}
Why should a virtualport be added if the interfaceId is null? Should the first if already check if both the type AND id are set? Or can the ID be set after the VM is running? (In OvsVifDriver?)
If so, couldn't that be an issue if the VM is migrated and this XML definition isn't properly transferred to the other host?
When setting the VLAN tag in the XML definition you do this check:
if (_vlanTag != -1) {
netBuilder.append("<vlan trunk='no'>\n<tag id='" + _vlanTag + "'/>\n</vlan>"); }
It's my understanding that VLAN ID 0 and 4095 are reserved VLAN IDs, so shouldn't it be:
if (_vlanTag > 0) {
...
}
This is what I found while doing my first review. Like I said, I can't test the real logic now.
Wido
> Hugo
>
RE: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
Posted by Alex Huang <Al...@citrix.com>.
Can we use [MERGE][ACS41] instead of this format? I don't know how others setup their mail filters but mine are on tags within the '[]' and it doesn't catch this email.
Thanks.
--Alex
> -----Original Message-----
> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
> Sent: Monday, January 21, 2013 3:37 AM
> To: cloudstack-dev@incubator.apache.org
> Subject: RE: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
>
> https://git-wip-us.apache.org/repos/asf?p=incubator-
> cloudstack.git;a=commit;h=a0373fe1ff2001c5a9deee537d6862327f8818cc
>
> Comments inline
>
> > -----Original Message-----
> > From: Wido den Hollander [mailto:wido@widodh.nl]
> > Sent: Thursday, January 17, 2013 12:47 PM
> > To: cloudstack-dev@incubator.apache.org
> > Subject: Re: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
> >
> > On 01/15/2013 10:32 AM, Hugo Trippaers wrote:
> > > Hey all,
> > >
> > > I would like to merge my branch with master. This branch deals with the
> > items reported in tickets CLOUDSTACK-934, CLOUDSTACK-727,
> CLOUDSTACK-
> > 101. Basically the patch introduces support for dealing with bridges and
> > interfaces on KVM hypervisors running openvswitch. Support is included
> for
> > both vlan and Nicira NVP based networks.
> > >
> > > During development some bugs were discovered, following the commits
> > > for those fixes
> > > - 1ceecc92c88c84b99c99547f5c086657ec26f8d7 Fix logic error in
> > > ConfigurationServerImpl.java
> > > - 87fe64695305d6a2c505f757ab7607b765c313b7 NPE in
> > > KVMStoragePoolManager
> > >
> > > Testing done
> > > - mvn clean test
> > > - setup clean CloudStack installation with KVM server using vlan based
> > isolation. Deploy systemvms and several instances on isolated networks
> and
> > test connectivity.
> > > - reuse existing CloudStack installation with Nicira NVP. Add cluster with
> > KVM hosts. Deploy several instances and networks and test connectivity.
> > >
> > > Testing not done
> > > - basic networking based setup, I figured that if advanced networks work,
> > basic networks should work as well.
> > >
> > > Risk
> > > - minimal risk, anyone willing to use this feature needs to explicitly enable
> > this by making changes to agent.properties for the cloud-agent. By default
> > the current bridge drivers will be used.
> > >
> > > Database changes
> > > - none
> > >
> > > Documentation tracked in a separate ticket: CLOUDSTACK-977
> > >
> > > Cheers,
> > >
> >
> > I've been reviewing this, but I haven't been able to test the code since I
> don't
> > have anything for this running.
> >
> > I did find some things which I report in random order :) I basically went
> from
> > commit to commit in the branch and checked it.
> >
> > Globally I've found some indentation things, like:
> >
> > switch (_bridgeType) {
> > case OPENVSWITCH:
> > getOvsPifs();
> > break;
> > case NATIVE:
> > default:
> > getPifs();
> > break;
> > }
> >
> > Isn't the indentation missing there for "case"?
> >
> > Further in for example OvsVifDriver there are some blank lines with spaces
> > or trailing spaces.
>
> Should all be fixed with commit https://git-wip-
> us.apache.org/repos/asf?p=incubator-
> cloudstack.git;a=commit;h=2d69a1855de5110c57010e248f83d88607c668fe
>
> >
> > You also seem to write if/else-contitions in various ways, like:
> >
> > if (....) {
> >
> > }
> > else {
> >
> > }
> >
> > I thought our code convention said we should use:
> >
> > if (....) {
> >
> > } else {
> >
> > }
> >
> > Not a very big deal, but just to prevent other people from taking over the
> > "habbits".
>
> Fixed :-)
>
> >
> > In OvsVifDriver there is a method called: "deleteExitingLinkLocalRoutTable"
> >
> > Isn't that a typo? Shouldn't 'Rout' be 'Route'?
>
> Copy paste error from BridgeVifDriver, fixed.
>
> >
> > What I don't get in LibvirtVMDef is this:
> >
> > if (_virtualPortType != null) {
> > netBuilder.append("<virtualport type='" + _virtualPortType + "'>\n");
> > if (_virtualPortInterfaceId != null) {
> > netBuilder.append("<parameters interfaceid='" +
> _virtualPortInterfaceId
> > + "'/>\n");
> > }
> > netBuilder.append("</virtualport>\n");
> > }
> >
> > Why should a virtualport be added if the interfaceId is null? Should the first
> if
> > already check if both the type AND id are set? Or can the ID be set after the
> > VM is running? (In OvsVifDriver?)
>
> The virtualport with virtual port type is required for LibVirt to know the port
> should be configured using OpenVswitch. Without this the default bridge
> implementation will be used (according to the documentation at
> http://libvirt.org/formatnetwork.html)
>
> The interface id is an optional extension that is required for using
> openvswitch with NiciraNvp. This id is used to match the vif on the hypervisor
> with the port configured on the logical switch in the nicira system.
>
>
> >
> > If so, couldn't that be an issue if the VM is migrated and this XML definition
> > isn't properly transferred to the other host?
> >
> > When setting the VLAN tag in the XML definition you do this check:
> >
> > if (_vlanTag != -1) {
> > netBuilder.append("<vlan trunk='no'>\n<tag id='" + _vlanTag +
> > "'/>\n</vlan>"); }
> >
> > It's my understanding that VLAN ID 0 and 4095 are reserved VLAN IDs, so
> > shouldn't it be:
> >
> > if (_vlanTag > 0) {
> > ...
> > }
>
> True, fixed.
>
> >
> > This is what I found while doing my first review. Like I said, I can't test the
> real
> > logic now.
> >
> > Wido
> >
> > > Hugo
> > >
RE: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=a0373fe1ff2001c5a9deee537d6862327f8818cc
Comments inline
> -----Original Message-----
> From: Wido den Hollander [mailto:wido@widodh.nl]
> Sent: Thursday, January 17, 2013 12:47 PM
> To: cloudstack-dev@incubator.apache.org
> Subject: Re: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
>
> On 01/15/2013 10:32 AM, Hugo Trippaers wrote:
> > Hey all,
> >
> > I would like to merge my branch with master. This branch deals with the
> items reported in tickets CLOUDSTACK-934, CLOUDSTACK-727, CLOUDSTACK-
> 101. Basically the patch introduces support for dealing with bridges and
> interfaces on KVM hypervisors running openvswitch. Support is included for
> both vlan and Nicira NVP based networks.
> >
> > During development some bugs were discovered, following the commits
> > for those fixes
> > - 1ceecc92c88c84b99c99547f5c086657ec26f8d7 Fix logic error in
> > ConfigurationServerImpl.java
> > - 87fe64695305d6a2c505f757ab7607b765c313b7 NPE in
> > KVMStoragePoolManager
> >
> > Testing done
> > - mvn clean test
> > - setup clean CloudStack installation with KVM server using vlan based
> isolation. Deploy systemvms and several instances on isolated networks and
> test connectivity.
> > - reuse existing CloudStack installation with Nicira NVP. Add cluster with
> KVM hosts. Deploy several instances and networks and test connectivity.
> >
> > Testing not done
> > - basic networking based setup, I figured that if advanced networks work,
> basic networks should work as well.
> >
> > Risk
> > - minimal risk, anyone willing to use this feature needs to explicitly enable
> this by making changes to agent.properties for the cloud-agent. By default
> the current bridge drivers will be used.
> >
> > Database changes
> > - none
> >
> > Documentation tracked in a separate ticket: CLOUDSTACK-977
> >
> > Cheers,
> >
>
> I've been reviewing this, but I haven't been able to test the code since I don't
> have anything for this running.
>
> I did find some things which I report in random order :) I basically went from
> commit to commit in the branch and checked it.
>
> Globally I've found some indentation things, like:
>
> switch (_bridgeType) {
> case OPENVSWITCH:
> getOvsPifs();
> break;
> case NATIVE:
> default:
> getPifs();
> break;
> }
>
> Isn't the indentation missing there for "case"?
>
> Further in for example OvsVifDriver there are some blank lines with spaces
> or trailing spaces.
Should all be fixed with commit https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=2d69a1855de5110c57010e248f83d88607c668fe
>
> You also seem to write if/else-contitions in various ways, like:
>
> if (....) {
>
> }
> else {
>
> }
>
> I thought our code convention said we should use:
>
> if (....) {
>
> } else {
>
> }
>
> Not a very big deal, but just to prevent other people from taking over the
> "habbits".
Fixed :-)
>
> In OvsVifDriver there is a method called: "deleteExitingLinkLocalRoutTable"
>
> Isn't that a typo? Shouldn't 'Rout' be 'Route'?
Copy paste error from BridgeVifDriver, fixed.
>
> What I don't get in LibvirtVMDef is this:
>
> if (_virtualPortType != null) {
> netBuilder.append("<virtualport type='" + _virtualPortType + "'>\n");
> if (_virtualPortInterfaceId != null) {
> netBuilder.append("<parameters interfaceid='" + _virtualPortInterfaceId
> + "'/>\n");
> }
> netBuilder.append("</virtualport>\n");
> }
>
> Why should a virtualport be added if the interfaceId is null? Should the first if
> already check if both the type AND id are set? Or can the ID be set after the
> VM is running? (In OvsVifDriver?)
The virtualport with virtual port type is required for LibVirt to know the port should be configured using OpenVswitch. Without this the default bridge implementation will be used (according to the documentation at http://libvirt.org/formatnetwork.html)
The interface id is an optional extension that is required for using openvswitch with NiciraNvp. This id is used to match the vif on the hypervisor with the port configured on the logical switch in the nicira system.
>
> If so, couldn't that be an issue if the VM is migrated and this XML definition
> isn't properly transferred to the other host?
>
> When setting the VLAN tag in the XML definition you do this check:
>
> if (_vlanTag != -1) {
> netBuilder.append("<vlan trunk='no'>\n<tag id='" + _vlanTag +
> "'/>\n</vlan>"); }
>
> It's my understanding that VLAN ID 0 and 4095 are reserved VLAN IDs, so
> shouldn't it be:
>
> if (_vlanTag > 0) {
> ...
> }
True, fixed.
>
> This is what I found while doing my first review. Like I said, I can't test the real
> logic now.
>
> Wido
>
> > Hugo
> >
Re: [MERGE, ACS41] Merge branch cloud-agent-with-openvswitch
Posted by Wido den Hollander <wi...@widodh.nl>.
On 01/15/2013 10:32 AM, Hugo Trippaers wrote:
> Hey all,
>
> I would like to merge my branch with master. This branch deals with the items reported in tickets CLOUDSTACK-934, CLOUDSTACK-727, CLOUDSTACK-101. Basically the patch introduces support for dealing with bridges and interfaces on KVM hypervisors running openvswitch. Support is included for both vlan and Nicira NVP based networks.
>
> During development some bugs were discovered, following the commits for those fixes
> - 1ceecc92c88c84b99c99547f5c086657ec26f8d7 Fix logic error in ConfigurationServerImpl.java
> - 87fe64695305d6a2c505f757ab7607b765c313b7 NPE in KVMStoragePoolManager
>
> Testing done
> - mvn clean test
> - setup clean CloudStack installation with KVM server using vlan based isolation. Deploy systemvms and several instances on isolated networks and test connectivity.
> - reuse existing CloudStack installation with Nicira NVP. Add cluster with KVM hosts. Deploy several instances and networks and test connectivity.
>
> Testing not done
> - basic networking based setup, I figured that if advanced networks work, basic networks should work as well.
>
> Risk
> - minimal risk, anyone willing to use this feature needs to explicitly enable this by making changes to agent.properties for the cloud-agent. By default the current bridge drivers will be used.
>
> Database changes
> - none
>
> Documentation tracked in a separate ticket: CLOUDSTACK-977
>
> Cheers,
>
I've been reviewing this, but I haven't been able to test the code since
I don't have anything for this running.
I did find some things which I report in random order :) I basically
went from commit to commit in the branch and checked it.
Globally I've found some indentation things, like:
switch (_bridgeType) {
case OPENVSWITCH:
getOvsPifs();
break;
case NATIVE:
default:
getPifs();
break;
}
Isn't the indentation missing there for "case"?
Further in for example OvsVifDriver there are some blank lines with
spaces or trailing spaces.
You also seem to write if/else-contitions in various ways, like:
if (....) {
}
else {
}
I thought our code convention said we should use:
if (....) {
} else {
}
Not a very big deal, but just to prevent other people from taking over
the "habbits".
In OvsVifDriver there is a method called: "deleteExitingLinkLocalRoutTable"
Isn't that a typo? Shouldn't 'Rout' be 'Route'?
What I don't get in LibvirtVMDef is this:
if (_virtualPortType != null) {
netBuilder.append("<virtualport type='" + _virtualPortType + "'>\n");
if (_virtualPortInterfaceId != null) {
netBuilder.append("<parameters interfaceid='" +
_virtualPortInterfaceId + "'/>\n");
}
netBuilder.append("</virtualport>\n");
}
Why should a virtualport be added if the interfaceId is null? Should the
first if already check if both the type AND id are set? Or can the ID be
set after the VM is running? (In OvsVifDriver?)
If so, couldn't that be an issue if the VM is migrated and this XML
definition isn't properly transferred to the other host?
When setting the VLAN tag in the XML definition you do this check:
if (_vlanTag != -1) {
netBuilder.append("<vlan trunk='no'>\n<tag id='" + _vlanTag +
"'/>\n</vlan>");
}
It's my understanding that VLAN ID 0 and 4095 are reserved VLAN IDs, so
shouldn't it be:
if (_vlanTag > 0) {
...
}
This is what I found while doing my first review. Like I said, I can't
test the real logic now.
Wido
> Hugo
>
Re: [MERGE] Merge branch api_limit
Posted by David Nalley <da...@gnsa.us>.
On Thu, Jan 17, 2013 at 7:09 PM, Min Chen <mi...@citrix.com> wrote:
> Hey all,
>
> I would like to merge my feature branch api_limit with master. This branch deals with JIRA tickets CLOUDSTACK-618. Basically it implemented a plugin to provide basic support for api rate limiting to avoid malicious attack on CloudStack server. Implementation details can be found in this FS document https://cwiki.apache.org/confluence/display/CLOUDSTACK/API+Request+Throttling. For this release, we have chosen to implement Ehcache based rate limit store. With clearly defined limit store interface, we can easily extend this to provide other limit store implementations based on Memcached or Redis which rely on setting up a dedicated proxy server.
>
> Testing done
>
> I have done the following two kinds of testing during development cycle:
>
> * Unit test to verify ApiRateLimitService pluggable service interface and Limit Store interface methods. These unit testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java.
> * Integration test to verify rate limit feature and new APIs through a running MS. These integration testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/integration/RateLimitIntegrationTest.java. These test cases are assuming that we have a "demo" user account created on your locally running MS.
>
> Risk
>
> This has minimal risk due to its plugin implementation. We can easily disable this feature by removing this plugin from components.xml.in. A potential impact may be that if this is enabled, UI needs to handle this situation (api failed due to over limit) gracefully.
>
> Database changes
> none
>
> Documentation tracked in a separate ticket: CLOUDSTACK-866
>
> Thanks
> -min
>
>
No objection here.
I especially appreciate calling out the risks, and making sure this
had testing in place up front.
--David
Re: [MERGE] Merge branch api_limit
Posted by Min Chen <mi...@citrix.com>.
Merged to master.
Thanks
-min
On 1/17/13 9:28 PM, "prasanna" <ts...@apache.org> wrote:
>On 18 January 2013 10:50, Min Chen <mi...@citrix.com> wrote:
>> This integration test just runs API server responding concurrent api
>> calls, but it assumes that there is a "demo" account (not root admin) in
>> the system to test this feature. I just coded this test to save my
>>manual
>> testing efforts:)
>>
>> Thanks
>> -min
>
>+1 Min - should always save the manual effort when one can. Thanks for
>the detailed spec and I'm sure I could figure out if I applied my lazy
>head a little on how to setup the integration test. But if you could
>detail that under the test plan section I'd be grateful. :)
Re: [MERGE] Merge branch api_limit
Posted by prasanna <ts...@apache.org>.
On 18 January 2013 10:50, Min Chen <mi...@citrix.com> wrote:
> This integration test just runs API server responding concurrent api
> calls, but it assumes that there is a "demo" account (not root admin) in
> the system to test this feature. I just coded this test to save my manual
> testing efforts:)
>
> Thanks
> -min
+1 Min - should always save the manual effort when one can. Thanks for
the detailed spec and I'm sure I could figure out if I applied my lazy
head a little on how to setup the integration test. But if you could
detail that under the test plan section I'd be grateful. :)
Re: [MERGE] Merge branch api_limit
Posted by Min Chen <mi...@citrix.com>.
This integration test just runs API server responding concurrent api
calls, but it assumes that there is a "demo" account (not root admin) in
the system to test this feature. I just coded this test to save my manual
testing efforts:)
Thanks
-min
On 1/17/13 9:15 PM, "prasanna" <ts...@apache.org> wrote:
>On 18 January 2013 10:23, Min Chen <mi...@citrix.com> wrote:
>> Hi Sudha,
>>
>> The unit tests will be automatically included in "mvn clean
>>install" run after my merge. Integration test will not be enabled
>>automatically since it relies on a running MS.
>>
>> Thanks
>> - min
>>
>>
>One can set it to run as part of the 'integration-test' phase of
>maven. Does this run any hypervisors or only the API server responding
>from a pre-seeded database?
Re: [MERGE] Merge branch api_limit
Posted by prasanna <ts...@apache.org>.
On 18 January 2013 10:23, Min Chen <mi...@citrix.com> wrote:
> Hi Sudha,
>
> The unit tests will be automatically included in "mvn clean install" run after my merge. Integration test will not be enabled automatically since it relies on a running MS.
>
> Thanks
> - min
>
>
One can set it to run as part of the 'integration-test' phase of
maven. Does this run any hypervisors or only the API server responding
from a pre-seeded database?
Re: [MERGE] Merge branch api_limit
Posted by Min Chen <mi...@citrix.com>.
Hi Sudha,
The unit tests will be automatically included in "mvn clean install" run after my merge. Integration test will not be enabled automatically since it relies on a running MS.
Thanks
- min
Sent from my iPhone
On Jan 17, 2013, at 6:47 PM, "Sudha Ponnaganti" <su...@citrix.com> wrote:
> Min,
>
> Would these unit tests be automatically included in our Junit suite for next run??
>
> Thanks
> /sudha
>
> -----Original Message-----
> From: Min Chen [mailto:min.chen@citrix.com]
> Sent: Thursday, January 17, 2013 4:10 PM
> To: cloudstack-dev@incubator.apache.org
> Subject: [MERGE] Merge branch api_limit
>
> Hey all,
>
> I would like to merge my feature branch api_limit with master. This branch deals with JIRA tickets CLOUDSTACK-618. Basically it implemented a plugin to provide basic support for api rate limiting to avoid malicious attack on CloudStack server. Implementation details can be found in this FS document https://cwiki.apache.org/confluence/display/CLOUDSTACK/API+Request+Throttling. For this release, we have chosen to implement Ehcache based rate limit store. With clearly defined limit store interface, we can easily extend this to provide other limit store implementations based on Memcached or Redis which rely on setting up a dedicated proxy server.
>
> Testing done
>
> I have done the following two kinds of testing during development cycle:
>
> * Unit test to verify ApiRateLimitService pluggable service interface and Limit Store interface methods. These unit testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java.
> * Integration test to verify rate limit feature and new APIs through a running MS. These integration testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/integration/RateLimitIntegrationTest.java. These test cases are assuming that we have a "demo" user account created on your locally running MS.
>
> Risk
>
> This has minimal risk due to its plugin implementation. We can easily disable this feature by removing this plugin from components.xml.in. A potential impact may be that if this is enabled, UI needs to handle this situation (api failed due to over limit) gracefully.
>
> Database changes
> none
>
> Documentation tracked in a separate ticket: CLOUDSTACK-866
>
> Thanks
> -min
>
>
>
RE: [MERGE] Merge branch api_limit
Posted by Sudha Ponnaganti <su...@citrix.com>.
Min,
Would these unit tests be automatically included in our Junit suite for next run??
Thanks
/sudha
-----Original Message-----
From: Min Chen [mailto:min.chen@citrix.com]
Sent: Thursday, January 17, 2013 4:10 PM
To: cloudstack-dev@incubator.apache.org
Subject: [MERGE] Merge branch api_limit
Hey all,
I would like to merge my feature branch api_limit with master. This branch deals with JIRA tickets CLOUDSTACK-618. Basically it implemented a plugin to provide basic support for api rate limiting to avoid malicious attack on CloudStack server. Implementation details can be found in this FS document https://cwiki.apache.org/confluence/display/CLOUDSTACK/API+Request+Throttling. For this release, we have chosen to implement Ehcache based rate limit store. With clearly defined limit store interface, we can easily extend this to provide other limit store implementations based on Memcached or Redis which rely on setting up a dedicated proxy server.
Testing done
I have done the following two kinds of testing during development cycle:
* Unit test to verify ApiRateLimitService pluggable service interface and Limit Store interface methods. These unit testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java.
* Integration test to verify rate limit feature and new APIs through a running MS. These integration testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/integration/RateLimitIntegrationTest.java. These test cases are assuming that we have a "demo" user account created on your locally running MS.
Risk
This has minimal risk due to its plugin implementation. We can easily disable this feature by removing this plugin from components.xml.in. A potential impact may be that if this is enabled, UI needs to handle this situation (api failed due to over limit) gracefully.
Database changes
none
Documentation tracked in a separate ticket: CLOUDSTACK-866
Thanks
-min
Re: [MERGE] Merge branch api_limit
Posted by Rohit Yadav <ro...@citrix.com>.
+1 It's plugin so wont' affect the core.
Regards.
On 17-Jan-2013, at 4:09 PM, Min Chen <mi...@citrix.com> wrote:
> Hey all,
>
> I would like to merge my feature branch api_limit with master. This branch deals with JIRA tickets CLOUDSTACK-618. Basically it implemented a plugin to provide basic support for api rate limiting to avoid malicious attack on CloudStack server. Implementation details can be found in this FS document https://cwiki.apache.org/confluence/display/CLOUDSTACK/API+Request+Throttling. For this release, we have chosen to implement Ehcache based rate limit store. With clearly defined limit store interface, we can easily extend this to provide other limit store implementations based on Memcached or Redis which rely on setting up a dedicated proxy server.
>
> Testing done
>
> I have done the following two kinds of testing during development cycle:
>
> * Unit test to verify ApiRateLimitService pluggable service interface and Limit Store interface methods. These unit testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java.
> * Integration test to verify rate limit feature and new APIs through a running MS. These integration testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/integration/RateLimitIntegrationTest.java. These test cases are assuming that we have a "demo" user account created on your locally running MS.
>
> Risk
>
> This has minimal risk due to its plugin implementation. We can easily disable this feature by removing this plugin from components.xml.in. A potential impact may be that if this is enabled, UI needs to handle this situation (api failed due to over limit) gracefully.
>
> Database changes
> none
>
> Documentation tracked in a separate ticket: CLOUDSTACK-866
>
> Thanks
> -min
>
>
>
[MERGE] Merge branch api_limit
Posted by Min Chen <mi...@citrix.com>.
Hey all,
I would like to merge my feature branch api_limit with master. This branch deals with JIRA tickets CLOUDSTACK-618. Basically it implemented a plugin to provide basic support for api rate limiting to avoid malicious attack on CloudStack server. Implementation details can be found in this FS document https://cwiki.apache.org/confluence/display/CLOUDSTACK/API+Request+Throttling. For this release, we have chosen to implement Ehcache based rate limit store. With clearly defined limit store interface, we can easily extend this to provide other limit store implementations based on Memcached or Redis which rely on setting up a dedicated proxy server.
Testing done
I have done the following two kinds of testing during development cycle:
* Unit test to verify ApiRateLimitService pluggable service interface and Limit Store interface methods. These unit testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java.
* Integration test to verify rate limit feature and new APIs through a running MS. These integration testcases are located in plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/integration/RateLimitIntegrationTest.java. These test cases are assuming that we have a "demo" user account created on your locally running MS.
Risk
This has minimal risk due to its plugin implementation. We can easily disable this feature by removing this plugin from components.xml.in. A potential impact may be that if this is enabled, UI needs to handle this situation (api failed due to over limit) gracefully.
Database changes
none
Documentation tracked in a separate ticket: CLOUDSTACK-866
Thanks
-min