> 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. > > > Alena Prokharchyk wrote: > 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. > > >
Contrail VPC Implementation has its own VPC Network Offering name. Please take a look at ContrailManagerImpl: locateNetworkOffering(for networks - vpcRouterOfferingName), locateVpcOffering(for vpc - juniperVPCOfferingName). When VPC is created using this vpc offering, networks created under this vpc should have "vpcRouterOfferingName". This VPC network offering should support "Service.Lb", but I wanted to re-use the existing Internal Lb implementation as is. >>If your ContrailVpcElementImpl and ContrailInternalLbElementImpl refer to the >>same physical provider (contrail VR), they should be combined into the same >>provider/networkElement. That is correct, but I can not combine them since I wanted to re-use existing Internal Lb implementation. - Suresh ----------------------------------------------------------- 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 > >
