Auto-delete dhcp ports on segment delete

Subnet delete triggers dhcp port deletion but asynchronously,
therefore in the condition described in the bug report we may
get a conflict when deleting the segment too fast after the subnet.

Here we follow the example of how we auto-delete ports in net delete.

Please also find a fullstack test in Related-Change below.

Change-Id: Iba02f5a2211b18c2deb9097daad6be5e7d21faf8
Closes-Bug: #1878632
Related-Change: https://review.opendev.org/728904

neutron/common/_constants.py

@@ -67,3 +67,12 @@ IPTABLES_RANDOM_FULLY_VERSION = '1.6.2'
 
 # Segmentation ID pool; DB select limit to improve the performace.
 IDPOOL_SELECT_SIZE = 100
+
+# Ports with the following 'device_owner' values will not prevent
+# network deletion.  If delete_network() finds that all ports on a
+# network have these owners, it will explicitly delete each port
+# and allow network deletion to continue.  Similarly, if delete_subnet()
+# finds out that all existing IP Allocations are associated with ports
+# with these owners, it will allow subnet deletion to proceed with the
+# IP allocations being cleaned up by cascade.
+AUTO_DELETE_PORT_OWNERS = [constants.DEVICE_OWNER_DHCP]

neutron/db/db_base_plugin_v2.py

@@ -46,6 +46,7 @@ from sqlalchemy import not_
 
 from neutron._i18n import _
 from neutron.api.rpc.agentnotifiers import l3_rpc_agent_api
+from neutron.common import _constants
 from neutron.common import ipv6_utils
 from neutron.common import utils
 from neutron.db import db_base_plugin_common
@@ -68,15 +69,6 @@ from neutron.objects import subnetpool as subnetpool_obj
 
 LOG = logging.getLogger(__name__)
 
-# Ports with the following 'device_owner' values will not prevent
-# network deletion.  If delete_network() finds that all ports on a
-# network have these owners, it will explicitly delete each port
-# and allow network deletion to continue.  Similarly, if delete_subnet()
-# finds out that all existing IP Allocations are associated with ports
-# with these owners, it will allow subnet deletion to proceed with the
-# IP allocations being cleaned up by cascade.
-AUTO_DELETE_PORT_OWNERS = [constants.DEVICE_OWNER_DHCP]
-
 
 def _ensure_subnet_not_used(context, subnet_id):
     models_v2.Subnet.lock_register(
@@ -472,7 +464,8 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon,
     def _ensure_network_not_in_use(self, context, net_id):
         non_auto_ports = context.session.query(
             models_v2.Port.id).filter_by(network_id=net_id).filter(
-            ~models_v2.Port.device_owner.in_(AUTO_DELETE_PORT_OWNERS))
+            ~models_v2.Port.device_owner.in_(
+                _constants.AUTO_DELETE_PORT_OWNERS))
         if non_auto_ports.count():
             raise exc.NetworkInUse(net_id=net_id)
 
@@ -485,7 +478,8 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon,
         with db_api.CONTEXT_READER.using(context):
             auto_delete_port_ids = [p.id for p in context.session.query(
                 models_v2.Port.id).filter_by(network_id=id).filter(
-                models_v2.Port.device_owner.in_(AUTO_DELETE_PORT_OWNERS))]
+                models_v2.Port.device_owner.in_(
+                    _constants.AUTO_DELETE_PORT_OWNERS))]
         for port_id in auto_delete_port_ids:
             try:
                 self.delete_port(context.elevated(), port_id)
@@ -1002,7 +996,7 @@ class NeutronDbPluginV2(db_base_plugin_common.DbBasePluginCommon,
     def _subnet_get_user_allocation(self, context, subnet_id):
         """Check if there are any user ports on subnet and return first."""
         return port_obj.IPAllocation.get_alloc_by_subnet_id(
-            context, subnet_id, AUTO_DELETE_PORT_OWNERS)
+            context, subnet_id, _constants.AUTO_DELETE_PORT_OWNERS)
 
     def _subnet_check_ip_allocations_internal_router_ports(self, context,
                                                            subnet_id):

neutron/objects/ports.py

@@ -20,6 +20,7 @@ from oslo_log import log as logging
 from oslo_utils import versionutils
 from oslo_versionedobjects import fields as obj_fields
 
+from neutron.common import _constants
 from neutron.common import utils
 from neutron.db.models import dns as dns_models
 from neutron.db.models import l3
@@ -427,14 +428,25 @@ class Port(base.NeutronDbObject):
         return port_array
 
     @classmethod
-    def get_port_ids_filter_by_segment_id(cls, context, segment_id):
+    def get_auto_deletable_port_ids_and_proper_port_count_by_segment(
+            cls, context, segment_id):
+
         query = context.session.query(models_v2.Port.id)
         query = query.join(
             ml2_models.PortBindingLevel,
             ml2_models.PortBindingLevel.port_id == models_v2.Port.id)
         query = query.filter(
             ml2_models.PortBindingLevel.segment_id == segment_id)
-        return [p.id for p in query]
+
+        q_delete = query.filter(
+            models_v2.Port.device_owner.in_(
+                _constants.AUTO_DELETE_PORT_OWNERS))
+
+        q_proper = query.filter(
+            ~models_v2.Port.device_owner.in_(
+                _constants.AUTO_DELETE_PORT_OWNERS))
+
+        return ([r.id for r in q_delete.all()], q_proper.count())
 
     @classmethod
     def modify_fields_to_db(cls, fields):

neutron/plugins/ml2/db.py

@@ -19,6 +19,7 @@ from neutron_lib.callbacks import registry
 from neutron_lib.callbacks import resources
 from neutron_lib import constants as n_const
 from neutron_lib.db import api as db_api
+from neutron_lib import exceptions as nlib_exc
 from neutron_lib.plugins import directory
 from oslo_db import exception as db_exc
 from oslo_log import log
@@ -322,17 +323,28 @@ def _prevent_segment_delete_with_port_bound(resource, event, trigger,
         return
 
     with db_api.CONTEXT_READER.using(payload.context):
-        port_ids = port_obj.Port.get_port_ids_filter_by_segment_id(
-            payload.context, segment_id=payload.resource_id)
-
-    # There are still some ports in the segment, segment should not be deleted
-    # TODO(xiaohhui): Should we delete the dhcp port automatically here?
-    if port_ids:
-        reason = _("The segment is still bound with port(s) "
-                   "%s") % ", ".join(port_ids)
+        auto_delete_port_ids, proper_port_count = port_obj.Port.\
+            get_auto_deletable_port_ids_and_proper_port_count_by_segment(
+                payload.context, segment_id=payload.resource_id)
+
+    if proper_port_count:
+        reason = (_("The segment is still bound with %s port(s)") %
+                  (proper_port_count + len(auto_delete_port_ids)))
         raise seg_exc.SegmentInUse(segment_id=payload.resource_id,
                                    reason=reason)
 
+    if auto_delete_port_ids:
+        LOG.debug("Auto-deleting dhcp port(s) on segment %s: %s",
+            payload.resource_id, ", ".join(auto_delete_port_ids))
+        plugin = directory.get_plugin()
+    for port_id in auto_delete_port_ids:
+        try:
+            plugin.delete_port(payload.context.elevated(), port_id)
+        except nlib_exc.PortNotFound:
+            # Don't raise if something else concurrently deleted the port
+            LOG.debug("Ignoring PortNotFound when deleting port '%s'. "
+                      "The port has already been deleted.", port_id)
+
 
 def subscribe():
     registry.subscribe(_prevent_segment_delete_with_port_bound,

neutron/tests/unit/objects/test_ports.py

@@ -568,3 +568,32 @@ class PortDbObjectTestCase(obj_test_base.BaseDbObjectTestCase,
                                                                   subnet_id)
         self.assertEqual(1, len(ports_alloc))
         self.assertEqual(objs[0].id, ports_alloc[0].id)
+
+    def _test_get_auto_deletable_ports(self, device_owner):
+        network_id = self._create_test_network_id()
+        segment_id = self._create_test_segment_id(network_id)
+        port = self._create_test_port(device_owner=device_owner)
+        binding = ports.PortBindingLevel(
+            self.context, port_id=port.id,
+            host='host1', level=0, segment_id=segment_id)
+        binding.create()
+        return (
+            ports.Port.
+            get_auto_deletable_port_ids_and_proper_port_count_by_segment(
+                self.context, segment_id))
+
+    def test_get_auto_deletable_ports_dhcp(self):
+        dhcp_ports, count = self._test_get_auto_deletable_ports(
+            'network:dhcp')
+        self.assertEqual(
+            (1, 0),
+            (len(dhcp_ports), count),
+        )
+
+    def test_get_auto_deletable_ports_not_dhcp(self):
+        dhcp_ports, count = self._test_get_auto_deletable_ports(
+            'not_network_dhcp')
+        self.assertEqual(
+            (0, 1),
+            (len(dhcp_ports), count),
+        )

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

@@ -22,6 +22,7 @@ from neutron_lib.api.definitions import portbindings
 from neutron_lib import constants
 from neutron_lib import context
 from neutron_lib.db import api as db_api
+from neutron_lib.plugins import directory
 from neutron_lib.plugins.ml2 import api
 from oslo_utils import uuidutils
 from sqlalchemy.orm import exc
@@ -34,6 +35,7 @@ from neutron.objects import network as network_obj
 from neutron.objects import ports as port_obj
 from neutron.plugins.ml2 import db as ml2_db
 from neutron.plugins.ml2 import models
+from neutron.services.segments import exceptions as seg_exc
 from neutron.tests.unit import testlib_api
 
 
@@ -300,6 +302,43 @@ class Ml2DBTestCase(testlib_api.SqlTestCase):
         for mac in macs:
             self.assertIsNotNone(re.search(mac_regex, mac))
 
+    def test__prevent_segment_delete_with_port_bound_raise(self):
+        payload_mock = mock.Mock()
+        payload_mock.metadata.get.return_value = False
+        payload_mock.context = self.ctx
+        with mock.patch.object(
+                 port_obj.Port,
+                 'get_auto_deletable_port_ids_and_proper_port_count_by_segment'
+             ) as mock_get:
+            mock_get.return_value = ([], 1)
+            self.assertRaises(
+                seg_exc.SegmentInUse,
+                ml2_db._prevent_segment_delete_with_port_bound,
+                resource=mock.Mock(),
+                event=mock.Mock(),
+                trigger=mock.Mock(),
+                payload=payload_mock,
+            )
+
+    def test__prevent_segment_delete_with_port_bound_auto_delete(self):
+        payload_mock = mock.Mock()
+        payload_mock.metadata.get.return_value = False
+        payload_mock.context = self.ctx
+        plugin = directory.get_plugin()
+        with mock.patch.object(
+                 port_obj.Port,
+                 'get_auto_deletable_port_ids_and_proper_port_count_by_segment'
+             ) as mock_get, \
+                mock.patch.object(plugin, 'delete_port') as mock_delete_port:
+            mock_get.return_value = (['fake-port'], 0)
+            ml2_db._prevent_segment_delete_with_port_bound(
+                resource=mock.Mock(),
+                event=mock.Mock(),
+                trigger=mock.Mock(),
+                payload=payload_mock,
+            )
+            mock_delete_port.assert_called_with(mock.ANY, 'fake-port')
+
 
 class Ml2DvrDBTestCase(testlib_api.SqlTestCase):