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