cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chiradeep Vittal <Chiradeep.Vit...@citrix.com>
Subject Re: Review Request: (CLOUDSTACK-1638) Network plugins won't be notified VM migration.
Date Tue, 02 Apr 2013 06:21:42 GMT
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" <kawai@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" <kawai@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
>>>>
>>>>
>>>
>>
>


Mime
View raw message