From 20b9b8934331556e217e63b9a0dab6e71c2ef7c6 Mon Sep 17 00:00:00 2001 From: venkata anil Date: Wed, 15 Mar 2017 14:57:38 +0000 Subject: [PATCH] Fix DetachedInstanceError on subnet delete While trying to delete subnets, some failed with DetachedInstanceError, because 'port' is not part of 'IPAllocation' object when accessed outside the DB session(in which 'IPAllocation' object was created). To avoid this error, we call get_port explicitly before using the port. This issue is not seen on master branch as recent code refactor[1] is also calling get_port there. [1] https://review.openstack.org/#/c/428774/ Closes-bug: #1672701 Change-Id: I924fa7e36ea9e45bf0ef3480972341a851bda86c (cherry picked from commit b9242c348cbf44ef1bd061826067c2ec96b8d2f0) --- neutron/plugins/ml2/plugin.py | 33 ++++++++++--------- neutron/tests/unit/plugins/ml2/test_plugin.py | 26 +++++++++++++++ 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 887a88a9dbb..bc1a1beb81a 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -1059,11 +1059,11 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, session = context.session deallocated = set() while True: - # NOTE(kevinbenton): this loop keeps db objects in scope - # so we must expire them or risk stale reads. - # see bug/1623990 - session.expire_all() with session.begin(subtransactions=True): + # NOTE(kevinbenton): this loop keeps db objects in scope + # so we must expire them or risk stale reads. + # see bug/1623990 + session.expire_all() record = self._get_subnet(context, id) subnet = self._make_subnet_dict(record, None, context=context) qry_allocated = (session.query(models_v2.IPAllocation). @@ -1080,7 +1080,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, qry_allocated = ( qry_allocated.filter(models_v2.Port.device_owner. in_(db_base_plugin_v2.AUTO_DELETE_PORT_OWNERS))) - allocated = set(qry_allocated.all()) + allocated = set(ipal.port_id for ipal in qry_allocated) LOG.debug("Ports to auto-deallocate: %s", allocated) if not is_auto_addr_subnet: user_alloc = self._subnet_get_user_allocation( @@ -1143,14 +1143,20 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, LOG.debug("Committing transaction") break - for a in to_deallocate: - if a.port: + for port_id in to_deallocate: + try: + port = self.get_port(context, port_id) + except exc.PortNotFound: + LOG.debug("Port %s deleted concurrently", port_id) + deallocated.add(port_id) + continue + if port: # calling update_port() for each allocation to remove the # IP from the port and call the MechanismDrivers - fixed_ips = [{'subnet_id': ip.subnet_id, - 'ip_address': ip.ip_address} - for ip in a.port.fixed_ips - if ip.subnet_id != id] + fixed_ips = [{'subnet_id': ip['subnet_id'], + 'ip_address': ip['ip_address']} + for ip in port['fixed_ips'] + if ip['subnet_id'] != id] # By default auto-addressed ips are not removed from port # on port update, so mark subnet with 'delete_subnet' flag # to force ip deallocation on port update. @@ -1159,11 +1165,8 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, 'delete_subnet': True}) data = {attributes.PORT: {'fixed_ips': fixed_ips}} try: - # NOTE Don't inline port_id; needed for PortNotFound. - port_id = a.port_id self.update_port(context, port_id, data) except exc.PortNotFound: - # NOTE Attempting to access a.port_id here is an error. LOG.debug("Port %s deleted concurrently", port_id) except exc.SubnetNotFound: # NOTE we hit here if another subnet was concurrently @@ -1176,7 +1179,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, utils.attach_exc_details( e, _LE("Exception deleting fixed_ip from " "port %s"), port_id) - deallocated.add(a) + deallocated.add(port_id) kwargs = {'context': context, 'subnet': subnet} registry.notify(resources.SUBNET, events.AFTER_DELETE, self, **kwargs) diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index 402e7c7349f..c1e6a4c2ba9 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -522,6 +522,32 @@ class TestMl2SubnetsV2(test_plugin.TestSubnetsV2, kwargs = after_create.mock_calls[0][2] self.assertEqual(s['subnet']['id'], kwargs['subnet']['id']) + def test_subnet_delete_no_DetachedInstanceError_for_port_update(self): + mock_check_subnet_not_used = mock.patch.object( + base_plugin, '_check_subnet_not_used').start() + with self.network() as n: + with self.subnet(network=n, cidr='1.1.1.0/24') as s1,\ + self.subnet(network=n, cidr='1.1.2.0/24') as s2: + fixed_ips = [{'subnet_id': s1['subnet']['id']}, + {'subnet_id': s2['subnet']['id']}] + with self.port(subnet=s1, fixed_ips=fixed_ips, + device_owner=constants.DEVICE_OWNER_DHCP) as p: + + def subnet_not_used(ctx, subnet_id): + # Make the session invalid after the query. + ctx.session.expunge_all() + mock_check_subnet_not_used.side_effect = None + mock_check_subnet_not_used.side_effect = subnet_not_used + req = self.new_delete_request('subnets', + s1['subnet']['id']) + res = req.get_response(self.api) + self.assertEqual(204, res.status_int) + # ensure port only has 1 IP on s2 + port = self._show('ports', p['port']['id'])['port'] + self.assertEqual(1, len(port['fixed_ips'])) + self.assertEqual(s2['subnet']['id'], + port['fixed_ips'][0]['subnet_id']) + def test_port_update_subnetnotfound(self): with self.network() as n: with self.subnet(network=n, cidr='1.1.1.0/24') as s1,\