incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dave Cahill" <dcah...@midokura.com>
Subject Re: Review Request: MidoNet Networking Plugin [2/2]
Date Thu, 14 Mar 2013 06:16:11 GMT


> On March 14, 2013, 3:59 a.m., Hiroaki Kawai wrote:
> > The patch set is so big that it is hard to review. It would be nice to separate
it into management-server, plugin, agent plugin and UI.

Thanks for the review Kawai-san!

With regard to the patch size, we followed the example of the Big Switch plugin by adding
the plugin in one shot, see: http://markmail.org/message/5c56kylaitmgu7tb

Some of the size is due to trailing whitespace fixes on existing files - we ran the git pre-commit
hook as advised here: http://markmail.org/message/vataj37cy4mnukzg


> On March 14, 2013, 3:59 a.m., Hiroaki Kawai wrote:
> > api/src/com/cloud/network/Network.java, line 140
> > <https://reviews.apache.org/r/9898/diff/1/?file=270107#file270107line140>
> >
> >     IMHO, plugins may not have to create a Provider instance here. Instead, it can
be created at plugin's configure method or getProvider, etc.

The MidoNet Provider already existed at this place in the code, as did the Nicira Provider;
we just renamed it. 

We can remove it easily though; we'll upload a new patch once this is done.


> On March 14, 2013, 3:59 a.m., Hiroaki Kawai wrote:
> > server/src/com/cloud/network/NetworkManagerImpl.java, line 1773
> > <https://reviews.apache.org/r/9898/diff/1/?file=270127#file270127line1773>
> >
> >     NetworkElement should be always called after NetworkGuru, isn't it?

Looking at NetworkManagerImpl, Element does seem to be called after Guru most of the time
- however in destroyNetwork(), the Elements are destroyed before the Guru.

I'm happy to change the order, but since the order is not always the same in NetworkManagerImpl,
do you have any document references / reasons for the suggested order? 

Since we have tested the current version and it works, I just want to make sure the change
makes sense before implementing.


- Dave


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9898/#review17849
-----------------------------------------------------------


On March 13, 2013, 9:32 a.m., Dave Cahill wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9898/
> -----------------------------------------------------------
> 
> (Updated March 13, 2013, 9:32 a.m.)
> 
> 
> Review request for cloudstack, Hugo Trippaers and Chiradeep Vittal.
> 
> 
> Description
> -------
> 
> Feature spec:
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Midokura+Networking+Plugin
> 
> Jira ticket:
> https://issues.apache.org/jira/browse/CLOUDSTACK-996
> 
> Notes:
> 
> * Moved plugin to nonoss since the MidoNet client jar is not publicly available
> 
> * Documentation will follow as a separate commit
> 
> * One main difference from existing networking plugins is the lack of a Resource class;
we didn't feel it was necessary in this case. As mentioned in Extending CloudStack Networking
[1]:
> "Just like managers, resources are not strictly necessary. In theory a Network Element
could implement a client for the API of the new controller and therefore be completely self-contained."
> 
> * We allow overriding Public traffic via the MidoNetPublicNetworkGuru. We checked this
approach with the list [2] and received no comments, so we're going with it for now.
> 
> [1] https://cwiki.apache.org/CLOUDSTACK/extending-cloudstack-networking.html
> [2] http://markmail.org/message/k5qse63eyylszm3i
> 
> 
> This addresses bug CLOUDSTACK-996.
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/network/Network.java efed5cd 
>   api/src/com/cloud/network/Networks.java e3d2158 
>   api/src/com/cloud/network/PhysicalNetwork.java 343a2b1 
>   api/src/org/apache/cloudstack/network/ExternalNetworkDeviceManager.java bc22804 
>   client/pom.xml cda6ab8 
>   client/tomcatconf/nonossComponentContext.xml.in 20e0c32 
>   deps/install-non-oss.sh 74575a8 
>   plugins/hypervisors/kvm/pom.xml 013a58d 
>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
b622b6d 
>   plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java c93aeeb

>   plugins/network-elements/midokura-midonet/pom.xml 7f2e2d3 
>   plugins/network-elements/midonet/pom.xml PRE-CREATION 
>   plugins/network-elements/midonet/src/com/cloud/network/element/MidoNetElement.java
48833b3 
>   plugins/network-elements/midonet/src/com/cloud/network/element/SimpleFirewallRule.java
PRE-CREATION 
>   plugins/network-elements/midonet/src/com/cloud/network/guru/MidoNetGuestNetworkGuru.java
ed0eb3c 
>   plugins/network-elements/midonet/src/com/cloud/network/guru/MidoNetPublicNetworkGuru.java
PRE-CREATION 
>   plugins/network-elements/midonet/src/com/cloud/network/resource/MidoNetVifDriver.java
PRE-CREATION 
>   plugins/network-elements/midonet/test/com/cloud/network/element/MidoNetElementTest.java
PRE-CREATION 
>   plugins/pom.xml 88f617b 
>   server/src/com/cloud/configuration/Config.java 64465a2 
>   server/src/com/cloud/network/NetworkManagerImpl.java 3220c91 
>   ui/scripts/system.js 4d529ae 
> 
> Diff: https://reviews.apache.org/r/9898/diff/
> 
> 
> Testing
> -------
> 
> Built and deployed, spun up Advanced Isolated network with two VMs, verified internal
and external connectivity via MidoNet.
> 
> 
> Thanks,
> 
> Dave Cahill
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message