From 8d753d71145c38cee042db587605787b0b2dc2ea Mon Sep 17 00:00:00 2001 From: Pavel Bondar Date: Thu, 31 Mar 2016 13:20:39 +0300 Subject: [PATCH] Update ml2 delete_subnet to deallocate via ipam Previosly during delete_subnet in ml2 plugin ip allocation were removed directly from database. This way ipam driver was unaware of this deallocation. Updated workflow to skip removing ip allocations directly from database. Now ips are deallocated during update_port workflow (L1008). This resolves issue with dhcp ports left in allocated state on ipam provides side. Now they are correctly deallocated via update_port. But this patch has next limitation: currently SLAAC allocations can not be delete via update_port workflow, so ipam driver is still unaware of such deallocations. This part of issue is expected to be fixed as separate patch. Subnet_in_use check was reworked. Previously it assumed that auto-allocated ip are already deleted by the time of this check, but with new workflow auto allocated ips are deleted later (on update_port). So now this check verifies if there are any user allocated ips instead of checking all allocations. Partial-Bug: #1564335 Change-Id: I08d66da8cb57ed88e11ec2b18c8345edfce37d37 (cherry picked from commit 3a2e41bf703d6721fcfe4d72f9ad5c0565fef42d) (cherry picked from commit 71686cdaedab8ea176a98fd760afd3c9af3f6d9d) --- neutron/plugins/ml2/plugin.py | 77 +++++++++++-------- neutron/tests/unit/plugins/ml2/test_plugin.py | 6 ++ 2 files changed, 50 insertions(+), 33 deletions(-) diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 56b4ec47a7b..01fd1a6c6ba 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -879,6 +879,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, LOG.debug("Deleting subnet %s", id) session = context.session + deallocated = set() while True: with session.begin(subtransactions=True): record = self._get_subnet(context, id) @@ -897,43 +898,52 @@ 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 = qry_allocated.all() - # Delete all the IPAllocation that can be auto-deleted - if allocated: - for x in allocated: - session.delete(x) + allocated = set(qry_allocated.all()) LOG.debug("Ports to auto-deallocate: %s", allocated) - # Check if there are more IP allocations, unless - # is_auto_address_subnet is True. In that case the check is - # unnecessary. This additional check not only would be wasteful - # for this class of subnet, but is also error-prone since when - # the isolation level is set to READ COMMITTED allocations made - # concurrently will be returned by this query if not is_auto_addr_subnet: - alloc = self._subnet_check_ip_allocations(context, id) - if alloc: - user_alloc = self._subnet_get_user_allocation( - context, id) - if user_alloc: - LOG.info(_LI("Found port (%(port_id)s, %(ip)s) " - "having IP allocation on subnet " - "%(subnet)s, cannot delete"), - {'ip': user_alloc.ip_address, - 'port_id': user_alloc.port_id, - 'subnet': id}) - raise exc.SubnetInUse(subnet_id=id) - else: - # allocation found and it was DHCP port - # that appeared after autodelete ports were - # removed - need to restart whole operation - raise os_db_exception.RetryRequest( - exc.SubnetInUse(subnet_id=id)) + user_alloc = self._subnet_get_user_allocation( + context, id) + if user_alloc: + LOG.info(_LI("Found port (%(port_id)s, %(ip)s) " + "having IP allocation on subnet " + "%(subnet)s, cannot delete"), + {'ip': user_alloc.ip_address, + 'port_id': user_alloc.port_id, + 'subnet': id}) + raise exc.SubnetInUse(subnet_id=id) db_base_plugin_v2._check_subnet_not_used(context, id) - # If allocated is None, then all the IPAllocation were - # correctly deleted during the previous pass. - if not allocated: + # SLAAC allocations currently can not be removed using + # update_port workflow, and will persist in 'allocated'. + # So for now just make sure update_port is called once for + # them so MechanismDrivers is aware of the change. + # This way SLAAC allocation is deleted by FK on subnet deletion + # TODO(pbondar): rework update_port workflow to allow deletion + # of SLAAC allocation via update_port. + to_deallocate = allocated - deallocated + + # If to_deallocate is blank, then all known IPAllocations + # (except SLAAC allocations) were correctly deleted + # during the previous pass. + # Check if there are more IP allocations, unless + # is_auto_address_subnet is True. If transaction isolation + # level is set to READ COMMITTED allocations made + # concurrently will be returned by this query and transaction + # will be restarted. It works for REPEATABLE READ isolation + # level too because this query is executed only once during + # transaction, and if concurrent allocations are detected + # transaction gets restarted. Executing this query second time + # in transaction would result in not seeing allocations + # committed by concurrent transactions. + if not to_deallocate: + if (not is_auto_addr_subnet and + self._subnet_check_ip_allocations(context, id)): + # allocation found and it was DHCP port + # that appeared after autodelete ports were + # removed - need to restart whole operation + raise os_db_exception.RetryRequest( + exc.SubnetInUse(subnet_id=id)) network = self.get_network(context, subnet['network_id']) mech_context = driver_context.SubnetContext(self, context, subnet, @@ -951,7 +961,8 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, LOG.debug("Committing transaction") break - for a in allocated: + for a in to_deallocate: + deallocated.add(a) if a.port: # calling update_port() for each allocation to remove the # IP from the port and call the MechanismDrivers diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index 03c0b409361..619855597a0 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -431,6 +431,12 @@ class TestMl2SubnetsV2(test_plugin.TestSubnetsV2, req = self.new_delete_request('subnets', subnet_id) res = req.get_response(self.api) self.assertEqual(204, res.status_int) + # Validate chat check is called twice, i.e. after the + # first check transaction gets restarted. + calls = [mock.call(mock.ANY, subnet_id), + mock.call(mock.ANY, subnet_id)] + plugin._subnet_check_ip_allocations.assert_has_calls = ( + calls) class TestMl2DbOperationBounds(test_plugin.DbOperationBoundMixin,