From 9c94d960310811b428abd0a48f37cf35a2e96940 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. Conflicts: neutron/db/l3_db.py neutron/plugins/bigswitch/plugin.py neutron/plugins/nuage/plugin.py Change-Id: I2411f2aa778ea088be416d062c4816c16f49d2bf Closes-Bug: 1330955 (cherry picked from commit 876c2c25e15806529f4e197cd265bc3b2184aa14) --- neutron/db/l3_db.py | 19 ++++++++-- 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/oneconvergence/plugin.py | 5 ++- .../plugins/openvswitch/ovs_neutron_plugin.py | 5 ++- .../plumgrid_plugin/plumgrid_plugin.py | 12 ++++-- neutron/plugins/ryu/ryu_neutron_plugin.py | 5 ++- neutron/tests/unit/ml2/test_ml2_plugin.py | 38 +++++++++++++++++++ 13 files changed, 111 insertions(+), 22 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index c8f26b5b2d2..7d47a29b4af 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -747,12 +747,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): + router_ids = [] + with context.session.begin(subtransactions=True): try: fip_qry = context.session.query(FloatingIP) floating_ip = fip_qry.filter_by(fixed_port_id=port_id).one() - router_id = floating_ip['router_id'] + router_ids.append(floating_ip['router_id']) floating_ip.update({'fixed_port_id': None, 'fixed_ip_address': None, 'router_id': None}) @@ -762,9 +764,18 @@ class L3_NAT_db_mixin(l3.RouterPluginBase): # should never happen raise Exception(_('Multiple floating IPs found for port %s') % port_id) - if router_id: + 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, [router_id]) + context, router_ids) def _build_routers_list(self, routers, gw_ports): gw_port_id_gw_port_dict = dict((gw_port['id'], gw_port) diff --git a/neutron/plugins/bigswitch/plugin.py b/neutron/plugins/bigswitch/plugin.py index 15a24cbbba0..5e0fd743219 100644 --- a/neutron/plugins/bigswitch/plugin.py +++ b/neutron/plugins/bigswitch/plugin.py @@ -817,7 +817,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 @@ -825,6 +826,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")) @@ -1095,11 +1099,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 def _send_floatingip_update(self, context): try: diff --git a/neutron/plugins/cisco/n1kv/n1kv_neutron_plugin.py b/neutron/plugins/cisco/n1kv/n1kv_neutron_plugin.py index 02c492f7726..a4e2961d382 100644 --- a/neutron/plugins/cisco/n1kv/n1kv_neutron_plugin.py +++ b/neutron/plugins/cisco/n1kv/n1kv_neutron_plugin.py @@ -1229,8 +1229,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 33d21388868..024797ff9e2 100644 --- a/neutron/plugins/embrane/base_plugin.py +++ b/neutron/plugins/embrane/base_plugin.py @@ -353,14 +353,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"] @@ -373,3 +374,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 52fbdcaddd3..8100917adef 100644 --- a/neutron/plugins/linuxbridge/lb_neutron_plugin.py +++ b/neutron/plugins/linuxbridge/lb_neutron_plugin.py @@ -524,11 +524,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 a72c5128aba..a127407ebf3 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -736,10 +736,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 920038f5bc9..280541a2b79 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 8b9f5b3d576..e4a7c6ffcb4 100644 --- a/neutron/plugins/nec/nec_plugin.py +++ b/neutron/plugins/nec/nec_plugin.py @@ -652,9 +652,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/oneconvergence/plugin.py b/neutron/plugins/oneconvergence/plugin.py index e4aea6f90a4..49bda6fe262 100644 --- a/neutron/plugins/oneconvergence/plugin.py +++ b/neutron/plugins/oneconvergence/plugin.py @@ -356,7 +356,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) @@ -365,6 +366,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 2cfe2674b60..7f66dfdac0f 100644 --- a/neutron/plugins/openvswitch/ovs_neutron_plugin.py +++ b/neutron/plugins/openvswitch/ovs_neutron_plugin.py @@ -627,9 +627,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 17c05c771bc..42df5c9f4d3 100644 --- a/neutron/plugins/plumgrid/plumgrid_plugin/plumgrid_plugin.py +++ b/neutron/plugins/plumgrid/plumgrid_plugin/plumgrid_plugin.py @@ -242,7 +242,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: @@ -257,6 +258,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, @@ -528,7 +532,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")) @@ -546,8 +550,8 @@ 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 9de98979d27..5f65c139f4b 100644 --- a/neutron/plugins/ryu/ryu_neutron_plugin.py +++ b/neutron/plugins/ryu/ryu_neutron_plugin.py @@ -235,11 +235,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 53bc1d8e5bc..4aab3535365 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 webob @@ -23,6 +24,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 plugin as ml2_plugin @@ -119,6 +121,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):