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

Revision history for this message
Somik Behera (somikbehera) 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.

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.

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

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

review: Approve (netstack-core)

« Back to merge proposal