neutron

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

l2network-plugin
Merge into diablo

Revision history for this message

dan wendlandt (danwent) wrote on 2011-08-15:

Very cool to see this code is ready for merge. Having a Cisco plugin is an important step for the project as a whole, so I'd like to see this code get in ASAP.

I have a good number of comments below, but for the most part they are just suggestions for how you could improve the readability of the code for developers (like me!) that are looking at it for the first time. I don't see any of them as blockers and since this code is entirely self-contained, I'm voting to approve for merge right away. Getting the pylint score up is a good goal, but as long as you take care of that with the next merge I'm cool with it.

Great work :)

Dan

'quantum/plugins/cisco/README'

* The notion of a "reference implementation" vs. the Palo/Nexus specific functionality makes sense, since you've explained it to me before. Perhaps separate out the audiences would make it more clear though. For quantum administrators, this code is useful because they can us it with UCS/Nexus based networks. For quantum developers this code is useful because they may want to base their own plugin on it.

* If you are using a special branch of nova, it may be cleaner + easier to just add a sqlalchemy migrate script to automatically create this table with the user syncs the DB.
200
+1. Create a table in the "nova" database for ports. This can be
201
+ accomplished with the following SQL statement:

quantum/plugins/cisco/common/cisco_constants.py

* Currently, the rest of the Quantum codebase uses "ACTIVE" instead of "UP". While I'm personally in favor of "UP", its probably best to be consistent, then we can switch all of them at the same time if we decide that's the right thing to do.

+PORT_UP = "UP"

* it strikes me that some of these constants are not plugin specific, so ideally we'd actually have those in quantum/common, so they can be leveraged by all plugins, clients, tests, etc. This isn't required, but would be a nice thing to do at some point.

quantum/plugins/cisco/common/cisco_credentials.py

* I'm guessing your OK with this, but it struck me as a bit odd that you had methods for modifying the store, but those modifications where not persisted. Just wanted to make sure that is what you intended.

quantum/plugins/cisco/l2device_plugin_base.py

It took me a bit of glancing to figure out how this file differed from the standard quantum/quantum_plugin_base.py. Main difference is that create_network takes vlan arguments, though there is also a **kwargs added to each method. Its probably worth having a comment at the top of the class describing the difference.

quantum/plugins/cisco/l2network_model.py

* Its not obvious to a reader why certain methods are implemented here while others aren't. A comment could help clarify why methods like update_port(), get_all_ports(), etc are not implemented (e.g., must they be implemented by a child class? If so, raising a NotImplemented exception might be better than silently passing).

quantum/plugins/cisco/l2network_model_base.py

* use of the term "model" threw me at first, as in nova and the main quantum code-base "model" tends to refer to an sqlalchemy database model, whereas this code (I don't think) is related to a database (I think it is using model more in the sense of "what the network looks like". I'm not sure there's another term you should be using instead, but at least be aware of the potential confusion.

quantum/plugins/cisco/nexus/cisco_nexus_network_driver.py

* for readability, i might move the xml snippets to another file that is imported by the main driver. This probably applies to the UCS driver as well.

* probably best to change these prints to LOGs, as they can probably get pretty verbose

print confstr

* this method seemed out of place, should it be in a test file instead?
def test_nxos_api(self, host, user, password):

* I agree with Edgar's TODO :)
2018
+ #TODO (Edgar) Move the SSH port to the configuration file

quantum/plugins/cisco/nexus/cisco_nexus_plugin.py

* I'm still trying to wrap my head around this, let me know if I am thinking about this the right way: this class isn't really a Quantum plugin per se, as it does not implement all of the methods. Rather, it is a class that can be used in conjunction with another "parent" plugin (quantum/plugins/cisco/l2network_plugin.py) that does things like determining the VLAN associated with a network and invoking "sub-plugins".

Given that the parent plugin stores all data, I'm a bit confused as to why the nexus "child" plugin also stores network data using the _networks variable. This isn't necessarily bad, I'm just not sure I follow what is going on. Is the thought that perhaps in the future the nexus plugin might be used stand-alone? The fact that data is in two places leads to some behavior I don't understand, like why the nexus plugin implements get_network_details when it seems like the parent plugin ignores what is returned by the child plugins and provides the info out of its own stored data. Maybe I'm just tired and missing something though :)

quantum/plugins/cisco/run_tests.py

* its best to avoid copying all of the code from the main run_tests.py . Check out the changes in: https://code.launchpad.net/~danwent/quantum/test-refactor/+merge/70664 . If you can, please review this branch so we can get it into trunk, then you can take advantage of it :)

General:

* there seem to be multiple config files that contain the same information (e.g., nova db credentials). It may be that these files are used by different plugins, but it was a bit confusing nonetheless. Totally up to you whether you want to change it.

* I think salvatore mentioned this, but it would be good to remove all of the extra main() functions used for testing. Not a big deal, but a good housekeeping task.

Very cool to see this code is ready for merge.  Having a Cisco plugin is an important step for the project as a whole, so I'd like to see this code get in ASAP.

I have a good number of comments below, but for the most part they are just suggestions for how you could improve the readability of the code for developers (like me!) that are looking at it for the first time.  I don't see any of them as blockers and since this code is entirely self-contained, I'm voting to approve for merge right away.  Getting the pylint score up is a good goal, but as long as you take care of that with the next merge I'm cool with it.

Great work :)

Dan

'quantum/plugins/cisco/README'

* The notion of a "reference implementation" vs. the Palo/Nexus specific functionality makes sense, since you've explained it to me before.  Perhaps separate out the audiences would make it more clear though.  For quantum administrators, this code is useful because they can us it with UCS/Nexus based networks.  For quantum developers this code is useful because they may want to base their own plugin on it.

* If you are using a special branch of nova, it may be cleaner + easier to just add a sqlalchemy migrate script to automatically create this table with the user syncs the DB.   
200
+1.  Create a table in the "nova" database for ports.  This can be 
201
+    accomplished with the following SQL statement:

quantum/plugins/cisco/common/cisco_constants.py

* Currently, the rest of the Quantum codebase uses "ACTIVE" instead of "UP".  While I'm personally in favor of "UP", its probably best to be consistent, then we can switch all of them at the same time if we decide that's the right thing to do.

+PORT_UP = "UP"

* it strikes me that some of these constants are not plugin specific, so ideally we'd actually have those in quantum/common, so they can be leveraged by all plugins, clients, tests, etc.  This isn't required, but would be a nice thing to do at some point.

quantum/plugins/cisco/common/cisco_credentials.py

* I'm guessing your OK with this, but it struck me as a bit odd that you had methods for modifying the store, but those modifications where not persisted.  Just wanted to make sure that is what you intended.

quantum/plugins/cisco/l2device_plugin_base.py

It took me a bit of glancing to figure out how this file differed from the standard quantum/quantum_plugin_base.py.  Main difference is that create_network takes vlan arguments, though there is also a **kwargs added to each method.  Its probably worth having a comment at the top of the class describing the difference.

quantum/plugins/cisco/l2network_model.py

* Its not obvious to a reader why certain methods are implemented here while others aren't.  A comment could help clarify why methods like update_port(), get_all_ports(), etc are not implemented (e.g., must they be implemented by a child class?  If so, raising a NotImplemented exception might be better than silently passing).

quantum/plugins/cisco/l2network_model_base.py

* use of the term "model" threw me at first, as in nova and the main quantum code-base "model" tends to refer to an sqlalchemy database model, whereas this code (I don't think) is related to a database (I think it is using model more in the sense of "what the network looks like".  I'm not sure there's another term you should be using instead, but at least be aware of the potential confusion.

quantum/plugins/cisco/nexus/cisco_nexus_network_driver.py

* for readability, i might move the xml snippets to another file that is imported by the main driver.  This probably applies to the UCS driver as well.

* probably best to change these prints to LOGs, as they can probably get pretty verbose

print confstr

* this method seemed out of place, should it be in a test file instead? 
def test_nxos_api(self, host, user, password):

* I agree with Edgar's TODO :) 
2018
+        #TODO (Edgar) Move the SSH port to the configuration file

quantum/plugins/cisco/nexus/cisco_nexus_plugin.py

* I'm still trying to wrap my head around this, let me know if I am thinking about this the right way:  this class isn't really a Quantum plugin per se, as it does not implement all of the methods.  Rather, it is a class that can be used in conjunction with another "parent" plugin (quantum/plugins/cisco/l2network_plugin.py) that does things like determining the VLAN associated with a network and invoking "sub-plugins".

Given that the parent plugin stores all data, I'm a bit confused as to why the nexus "child" plugin also stores network data using the _networks variable.  This isn't necessarily bad, I'm just not sure I follow what is going on.  Is the thought that perhaps in the future the nexus plugin might be used stand-alone?  The fact that data is in two places leads to some behavior I don't understand, like why the nexus plugin implements get_network_details when it seems like the parent plugin ignores what is returned by the child plugins and provides the info out of its own stored data.  Maybe I'm just tired and missing something though :)

quantum/plugins/cisco/run_tests.py

* its best to avoid copying all of the code from the main run_tests.py .  Check out the changes in: https://code.launchpad.net/~danwent/quantum/test-refactor/+merge/70664 .  If you can, please review this branch so we can get it into trunk, then you can take advantage of it :)

General:

* there seem to be multiple config files that contain the same information (e.g., nova db credentials).  It may be that these files are used by different plugins, but it was a bit confusing nonetheless.  Totally up to you whether you want to change it.

* I think salvatore mentioned this, but it would be good to remove all of the extra main() functions used for testing.  Not a big deal, but a good housekeeping task.

review: Approve

« Back to merge proposal