From 22d2bb94f71d81116046dcf56d8e51e66b6548b6 Mon Sep 17 00:00:00 2001 From: Rico Lin Date: Thu, 28 Nov 2019 04:16:12 +0000 Subject: [PATCH] Revert "Revise log when create port failed" After this patch, during delete subnets with fixed ip, there are high chances a subnet can't be found(because it just deleted) while deleting another subnet and Raise BadRequest instead of Notfound. The behavior change led to heat gate failure, since Heat expect to see NotFound exception instead of BadRequest. Also the error message is really confusing while delete subnets (you can check out more detail in https://storyboard.openstack.org/#!/story/2006962) hence, I think to revert this patch will make sense to maintain the behavior before a proper solution comes out. Story: 2006962 Task: 37652 This reverts commit cae66a4d8d13deda74db806d733c2ac39ea2e849. Change-Id: I9fc200de376668b662cd8c2bfba01483dc9bb677 --- neutron/db/ipam_backend_mixin.py | 6 ++++-- neutron/db/ipam_pluggable_backend.py | 9 +++++---- neutron/tests/unit/db/test_db_base_plugin_v2.py | 2 +- .../tests/unit/db/test_ipam_pluggable_backend.py | 3 ++- neutron/tests/unit/plugins/ml2/test_plugin.py | 13 +++++-------- 5 files changed, 17 insertions(+), 16 deletions(-) diff --git a/neutron/db/ipam_backend_mixin.py b/neutron/db/ipam_backend_mixin.py index 6f67ad5ac02..15656d1880c 100644 --- a/neutron/db/ipam_backend_mixin.py +++ b/neutron/db/ipam_backend_mixin.py @@ -379,7 +379,7 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon): if segment.is_dynamic: raise segment_exc.SubnetCantAssociateToDynamicSegment() - def _get_subnet_for_fixed_ip(self, fixed, subnets, network_id): + def _get_subnet_for_fixed_ip(self, context, fixed, subnets): # Subnets are all the subnets belonging to the same network. if not subnets: msg = _('IP allocation requires subnets for network') @@ -392,10 +392,12 @@ class IpamBackendMixin(db_base_plugin_common.DbBasePluginCommon): return subnet subnet = get_matching_subnet() if not subnet: + subnet_obj = self._get_subnet_object(context, + fixed['subnet_id']) msg = (_("Failed to create port on network %(network_id)s" ", because fixed_ips included invalid subnet " "%(subnet_id)s") % - {'network_id': network_id, + {'network_id': subnet_obj.network_id, 'subnet_id': fixed['subnet_id']}) raise exc.InvalidInput(error_message=msg) # Ensure that the IP is valid on the subnet diff --git a/neutron/db/ipam_pluggable_backend.py b/neutron/db/ipam_pluggable_backend.py index b22c324feff..7282ef80ed2 100644 --- a/neutron/db/ipam_pluggable_backend.py +++ b/neutron/db/ipam_pluggable_backend.py @@ -240,7 +240,8 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): context, subnets) if fixed_configured: - ips = self._test_fixed_ips_for_port(p["network_id"], + ips = self._test_fixed_ips_for_port(context, + p["network_id"], p['fixed_ips'], p['device_owner'], subnets) @@ -274,7 +275,7 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): 'mac': port['mac_address']}) return ips - def _test_fixed_ips_for_port(self, network_id, fixed_ips, + def _test_fixed_ips_for_port(self, context, network_id, fixed_ips, device_owner, subnets): """Test fixed IPs for port. @@ -288,7 +289,7 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): fixed_ip_list = [] for fixed in fixed_ips: fixed['device_owner'] = device_owner - subnet = self._get_subnet_for_fixed_ip(fixed, subnets, network_id) + subnet = self._get_subnet_for_fixed_ip(context, fixed, subnets) is_auto_addr_subnet = ipv6_utils.is_auto_address_subnet(subnet) if ('ip_address' in fixed and @@ -350,7 +351,7 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): # Check if the IP's to add are OK to_add = self._test_fixed_ips_for_port( - port['network_id'], changes.add, + context, port['network_id'], changes.add, port['device_owner'], subnets) if port['device_owner'] not in constants.ROUTER_INTERFACE_OWNERS: diff --git a/neutron/tests/unit/db/test_db_base_plugin_v2.py b/neutron/tests/unit/db/test_db_base_plugin_v2.py index bc4e96c83b1..ea3cdd288a6 100644 --- a/neutron/tests/unit/db/test_db_base_plugin_v2.py +++ b/neutron/tests/unit/db/test_db_base_plugin_v2.py @@ -2466,7 +2466,7 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s net_id = port['port']['network_id'] res = self._create_port(self.fmt, net_id=net_id, **kwargs) port2 = self.deserialize(self.fmt, res) - self.assertEqual(webob.exc.HTTPBadRequest.code, res.status_int) + self.assertEqual(webob.exc.HTTPNotFound.code, res.status_int) # Test invalid IP address on specified subnet_id kwargs = {"fixed_ips": diff --git a/neutron/tests/unit/db/test_ipam_pluggable_backend.py b/neutron/tests/unit/db/test_ipam_pluggable_backend.py index 13ad3642d59..c013176e471 100644 --- a/neutron/tests/unit/db/test_ipam_pluggable_backend.py +++ b/neutron/tests/unit/db/test_ipam_pluggable_backend.py @@ -368,6 +368,7 @@ class TestDbBasePluginIpam(test_db_base.NeutronDbPluginV2TestCase): mocks['subnets'].allocate.assert_called_once_with(mock.ANY) def test_test_fixed_ips_for_port_pd_gateway(self): + context = mock.Mock() pluggable_backend = ipam_pluggable_backend.IpamPluggableBackend() with self.subnet(cidr=constants.PROVISIONAL_IPV6_PD_PREFIX, ip_version=constants.IP_VERSION_6) as subnet: @@ -375,7 +376,7 @@ class TestDbBasePluginIpam(test_db_base.NeutronDbPluginV2TestCase): fixed_ips = [{'subnet_id': subnet['id'], 'ip_address': '::1'}] filtered_ips = (pluggable_backend. - _test_fixed_ips_for_port( + _test_fixed_ips_for_port(context, subnet['network_id'], fixed_ips, constants.DEVICE_OWNER_ROUTER_INTF, diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index b4b0811e854..b4c2fd5ae93 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -720,15 +720,12 @@ class TestMl2SubnetsV2(test_plugin.TestSubnetsV2, req = self.new_delete_request('subnets', s1['subnet']['id']) res = req.get_response(self.api) - self.assertEqual(webob.exc.HTTPBadRequest.code, - res.status_int) - # ensure port has IP on s1 and s3 + self.assertEqual(204, res.status_int) + # ensure port only has 1 IP on s3 port = self._show('ports', p['port']['id'])['port'] - self.assertEqual(2, len(port['fixed_ips'])) - port_subnet_id = [port['fixed_ips'][0]['subnet_id'], - port['fixed_ips'][1]['subnet_id']] - self.assertIn(s1['subnet']['id'], port_subnet_id) - self.assertIn(s3['subnet']['id'], port_subnet_id) + self.assertEqual(1, len(port['fixed_ips'])) + self.assertEqual(s3['subnet']['id'], + port['fixed_ips'][0]['subnet_id']) def test_update_subnet_with_empty_body(self): with self.subnet() as subnet: