You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Murali Reddy <mu...@gmail.com> on 2012/07/06 01:36:40 UTC

Re: Review Request: Nicira NVP integration for CloudStack

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5590/#review8903
-----------------------------------------------------------


Hugo, code in general looks good. Below are some minor comments. Is there is trial version of NVP controller that can used for trying out integration or for testing?

- In CitrixResourceBase it seems to me that Nicira config params are added for every VIF that is created. Should it be done only for the network's using STT isolation?

- In GuestNetworkGuru:

   + In isMyIsolationMethod(), if no isolation method is configured, its treated as acceptable isolation by GuestNetworkGuru. I suggest a guru check only for what isolation it can support.

   + Whats is the rational for canHanlde() being implemented in each of the implementation of GuestNetworkGuru? I think single abstract implementations should be fine. Do you see that logic will change across the implementations?

- CloudStack Account names are not unique. So please use a combination of domain id + account name or some other unique tag when creating a logical switch.

- I see that it is permitted to configure multiple NVP controllers per physical network?  But always use the first controller available for the physical network when implementing the network. Is it intended behavior?

- Not sure you mentioned this in the your earlier mails. Does this implementation work only with Xen? or works with Kvm, Vmware as well?

- Murali Reddy


On June 26, 2012, 4:56 p.m., Hugo Trippaers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5590/
> -----------------------------------------------------------
> 
> (Updated June 26, 2012, 4:56 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Description
> -------
> 
> Patch to add Nicira NVP support to CloudStack. As discussed this patch is related to phase 1, which is basic L2 connectivity. L3 connectivity and integration with the network offering for SNAT will be in phase2.
> 
> 
> Diffs
> -----
> 
>   README.NiciraIntegration PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchPortAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchPortCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchPortAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchPortCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/StartupNiciraNvpCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/to/NicTO.java b65c61ee47c03bf33bcbf2ee10a2f8d6405538f0 
>   api/src/com/cloud/agent/api/to/VirtualMachineTO.java 42d91626e35d20c960561ca1101aad17dc19febb 
>   api/src/com/cloud/api/ApiConstants.java 00ec392d7b930a1ec0d2d77c2cccd035bc9f3321 
>   api/src/com/cloud/host/Host.java 0c9d06d48bfdaf1f491c71cda6e1d878214a2dd1 
>   api/src/com/cloud/network/Network.java 0443a0f41cc112105b48bb80dae58f6f3adc4b8d 
>   api/src/com/cloud/network/Networks.java 84135b8ee45489fb6c284c6cf3ae0356596cbc83 
>   api/src/com/cloud/network/PhysicalNetwork.java e54fe00bef0846ec96a3b3dde43ba433bab6f1ac 
>   client/tomcatconf/components.xml.in 58541a59606aebb69c1e9566934736b1a6f86b21 
>   client/tomcatconf/nicira-nvp_commands.properties.in PRE-CREATION 
>   core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 39172423e36f06f3721a9b108f0f05f5008f5613 
>   core/src/com/cloud/network/nicira/Attachment.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/LogicalSwitch.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/LogicalSwitchPort.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/LogicalSwitchPortList.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/NiciraNvpApi.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/NiciraNvpApiException.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/NiciraNvpTag.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/TransportZoneBinding.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/VifAttachment.java PRE-CREATION 
>   core/src/com/cloud/network/resource/NiciraNvpResource.java PRE-CREATION 
>   server/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java PRE-CREATION 
>   server/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java PRE-CREATION 
>   server/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java PRE-CREATION 
>   server/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java PRE-CREATION 
>   server/src/com/cloud/api/response/NiciraNvpDeviceResponse.java PRE-CREATION 
>   server/src/com/cloud/configuration/DefaultComponentLibrary.java 3bfe84df25dd4027fdaad48b5ea2c24fff2ce432 
>   server/src/com/cloud/host/dao/HostDaoImpl.java 745567dd7d260de5fab0945d72f89a8133cca4c4 
>   server/src/com/cloud/hypervisor/HypervisorGuruBase.java d62e3800542e0394203506b25f5eb44c5fc90308 
>   server/src/com/cloud/network/ExternalNetworkDeviceManager.java e09ff8e9580612ae010f550fc6f0a24b9cfd971d 
>   server/src/com/cloud/network/NetworkManagerImpl.java 1f306ab07d136a785b9b433122fcd21418da12eb 
>   server/src/com/cloud/network/NiciraNvpDeviceVO.java PRE-CREATION 
>   server/src/com/cloud/network/NiciraNvpNicMappingVO.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpDao.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpDaoImpl.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpNicMappingDao.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpNicMappingDaoImpl.java PRE-CREATION 
>   server/src/com/cloud/network/element/NiciraNvpElement.java PRE-CREATION 
>   server/src/com/cloud/network/element/NiciraNvpElementService.java PRE-CREATION 
>   server/src/com/cloud/network/guru/ExternalGuestNetworkGuru.java 6c381f2b18eb559e34b3d4d51329df0b7d8f5afb 
>   server/src/com/cloud/network/guru/GuestNetworkGuru.java f9977102b1f4b7652a87f3b6c7e04e44aae6a2d8 
>   server/src/com/cloud/network/guru/NiciraNvpGuestNetworkGuru.java PRE-CREATION 
>   server/src/com/cloud/network/guru/OvsGuestNetworkGuru.java d031feeb6ebb40155234d6b856746c8a5eab80de 
>   setup/db/create-schema.sql afcee3f9f6f48d1e0da42f9f952018aec0904929 
>   ui/scripts/ui-custom/zoneWizard.js d46bad9800bac5b671fbe40c8456ab2da19e5531 
> 
> Diff: https://reviews.apache.org/r/5590/diff/
> 
> 
> Testing
> -------
> 
> Simple build check
>   clean-all build-all
> 
> Testing of all api calls
>  * addNiciraNvpDevice
>  * deleteNiciraNvpDevice		
>  * listNiciraNvpDevices
>  * listNiciraNvpDeviceNetwork
> 
> Functional testing using the following procedure
> * start from clean db and create zone (with guest traffic on a physical network with stt isolation type)
> * add NiciraNvp network service provider and enable
> * add NiciraNcpDevice to physical network and configure using api
> * create guestnetwork
> * create instance linked to guest network
> * check existence of logical switch and logical ports for routervm and instance
> * check connectivity between routervm and instance
> * destroy host
> * after shutdown of routervm and network check logical switch and ports on nicira (should be gone)
> 
> 
> Thanks,
> 
> Hugo Trippaers
> 
>


Re: Review Request: Nicira NVP integration for CloudStack

Posted by Hugo Trippaers <ht...@schubergphilis.com>.

> On July 5, 2012, 11:36 p.m., Murali Reddy wrote:
> > Hugo, code in general looks good. Below are some minor comments. Is there is trial version of NVP controller that can used for trying out integration or for testing?
> > 
> > - In CitrixResourceBase it seems to me that Nicira config params are added for every VIF that is created. Should it be done only for the network's using STT isolation?
> > 
> > - In GuestNetworkGuru:
> > 
> >    + In isMyIsolationMethod(), if no isolation method is configured, its treated as acceptable isolation by GuestNetworkGuru. I suggest a guru check only for what isolation it can support.
> > 
> >    + Whats is the rational for canHanlde() being implemented in each of the implementation of GuestNetworkGuru? I think single abstract implementations should be fine. Do you see that logic will change across the implementations?
> > 
> > - CloudStack Account names are not unique. So please use a combination of domain id + account name or some other unique tag when creating a logical switch.
> > 
> > - I see that it is permitted to configure multiple NVP controllers per physical network?  But always use the first controller available for the physical network when implementing the network. Is it intended behavior?
> > 
> > - Not sure you mentioned this in the your earlier mails. Does this implementation work only with Xen? or works with Kvm, Vmware as well?

Hey Murali,

Thanks for the review!

I'll have to pass the question about the trial version of the NVP Controller on to Nicira. I've been testing with a Nicira setup at our office and with a setup at their office. Of course you are more than welcome to spend some time at our office to test it here, but it might not be feasible as i am located in the Netherlands. What i can do is provide access to my test servers, would that be helpful? 

Regarding your comments, i tried to answer them point by point below:

1. The other-config in the VIF should only be set for interfaces connected to networks that are controlled by a Nicira NVP. My problem is that i don't seem to have this information available in the createVif call. The option is see if to add a field with other options to the nic template that gets passed to createvif during creation. Would that be alright or is there a better way to do this?

2a. This is for backwards compatibility, the current ExternalGuestNetworkGuru also supports empty isolation method. But i think this is fixed in the UI now, you can no longer select empty isolation when creating an advanced network. I think is should be that ExternalGuestNetworkGuru supports VLAN, OvsGuestNetorkGuru supports GRE and NiciraNVPGuestNetorkGuru supports STT.

2b. My reasoning was that should the guru's become pluggable in the future, the guru should be able to decide what it can and will handle based on its internal logic, which can be more than checking supported isolation type. In fact in there might be overlap if more SDN solutions become available. Technically there is already some overlap as the Nicira solution also supports GRE (i decided with Somik to ignore this and focus on STT)

3. Ok

4. Should not be possible, will fix.

5. For now it only works with Xen, but i will make sure support is extended to other hypervisors. I'll be working with Nicira to see what is possible to support after this bit is done.


I'll take care of the fixes and provide a new patch base on the current master, is that ok? That patch will also include the move of the nicira code to the plugins folder.

Cheers,

Hugo


- Hugo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5590/#review8903
-----------------------------------------------------------


On June 26, 2012, 4:56 p.m., Hugo Trippaers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5590/
> -----------------------------------------------------------
> 
> (Updated June 26, 2012, 4:56 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Description
> -------
> 
> Patch to add Nicira NVP support to CloudStack. As discussed this patch is related to phase 1, which is basic L2 connectivity. L3 connectivity and integration with the network offering for SNAT will be in phase2.
> 
> 
> Diffs
> -----
> 
>   README.NiciraIntegration PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchPortAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchPortCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchPortAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchPortCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/StartupNiciraNvpCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/to/NicTO.java b65c61ee47c03bf33bcbf2ee10a2f8d6405538f0 
>   api/src/com/cloud/agent/api/to/VirtualMachineTO.java 42d91626e35d20c960561ca1101aad17dc19febb 
>   api/src/com/cloud/api/ApiConstants.java 00ec392d7b930a1ec0d2d77c2cccd035bc9f3321 
>   api/src/com/cloud/host/Host.java 0c9d06d48bfdaf1f491c71cda6e1d878214a2dd1 
>   api/src/com/cloud/network/Network.java 0443a0f41cc112105b48bb80dae58f6f3adc4b8d 
>   api/src/com/cloud/network/Networks.java 84135b8ee45489fb6c284c6cf3ae0356596cbc83 
>   api/src/com/cloud/network/PhysicalNetwork.java e54fe00bef0846ec96a3b3dde43ba433bab6f1ac 
>   client/tomcatconf/components.xml.in 58541a59606aebb69c1e9566934736b1a6f86b21 
>   client/tomcatconf/nicira-nvp_commands.properties.in PRE-CREATION 
>   core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 39172423e36f06f3721a9b108f0f05f5008f5613 
>   core/src/com/cloud/network/nicira/Attachment.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/LogicalSwitch.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/LogicalSwitchPort.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/LogicalSwitchPortList.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/NiciraNvpApi.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/NiciraNvpApiException.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/NiciraNvpTag.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/TransportZoneBinding.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/VifAttachment.java PRE-CREATION 
>   core/src/com/cloud/network/resource/NiciraNvpResource.java PRE-CREATION 
>   server/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java PRE-CREATION 
>   server/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java PRE-CREATION 
>   server/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java PRE-CREATION 
>   server/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java PRE-CREATION 
>   server/src/com/cloud/api/response/NiciraNvpDeviceResponse.java PRE-CREATION 
>   server/src/com/cloud/configuration/DefaultComponentLibrary.java 3bfe84df25dd4027fdaad48b5ea2c24fff2ce432 
>   server/src/com/cloud/host/dao/HostDaoImpl.java 745567dd7d260de5fab0945d72f89a8133cca4c4 
>   server/src/com/cloud/hypervisor/HypervisorGuruBase.java d62e3800542e0394203506b25f5eb44c5fc90308 
>   server/src/com/cloud/network/ExternalNetworkDeviceManager.java e09ff8e9580612ae010f550fc6f0a24b9cfd971d 
>   server/src/com/cloud/network/NetworkManagerImpl.java 1f306ab07d136a785b9b433122fcd21418da12eb 
>   server/src/com/cloud/network/NiciraNvpDeviceVO.java PRE-CREATION 
>   server/src/com/cloud/network/NiciraNvpNicMappingVO.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpDao.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpDaoImpl.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpNicMappingDao.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpNicMappingDaoImpl.java PRE-CREATION 
>   server/src/com/cloud/network/element/NiciraNvpElement.java PRE-CREATION 
>   server/src/com/cloud/network/element/NiciraNvpElementService.java PRE-CREATION 
>   server/src/com/cloud/network/guru/ExternalGuestNetworkGuru.java 6c381f2b18eb559e34b3d4d51329df0b7d8f5afb 
>   server/src/com/cloud/network/guru/GuestNetworkGuru.java f9977102b1f4b7652a87f3b6c7e04e44aae6a2d8 
>   server/src/com/cloud/network/guru/NiciraNvpGuestNetworkGuru.java PRE-CREATION 
>   server/src/com/cloud/network/guru/OvsGuestNetworkGuru.java d031feeb6ebb40155234d6b856746c8a5eab80de 
>   setup/db/create-schema.sql afcee3f9f6f48d1e0da42f9f952018aec0904929 
>   ui/scripts/ui-custom/zoneWizard.js d46bad9800bac5b671fbe40c8456ab2da19e5531 
> 
> Diff: https://reviews.apache.org/r/5590/diff/
> 
> 
> Testing
> -------
> 
> Simple build check
>   clean-all build-all
> 
> Testing of all api calls
>  * addNiciraNvpDevice
>  * deleteNiciraNvpDevice		
>  * listNiciraNvpDevices
>  * listNiciraNvpDeviceNetwork
> 
> Functional testing using the following procedure
> * start from clean db and create zone (with guest traffic on a physical network with stt isolation type)
> * add NiciraNvp network service provider and enable
> * add NiciraNcpDevice to physical network and configure using api
> * create guestnetwork
> * create instance linked to guest network
> * check existence of logical switch and logical ports for routervm and instance
> * check connectivity between routervm and instance
> * destroy host
> * after shutdown of routervm and network check logical switch and ports on nicira (should be gone)
> 
> 
> Thanks,
> 
> Hugo Trippaers
> 
>


RE: Review Request: Nicira NVP integration for CloudStack

Posted by Hugo Trippaers <HT...@schubergphilis.com>.
Hey Murali,

The first bit of the documentation is here http://wiki.cloudstack.org/display/gen/Nicira+Integration. I will beef it up shortly.

I have some trouble with the Jira site, but I will see if I can make a patch for this.

Hugo

-----Original Message-----
From: Murali Reddy [mailto:Murali.Reddy@citrix.com] 
Sent: Tuesday, July 10, 2012 11:09 PM
To: cloudstack-dev@incubator.apache.org
Subject: Re: Review Request: Nicira NVP integration for CloudStack

Hugo,

I reviewed the patch and its good to go into master. I will get the patch integrated into master branch. Can you please do below tasks to conclude phase 1 effort.

- Since there in no flexibility to tag VIF's as Nicira integration would want, we can leave the changes you done in the CitrixResourceBase for now.
But we may need to get rid of custom handling and make it more generic in the long run. One of the idea Alex & Chiradeep suggested as a solution is to have NicTO handler function which takes in NicTo and returns VIF object. So this handler needs to be implemented as CloudStack adapter, and be implemented in the Nicira integration plug-in. CitrixResoucebase needs to be changed to load the handler adapters if there is any configured adapter. Can you please open a bug to track this change? Even better if you can submit a patch.

- Start documenting the effort on confluence. If can put up a small functional spec/write up on confluence explaining the feature that would be great.

Thanks,
Murali

On 09/07/12 11:48 PM, "Hugo Trippaers" <HT...@schubergphilis.com>
wrote:

>Hello Murali,
>
>Did you have a chance to look at the updated patch? I made some changes 
>based on your feedback and I'm curious if you agree with the solutions.
>
>I'm also wondering on how to proceed to get this patch integrated in 
>master branch? What needs to be fixed to make this happen? For the time 
>being I'm fulltime working on CloudStack development and particularly 
>this patch so I have ample time to work on this. If you have any 
>concerns please let me know so I can start working on it. Nicira is 
>willing to help out as well, so if you need anything be sure to let us know.
>
>We (Schuberg Philis) are running nicira-phase1-p4 on our internal cloud 
>in silent mode (i.e. no Nicira options activated) to evaluate 
>stability, I'm hoping to activate the Nicira devices soon to gather real world data.
>
>Cheers,
>
>Hugo


Re: Review Request: Nicira NVP integration for CloudStack

Posted by Murali Reddy <Mu...@citrix.com>.
Hugo,

I reviewed the patch and its good to go into master. I will get the patch
integrated into master branch. Can you please do below tasks to conclude
phase 1 effort.

- Since there in no flexibility to tag VIF's as Nicira integration would
want, we can leave the changes you done in the CitrixResourceBase for now.
But we may need to get rid of custom handling and make it more generic in
the long run. One of the idea Alex & Chiradeep suggested as a solution is
to have NicTO handler function which takes in NicTo and returns VIF
object. So this handler needs to be implemented as CloudStack adapter, and
be implemented in the Nicira integration plug-in. CitrixResoucebase needs
to be changed to load the handler adapters if there is any configured
adapter. Can you please open a bug to track this change? Even better if
you can submit a patch.

- Start documenting the effort on confluence. If can put up a small
functional spec/write up on confluence explaining the feature that would
be great.

Thanks,
Murali

On 09/07/12 11:48 PM, "Hugo Trippaers" <HT...@schubergphilis.com>
wrote:

>Hello Murali,
>
>Did you have a chance to look at the updated patch? I made some changes
>based on your feedback and I'm curious if you agree with the solutions.
>
>I'm also wondering on how to proceed to get this patch integrated in
>master branch? What needs to be fixed to make this happen? For the time
>being I'm fulltime working on CloudStack development and particularly
>this patch so I have ample time to work on this. If you have any concerns
>please let me know so I can start working on it. Nicira is willing to
>help out as well, so if you need anything be sure to let us know.
>
>We (Schuberg Philis) are running nicira-phase1-p4 on our internal cloud
>in silent mode (i.e. no Nicira options activated) to evaluate stability,
>I'm hoping to activate the Nicira devices soon to gather real world data.
>
>Cheers,
>
>Hugo


RE: Review Request: Nicira NVP integration for CloudStack

Posted by Hugo Trippaers <HT...@schubergphilis.com>.
Hello Murali,

Did you have a chance to look at the updated patch? I made some changes based on your feedback and I'm curious if you agree with the solutions. 

I'm also wondering on how to proceed to get this patch integrated in master branch? What needs to be fixed to make this happen? For the time being I'm fulltime working on CloudStack development and particularly this patch so I have ample time to work on this. If you have any concerns please let me know so I can start working on it. Nicira is willing to help out as well, so if you need anything be sure to let us know. 

We (Schuberg Philis) are running nicira-phase1-p4 on our internal cloud in silent mode (i.e. no Nicira options activated) to evaluate stability, I'm hoping to activate the Nicira devices soon to gather real world data. 

Cheers,

Hugo

 

-----Original Message-----
From: Murali Reddy [mailto:noreply@reviews.apache.org] On Behalf Of Murali Reddy
Sent: Friday, July 06, 2012 1:37 AM
To: cloudstack; Murali Reddy; Hugo Trippaers
Subject: Re: Review Request: Nicira NVP integration for CloudStack


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5590/#review8903
-----------------------------------------------------------


Hugo, code in general looks good. Below are some minor comments. Is there is trial version of NVP controller that can used for trying out integration or for testing?

- In CitrixResourceBase it seems to me that Nicira config params are added for every VIF that is created. Should it be done only for the network's using STT isolation?

- In GuestNetworkGuru:

   + In isMyIsolationMethod(), if no isolation method is configured, its treated as acceptable isolation by GuestNetworkGuru. I suggest a guru check only for what isolation it can support.

   + Whats is the rational for canHanlde() being implemented in each of the implementation of GuestNetworkGuru? I think single abstract implementations should be fine. Do you see that logic will change across the implementations?

- CloudStack Account names are not unique. So please use a combination of domain id + account name or some other unique tag when creating a logical switch.

- I see that it is permitted to configure multiple NVP controllers per physical network?  But always use the first controller available for the physical network when implementing the network. Is it intended behavior?

- Not sure you mentioned this in the your earlier mails. Does this implementation work only with Xen? or works with Kvm, Vmware as well?

- Murali Reddy


On June 26, 2012, 4:56 p.m., Hugo Trippaers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5590/
> -----------------------------------------------------------
> 
> (Updated June 26, 2012, 4:56 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Description
> -------
> 
> Patch to add Nicira NVP support to CloudStack. As discussed this patch is related to phase 1, which is basic L2 connectivity. L3 connectivity and integration with the network offering for SNAT will be in phase2.
> 
> 
> Diffs
> -----
> 
>   README.NiciraIntegration PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchPortAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/CreateLogicalSwitchPortCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchPortAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/DeleteLogicalSwitchPortCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/StartupNiciraNvpCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/to/NicTO.java b65c61ee47c03bf33bcbf2ee10a2f8d6405538f0 
>   api/src/com/cloud/agent/api/to/VirtualMachineTO.java 42d91626e35d20c960561ca1101aad17dc19febb 
>   api/src/com/cloud/api/ApiConstants.java 00ec392d7b930a1ec0d2d77c2cccd035bc9f3321 
>   api/src/com/cloud/host/Host.java 0c9d06d48bfdaf1f491c71cda6e1d878214a2dd1 
>   api/src/com/cloud/network/Network.java 0443a0f41cc112105b48bb80dae58f6f3adc4b8d 
>   api/src/com/cloud/network/Networks.java 84135b8ee45489fb6c284c6cf3ae0356596cbc83 
>   api/src/com/cloud/network/PhysicalNetwork.java e54fe00bef0846ec96a3b3dde43ba433bab6f1ac 
>   client/tomcatconf/components.xml.in 58541a59606aebb69c1e9566934736b1a6f86b21 
>   client/tomcatconf/nicira-nvp_commands.properties.in PRE-CREATION 
>   core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 39172423e36f06f3721a9b108f0f05f5008f5613 
>   core/src/com/cloud/network/nicira/Attachment.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/LogicalSwitch.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/LogicalSwitchPort.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/LogicalSwitchPortList.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/NiciraNvpApi.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/NiciraNvpApiException.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/NiciraNvpTag.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/TransportZoneBinding.java PRE-CREATION 
>   core/src/com/cloud/network/nicira/VifAttachment.java PRE-CREATION 
>   core/src/com/cloud/network/resource/NiciraNvpResource.java PRE-CREATION 
>   server/src/com/cloud/api/commands/AddNiciraNvpDeviceCmd.java PRE-CREATION 
>   server/src/com/cloud/api/commands/DeleteNiciraNvpDeviceCmd.java PRE-CREATION 
>   server/src/com/cloud/api/commands/ListNiciraNvpDeviceNetworksCmd.java PRE-CREATION 
>   server/src/com/cloud/api/commands/ListNiciraNvpDevicesCmd.java PRE-CREATION 
>   server/src/com/cloud/api/response/NiciraNvpDeviceResponse.java PRE-CREATION 
>   server/src/com/cloud/configuration/DefaultComponentLibrary.java 3bfe84df25dd4027fdaad48b5ea2c24fff2ce432 
>   server/src/com/cloud/host/dao/HostDaoImpl.java 745567dd7d260de5fab0945d72f89a8133cca4c4 
>   server/src/com/cloud/hypervisor/HypervisorGuruBase.java d62e3800542e0394203506b25f5eb44c5fc90308 
>   server/src/com/cloud/network/ExternalNetworkDeviceManager.java e09ff8e9580612ae010f550fc6f0a24b9cfd971d 
>   server/src/com/cloud/network/NetworkManagerImpl.java 1f306ab07d136a785b9b433122fcd21418da12eb 
>   server/src/com/cloud/network/NiciraNvpDeviceVO.java PRE-CREATION 
>   server/src/com/cloud/network/NiciraNvpNicMappingVO.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpDao.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpDaoImpl.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpNicMappingDao.java PRE-CREATION 
>   server/src/com/cloud/network/dao/NiciraNvpNicMappingDaoImpl.java PRE-CREATION 
>   server/src/com/cloud/network/element/NiciraNvpElement.java PRE-CREATION 
>   server/src/com/cloud/network/element/NiciraNvpElementService.java PRE-CREATION 
>   server/src/com/cloud/network/guru/ExternalGuestNetworkGuru.java 6c381f2b18eb559e34b3d4d51329df0b7d8f5afb 
>   server/src/com/cloud/network/guru/GuestNetworkGuru.java f9977102b1f4b7652a87f3b6c7e04e44aae6a2d8 
>   server/src/com/cloud/network/guru/NiciraNvpGuestNetworkGuru.java PRE-CREATION 
>   server/src/com/cloud/network/guru/OvsGuestNetworkGuru.java d031feeb6ebb40155234d6b856746c8a5eab80de 
>   setup/db/create-schema.sql afcee3f9f6f48d1e0da42f9f952018aec0904929 
>   ui/scripts/ui-custom/zoneWizard.js d46bad9800bac5b671fbe40c8456ab2da19e5531 
> 
> Diff: https://reviews.apache.org/r/5590/diff/
> 
> 
> Testing
> -------
> 
> Simple build check
>   clean-all build-all
> 
> Testing of all api calls
>  * addNiciraNvpDevice
>  * deleteNiciraNvpDevice		
>  * listNiciraNvpDevices
>  * listNiciraNvpDeviceNetwork
> 
> Functional testing using the following procedure
> * start from clean db and create zone (with guest traffic on a physical network with stt isolation type)
> * add NiciraNvp network service provider and enable
> * add NiciraNcpDevice to physical network and configure using api
> * create guestnetwork
> * create instance linked to guest network
> * check existence of logical switch and logical ports for routervm and instance
> * check connectivity between routervm and instance
> * destroy host
> * after shutdown of routervm and network check logical switch and ports on nicira (should be gone)
> 
> 
> Thanks,
> 
> Hugo Trippaers
> 
>