diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 7405917c163..2a0761b179f 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -599,8 +599,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, raise n_exc.BadRequest(resource='router', msg=msg) return port_id_specified, subnet_id_specified - def _check_router_port(self, context, port_id, device_id, - router_id=None): + def _check_router_port(self, context, port_id, device_id): """Check that a port is available for an attachment to a router :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 the device_id of the port. It raises PortInUse exception if it 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. :raises: PortInUse if the device_id is not the same as the port's one. :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']: msg = _('Router port must have at least one fixed IP') 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 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) 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 router :param context: The context of the request. - :param router_id: The id of the router the interface is being - attached to. - :param network_id: The id of the network the port belongs to. + :param router_db: The router db object having an interface attached. + :param port: The port object being attached to the router. + :param interface_info: The requested interface attachment info passed + to add_router_interface. :raises: RouterInterfaceAttachmentConflict if a third party code prevent the port to be attach to the router. """ @@ -683,9 +676,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, events.BEFORE_CREATE, self, context=context, - router_id=router_id, - network_id=network_id - ) + router_db=router_db, + port=port, + interface_info=interface_info, + router_id=router_db.id, + network_id=port['network_id']) except exceptions.CallbackFailure as e: # raise the underlying exception 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): subnet = self._core_plugin.get_subnet(context, subnet_id) - self._notify_attaching_interface(context, router.id, - subnet['network_id']) if not subnet['gateway_ip']: msg = _('Subnet for router interface must have a gateway IP') 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) # This should be True unless adding an IPv6 prefix to an existing port - new_port = True + new_router_intf = True cleanup_port = False if add_by_port: port_id = interface_info['port_id'] - port = self._check_router_port(context, port_id, '', - router_id=router_id) + port = self._check_router_port(context, port_id, '') revert_value = {'device_id': '', 'device_owner': port['device_owner']} 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 # _validate_interface_info ensures that either of add_by_* is True. 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) - cleanup_port = new_port # only cleanup port we created + cleanup_port = new_router_intf # only cleanup port we created revert_value = {'device_id': '', 'device_owner': port['device_owner']} @@ -802,8 +794,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, mgr = p_utils.update_port_on_error( self._core_plugin, context, port['id'], revert_value) - if new_port: + if new_router_intf: with mgr: + self._notify_attaching_interface(context, router_db=router, + port=port, + interface_info=interface_info) with context.session.begin(subtransactions=True): router_port = l3_models.RouterPort( port_id=port['id'], @@ -823,7 +818,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, gw_network_id = None if router.gw_port: 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, events.AFTER_CREATE, @@ -832,9 +827,11 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, network_id=gw_network_id, gateway_ips=gw_ips, cidrs=[x['cidr'] for x in subnets], + subnets=subnets, port_id=port['id'], router_id=router_id, port=port, + new_interface=new_router_intf, interface_info=interface_info) return self._make_router_interface_info( diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index 0aca1c02379..15c21dc3e95 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -83,6 +83,10 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, resources.ROUTER, events.PRECOMMIT_CREATE) registry.subscribe(n._handle_distributed_migration, 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 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() @db_api.retry_if_session_inactive() - def add_router_interface(self, context, router_id, interface_info): - add_by_port, add_by_sub = self._validate_interface_info(interface_info) - router = self._get_router(context, router_id) - device_owner = self._get_device_owner(context, router) - - # This should be True unless adding an IPv6 prefix to an existing port - new_port = True - cleanup_port = False - - 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']} + def _add_csnat_on_interface_create(self, resource, event, trigger, + context, router_db, port, **kwargs): + """Event handler to for csnat port creation on interface creation.""" + if not router_db.extra_attributes.distributed or not router_db.gw_port: + return + admin_context = context.elevated() + self._add_csnat_router_interface_port( + admin_context, router_db, port['network_id'], + port['fixed_ips'][-1]['subnet_id']) + @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] - - if cleanup_port: - 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}}) - + if not subnet or subnet['ip_version'] != 6: + return # NOTE: For IPv6 additional subnets added to the same # network we need to update the CSNAT port with respective # IPv6 subnet - elif subnet and port: - fixed_ip = {'subnet_id': subnet['id']} - if subnet['ip_version'] == 6: - # Add new prefix to an existing ipv6 csnat port with the - # same network id if one exists - cs_port = ( - self._find_v6_router_port_by_network_and_device_owner( - router, subnet['network_id'], - const.DEVICE_OWNER_ROUTER_SNAT)) - if cs_port: - fixed_ips = list(cs_port['fixed_ips']) - fixed_ips.append(fixed_ip) - try: - updated_port = self._core_plugin.update_port( - context.elevated(), - cs_port['id'], - {'port': {'fixed_ips': fixed_ips}}) - except Exception: - with excutils.save_and_reraise_exception(): - # we need to try to undo the updated router - # interface from above so it's not out of sync - # with the csnat port. - # TODO(kevinbenton): switch to taskflow to manage - # these rollbacks. - @db_api.retry_db_errors - def revert(): - # TODO(kevinbenton): even though we get the - # port each time, there is a potential race - # where we update the port with stale IPs if - # another interface operation is occuring at - # the same time. This can be fixed in the - # future with a compare-and-swap style update - # using the revision number of the port. - p = self._core_plugin.get_port( - context.elevated(), port['id']) - upd = {'port': {'fixed_ips': [ - ip for ip in p['fixed_ips'] - if ip['subnet_id'] != fixed_ip['subnet_id'] - ]}} - self._core_plugin.update_port( - context.elevated(), port['id'], upd) - try: - 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 + # Add new prefix to an existing ipv6 csnat port with the + # same network id if one exists + admin_ctx = context.elevated() + router = self._get_router(admin_ctx, router_id) + cs_port = self._find_v6_router_port_by_network_and_device_owner( + router, subnet['network_id'], const.DEVICE_OWNER_ROUTER_SNAT) + if not cs_port: + return + new_fixed_ip = {'subnet_id': subnet['id']} + fixed_ips = list(cs_port['fixed_ips']) + fixed_ips.append(new_fixed_ip) + try: + updated_port = self._core_plugin.update_port( + admin_ctx, cs_port['id'], {'port': {'fixed_ips': fixed_ips}}) + except Exception: + with excutils.save_and_reraise_exception(): + # we need to try to undo the updated router + # interface from above so it's not out of sync + # with the csnat port. + # TODO(kevinbenton): switch to taskflow to manage + # these rollbacks. + @db_api.retry_db_errors + def revert(): + # TODO(kevinbenton): even though we get the + # port each time, there is a potential race + # where we update the port with stale IPs if + # another interface operation is occuring at + # the same time. This can be fixed in the + # future with a compare-and-swap style update + # using the revision number of the port. + p = self._core_plugin.get_port(admin_ctx, port['id']) + rollback_fixed_ips = [ip for ip in p['fixed_ips'] + if ip['subnet_id'] != subnet['id']] + upd = {'port': {'fixed_ips': rollback_fixed_ips}} + self._core_plugin.update_port(admin_ctx, port['id'], upd) + try: + 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) def _port_has_ipv6_address(self, port, csnat_port_check=True): """Overridden to return False if DVR SNAT port.""" diff --git a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py index ee43f517ebd..f0239eebdb2 100644 --- a/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py +++ b/neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py @@ -1552,11 +1552,14 @@ class L3DvrTestCase(L3DvrTestCaseBase): router = self._create_router() with self.network() as net, \ self.subnet(network=net) as subnet: + interface_info = {'subnet_id': subnet['subnet']['id']} self.l3_plugin.add_router_interface( - self.context, router['id'], - {'subnet_id': subnet['subnet']['id']}) + self.context, router['id'], interface_info) 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( resources.ROUTER_INTERFACE, events.BEFORE_CREATE, mock.ANY, **kwargs) @@ -1566,6 +1569,8 @@ class L3DvrTestCase(L3DvrTestCaseBase): 'interface_info': mock.ANY, 'network_id': None, 'port': mock.ANY, + 'new_interface': True, + 'subnets': mock.ANY, 'port_id': mock.ANY, 'router_id': router['id']} notif_handler_after.callback.assert_called_once_with( @@ -1585,11 +1590,14 @@ class L3DvrTestCase(L3DvrTestCaseBase): with self.network() as net, \ self.subnet(network=net) as subnet, \ self.port(subnet=subnet) as port: + interface_info = {'port_id': port['port']['id']} self.l3_plugin.add_router_interface( - self.context, router['id'], - {'port_id': port['port']['id']}) + self.context, router['id'], interface_info) 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( resources.ROUTER_INTERFACE, events.BEFORE_CREATE, mock.ANY, **kwargs) @@ -1599,6 +1607,8 @@ class L3DvrTestCase(L3DvrTestCaseBase): 'interface_info': mock.ANY, 'network_id': None, 'port': mock.ANY, + 'new_interface': True, + 'subnets': mock.ANY, 'port_id': port['port']['id'], 'router_id': router['id']} notif_handler_after.callback.assert_called_once_with( diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index 574575d792c..e28da482461 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/neutron/tests/unit/db/test_l3_db.py @@ -233,9 +233,14 @@ class TestL3_NAT_dbonly_mixin(base.BaseTestCase): context = mock.MagicMock() router_id = 'router_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, - 'network_id': net_id} + 'network_id': net_id, 'interface_info': intf, + 'router_db': router_db, 'port': port} mock_notify.assert_called_once_with( resources.ROUTER_INTERFACE, events.BEFORE_CREATE, self.db, **kwargs) diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index 4d6d716f289..bfceb0c2631 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -18,7 +18,6 @@ from neutron_lib import constants as const from neutron_lib import exceptions from neutron_lib.plugins import directory from oslo_utils import uuidutils -import testtools from neutron.callbacks import events 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 l3_agentschedulers_db from neutron.db import l3_dvr_db +from neutron.extensions import l3 from neutron.extensions import portbindings 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', gateway_ip='fe81::1', ip_version=6) as subnet2_v6: - with testtools.ExpectedException(RuntimeError): - self.mixin.add_router_interface(self.ctx, router['id'], - {'subnet_id': subnet2_v6['subnet']['id']}) + self.mixin.add_router_interface(self.ctx, router['id'], + {'subnet_id': subnet2_v6['subnet']['id']}) if fail_revert: # a revert failure will mean the interface is still added # 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']} self.mixin.add_router_interface(self.ctx, router_db.id, interface_info) - mock_notify.assert_called_once_with(self.ctx, router_db.id, - sub['subnet']['network_id']) + mock_notify.assert_called_once_with(self.ctx, router_db=router_db, + port=mock.ANY, + interface_info=interface_info) def test_validate_add_router_interface_by_port_notify_advanced_services( self): @@ -680,8 +680,9 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): interface_info = {'port_id': port['port']['id']} self.mixin.add_router_interface(self.ctx, router_db.id, interface_info) - mock_notify.assert_called_once_with(self.ctx, router_db.id, - net['network']['id']) + mock_notify.assert_called_once_with(self.ctx, router_db=router_db, + port=mock.ANY, + interface_info=interface_info) def _test_update_arp_entry_for_dvr_service_port( 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: f.side_effect = RuntimeError() self.assertRaises( - RuntimeError, + l3.RouterInterfaceAttachmentConflict, self.mixin.add_router_interface, self.ctx, router['id'], {'subnet_id': subnet['subnet']['id']})