Re: Review Request: MidoNet Networking Plugin [2/2]
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? Dave Cahill wrote: 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. Hiroaki Kawai wrote: I want to see a documentation, too. After reading the code again, I think NetworkElement#release should not be called in the context with NetworkGuru#deallocate . Do you really need this code block here? NetworkElement#release should be called with NetworkGuru#release, and you would be able to do something there. Network lifecycle: -- NetworkManagerImpl#implementNetwork - NetworkGuru#implement - NetworkManagerImpl#implementNetworkElementsAndResources -- NetworkElement#implement NetworkManagerImpl#restartNetwork - NetworkManagerImpl#shutdownNetworkElementsAndResources -- NetworkElement#shutdown - NetworkManagerImpl#implemetNetworkElementsAndResources -- NetworkElement#implement NetworkManagerImpl#updateGuestNetwork - NetworkManagerImpl#shutdownNetworkElementsAndResource -- NetworkElement#shutdown - NetworkManagerImpl#implemetNetworkElementsAndResources -- NetworkElement#implement NetworkManagerImpl#shutdownNetwork - NetworkManagerImpl#shutdownNetworkElementsAndResource -- NetworkElement#shutdown - NetworkGuru#shutdown Nic lifecycle: -- NetworkManagerImpl#allocate - NetworkManagerImpl#allocateNic -- NetworkGuru#allocate NetworkManager#prepareNic - NetworkGuru#reserve - NetworkManager#prepareElement -- NetworkElement#prepare NetworkManagerImpl#release - NetworkManagerImpl#releaseNic -- NetworkGuru#release -- NetworkElement#release NetworkManagerImpl#deallocate - NetworkManagerImpl#removeNic -- NetworkGuru#deallocate NetworkElement#release is being called with NetworkGuru#release, but only for NICs that have the start reservation strategy [1]. The start reservation strategy means that the IP address should be assigned on start and released on stop [2]. However, the NICs on the public network are assigned a reservation strategy of 'Create' which means that they should not be released between starts and stops. There are no other calls to element.release, so the public NIC never gets released. This is generally not a problem because with the VirtualRouterElement there are no resources hanging around that NetworkGuru#dealloc and destroying the entire VM won't take care of. But with our plugin, we want to handle the Public network and we do allocate resources that will not go away by simply destroying the VM and calling NetworkGuru#dealloc. Therefore we need the corresponding NetworkElement#release call to happen, and this is the code path that gets executed when we destroy the NIC. Given that background, do you think any changes are needed? [1] https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=blob;f=server/src/com/cloud/network/NetworkManagerImpl.java;h=591910b13c63e88d3d831cd584e4e92e0db14eca;hb=HEAD#l1711 [2] http://mail-archives.apache.org/mod_mbox/incubator-cloudstack-dev/201205.mbox/%3cb1df26ecc0458748ac97cece2da98d41011d278da...@sjcpmailbox01.citrite.net%3E - joe --- 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
Re: Review Request: MidoNet Networking Plugin [2/2]
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? Dave Cahill wrote: 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. I want to see a documentation, too. After reading the code again, I think NetworkElement#release should not be called in the context with NetworkGuru#deallocate . Do you really need this code block here? NetworkElement#release should be called with NetworkGuru#release, and you would be able to do something there. Network lifecycle: -- NetworkManagerImpl#implementNetwork - NetworkGuru#implement - NetworkManagerImpl#implementNetworkElementsAndResources -- NetworkElement#implement NetworkManagerImpl#restartNetwork - NetworkManagerImpl#shutdownNetworkElementsAndResources -- NetworkElement#shutdown - NetworkManagerImpl#implemetNetworkElementsAndResources -- NetworkElement#implement NetworkManagerImpl#updateGuestNetwork - NetworkManagerImpl#shutdownNetworkElementsAndResource -- NetworkElement#shutdown - NetworkManagerImpl#implemetNetworkElementsAndResources -- NetworkElement#implement NetworkManagerImpl#shutdownNetwork - NetworkManagerImpl#shutdownNetworkElementsAndResource -- NetworkElement#shutdown - NetworkGuru#shutdown Nic lifecycle: -- NetworkManagerImpl#allocate - NetworkManagerImpl#allocateNic -- NetworkGuru#allocate NetworkManager#prepareNic - NetworkGuru#reserve - NetworkManager#prepareElement -- NetworkElement#prepare NetworkManagerImpl#release - NetworkManagerImpl#releaseNic -- NetworkGuru#release -- NetworkElement#release NetworkManagerImpl#deallocate - NetworkManagerImpl#removeNic -- NetworkGuru#deallocate - Hiroaki --- 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
Re: Review Request: MidoNet Networking Plugin [2/2]
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
Re: Review Request: MidoNet Networking Plugin [2/2]
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9898/#review17865 --- deps/install-non-oss.sh https://reviews.apache.org/r/9898/#comment37826 I'm not really thrilled by adding components to the non-oss build. What are the possibilities of making the the midonet-client.jar publicly available? If that is not possible it might be worthwhile to discuss if this plugin should be maintained in a separate repository. plugins/hypervisors/kvm/pom.xml https://reviews.apache.org/r/9898/#comment37827 This dependency should only be activated during the nonoss build. plugins/network-elements/midonet/src/com/cloud/network/resource/MidoNetVifDriver.java https://reviews.apache.org/r/9898/#comment37828 Is this always fixed to this host/port or should this be configurable? - Hugo Trippaers 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
Re: Review Request: MidoNet Networking Plugin [2/2]
On March 14, 2013, 8:15 a.m., Hugo Trippaers wrote: Hi Hugo, thanks for the quick review reply. On March 14, 2013, 8:15 a.m., Hugo Trippaers wrote: deps/install-non-oss.sh, line 19 https://reviews.apache.org/r/9898/diff/1/?file=270113#file270113line19 I'm not really thrilled by adding components to the non-oss build. What are the possibilities of making the the midonet-client.jar publicly available? If that is not possible it might be worthwhile to discuss if this plugin should be maintained in a separate repository. Making the client jar public is definitely possible - we were making it nonoss for the initial commit to avoid the jar distribution logistics, but we can certainly tackle it now if needed. How would it work exactly? Could we upload the jar to a maven repo and have the build download it as a dependency, and if so, which maven repo could we upload to? Is there a precedent in the code base we can refer to? On March 14, 2013, 8:15 a.m., Hugo Trippaers wrote: plugins/hypervisors/kvm/pom.xml, line 37 https://reviews.apache.org/r/9898/diff/1/?file=270114#file270114line37 This dependency should only be activated during the nonoss build. Nice catch. Let's see how the nonoss discussion above shakes out before addressing this. On March 14, 2013, 8:15 a.m., Hugo Trippaers wrote: plugins/network-elements/midonet/src/com/cloud/network/resource/MidoNetVifDriver.java, line 55 https://reviews.apache.org/r/9898/diff/1/?file=270123#file270123line55 Is this always fixed to this host/port or should this be configurable? This is configurable - see the // Load Midonet API server location section of the configure() method. - Dave --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9898/#review17865 --- 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
Re: Review Request: MidoNet Networking Plugin [2/2]
On March 14, 2013, 8:15 a.m., Hugo Trippaers wrote: deps/install-non-oss.sh, line 19 https://reviews.apache.org/r/9898/diff/1/?file=270113#file270113line19 I'm not really thrilled by adding components to the non-oss build. What are the possibilities of making the the midonet-client.jar publicly available? If that is not possible it might be worthwhile to discuss if this plugin should be maintained in a separate repository. Dave Cahill wrote: Making the client jar public is definitely possible - we were making it nonoss for the initial commit to avoid the jar distribution logistics, but we can certainly tackle it now if needed. How would it work exactly? Could we upload the jar to a maven repo and have the build download it as a dependency, and if so, which maven repo could we upload to? Is there a precedent in the code base we can refer to? Updating to keep status info on the ticket. Discussed on #cloudstack-dev yesterday, options would be: 1. Upload to Maven Central (several hoops to jump through) 2. Upload to own Maven repo We have started on Option 2, but as it will take a while to complete, we would like to go ahead with the nonoss commit initially. This appears to be the approach Netscaler took; nonoss first, then switch to oss once jar distribution was in place. - Dave --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9898/#review17865 --- 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
Review Request: MidoNet Networking Plugin [2/2]
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9898/ --- 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
Re: Review Request: MidoNet Networking Plugin [2/2]
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9898/#review17849 --- 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. api/src/com/cloud/network/Network.java https://reviews.apache.org/r/9898/#comment37807 IMHO, plugins may not have to create a Provider instance here. Instead, it can be created at plugin's configure method or getProvider, etc. server/src/com/cloud/network/NetworkManagerImpl.java https://reviews.apache.org/r/9898/#comment37808 NetworkElement should be always called after NetworkGuru, isn't it? - Hiroaki Kawai 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
