From bcef61703061c578826a3ea26fbf846b6670fb0e Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Tue, 2 Aug 2016 18:46:11 -0700 Subject: [PATCH] Fix indexerror in delete_csnat_port The code assumed every port returned from the csnat port query would have a fixed_ip that it could compare the subnet it is looking for to. This should be a valid assumption however there is a path leading to a condition where it has no IPs. This makes the cleanup code handle this case and dump a warning until we can figure out what causes the interface to lose the IP. Partial-Bug: #1609540 Change-Id: Ida024a231bb3fc09dad0e80498f57a5761ca3420 --- neutron/db/l3_dvr_db.py | 4 +- neutron/tests/unit/db/test_l3_dvr_db.py | 78 ++++++++++++++++--------- 2 files changed, 52 insertions(+), 30 deletions(-) diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index 2cc082fd716..2dab8d9d9d4 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -898,7 +898,9 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin, filters={'id': ports} ) for p in c_snat_ports: - if subnet_id is None: + if subnet_id is None or not p['fixed_ips']: + if not p['fixed_ips']: + LOG.debug("CSNAT port has no IPs: %s", p) self._core_plugin.delete_port(context, p['id'], l3_port_check=False) diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index 6885c5e4130..aa2563423ff 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -525,7 +525,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): self.ctx, filters=dvr_filters) self.assertEqual(1, len(dvr_ports)) - def test_remove_router_interface_csnat_ports_removal_with_ipv6(self): + def _setup_router_with_v4_and_v6(self): router_dict = {'name': 'test_router', 'admin_state_up': True, 'distributed': True} router = self._create_router(router_dict) @@ -548,36 +548,56 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): {'subnet_id': subnet_v4['subnet']['id']}) self.mixin.add_router_interface(self.ctx, router['id'], {'subnet_id': subnet_v6['subnet']['id']}) - csnat_filters = {'device_owner': - [l3_const.DEVICE_OWNER_ROUTER_SNAT]} - csnat_ports = self.core_plugin.get_ports( - self.ctx, filters=csnat_filters) - self.assertEqual(2, len(csnat_ports)) - dvr_filters = {'device_owner': - [l3_const.DEVICE_OWNER_DVR_INTERFACE]} - dvr_ports = self.core_plugin.get_ports( - self.ctx, filters=dvr_filters) - self.assertEqual(2, len(dvr_ports)) - with mock.patch.object( - manager.NeutronManager, - 'get_service_plugins') as get_svc_plugin: - get_svc_plugin.return_value = { - plugin_const.L3_ROUTER_NAT: plugin} - self.mixin.manager = manager - self.mixin.remove_router_interface( - self.ctx, router['id'], - {'subnet_id': subnet_v4['subnet']['id']}) + get_svc_plugin = mock.patch.object( + manager.NeutronManager, 'get_service_plugins').start() + get_svc_plugin.return_value = { + plugin_const.L3_ROUTER_NAT: plugin} + self.mixin.manager = manager + return router, subnet_v4, subnet_v6 - csnat_ports = self.core_plugin.get_ports( - self.ctx, filters=csnat_filters) - self.assertEqual(1, len(csnat_ports)) - self.assertEqual( - subnet_v6['subnet']['id'], - csnat_ports[0]['fixed_ips'][0]['subnet_id']) + def test_remove_router_interface_csnat_ports_removal_with_ipv6(self): + router, subnet_v4, subnet_v6 = self._setup_router_with_v4_and_v6() + csnat_filters = {'device_owner': + [l3_const.DEVICE_OWNER_ROUTER_SNAT]} + csnat_ports = self.core_plugin.get_ports( + self.ctx, filters=csnat_filters) + self.assertEqual(2, len(csnat_ports)) + dvr_filters = {'device_owner': + [l3_const.DEVICE_OWNER_DVR_INTERFACE]} + dvr_ports = self.core_plugin.get_ports( + self.ctx, filters=dvr_filters) + self.assertEqual(2, len(dvr_ports)) + self.mixin.remove_router_interface( + self.ctx, router['id'], + {'subnet_id': subnet_v4['subnet']['id']}) + csnat_ports = self.core_plugin.get_ports( + self.ctx, filters=csnat_filters) + self.assertEqual(1, len(csnat_ports)) + self.assertEqual( + subnet_v6['subnet']['id'], + csnat_ports[0]['fixed_ips'][0]['subnet_id']) - dvr_ports = self.core_plugin.get_ports( - self.ctx, filters=dvr_filters) - self.assertEqual(1, len(dvr_ports)) + dvr_ports = self.core_plugin.get_ports( + self.ctx, filters=dvr_filters) + self.assertEqual(1, len(dvr_ports)) + + def test_remove_router_interface_csnat_port_missing_ip(self): + # NOTE(kevinbenton): this is a contrived scenario to reproduce + # a condition observed in bug/1609540. Once we figure out why + # these ports lose their IP we can remove this test. + router, subnet_v4, subnet_v6 = self._setup_router_with_v4_and_v6() + self.mixin.remove_router_interface( + self.ctx, router['id'], + {'subnet_id': subnet_v4['subnet']['id']}) + csnat_filters = {'device_owner': + [l3_const.DEVICE_OWNER_ROUTER_SNAT]} + csnat_ports = self.core_plugin.get_ports( + self.ctx, filters=csnat_filters) + self.core_plugin.update_port(self.ctx, csnat_ports[0]['id'], + {'port': {'fixed_ips': []}}) + self.mixin.remove_router_interface( + self.ctx, router['id'], + {'subnet_id': subnet_v6['subnet']['id']}) def test__validate_router_migration_notify_advanced_services(self): router = {'name': 'foo_router', 'admin_state_up': False}