cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hiroaki Kawai" <ka...@stratosphere.co.jp>
Subject Re: Review Request: MidoNet Networking Plugin [2/2]
Date Tue, 26 Mar 2013 08:02:14 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
>     
>
> 
> 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
> 
>


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