Code review comment for lp:~salvatore-orlando/neutron/quantum-api-alignment

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

> > Hi Somik,
> >
> > thanks for your reply.
> > It seems we have now an agreement and can finally close down API v1 for
> > Quantum.
> > I've updated the branch for taking into account the issue with the
> > 'attachment-id' identifier you spotted.
> >
> > However, prompted by your review, I double-checked consistency with
> > specification in quantum-plugin-base, and I spotted a problem with
> > get_network_details.
> > Quantum_plugin_base states that id should return a sequence whose elements
> > have the following attributes: 'net-id', 'net-name', 'net-ifaces', with the
> > latter being a list of interface identifiers.
> >
> > However:
> > - the API does not look for 'net-ifaces' at all. It instead invoke
> > get_all_ports and get_port_details for each port; the API therefore works,
> but
> > not in the most efficient way. This could be fixed quite easily;
> > - however the plugins (FakePlugin, Cisco, and even Openvswitch), return a
> > sequence
> > whose elements have the following attributes: 'net-id', 'net-name', 'net-
> > ports' (instead of 'net-ifaces').
> >
>
> Good find Salvatore, this is an interesting issue where the plugin interface,
> API interface and the plugin implementation all diverge.
>
> This is what I suggest, we should keep the API working efficiently, having
> net-ifaces, doesn't add overhead and makes the API efficient. With that in
> mind, this seems to be a good thing to keep in quantum_plugin_base interface.
>

Definetely agree.

> For the other issues, I believe we should file bugs, and once the plugins have
> fixed themselves, the API can then take advantage of the more performant
> mechanism.
>
> Iterating over all ports, while ok in short term, will be an issue in large
> scale environments.

It is not efficient at all, as it will force clients to do a lot of largely unnecessary round trips.
I will file one or more bugs to improve API and client library accordingly (for instance the client library does not currently leverage the 'detail' actions)

>
> > I think we probably want to sort this out. We can either change plugin
> > interface documentation, or the plugins. Whatever we decide, we should also
> > update the API to ensure it works in an efficient way.
> >
> > What's the best approach to tackle this problem in your opinion?
> >
> > Cheers,
> > Salvatore
>
> Everything else looks good!
>

Thanks!

> But, I noticed, the somehow some pep8 violations have creeped into the trunk,
> I'll be filing those bugs soon.

Good thing you spotted this. I do pep8 tests executing run_tests.sh, which does not check .py files in root directory!

« Back to merge proposal