----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14549/#review27762 -----------------------------------------------------------
Hey Pedro, Nice work, there is some serious effort in getting this all done. However i'm just a bit concerned by the way the plugin is implemented. You are breaking away from some of the established patterns we already have in CloudStack. The existing SDN solutions more or less work the same way. They are added as a provider, they use an isolation type to determine if the guru should take action and they use the network offering model to determine when to take action. Also the general way of interacting southbound is through the agent system (CloudStack managers/elements and gurus decide what to do, the agent implementation decided how to do it). While your plugin seems to be fine from the functionality perspective, not using the established patterns will make it exceedingly hard to maintain this plugin and to keep supporting it from within CloudStack core. Can you explain why you did not use the established patterns? You are not using the agent system for the interaction southbound, how would this plugin work with a cluster of CloudStack management servers? Currently in CloudStack the agent is a unique instance in the cluster that will receive instructions from the managers, how does this work with this plugin? The guru is checking for a specific offering that is created by the plugin. I agree with Murali here that this is not the best way of doing things. You are enforcing choices on the administrator that he might want to make himself. Also this will conflict with the new capabilities system i'm putting into place where the network orchestrator will check if a certain combination of networking types can be supported by the guru. See the network-guru-orchestration branch on git for more information on that. Cheers, Hugo plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/14549/#comment53940> Why do you need the vmSpec.getUuid here and not the vm.getUuid which is already present. Then you won't have to pass the vmspec. plugins/network-elements/juniper-contrail/pom.xml <https://reviews.apache.org/r/14549/#comment53942> I don't think we want to list organizations here. The only organization is Apache Software Foundation. plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailElementImpl.java <https://reviews.apache.org/r/14549/#comment53945> Are you actually supporting all these features in the current version of the code. I see a lot of code with just debug placeholders. To keep things clear for others reading the code you might want to limit your support for things that are actually implemented and remove placeholder functions. plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailElementImpl.java <https://reviews.apache.org/r/14549/#comment53947> Remove the TODO's if the code is done? plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ManagementNetworkGuru.java <https://reviews.apache.org/r/14549/#comment53952> author tags should not be in apache code. - Hugo Trippaers On Oct. 23, 2013, 6:26 p.m., Pedro Marques wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14549/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2013, 6:26 p.m.) > > > Review request for cloudstack. > > > Repository: cloudstack-git > > > Description > ------- > > Rename net.juniper.contrail to org.apache.cloudstack.network.contrail. > > > Diffs > ----- > > api/src/com/cloud/network/Network.java 49f380b > client/pom.xml fd1f13a > client/tomcatconf/commands.properties.in 0296de0 > client/tomcatconf/componentContext.xml.in df5b002 > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java > f16a6f5 > plugins/network-elements/juniper-contrail/pom.xml PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/api/command/CreateServiceInstanceCmd.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/api/response/ServiceInstanceResponse.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailElement.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailElementImpl.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailGuru.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailManager.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailManagerImpl.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/DBSyncGeneric.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/EventUtils.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ManagementNetworkGuru.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ModelDatabase.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ServerDBSync.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ServerDBSyncImpl.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ServerEventHandler.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ServerEventHandlerImpl.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ServiceManager.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ServiceManagerImpl.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ServiceVirtualMachine.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/FloatingIpModel.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/FloatingIpPoolModel.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/InstanceIpModel.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ModelController.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ModelObject.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ModelObjectBase.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ServiceInstanceModel.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VMInterfaceModel.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VirtualMachineModel.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VirtualNetworkModel.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/ApiConnectorMockito.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/IntegrationTestConfiguration.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/ManagementServerMock.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/NetworkProviderTest.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/ProviderTestConfiguration.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/PublicNetworkTest.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/PublicNetworkTestConfiguration.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/TestDbSetup.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/VirtualNetworkModelTest.java > PRE-CREATION > plugins/network-elements/juniper-contrail/test/resources/commonContext.xml > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/resources/contrail.properties > PRE-CREATION > plugins/network-elements/juniper-contrail/test/resources/db.properties > PRE-CREATION > plugins/network-elements/juniper-contrail/test/resources/log4j.properties > PRE-CREATION > plugins/network-elements/juniper-contrail/test/resources/mysql_db_start.sh > PRE-CREATION > plugins/network-elements/juniper-contrail/test/resources/mysql_db_stop.sh > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/resources/providerContext.xml > PRE-CREATION > > plugins/network-elements/juniper-contrail/test/resources/publicNetworkContext.xml > PRE-CREATION > plugins/pom.xml ca41dff > > Diff: https://reviews.apache.org/r/14549/diff/ > > > Testing > ------- > > Integration test passes. > > > Thanks, > > Pedro Marques > >
