From 966fa566e5df29289136105b70ea795890c0062e Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Thu, 16 May 2024 10:42:50 -0400 Subject: [PATCH] Revert "[OVN] Prevent Trunk creation/deletion with parent port bound" There are three reasons to revert this patch. 1. It broke RPC push API for trunks because it added port db model to event payload that is not serializeable. 2. It also broke the callback event payload interface, which requires that all entries in .states attribute belong to the same core object. To quote from neutron-lib, ``` # an iterable of states for the resource from the newest to the oldest # for example db states or api request/response # the actual object type for states will vary depending on event caller self.states = ... ``` 3. There is no good justification why ml2/ovn would not allow this operation. The rationale for the original patch was to align the behavior with ml2/ovs, but we don't such parity requirements. The 409 error that can be returned by the API endpoints is backend specific. To quote api-ref, ``` 409 The operation returns this error code for one of these reasons: A system configuration prevents the operation from succeeding. ``` AFAIU there is nothing that prevents ml2/ovn to create a trunk in this situation. This will have to be backported in all supported branches (the original patch was backported down to Wallaby). Conflicts: neutron/services/trunk/drivers/ovn/trunk_driver.py This reverts commit 833a6d82cd705548130cdac73a88d388f52c7824. Closes-Bug: #2065707 Related-Bug: #2022059 Change-Id: I067c2f7286b2684b67b4389ca085d06a93f856ce (cherry picked from commit ac15191f88a63bd5e0510c3602fb6d19c9ac1c92) --- neutron/common/utils.py | 16 --------- neutron/db/l3_dvr_db.py | 15 ++++++++- .../trunk/drivers/ovn/trunk_driver.py | 26 --------------- neutron/services/trunk/plugin.py | 5 ++- .../trunk/drivers/ovn/test_trunk_driver.py | 33 ------------------- neutron/tests/unit/db/test_l3_dvr_db.py | 3 +- .../trunk/drivers/ovn/test_trunk_driver.py | 4 --- .../tests/unit/services/trunk/test_plugin.py | 3 +- ...nk-check-parent-port-eeca2eceaca9d158.yaml | 6 ---- 9 files changed, 18 insertions(+), 93 deletions(-) delete mode 100644 releasenotes/notes/ovn-trunk-check-parent-port-eeca2eceaca9d158.yaml diff --git a/neutron/common/utils.py b/neutron/common/utils.py index 412489cf67f..3ac808ba5aa 100644 --- a/neutron/common/utils.py +++ b/neutron/common/utils.py @@ -37,12 +37,9 @@ 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 @@ -1106,16 +1103,3 @@ 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 5b4f1d99532..1f3b3eb8962 100644 --- a/neutron/db/l3_dvr_db.py +++ b/neutron/db/l3_dvr_db.py @@ -17,6 +17,7 @@ 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 @@ -70,6 +71,18 @@ 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. @@ -1422,7 +1435,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 n_utils.is_port_bound(p)] + ports = [p for p in ports if 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 d0228844ddb..f57e45a858d 100644 --- a/neutron/services/trunk/drivers/ovn/trunk_driver.py +++ b/neutron/services/trunk/drivers/ovn/trunk_driver.py @@ -22,12 +22,10 @@ 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 = ( @@ -157,10 +155,6 @@ 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_created(self, resource, event, trunk_plugin, payload): trunk = payload.states[0] # Check if parent port is handled by OVN. @@ -176,18 +170,6 @@ class OVNTrunkHandler(object): if trunk.sub_ports: self._unset_sub_ports(trunk.sub_ports) - def trunk_created_precommit(self, resource, event, trunk_plugin, payload): - # payload.desired_state below is the trunk object - parent_port = payload.desired_state.db_obj.port - if self._is_port_bound(parent_port): - raise trunk_exc.ParentPortInUse(port_id=parent_port.id) - - def trunk_deleted_precommit(self, resource, event, trunk_plugin, payload): - 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 subports_added(self, resource, event, trunk_plugin, payload): trunk = payload.states[0] subports = payload.metadata['subports'] @@ -226,14 +208,6 @@ class OVNTrunkDriver(trunk_base.DriverBase): resource, event, trigger, payload=payload) self._handler = OVNTrunkHandler(self.plugin_driver) - registry.subscribe( - self._handler.trunk_created_precommit, - resources.TRUNK, - events.PRECOMMIT_CREATE) - registry.subscribe( - self._handler.trunk_deleted_precommit, - resources.TRUNK, - events.PRECOMMIT_DELETE) registry.subscribe( self._handler.trunk_created, resources.TRUNK, events.AFTER_CREATE) registry.subscribe( diff --git a/neutron/services/trunk/plugin.py b/neutron/services/trunk/plugin.py index d19515777db..edb98ef5dea 100644 --- a/neutron/services/trunk/plugin.py +++ b/neutron/services/trunk/plugin.py @@ -294,7 +294,6 @@ 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 @@ -308,7 +307,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, parent_port)) + states=(trunk,)) registry.publish(resources.TRUNK, events.PRECOMMIT_DELETE, self, payload=payload) else: @@ -318,7 +317,7 @@ class TrunkPlugin(service_base.ServicePluginBase): registry.publish(resources.TRUNK, events.AFTER_DELETE, self, payload=events.DBEventPayload( context, resource_id=trunk_id, - states=(trunk, parent_port))) + states=(trunk,))) @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 fa281232c42..39d5eb6cd6e 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 @@ -14,8 +14,6 @@ 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.objects import registry as obj_reg from neutron_lib.plugins import utils @@ -23,7 +21,6 @@ from neutron_lib.services.trunk import constants as trunk_consts from oslo_utils import uuidutils from neutron.common.ovn import constants as ovn_const -from neutron.objects import ports as port_obj from neutron.services.trunk import plugin as trunk_plugin from neutron.tests.functional import base @@ -108,25 +105,6 @@ 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: @@ -149,14 +127,3 @@ 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 6767127bc0d..78171d8366f 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -30,7 +30,6 @@ 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 @@ -1511,7 +1510,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): self.assertTrue( self.mixin.is_router_distributed(self.ctx, router_id)) - @mock.patch.object(n_utils, 'is_port_bound') + @mock.patch.object(l3_dvr_db, "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/drivers/ovn/test_trunk_driver.py b/neutron/tests/unit/services/trunk/drivers/ovn/test_trunk_driver.py index b036778877e..f98e16556be 100644 --- a/neutron/tests/unit/services/trunk/drivers/ovn/test_trunk_driver.py +++ b/neutron/tests/unit/services/trunk/drivers/ovn/test_trunk_driver.py @@ -454,10 +454,6 @@ class TestTrunkDriver(base.BaseTestCase): with mock.patch.object(registry, 'subscribe') as mock_subscribe: driver.register(mock.ANY, mock.ANY, mock.Mock()) calls = [ - mock.call.mock_subscribe( - mock.ANY, resources.TRUNK, events.PRECOMMIT_CREATE), - mock.call.mock_subscribe( - mock.ANY, resources.TRUNK, events.PRECOMMIT_DELETE), mock.call.mock_subscribe( mock.ANY, resources.TRUNK, events.AFTER_CREATE), mock.call.mock_subscribe( diff --git a/neutron/tests/unit/services/trunk/test_plugin.py b/neutron/tests/unit/services/trunk/test_plugin.py index 271c16e66db..f2c26cdcef3 100644 --- a/neutron/tests/unit/services/trunk/test_plugin.py +++ b/neutron/tests/unit/services/trunk/test_plugin.py @@ -162,8 +162,7 @@ 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.states[0]) - self.assertEqual(parent_port['port']['id'], payload.states[1].id) + self.assertEqual(trunk_obj, payload.latest_state) 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 deleted file mode 100644 index 111dc99ca12..00000000000 --- a/releasenotes/notes/ovn-trunk-check-parent-port-eeca2eceaca9d158.yaml +++ /dev/null @@ -1,6 +0,0 @@ ---- -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.