From 33fb5917248255b759dfe7ff20e53273c9f6170c Mon Sep 17 00:00:00 2001 From: Swaminathan Vasudevan Date: Wed, 16 Aug 2017 21:48:08 -0700 Subject: [PATCH] DVR: Multiple csnat ports created when RouterPort table update fails When router interfaces are added to DVR router, if the router has gateway configured, then the internal csnat ports are created for the corresponding router interfaces. We have seen recently after the csnat port is created if the RouterPort table update fails, there is a DB retry that is happening and that retry operation is creating an additional csnat port. This additional port is not getting removed automatically when the router interfaces are deleted. This issue is seen when testing with a simple heat template as per the bug report. This patch fixes the issue by calling the RouterPort create with delete_port_on_error context. Change-Id: I916011f2200f02556ebb30bce30e349a8023602c Closes-Bug: #1709774 (cherry picked from commit 8c3cb2e15b48f5e2b0c3d22550f00f3c7adc7f33) --- neutron/db/l3_dvr_db.py | 22 +++---- .../l3_router/test_l3_dvr_router_plugin.py | 57 +++++++++++++++++++ neutron/tests/unit/db/test_l3_dvr_db.py | 31 ++++++++++ 3 files changed, 100 insertions(+), 10 deletions(-) diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index c9308ef982f..1d490038e68 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -206,17 +206,19 @@ class DVRResourceOperationHandler(object): msg = _("Unable to create the SNAT Interface Port") raise n_exc.BadRequest(resource='router', msg=msg) - l3_obj.RouterPort( - context, - port_id=snat_port['id'], - router_id=router.id, - port_type=const.DEVICE_OWNER_ROUTER_SNAT - ).create() + with p_utils.delete_port_on_error( + self.l3plugin._core_plugin, context.elevated(), snat_port['id']): + l3_obj.RouterPort( + context, + port_id=snat_port['id'], + router_id=router.id, + port_type=const.DEVICE_OWNER_ROUTER_SNAT + ).create() - if do_pop: - return self.l3plugin._populate_mtu_and_subnets_for_ports( - context, [snat_port]) - return snat_port + if do_pop: + return self.l3plugin._populate_mtu_and_subnets_for_ports( + context, [snat_port]) + return snat_port def _create_snat_intf_ports_if_not_exists(self, context, router): """Function to return the snat interface port list. 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 c71a983a395..55aa4a950fd 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 @@ -85,6 +85,63 @@ class L3DvrTestCase(L3DvrTestCaseBase): self.l3_plugin._get_agent_gw_ports_exist_for_network( self.context, 'network_id', 'host', 'agent_id')) + def test_csnat_ports_are_created_and_deleted_based_on_router_subnet(self): + kwargs = {'arg_list': (external_net.EXTERNAL,), + external_net.EXTERNAL: True} + net1 = self._make_network(self.fmt, 'net1', True) + subnet1 = self._make_subnet( + self.fmt, net1, '10.1.0.1', '10.1.0.0/24', enable_dhcp=True) + subnet2 = self._make_subnet( + self.fmt, net1, '10.2.0.1', '10.2.0.0/24', enable_dhcp=True) + ext_net = self._make_network(self.fmt, 'ext_net', True, **kwargs) + self._make_subnet( + self.fmt, ext_net, '20.0.0.1', '20.0.0.0/24', enable_dhcp=True) + # Create first router and add an interface + router1 = self._create_router() + ext_net_id = ext_net['network']['id'] + net1_id = net1['network']['id'] + # Set gateway to router + self.l3_plugin._update_router_gw_info( + self.context, router1['id'], + {'network_id': ext_net_id}) + # Now add router interface (subnet1) from net1 to router + self.l3_plugin.add_router_interface( + self.context, router1['id'], + {'subnet_id': subnet1['subnet']['id']}) + # Now add router interface (subnet2) from net1 to router + self.l3_plugin.add_router_interface( + self.context, router1['id'], + {'subnet_id': subnet2['subnet']['id']}) + # Now check the valid snat interfaces passed to the agent + snat_router_intfs = self.l3_plugin._get_snat_sync_interfaces( + self.context, [router1['id']]) + self.assertEqual(2, len(snat_router_intfs[router1['id']])) + # Also make sure that there are no csnat ports created and + # left over. + csnat_ports = self.core_plugin.get_ports( + self.context, filters={ + 'network_id': [net1_id], + 'device_owner': [constants.DEVICE_OWNER_ROUTER_SNAT]}) + self.assertEqual(2, len(csnat_ports)) + # Now remove router interface (subnet1) from net1 to router + self.l3_plugin.remove_router_interface( + self.context, router1['id'], + {'subnet_id': subnet1['subnet']['id']}) + # Now remove router interface (subnet2) from net1 to router + self.l3_plugin.remove_router_interface( + self.context, router1['id'], + {'subnet_id': subnet2['subnet']['id']}) + snat_router_intfs = self.l3_plugin._get_snat_sync_interfaces( + self.context, [router1['id']]) + self.assertEqual(0, len(snat_router_intfs[router1['id']])) + # Also make sure that there are no csnat ports created and + # left over. + csnat_ports = self.core_plugin.get_ports( + self.context, filters={ + 'network_id': [net1_id], + 'device_owner': [constants.DEVICE_OWNER_ROUTER_SNAT]}) + self.assertEqual(0, len(csnat_ports)) + def _test_remove_router_interface_leaves_snat_intact(self, by_subnet): with self.subnet() as subnet1, \ self.subnet(cidr='20.0.0.0/24') as subnet2: diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index d48290f81e6..55204069706 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -30,6 +30,7 @@ from neutron.db import common_db_mixin from neutron.db import l3_dvr_db from neutron.db import l3_dvrscheduler_db from neutron.extensions import l3 +from neutron.objects import router as router_obj from neutron.tests.unit.db import test_db_base_plugin_v2 _uuid = uuidutils.generate_uuid @@ -797,6 +798,36 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): self.assertEqual(const.DEVICE_OWNER_ROUTER_GW, router_ports[0]['device_owner']) + def test_csnat_port_not_created_on_RouterPort_update_exception(self): + router_dict = {'name': 'test_router', 'admin_state_up': True, + 'distributed': True} + router = self._create_router(router_dict) + with self.network() as net_ext,\ + self.subnet() as subnet: + ext_net_id = net_ext['network']['id'] + self.core_plugin.update_network( + self.ctx, ext_net_id, + {'network': {'router:external': True}}) + self.mixin.update_router( + self.ctx, router['id'], + {'router': {'external_gateway_info': + {'network_id': ext_net_id}}}) + net_id = subnet['subnet']['network_id'] + with mock.patch.object( + router_obj.RouterPort, 'create') as rtrport_update: + rtrport_update.side_effect = Exception() + self.assertRaises( + l3.RouterInterfaceAttachmentConflict, + self.mixin.add_router_interface, + self.ctx, router['id'], + {'subnet_id': subnet['subnet']['id']}) + filters = { + 'network_id': [net_id], + 'device_owner': [const.DEVICE_OWNER_ROUTER_SNAT] + } + router_ports = self.core_plugin.get_ports(self.ctx, filters) + self.assertEqual(0, len(router_ports)) + def test_add_router_interface_by_port_failure(self): router_dict = {'name': 'test_router', 'admin_state_up': True,