Hi Hugo, Thanks for reviewing NuageVsp plugin and supporting us.
I have update the comments with my response. Thanks, Suresh On Tue, Jul 8, 2014 at 3:07 AM, Hugo Trippaers < htrippa...@schubergphilis.com> wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23282/ > > Hey Suresh, > > Great to see this review, happy to help integrate the NuageVsp code into > CloudStack. I did an initial review an put my first feedback in the line > comments. I haven't tested functionality or tried to apply the patch yet, but > will do that after you had a chance to review these comments. > > Cheers, > > Hugo > > > client/tomcatconf/log4j-cloud.xml.in > <https://reviews.apache.org/r/23282/diff/1/?file=624288#file624288line66> > (Diff > revision 1) > > 66 > > <appender name="NUAGEVSP" > class="org.apache.log4j.rolling.RollingFileAppender"> > > Why do you introduce a new logfile specific for Nuage? Isn't is easier for > admins if all cloudstack related messages appear in the already existing > logfile that admins are familiar with? > > > > plugins/network-elements/nuage-vsp/resources/META-INF/cloudstack/vsp/vsp-defaults.ini > <https://reviews.apache.org/r/23282/diff/1/?file=624296#file624296line1> (Diff > revision 1) > > 1 > > #This property file contains the default values to be set for the entities > that are > > Missing Apache License Header > > > > plugins/network-elements/nuage-vsp/resources/META-INF/cloudstack/vsp/vsp-defaults.ini > <https://reviews.apache.org/r/23282/diff/1/?file=624296#file624296line4> (Diff > revision 1) > > 4 > > noOfSyncThreads=5 > > Why introduce another configuration file? CloudStack provides options to > set configuration using either network service provides or global > configuration. > > > > plugins/network-elements/nuage-vsp/resources/META-INF/cloudstack/vsp/vsp-defaults.ini > <https://reviews.apache.org/r/23282/diff/1/?file=624296#file624296line10> > (Diff > revision 1) > > 10 > > nuagePluginClientJarLocation=/usr/share/cloudstack-management/webapps/client/WEB-INF/lib > > Please do not make any assumptions on the locations of jar files. That is > up to the people doing the packaging and might vary from distribution to > distribution. Expect everything to be available using the web app class > loader. > > > > plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResource.java > <https://reviews.apache.org/r/23282/diff/1/?file=624333#file624333line95> > (Diff > revision 1) > > 95 > > private static final String NUAGE_PLUGIN_CLIENT_JAR_LOCATION = > "/usr/share/cloudstack-management/webapps/client/WEB-INF/lib"; > > Don't depend on file locations as they will change. Use the java class > loader to load any libraries. > > > > plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResource.java > <https://reviews.apache.org/r/23282/diff/1/?file=624333#file624333line196> > (Diff > revision 1) > > 196 > > private <A extends NuageVspApiClient, B extends NuageVspElementClient, C > extends NuageVspSyncClient, D extends NuageVspGuruClient> void > loadNuageClient() throws Exception { > > I'm not really agreeing with this method of loading the client. The client > should be loaded using the classloader. > > Is the library available from maven central or is distribution restricted? If > the latter case we need to move this plugin and the dependency on the library > to the noredist build. > > > > plugins/network-elements/nuage-vsp/test/com/cloud/network/element/NuageVspElementTest.java > <https://reviews.apache.org/r/23282/diff/1/?file=624341#file624341line103> > (Diff > revision 1) > > 103 > > // NVP provider does not provide Connectivity for this network > > copy-paste error ;-) > > > server/src/com/cloud/network/vpc/VpcManagerImpl.java > <https://reviews.apache.org/r/23282/diff/1/?file=624347#file624347line1394> > (Diff > revision 1) > > public void doInTransactionWithoutResult(TransactionStatus status) { > > 1394 > > if (vpcElements != null && vpcElements.size() < 3) { > > Depending on a size here looks like a bug waiting to happen when somebody > adds an element. Is there another way to solve this? > > > setup/db/create-schema.sql > <https://reviews.apache.org/r/23282/diff/1/?file=624348#file624348line149> > (Diff > revision 1) > > 149 > > DROP TABLE IF EXISTS `cloud`.`external_nuage_vsp_devices`; > > Modifying create-schema.sql is not allowed. Please make all necessary > database changes in the upgrade scripts for the current master branch. > > > - Hugo Trippaers > > On July 7th, 2014, 6 p.m. UTC, Suresh Ramamurthy wrote: > Review request for cloudstack and Hugo Trippaers. > By Suresh Ramamurthy. > > *Updated July 7, 2014, 6 p.m.* > *Bugs: * CLOUDSTACK-6845 > <https://issues.apache.org/jira/browse/CLOUDSTACK-6845> > *Repository: * cloudstack-git > Description > > This is first code drop for NuageVsp Network plugin support that will bring > the Nuage VSP network virtualization technology to CloudStack. > > We need a new branch to checkin the fixes once the review is done. Please > create a new branch for NuageVsp plugin. > > > Testing > > Nuage VSP plugin depends on following components of Nuage SDN solution > a) Nuage VSD > b) Nuage VSC > c) Nuage VRS, this needs installed on the Hypervisor > All the above components needs to be provisioned for the plugin to function > properly. Also, Nuage VSP plugin directly talks with Nuage VSD using Rest > API. So, all the components needs to be running to test the plugin > functionality. > > The following tests are tested > > Isolated Network Test Cases > > a) Create a network offering with default egress deny rule and select > services supported by Nuage VSP plugin. Choose NuageVsp as the service > provider for DHCP, SourceNAT, StaticNAT, Firewall and Virtual Networking > services. > Choose VirtualRouter as the service provider for UserData service. > b) Create an isolated Network with network offering created above > c) Spawn 2 VMs. Verify that VMs should each get an IP address. They should > ping each other. Verify that SSH to a box on the external network should fail > b) Create a Static NAT and associate it one of the VM. Add an Egress rule for > the network with source CIDR as 0.0.0.0/0, protocol as TCP and ssh port number > d) Verify that SSH to box that is in the external network should work > e) Verify that Password reset for the VM should work > > VPC Test Cases > > a) Create a network offering for VPC with default deny all rule and select > services supported by Nuage VSP plugin for VPC. Choose NuageVsp as the > service provider for DHCP, SourceNAT, StaticNAT and Virtual Networking > services. Choose NuageVspVpc for NerworkACL service. > b) Create an VPC and select "Default VPC offering with NuageVsp" as the VPC > offering > c) Create a tier and select the network offering created above > c) Spawn 2 VMs. Verify that VMs should each get an IP address. They should > ping each other. SSH to a box on the external network should fail > d) Create a Static NAT and associate it one of the VM > e) Add an Network ACL Egress rule for the network with source CIDR as > 0.0.0.0/0, protocol as TCP and ssh port number > f) Verify that SSH to box that is in the external network should work > > Diffs > > - api/src/com/cloud/event/EventTypes.java (5b9ea5c) > - api/src/com/cloud/network/Network.java (885bffe) > - api/src/com/cloud/network/Networks.java (1e4d229) > - api/src/com/cloud/network/PhysicalNetwork.java (8cc214e) > - build/replace.properties (265f335) > - client/WEB-INF/classes/resources/messages.properties (b192cb0) > - client/WEB-INF/classes/resources/messages_zh_CN.properties (1ec4e95) > - client/pom.xml (29fef4f) > - client/tomcatconf/commands.properties.in (b9ac27b) > - client/tomcatconf/log4j-cloud.xml.in (08021f2) > - packaging/debian/replace.properties (5a0bd58) > - > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/OvsVifDriver.java > (8e4c710) > - > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java > (5b49e5b) > - > plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java > (a9840bd) > - plugins/network-elements/nuage-vsp/pom.xml (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/resources/META-INF/cloudstack/vsp/module.properties > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/resources/META-INF/cloudstack/vsp/spring-vsp-context.xml > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/resources/META-INF/cloudstack/vsp/vsp-defaults.ini > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/StartupVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/VspResourceAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/VspResourceCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/element/ApplyAclRuleVspAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/element/ApplyAclRuleVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/element/ApplyStaticNatVspAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/element/ApplyStaticNatVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/element/ShutDownVpcVspAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/element/ShutDownVpcVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/DeallocateVmVspAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/DeallocateVmVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/ImplementNetworkVspAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/ImplementNetworkVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/ReleaseVmVspAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/ReleaseVmVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/ReserveVmInterfaceVspAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/ReserveVmInterfaceVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/TrashNetworkVspAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/TrashNetworkVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/sync/SyncVspAnswer.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/sync/SyncVspCommand.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/api/commands/AddNuageVspDeviceCmd.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/api/commands/DeleteNuageVspDeviceCmd.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/api/commands/IssueNuageVspResourceRequestCmd.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/api/commands/ListNuageVspDevicesCmd.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/api/commands/VspConstants.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/api/response/NuageVspDeviceResponse.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/api/response/NuageVspResourceResponse.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/NuageVspDeviceVO.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/dao/NuageVspDao.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/dao/NuageVspDaoImpl.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspVpcElement.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManager.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManagerImpl.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResource.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/sync/NuageVspSync.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/com/cloud/network/sync/NuageVspSyncImpl.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/net/nuage/vsp/acs/NuageVspPluginClientLoader.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/net/nuage/vsp/acs/client/NuageVspApiClient.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/net/nuage/vsp/acs/client/NuageVspElementClient.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/net/nuage/vsp/acs/client/NuageVspGuruClient.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/src/net/nuage/vsp/acs/client/NuageVspSyncClient.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/test/com/cloud/network/element/NuageVspElementTest.java > (PRE-CREATION) > - > plugins/network-elements/nuage-vsp/test/com/cloud/network/guru/NuageVspGuestNetworkGuruTest.java > (PRE-CREATION) > - plugins/pom.xml (b5e6a61) > - server/src/com/cloud/api/ApiResponseHelper.java (51122e0) > - server/src/com/cloud/configuration/ConfigurationManagerImpl.java > (897f8e1) > - server/src/com/cloud/network/NetworkServiceImpl.java (b3de9e3) > - server/src/com/cloud/network/vpc/VpcManagerImpl.java (c7237c1) > - setup/db/create-schema.sql (fe5cd0a) > - setup/db/db/schema-440to450.sql (c88a18a) > - tools/apidoc/gen_toc.py (827d6bf) > - ui/dictionary.jsp (e9d84de) > - ui/scripts/configuration.js (9311e37) > - ui/scripts/docs.js (74a08bc) > - ui/scripts/system.js (9012580) > - ui/scripts/ui-custom/zoneWizard.js (4091c03) > - vmware-base/src/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java > (dd55439) > > View Diff <https://reviews.apache.org/r/23282/diff/> >
