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