> On March 18, 2014, 6:19 p.m., Alena Prokharchyk wrote: > > 1) Why you've decided to introduce a new provider for the Internal LB? What > > different does it make from the class its extending? I can't find anything > > different, neither in capabilities terms, nor in other behavior. Why your > > feature can't just use existing InternalLbProvider? Please update > > > > 2) If you introduce a new provider element, how are you planning to > > add/configure/stop-start it? I dont see you've introduced any new APIs for > > that matter. Adding a provider is much more than just adding the class for > > the element. Its also adding the API logic for handling all the operations. > > Look at api package, commands/internallb folder. Each element is gotta have > > its own set of APIs. > > > > > > > > Suresh Balineni wrote: > Hi Alena, > > >>1) Why you've decided to introduce a new provider for the Internal LB? > What different does it make from the class its >.extending? I can't find > anything different, neither in capabilities terms, nor in other behavior. Why > your feature >>can't just use existing InternalLbProvider? Please update > > Contrail plugin has its own Network offering for VPC Networks. Please > take a look at ContrailManagerImpl class. Existing Internal LB Provider uses > Default Network Offering, default network offering does not work with > Contrail VPC. My intention was to use the Internal LB functionality as it is > except this LB VM nics should be created by contrail vrouter. > > >>2) If you introduce a new provider element, how are you planning to > add/configure/stop-start it? I dont see you've >>introduced any new APIs for > that matter. Adding a provider is much more than just adding the class for > the element. Its >>also adding the API logic for handling all the operations. > Look at api package, commands/internallb folder. Each >>element is gotta have > its own set of APIs. > > As per CS implementation Provider<->Network Element must have one-one > mapping. Since I introduced new Provider, there must be a new Network Element. > Since I extended InternalLBElement, all the functionality it provides is > re-used(like internal lb vm start/start/configure) in the derived > class(except the Provider). When this VM needs Nic resources, contrail > network element will get invoked. > > Each element need not have its own set of APIs say for example, if we add > a new Network Element for Network creation/deletion, we don't need to add a > new set of APIs. This new network element can get invoked only if we set the > associated Network offering used by new network element while creating a > network. Another example is, we added a new ContrailVPCElement without any > new set of APIs. This VPC Element uses existing CS VPC APIs. It is just that, > based on the network offering chosen the new Contrail VPC Element can get > invoked. >
1) "Contrail plugin has its own Network offering for VPC Networks. Please take a look at ContrailManagerImpl class. Existing Internal LB Provider uses Default Network Offering, default network offering does not work with Contrail VPC. My intention was to use the Internal LB functionality as it is except this LB VM nics should be created by contrail vrouter." You can never base a decision whether to create a new provider, just base on the offering presence. You should create a new provider only if its functionality is different. The reason why internal lb provider was implemented, is - it has diff set of services/capabilities supported, and the logic for it deployment is different for other routers. 2) "is except this LB VM nics should be created by contrail vrouter. ". I don't see the code in ContrailManagerImpl creating nics fot the Contrail Internal lb. Can you please point out to that. Or are you saying, that the ContrailVpcElementImpl and ContrailInternalLbElementImpl.java, refer to the same VR element in Contrail network? If your ContrailVpcElementImpl and ContrailInternalLbElementImpl refer to the same physical provider (contrail VR), they should be combined into the same provider/networkElement. If its a separate VR managed just the way INternal LB vm is managed, I would like to see what is diff in the process of nic creating for your internal lb provider as opposed to VPC internal lb provider. - Alena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18552/#review37581 ----------------------------------------------------------- On March 12, 2014, 6:19 p.m., Suresh Balineni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18552/ > ----------------------------------------------------------- > > (Updated March 12, 2014, 6:19 p.m.) > > > Review request for cloudstack and Alena Prokharchyk. > > > Repository: cloudstack-git > > > Description > ------- > > Internal LB support for Juniper contrail VPC implementation. > > - This implementation just extends the existing implementation of internal lb. > - New element uses juniper contrail network offering so that nics are > impelemented by contrail element. > - LB VM deployment functionality is reused (new element is extended from > existing Internal LB element implementation). > > > Diffs > ----- > > api/src/com/cloud/network/Network.java 6dc6752 > api/src/org/apache/cloudstack/api/BaseCmd.java 0e83cee > plugins/network-elements/juniper-contrail/pom.xml 8c6877d > > 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/ContrailInternalLbElementImpl.java > PRE-CREATION > > plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailManagerImpl.java > 01be7db > server/src/com/cloud/network/vpc/VpcManagerImpl.java 2157eac > > Diff: https://reviews.apache.org/r/18552/diff/ > > > Testing > ------- > > Tested LB VM deployment. Traffic tests are done. > > > Thanks, > > Suresh Balineni > >
