Re: Review Request: (CLOUDSTACK-1638) Network plugins won't be notified VM migration.
OK, I would just go ahead and merge it. --Sheng On Thu, May 16, 2013 at 8:03 PM, Hiroaki KAWAI ka...@stratosphere.co.jpwrote: +1 I'm sorry that I could not make time to respond. I'll catch up after the merge. (2013/05/17 3:57), Chip Childers wrote: On Thu, May 16, 2013 at 06:55:30PM +, Chiradeep Vittal wrote: Sheng, I'd suggest that you go ahead and merge the patch. +1
Re: Review Request: (CLOUDSTACK-1638) Network plugins won't be notified VM migration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review20797 --- Commit 7260e8d83f07d90b48c34adaeb227de265019487 in branch refs/heads/master from Sheng Yang sheng.y...@citrix.com [ https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=7260e8d ] CLOUDSTACK-1638: Introduce NetworkMigrationResponder 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. (Sheng Yang: rebase code, add license header) Signed-off-by: Sheng Yang sheng.y...@citrix.com - ASF Subversion and Git Services On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review20799 --- Ship it! Shipped. PVLAN migration support shipped with it. - Sheng Yang On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
Hi Hiroaki, Ping again. I've got a patch(pvlan migration support for xen/kvm) based on your patch here. I was assuming it would be soon for you to submit/merge the patch, but it has been one month since last update. And now the lacking of your patch blocked my fix... Could you tell what's happened to this patch? Do you still want to merge it? Thanks! --Sheng On Fri, May 3, 2013 at 10:39 AM, Sheng Yang sh...@yasker.org wrote: Hi Hiroaki, Any update? --Sheng On Fri, Apr 26, 2013 at 1:28 PM, Sheng Yang sh...@yasker.org wrote: Hi Hiroaki, Any update on the patch? I am planning to use it on PVLAN VM migration support. --Sheng On Thu, Apr 18, 2013 at 6:33 PM, Hiroaki Kawai ka...@stratosphere.co.jpwrote: On April 18, 2013, 10:45 p.m., Chiradeep Vittal wrote: Looks good. Is this the final? Yes. :-) Ready to ship? Any comments from anybody else? - Hiroaki --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review19414 --- On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
Sheng, I'd suggest that you go ahead and merge the patch. On 5/16/13 11:20 AM, Sheng Yang sh...@yasker.org wrote: Hi Hiroaki, Ping again. I've got a patch(pvlan migration support for xen/kvm) based on your patch here. I was assuming it would be soon for you to submit/merge the patch, but it has been one month since last update. And now the lacking of your patch blocked my fix... Could you tell what's happened to this patch? Do you still want to merge it? Thanks! --Sheng On Fri, May 3, 2013 at 10:39 AM, Sheng Yang sh...@yasker.org wrote: Hi Hiroaki, Any update? --Sheng On Fri, Apr 26, 2013 at 1:28 PM, Sheng Yang sh...@yasker.org wrote: Hi Hiroaki, Any update on the patch? I am planning to use it on PVLAN VM migration support. --Sheng On Thu, Apr 18, 2013 at 6:33 PM, Hiroaki Kawai ka...@stratosphere.co.jpwrote: On April 18, 2013, 10:45 p.m., Chiradeep Vittal wrote: Looks good. Is this the final? Yes. :-) Ready to ship? Any comments from anybody else? - Hiroaki --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review19414 --- On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
On Thu, May 16, 2013 at 06:55:30PM +, Chiradeep Vittal wrote: Sheng, I'd suggest that you go ahead and merge the patch. +1
Re: Review Request: (CLOUDSTACK-1638) Network plugins won't be notified VM migration.
Hi Hiroaki, Any update? --Sheng On Fri, Apr 26, 2013 at 1:28 PM, Sheng Yang sh...@yasker.org wrote: Hi Hiroaki, Any update on the patch? I am planning to use it on PVLAN VM migration support. --Sheng On Thu, Apr 18, 2013 at 6:33 PM, Hiroaki Kawai ka...@stratosphere.co.jpwrote: On April 18, 2013, 10:45 p.m., Chiradeep Vittal wrote: Looks good. Is this the final? Yes. :-) Ready to ship? Any comments from anybody else? - Hiroaki --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review19414 --- On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review19414 --- Looks good. Is this the final? - Chiradeep Vittal On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
On April 18, 2013, 10:45 p.m., Chiradeep Vittal wrote: Looks good. Is this the final? Yes. :-) Ready to ship? Any comments from anybody else? - Hiroaki --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review19414 --- On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
On April 18, 2013, 10:45 p.m., Chiradeep Vittal wrote: Looks good. Is this the final? Hiroaki Kawai wrote: Yes. :-) Ready to ship? Any comments from anybody else? Any tests? - Chiradeep --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review19414 --- On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
On April 18, 2013, 10:45 p.m., Chiradeep Vittal wrote: Looks good. Is this the final? Hiroaki Kawai wrote: Yes. :-) Ready to ship? Any comments from anybody else? Chiradeep Vittal wrote: Any tests? Tested functional with our private plugin, which will be public. - Hiroaki --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review19414 --- On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
On April 18, 2013, 10:45 p.m., Chiradeep Vittal wrote: Looks good. Is this the final? Hiroaki Kawai wrote: Yes. :-) Ready to ship? Any comments from anybody else? Chiradeep Vittal wrote: Any tests? Hiroaki Kawai wrote: Tested functional with our private plugin, which will be public. Can you add a unit test or a MockMigrationListener or something so that we know it works - Chiradeep --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review19414 --- On April 11, 2013, 7:22 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 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 - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated April 11, 2013, 7:22 a.m.) Review request for cloudstack, Hugo Trippaers and Chiradeep Vittal. Changes --- I came to the idea of introducing a new separate interface for migration. The patch will have less impact than before. If the network plugin (NetworkGuru and NetworkElement) implement the new interface, that method will be invoked from NetworkManagerImpl during the vm migration. 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 (updated) - api/src/com/cloud/network/NetworkMigrationResponder.java PRE-CREATION server/src/com/cloud/network/NetworkManager.java 4124b19 server/src/com/cloud/network/NetworkManagerImpl.java a98bdd4 server/src/com/cloud/vm/VirtualMachineManagerImpl.java 9230f4a 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.
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.
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.
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.
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.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9871/#review18531
---
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.
- Chiradeep Vittal
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.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated March 28, 2013, 9:10 a.m.) Review request for cloudstack, Hugo Trippaers and Chiradeep Vittal. Changes --- Updated a patch fully to make every NetworkGuru and NetworkElement would be ready for handling migration. I did `mvn test` runs. As it is hard to test everything, I'd like to ask people for testing in a real deployment... For Nicira plugin, I extended the database to track the migration phases. 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 (updated) - 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.
On March 20, 2013, 1:51 a.m., Chiradeep Vittal wrote: Kawai-san, it looks OK since most NetworkElement::release do not do anything, but if you look at the other plugins (e.g., NiciraNvp), they destroy the logical port on the NVP switch when release is called. Perhaps we need a new method in the NetworkElement interface explicitly for migration I'll try creating a patch for NiciraNvp element, rather than adding a new interface. Just a moment... - Hiroaki --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/#review18137 --- On March 15, 2013, 6:51 a.m., Hiroaki Kawai wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9871/ --- (Updated March 15, 2013, 6:51 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 - server/src/com/cloud/network/NetworkManager.java 8b6bf9a server/src/com/cloud/network/NetworkManagerImpl.java ba5ab5d server/src/com/cloud/vm/VirtualMachineManagerImpl.java 0aeef0e Diff: https://reviews.apache.org/r/9871/diff/ Testing --- Thanks, Hiroaki Kawai
