Avoid notifying while inside transaction opened in delete_port()

delete_port() calls to disassociate_floatingips() while in transaction.
The latter method sends RPC notification which may result in eventlet
yield. If yield switches a thread to another one that tries to access
the same floating IP object in db as disassociate_floatingips() method
does, we're locked and get db timeout.

We should avoid calling to notifier while under transaction.

To achieve this, I introduce a do_notify argument that controls whether
notification is done by disassociate_floatingips() itself or delegated
to caller. Callers that call to disassociate_floatingips() from under
transactions should handle notifications on their own. For this,
disassociate_floatingips() returns a set of routers that require
notification.

Updated drivers to reflect new behaviour. Added unit test.

Change-Id: I2411f2aa778ea088be416d062c4816c16f49d2bf
Closes-Bug: 1330955

diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py
index cad9e48153dae86ec58ade3f1657b881f4b8845b..f2bdfdda57ac6cf73275d96cc292e0cf170a057e 100644 (file)
--- a/neutron/db/l3_db.py
+++ b/neutron/db/l3_db.py
@@ -877,7 +877,14 @@ class L3_NAT_db_mixin(l3.RouterPluginBase):
                           {'port_id': port_db['id'],
                            'port_owner': port_db['device_owner']})
 
-    def disassociate_floatingips(self, context, port_id):
+    def disassociate_floatingips(self, context, port_id, do_notify=True):
+        """Disassociate all floating IPs linked to specific port.
+
+        @param port_id: ID of the port to disassociate floating IPs.
+        @param do_notify: whether we should notify routers right away.
+        @return: set of router-ids that require notification updates
+                 if do_notify is False, otherwise None.
+        """
         router_ids = set()
 
         with context.session.begin(subtransactions=True):
@@ -888,7 +895,15 @@ class L3_NAT_db_mixin(l3.RouterPluginBase):
                 floating_ip.update({'fixed_port_id': None,
                                     'fixed_ip_address': None,
                                     'router_id': None})
+        if do_notify:
+            self.notify_routers_updated(context, router_ids)
+            # since caller assumes that we handled notifications on its
+            # behalf, return nothing
+            return
+
+        return router_ids
 
+    def notify_routers_updated(self, context, router_ids):
         if router_ids:
             self.l3_rpc_notifier.routers_updated(
                 context, list(router_ids),

diff --git a/neutron/plugins/bigswitch/plugin.py b/neutron/plugins/bigswitch/plugin.py
index 306016a8feab559171589628b54e10a8469dcaec..b2d11fe8bf19f4c4cbf6e3d02e41c116380b0c89 100644 (file)
--- a/neutron/plugins/bigswitch/plugin.py
+++ b/neutron/plugins/bigswitch/plugin.py
@@ -810,7 +810,8 @@ class NeutronRestProxyV2(NeutronRestProxyV2Base,
         if l3_port_check:
             self.prevent_l3_port_deletion(context, port_id)
         with context.session.begin(subtransactions=True):
-            self.disassociate_floatingips(context, port_id)
+            router_ids = self.disassociate_floatingips(
+                context, port_id, do_notify=False)
             self._delete_port_security_group_bindings(context, port_id)
             port = super(NeutronRestProxyV2, self).get_port(context, port_id)
             # Tenant ID must come from network in case the network is shared
@@ -818,6 +819,9 @@ class NeutronRestProxyV2(NeutronRestProxyV2Base,
             self._delete_port(context, port_id)
             self.servers.rest_delete_port(tenid, port['network_id'], port_id)
 
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
+
     @put_context_in_serverpool
     def create_subnet(self, context, subnet):
         LOG.debug(_("NeutronRestProxyV2: create_subnet() called"))
@@ -1090,11 +1094,12 @@ class NeutronRestProxyV2(NeutronRestProxyV2Base,
                 self._send_floatingip_update(context)
 
     @put_context_in_serverpool
-    def disassociate_floatingips(self, context, port_id):
+    def disassociate_floatingips(self, context, port_id, do_notify=True):
         LOG.debug(_("NeutronRestProxyV2: diassociate_floatingips() called"))
-        super(NeutronRestProxyV2, self).disassociate_floatingips(context,
-                                                                 port_id)
+        router_ids = super(NeutronRestProxyV2, self).disassociate_floatingips(
+            context, port_id, do_notify=do_notify)
         self._send_floatingip_update(context)
+        return router_ids
 
     # overriding method from l3_db as original method calls
     # self.delete_floatingip() which in turn calls self.delete_port() which

diff --git a/neutron/plugins/cisco/n1kv/n1kv_neutron_plugin.py b/neutron/plugins/cisco/n1kv/n1kv_neutron_plugin.py
index 431cbc6d8be9a9340cfb4b576b71fc0ad72b36db..6e259ccc17e23e43c63e9b55b54ce260726fdb2b 100644 (file)
--- a/neutron/plugins/cisco/n1kv/n1kv_neutron_plugin.py
+++ b/neutron/plugins/cisco/n1kv/n1kv_neutron_plugin.py
@@ -1228,8 +1228,13 @@ class N1kvNeutronPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
                 n1kv_db_v2.delete_vm_network(context.session,
                                              port[n1kv.PROFILE_ID],
                                              port['network_id'])
-            self.disassociate_floatingips(context, id)
+            router_ids = self.disassociate_floatingips(
+                context, id, do_notify=False)
             super(N1kvNeutronPluginV2, self).delete_port(context, id)
+
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
+
         self._send_delete_port_request(context, port, vm_network)
 
     def get_port(self, context, id, fields=None):

diff --git a/neutron/plugins/embrane/base_plugin.py b/neutron/plugins/embrane/base_plugin.py
index 3434a96d41a545b450e5460c3dfe90865f8481c8..8dedc653ae5303d1c15462e72871b58269900f3e 100644 (file)
--- a/neutron/plugins/embrane/base_plugin.py
+++ b/neutron/plugins/embrane/base_plugin.py
@@ -351,14 +351,15 @@ class EmbranePlugin(object):
                 args=(nat_info,))
         return result
 
-    def disassociate_floatingips(self, context, port_id):
+    def disassociate_floatingips(self, context, port_id, do_notify=True):
         try:
             fip_qry = context.session.query(l3_db.FloatingIP)
             floating_ip = fip_qry.filter_by(fixed_port_id=port_id).one()
             router_id = floating_ip["router_id"]
         except exc.NoResultFound:
             return
-        self._l3super.disassociate_floatingips(self, context, port_id)
+        router_ids = self._l3super.disassociate_floatingips(
+            self, context, port_id, do_notify=do_notify)
         if router_id:
             neutron_router = self._get_router(context, router_id)
             fip_id = floating_ip["id"]
@@ -371,3 +372,4 @@ class EmbranePlugin(object):
                     p_con.Events.RESET_NAT_RULE, neutron_router, context,
                     state_change),
                 args=(fip_id,))
+        return router_ids

diff --git a/neutron/plugins/linuxbridge/lb_neutron_plugin.py b/neutron/plugins/linuxbridge/lb_neutron_plugin.py
index 89b1354a426f4e17e0aa8fc5c2a7a50c5588b1f5..6ddbba680d9020e944709f180c236881a0039072 100644 (file)
--- a/neutron/plugins/linuxbridge/lb_neutron_plugin.py
+++ b/neutron/plugins/linuxbridge/lb_neutron_plugin.py
@@ -526,11 +526,14 @@ class LinuxBridgePluginV2(db_base_plugin_v2.NeutronDbPluginV2,
 
         session = context.session
         with session.begin(subtransactions=True):
-            self.disassociate_floatingips(context, id)
+            router_ids = self.disassociate_floatingips(
+                context, id, do_notify=False)
             port = self.get_port(context, id)
             self._delete_port_security_group_bindings(context, id)
             super(LinuxBridgePluginV2, self).delete_port(context, id)
 
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
         self.notify_security_groups_member_updated(context, port)
 
     def _notify_port_updated(self, context, port):

diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py
index 4a46bf5482961caace5b8f6cb63def68d4342a46..9407b31ca905a9782b743e9275825b9d6d4b0aaf 100644 (file)
--- a/neutron/plugins/ml2/plugin.py
+++ b/neutron/plugins/ml2/plugin.py
@@ -744,10 +744,15 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
             self._delete_port_security_group_bindings(context, id)
             LOG.debug(_("Calling base delete_port"))
             if l3plugin:
-                l3plugin.disassociate_floatingips(context, id)
+                router_ids = l3plugin.disassociate_floatingips(
+                    context, id, do_notify=False)
 
             super(Ml2Plugin, self).delete_port(context, id)
 
+        # now that we've left db transaction, we are safe to notify
+        if l3plugin:
+            l3plugin.notify_routers_updated(context, router_ids)
+
         try:
             self.mechanism_manager.delete_port_postcommit(mech_context)
         except ml2_exc.MechanismDriverError:

diff --git a/neutron/plugins/mlnx/mlnx_plugin.py b/neutron/plugins/mlnx/mlnx_plugin.py
index 79af9225045005dc3853649037b38c1883652608..4ce3d513bdfc5f6a2e346ce02d68bece16528c6d 100644 (file)
--- a/neutron/plugins/mlnx/mlnx_plugin.py
+++ b/neutron/plugins/mlnx/mlnx_plugin.py
@@ -502,9 +502,12 @@ class MellanoxEswitchPlugin(db_base_plugin_v2.NeutronDbPluginV2,
 
         session = context.session
         with session.begin(subtransactions=True):
-            self.disassociate_floatingips(context, port_id)
+            router_ids = self.disassociate_floatingips(
+                context, port_id, do_notify=False)
             port = self.get_port(context, port_id)
             self._delete_port_security_group_bindings(context, port_id)
             super(MellanoxEswitchPlugin, self).delete_port(context, port_id)
 
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
         self.notify_security_groups_member_updated(context, port)

diff --git a/neutron/plugins/nec/nec_plugin.py b/neutron/plugins/nec/nec_plugin.py
index fbeebd7a1ae0e5155aebbeb7391cf902c48cb0cc..45733dc9e1ffa0a5b8fe3c8aa00d1fdd24e9d8f9 100644 (file)
--- a/neutron/plugins/nec/nec_plugin.py
+++ b/neutron/plugins/nec/nec_plugin.py
@@ -651,9 +651,13 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
         if l3_port_check:
             self.prevent_l3_port_deletion(context, id)
         with context.session.begin(subtransactions=True):
-            self.disassociate_floatingips(context, id)
+            router_ids = self.disassociate_floatingips(
+                context, id, do_notify=False)
             self._delete_port_security_group_bindings(context, id)
             super(NECPluginV2, self).delete_port(context, id)
+
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
         self.notify_security_groups_member_updated(context, port)
 
 

diff --git a/neutron/plugins/nuage/plugin.py b/neutron/plugins/nuage/plugin.py
index bf95c1eecf0fe70e287b0cf3090c61c77bc76e49..c5257ae35331c4a1a9b2ac19d93225bea0e9d265 100644 (file)
--- a/neutron/plugins/nuage/plugin.py
+++ b/neutron/plugins/nuage/plugin.py
@@ -935,8 +935,10 @@ class NuagePlugin(db_base_plugin_v2.NeutronDbPluginV2,
                         context, neutron_fip['id'])
             return neutron_fip
 
-    def disassociate_floatingips(self, context, port_id):
-        super(NuagePlugin, self).disassociate_floatingips(context, port_id)
+    def disassociate_floatingips(self, context, port_id, do_notify=True):
+        router_ids = super(NuagePlugin, self).disassociate_floatingips(
+            context, port_id, do_notify=do_notify)
+
         port_mapping = nuagedb.get_port_mapping_by_id(context.session,
                                                       port_id)
         if port_mapping:
@@ -946,10 +948,13 @@ class NuagePlugin(db_base_plugin_v2.NeutronDbPluginV2,
             }
             self.nuageclient.update_nuage_vm_vport(params)
 
+        return router_ids
+
     def update_floatingip(self, context, id, floatingip):
         fip = floatingip['floatingip']
         orig_fip = self._get_floatingip(context, id)
         port_id = orig_fip['fixed_port_id']
+        router_ids = []
         with context.session.begin(subtransactions=True):
             neutron_fip = super(NuagePlugin, self).update_floatingip(
                 context, id, floatingip)
@@ -965,9 +970,9 @@ class NuagePlugin(db_base_plugin_v2.NeutronDbPluginV2,
                                                    fip['port_id'])
                 except nuage_exc.OperationNotSupported:
                     with excutils.save_and_reraise_exception():
-                        super(NuagePlugin,
-                              self).disassociate_floatingips(context,
-                                                             fip['port_id'])
+                        router_ids = super(
+                            NuagePlugin, self).disassociate_floatingips(
+                                context, fip['port_id'], do_notify=False)
                 except n_exc.BadRequest:
                     with excutils.save_and_reraise_exception():
                         super(NuagePlugin, self).delete_floatingip(context,
@@ -981,7 +986,11 @@ class NuagePlugin(db_base_plugin_v2.NeutronDbPluginV2,
                         'nuage_fip_id': None
                     }
                     self.nuageclient.update_nuage_vm_vport(params)
-            return neutron_fip
+
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
+
+        return neutron_fip
 
     def delete_floatingip(self, context, id):
         fip = self._get_floatingip(context, id)

diff --git a/neutron/plugins/oneconvergence/plugin.py b/neutron/plugins/oneconvergence/plugin.py
index 1456007bdf3331db5b8d40609ae98f9194c443a8..a07005c1905cccec19034539b307f3271ef3d19e 100644 (file)
--- a/neutron/plugins/oneconvergence/plugin.py
+++ b/neutron/plugins/oneconvergence/plugin.py
@@ -351,7 +351,8 @@ class OneConvergencePluginV2(db_base_plugin_v2.NeutronDbPluginV2,
 
             self._delete_port_security_group_bindings(context, port_id)
 
-            self.disassociate_floatingips(context, port_id)
+            router_ids = self.disassociate_floatingips(
+                context, port_id, do_notify=False)
 
             super(OneConvergencePluginV2, self).delete_port(context, port_id)
 
@@ -360,6 +361,8 @@ class OneConvergencePluginV2(db_base_plugin_v2.NeutronDbPluginV2,
 
             self.nvsdlib.delete_port(port_id, neutron_port)
 
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
         self.notify_security_groups_member_updated(context, neutron_port)
 
     def create_floatingip(self, context, floatingip):

diff --git a/neutron/plugins/openvswitch/ovs_neutron_plugin.py b/neutron/plugins/openvswitch/ovs_neutron_plugin.py
index d5f8990259232407961ce297bc5562608a9dd8c1..005fb308b7ba2c35222eb50a064d1d75ae749c3a 100644 (file)
--- a/neutron/plugins/openvswitch/ovs_neutron_plugin.py
+++ b/neutron/plugins/openvswitch/ovs_neutron_plugin.py
@@ -626,9 +626,12 @@ class OVSNeutronPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
 
         session = context.session
         with session.begin(subtransactions=True):
-            self.disassociate_floatingips(context, id)
+            router_ids = self.disassociate_floatingips(
+                context, id, do_notify=False)
             port = self.get_port(context, id)
             self._delete_port_security_group_bindings(context, id)
             super(OVSNeutronPluginV2, self).delete_port(context, id)
 
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
         self.notify_security_groups_member_updated(context, port)

diff --git a/neutron/plugins/plumgrid/plumgrid_plugin/plumgrid_plugin.py b/neutron/plugins/plumgrid/plumgrid_plugin/plumgrid_plugin.py
index 4fb61bf7c066657225ff7d7cb832b6b853dbe629..32177fdfc0f5bb4bfd22a2e6ed7feb8d74b37294 100644 (file)
--- a/neutron/plugins/plumgrid/plumgrid_plugin/plumgrid_plugin.py
+++ b/neutron/plugins/plumgrid/plumgrid_plugin/plumgrid_plugin.py
@@ -245,7 +245,8 @@ class NeutronPluginPLUMgridV2(db_base_plugin_v2.NeutronDbPluginV2,
             # Plugin DB - Port Create and Return port
             port_db = super(NeutronPluginPLUMgridV2,
                             self).get_port(context, port_id)
-            self.disassociate_floatingips(context, port_id)
+            router_ids = self.disassociate_floatingips(
+                context, port_id, do_notify=False)
             super(NeutronPluginPLUMgridV2, self).delete_port(context, port_id)
 
             if port_db["device_owner"] == constants.DEVICE_OWNER_ROUTER_GW:
@@ -260,6 +261,9 @@ class NeutronPluginPLUMgridV2(db_base_plugin_v2.NeutronDbPluginV2,
             except Exception as err_message:
                 raise plum_excep.PLUMgridException(err_msg=err_message)
 
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
+
     def get_port(self, context, id, fields=None):
         with context.session.begin(subtransactions=True):
             port_db = super(NeutronPluginPLUMgridV2,
@@ -531,7 +535,7 @@ class NeutronPluginPLUMgridV2(db_base_plugin_v2.NeutronDbPluginV2,
             except Exception as err_message:
                 raise plum_excep.PLUMgridException(err_msg=err_message)
 
-    def disassociate_floatingips(self, context, port_id):
+    def disassociate_floatingips(self, context, port_id, do_notify=True):
         LOG.debug(_("Neutron PLUMgrid Director: disassociate_floatingips() "
                     "called"))
 
@@ -549,8 +553,9 @@ class NeutronPluginPLUMgridV2(db_base_plugin_v2.NeutronDbPluginV2,
         except Exception as err_message:
             raise plum_excep.PLUMgridException(err_msg=err_message)
 
-        super(NeutronPluginPLUMgridV2,
-              self).disassociate_floatingips(context, port_id)
+        return super(NeutronPluginPLUMgridV2,
+                     self).disassociate_floatingips(
+                             context, port_id, do_notify=do_notify)
 
     """
     Internal PLUMgrid Fuctions

diff --git a/neutron/plugins/ryu/ryu_neutron_plugin.py b/neutron/plugins/ryu/ryu_neutron_plugin.py
index 34ace9d7d02f60a35d14d920a2783381e752458e..dfcec0a453ae8a9df626247f3f1a65724c31e6de 100644 (file)
--- a/neutron/plugins/ryu/ryu_neutron_plugin.py
+++ b/neutron/plugins/ryu/ryu_neutron_plugin.py
@@ -231,11 +231,14 @@ class RyuNeutronPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
             self.prevent_l3_port_deletion(context, id)
 
         with context.session.begin(subtransactions=True):
-            self.disassociate_floatingips(context, id)
+            router_ids = self.disassociate_floatingips(
+                context, id, do_notify=False)
             port = self.get_port(context, id)
             self._delete_port_security_group_bindings(context, id)
             super(RyuNeutronPluginV2, self).delete_port(context, id)
 
+        # now that we've left db transaction, we are safe to notify
+        self.notify_routers_updated(context, router_ids)
         self.notify_security_groups_member_updated(context, port)
 
     def update_port(self, context, id, port):

diff --git a/neutron/tests/unit/ml2/test_ml2_plugin.py b/neutron/tests/unit/ml2/test_ml2_plugin.py
index 9bab5382c8b3c5899b4bf7b9a1ece2e274ab82e0..05a212dcae64db9ccbbd08812b02a81873676fbb 100644 (file)
--- a/neutron/tests/unit/ml2/test_ml2_plugin.py
+++ b/neutron/tests/unit/ml2/test_ml2_plugin.py
@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import contextlib
 import mock
 import testtools
 import uuid
@@ -24,6 +25,7 @@ from neutron.extensions import multiprovidernet as mpnet
 from neutron.extensions import portbindings
 from neutron.extensions import providernet as pnet
 from neutron import manager
+from neutron.plugins.common import constants as service_constants
 from neutron.plugins.ml2.common import exceptions as ml2_exc
 from neutron.plugins.ml2 import config
 from neutron.plugins.ml2 import driver_api
@@ -133,6 +135,42 @@ class TestMl2PortsV2(test_plugin.TestPortsV2, Ml2PluginV2TestCase):
                 mock.call(_("The port '%s' was deleted"), 'invalid-uuid')
             ])
 
+    def test_delete_port_no_notify_in_disassociate_floatingips(self):
+        ctx = context.get_admin_context()
+        plugin = manager.NeutronManager.get_plugin()
+        l3plugin = manager.NeutronManager.get_service_plugins().get(
+            service_constants.L3_ROUTER_NAT)
+        with contextlib.nested(
+            self.port(no_delete=True),
+            mock.patch.object(l3plugin, 'disassociate_floatingips'),
+            mock.patch.object(l3plugin, 'notify_routers_updated')
+        ) as (port, disassociate_floatingips, notify):
+
+            port_id = port['port']['id']
+            plugin.delete_port(ctx, port_id)
+
+            # check that no notification was requested while under
+            # transaction
+            disassociate_floatingips.assert_has_calls([
+                mock.call(ctx, port_id, do_notify=False)
+            ])
+
+            # check that notifier was still triggered
+            notify.assert_has_calls([
+                mock.call(ctx, disassociate_floatingips.return_value)
+            ])
+
+    def test_disassociate_floatingips_do_notify_returns_nothing(self):
+        ctx = context.get_admin_context()
+        l3plugin = manager.NeutronManager.get_service_plugins().get(
+            service_constants.L3_ROUTER_NAT)
+        with self.port() as port:
+
+            port_id = port['port']['id']
+            # check that nothing is returned when notifications are handled
+            # by the called method
+            self.assertIsNone(l3plugin.disassociate_floatingips(ctx, port_id))
+
 
 class TestMl2PortBinding(Ml2PluginV2TestCase,
                          test_bindings.PortBindingsTestCase):