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/04/03 03:00:29 UTC

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