Ok, sounds like we're in good shape with this branch. I know Salvatore said
he was at least partially out-of-office this week, but it doesn't sound like
there were any blockers from his end, so hopefully we can get this merged
early this week.

Dan

p.s.  the diagram in the document does indeed make things more clear.  Its
funny, as I had almost added a comment to my review saying that a diagram
would be helpful :)

On Mon, Aug 15, 2011 at 2:31 AM, Sumit Naiksatam <email address hidden> wrote:

> 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.
> >
>
> <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.
> >
>
> <Sumit> Ok, will look into it. </Sumit>
>
> > 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).
> >
>
> <Sumit> Will do. </Sumit>
>
> > 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
> >
>
> <Sumit> Will do. </Sumit>
>
> > 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>
> --
>
> https://code.launchpad.net/~cisco-openstack/quantum/l2network-plugin/+merge/70804
> You are reviewing the proposed merge of
> lp:~cisco-openstack/quantum/l2network-plugin into lp:quantum.
>



-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dan Wendlandt
Nicira Networks, Inc.
www.nicira.com | www.openvswitch.org
Sr. Product Manager
cell: 650-906-2650
~~~~~~~~~~~~~~~~~~~~~~~~~~~