Thanks Salvatore for your in-depth review. I will be happy to provide the information you need. My responses are embedded inline. > Hi Sumit, > > This is a massive piece of work, well done! > The code looks great. It is well written and easily understandable. > > - README states that this plugin provides "A reference implementation of > plugin framework for L2 network". My understanding of a "reference > implementation" is that it should be a relatively basic implementation of a > standardized interface, which could be executed by a large number of users. > Probably the cisco plugin should not be considered a "reference" > implementation for Quantum, as it has pretty much strict sw & hw requirements. > It seems the presence of the "Pre-requisites" section at the top of the README misleads the reader into thinking that the L2Network-plugin framework presented here can be only run with those specific requirements. We wanted to convey that the requirements mentioned in the pre-requisites were necessary when using the UCS and/or Nexus hardware. However, in the absence of the specific hardware and software, the plugin framework presented here can still be deployed and used. For instance we have run this on an Ubuntu box by disabling the UCS and Nexus device-plugins (disabling is a matter of configuring the quantum/plugins/cisco/conf/plugins.ini file). One could envision writing a Linux bridge device-plugin based on the device-plugin interface proposed in this framework (l2device_plugin_base.py). Similarly, we are proposing the notion of a network "model" which represents the actual physical interconnection of network devices. To that end, we have proposed a base class definition of this model (l2network_model_based.py), which can be extended on a per-deployment basis. When taken together, these artifacts, namely: l2network_plugin.py, l2network_model_base.py, l2device_plugin_base.py, common/*, and conf/*, embody a framework which is not specific to any hardware or software, and can be easily reused and adapted to any other combination of devices and networks. This notion of the framework is also elaborated in the design document for the blueprint: http://wiki.openstack.org/quantum-multi-switch-plugin To summarize, it should be noted that there are two distinct broad contributions in this branch (1) a device/technology/vendor independent L2 network plugin framework proposal/implementation, (2) specific support for UCS and Nexus adhering to the framework outlined in (1). The comment on the reference implementation is in the context of (1). Coming back to your comment, would it be ok if we qualify the "Pre-requisites" section with the additional information that those are needed only in the case of UCS and/or Nexus hardware/software? (I have modified the README file to reflect this.) > - It would be great if you can improve a bit pylint score for your branch. > Your branch scores 6.53/10, it would be great if could bring that result to at > leasr 7.5/10. We are already doing some work for improving pylint in trunk > code. > Would it be acceptable if we achieve this in an upcoming merge (which we are waiting to propose as soon as this particular merge is approved)? You can decline that merge if the pylint score is below 7.5. :-) > -As you pointed out in comments, the plugin currently does not work on multi- > host. This should be explictly noted in the README file. > Thanks, we can add that information. (The support for mulitiple hosts is almost complete and should also be available in an upcoming merge.) > - Several modules have a main method, which was probably used for testing > during developed. From what I see from the code, this main methods can be > removed. > Done. > Before approving for merge, it would be great if you could answer some > questions: > > - README mentions some changes are required to nova, and that these changes > should be merged in nova in time for diablo release. Are you referring to > nova-refactoring work led by Ryu? Are these changes specific for the Cisco > plugin? > Both. We are referring to the Nova refactoring changes, and also to changes which we plan to incorporate in the form of a 802.1Qbh-specific VIF-driver, and scheduler, which will be needed when running on UCS hardware. Those blueprints can be found here: https://blueprints.launchpad.net/nova/+spec/vif-driver-802-1qbh https://blueprints.launchpad.net/nova/+spec/scheduler-for-802-1qbh > - How does this branch implement the blue print for the Quantum plugin > interface? It contains only code specific for the Cisco plugin. > As explained earlier, the entire code is not Cisco device specific, code contained in the ucs and nexus directories is Cisco-device specific. > - It seems to be possible to configure the Cisco plugin to work with the Nexus > sub-plugin only excluding the UCS one; however in that case the plugin will > not work correctly as some operations need the UCS plugin. Would it be the > case to make the UCS compulsory? > One can use them in any combination. When a particular device-specific plugin gets called depends on the implementation of the network "model". We have provided the implementation of one such model in l2network_model.py which conforms to the base class specification of l2network_model_base.py. > - id generation - Looks like you're using absolute counters. Have you > considered using per-tenant counters? > Yes, you will notice that this has changed in the upcoming release where we will be using UUIDs. > - Plugin returns some extra parameters which will be ignored by API. Is there > a plan to make these parameters available through extensions? That was the original plan. However, in an upcoming merge you will notice that we will be returning only the mandatory parameters. We will make any extra information available via extensions. > > - A single logger if being used for the whole plugin. As there are several > thousands lines of code, it may be the case to have per-module loggers. This > is not really important, but it would improved debugging. Thanks, we will consider this before the next merge prop.