cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ritu Sabharwal" <rsabh...@brocade.com>
Subject Re: Review Request 23314: Plugin specific code for the Brocade Network Plugin
Date Mon, 14 Jul 2014 18:51:06 GMT


> On July 8, 2014, 9:29 a.m., Hugo Trippaers wrote:
> > Thanks for make this a separate review, much easier to read.
> > 
> > The licenses are not added to all files and in some files they appear not to be
complete. A good trick to solve this is to include the license-maven-plugin in the build section.
See plugins/network-elements/nicira-nvp/pom.xml for an example.
> > 
> > Also the plugin doesn't contain any tests. We need unit tests or functional tests
(and preferably both) to validate that this plugin works and keeps on working in future versions
of CloudStack. It would be great if you could supply unit tests to validate this inner workings
of this module and marvin based integration tests that we can run to see if this work with
the intended hardware.
> > 
> > The last general point is that we would also need documentation on this plugin so
administrators will know how to use this.
> 
> Ritu  Sabharwal wrote:
>     Thanks for reviewing the code.
>     
>     I have fixed the license in all the files.
>     
>     I am working on writing the unit tests for the plugin.
>     
>     I am working on providing the documentation on using the plugin.
>     
>     I have tested the plugin with the simulator and see the configurations on Brocade
switches happening. Currently, I am setting up a real hardware setup with Vmware hypervisor
and Brocade switches so test the connectivity between spawned VMs.
>     
>     Can I continue writing the unit tests and marvin integration tests and documentation
after the July 19 date so that this plugin makes to 4.5 feature freeze date.
>     
>     I have updated the diffs with the changes for the license and other comments.
> 
> Ritu  Sabharwal wrote:
>     I am planning to include the following in the documentation:
>      - Brocade switch setup
>      - Connections to the Hypervisor hosts
>      - the flow to use the plugin from UI
> 
> Hugo Trippaers wrote:
>     Thanks for fixing the open issues.
>     
>     I think it's ok to work on the documentation while a release is being tested (eventhough
it is far better to have it ready for the testers). However unittests and functional testcases
need to be there before we can merge the plugin. Without it we can not properly test the plugin
to see if it is fit to be part of the release. 
>     
>     I'd rather have a completely done plugin in the 4.6 release than something that is
not complete in 4.5. So currently the blockers for a merge into master are the two property
files and we need unit and integration tests.
> 
> Ritu  Sabharwal wrote:
>     Hi Hugo,
>     
>     Thanks for reviewing the plugin.
>     
>     Few points:
>     
>     1. The configuration using properties file was part of the Design document https://cwiki.apache.org/confluence/display/CLOUDSTACK/Brocade+Network+Plugin+to+Orchestrate+Brocade+VDX+Switches
and was not raised as a concern earlier and the implementation is based on that.  
>     
>     Changing the implementation right now would involve some work. Can this be done as
an enhancement as part of bug fix during testing?
>     
>     2. For functional testcases, we are doing functional testing but how will functional
testcases be run by the community without the Brocade hardware? 
>     
>     We are working on writing the unit and functional testcases.
>
> 
> Ritu  Sabharwal wrote:
>     Hi Hugo,
>     
>     I have added the unit tests, working on the the marvin integration tests.
>     
>     Please reply back for the properties file change if that can be treated an an enhancement
bug fix during testing phase.
>     
>     Thanks for reviewing and supporting the plugin.
>     
>     Thanks & Regards,
>     Ritu S.

Hi Hugo,

Just an update, I am working on removing the reading of configurations from the properties
file and adding them as part of network provider configuration through APIs and UI. Also,
working ont he marvin integration tests.

Should be able to update code by tomorrow for review.

Thanks & Regards,
Ritu S.


- Ritu


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


On July 11, 2014, 11:44 p.m., Ritu  Sabharwal wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23314/
> -----------------------------------------------------------
> 
> (Updated July 11, 2014, 11:44 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Bugs: CLOUDSTACK-6823
>     https://issues.apache.org/jira/browse/CLOUDSTACK-6823
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Plugin specific code.
> 
> 
> Diffs
> -----
> 
>   plugins/network-elements/brocade-vcs/pom.xml PRE-CREATION 
>   plugins/network-elements/brocade-vcs/resources/BrocadeInterfaceSchema.xsd PRE-CREATION

>   plugins/network-elements/brocade-vcs/resources/BrocadePortProfileSchema.xsd PRE-CREATION

>   plugins/network-elements/brocade-vcs/resources/BrocadeShowVcsSchema.xsd PRE-CREATION

>   plugins/network-elements/brocade-vcs/resources/META-INF/cloudstack/vcs/module.properties
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/resources/META-INF/cloudstack/vcs/spring-vcs-context.xml
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/AssociateMacToNetworkAnswer.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/AssociateMacToNetworkCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/CreateNetworkAnswer.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/CreateNetworkCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/DeleteNetworkAnswer.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/DeleteNetworkCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/DisassociateMacFromNetworkAnswer.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/DisassociateMacFromNetworkCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/agent/api/StartupBrocadeVcsCommand.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/BrocadeVcsNetworkHostMappingVO.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/BrocadeVcsApi.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/BrocadeVcsApiException.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/Cache.java PRE-CREATION

>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/CacheManager.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/Constants.java PRE-CREATION

>   plugins/network-elements/brocade-vcs/src/com/cloud/network/brocade/Switch.java PRE-CREATION

>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkHostMappingDao.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/dao/BrocadeVcsNetworkHostMappingDaoImpl.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/element/BrocadeVcsElement.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/guru/BrocadeVcsGuestNetworkGuru.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/src/com/cloud/network/resource/BrocadeVcsResource.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/test/com/cloud/network/brocade/BrocadeVcsApiTest.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/test/com/cloud/network/guru/BrocadeVcsGuestNetworkGuruTest.java
PRE-CREATION 
>   plugins/network-elements/brocade-vcs/test/com/cloud/network/resource/BrocadeVcsResourceTest.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23314/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ritu  Sabharwal
> 
>


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