cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Suresh Ramamurthy <suresh.ramamur...@nuagenetworks.net>
Subject Re: Review Request 23282: CLOUDSTACK-6845 : First Code drop for NuageVsp Network plugin
Date Tue, 08 Jul 2014 19:02:03 GMT
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 <
htrippaers@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/>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message