> 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 > >
