openstack/neutron
Code Issues Proposed changes

Get rid of ML2 inheritance of delete_network

Browse Source 
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
This commit is contained in:
Kevin Benton 2017-01-20 21:23:05 -08:00
parent bb3931a992
commit 1f5ee0e894
6 changed files with 70 additions and 154 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
neutron/db/db_base_plugin_v2.py
View File

@ -395,29 +395,38 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon,
ndb_utils.filter_non_model_columns(n, models_v2.Network)) ndb_utils.filter_non_model_columns(n, models_v2.Network))
return self._make_network_dict(network, context=context) 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() @db_api.retry_if_session_inactive()
def delete_network(self, context, id): def delete_network(self, context, id):
with context.session.begin(subtransactions=True): registry.notify(resources.NETWORK, events.BEFORE_DELETE, self,
network = self._get_network(context, id) context=context, network_id=id)
self._ensure_network_not_in_use(context, id)
auto_delete_ports = context.session.query( auto_delete_port_ids = [p.id for p in context.session.query(
models_v2.Port).filter_by(network_id=id).filter( models_v2.Port.id).filter_by(network_id=id).filter(
models_v2.Port.device_owner.in_(AUTO_DELETE_PORT_OWNERS)) models_v2.Port.device_owner.in_(AUTO_DELETE_PORT_OWNERS))]
for port in auto_delete_ports: for port_id in auto_delete_port_ids:
context.session.delete(port) self.delete_port(context.elevated(), port_id)
# clean up subnets
port_in_use = context.session.query(models_v2.Port).filter_by( subnets = self._get_subnets_by_network(context, id)
network_id=id).first() with db_api.exc_to_retry(os_db_exc.DBReferenceError):
# retry reference errors so we can check the port type and
if port_in_use: # cleanup if a network-owned port snuck in without failing
raise exc.NetworkInUse(net_id=id)
# clean up subnets
subnets = self._get_subnets_by_network(context, id)
for subnet in subnets: for subnet in subnets:
self.delete_subnet(context, subnet['id']) self.delete_subnet(context, subnet['id'])
with context.session.begin(subtransactions=True):
context.session.delete(network) 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() @db_api.retry_if_session_inactive()
def get_network(self, context, id, fields=None): def get_network(self, context, id, fields=None):

5
neutron/db/external_net_db.py
View File

@ -258,3 +258,8 @@ class External_net_db_mixin(object):
"depend on this policy for access.") "depend on this policy for access.")
raise rbac_ext.RbacPolicyInUse(object_id=policy['object_id'], raise rbac_ext.RbacPolicyInUse(object_id=policy['object_id'],
details=msg) 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)

130
neutron/plugins/ml2/plugin.py
View File

@ -863,126 +863,38 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
return [db_utils.resource_fields(net, fields) for net in nets] 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 @utils.transaction_guard
@db_api.retry_if_session_inactive()
def delete_network(self, context, id): def delete_network(self, context, id):
# REVISIT(rkukura) The super(Ml2Plugin, self).delete_network() # the only purpose of this override is to protect this from being
# function is not used because it auto-deletes ports and # called inside of a transaction.
# subnets from the DB without invoking the derived class's return super(Ml2Plugin, self).delete_network(context, id)
# delete_port() or delete_subnet(), preventing mechanism
# drivers from being called. This approach should be revisited
# when the API layer is reworked during icehouse.
LOG.debug("Deleting network %s", id) @registry.receives(resources.NETWORK, [events.PRECOMMIT_DELETE])
session = context.session def _network_delete_precommit_handler(self, rtype, event, trigger,
while True: context, network_id, **kwargs):
# NOTE(kevinbenton): this loop keeps db objects in scope network = self.get_network(context, network_id)
# so we must expire them or risk stale reads. mech_context = driver_context.NetworkContext(self,
# see bug/1623990 context,
session.expire_all() network)
try: # TODO(kevinbenton): move this mech context into something like
# REVISIT: Serialize this operation with a semaphore # a 'delete context' so it's not polluting the real context object
# to prevent deadlock waiting to acquire a DB lock setattr(context, '_mech_context', mech_context)
# held by another thread in the same process, leading self.mechanism_manager.delete_network_precommit(
# to 'lock wait timeout' errors. mech_context)
#
# 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)
# Get subnets to auto-delete. @registry.receives(resources.NETWORK, [events.AFTER_DELETE])
subnets = (session.query(models_v2.Subnet). def _network_delete_after_delete_handler(self, rtype, event, trigger,
enable_eagerloads(False). context, network, **kwargs):
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)
try: try:
self.mechanism_manager.delete_network_postcommit(mech_context) self.mechanism_manager.delete_network_postcommit(
context._mech_context)
except ml2_exc.MechanismDriverError: except ml2_exc.MechanismDriverError:
# TODO(apech) - One or more mechanism driver failed to # TODO(apech) - One or more mechanism driver failed to
# delete the network. Ideally we'd notify the caller of # delete the network. Ideally we'd notify the caller of
# the fact that an error occurred. # the fact that an error occurred.
LOG.error(_LE("mechanism_manager.delete_network_postcommit" LOG.error(_LE("mechanism_manager.delete_network_postcommit"
" failed")) " failed"))
self.notifier.network_delete(context, id) self.notifier.network_delete(context, network['id'])
def _create_subnet_db(self, context, subnet): def _create_subnet_db(self, context, subnet):
session = context.session session = context.session

16
neutron/tests/unit/db/test_db_base_plugin_v2.py
View File

@ -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) res = req.get_response(self.api)
self.assertEqual(webob.exc.HTTPNoContent.code, res.status_int) 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): def test_update_port_delete_ip(self):
with self.subnet() as subnet: with self.subnet() as subnet:
with self.port(subnet=subnet) as port: with self.port(subnet=subnet) as port:

5
neutron/tests/unit/extensions/test_l3.py
View File

@ -256,11 +256,6 @@ class TestL3NatBasePlugin(db_base_plugin_v2.NeutronDbPluginV2,
self._process_l3_update(context, net, network['network']) self._process_l3_update(context, net, network['network'])
return net 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): def delete_port(self, context, id, l3_port_check=True):
plugin = directory.get_plugin(lib_constants.L3) plugin = directory.get_plugin(lib_constants.L3)
if plugin: if plugin:

21
neutron/tests/unit/plugins/ml2/test_plugin.py
View File

@ -26,7 +26,6 @@ from neutron_lib import exceptions as exc
from neutron_lib.plugins import directory from neutron_lib.plugins import directory
from oslo_db import exception as db_exc from oslo_db import exception as db_exc
from oslo_utils import uuidutils from oslo_utils import uuidutils
from sqlalchemy.orm import exc as sqla_exc
from neutron._i18n import _ from neutron._i18n import _
from neutron.callbacks import events from neutron.callbacks import events
@ -243,26 +242,6 @@ class TestMl2NetworksV2(test_plugin.TestNetworksV2,
self.assertEqual(n['network']['id'], self.assertEqual(n['network']['id'],
kwargs['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): def _create_and_verify_networks(self, networks):
for net_idx, net in enumerate(networks): for net_idx, net in enumerate(networks):
# create # create