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