Merge lp:~salvatore-orlando/neutron/bug834012 into lp:neutron/diablo

[Salvatore Orlando]

This branch targets two bugs:

Bug #834012: API: GET networks/{net_id}/detail is inefficient
Bug #837535: plugins do not comply with quantum_plugin_interface

They are both addressed by this branch as they are strictly related.
For Bug #834012 - changed the _item method code in the network API controller in order to avoid unnecessary calls to the plugin. The view builder was also adapted for taking into account this change.
For Bug #837535 - simply updated the documentation of quantum plugin base to state that get_network_details should return an attribute called net-ports with a sequence of ports. This seems a reasonable thing to do, as this is the way in which the currently available plugins work. Morever, and this is just my personal opinion, net-ports makes more sense than net-iface (which return the interface list only).

I realize a change in the plugin interface will probably require a deeper discussion. Please feel free to disapprove this branch if you don't agree with the proposed change.

Also, you'll see a small change in SamplePlugin. This is because it was not 100% compliant with the plugin interface.

[dan wendlandt (danwent)]

Hi Salvatore, thanks for taking care of this. I definitely agree with the goal of the patch, my only concern about timing. In general, since it seems like none of these bugs impair the correct operation of quantum, my bias would be to avoid making this change unless we have high confidence that we're not breaking plugins.

I downloaded the branch and ran unit tests with the OVS plugin and they seemed to pass. Is your feeling that the same would be true for other plugins?

Given that not all plugins have to be part of the Quantum repo, I'm still a bit wary of making a change so late, so at the very least we'd need to send an email to the list describing how plugins would have to change (assuming they were written to spec) and see if anyone raises a concern.

review: Needs Information

[Salvatore Orlando (salvatore-orlando)]

> Hi Salvatore, thanks for taking care of this. I definitely agree with the
> goal of the patch, my only concern about timing. In general, since it seems
> like none of these bugs impair the correct operation of quantum, my bias would
> be to avoid making this change unless we have high confidence that we're not
> breaking plugins.

I don't see any urgent reason for merging this bug fix before rbp.

>
> I downloaded the branch and ran unit tests with the OVS plugin and they seemed
> to pass. Is your feeling that the same would be true for other plugins?

I don't think so, as the change in the plugin interface is just in the documentation. Basically, what I've done is making sure the documentation of get_network_details is aligned with what the plugins actually do.

>
> Given that not all plugins have to be part of the Quantum repo, I'm still a
> bit wary of making a change so late, so at the very least we'd need to send an
> email to the list describing how plugins would have to change (assuming they
> were written to spec) and see if anyone raises a concern.

Agreed. I will try and provide a different fix that does not change the plugin interface.
I thinj the blukprint Somik recently submitted is aimed at making sure plugins fully comply with the plugin interface.

[Salvatore Orlando (salvatore-orlando)]

Reverting to WIP for fixing the API behaviour without affecting the plugin interface.

[Salvatore Orlando (salvatore-orlando)]

Updated the code for addressing bug #834012 only

[lp:~salvatore-orlando/neutron/bug834012]

64. By Salvatore Orlando on 2011-09-02

Fuxing bug #834012 only

[dan wendlandt (danwent)]

In general I'm hesitant make changes at this in the release cycle that do not improve test coverage or fix a real correctness problem, as they can have unintended consequences. We can make this change if you like, but the fewer the better in my opinion.

I don't think this branch does quite what you're looking to do. I believe you want it to be OK for a plugin to return a network diction that does not contain "net-ports", but it actually will cause an exception in that case.

You probably want to replace:

if not network['net-ports']:

with

if not 'net-ports' in network:

The former will throw a KeyError if 'net-ports' is not in the networks dictionary.

Otherwise it looks good, though we'll want to make sure that we get around to updating the plugins to take advantage of the more efficient approach in essex. I can file a bug against the OVS plugin.

review: Needs Fixing

[lp:~salvatore-orlando/neutron/bug834012]

65. By Salvatore Orlando on 2011-09-06

Addressing Dan's comment

[Salvatore Orlando (salvatore-orlando)]

> In general I'm hesitant make changes at this in the release cycle that do not
> improve test coverage or fix a real correctness problem, as they can have
> unintended consequences. We can make this change if you like, but the fewer
> the better in my opinion.

I'm not either a big fan of this change anymore. This change does not fix anything, it is more a "quick and dirty" patch for making the detail operation more efficient.

>
> I don't think this branch does quite what you're looking to do. I believe you
> want it to be OK for a plugin to return a network diction that does not
> contain "net-ports", but it actually will cause an exception in that case.
>
> You probably want to replace:
>
> if not network['net-ports']:
>
> with
>
> if not 'net-ports' in network:
>

Thanks for pointing this out. The branch has been updated.

> The former will throw a KeyError if 'net-ports' is not in the networks
> dictionary.
>
> Otherwise it looks good, though we'll want to make sure that we get around to
> updating the plugins to take advantage of the more efficient approach in
> essex. I can file a bug against the OVS plugin.

I'd say we should fix this bug in Essex as well.
I'll revert the MP to WIP, and then discuss this bug fix at today's meeting.

Unmerged revisions

65. By Salvatore Orlando on 2011-09-06

Addressing Dan's comment

64. By Salvatore Orlando on 2011-09-02

Fuxing bug #834012 only

63. By Salvatore Orlando on 2011-09-01

Now properly doing the commit.

=== modified file 'quantum/api/networks.py'
--- quantum/api/networks.py	2011-08-17 21:48:56 +0000
+++ quantum/api/networks.py	2011-09-06 09:13:02 +0000
@@ -52,14 +52,17 @@
         # concerning logical ports as well.
         network = self._plugin.get_network_details(
                             tenant_id, network_id)
-        port_list = self._plugin.get_all_ports(
-                            tenant_id, network_id)
-        ports_data = [self._plugin.get_port_details(
-                                   tenant_id, network_id, port['port-id'])
-                      for port in port_list]
+        # We try to leverage the fact that current plugin
+        # return a net-ports structure. However, do not bet
+        # on it, as it is not sanctioned by the plugin interface
+        if not 'net-ports' in network:
+            port_list = self._plugin.get_all_ports(tenant_id, network_id)
+            ports_data = [self._plugin.get_port_details(
+                                       tenant_id, network_id, port['port-id'])
+                          for port in port_list]
+            network['net-ports'] = ports_data
         builder = networks_view.get_view_builder(req)
-        result = builder.build(network, net_details,
-                               ports_data, port_details)['network']
+        result = builder.build(network, net_details, port_details)['network']
         return dict(network=result)
 
     def _items(self, req, tenant_id, net_details=False):

=== modified file 'quantum/api/views/networks.py'
--- quantum/api/views/networks.py	2011-08-25 17:50:05 +0000
+++ quantum/api/views/networks.py	2011-09-06 09:13:02 +0000
@@ -29,15 +29,15 @@
         """
         self.base_url = base_url
 
-    def build(self, network_data, net_detail=False,
-              ports_data=None, port_detail=False):
+    def build(self, network_data, net_detail=False, port_detail=False):
         """Generic method used to generate a network entity."""
         if net_detail:
             network = self._build_detail(network_data)
         else:
             network = self._build_simple(network_data)
         if port_detail:
-            ports = [self._build_port(port_data) for port_data in ports_data]
+            ports = [self._build_port(port_data)
+                     for port_data in network_data['net-ports']]
             network['network']['ports'] = ports
         return network
 

=== modified file 'quantum/plugins/SamplePlugin.py'
--- quantum/plugins/SamplePlugin.py	2011-09-01 16:17:36 +0000
+++ quantum/plugins/SamplePlugin.py	2011-09-06 09:13:02 +0000
@@ -234,16 +234,16 @@
 
     def get_all_ports(self, tenant_id, net_id):
         """
-        Retrieves all port identifiers belonging to the
+        Retrieves all ports belonging to the
         specified Virtual Network.
         """
         LOG.debug("FakePlugin.get_all_ports() called")
-        port_ids = []
+        port_data = []
         ports = db.port_list(net_id)
-        for x in ports:
-            d = {'port-id': str(x.uuid)}
-            port_ids.append(d)
-        return port_ids
+        for port in ports:
+            port_data.append(self.get_port_details(tenant_id,
+                                                   net_id, port.uuid))
+        return port_data
 
     def get_port_details(self, tenant_id, net_id, port_id):
         """