From b4130c0dc1787e30a70ab33486047fee720ab286 Mon Sep 17 00:00:00 2001 From: Bence Romsics Date: Tue, 19 May 2020 13:21:52 +0200 Subject: [PATCH] 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 (cherry picked from commit da45bbbff4e10e3d924ac3d85c9f382623708b11) --- neutron/common/_constants.py | 9 ++++++ neutron/db/db_base_plugin_v2.py | 18 ++++------- neutron/objects/ports.py | 16 ++++++++-- neutron/plugins/ml2/db.py | 26 +++++++++++---- neutron/tests/unit/objects/test_ports.py | 29 +++++++++++++++++ neutron/tests/unit/plugins/ml2/test_db.py | 39 +++++++++++++++++++++++ 6 files changed, 116 insertions(+), 21 deletions(-) diff --git a/neutron/common/_constants.py b/neutron/common/_constants.py index 5673d3af5bb..2260441c1e0 100644 --- a/neutron/common/_constants.py +++ b/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] diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index c983df7dc69..2d9ba772132 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/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): diff --git a/neutron/objects/ports.py b/neutron/objects/ports.py index 8527d9f6408..206feabb0fb 100644 --- a/neutron/objects/ports.py +++ b/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.db.models import dns as dns_models from neutron.db.models import l3 from neutron.db.models import securitygroup as sg_models @@ -423,14 +424,25 @@ class Port(base.NeutronDbObject): **kwargs) @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): diff --git a/neutron/plugins/ml2/db.py b/neutron/plugins/ml2/db.py index 5eae1daffa1..0a038c3898a 100644 --- a/neutron/plugins/ml2/db.py +++ b/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) + 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) - # 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) + 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, diff --git a/neutron/tests/unit/objects/test_ports.py b/neutron/tests/unit/objects/test_ports.py index d39d5438100..ccb519c3130 100644 --- a/neutron/tests/unit/objects/test_ports.py +++ b/neutron/tests/unit/objects/test_ports.py @@ -573,3 +573,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), + ) diff --git a/neutron/tests/unit/plugins/ml2/test_db.py b/neutron/tests/unit/plugins/ml2/test_db.py index 241b4ef6f5d..bbebd1f6701 100644 --- a/neutron/tests/unit/plugins/ml2/test_db.py +++ b/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):