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, 26 Mar 2013 09:11:13 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
> 
> 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
> 
>


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