Hi Rajesh,
My comments inline marked [SACHIN].
Thanks,
Sachin
From: Rajesh Battala
<rajesh.batt...@citrix.com<mailto:rajesh.batt...@citrix.com>>
Reply-To: Rajesh Battala
<rajesh.batt...@citrix.com<mailto:rajesh.batt...@citrix.com>>
Date: Tuesday, May 6, 2014 12:33 AM
To: Rajesh Battala <rajesh.batt...@citrix.com<mailto:rajesh.batt...@citrix.com>>
Cc: Sachchidanand Vaidya <vaidy...@juniper.net<mailto:vaidy...@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/>
