Understood. However it is hard to hold an interface steady in such an early project. I hope there are not too many of these non-public implementations (at least, I have not heard of any).
On 4/1/13 7:22 PM, "Hiroaki KAWAI" <ka...@stratosphere.co.jp> wrote: >If we spread a new implementation all over the network elements, >then we might silently break third party network elements that >is not included in apache repository. IMHO, that's the impact. > >(2013/04/02 5:30), Chiradeep Vittal wrote: >> Yes, please. If you add a no-op implementation for all the existing >> network elements then there is no impact. >> >> On 4/1/13 12:28 AM, "Hiroaki Kawai" <ka...@stratosphere.co.jp> wrote: >> >>> >>> >>>> On March 29, 2013, 8 p.m., Chiradeep Vittal wrote: >>>>> I do think an explicit migration interface on NetworkElement is the >>>> right way to do it. This way, network elements can decide explicitly >>>> when and how to handle this state. >>>>> Sprinkling >>>>> if(!nic.getReservationId().equals(context.getReservationId())){ >>>>> // migration operation >>>>> return; >>>>> } >>>>> everywhere is error prone: >>>>> - Implementors of new NetworkElements are not aware of this >>>> indirectly expressed dependency >>>>> - This snippet of code (except for the comment) does not in any way >>>> indicate the operation. >>> >>> I agree that introducing a new interface is a good solution. But the >>>kind >>> of interface changes seems to happen in the next cloudstack >>>refactoring, >>> so I implemented as shown not to change the interface as possible as I >>> can. If we add a new interface, we must spread that implementation for >>> that interface to every plugins anyway. >>> >>> If you do want to add a new interface right now, I'll create another >>> patch. >>> >>> >>> - Hiroaki >>> >>> >>> ----------------------------------------------------------- >>> This is an automatically generated e-mail. To reply, visit: >>> https://reviews.apache.org/r/9871/#review18531 >>> ----------------------------------------------------------- >>> >>> >>> On March 29, 2013, 1:49 a.m., Hiroaki Kawai wrote: >>>> >>>> ----------------------------------------------------------- >>>> This is an automatically generated e-mail. To reply, visit: >>>> https://reviews.apache.org/r/9871/ >>>> ----------------------------------------------------------- >>>> >>>> (Updated March 29, 2013, 1:49 a.m.) >>>> >>>> >>>> Review request for cloudstack, Hugo Trippaers and Chiradeep Vittal. >>>> >>>> >>>> Description >>>> ------- >>>> >>>> The location of the virtual machine is provided by DeployDestination, >>>> which will be passed in NetworkGuru#reserve and >>>>NetworkElement#prepare. >>>> >>>> During the virtual machine migration, it actually changes >>>> DeployDestination and it looks like that it will tell that event to >>>> network components as it has NetworkManager#prepareNicForMigration. >>>>The >>>> problem is that althogh the interface has that method, >>>> NetworkManagerImpl does not tell the DeployDestination changes to >>>> network components. >>>> >>>> So IMHO, we need to add calls of NetworkGuru#reserve and >>>> NetworkElement#prepare in NetworkManagerImpl#prepareNicForMigration . >>>> And then, we also need to add calls NetworkGuru#release and >>>> NetworkElement#release after the migration, otherwise the network >>>> resources that plugin reserved will be kept even when the vm leaves >>>>off. >>>> >>>> Created a first minimum patch to show the concept. >>>> >>>> >>>> This addresses bug CLOUDSTACK-1638. >>>> >>>> >>>> Diffs >>>> ----- >>>> >>>> docs/en-US/plugin-niciranvp-tables.xml 4f81655 >>>> >>>> >>>>plugins/network-elements/nicira-nvp/src/com/cloud/network/NiciraNvpNicM >>>>ap >>>> pingVO.java 0779e69 >>>> >>>> >>>>plugins/network-elements/nicira-nvp/src/com/cloud/network/element/Nicir >>>>aN >>>> vpElement.java 1fcccdb >>>> server/src/com/cloud/network/NetworkManager.java 4124b19 >>>> server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 >>>> server/src/com/cloud/network/guru/ControlNetworkGuru.java 934cd70 >>>> server/src/com/cloud/network/guru/DirectNetworkGuru.java ee824af >>>> server/src/com/cloud/network/guru/DirectPodBasedNetworkGuru.java >>>> 354d7ed >>>> server/src/com/cloud/network/guru/ExternalGuestNetworkGuru.java >>>> 24d24f8 >>>> server/src/com/cloud/network/guru/GuestNetworkGuru.java cebfb08 >>>> server/src/com/cloud/network/guru/PodBasedNetworkGuru.java b513325 >>>> server/src/com/cloud/network/guru/StorageNetworkGuru.java 879d0cd >>>> server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a >>>> setup/db/create-schema.sql 5b6dc04 >>>> >>>> Diff: https://reviews.apache.org/r/9871/diff/ >>>> >>>> >>>> Testing >>>> ------- >>>> >>>> >>>> Thanks, >>>> >>>> Hiroaki Kawai >>>> >>>> >>> >> >