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