From 833a6d82cd705548130cdac73a88d388f52c7824 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 30 May 2023 11:45:37 +0200 Subject: [PATCH] [OVN] Prevent Trunk creation/deletion with parent port bound This patch imitates the ML2/OVS Trunk driver behaviour. When the trunk parent port is bound: * A new trunk cannot be created using this parent port. * If the port is assigned as parent port of a trunk, this trunk cannot be deleted. Closes-Bug: #2022059 Change-Id: I8cfa7e67524a42224cbb4b3c3cec3cfa49b795fd --- neutron/common/utils.py | 16 ++++++++++ neutron/db/l3_dvr_db.py | 15 +-------- .../trunk/drivers/ovn/trunk_driver.py | 19 +++++++++++- neutron/services/trunk/plugin.py | 5 +-- .../trunk/drivers/ovn/test_trunk_driver.py | 31 +++++++++++++++++++ neutron/tests/unit/db/test_l3_dvr_db.py | 3 +- .../tests/unit/services/trunk/test_plugin.py | 3 +- ...nk-check-parent-port-eeca2eceaca9d158.yaml | 6 ++++ 8 files changed, 79 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/ovn-trunk-check-parent-port-eeca2eceaca9d158.yaml diff --git a/neutron/common/utils.py b/neutron/common/utils.py index 5201a9e6b52..31e562c4b51 100644 --- a/neutron/common/utils.py +++ b/neutron/common/utils.py @@ -37,9 +37,12 @@ import eventlet from eventlet.green import subprocess import netaddr from neutron_lib.api.definitions import availability_zone as az_def +from neutron_lib.api.definitions import portbindings +from neutron_lib.api.definitions import portbindings_extended from neutron_lib import constants as n_const from neutron_lib import context as n_context from neutron_lib.db import api as db_api +from neutron_lib.plugins import utils as plugin_utils from neutron_lib.services.qos import constants as qos_consts from neutron_lib.services.trunk import constants as trunk_constants from neutron_lib.utils import helpers @@ -1108,3 +1111,16 @@ def parse_permitted_ethertypes(permitted_ethertypes): continue return ret + + +# TODO(slaweq): this should be moved to neutron_lib.plugins.utils module +def is_port_bound(port, log_message=True): + active_binding = plugin_utils.get_port_binding_by_status_and_host( + port.get('port_bindings', []), n_const.ACTIVE) + if not active_binding: + if log_message: + LOG.warning('Binding for port %s was not found.', port) + return False + return active_binding[portbindings_extended.VIF_TYPE] not in ( + portbindings.VIF_TYPE_UNBOUND, + portbindings.VIF_TYPE_BINDING_FAILED) diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py index 2f008bf6f1b..0c81929acc5 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -17,7 +17,6 @@ import netaddr from neutron_lib.api.definitions import external_net as extnet_apidef from neutron_lib.api.definitions import l3 as l3_apidef from neutron_lib.api.definitions import portbindings -from neutron_lib.api.definitions import portbindings_extended from neutron_lib.api.definitions import router_admin_state_down_before_update from neutron_lib.api import validators from neutron_lib.callbacks import events @@ -71,18 +70,6 @@ def is_admin_state_down_necessary(): return _IS_ADMIN_STATE_DOWN_NECESSARY -# TODO(slaweq): this should be moved to neutron_lib.plugins.utils module -def is_port_bound(port): - active_binding = plugin_utils.get_port_binding_by_status_and_host( - port.get("port_bindings", []), const.ACTIVE) - if not active_binding: - LOG.warning("Binding for port %s was not found.", port) - return False - return active_binding[portbindings_extended.VIF_TYPE] not in [ - portbindings.VIF_TYPE_UNBOUND, - portbindings.VIF_TYPE_BINDING_FAILED] - - @registry.has_registry_receivers class DVRResourceOperationHandler(object): """Contains callbacks for DVR operations. @@ -1428,7 +1415,7 @@ class L3_NAT_with_dvr_db_mixin(_DVRAgentInterfaceMixin, def get_ports_under_dvr_connected_subnet(self, context, subnet_id): ports = dvr_mac_db.get_ports_query_by_subnet_and_ip(context, subnet_id) - ports = [p for p in ports if is_port_bound(p)] + ports = [p for p in ports if n_utils.is_port_bound(p)] # TODO(slaweq): if there would be way to pass to neutron-lib only # list of extensions which actually should be processed, than setting # process_extensions=True below could avoid that second loop and diff --git a/neutron/services/trunk/drivers/ovn/trunk_driver.py b/neutron/services/trunk/drivers/ovn/trunk_driver.py index 6d248aff79e..aaf03cee607 100644 --- a/neutron/services/trunk/drivers/ovn/trunk_driver.py +++ b/neutron/services/trunk/drivers/ovn/trunk_driver.py @@ -22,10 +22,12 @@ from oslo_config import cfg from oslo_log import log from neutron.common.ovn import constants as ovn_const +from neutron.common import utils as n_utils from neutron.db import db_base_plugin_common from neutron.db import ovn_revision_numbers_db as db_rev from neutron.objects import ports as port_obj from neutron.services.trunk.drivers import base as trunk_base +from neutron.services.trunk import exceptions as trunk_exc SUPPORTED_INTERFACES = ( @@ -162,6 +164,10 @@ class OVNTrunkHandler(object): LOG.debug("Done unsetting parent for subport %s", subport.port_id) return db_port + @staticmethod + def _is_port_bound(port): + return n_utils.is_port_bound(port, log_message=False) + def trunk_updated(self, trunk): # Check if parent port is handled by OVN. if not self.plugin_driver.nb_ovn.lookup('Logical_Switch_Port', @@ -208,6 +214,16 @@ class OVNTrunkHandler(object): self.trunk_updated(payload.states[0]) elif event == events.AFTER_DELETE: self.trunk_deleted(payload.states[0]) + elif event == events.PRECOMMIT_CREATE: + trunk = payload.desired_state + parent_port = trunk.db_obj.port + if self._is_port_bound(parent_port): + raise trunk_exc.ParentPortInUse(port_id=parent_port.id) + elif event == events.PRECOMMIT_DELETE: + trunk = payload.states[0] + parent_port = payload.states[1] + if self._is_port_bound(parent_port): + raise trunk_exc.TrunkInUse(trunk_id=trunk.id) def subport_event(self, resource, event, trunk_plugin, payload): if event == events.AFTER_CREATE: @@ -233,7 +249,8 @@ class OVNTrunkDriver(trunk_base.DriverBase): resource, event, trigger, payload=payload) self._handler = OVNTrunkHandler(self.plugin_driver) for _event in (events.AFTER_CREATE, events.AFTER_UPDATE, - events.AFTER_DELETE): + events.AFTER_DELETE, events.PRECOMMIT_CREATE, + events.PRECOMMIT_DELETE): registry.subscribe(self._handler.trunk_event, resources.TRUNK, _event) diff --git a/neutron/services/trunk/plugin.py b/neutron/services/trunk/plugin.py index 4a652600c7e..8f72cce5688 100644 --- a/neutron/services/trunk/plugin.py +++ b/neutron/services/trunk/plugin.py @@ -294,6 +294,7 @@ class TrunkPlugin(service_base.ServicePluginBase): trunk = self._get_trunk(context, trunk_id) rules.trunk_can_be_managed(context, trunk) trunk_port_validator = rules.TrunkPortValidator(trunk.port_id) + parent_port = trunk.db_obj.port if trunk_port_validator.can_be_trunked_or_untrunked(context): # NOTE(status_police): when a trunk is deleted, the logical # object disappears from the datastore, therefore there is no @@ -307,7 +308,7 @@ class TrunkPlugin(service_base.ServicePluginBase): 'deleting trunk port %s: %s', trunk_id, str(e)) payload = events.DBEventPayload(context, resource_id=trunk_id, - states=(trunk,)) + states=(trunk, parent_port)) registry.publish(resources.TRUNK, events.PRECOMMIT_DELETE, self, payload=payload) else: @@ -317,7 +318,7 @@ class TrunkPlugin(service_base.ServicePluginBase): registry.publish(resources.TRUNK, events.AFTER_DELETE, self, payload=events.DBEventPayload( context, resource_id=trunk_id, - states=(trunk,))) + states=(trunk, parent_port))) @db_base_plugin_common.convert_result_to_dict def add_subports(self, context, trunk_id, subports): diff --git a/neutron/tests/functional/services/trunk/drivers/ovn/test_trunk_driver.py b/neutron/tests/functional/services/trunk/drivers/ovn/test_trunk_driver.py index 1c19b38b282..099c7d63bdb 100644 --- a/neutron/tests/functional/services/trunk/drivers/ovn/test_trunk_driver.py +++ b/neutron/tests/functional/services/trunk/drivers/ovn/test_trunk_driver.py @@ -15,6 +15,7 @@ import contextlib from neutron_lib.api.definitions import portbindings +from neutron_lib.callbacks import exceptions as n_exc from neutron_lib import constants as n_consts from neutron_lib.db import api as db_api from neutron_lib.plugins import utils @@ -113,6 +114,25 @@ class TestOVNTrunkDriver(base.TestOVNFunctionalBase): with self.trunk([subport]) as trunk: self._verify_trunk_info(trunk, has_items=True) + def test_trunk_create_parent_port_bound(self): + with self.network() as network: + with self.subnet(network=network) as subnet: + with self.port(subnet=subnet) as parent_port: + pb = port_obj.PortBinding.get_objects( + self.context, port_id=parent_port['port']['id']) + port_obj.PortBinding.update_object( + self.context, {'vif_type': portbindings.VIF_TYPE_OVS}, + port_id=pb[0].port_id, host=pb[0].host) + tenant_id = uuidutils.generate_uuid() + trunk = {'trunk': { + 'port_id': parent_port['port']['id'], + 'tenant_id': tenant_id, 'project_id': tenant_id, + 'admin_state_up': True, + 'name': 'trunk', 'sub_ports': []}} + self.assertRaises(n_exc.CallbackFailure, + self.trunk_plugin.create_trunk, + self.context, trunk) + def test_subport_add(self): with self.subport() as subport: with self.trunk() as trunk: @@ -147,3 +167,14 @@ class TestOVNTrunkDriver(base.TestOVNFunctionalBase): with self.trunk() as trunk: self.trunk_plugin.delete_trunk(self.context, trunk['id']) self._verify_trunk_info({}, has_items=False) + + def test_trunk_delete_parent_port_bound(self): + with self.trunk() as trunk: + bp = port_obj.PortBinding.get_objects( + self.context, port_id=trunk['port_id']) + port_obj.PortBinding.update_object( + self.context, {'vif_type': portbindings.VIF_TYPE_OVS}, + port_id=bp[0].port_id, host=bp[0].host) + self.assertRaises(n_exc.CallbackFailure, + self.trunk_plugin.delete_trunk, + self.context, trunk['id']) diff --git a/neutron/tests/unit/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index ac3631fcb73..9c2cd24a3eb 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -30,6 +30,7 @@ from neutron_lib.plugins import directory from neutron_lib.plugins import utils as plugin_utils from oslo_utils import uuidutils +from neutron.common import utils as n_utils from neutron.db import agents_db from neutron.db import l3_dvr_db from neutron.db import l3_dvrscheduler_db @@ -1521,7 +1522,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): self.assertTrue( self.mixin.is_router_distributed(self.ctx, router_id)) - @mock.patch.object(l3_dvr_db, "is_port_bound") + @mock.patch.object(n_utils, 'is_port_bound') def test_get_ports_under_dvr_connected_subnet(self, is_port_bound_mock): router_dict = {'name': 'test_router', 'admin_state_up': True, 'distributed': True} diff --git a/neutron/tests/unit/services/trunk/test_plugin.py b/neutron/tests/unit/services/trunk/test_plugin.py index f2c26cdcef3..271c16e66db 100644 --- a/neutron/tests/unit/services/trunk/test_plugin.py +++ b/neutron/tests/unit/services/trunk/test_plugin.py @@ -162,7 +162,8 @@ class TrunkPluginTestCase(test_plugin.Ml2PluginV2TestCase): resources.TRUNK, event, self.trunk_plugin, payload=mock.ANY) payload = callback.mock_calls[0][2]['payload'] self.assertEqual(self.context, payload.context) - self.assertEqual(trunk_obj, payload.latest_state) + self.assertEqual(trunk_obj, payload.states[0]) + self.assertEqual(parent_port['port']['id'], payload.states[1].id) self.assertEqual(trunk['id'], payload.resource_id) def test_delete_trunk_notify_after_delete(self): diff --git a/releasenotes/notes/ovn-trunk-check-parent-port-eeca2eceaca9d158.yaml b/releasenotes/notes/ovn-trunk-check-parent-port-eeca2eceaca9d158.yaml new file mode 100644 index 00000000000..111dc99ca12 --- /dev/null +++ b/releasenotes/notes/ovn-trunk-check-parent-port-eeca2eceaca9d158.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Now the ML2/OVN trunk driver prevents a trunk creation if the parent port + is already bound. In the same way, if a parent port being used in a trunk + is bound, the trunk cannot be deleted.