> 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 > >
