neutron

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

l2network-plugin
Merge into diablo

Revision history for this message

Sumit Naiksatam (snaiksat) wrote on 2011-08-15:

Thanks Dan for the thorough review, and the words of appreciation. My responses inline, pretty much in agreement with most of your suggestions (and a lot of the changes have already gone into the ongoing work). Some of the points which you have raised are better clarified in the document posted here:
http://wiki.openstack.org/quantum-multi-switch-plugin (we should have made this available sooner).

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

<Sumit> Agree. This is actually a temporary requirement, and should go away altogether. </Sumit>

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

<Sumit> Agree. This has already been changed in a new branch. </Sumit>

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

<Sumit> Good catch. This branch in general has no persistence support. This has been introduced in a different branch which we would like to propose merging once this one is approved. </Sumit>

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

<Sumit> Will do that. In general we can do with a little more documenting (and pylint is probably going to push us into doing that as well). This particular base class definition is different from the Quantum plugin in that this is proposed for a device-specific plugin to accommodate the requirements of specific devices when implementing the core abstractions. For example, a device-plugin would at the very least require the IP address of the device which it needs to configure. This and other such parameters can be passed as args/kwargs. In general, it will be helpful to read the document posted at: http://wiki.openstack.org/quantum-multi-switch-plugin (if not the whole document, even looking at the diagram will convey enough information) to understand the general structure/design being proposed. </Sumit>

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

<Sumit> Agree. The term "model" does tend to throw people off, and for good reason. But I couldn't come up with a better term either. I am totally open for suggestions (wanted to avoid "topology"). Your understanding is however spot on. </Sumit>

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

<Sumit> Agree. </Sumit>

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

<Sumit> Agree. </Sumit>

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

<Sumit> Good catch, we wanted to leave it there so that one can independently test the driver, but agree with your point. </Sumit>

> * 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 :)

<Sumit> Most of your understanding is spot on, the nexus plugin is part of a plugin hierarchy, and the l2network_plugin is it's parent in that hierarchy. The thought was indeed that each device-specific plugin would maintain it's own state. The additional information (which you do not see being propagated to the parent) is intended to be propagated to extension API calls (which are currently not implemented in this branch). </Sumit>
>
> 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 :)
>

<Sumit> Will do, AI for Shweta I believe. </Sumit>

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

<Sumit> we wanted to keep the configuration files to be plugin-specific so that one could independently modify them without hurting the other. Some of the nova-specific configuration will go away since this is temporary. But thanks for the tip, we will try to avoid duplication or sprawl wherever applicable. </Sumit>

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

<Sumit> Yes, I had done that immediately after receiving Salvatore's review, apparently I committed, but forgot to push the changes. At any rate, I just did it. </Sumit>

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

<Sumit> Agreed. We will try to do a better job of highlighting the L2 Network Plugin Framework (to support multiple devices/switches) versus the specific support for UCS/Nexus, both of which have gone into this branch. The former is intended to be useful for anyone using the Quantum service, and faced with the issue of handling multiple types of devices.</Sumit>
 
> * 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:
>

<Sumit> Agree. This is actually a temporary requirement, and should go away altogether. </Sumit>

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

<Sumit> Agree. This has already been changed in a new branch. </Sumit>

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

<Sumit> Good catch. This branch in general has no persistence support. This has been introduced in a different branch which we would like to propose merging once this one is approved. </Sumit>

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

<Sumit> Agree. </Sumit>

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

<Sumit> Agree. </Sumit>

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

<Sumit> Good catch, we wanted to leave it there so that one can independently test the driver, but agree with your point. </Sumit>

> * 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 :)

<Sumit> Most of your understanding is spot on, the nexus plugin is part of a plugin hierarchy, and the l2network_plugin is it's parent in that hierarchy. The thought was indeed that each device-specific plugin would maintain it's own state. The additional information (which you do not see being propagated to the parent) is intended to be propagated to extension API calls (which are currently not implemented in this branch). </Sumit>
> 
> 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 :)
>

<Sumit> Will do, AI for Shweta I believe. </Sumit>

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

<Sumit> Yes, I had done that immediately after receiving Salvatore's review, apparently I committed, but forgot to push the changes. At any rate, I just did it. </Sumit>

« Back to merge proposal