cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sachchidanand Vaidya <vaidy...@juniper.net>
Subject Re: Review Request 19758: Added a Network Service Provider for JuniperContrailvSRX. This NSP provides sourceNAT functionality.
Date Thu, 08 May 2014 03:11:53 GMT
Hi Rajesh,
    My comments inline marked [SACHIN].

Thanks,
Sachin

From: Rajesh Battala <rajesh.battala@citrix.com<mailto:rajesh.battala@citrix.com>>
Reply-To: Rajesh Battala <rajesh.battala@citrix.com<mailto:rajesh.battala@citrix.com>>
Date: Tuesday, May 6, 2014 12:33 AM
To: Rajesh Battala <rajesh.battala@citrix.com<mailto:rajesh.battala@citrix.com>>
Cc: Sachchidanand Vaidya <vaidyasd@juniper.net<mailto:vaidyasd@juniper.net>>,
cloudstack <dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>
Subject: Re: Review Request 19758: Added a Network Service Provider for JuniperContrailvSRX.
This NSP provides sourceNAT functionality.

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

plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java<https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line1>(Diff
revision 1)

        1

// Licensed to the Apache Software Foundation (ASF) under one


As this is a new file content. please run rat build to verify the license

[SACHIN] No issues with rat build found.


plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java<https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line3>(Diff
revision 1)

        3

// distributed with this work for additional information


Are there any UnitTests available to test this element?

any marvin tests are available?

[SACHIN] There are none currently. Will look at adding few.

plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java<https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line126>(Diff
revision 1)

        126

        ServiceOfferingVO vsrx_compute_offering = null;


systemvm service offering can be used or seed required offering and use it.

[SACHIN] Systemvm offering can't be used as vSRX needs larger memory footprint.
plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java<https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line140>(Diff
revision 1)

        140

        VMTemplateVO tmplt = _tmpltDao.findByTemplateName("Juniper vSRX");


how this template will be seeded?
user will upload this template?

[SACHIN] User needs to upload vSRX images as they are not freely distributed.
plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java<https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line141>(Diff
revision 1)

        141

        VirtualMachineTemplate template = _entityMgr.findById(VirtualMachineTemplate.class,
tmplt.getId());


what if there are multiple "Juniper vSRX" templates are available?

findBytemplateName method will not search on "uniquename" it will search on template name
and template name can be same for other templates also.
this should be documented that uploaded template name should be with "Juniper vSRx" otherwise
even user uploads it, code wont be able to find it.

which hypervisor template is supported?


[SACHIN] Currently only Xen Hypervisor template is supported. I will add a check for it. Also
I agree with your comment on unique name vs name.
I will change the code to check for unique name and send you code for review.

plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java<https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line206>(Diff
revision 1)

        206

        @Override


missed formatting

[SACHIN] Will take care.

- Rajesh Battala


On May 3rd, 2014, 7:30 a.m. UTC, Sachchidanand Vaidya wrote:

Review request for cloudstack and Rajesh Battala.
By Sachchidanand Vaidya.

Updated May 3, 2014, 7:30 a.m.

Repository: cloudstack-git
Description

 - Added new Network Service Provider "JuniperContrailvSRX".
 - Impelemted an element (ContrailVSrxElement) to support this Network Service Provider.
   vSRX element currently supports only sourceNAT. It will be enhanced later to support
   more features.
 - Changes to service-instance creation code to use left virtual-network's SourceNAT addr
   as an interface address on vSRX instance's Public-network interface.



Testing

Performed unit tests with vSRX network service provider. Also other unit tests pass in local
testbed.


Diffs

  *   api/src/com/cloud/network/Network.java (ef3bcdf)
  *   plugins/network-elements/juniper-contrail/resources/META-INF/cloudstack/contrail/spring-contrail-context.xml
(99ab02e)
  *   plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java
(PRE-CREATION)
  *   plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ServiceManagerImpl.java
(f34eacc)
  *   plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VMInterfaceModel.java
(49060f1)

View Diff<https://reviews.apache.org/r/19758/diff/>


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