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