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 3a2e41b)
(cherry picked from commit 71686cd)

neutron/plugins/ml2/plugin.py

@@ -879,6 +879,7 @@ def delete_subnet(self, context, id):
 
         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 @@ def delete_subnet(self, context, id):
                     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 @@ def delete_subnet(self, context, id):
                     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

neutron/tests/unit/plugins/ml2/test_plugin.py

@@ -431,6 +431,12 @@ def check_and_create_ports(context, subnet_id):
                     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,