You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Sachchidanand Vaidya <va...@juniper.net> on 2014/05/03 09:30:18 UTC
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/
-----------------------------------------------------------
(Updated May 3, 2014, 12:30 a.m.)
Review request for cloudstack and Rajesh Battala.
Changes
-------
.
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.
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
Diff: https://reviews.apache.org/r/19758/diff/
Testing
-------
Performed unit tests with vSRX network service provider. Also other unit tests pass in local testbed.
Thanks,
Sachchidanand Vaidya
Re: Review Request 19758: Added a Network Service Provider for
JuniperContrailvSRX. This NSP provides sourceNAT functionality.
Posted by Sachchidanand Vaidya <va...@juniper.net>.
> On May 6, 2014, 12:33 a.m., Rajesh Battala wrote:
> > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java, line 1
> > <https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line1>
> >
> > As this is a new file content. please run rat build to verify the license
Fixed.
> On May 6, 2014, 12:33 a.m., Rajesh Battala wrote:
> > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java, line 3
> > <https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line3>
> >
> > Are there any UnitTests available to test this element?
> >
> > any marvin tests are available?
No unit tests. Will add it later.
> On May 6, 2014, 12:33 a.m., Rajesh Battala wrote:
> > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java, line 126
> > <https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line126>
> >
> > systemvm service offering can be used or seed required offering and use it.
systemvm offering can't be used. vSRX needs bigger memory foorprint.
> On May 6, 2014, 12:33 a.m., Rajesh Battala wrote:
> > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java, line 140
> > <https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line140>
> >
> > how this template will be seeded?
> > user will upload this template?
Use will upload this template.
> On May 6, 2014, 12:33 a.m., Rajesh Battala wrote:
> > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java, line 141
> > <https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line141>
> >
> > 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?
> >
Currently we have image for XenServer only. Also there will be single vSRX template for now.
I have added check for hypervisor type and status of the template.
> On May 6, 2014, 12:33 a.m., Rajesh Battala wrote:
> > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java, line 206
> > <https://reviews.apache.org/r/19758/diff/1/?file=539139#file539139line206>
> >
> > missed formatting
fixed.
- Sachchidanand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19758/#review42255
-----------------------------------------------------------
On May 8, 2014, 10:36 p.m., Sachchidanand Vaidya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19758/
> -----------------------------------------------------------
>
> (Updated May 8, 2014, 10:36 p.m.)
>
>
> Review request for cloudstack and Rajesh Battala.
>
>
> 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.
>
>
> 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
>
> Diff: https://reviews.apache.org/r/19758/diff/
>
>
> Testing
> -------
>
> Performed unit tests with vSRX network service provider. Also other unit tests pass in local testbed.
>
>
> Thanks,
>
> Sachchidanand Vaidya
>
>
Re: Review Request 19758: Added a Network Service Provider for
JuniperContrailvSRX. This NSP provides sourceNAT functionality.
Posted by Sachchidanand Vaidya <va...@juniper.net>.
Hi Rajesh,
My comments inline marked [SACHIN].
Thanks,
Sachin
From: Rajesh Battala <ra...@citrix.com>>
Reply-To: Rajesh Battala <ra...@citrix.com>>
Date: Tuesday, May 6, 2014 12:33 AM
To: Rajesh Battala <ra...@citrix.com>>
Cc: Sachchidanand Vaidya <va...@juniper.net>>, cloudstack <de...@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/>
Re: Review Request 19758: Added a Network Service Provider for
JuniperContrailvSRX. This NSP provides sourceNAT functionality.
Posted by Rajesh Battala <ra...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19758/#review42255
-----------------------------------------------------------
plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java
<https://reviews.apache.org/r/19758/#comment76046>
As this is a new file content. please run rat build to verify the license
plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java
<https://reviews.apache.org/r/19758/#comment76050>
Are there any UnitTests available to test this element?
any marvin tests are available?
plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java
<https://reviews.apache.org/r/19758/#comment76047>
systemvm service offering can be used or seed required offering and use it.
plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java
<https://reviews.apache.org/r/19758/#comment76048>
how this template will be seeded?
user will upload this template?
plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java
<https://reviews.apache.org/r/19758/#comment76049>
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?
plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailVSrxElementImpl.java
<https://reviews.apache.org/r/19758/#comment76051>
missed formatting
- Rajesh Battala
On May 3, 2014, 7:30 a.m., Sachchidanand Vaidya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19758/
> -----------------------------------------------------------
>
> (Updated May 3, 2014, 7:30 a.m.)
>
>
> Review request for cloudstack and Rajesh Battala.
>
>
> 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.
>
>
> 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
>
> Diff: https://reviews.apache.org/r/19758/diff/
>
>
> Testing
> -------
>
> Performed unit tests with vSRX network service provider. Also other unit tests pass in local testbed.
>
>
> Thanks,
>
> Sachchidanand Vaidya
>
>
Re: Review Request 19758: Added a Network Service Provider for
JuniperContrailvSRX. This NSP provides sourceNAT functionality.
Posted by Sebastien Goasguen <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19758/#review79049
-----------------------------------------------------------
Thank you for submitting your CloudStack contribution through review board. After discussion on the dev@cloudstack.apache.org the community decided to close down review board and start accepting contributiong through GitHub pull requests. We have been using GH PR for several months now and the process is better than review board.
We will keep Review Board open for another week to give you time to migrate your patch to a github PR if you wish. After that time, your patch will no longer be viewable (even though it will not be deleted).
Please consider submitting a pull request.
Great instructions are available at:
https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md
Thank you very much for your time and your contribution to Apache CloudStack, we hope that using this new process will encourage you to do more.
- Sebastien Goasguen
On May 9, 2014, 5:36 a.m., Sachchidanand Vaidya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19758/
> -----------------------------------------------------------
>
> (Updated May 9, 2014, 5:36 a.m.)
>
>
> Review request for cloudstack and Rajesh Battala.
>
>
> 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.
>
>
> 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
>
> Diff: https://reviews.apache.org/r/19758/diff/
>
>
> Testing
> -------
>
> Performed unit tests with vSRX network service provider. Also other unit tests pass in local testbed.
>
>
> Thanks,
>
> Sachchidanand Vaidya
>
>
Re: Review Request 19758: Added a Network Service Provider for
JuniperContrailvSRX. This NSP provides sourceNAT functionality.
Posted by Sachchidanand Vaidya <va...@juniper.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19758/
-----------------------------------------------------------
(Updated May 8, 2014, 10:36 p.m.)
Review request for cloudstack and Rajesh Battala.
Changes
-------
Updated code based on review comment - check added for template, formatting fixed.
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.
Diffs (updated)
-----
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
Diff: https://reviews.apache.org/r/19758/diff/
Testing
-------
Performed unit tests with vSRX network service provider. Also other unit tests pass in local testbed.
Thanks,
Sachchidanand Vaidya