> On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > api/src/com/cloud/api/ApiConstants.java, line 386 > > <https://reviews.apache.org/r/9131/diff/1/?file=252824#file252824line386> > > > > There is no reason to add your constant here. You can add it in your > > own plugin package
Moved to plugin. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > api/src/com/cloud/event/EventTypes.java, line 298 > > <https://reviews.apache.org/r/9131/diff/1/?file=252825#file252825line298> > > > > You can add your event types inside your own plugin Moved to plugin. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > api/src/com/cloud/network/Network.java, line 126 > > <https://reviews.apache.org/r/9131/diff/1/?file=252826#file252826line126> > > > > You can add this in your own class in your plugin Moved to plugin. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > api/src/com/cloud/network/PhysicalNetwork.java, line 36 > > <https://reviews.apache.org/r/9131/diff/1/?file=252827#file252827line36> > > > > What is this VNS method? Is the VNS an isolation technology? What is > > the RFC for this? VNS is the isolation method implemented by the BigSwitch controller. In this release, it is a vlan-based isolation. In the future, I plan to add switch/port based isolation. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > build/build-cloud-plugins.xml, line 316 > > <https://reviews.apache.org/r/9131/diff/1/?file=252828#file252828line316> > > > > ant is not required Removed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > build/developer.xml, line 408 > > <https://reviews.apache.org/r/9131/diff/1/?file=252829#file252829line408> > > > > ant is deprecated/unsupported so this is unnecessary Removed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/CreateNetworkAnswer.java, > > line 1 > > <https://reviews.apache.org/r/9131/diff/1/?file=252837#file252837line1> > > > > this can be in project cloud-api/com.cloud.agent.api as it is likely to > > be useful to others. if not, then it should be CreateBVSNetworkAnswer Renamed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/CreateNetworkCommand.java, > > line 1 > > <https://reviews.apache.org/r/9131/diff/1/?file=252838#file252838line1> > > > > Rename to CreateBvsNetworkCommand? Renamed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/CreatePortAnswer.java, > > line 1 > > <https://reviews.apache.org/r/9131/diff/1/?file=252839#file252839line1> > > > > Rename? Renamed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/CreatePortCommand.java, > > line 1 > > <https://reviews.apache.org/r/9131/diff/1/?file=252840#file252840line1> > > > > Rename? Renamed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/DeletePortAnswer.java, > > line 1 > > <https://reviews.apache.org/r/9131/diff/1/?file=252843#file252843line1> > > > > Rename? Renamed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/DeletePortCommand.java, > > line 1 > > <https://reviews.apache.org/r/9131/diff/1/?file=252844#file252844line1> > > > > Rename? Renamed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/AddBigSwitchVnsDeviceCmd.java, > > line 48 > > <https://reviews.apache.org/r/9131/diff/1/?file=252848#file252848line48> > > > > IdentityMapper is deprecated Removed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/DeleteBigSwitchVnsDeviceCmd.java, > > line 47 > > <https://reviews.apache.org/r/9131/diff/1/?file=252849#file252849line47> > > > > IdentityMapper is deprecated with the new api refactoring Removed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/ListBigSwitchVnsDevicesCmd.java, > > line 50 > > <https://reviews.apache.org/r/9131/diff/1/?file=252850#file252850line50> > > > > IdentityMapper Removed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/Attachment.java, > > line 22 > > <https://reviews.apache.org/r/9131/diff/1/?file=252853#file252853line22> > > > > What is an attachment? > > File has tabs Attachment contains portUUID and MAC address of the VM. It is used to classify VM into the correct virtual network. Tab has been removed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/BigSwitchVnsApi.java, > > line 1 > > <https://reviews.apache.org/r/9131/diff/1/?file=252854#file252854line1> > > > > File has tabs instead of spaces Tab has been removed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/BigSwitchVnsApi.java, > > lines 172-173 > > <https://reviews.apache.org/r/9131/diff/1/?file=252854#file252854line172> > > > > These 2 are duplicated. Should be made constants? > > Also suggest org.apache.cloudstack instead of CloudStack Fixed. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/BigSwitchVnsApi.java, > > line 299 > > <https://reviews.apache.org/r/9131/diff/1/?file=252854#file252854line299> > > > > How about a finally() { method.releaseConnection()} ? No, releaseConnection can't be called if no HTTP or IO exception is caught since the responseBody needs to be retrieved later. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/Network.java, > > line 19 > > <https://reviews.apache.org/r/9131/diff/1/?file=252857#file252857line19> > > > > Name clash with CloudStack's Network. Should be BvSNetwork? > > File has tabs Renamed and removed tabs. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/Port.java, > > line 18 > > <https://reviews.apache.org/r/9131/diff/1/?file=252858#file252858line18> > > > > Name clash? Renamed > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/element/BigSwitchVnsElement.java, > > line 114 > > <https://reviews.apache.org/r/9131/diff/1/?file=252861#file252861line114> > > > > Shouldn't need a reference to the NetworkManager. Perhaps you want > > NetworkModel? NetworkManager is used to check if it is the right provider. Didn't find NetworkModel. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/element/BigSwitchVnsElement.java, > > line 323 > > <https://reviews.apache.org/r/9131/diff/1/?file=252861#file252861line323> > > > > tabs. > > Are you sure the BVS can handle these? Removed tabs. Fixed the capability. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > server/src/com/cloud/network/ExternalNetworkDeviceManager.java, line 44 > > <https://reviews.apache.org/r/9131/diff/1/?file=252868#file252868line44> > > > > This can be inside the plugin code Moved to plugin. > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/element/BigSwitchVnsElement.java, > > line 283 > > <https://reviews.apache.org/r/9131/diff/1/?file=252861#file252861line283> > > > > Should you not be destroying the network here? The virtual network is destroyed when BigSwitchVnsGuestNetworkGuru.shutdown() is called. When is NetworkElement.shutdown() is called? > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/element/BigSwitchVnsElement.java, > > line 169 > > <https://reviews.apache.org/r/9131/diff/1/?file=252861#file252861line169> > > > > Empty? The attachment point of the VM cloud be created when either NetworkElement.prepare() or NetworkElement().implement() is called. I chose to create the network in prepare(). > On Jan. 29, 2013, 10:19 p.m., Chiradeep Vittal wrote: > > plugins/network-elements/bigswitch-vns/src/com/cloud/api/response/BigSwitchVnsDeviceResponse.java, > > line 23 > > <https://reviews.apache.org/r/9131/diff/1/?file=252851#file252851line23> > > > > Requires an annotation referring to the table Please help. I am not sure what annotation you are referring to. - Kanzhe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9131/#review15807 ----------------------------------------------------------- On Jan. 29, 2013, 5:59 a.m., Kanzhe Jiang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9131/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2013, 5:59 a.m.) > > > Review request for cloudstack. > > > Description > ------- > > This is the first patch for BigSwitch Network Plugin to CloudStack. The patch > follows the design spec that can be found at > https://cwiki.apache.org/confluence/display/CLOUDSTACK/BigSwitch+Networking+Plugin. > > > Diffs > ----- > > api/src/com/cloud/api/ApiConstants.java 78a3ded > api/src/com/cloud/event/EventTypes.java e69e4a4 > api/src/com/cloud/network/Network.java d38f740 > api/src/com/cloud/network/PhysicalNetwork.java be4b1d0 > build/build-cloud-plugins.xml 207ef71 > build/developer.xml fdda171 > client/pom.xml 1673429 > client/tomcatconf/bigswitch-vns_commands.properties.in PRE-CREATION > client/tomcatconf/components.xml.in 5957b61 > cloud.spec 9f46dd6 > debian/cloud-server.install 9cd1eeb > docs/en-US/Release_Notes.xml c8cc686 > plugins/network-elements/bigswitch-vns/pom.xml PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/CreateNetworkAnswer.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/CreateNetworkCommand.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/CreatePortAnswer.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/CreatePortCommand.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/DeleteNetworkAnswer.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/DeleteNetworkCommand.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/DeletePortAnswer.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/DeletePortCommand.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/StartupBigSwitchVnsCommand.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/UpdatePortAnswer.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/agent/api/UpdatePortCommand.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/AddBigSwitchVnsDeviceCmd.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/DeleteBigSwitchVnsDeviceCmd.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/api/commands/ListBigSwitchVnsDevicesCmd.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/api/response/BigSwitchVnsDeviceResponse.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/BigSwitchVnsDeviceVO.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/Attachment.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/BigSwitchVnsApi.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/BigSwitchVnsApiException.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/ControlClusterStatus.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/Network.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/bigswitch/Port.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/dao/BigSwitchVnsDao.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/dao/BigSwitchVnsDaoImpl.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/element/BigSwitchVnsElement.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/element/BigSwitchVnsElementService.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/guru/BigSwitchVnsGuestNetworkGuru.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/resource/BigSwitchVnsResource.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/test/com/cloud/network/bigswitch/BigSwitchApiTest.java > PRE-CREATION > > plugins/network-elements/bigswitch-vns/test/com/cloud/network/resource/BigSwitchVnsResourceTest.java > PRE-CREATION > plugins/pom.xml 2009302 > server/src/com/cloud/network/ExternalNetworkDeviceManager.java b1de86f > setup/db/create-schema.sql fff084e > tools/apidoc/gen_toc.py eeaf2a2 > tools/apidoc/pom.xml b75ee82 > wscript_configure 3b9377b > > Diff: https://reviews.apache.org/r/9131/diff/ > > > Testing > ------- > > Verify the builds: mvn clean install; > The database can be deployed; > CloudStack can be launched with the plugin modules. > > JUnit tests of all api commands to a mock controller. > > > Thanks, > > Kanzhe Jiang > >
