You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Hiroaki Kawai <ka...@stratosphere.co.jp> on 2013/04/01 09:28:15 UTC
Re: Review Request: (CLOUDSTACK-1638) Network plugins won't be notified VM
migration.
> 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/NiciraNvpNicMappingVO.java 0779e69
> plugins/network-elements/nicira-nvp/src/com/cloud/network/element/NiciraNvpElement.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
>
>
Re: Review Request: (CLOUDSTACK-1638) Network plugins won't be
notified VM migration.
Posted by Chiradeep Vittal <Ch...@citrix.com>.
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
>>>>
>>>>
>>>
>>
>
Re: Review Request: (CLOUDSTACK-1638) Network plugins won't be notified
VM migration.
Posted by Hiroaki KAWAI <ka...@stratosphere.co.jp>.
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/NiciraNvpNicMap
>>> pingVO.java 0779e69
>>>
>>> plugins/network-elements/nicira-nvp/src/com/cloud/network/element/NiciraN
>>> 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
>>>
>>>
>>
>
Re: Review Request: (CLOUDSTACK-1638) Network plugins won't be
notified VM migration.
Posted by Chiradeep Vittal <Ch...@citrix.com>.
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/NiciraNvpNicMap
>>pingVO.java 0779e69
>>
>>plugins/network-elements/nicira-nvp/src/com/cloud/network/element/NiciraN
>>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
>>
>>
>