From 1f5ee0e8942e4b77a89a00ee0249de5d5014e2bc Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Fri, 20 Jan 2017 21:23:05 -0800 Subject: [PATCH] Get rid of ML2 inheritance of delete_network In an effort to simplify the codebase, this eliminates all of the custom code in the ML2 delete_network method and leverages registry callbacks for network events. The only purpose of the remaining inheritance of the method is to leave the transaction guard on. Partial-Bug: #1666424 Change-Id: Ib7d1c2081654dda82b5e015c71d20f378f3308f7 --- neutron/db/db_base_plugin_v2.py | 47 ++++--- neutron/db/external_net_db.py | 5 + neutron/plugins/ml2/plugin.py | 130 +++--------------- .../tests/unit/db/test_db_base_plugin_v2.py | 16 +++ neutron/tests/unit/extensions/test_l3.py | 5 - neutron/tests/unit/plugins/ml2/test_plugin.py | 21 --- 6 files changed, 70 insertions(+), 154 deletions(-) diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 34f6b76da70..4f589636270 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -395,29 +395,38 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon, ndb_utils.filter_non_model_columns(n, models_v2.Network)) return self._make_network_dict(network, context=context) + def _ensure_network_not_in_use(self, context, net_id): + non_auto_ports = context.session.query( + models_v2.Port.id).filter_by(network_id=net_id).filter( + ~models_v2.Port.device_owner.in_(AUTO_DELETE_PORT_OWNERS)) + if non_auto_ports.count(): + raise exc.NetworkInUse(net_id=net_id) + @db_api.retry_if_session_inactive() def delete_network(self, context, id): - with context.session.begin(subtransactions=True): - network = self._get_network(context, id) - - auto_delete_ports = context.session.query( - models_v2.Port).filter_by(network_id=id).filter( - models_v2.Port.device_owner.in_(AUTO_DELETE_PORT_OWNERS)) - for port in auto_delete_ports: - context.session.delete(port) - - port_in_use = context.session.query(models_v2.Port).filter_by( - network_id=id).first() - - if port_in_use: - raise exc.NetworkInUse(net_id=id) - - # clean up subnets - subnets = self._get_subnets_by_network(context, id) + registry.notify(resources.NETWORK, events.BEFORE_DELETE, self, + context=context, network_id=id) + self._ensure_network_not_in_use(context, id) + auto_delete_port_ids = [p.id for p in context.session.query( + models_v2.Port.id).filter_by(network_id=id).filter( + models_v2.Port.device_owner.in_(AUTO_DELETE_PORT_OWNERS))] + for port_id in auto_delete_port_ids: + self.delete_port(context.elevated(), port_id) + # clean up subnets + subnets = self._get_subnets_by_network(context, id) + with db_api.exc_to_retry(os_db_exc.DBReferenceError): + # retry reference errors so we can check the port type and + # cleanup if a network-owned port snuck in without failing for subnet in subnets: self.delete_subnet(context, subnet['id']) - - context.session.delete(network) + with context.session.begin(subtransactions=True): + network_db = self._get_network(context, id) + network = self._make_network_dict(network_db, context=context) + registry.notify(resources.NETWORK, events.PRECOMMIT_DELETE, + self, context=context, network_id=id) + context.session.delete(network_db) + registry.notify(resources.NETWORK, events.AFTER_DELETE, + self, context=context, network=network) @db_api.retry_if_session_inactive() def get_network(self, context, id, fields=None): diff --git a/neutron/db/external_net_db.py b/neutron/db/external_net_db.py index 196a241c68f..49f67aa0f31 100644 --- a/neutron/db/external_net_db.py +++ b/neutron/db/external_net_db.py @@ -258,3 +258,8 @@ class External_net_db_mixin(object): "depend on this policy for access.") raise rbac_ext.RbacPolicyInUse(object_id=policy['object_id'], details=msg) + + @registry.receives(resources.NETWORK, [events.BEFORE_DELETE]) + def _before_network_delete_handler(self, resource, event, trigger, + context, network_id, **kwargs): + self._process_l3_delete(context, network_id) diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 4b2cbee83fe..10ec729328f 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -863,126 +863,38 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, return [db_utils.resource_fields(net, fields) for net in nets] - def _delete_ports(self, context, port_ids): - for port_id in port_ids: - try: - self.delete_port(context, port_id) - except (exc.PortNotFound, sa_exc.ObjectDeletedError): - # concurrent port deletion can be performed by - # release_dhcp_port caused by concurrent subnet_delete - LOG.info(_LI("Port %s was deleted concurrently"), port_id) - except Exception as e: - with excutils.save_and_reraise_exception(): - utils.attach_exc_details( - e, - _LE("Exception auto-deleting port %s"), port_id) - - def _delete_subnets(self, context, subnet_ids): - for subnet_id in subnet_ids: - try: - self.delete_subnet(context, subnet_id) - except (exc.SubnetNotFound, sa_exc.ObjectDeletedError): - LOG.info(_LI("Subnet %s was deleted concurrently"), - subnet_id) - except Exception as e: - with excutils.save_and_reraise_exception(): - utils.attach_exc_details( - e, - _LE("Exception auto-deleting subnet %s"), subnet_id) - @utils.transaction_guard - @db_api.retry_if_session_inactive() def delete_network(self, context, id): - # REVISIT(rkukura) The super(Ml2Plugin, self).delete_network() - # function is not used because it auto-deletes ports and - # subnets from the DB without invoking the derived class's - # delete_port() or delete_subnet(), preventing mechanism - # drivers from being called. This approach should be revisited - # when the API layer is reworked during icehouse. + # the only purpose of this override is to protect this from being + # called inside of a transaction. + return super(Ml2Plugin, self).delete_network(context, id) - LOG.debug("Deleting network %s", id) - session = context.session - while True: - # NOTE(kevinbenton): this loop keeps db objects in scope - # so we must expire them or risk stale reads. - # see bug/1623990 - session.expire_all() - try: - # REVISIT: Serialize this operation with a semaphore - # to prevent deadlock waiting to acquire a DB lock - # held by another thread in the same process, leading - # to 'lock wait timeout' errors. - # - # Process L3 first, since, depending on the L3 plugin, it may - # involve sending RPC notifications, and/or calling delete_port - # on this plugin. - # Additionally, a rollback may not be enough to undo the - # deletion of a floating IP with certain L3 backends. - self._process_l3_delete(context, id) - # Using query().with_lockmode isn't necessary. Foreign-key - # constraints prevent deletion if concurrent creation happens. - with session.begin(subtransactions=True): - # Get ports to auto-delete. - ports = (session.query(models_v2.Port). - enable_eagerloads(False). - filter_by(network_id=id).all()) - LOG.debug("Ports to auto-delete: %s", ports) - only_auto_del = all(p.device_owner - in db_base_plugin_v2. - AUTO_DELETE_PORT_OWNERS - for p in ports) - if not only_auto_del: - LOG.debug("Tenant-owned ports exist") - raise exc.NetworkInUse(net_id=id) + @registry.receives(resources.NETWORK, [events.PRECOMMIT_DELETE]) + def _network_delete_precommit_handler(self, rtype, event, trigger, + context, network_id, **kwargs): + network = self.get_network(context, network_id) + mech_context = driver_context.NetworkContext(self, + context, + network) + # TODO(kevinbenton): move this mech context into something like + # a 'delete context' so it's not polluting the real context object + setattr(context, '_mech_context', mech_context) + self.mechanism_manager.delete_network_precommit( + mech_context) - # Get subnets to auto-delete. - subnets = (session.query(models_v2.Subnet). - enable_eagerloads(False). - filter_by(network_id=id).all()) - LOG.debug("Subnets to auto-delete: %s", subnets) - - if not (ports or subnets): - network = self.get_network(context, id) - mech_context = driver_context.NetworkContext(self, - context, - network) - self.mechanism_manager.delete_network_precommit( - mech_context) - - registry.notify(resources.NETWORK, - events.PRECOMMIT_DELETE, - self, - context=context, - network_id=id) - record = self._get_network(context, id) - LOG.debug("Deleting network record %s", record) - session.delete(record) - - # The segment records are deleted via cascade from the - # network record, so explicit removal is not necessary. - LOG.debug("Committing transaction") - break - - port_ids = [port.id for port in ports] - subnet_ids = [subnet.id for subnet in subnets] - except os_db_exception.DBDuplicateEntry: - LOG.warning(_LW("A concurrent port creation has " - "occurred")) - continue - self._delete_ports(context, port_ids) - self._delete_subnets(context, subnet_ids) - - kwargs = {'context': context, 'network': network} - registry.notify(resources.NETWORK, events.AFTER_DELETE, self, **kwargs) + @registry.receives(resources.NETWORK, [events.AFTER_DELETE]) + def _network_delete_after_delete_handler(self, rtype, event, trigger, + context, network, **kwargs): try: - self.mechanism_manager.delete_network_postcommit(mech_context) + self.mechanism_manager.delete_network_postcommit( + context._mech_context) except ml2_exc.MechanismDriverError: # TODO(apech) - One or more mechanism driver failed to # delete the network. Ideally we'd notify the caller of # the fact that an error occurred. LOG.error(_LE("mechanism_manager.delete_network_postcommit" " failed")) - self.notifier.network_delete(context, id) + self.notifier.network_delete(context, network['id']) def _create_subnet_db(self, context, subnet): session = context.session diff --git a/neutron/tests/unit/db/test_db_base_plugin_v2.py b/neutron/tests/unit/db/test_db_base_plugin_v2.py index 255b0246f7c..bc70083f55b 100644 --- a/neutron/tests/unit/db/test_db_base_plugin_v2.py +++ b/neutron/tests/unit/db/test_db_base_plugin_v2.py @@ -1522,6 +1522,22 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s res = req.get_response(self.api) self.assertEqual(webob.exc.HTTPNoContent.code, res.status_int) + def test_delete_network_port_exists_owned_by_network_race(self): + res = self._create_network(fmt=self.fmt, name='net', + admin_state_up=True) + network = self.deserialize(self.fmt, res) + network_id = network['network']['id'] + self._create_port(self.fmt, network_id, + device_owner=constants.DEVICE_OWNER_DHCP) + # skip first port delete to simulate create after auto clean + plugin = directory.get_plugin() + p = mock.patch.object(plugin, 'delete_port') + mock_del_port = p.start() + mock_del_port.side_effect = lambda *a, **k: p.stop() + req = self.new_delete_request('networks', network_id) + res = req.get_response(self.api) + self.assertEqual(webob.exc.HTTPNoContent.code, res.status_int) + def test_update_port_delete_ip(self): with self.subnet() as subnet: with self.port(subnet=subnet) as port: diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index e16fe4a026c..bdf1adf1087 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -256,11 +256,6 @@ class TestL3NatBasePlugin(db_base_plugin_v2.NeutronDbPluginV2, self._process_l3_update(context, net, network['network']) return net - def delete_network(self, context, id): - with context.session.begin(subtransactions=True): - self._process_l3_delete(context, id) - super(TestL3NatBasePlugin, self).delete_network(context, id) - def delete_port(self, context, id, l3_port_check=True): plugin = directory.get_plugin(lib_constants.L3) if plugin: diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index 50d4d62dfeb..bb3a026ce64 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -26,7 +26,6 @@ from neutron_lib import exceptions as exc from neutron_lib.plugins import directory from oslo_db import exception as db_exc from oslo_utils import uuidutils -from sqlalchemy.orm import exc as sqla_exc from neutron._i18n import _ from neutron.callbacks import events @@ -243,26 +242,6 @@ class TestMl2NetworksV2(test_plugin.TestNetworksV2, self.assertEqual(n['network']['id'], kwargs['network']['id']) - def test_port_delete_helper_tolerates_failure(self): - plugin = directory.get_plugin() - with mock.patch.object(plugin, "delete_port", - side_effect=exc.PortNotFound(port_id="123")): - plugin._delete_ports(mock.MagicMock(), [mock.MagicMock()]) - - with mock.patch.object(plugin, "delete_port", - side_effect=sqla_exc.ObjectDeletedError(None)): - plugin._delete_ports(mock.MagicMock(), [mock.MagicMock()]) - - def test_subnet_delete_helper_tolerates_failure(self): - plugin = directory.get_plugin() - with mock.patch.object(plugin, "delete_subnet", - side_effect=exc.SubnetNotFound(subnet_id="1")): - plugin._delete_subnets(mock.MagicMock(), [mock.MagicMock()]) - - with mock.patch.object(plugin, "delete_subnet", - side_effect=sqla_exc.ObjectDeletedError(None)): - plugin._delete_subnets(mock.MagicMock(), [mock.MagicMock()]) - def _create_and_verify_networks(self, networks): for net_idx, net in enumerate(networks): # create