Code review comment for lp:~cisco-openstack/neutron/l2network-plugin

Revision history for this message
Salvatore Orlando (salvatore-orlando) wrote :

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 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.

-As you pointed out in comments, the plugin currently does not work on multi-host. This should be explictly noted in the README file.

- 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.

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?

- How does this branch implement the blue print for the Quantum plugin interface? It contains only code specific for the Cisco plugin.

- 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?

- id generation - Looks like you're using absolute counters. Have you considered using per-tenant counters?

- Plugin returns some extra parameters which will be ignored by API. Is there a plan to make these parameters available through 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.

review: Needs Information

« Back to merge proposal