From 5e4b0c6fc6670ea036d801ce53444272bc311929 Mon Sep 17 00:00:00 2001 From: Akihiro Motoki Date: Fri, 7 Mar 2014 15:58:46 +0900 Subject: [PATCH] Avoid long transaction in plugin.delete_ports() db_plugin.delete_ports() called plugin.delete_port() under a transaction. It leads to long transaction if plugin.delete_port talks with external systems. This commit changes each delete_port outside of a transaction to avoid longer transaction. plugin.delete_ports is now called by release_dhcp_ports and dhcp-agent ports can be deleted separately, so this changes does not break the existing behavior. delete_ports is renamed to delete_ports_by_device_id to clarify the usage of this method. NEC plugin already has this change and it is no longer needed. _do_side_effect helper method in test_db_plugin is renamed to more self-descriptive name. Change-Id: Ied5883a57c7774c3b0778453d84c717b337f88c0 Closes-Bug: #1282925 Related-Bug: #1283522 --- neutron/db/db_base_plugin_v2.py | 21 +++-- neutron/db/dhcp_rpc_base.py | 3 +- neutron/plugins/nec/nec_plugin.py | 13 ---- .../tests/unit/cisco/test_network_plugin.py | 4 + neutron/tests/unit/nec/test_nec_plugin.py | 25 ------ neutron/tests/unit/test_db_plugin.py | 76 +++++++++++++++---- neutron/tests/unit/test_db_rpc_base.py | 9 +-- 7 files changed, 78 insertions(+), 73 deletions(-) diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 13b800974..b7ad1ecff 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -1349,18 +1349,15 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, with context.session.begin(subtransactions=True): self._delete_port(context, id) - def delete_ports(self, context, filters): - with context.session.begin(subtransactions=True): - # Disable eagerloads to avoid postgresql issues with outer joins - # and SELECT FOR UPDATE. This means that only filters for columns - # on the Port model will be effective, which is fine in nearly all - # the cases where filters are used - query = context.session.query( - models_v2.Port).enable_eagerloads(False) - ports = self._apply_filters_to_query( - query, models_v2.Port, filters).with_lockmode('update').all() - for port in ports: - self.delete_port(context, port['id']) + def delete_ports_by_device_id(self, context, device_id, network_id=None): + query = (context.session.query(models_v2.Port.id) + .enable_eagerloads(False) + .filter(models_v2.Port.device_id == device_id)) + if network_id: + query = query.filter(models_v2.Port.network_id == network_id) + port_ids = [p[0] for p in query] + for port_id in port_ids: + self.delete_port(context, port_id) def _delete_port(self, context, id): query = (context.session.query(models_v2.Port). diff --git a/neutron/db/dhcp_rpc_base.py b/neutron/db/dhcp_rpc_base.py index 2fce470c0..953512d83 100644 --- a/neutron/db/dhcp_rpc_base.py +++ b/neutron/db/dhcp_rpc_base.py @@ -216,8 +216,7 @@ class DhcpRpcCallbackMixin(object): '%(host)s'), {'network_id': network_id, 'host': host}) plugin = manager.NeutronManager.get_plugin() - filters = dict(network_id=[network_id], device_id=[device_id]) - plugin.delete_ports(context, filters=filters) + plugin.delete_ports_by_device_id(context, device_id, network_id) def release_port_fixed_ip(self, context, **kwargs): """Release the fixed_ip associated the subnet on a port.""" diff --git a/neutron/plugins/nec/nec_plugin.py b/neutron/plugins/nec/nec_plugin.py index 917f1e106..f19781fbb 100644 --- a/neutron/plugins/nec/nec_plugin.py +++ b/neutron/plugins/nec/nec_plugin.py @@ -31,7 +31,6 @@ from neutron.db import db_base_plugin_v2 from neutron.db import dhcp_rpc_base from neutron.db import external_net_db from neutron.db import l3_rpc_base -from neutron.db import models_v2 from neutron.db import portbindings_base from neutron.db import portbindings_db from neutron.db import quota_db # noqa @@ -646,18 +645,6 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, super(NECPluginV2, self).delete_port(context, id) self.notify_security_groups_member_updated(context, port) - def delete_ports(self, context, filters): - # Note(amotoki): Override the superclass method to avoid - # a long transaction over external API calls. - # TODO(amotoki): Need to revisit after bug 1282925 is addressed. - query = context.session.query( - models_v2.Port).enable_eagerloads(False) - query = self._apply_filters_to_query( - query, models_v2.Port, filters) - port_ids = [p['id'] for p in query] - for port_id in port_ids: - self.delete_port(context, port_id) - class NECPluginV2AgentNotifierApi(proxy.RpcProxy, sg_rpc.SecurityGroupAgentRpcApiMixin): diff --git a/neutron/tests/unit/cisco/test_network_plugin.py b/neutron/tests/unit/cisco/test_network_plugin.py index 23f4f41c2..fd3fac031 100644 --- a/neutron/tests/unit/cisco/test_network_plugin.py +++ b/neutron/tests/unit/cisco/test_network_plugin.py @@ -859,6 +859,10 @@ class TestCiscoPortsV2(CiscoNetworkPluginV2TestCase, data['device_id'], NEXUS_PORT_2) + def test_delete_ports_by_device_id_second_call_failure(self): + plugin_ref = self._get_plugin_ref() + self._test_delete_ports_by_device_id_second_call_failure(plugin_ref) + class TestCiscoNetworksV2(CiscoNetworkPluginV2TestCase, test_db_plugin.TestNetworksV2): diff --git a/neutron/tests/unit/nec/test_nec_plugin.py b/neutron/tests/unit/nec/test_nec_plugin.py index ca9a2ec59..182e98abd 100644 --- a/neutron/tests/unit/nec/test_nec_plugin.py +++ b/neutron/tests/unit/nec/test_nec_plugin.py @@ -13,7 +13,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import contextlib import os import fixtures @@ -116,30 +115,6 @@ class TestNecV2HTTPResponse(test_plugin.TestV2HTTPResponse, pass -class TestNecPortsV2(test_plugin.TestPortsV2, NecPluginV2TestCase): - - def test_delete_ports(self): - with self.subnet() as subnet: - with contextlib.nested( - self.port(subnet=subnet, device_owner='test-owner', - no_delete=True), - self.port(subnet=subnet, device_owner='test-owner', - no_delete=True), - self.port(subnet=subnet, device_owner='other-owner'), - ) as (p1, p2, p3): - network_id = subnet['subnet']['network_id'] - filters = {'network_id': [network_id], - 'device_owner': ['test-owner']} - self.plugin.delete_ports(self.context, filters) - - self._show('ports', p1['port']['id'], - expected_code=webob.exc.HTTPNotFound.code) - self._show('ports', p2['port']['id'], - expected_code=webob.exc.HTTPNotFound.code) - self._show('ports', p3['port']['id'], - expected_code=webob.exc.HTTPOk.code) - - class TestNecNetworksV2(test_plugin.TestNetworksV2, NecPluginV2TestCase): pass diff --git a/neutron/tests/unit/test_db_plugin.py b/neutron/tests/unit/test_db_plugin.py index 56b20d29f..c7e780ac0 100644 --- a/neutron/tests/unit/test_db_plugin.py +++ b/neutron/tests/unit/test_db_plugin.py @@ -479,7 +479,7 @@ class NeutronDbPluginV2TestCase(testlib_api.WebTestCase): self.assertEqual(res.status_int, webob.exc.HTTPOk.code) return self.deserialize(fmt, res) - def _do_side_effect(self, patched_plugin, orig, *args, **kwargs): + def _fail_second_call(self, patched_plugin, orig, *args, **kwargs): """Invoked by test cases for injecting failures in plugin.""" def second_call(*args, **kwargs): raise n_exc.NeutronException() @@ -901,8 +901,8 @@ class TestPortsV2(NeutronDbPluginV2TestCase): 'create_port') as patched_plugin: def side_effect(*args, **kwargs): - return self._do_side_effect(patched_plugin, orig, - *args, **kwargs) + return self._fail_second_call(patched_plugin, orig, + *args, **kwargs) patched_plugin.side_effect = side_effect with self.network() as net: @@ -925,8 +925,8 @@ class TestPortsV2(NeutronDbPluginV2TestCase): 'create_port') as patched_plugin: def side_effect(*args, **kwargs): - return self._do_side_effect(patched_plugin, orig, - *args, **kwargs) + return self._fail_second_call(patched_plugin, orig, + *args, **kwargs) patched_plugin.side_effect = side_effect res = self._create_port_bulk(self.fmt, 2, net['network']['id'], @@ -1655,6 +1655,56 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s self.assertEqual(res.status_int, webob.exc.HTTPClientError.code) + def test_delete_ports_by_device_id(self): + plugin = NeutronManager.get_plugin() + ctx = context.get_admin_context() + with self.subnet() as subnet: + with contextlib.nested( + self.port(subnet=subnet, device_id='owner1', no_delete=True), + self.port(subnet=subnet, device_id='owner1', no_delete=True), + self.port(subnet=subnet, device_id='owner2'), + ) as (p1, p2, p3): + network_id = subnet['subnet']['network_id'] + plugin.delete_ports_by_device_id(ctx, 'owner1', + network_id) + self._show('ports', p1['port']['id'], + expected_code=webob.exc.HTTPNotFound.code) + self._show('ports', p2['port']['id'], + expected_code=webob.exc.HTTPNotFound.code) + self._show('ports', p3['port']['id'], + expected_code=webob.exc.HTTPOk.code) + + def _test_delete_ports_by_device_id_second_call_failure(self, plugin): + ctx = context.get_admin_context() + with self.subnet() as subnet: + with contextlib.nested( + self.port(subnet=subnet, device_id='owner1', no_delete=True), + self.port(subnet=subnet, device_id='owner1'), + self.port(subnet=subnet, device_id='owner2'), + ) as (p1, p2, p3): + orig = plugin.delete_port + with mock.patch.object(plugin, 'delete_port') as del_port: + + def side_effect(*args, **kwargs): + return self._fail_second_call(del_port, orig, + *args, **kwargs) + + del_port.side_effect = side_effect + network_id = subnet['subnet']['network_id'] + self.assertRaises(n_exc.NeutronException, + plugin.delete_ports_by_device_id, + ctx, 'owner1', network_id) + self._show('ports', p1['port']['id'], + expected_code=webob.exc.HTTPNotFound.code) + self._show('ports', p2['port']['id'], + expected_code=webob.exc.HTTPOk.code) + self._show('ports', p3['port']['id'], + expected_code=webob.exc.HTTPOk.code) + + def test_delete_ports_by_device_id_second_call_failure(self): + plugin = NeutronManager.get_plugin() + self._test_delete_ports_by_device_id_second_call_failure(plugin) + class TestNetworksV2(NeutronDbPluginV2TestCase): # NOTE(cerberus): successful network update and delete are @@ -1915,8 +1965,8 @@ class TestNetworksV2(NeutronDbPluginV2TestCase): 'create_network') as patched_plugin: def side_effect(*args, **kwargs): - return self._do_side_effect(patched_plugin, orig, - *args, **kwargs) + return self._fail_second_call(patched_plugin, orig, + *args, **kwargs) patched_plugin.side_effect = side_effect res = self._create_network_bulk(self.fmt, 2, 'test', True) @@ -1933,8 +1983,8 @@ class TestNetworksV2(NeutronDbPluginV2TestCase): 'create_network') as patched_plugin: def side_effect(*args, **kwargs): - return self._do_side_effect(patched_plugin, orig, - *args, **kwargs) + return self._fail_second_call(patched_plugin, orig, + *args, **kwargs) patched_plugin.side_effect = side_effect res = self._create_network_bulk(self.fmt, 2, 'test', True) @@ -2343,8 +2393,8 @@ class TestSubnetsV2(NeutronDbPluginV2TestCase): 'create_subnet') as patched_plugin: def side_effect(*args, **kwargs): - self._do_side_effect(patched_plugin, orig, - *args, **kwargs) + self._fail_second_call(patched_plugin, orig, + *args, **kwargs) patched_plugin.side_effect = side_effect with self.network() as net: @@ -2363,8 +2413,8 @@ class TestSubnetsV2(NeutronDbPluginV2TestCase): with mock.patch.object(NeutronManager._instance.plugin, 'create_subnet') as patched_plugin: def side_effect(*args, **kwargs): - return self._do_side_effect(patched_plugin, orig, - *args, **kwargs) + return self._fail_second_call(patched_plugin, orig, + *args, **kwargs) patched_plugin.side_effect = side_effect with self.network() as net: diff --git a/neutron/tests/unit/test_db_rpc_base.py b/neutron/tests/unit/test_db_rpc_base.py index 7de3ece8a..3ba662313 100644 --- a/neutron/tests/unit/test_db_rpc_base.py +++ b/neutron/tests/unit/test_db_rpc_base.py @@ -34,11 +34,6 @@ class TestDhcpRpcCallbackMixin(base.BaseTestCase): self.log_p = mock.patch('neutron.db.dhcp_rpc_base.LOG') self.log = self.log_p.start() - def tearDown(self): - self.log_p.stop() - self.plugin_p.stop() - super(TestDhcpRpcCallbackMixin, self).tearDown() - def test_get_active_networks(self): plugin_retval = [dict(id='a'), dict(id='b')] self.plugin.get_networks.return_value = plugin_retval @@ -221,9 +216,7 @@ class TestDhcpRpcCallbackMixin(base.BaseTestCase): device_id='devid') self.plugin.assert_has_calls([ - mock.call.delete_ports(mock.ANY, - filters=dict(network_id=['netid'], - device_id=['devid']))]) + mock.call.delete_ports_by_device_id(mock.ANY, 'devid', 'netid')]) def test_release_port_fixed_ip(self): port_retval = dict(id='port_id', fixed_ips=[dict(subnet_id='a')])