openstack/neutron
Code Issues Proposed changes

Get rid of delete_subnet method in ML2

Browse Source 
This method has grown to be one of the most hideous methods in
Neutron. It's well past Icehouse so it's time to address the
REVISIT and get rid of it entirely. :)

This patch refactors ML2 and the db_base_plugin_v2 delete_subnet
method so ML2 can perform its ML2-specific tasks with callbacks
and db_base_plugin_v2 can handle the rest.

Partial-Bug: #1666424
Change-Id: Ie29acfbe323b60205ade9d660f7497f5bf4a35ca
This commit is contained in:
Kevin Benton 2017-02-03 07:16:33 -08:00
parent 1f5ee0e894
commit 31fa23d84c
3 changed files with 88 additions and 197 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

109
neutron/db/db_base_plugin_v2.py
View File

@ -26,6 +26,7 @@ from oslo_log import log as logging
from oslo_utils import excutils from oslo_utils import excutils
from oslo_utils import uuidutils from oslo_utils import uuidutils
from sqlalchemy import and_ from sqlalchemy import and_
from sqlalchemy import exc as sql_exc
from sqlalchemy import not_ from sqlalchemy import not_
from neutron._i18n import _, _LE, _LI from neutron._i18n import _, _LE, _LI
@ -883,10 +884,6 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon,
return result return result
def _subnet_check_ip_allocations(self, context, subnet_id):
return (context.session.query(models_v2.IPAllocation).
filter_by(subnet_id=subnet_id).join(models_v2.Port).first())
def _subnet_get_user_allocation(self, context, subnet_id): def _subnet_get_user_allocation(self, context, subnet_id):
"""Check if there are any user ports on subnet and return first.""" """Check if there are any user ports on subnet and return first."""
# need to join with ports table as IPAllocation's port # need to join with ports table as IPAllocation's port
@ -911,55 +908,71 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon,
"cannot delete", subnet_id) "cannot delete", subnet_id)
raise exc.SubnetInUse(subnet_id=subnet_id) raise exc.SubnetInUse(subnet_id=subnet_id)
@db_api.retry_if_session_inactive()
def _remove_subnet_from_port(self, context, sub_id, port_id, auto_subnet):
try:
fixed = [f for f in self.get_port(context, port_id)['fixed_ips']
if f['subnet_id'] != sub_id]
if auto_subnet:
# special flag to avoid re-allocation on auto subnets
fixed.append({'subnet_id': sub_id, 'delete_subnet': True})
data = {attributes.PORT: {'fixed_ips': fixed}}
self.update_port(context, port_id, data)
except exc.PortNotFound:
# port is gone
return
except exc.SubnetNotFound as e:
# another subnet in the fixed ips was concurrently removed. retry
raise os_db_exc.RetryRequest(e)
def _ensure_no_user_ports_on_subnet(self, context, id):
alloc = self._subnet_get_user_allocation(context, id)
if alloc:
LOG.info(_LI("Found port (%(port_id)s, %(ip)s) having IP "
"allocation on subnet "
"%(subnet)s, cannot delete"),
{'ip': alloc.ip_address,
'port_id': alloc.port_id,
'subnet': id})
raise exc.SubnetInUse(subnet_id=id)
@db_api.retry_if_session_inactive()
def _remove_subnet_ip_allocations_from_ports(self, context, id):
# Do not allow a subnet to be deleted if a router is attached to it
self._subnet_check_ip_allocations_internal_router_ports(
context, id)
subnet = self._get_subnet(context, id)
is_auto_addr_subnet = ipv6_utils.is_auto_address_subnet(subnet)
if not is_auto_addr_subnet:
# we only automatically remove IP addresses from user ports if
# the IPs come from auto allocation subnets.
self._ensure_no_user_ports_on_subnet(context, id)
net_allocs = (context.session.query(models_v2.IPAllocation.port_id).
filter_by(subnet_id=id))
port_ids_on_net = [ipal.port_id for ipal in net_allocs]
for port_id in port_ids_on_net:
self._remove_subnet_from_port(context, id, port_id,
auto_subnet=is_auto_addr_subnet)
@db_api.retry_if_session_inactive() @db_api.retry_if_session_inactive()
def delete_subnet(self, context, id): def delete_subnet(self, context, id):
with context.session.begin(subtransactions=True): LOG.debug("Deleting subnet %s", id)
subnet = self._get_subnet(context, id) # Make sure the subnet isn't used by other resources
_check_subnet_not_used(context, id)
# Make sure the subnet isn't used by other resources self._remove_subnet_ip_allocations_from_ports(context, id)
_check_subnet_not_used(context, id) # retry integrity errors to catch ip allocation races
with db_api.exc_to_retry(sql_exc.IntegrityError), \
# Delete all network owned ports context.session.begin(subtransactions=True):
qry_network_ports = ( subnet_db = self._get_subnet(context, id)
context.session.query(models_v2.IPAllocation). subnet = self._make_subnet_dict(subnet_db, context=context)
filter_by(subnet_id=subnet['id']). registry.notify(resources.SUBNET, events.PRECOMMIT_DELETE,
join(models_v2.Port)) self, context=context, subnet_id=id)
# Remove network owned ports, and delete IP allocations context.session.delete(subnet_db)
# for IPv6 addresses which were automatically generated
# via SLAAC
is_auto_addr_subnet = ipv6_utils.is_auto_address_subnet(subnet)
if is_auto_addr_subnet:
self._subnet_check_ip_allocations_internal_router_ports(
context, id)
else:
qry_network_ports = (
qry_network_ports.filter(models_v2.Port.device_owner.
in_(AUTO_DELETE_PORT_OWNERS)))
network_ports = qry_network_ports.all()
if network_ports:
for port in network_ports:
context.session.delete(port)
# Check if there are more IP allocations, unless
# is_auto_address_subnet is True. In that case the check is
# unnecessary. This additional check not only would be wasteful
# for this class of subnet, but is also error-prone since when
# the isolation level is set to READ COMMITTED allocations made
# concurrently will be returned by this query
if not is_auto_addr_subnet:
alloc = self._subnet_check_ip_allocations(context, id)
if alloc:
LOG.info(_LI("Found port (%(port_id)s, %(ip)s) having IP "
"allocation on subnet "
"%(subnet)s, cannot delete"),
{'ip': alloc.ip_address,
'port_id': alloc.port_id,
'subnet': id})
raise exc.SubnetInUse(subnet_id=id)
context.session.delete(subnet)
# Delete related ipam subnet manually, # Delete related ipam subnet manually,
# since there is no FK relationship # since there is no FK relationship
self.ipam.delete_subnet(context, id) self.ipam.delete_subnet(context, id)
registry.notify(resources.SUBNET, events.AFTER_DELETE,
self, context=context, subnet=subnet)
@db_api.retry_if_session_inactive() @db_api.retry_if_session_inactive()
def get_subnet(self, context, id, fields=None): def get_subnet(self, context, id, fields=None):

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

@ -43,7 +43,6 @@ from neutron.callbacks import exceptions
from neutron.callbacks import registry from neutron.callbacks import registry
from neutron.callbacks import resources from neutron.callbacks import resources
from neutron.common import constants as n_const from neutron.common import constants as n_const
from neutron.common import ipv6_utils
from neutron.common import rpc as n_rpc from neutron.common import rpc as n_rpc
from neutron.common import topics from neutron.common import topics
from neutron.common import utils from neutron.common import utils
@ -964,142 +963,30 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2,
return updated_subnet return updated_subnet
@utils.transaction_guard @utils.transaction_guard
@db_api.retry_if_session_inactive()
def delete_subnet(self, context, id): def delete_subnet(self, context, id):
# REVISIT(rkukura) The super(Ml2Plugin, self).delete_subnet() # the only purpose of this override is to protect this from being
# function is not used because it deallocates the subnet's addresses # called inside of a transaction.
# from ports in the DB without invoking the derived class's return super(Ml2Plugin, self).delete_subnet(context, id)
# update_port(), preventing mechanism drivers from being called.
# This approach should be revisited when the API layer is reworked
# during icehouse.
LOG.debug("Deleting subnet %s", id) @registry.receives(resources.SUBNET, [events.PRECOMMIT_DELETE])
session = context.session def _subnet_delete_precommit_handler(self, rtype, event, trigger,
deallocated = set() context, subnet_id, **kwargs):
while True: record = self._get_subnet(context, subnet_id)
# NOTE(kevinbenton): this loop keeps db objects in scope subnet = self._make_subnet_dict(record, context=context)
# so we must expire them or risk stale reads. network = self.get_network(context, subnet['network_id'])
# see bug/1623990 mech_context = driver_context.SubnetContext(self, context,
session.expire_all() subnet, network)
with session.begin(subtransactions=True): # TODO(kevinbenton): move this mech context into something like
record = self._get_subnet(context, id) # a 'delete context' so it's not polluting the real context object
subnet = self._make_subnet_dict(record, None, context=context) setattr(context, '_mech_context', mech_context)
qry_allocated = (session.query(models_v2.IPAllocation). self.mechanism_manager.delete_subnet_precommit(mech_context)
filter_by(subnet_id=id).
join(models_v2.Port))
is_auto_addr_subnet = ipv6_utils.is_auto_address_subnet(subnet)
# Remove network owned ports, and delete IP allocations
# for IPv6 addresses which were automatically generated
# via SLAAC
if is_auto_addr_subnet:
self._subnet_check_ip_allocations_internal_router_ports(
context, id)
else:
qry_allocated = (
qry_allocated.filter(models_v2.Port.device_owner.
in_(db_base_plugin_v2.AUTO_DELETE_PORT_OWNERS)))
allocated = set(qry_allocated.all())
LOG.debug("Ports to auto-deallocate: %s", allocated)
if not is_auto_addr_subnet:
user_alloc = self._subnet_get_user_allocation(
context, id)
if user_alloc:
LOG.info(_LI("Found port (%(port_id)s, %(ip)s) "
"having IP allocation on subnet "
"%(subnet)s, cannot delete"),
{'ip': user_alloc.ip_address,
'port_id': user_alloc.port_id,
'subnet': id})
raise exc.SubnetInUse(subnet_id=id)
db_base_plugin_v2._check_subnet_not_used(context, id) @registry.receives(resources.SUBNET, [events.AFTER_DELETE])
def _subnet_delete_after_delete_handler(self, rtype, event, trigger,
# SLAAC allocations currently can not be removed using context, subnet, **kwargs):
# update_port workflow, and will persist in 'allocated'.
# So for now just make sure update_port is called once for
# them so MechanismDrivers is aware of the change.
# This way SLAAC allocation is deleted by FK on subnet deletion
# TODO(pbondar): rework update_port workflow to allow deletion
# of SLAAC allocation via update_port.
to_deallocate = allocated - deallocated
# If to_deallocate is blank, then all known IPAllocations
# (except SLAAC allocations) were correctly deleted
# during the previous pass.
# Check if there are more IP allocations, unless
# is_auto_address_subnet is True. If transaction isolation
# level is set to READ COMMITTED allocations made
# concurrently will be returned by this query and transaction
# will be restarted. It works for REPEATABLE READ isolation
# level too because this query is executed only once during
# transaction, and if concurrent allocations are detected
# transaction gets restarted. Executing this query second time
# in transaction would result in not seeing allocations
# committed by concurrent transactions.
if not to_deallocate:
if (not is_auto_addr_subnet and
self._subnet_check_ip_allocations(context, id)):
# allocation found and it was DHCP port
# that appeared after autodelete ports were
# removed - continue iteration to find offending port
# to update or raise SubnetInUse
continue
network = self.get_network(context, subnet['network_id'])
mech_context = driver_context.SubnetContext(self, context,
subnet,
network)
self.mechanism_manager.delete_subnet_precommit(
mech_context)
LOG.debug("Deleting subnet record")
session.delete(record)
# The super(Ml2Plugin, self).delete_subnet() is not called,
# so need to manually call delete_subnet for pluggable ipam
self.ipam.delete_subnet(context, id)
LOG.debug("Committing transaction")
break
for a in to_deallocate:
if a.port:
# calling update_port() for each allocation to remove the
# IP from the port and call the MechanismDrivers
fixed_ips = [{'subnet_id': ip.subnet_id,
'ip_address': ip.ip_address}
for ip in a.port.fixed_ips
if ip.subnet_id != id]
# By default auto-addressed ips are not removed from port
# on port update, so mark subnet with 'delete_subnet' flag
# to force ip deallocation on port update.
if is_auto_addr_subnet:
fixed_ips.append({'subnet_id': id,
'delete_subnet': True})
data = {attributes.PORT: {'fixed_ips': fixed_ips}}
try:
# NOTE Don't inline port_id; needed for PortNotFound.
port_id = a.port_id
self.update_port(context, port_id, data)
except exc.PortNotFound:
# NOTE Attempting to access a.port_id here is an error.
LOG.debug("Port %s deleted concurrently", port_id)
except exc.SubnetNotFound:
# NOTE we hit here if another subnet was concurrently
# removed that the port has a fixed_ip on. we just
# continue so the loop re-iterates and the IPs are
# looked up again
continue
except Exception as e:
with excutils.save_and_reraise_exception():
utils.attach_exc_details(
e, _LE("Exception deleting fixed_ip from "
"port %s"), port_id)
deallocated.add(a)
kwargs = {'context': context, 'subnet': subnet}
registry.notify(resources.SUBNET, events.AFTER_DELETE, self, **kwargs)
try: try:
self.mechanism_manager.delete_subnet_postcommit(mech_context) self.mechanism_manager.delete_subnet_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 subnet. Ideally we'd notify the caller of # delete the subnet. Ideally we'd notify the caller of

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

@ -515,7 +515,7 @@ class TestMl2SubnetsV2(test_plugin.TestSubnetsV2,
subnet_id = subnet['subnet']['id'] subnet_id = subnet['subnet']['id']
attempt = [0] attempt = [0]
def check_and_create_ports(context, subnet_id): def create_dhcp_port(*args, **kwargs):
"""A method to emulate race condition. """A method to emulate race condition.
Adds dhcp port in the middle of subnet delete Adds dhcp port in the middle of subnet delete
@ -534,26 +534,17 @@ class TestMl2SubnetsV2(test_plugin.TestSubnetsV2,
port_req = self.new_create_request('ports', data) port_req = self.new_create_request('ports', data)
port_res = port_req.get_response(self.api) port_res = port_req.get_response(self.api)
self.assertEqual(201, port_res.status_int) self.assertEqual(201, port_res.status_int)
return (context.session.query(models_v2.IPAllocation).
filter_by(subnet_id=subnet_id).
join(models_v2.Port).first())
plugin = directory.get_plugin()
# we mock _subnet_check_ip_allocations with method # we mock _subnet_check_ip_allocations with method
# that creates DHCP port 'in the middle' of subnet_delete # that creates DHCP port 'in the middle' of subnet_delete
# causing retry this way subnet is deleted on the # causing retry this way subnet is deleted on the
# second attempt # second attempt
with mock.patch.object(plugin, '_subnet_check_ip_allocations', registry.subscribe(create_dhcp_port, resources.SUBNET,
side_effect=check_and_create_ports): events.PRECOMMIT_DELETE)
req = self.new_delete_request('subnets', subnet_id) req = self.new_delete_request('subnets', subnet_id)
res = req.get_response(self.api) res = req.get_response(self.api)
self.assertEqual(204, res.status_int) self.assertEqual(204, res.status_int)
# Validate chat check is called twice, i.e. after the self.assertEqual(1, attempt[0])
# first check transaction gets restarted.
calls = [mock.call(mock.ANY, subnet_id),
mock.call(mock.ANY, subnet_id)]
plugin._subnet_check_ip_allocations.assert_has_calls = (
calls)
def test_create_subnet_check_mtu_in_mech_context(self): def test_create_subnet_check_mtu_in_mech_context(self):
plugin = directory.get_plugin() plugin = directory.get_plugin()