On Fri, Jul 29, 2011 at 8:25 AM, Salvatore Orlando < salvatore.orla...@eu.citrix.com> wrote:
> Review: Needs Fixing > Hi Santhosh! > Thanks for this important contribution to the Quantum project. > > >From the code it seems the implementation allows for defining new > resources, extending quantum core API resources, and adding actions to Core > API resources. It was my understanding that in the NetStack meeting we kind > of agreed extensions will not alter core API resources. > > For instance a 'QoS' attribute for a port should have not been defined as > an 'extended' attribute of the core API port object, but within a > "port-profile" extended resources, which would then be bound to the core API > port object. > This does not mean I'm not willing to accept code that extends core API > resources; I just want to clarify what we decided in our meetings. > I think this is a key point to discuss, thus, I'm moving it to the nestack list. I do not remember us reaching any official decision on whether or not "core" API objects (right now, "network" and "port") could or could not be modified using "data extension". I remember an IRC discussion where this was suggested as an option though. I think this is great design discussion to have on the netstack list. Here are a couple thoughts to get the ball rolling: - Data extension of core objects will probably lead to the most intuitive API from a user perspective, as there can be a smaller number of unique objects to deal with. - Data extension of core objects may complicate API internals + validation, as the core API code no longer knows exactly what is or is not valid within a network or port object. - I believe nova currently allows data extension of core objects, and our bias is to "do as OpenStack does" unless we have a strong reason to differ. - My goal is that people will propose extensions with the goal of them being incorporated as part of the "core" eventually. Thus, it would seem logical that they could implement the extension so that it is exposed the same way it would be exposed as part of the core API (i.e., data extension is an option). - It is important that we make it easy for API clients to validate that a particular extension is supported by a Quantum instance. I believe this is possible both with and without data extension, but doing so may be simpler without data extension. Thoughts? Dan p.s. I also think this is related to https://bugs.launchpad.net/quantum/+bug/814518, which is a unit test that fails because we currently do not reject request bodies with "extra" bogus fields. Whether we want to add that kind of validation probably depends on the answer to the above question. > There are also a few minor comments: > - I cannot run unit tests as it seems there is a missing file. I have an > import failure in test_extensions.pu, line 23: from webtest import TestApp > - There is a small, probably reasonable change in quantum/manager.py. This > change introduces an "options" parameter for the quantum plugin; it will > cause the OVS plugin to fail upon load. I don't mind having the capability > of passing options to a plugin, but we should first agree on a change on the > plugin interface. > NOTE: The above point might have resulted from branching from > lp:~quantum/quantum-unit-tests. I added a similare feature (options to the > plugin), and then removed it after reviewers found it was requiring a change > in the plugin interface. > > Another minor-minor point: > - I would move all stub code into an appropriate file, leaving test cases > alone in test_extensions; IMHO this will improve readability and > mainteinance. > > Apart from the above points, the code looks pretty good. > -- > > https://code.launchpad.net/~raxnetworking/quantum/api_extensions/+merge/69227 > Your team Netstack Core Developers is subscribed to branch lp:quantum. > -- ~~~~~~~~~~~~~~~~~~~~~~~~~~~ Dan Wendlandt Nicira Networks, Inc. www.nicira.com | www.openvswitch.org Sr. Product Manager cell: 650-906-2650 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- Mailing list: https://launchpad.net/~netstack Post to : netstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~netstack More help : https://help.launchpad.net/ListHelp
