Get rid of DVR override of add_router_interface

DVR had a complete copy and paste of the l3_db
add_router_interface. This patch gets rid of that
by adding in a some args to existing callbacks. \o/

This also has a variable rename from 'new_port' to
'new_router_intf' to clearly indicate that it's referring
to the creation of a new router interface regardless of
the creation of a core plugin port.

Change-Id: I6192c41419a992be9d0ded338f7a87ebcefda6af
This commit is contained in:
Kevin Benton 2016-11-18 01:53:21 -07:00
parent 5e674bf738
commit efe1c3087c
5 changed files with 123 additions and 173 deletions

View File

@ -599,8 +599,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
raise n_exc.BadRequest(resource='router', msg=msg) raise n_exc.BadRequest(resource='router', msg=msg)
return port_id_specified, subnet_id_specified return port_id_specified, subnet_id_specified
def _check_router_port(self, context, port_id, device_id, def _check_router_port(self, context, port_id, device_id):
router_id=None):
"""Check that a port is available for an attachment to a router """Check that a port is available for an attachment to a router
:param context: The context of the request. :param context: The context of the request.
@ -608,11 +607,6 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
:param device_id: This method will check that device_id corresponds to :param device_id: This method will check that device_id corresponds to
the device_id of the port. It raises PortInUse exception if it the device_id of the port. It raises PortInUse exception if it
doesn't. doesn't.
:param router_id: This method will use this router_id to notify
third party code that an attachment is occurring on this router.
Third party code can prevent this attachment by raising an exception
that will be caught and reraised with a
RouterInterfaceAttachmentConflict exception.
:returns: The port description returned by the core plugin. :returns: The port description returned by the core plugin.
:raises: PortInUse if the device_id is not the same as the port's one. :raises: PortInUse if the device_id is not the same as the port's one.
:raises: BadRequest if the port has no fixed IP. :raises: BadRequest if the port has no fixed IP.
@ -625,9 +619,6 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
if not port['fixed_ips']: if not port['fixed_ips']:
msg = _('Router port must have at least one fixed IP') msg = _('Router port must have at least one fixed IP')
raise n_exc.BadRequest(resource='router', msg=msg) raise n_exc.BadRequest(resource='router', msg=msg)
if router_id is not None:
self._notify_attaching_interface(context, router_id,
port['network_id'])
return port return port
def _validate_router_port_info(self, context, router, port_id): def _validate_router_port_info(self, context, router, port_id):
@ -667,14 +658,16 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
raise n_exc.BadRequest(resource='router', msg=msg) raise n_exc.BadRequest(resource='router', msg=msg)
return port, subnets return port, subnets
def _notify_attaching_interface(self, context, router_id, network_id): def _notify_attaching_interface(self, context, router_db, port,
interface_info):
"""Notify third party code that an interface is being attached to a """Notify third party code that an interface is being attached to a
router router
:param context: The context of the request. :param context: The context of the request.
:param router_id: The id of the router the interface is being :param router_db: The router db object having an interface attached.
attached to. :param port: The port object being attached to the router.
:param network_id: The id of the network the port belongs to. :param interface_info: The requested interface attachment info passed
to add_router_interface.
:raises: RouterInterfaceAttachmentConflict if a third party code :raises: RouterInterfaceAttachmentConflict if a third party code
prevent the port to be attach to the router. prevent the port to be attach to the router.
""" """
@ -683,9 +676,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
events.BEFORE_CREATE, events.BEFORE_CREATE,
self, self,
context=context, context=context,
router_id=router_id, router_db=router_db,
network_id=network_id port=port,
) interface_info=interface_info,
router_id=router_db.id,
network_id=port['network_id'])
except exceptions.CallbackFailure as e: except exceptions.CallbackFailure as e:
# raise the underlying exception # raise the underlying exception
reason = (_('cannot perform router interface attachment ' reason = (_('cannot perform router interface attachment '
@ -715,8 +710,6 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
def _add_interface_by_subnet(self, context, router, subnet_id, owner): def _add_interface_by_subnet(self, context, router, subnet_id, owner):
subnet = self._core_plugin.get_subnet(context, subnet_id) subnet = self._core_plugin.get_subnet(context, subnet_id)
self._notify_attaching_interface(context, router.id,
subnet['network_id'])
if not subnet['gateway_ip']: if not subnet['gateway_ip']:
msg = _('Subnet for router interface must have a gateway IP') msg = _('Subnet for router interface must have a gateway IP')
raise n_exc.BadRequest(resource='router', msg=msg) raise n_exc.BadRequest(resource='router', msg=msg)
@ -773,13 +766,12 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
device_owner = self._get_device_owner(context, router_id) device_owner = self._get_device_owner(context, router_id)
# This should be True unless adding an IPv6 prefix to an existing port # This should be True unless adding an IPv6 prefix to an existing port
new_port = True new_router_intf = True
cleanup_port = False cleanup_port = False
if add_by_port: if add_by_port:
port_id = interface_info['port_id'] port_id = interface_info['port_id']
port = self._check_router_port(context, port_id, '', port = self._check_router_port(context, port_id, '')
router_id=router_id)
revert_value = {'device_id': '', revert_value = {'device_id': '',
'device_owner': port['device_owner']} 'device_owner': port['device_owner']}
with p_utils.update_port_on_error( with p_utils.update_port_on_error(
@ -789,9 +781,9 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
# add_by_subnet is not used here, because the validation logic of # add_by_subnet is not used here, because the validation logic of
# _validate_interface_info ensures that either of add_by_* is True. # _validate_interface_info ensures that either of add_by_* is True.
else: else:
port, subnets, new_port = self._add_interface_by_subnet( port, subnets, new_router_intf = self._add_interface_by_subnet(
context, router, interface_info['subnet_id'], device_owner) context, router, interface_info['subnet_id'], device_owner)
cleanup_port = new_port # only cleanup port we created cleanup_port = new_router_intf # only cleanup port we created
revert_value = {'device_id': '', revert_value = {'device_id': '',
'device_owner': port['device_owner']} 'device_owner': port['device_owner']}
@ -802,8 +794,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
mgr = p_utils.update_port_on_error( mgr = p_utils.update_port_on_error(
self._core_plugin, context, port['id'], revert_value) self._core_plugin, context, port['id'], revert_value)
if new_port: if new_router_intf:
with mgr: with mgr:
self._notify_attaching_interface(context, router_db=router,
port=port,
interface_info=interface_info)
with context.session.begin(subtransactions=True): with context.session.begin(subtransactions=True):
router_port = l3_models.RouterPort( router_port = l3_models.RouterPort(
port_id=port['id'], port_id=port['id'],
@ -823,7 +818,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
gw_network_id = None gw_network_id = None
if router.gw_port: if router.gw_port:
gw_network_id = router.gw_port.network_id gw_network_id = router.gw_port.network_id
gw_ips = router.gw_port.fixed_ips gw_ips = [x['ip_address'] for x in router.gw_port.fixed_ips]
registry.notify(resources.ROUTER_INTERFACE, registry.notify(resources.ROUTER_INTERFACE,
events.AFTER_CREATE, events.AFTER_CREATE,
@ -832,9 +827,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
network_id=gw_network_id, network_id=gw_network_id,
gateway_ips=gw_ips, gateway_ips=gw_ips,
cidrs=[x['cidr'] for x in subnets], cidrs=[x['cidr'] for x in subnets],
subnets=subnets,
port_id=port['id'], port_id=port['id'],
router_id=router_id, router_id=router_id,
port=port, port=port,
new_interface=new_router_intf,
interface_info=interface_info) interface_info=interface_info)
return self._make_router_interface_info( return self._make_router_interface_info(

View File

@ -83,6 +83,10 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
resources.ROUTER, events.PRECOMMIT_CREATE) resources.ROUTER, events.PRECOMMIT_CREATE)
registry.subscribe(n._handle_distributed_migration, registry.subscribe(n._handle_distributed_migration,
resources.ROUTER, events.PRECOMMIT_UPDATE) resources.ROUTER, events.PRECOMMIT_UPDATE)
registry.subscribe(n._add_csnat_on_interface_create,
resources.ROUTER_INTERFACE, events.BEFORE_CREATE)
registry.subscribe(n._update_snat_v6_addrs_after_intf_update,
resources.ROUTER_INTERFACE, events.AFTER_CREATE)
return n return n
def _set_distributed_flag(self, resource, event, trigger, context, def _set_distributed_flag(self, resource, event, trigger, context,
@ -295,141 +299,74 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
return floating_ip.first() return floating_ip.first()
@db_api.retry_if_session_inactive() @db_api.retry_if_session_inactive()
def add_router_interface(self, context, router_id, interface_info): def _add_csnat_on_interface_create(self, resource, event, trigger,
add_by_port, add_by_sub = self._validate_interface_info(interface_info) context, router_db, port, **kwargs):
router = self._get_router(context, router_id) """Event handler to for csnat port creation on interface creation."""
device_owner = self._get_device_owner(context, router) if not router_db.extra_attributes.distributed or not router_db.gw_port:
return
# This should be True unless adding an IPv6 prefix to an existing port admin_context = context.elevated()
new_port = True self._add_csnat_router_interface_port(
cleanup_port = False admin_context, router_db, port['network_id'],
port['fixed_ips'][-1]['subnet_id'])
if add_by_port:
port_id = interface_info['port_id']
port = self._check_router_port(context, port_id, '',
router_id=router_id)
revert_value = {'device_id': '',
'device_owner': port['device_owner']}
with p_utils.update_port_on_error(
self._core_plugin, context, port_id, revert_value):
port, subnets = self._add_interface_by_port(
context, router, port_id, device_owner)
elif add_by_sub:
port, subnets, new_port = self._add_interface_by_subnet(
context, router, interface_info['subnet_id'], device_owner)
cleanup_port = new_port
revert_value = {'device_id': '',
'device_owner': port['device_owner']}
@db_api.retry_if_session_inactive()
def _update_snat_v6_addrs_after_intf_update(self, resource, event, triger,
context, subnets, port,
router_id, new_interface,
**kwargs):
if new_interface:
# _add_csnat_on_interface_create handler deals with new ports
return
# if not a new interface, the interface was added to a new subnet,
# which is the first in this list
subnet = subnets[0] subnet = subnets[0]
if not subnet or subnet['ip_version'] != 6:
if cleanup_port: return
mgr = p_utils.delete_port_on_error(
self._core_plugin, context, port['id'])
else:
mgr = p_utils.update_port_on_error(
self._core_plugin, context, port['id'], revert_value)
if new_port:
with mgr:
if router.extra_attributes.distributed and router.gw_port:
admin_context = context.elevated()
self._add_csnat_router_interface_port(
admin_context, router, port['network_id'],
port['fixed_ips'][-1]['subnet_id'])
with context.session.begin(subtransactions=True):
router_port = l3_models.RouterPort(
port_id=port['id'],
router_id=router.id,
port_type=device_owner
)
context.session.add(router_port)
# Update owner after actual process again in order to
# make sure the records in routerports table and ports
# table are consistent.
self._core_plugin.update_port(
context, port['id'], {'port': {
'device_id': router.id,
'device_owner': device_owner}})
# NOTE: For IPv6 additional subnets added to the same # NOTE: For IPv6 additional subnets added to the same
# network we need to update the CSNAT port with respective # network we need to update the CSNAT port with respective
# IPv6 subnet # IPv6 subnet
elif subnet and port: # Add new prefix to an existing ipv6 csnat port with the
fixed_ip = {'subnet_id': subnet['id']} # same network id if one exists
if subnet['ip_version'] == 6: admin_ctx = context.elevated()
# Add new prefix to an existing ipv6 csnat port with the router = self._get_router(admin_ctx, router_id)
# same network id if one exists cs_port = self._find_v6_router_port_by_network_and_device_owner(
cs_port = ( router, subnet['network_id'], const.DEVICE_OWNER_ROUTER_SNAT)
self._find_v6_router_port_by_network_and_device_owner( if not cs_port:
router, subnet['network_id'], return
const.DEVICE_OWNER_ROUTER_SNAT)) new_fixed_ip = {'subnet_id': subnet['id']}
if cs_port: fixed_ips = list(cs_port['fixed_ips'])
fixed_ips = list(cs_port['fixed_ips']) fixed_ips.append(new_fixed_ip)
fixed_ips.append(fixed_ip) try:
try: updated_port = self._core_plugin.update_port(
updated_port = self._core_plugin.update_port( admin_ctx, cs_port['id'], {'port': {'fixed_ips': fixed_ips}})
context.elevated(), except Exception:
cs_port['id'], with excutils.save_and_reraise_exception():
{'port': {'fixed_ips': fixed_ips}}) # we need to try to undo the updated router
except Exception: # interface from above so it's not out of sync
with excutils.save_and_reraise_exception(): # with the csnat port.
# we need to try to undo the updated router # TODO(kevinbenton): switch to taskflow to manage
# interface from above so it's not out of sync # these rollbacks.
# with the csnat port. @db_api.retry_db_errors
# TODO(kevinbenton): switch to taskflow to manage def revert():
# these rollbacks. # TODO(kevinbenton): even though we get the
@db_api.retry_db_errors # port each time, there is a potential race
def revert(): # where we update the port with stale IPs if
# TODO(kevinbenton): even though we get the # another interface operation is occuring at
# port each time, there is a potential race # the same time. This can be fixed in the
# where we update the port with stale IPs if # future with a compare-and-swap style update
# another interface operation is occuring at # using the revision number of the port.
# the same time. This can be fixed in the p = self._core_plugin.get_port(admin_ctx, port['id'])
# future with a compare-and-swap style update rollback_fixed_ips = [ip for ip in p['fixed_ips']
# using the revision number of the port. if ip['subnet_id'] != subnet['id']]
p = self._core_plugin.get_port( upd = {'port': {'fixed_ips': rollback_fixed_ips}}
context.elevated(), port['id']) self._core_plugin.update_port(admin_ctx, port['id'], upd)
upd = {'port': {'fixed_ips': [ try:
ip for ip in p['fixed_ips'] revert()
if ip['subnet_id'] != fixed_ip['subnet_id'] except Exception:
]}} LOG.exception(_LE("Failed to revert change "
self._core_plugin.update_port( "to router port %s."),
context.elevated(), port['id'], upd) port['id'])
try: LOG.debug("CSNAT port updated for IPv6 subnet: %s", updated_port)
revert()
except Exception:
LOG.exception(_LE("Failed to revert change "
"to router port %s."),
port['id'])
LOG.debug("CSNAT port updated for IPv6 subnet: "
"%s", updated_port)
router_interface_info = self._make_router_interface_info(
router_id, port['tenant_id'], port['id'], port['network_id'],
subnet['id'], [subnet['id']])
self.notify_router_interface_action(
context, router_interface_info, 'add')
gw_ips = []
gw_network_id = None
if router.gw_port:
gw_network_id = router.gw_port.network_id
gw_ips = [x['ip_address'] for x in router.gw_port.fixed_ips]
registry.notify(resources.ROUTER_INTERFACE,
events.AFTER_CREATE,
self,
context=context,
network_id=gw_network_id,
gateway_ips=gw_ips,
cidrs=[x['cidr'] for x in subnets],
port_id=port['id'],
router_id=router_id,
port=port,
interface_info=interface_info)
return router_interface_info
def _port_has_ipv6_address(self, port, csnat_port_check=True): def _port_has_ipv6_address(self, port, csnat_port_check=True):
"""Overridden to return False if DVR SNAT port.""" """Overridden to return False if DVR SNAT port."""

View File

@ -1552,11 +1552,14 @@ class L3DvrTestCase(L3DvrTestCaseBase):
router = self._create_router() router = self._create_router()
with self.network() as net, \ with self.network() as net, \
self.subnet(network=net) as subnet: self.subnet(network=net) as subnet:
interface_info = {'subnet_id': subnet['subnet']['id']}
self.l3_plugin.add_router_interface( self.l3_plugin.add_router_interface(
self.context, router['id'], self.context, router['id'], interface_info)
{'subnet_id': subnet['subnet']['id']})
kwargs = {'context': self.context, 'router_id': router['id'], kwargs = {'context': self.context, 'router_id': router['id'],
'network_id': net['network']['id']} 'network_id': net['network']['id'],
'router_db': mock.ANY,
'port': mock.ANY,
'interface_info': interface_info}
notif_handler_before.callback.assert_called_once_with( notif_handler_before.callback.assert_called_once_with(
resources.ROUTER_INTERFACE, events.BEFORE_CREATE, resources.ROUTER_INTERFACE, events.BEFORE_CREATE,
mock.ANY, **kwargs) mock.ANY, **kwargs)
@ -1566,6 +1569,8 @@ class L3DvrTestCase(L3DvrTestCaseBase):
'interface_info': mock.ANY, 'interface_info': mock.ANY,
'network_id': None, 'network_id': None,
'port': mock.ANY, 'port': mock.ANY,
'new_interface': True,
'subnets': mock.ANY,
'port_id': mock.ANY, 'port_id': mock.ANY,
'router_id': router['id']} 'router_id': router['id']}
notif_handler_after.callback.assert_called_once_with( notif_handler_after.callback.assert_called_once_with(
@ -1585,11 +1590,14 @@ class L3DvrTestCase(L3DvrTestCaseBase):
with self.network() as net, \ with self.network() as net, \
self.subnet(network=net) as subnet, \ self.subnet(network=net) as subnet, \
self.port(subnet=subnet) as port: self.port(subnet=subnet) as port:
interface_info = {'port_id': port['port']['id']}
self.l3_plugin.add_router_interface( self.l3_plugin.add_router_interface(
self.context, router['id'], self.context, router['id'], interface_info)
{'port_id': port['port']['id']})
kwargs = {'context': self.context, 'router_id': router['id'], kwargs = {'context': self.context, 'router_id': router['id'],
'network_id': net['network']['id']} 'network_id': net['network']['id'],
'router_db': mock.ANY,
'port': mock.ANY,
'interface_info': interface_info}
notif_handler_before.callback.assert_called_once_with( notif_handler_before.callback.assert_called_once_with(
resources.ROUTER_INTERFACE, events.BEFORE_CREATE, resources.ROUTER_INTERFACE, events.BEFORE_CREATE,
mock.ANY, **kwargs) mock.ANY, **kwargs)
@ -1599,6 +1607,8 @@ class L3DvrTestCase(L3DvrTestCaseBase):
'interface_info': mock.ANY, 'interface_info': mock.ANY,
'network_id': None, 'network_id': None,
'port': mock.ANY, 'port': mock.ANY,
'new_interface': True,
'subnets': mock.ANY,
'port_id': port['port']['id'], 'port_id': port['port']['id'],
'router_id': router['id']} 'router_id': router['id']}
notif_handler_after.callback.assert_called_once_with( notif_handler_after.callback.assert_called_once_with(

View File

@ -233,9 +233,14 @@ class TestL3_NAT_dbonly_mixin(base.BaseTestCase):
context = mock.MagicMock() context = mock.MagicMock()
router_id = 'router_id' router_id = 'router_id'
net_id = 'net_id' net_id = 'net_id'
self.db._notify_attaching_interface(context, router_id, net_id) router_db = mock.Mock()
router_db.id = router_id
port = {'network_id': net_id}
intf = {}
self.db._notify_attaching_interface(context, router_db, port, intf)
kwargs = {'context': context, 'router_id': router_id, kwargs = {'context': context, 'router_id': router_id,
'network_id': net_id} 'network_id': net_id, 'interface_info': intf,
'router_db': router_db, 'port': port}
mock_notify.assert_called_once_with( mock_notify.assert_called_once_with(
resources.ROUTER_INTERFACE, events.BEFORE_CREATE, self.db, resources.ROUTER_INTERFACE, events.BEFORE_CREATE, self.db,
**kwargs) **kwargs)

View File

@ -18,7 +18,6 @@ from neutron_lib import constants as const
from neutron_lib import exceptions from neutron_lib import exceptions
from neutron_lib.plugins import directory from neutron_lib.plugins import directory
from oslo_utils import uuidutils from oslo_utils import uuidutils
import testtools
from neutron.callbacks import events from neutron.callbacks import events
from neutron.callbacks import registry from neutron.callbacks import registry
@ -29,6 +28,7 @@ from neutron.db import agents_db
from neutron.db import common_db_mixin from neutron.db import common_db_mixin
from neutron.db import l3_agentschedulers_db from neutron.db import l3_agentschedulers_db
from neutron.db import l3_dvr_db from neutron.db import l3_dvr_db
from neutron.extensions import l3
from neutron.extensions import portbindings from neutron.extensions import portbindings
from neutron.tests.unit.db import test_db_base_plugin_v2 from neutron.tests.unit.db import test_db_base_plugin_v2
@ -587,9 +587,8 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
with self.subnet(network=net, cidr='fe81::/64', with self.subnet(network=net, cidr='fe81::/64',
gateway_ip='fe81::1', ip_version=6) as subnet2_v6: gateway_ip='fe81::1', ip_version=6) as subnet2_v6:
with testtools.ExpectedException(RuntimeError): self.mixin.add_router_interface(self.ctx, router['id'],
self.mixin.add_router_interface(self.ctx, router['id'], {'subnet_id': subnet2_v6['subnet']['id']})
{'subnet_id': subnet2_v6['subnet']['id']})
if fail_revert: if fail_revert:
# a revert failure will mean the interface is still added # a revert failure will mean the interface is still added
# so we can't re-add it # so we can't re-add it
@ -664,8 +663,9 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
interface_info = {'subnet_id': sub['subnet']['id']} interface_info = {'subnet_id': sub['subnet']['id']}
self.mixin.add_router_interface(self.ctx, router_db.id, self.mixin.add_router_interface(self.ctx, router_db.id,
interface_info) interface_info)
mock_notify.assert_called_once_with(self.ctx, router_db.id, mock_notify.assert_called_once_with(self.ctx, router_db=router_db,
sub['subnet']['network_id']) port=mock.ANY,
interface_info=interface_info)
def test_validate_add_router_interface_by_port_notify_advanced_services( def test_validate_add_router_interface_by_port_notify_advanced_services(
self): self):
@ -680,8 +680,9 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
interface_info = {'port_id': port['port']['id']} interface_info = {'port_id': port['port']['id']}
self.mixin.add_router_interface(self.ctx, router_db.id, self.mixin.add_router_interface(self.ctx, router_db.id,
interface_info) interface_info)
mock_notify.assert_called_once_with(self.ctx, router_db.id, mock_notify.assert_called_once_with(self.ctx, router_db=router_db,
net['network']['id']) port=mock.ANY,
interface_info=interface_info)
def _test_update_arp_entry_for_dvr_service_port( def _test_update_arp_entry_for_dvr_service_port(
self, device_owner, action): self, device_owner, action):
@ -747,7 +748,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase):
self.mixin, '_add_csnat_router_interface_port') as f: self.mixin, '_add_csnat_router_interface_port') as f:
f.side_effect = RuntimeError() f.side_effect = RuntimeError()
self.assertRaises( self.assertRaises(
RuntimeError, l3.RouterInterfaceAttachmentConflict,
self.mixin.add_router_interface, self.mixin.add_router_interface,
self.ctx, router['id'], self.ctx, router['id'],
{'subnet_id': subnet['subnet']['id']}) {'subnet_id': subnet['subnet']['id']})