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

Revision history for this message
Somik Behera (somikbehera) wrote :

Hi Salvatore,

This is a good step in the right direction, towards 1.0!

A reviewed, this changeset, the API wiki, quantum_plugin_base that species the data structure that plugins are mandated to use while passing data to the API layer to transform into XML/JSON for external world to consume.

I see a few shortcomings:

1) quantum_plugin_base.py specifies the "interface" for plugin developers, so that plugin developers can follow this file and implement their plugins without looking at any other place. Along those lines, this "interface" file also specifies the data structure/ "format" that plugins are supposed to use while returning data to the API layer.

Currently, the API layer's expectation regarding data "format" are not aligned with what quantum_plugin_base specifies. Therefore, one or other, or both might need fixing.

2) I noticed we upped the version to /v1.0 and I just wanted ask you if we have appropriate bugs on client-side, if any, already filed to ensure that the client is compliant.

3) One thing I have noticed that is not very consistent, within the API, Unit test and documentation implementation is the data format of for the "attachment"

3.1) This is what plugin_base specifies( API can provide a different format to Northbound API client while keeping the specified format on southbound side plugin side):

    @abstractmethod
    def get_port_details(self, tenant_id, net_id, port_id):
        """
        This method allows the user to retrieve a remote interface
        that is attached to this particular port.

        :returns: a mapping sequence with the following signature:
                    {'port-id': uuid representing the port on
                                 specified quantum network
                     'net-id': uuid representing the particular
                                quantum network
                     'attachment': uuid of the virtual interface
                                   bound to the port, None otherwise
                    }

3.2) Here's the attachment format for Northbound API from wiki:

{
 "attachment":
     {
         "id": "test_interface_identifier"
     }
}

3.3) file quantum/api/views/attachments.py looks for "attachment-id" when looking for the identifier representing the attachment, which is inconsistent on the southbound(quantum_plugin_base) and northbound( API wiki) side.

Looking at file history, it looks like its being changed from attachment -> attachment-id

I would prefer that the southbound side( quantum_plugin_base specification) should be left as attachment if there is not good reason to change the naming as it will break compatibility with all the plugins that have been written, and won't provide any enhanced functionality. We can definitely change things northbound as thats our public API.

3.4) Similarly, net-name was changed to "name" on northbound and southbound side, I have similar feelings that we keep the the southbound side( data format specified in quantum_plugin_base) be kept same while changing northbound. Same is try for port-state -> "state" change.

4) Based on final conclusions, we will need to update unit tests and quantum_plugin_base.py doc as well. I am of the opinion that we keep the existing southbound naming( as specified in quantum_plugin_base) and update the northbound data format.

I think we are pretty close and my apologies for not bringing this issues to the front earlier.

review: Needs Fixing (netstack-core)

« Back to merge proposal