You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@cloudstack.apache.org by Dave Cahill <dc...@midokura.com> on 2013/03/13 10:32:25 UTC

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]

Posted by Dave Cahill <dc...@midokura.com>.


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

Re: Review Request: MidoNet Networking Plugin [2/2]

Posted by Dave Cahill <dc...@midokura.com>.


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

Posted by Hugo Trippaers <ht...@schubergphilis.com>.

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

Posted by Hiroaki Kawai <ka...@stratosphere.co.jp>.

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



server/src/com/cloud/network/NetworkManagerImpl.java
<https://reviews.apache.org/r/9898/#comment38591>

    We must not catch bare Exception because it also catches RuntimeException.


- Hiroaki Kawai


On March 25, 2013, 4:02 a.m., Dave Cahill wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9898/
> -----------------------------------------------------------
> 
> (Updated March 25, 2013, 4:02 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:
> 
> * 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 c2ab655 
>   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 7ad2eff 
>   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 39d9907 
>   server/src/com/cloud/configuration/Config.java 9db7dbd 
>   server/src/com/cloud/network/NetworkManagerImpl.java b1236cc 
>   ui/scripts/system.js c0a5d14 
> 
> 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]

Posted by Hugo Trippaers <ht...@schubergphilis.com>.


> On April 3, 2013, 10:11 a.m., Hugo Trippaers wrote:
> > Ship It!

commit eddf7b9357bc18497b8cb16a6c6f3382ac52f61c
Author: Dave Cahill <dc...@midokura.com>
Date:   Mon Mar 25 10:56:13 2013 +0900

    MidoNet Networking Plugin

    - Supports DHCP, Source NAT, Static NAT, Firewall rules, Port Forwarding
    - Renamed MidokuraMidonet to MidoNet
    - Related Jira ticket is CLOUDSTACK-996

    Signed-off-by: Dave Cahill <dc...@midokura.com>
    Signed-off-by: Hugo Trippaers <ht...@schubergphilis.com>


- Hugo


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


On March 27, 2013, 6:33 a.m., Dave Cahill wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9898/
> -----------------------------------------------------------
> 
> (Updated March 27, 2013, 6:33 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:
> 
> * 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 c2ab655 
>   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 7ad2eff 
>   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 39d9907 
>   server/src/com/cloud/configuration/Config.java 9db7dbd 
>   server/src/com/cloud/network/NetworkManagerImpl.java b1236cc 
>   ui/scripts/system.js c0a5d14 
> 
> 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]

Posted by Hugo Trippaers <ht...@schubergphilis.com>.

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

Ship it!


Ship It!

- Hugo Trippaers


On March 27, 2013, 6:33 a.m., Dave Cahill wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9898/
> -----------------------------------------------------------
> 
> (Updated March 27, 2013, 6:33 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:
> 
> * 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 c2ab655 
>   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 7ad2eff 
>   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 39d9907 
>   server/src/com/cloud/configuration/Config.java 9db7dbd 
>   server/src/com/cloud/network/NetworkManagerImpl.java b1236cc 
>   ui/scripts/system.js c0a5d14 
> 
> 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]

Posted by Hugo Trippaers <HT...@schubergphilis.com>.

Apologies for the long wait and thanks for your patience. I've applied your patches to the master branch :-)

commit 44567453e0511e3090ac22518113b283cfa26b4b
Author: Hugo Trippaers <ht...@schubergphilis.com>
Date:   Wed Apr 3 12:04:58 2013 +0200

    Enable the midonet plugin

commit eddf7b9357bc18497b8cb16a6c6f3382ac52f61c
Author: Dave Cahill <dc...@midokura.com>
Date:   Mon Mar 25 10:56:13 2013 +0900

    MidoNet Networking Plugin

    - Supports DHCP, Source NAT, Static NAT, Firewall rules, Port Forwarding
    - Renamed MidokuraMidonet to MidoNet
    - Related Jira ticket is CLOUDSTACK-996

    Signed-off-by: Dave Cahill <dc...@midokura.com>
    Signed-off-by: Hugo Trippaers <ht...@schubergphilis.com>

commit d392445f7e9adab7d6effd4daf0d76bb0e6f9bd9
Author: Dave Cahill <dc...@midokura.com>
Date:   Wed Mar 13 17:27:09 2013 +0900

    Rename MidoNetElement and MidoNetGuestNetworkGuru

    - Creating this as a separate commit so that it is marked as a rename
    - Over 50% code changed, so would count as a delete and add otherwise

    Signed-off-by: Dave Cahill <dc...@midokura.com>
    Signed-off-by: Hugo Trippaers htrippaers@schubergphilis.com<ma...@schubergphilis.com>


Cheers,

Hugo



From: dcahill@midokura.jp [mailto:dcahill@midokura.jp] On Behalf Of Dave Cahill
Sent: Wednesday, April 03, 2013 3:00 AM
To: Hugo Trippaers
Cc: Chiradeep Vittal; cloudstack; Hiroaki Kawai; joe mills
Subject: Re: Review Request: MidoNet Networking Plugin [2/2]

Checking back in - any progress on this review? We're now at the 3 week point from initial submission.

Hugo, is your Ship It (once test run completes) equivalent to a "commit now", or is there an additional review required? I don't see any other review activity, so I'm guessing you're the only one actively looking at this at the moment.

On Fri, Mar 29, 2013 at 11:37 PM, Dave Cahill <dc...@midokura.com>> wrote:
Sounds great, thanks Hugo!

On Fri, Mar 29, 2013 at 8:47 PM, Hugo Trippaers <HT...@schubergphilis.com>> wrote:
Hey Dave,

My "ship it" is waiting for a last compile and test run on my dev platform. I'll try to do that over the weekend.

 Cheers,

Hugo

Sent from my iPhone

On 29 mrt. 2013, at 02:17, "Dave Cahill" <dc...@midokura.com>> wrote:
Hi all,

I think all review comments have been addressed on this, but review progress seems to have stalled - anything I should be doing to keep things moving?

Thanks,
Dave.

On Wed, Mar 27, 2013 at 3:33 PM, Dave Cahill <dc...@midokura.com>> wrote:
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.
By Dave Cahill.

Updated March 27, 2013, 6:33 a.m.

Changes

Fixed Hiroaki's exception-related review comment.


Description

Feature spec:

https://cwiki.apache.org/confluence/display/CLOUDSTACK/Midokura+Networking+Plugin



Jira ticket:

https://issues.apache.org/jira/browse/CLOUDSTACK-996



Notes:



* 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


Testing

Built and deployed, spun up Advanced Isolated network with two VMs, verified internal and external connectivity via MidoNet.

Bugs: CLOUDSTACK-996
Diffs (updated)

  *   api/src/com/cloud/network/Network.java (c2ab655)
  *   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 (7ad2eff)
  *   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 (39d9907)
  *   server/src/com/cloud/configuration/Config.java (9db7dbd)
  *   server/src/com/cloud/network/NetworkManagerImpl.java (b1236cc)
  *   ui/scripts/system.js (c0a5d14)

View Diff<https://reviews.apache.org/r/9898/diff/>

Re: Review Request: MidoNet Networking Plugin [2/2]

Posted by Dave Cahill <dc...@midokura.com>.

Checking back in - any progress on this review? We're now at the 3 week
point from initial submission.

Hugo, is your Ship It (once test run completes) equivalent to a "commit
now", or is there an additional review required? I don't see any other
review activity, so I'm guessing you're the only one actively looking at
this at the moment.


On Fri, Mar 29, 2013 at 11:37 PM, Dave Cahill <dc...@midokura.com> wrote:

> Sounds great, thanks Hugo!
>
>
> On Fri, Mar 29, 2013 at 8:47 PM, Hugo Trippaers <
> HTrippaers@schubergphilis.com> wrote:
>
>>  Hey Dave,
>>
>>  My "ship it" is waiting for a last compile and test run on my dev
>> platform. I'll try to do that over the weekend.
>>
>>   Cheers,
>>
>>  Hugo
>>
>> Sent from my iPhone
>>
>> On 29 mrt. 2013, at 02:17, "Dave Cahill" <dc...@midokura.com> wrote:
>>
>>   Hi all,
>>
>>  I think all review comments have been addressed on this, but review
>> progress seems to have stalled - anything I should be doing to keep things
>> moving?
>>
>>  Thanks,
>> Dave.
>>
>>
>> On Wed, Mar 27, 2013 at 3:33 PM, Dave Cahill <dc...@midokura.com>wrote:
>>
>>>    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.
>>> By Dave Cahill.
>>>
>>> *Updated March 27, 2013, 6:33 a.m.*
>>> Changes
>>>
>>> Fixed Hiroaki's exception-related review comment.
>>>
>>>   Description
>>>
>>> Feature spec:https://cwiki.apache.org/confluence/display/CLOUDSTACK/Midokura+Networking+Plugin
>>>
>>> Jira ticket:https://issues.apache.org/jira/browse/CLOUDSTACK-996
>>>
>>> Notes:
>>>
>>> * 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
>>>
>>>   Testing
>>>
>>> Built and deployed, spun up Advanced Isolated network with two VMs, verified internal and external connectivity via MidoNet.
>>>
>>>   *Bugs: *CLOUDSTACK-996
>>> Diffs (updated)
>>>
>>>    - api/src/com/cloud/network/Network.java (c2ab655)
>>>    - 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 (7ad2eff)
>>>    - 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 (39d9907)
>>>    - server/src/com/cloud/configuration/Config.java (9db7dbd)
>>>    - server/src/com/cloud/network/NetworkManagerImpl.java (b1236cc)
>>>    - ui/scripts/system.js (c0a5d14)
>>>
>>> View Diff <https://reviews.apache.org/r/9898/diff/>
>>>
>>
>>
>

Re: Review Request: MidoNet Networking Plugin [2/2]

Posted by Dave Cahill <dc...@midokura.com>.

Sounds great, thanks Hugo!


On Fri, Mar 29, 2013 at 8:47 PM, Hugo Trippaers <
HTrippaers@schubergphilis.com> wrote:

>  Hey Dave,
>
>  My "ship it" is waiting for a last compile and test run on my dev
> platform. I'll try to do that over the weekend.
>
>   Cheers,
>
>  Hugo
>
> Sent from my iPhone
>
> On 29 mrt. 2013, at 02:17, "Dave Cahill" <dc...@midokura.com> wrote:
>
>   Hi all,
>
>  I think all review comments have been addressed on this, but review
> progress seems to have stalled - anything I should be doing to keep things
> moving?
>
>  Thanks,
> Dave.
>
>
> On Wed, Mar 27, 2013 at 3:33 PM, Dave Cahill <dc...@midokura.com> wrote:
>
>>    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.
>> By Dave Cahill.
>>
>> *Updated March 27, 2013, 6:33 a.m.*
>> Changes
>>
>> Fixed Hiroaki's exception-related review comment.
>>
>>   Description
>>
>> Feature spec:https://cwiki.apache.org/confluence/display/CLOUDSTACK/Midokura+Networking+Plugin
>>
>> Jira ticket:https://issues.apache.org/jira/browse/CLOUDSTACK-996
>>
>> Notes:
>>
>> * 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
>>
>>   Testing
>>
>> Built and deployed, spun up Advanced Isolated network with two VMs, verified internal and external connectivity via MidoNet.
>>
>>   *Bugs: *CLOUDSTACK-996
>> Diffs (updated)
>>
>>    - api/src/com/cloud/network/Network.java (c2ab655)
>>    - 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 (7ad2eff)
>>    - 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 (39d9907)
>>    - server/src/com/cloud/configuration/Config.java (9db7dbd)
>>    - server/src/com/cloud/network/NetworkManagerImpl.java (b1236cc)
>>    - ui/scripts/system.js (c0a5d14)
>>
>> View Diff <https://reviews.apache.org/r/9898/diff/>
>>
>
>

Re: Review Request: MidoNet Networking Plugin [2/2]

Posted by Hugo Trippaers <HT...@schubergphilis.com>.

Hey Dave,

My "ship it" is waiting for a last compile and test run on my dev platform. I'll try to do that over the weekend.

 Cheers,

Hugo

Sent from my iPhone

On 29 mrt. 2013, at 02:17, "Dave Cahill" <dc...@midokura.com>> wrote:

Hi all,

I think all review comments have been addressed on this, but review progress seems to have stalled - anything I should be doing to keep things moving?

Thanks,
Dave.


On Wed, Mar 27, 2013 at 3:33 PM, Dave Cahill <dc...@midokura.com>> wrote:
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.
By Dave Cahill.

Updated March 27, 2013, 6:33 a.m.

Changes

Fixed Hiroaki's exception-related review comment.


Description

Feature spec:
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Midokura+Networking+Plugin

Jira ticket:
https://issues.apache.org/jira/browse/CLOUDSTACK-996

Notes:

* 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


Testing

Built and deployed, spun up Advanced Isolated network with two VMs, verified internal and external connectivity via MidoNet.


Bugs: CLOUDSTACK-996
Diffs (updated)

  *   api/src/com/cloud/network/Network.java (c2ab655)
  *   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 (7ad2eff)
  *   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 (39d9907)
  *   server/src/com/cloud/configuration/Config.java (9db7dbd)
  *   server/src/com/cloud/network/NetworkManagerImpl.java (b1236cc)
  *   ui/scripts/system.js (c0a5d14)

View Diff<https://reviews.apache.org/r/9898/diff/>

Re: Review Request: MidoNet Networking Plugin [2/2]

Posted by Dave Cahill <dc...@midokura.com>.

Hi all,

I think all review comments have been addressed on this, but review
progress seems to have stalled - anything I should be doing to keep things
moving?

Thanks,
Dave.


On Wed, Mar 27, 2013 at 3:33 PM, Dave Cahill <dc...@midokura.com> wrote:

>    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.
> By Dave Cahill.
>
> *Updated March 27, 2013, 6:33 a.m.*
> Changes
>
> Fixed Hiroaki's exception-related review comment.
>
>   Description
>
> Feature spec:https://cwiki.apache.org/confluence/display/CLOUDSTACK/Midokura+Networking+Plugin
>
> Jira ticket:https://issues.apache.org/jira/browse/CLOUDSTACK-996
>
> Notes:
>
> * 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
>
>   Testing
>
> Built and deployed, spun up Advanced Isolated network with two VMs, verified internal and external connectivity via MidoNet.
>
>   *Bugs: * CLOUDSTACK-996
> Diffs (updated)
>
>    - api/src/com/cloud/network/Network.java (c2ab655)
>    - 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 (7ad2eff)
>    - 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 (39d9907)
>    - server/src/com/cloud/configuration/Config.java (9db7dbd)
>    - server/src/com/cloud/network/NetworkManagerImpl.java (b1236cc)
>    - ui/scripts/system.js (c0a5d14)
>
> View Diff <https://reviews.apache.org/r/9898/diff/>
>

Re: Review Request: MidoNet Networking Plugin [2/2]

Posted by Dave Cahill <dc...@midokura.com>.

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

(Updated March 27, 2013, 6:33 a.m.)


Review request for cloudstack, Hugo Trippaers and Chiradeep Vittal.


Changes
-------

Fixed Hiroaki's exception-related review comment.


Description
-------

Feature spec:
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Midokura+Networking+Plugin

Jira ticket:
https://issues.apache.org/jira/browse/CLOUDSTACK-996

Notes:

* 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 (updated)
-----

  api/src/com/cloud/network/Network.java c2ab655 
  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 7ad2eff 
  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 39d9907 
  server/src/com/cloud/configuration/Config.java 9db7dbd 
  server/src/com/cloud/network/NetworkManagerImpl.java b1236cc 
  ui/scripts/system.js c0a5d14 

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]

Posted by Dave Cahill <dc...@midokura.com>.

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

(Updated March 25, 2013, 4:02 a.m.)


Review request for cloudstack, Hugo Trippaers and Chiradeep Vittal.


Changes
-------

Rebased from master and updated diff to fix review comments:
* Serving jar from publicly available Maven repo (not Maven central), so plugin is now part of OSS build
* Moving the MidoNet Provider into plugin code

We don't seem to have consensus on Kawai san's question around ordering of NetworkElement calls, so no changes made there.

This is still patch #2, where review board 9897 is the first (simple, file rename) patch.


Description (updated)
-------

Feature spec:
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Midokura+Networking+Plugin

Jira ticket:
https://issues.apache.org/jira/browse/CLOUDSTACK-996

Notes:

* 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 (updated)
-----

  api/src/com/cloud/network/Network.java c2ab655 
  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 7ad2eff 
  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 39d9907 
  server/src/com/cloud/configuration/Config.java 9db7dbd 
  server/src/com/cloud/network/NetworkManagerImpl.java b1236cc 
  ui/scripts/system.js c0a5d14 

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]

Posted by joe mills <jo...@midokura.jp>.


> 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
>     
>
> 
> joe mills wrote:
>     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/%3CB1DF26ECC0458748AC97CECE2DA98D41011D278DAB17@SJCPMAILBOX01.citrite.net%3E
> 
> Hiroaki Kawai wrote:
>     The code seems to mean, NetworkElement/NetworkGuru#release for releasing what was reserved. Information about if there was a reserved network resource or not, is stored in Nic state (Nic.State.Reserved or Nic.State.Reserving). So the flow that NetworkElement#release is not called in NetworkManagerImpl#releaseNic is just a BUG, and I think we can put the NetworkElement#release out of the scope of Start RervationStrategy.

We also thought that the NetworkElement#release call placement was a bug, but after trying out a few things, we found that it needs to stay inside that 'Start' reservation strategy check. This is because when we "stop" Nics with a 'Create' reservation strategy, we should not call NetworkElement#release. This is why the NetworkElement#release call needs to be made within the scope of the 'Start' ReservationStrategy check you are referring to, in releaseNic. Given that limitation (and we agree with you that this doesn't seem ideal), we think the best place for the 'Create' NetworkElement#release call to go is in the removeNic call. Do you agree?


- joe


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


On March 25, 2013, 4:02 a.m., Dave Cahill wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9898/
> -----------------------------------------------------------
> 
> (Updated March 25, 2013, 4:02 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:
> 
> * 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 c2ab655 
>   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 7ad2eff 
>   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 39d9907 
>   server/src/com/cloud/configuration/Config.java 9db7dbd 
>   server/src/com/cloud/network/NetworkManagerImpl.java b1236cc 
>   ui/scripts/system.js c0a5d14 
> 
> 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]

Posted by Dave Cahill <dc...@midokura.com>.


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

Re: Review Request: MidoNet Networking Plugin [2/2]

Posted by Hiroaki Kawai <ka...@stratosphere.co.jp>.


> 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
>     
>
> 
> joe mills wrote:
>     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/%3CB1DF26ECC0458748AC97CECE2DA98D41011D278DAB17@SJCPMAILBOX01.citrite.net%3E

The code seems to mean, NetworkElement/NetworkGuru#release for releasing what was reserved. Information about if there was a reserved network resource or not, is stored in Nic state (Nic.State.Reserved or Nic.State.Reserving). So the flow that NetworkElement#release is not called in NetworkManagerImpl#releaseNic is just a BUG, and I think we can put the NetworkElement#release out of the scope of Start RervationStrategy.


- Hiroaki


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


On March 25, 2013, 4:02 a.m., Dave Cahill wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9898/
> -----------------------------------------------------------
> 
> (Updated March 25, 2013, 4:02 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:
> 
> * 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 c2ab655 
>   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 7ad2eff 
>   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 39d9907 
>   server/src/com/cloud/configuration/Config.java 9db7dbd 
>   server/src/com/cloud/network/NetworkManagerImpl.java b1236cc 
>   ui/scripts/system.js c0a5d14 
> 
> 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]

Posted by Hiroaki Kawai <ka...@stratosphere.co.jp>.


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

Posted by joe mills <jo...@midokura.jp>.


> 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/%3CB1DF26ECC0458748AC97CECE2DA98D41011D278DAB17@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 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]

Posted by Hiroaki Kawai <ka...@stratosphere.co.jp>.

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