incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "joe mills" <...@midokura.jp>
Subject Re: Review Request: MidoNet Networking Plugin [2/2]
Date Tue, 19 Mar 2013 01:04:44 GMT


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


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