From be101776c64c735b2cdd73dc0dd7a8653170cbc3 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Wed, 18 Jun 2014 16:56:25 +0200 Subject: [PATCH] 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 --- neutron/db/l3_db.py | 17 ++++++++- neutron/plugins/bigswitch/plugin.py | 13 +++++-- .../plugins/cisco/n1kv/n1kv_neutron_plugin.py | 7 +++- neutron/plugins/embrane/base_plugin.py | 6 ++- .../plugins/linuxbridge/lb_neutron_plugin.py | 5 ++- neutron/plugins/ml2/plugin.py | 7 +++- neutron/plugins/mlnx/mlnx_plugin.py | 5 ++- neutron/plugins/nec/nec_plugin.py | 6 ++- neutron/plugins/nuage/plugin.py | 21 +++++++--- neutron/plugins/oneconvergence/plugin.py | 5 ++- .../plugins/openvswitch/ovs_neutron_plugin.py | 5 ++- .../plumgrid_plugin/plumgrid_plugin.py | 13 +++++-- neutron/plugins/ryu/ryu_neutron_plugin.py | 5 ++- neutron/tests/unit/ml2/test_ml2_plugin.py | 38 +++++++++++++++++++ 14 files changed, 128 insertions(+), 25 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index cad9e48153..f2bdfdda57 100644 --- 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 306016a8fe..b2d11fe8bf 100644 --- 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 431cbc6d8b..6e259ccc17 100644 --- 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 3434a96d41..8dedc653ae 100644 --- 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 89b1354a42..6ddbba680d 100644 --- 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 4a46bf5482..9407b31ca9 100644 --- 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 79af922504..4ce3d513bd 100644 --- 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 fbeebd7a1a..45733dc9e1 100644 --- 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 bf95c1eecf..c5257ae353 100644 --- 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 1456007bdf..a07005c190 100644 --- 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 d5f8990259..005fb308b7 100644 --- 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 4fb61bf7c0..32177fdfc0 100644 --- 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 34ace9d7d0..dfcec0a453 100644 --- 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 9bab5382c8..05a212dcae 100644 --- 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):