> 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/%3cb1df26ecc0458748ac97cece2da98d41011d278da...@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 > >
