From e69505d293ef12999f948531acbde75e16a65cd4 Mon Sep 17 00:00:00 2001 From: Bence Romsics Date: Wed, 7 May 2025 16:29:58 +0200 Subject: [PATCH] Limit trunk ACTIVE state hack to OVN In https://review.opendev.org/c/openstack/neutron/+/853779 we started moving a trunk to ACTIVE when its parent port went to ACTIVE. The intention was to not leave the trunk in DOWN after a live migration as reported in #1988549. However this had side effects. Earlier we moved a trunk to ACTIVE when all of its ports were processed. That means we unintentionally changed the meaning of the trunk ACTIVE status. This affected all backends and not just live migrate but create too. This change moves the logic of propagating the trunk parent's ACTIVE to the trunk itself to the OVN trunk driver, so we limit the undesired effects to ml2/ovn. By that we restore the original meaning of trunk ACTIVE for all non-OVN backends. Ideally we would want to limit the effect to live migrate (so we don't affect create) but I did not find a way to do that. Change-Id: I4d2c3db355e29fffcce0f50cd12bb1e31d1be43a Closes-Bug: #2095152 Related-Bug: #1988549 Related-Change: https://review.opendev.org/c/openstack/os-vif/+/949736 --- .../trunk/drivers/ovn/trunk_driver.py | 48 +++++++++++++ neutron/services/trunk/plugin.py | 10 +-- .../trunk/drivers/ovn/test_trunk_driver.py | 69 +++++++++++++++++++ .../tests/unit/services/trunk/test_plugin.py | 27 -------- 4 files changed, 118 insertions(+), 36 deletions(-) diff --git a/neutron/services/trunk/drivers/ovn/trunk_driver.py b/neutron/services/trunk/drivers/ovn/trunk_driver.py index 2195c70e190..808e991b7d1 100644 --- a/neutron/services/trunk/drivers/ovn/trunk_driver.py +++ b/neutron/services/trunk/drivers/ovn/trunk_driver.py @@ -14,6 +14,7 @@ from neutron_lib.api.definitions import portbindings from neutron_lib.callbacks import events from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources +from neutron_lib import constants from neutron_lib import context as n_context from neutron_lib.db import api as db_api from neutron_lib import exceptions as n_exc @@ -25,7 +26,9 @@ from neutron.common.ovn import constants as ovn_const 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.objects import trunk as trunk_objects from neutron.services.trunk.drivers import base as trunk_base +from neutron.services.trunk import exceptions as trunk_exc SUPPORTED_INTERFACES = ( @@ -192,6 +195,46 @@ class OVNTrunkHandler: self._unset_sub_ports(subports) trunk.update(status=trunk_consts.TRUNK_ACTIVE_STATUS) + def port_updated(self, resource, event, trunk_plugin, payload): + '''Propagate trunk parent port ACTIVE to trunk ACTIVE + + During a live migration with a trunk the only way we found to update + the trunk to ACTIVE is to do this when the trunk's parent port gets + updated to ACTIVE. This is clearly suboptimal, because the trunk's + ACTIVE status should mean that all of its ports (parent and sub) are + active. But in ml2/ovn the parent port's binding is not cascaded to the + subports. Actually the subports' binding:host is left empty. This way + here we don't know anything about the subports' state changes during a + live migration. If we don't want to leave the trunk in DOWN this is + what we have. + + Please note that this affects trunk create as well. Because of this we + move ml2/ovn trunks to ACTIVE way early. But at least here we don't + affect other mechanism drivers and their corresponding trunk drivers. + + See also: + https://bugs.launchpad.net/neutron/+bug/1988549 + https://review.opendev.org/c/openstack/neutron/+/853779 + https://bugs.launchpad.net/neutron/+bug/2095152 + ''' + updated_port = payload.latest_state + trunk_details = updated_port.get('trunk_details') + # If no trunk_details, the port is not the parent of a trunk. + if not trunk_details: + return + + original_port = payload.states[0] + orig_status = original_port.get('status') + new_status = updated_port.get('status') + context = payload.context + trunk_id = trunk_details['trunk_id'] + if (new_status == constants.PORT_STATUS_ACTIVE and + new_status != orig_status): + trunk = trunk_objects.Trunk.get_object(context, id=trunk_id) + if trunk is None: + raise trunk_exc.TrunkNotFound(trunk_id=trunk_id) + trunk.update(status=trunk_consts.TRUNK_ACTIVE_STATUS) + class OVNTrunkDriver(trunk_base.DriverBase): @property @@ -222,6 +265,11 @@ class OVNTrunkDriver(trunk_base.DriverBase): resources.SUBPORTS, events.AFTER_DELETE) + registry.subscribe( + self._handler.port_updated, + resources.PORT, + events.AFTER_UPDATE) + @classmethod def create(cls, plugin_driver): cls.plugin_driver = plugin_driver diff --git a/neutron/services/trunk/plugin.py b/neutron/services/trunk/plugin.py index d2f005171fb..c92fc63fb42 100644 --- a/neutron/services/trunk/plugin.py +++ b/neutron/services/trunk/plugin.py @@ -21,7 +21,6 @@ from neutron_lib.api.definitions import trunk_details from neutron_lib.callbacks import events from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources -from neutron_lib import constants as const from neutron_lib import context from neutron_lib.db import api as db_api from neutron_lib.db import resource_extend @@ -471,19 +470,12 @@ class TrunkPlugin(service_base.ServicePluginBase): original_port = payload.states[0] orig_vif_type = original_port.get(portbindings.VIF_TYPE) new_vif_type = updated_port.get(portbindings.VIF_TYPE) - orig_status = original_port.get('status') - new_status = updated_port.get('status') vif_type_changed = orig_vif_type != new_vif_type - trunk_id = trunk_details['trunk_id'] if vif_type_changed and new_vif_type == portbindings.VIF_TYPE_UNBOUND: + trunk_id = trunk_details['trunk_id'] # NOTE(status_police) Trunk status goes to DOWN when the parent # port is unbound. This means there are no more physical resources # associated with the logical resource. self.update_trunk( context, trunk_id, {'trunk': {'status': constants.TRUNK_DOWN_STATUS}}) - elif new_status == const.PORT_STATUS_ACTIVE and \ - new_status != orig_status: - self.update_trunk( - context, trunk_id, - {'trunk': {'status': constants.TRUNK_ACTIVE_STATUS}}) 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 c6fc24b519d..14b35bacb56 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 @@ -18,6 +18,7 @@ from neutron_lib.api.definitions import portbindings from neutron_lib.callbacks import events from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources +from neutron_lib import constants as nlib_consts from neutron_lib import exceptions as n_exc from neutron_lib.services.trunk import constants as trunk_consts from oslo_config import cfg @@ -25,9 +26,13 @@ from oslo_config import cfg from neutron.common.ovn.constants import OVN_ML2_MECH_DRIVER_NAME from neutron.objects.ports import Port from neutron.objects.ports import PortBinding +from neutron.objects import trunk as trunk_objects +from neutron.services.trunk import drivers from neutron.services.trunk.drivers.ovn import trunk_driver +from neutron.services.trunk import plugin as trunk_plugin from neutron.tests import base from neutron.tests.unit import fake_resources +from neutron.tests.unit.plugins.ml2 import test_plugin class FakePayload: @@ -430,6 +435,70 @@ class TestTrunkHandler(base.BaseTestCase): m__unset_sub_ports.assert_not_called() +class TestTrunkHandlerWithPlugin(test_plugin.Ml2PluginV2TestCase): + def setUp(self): + super().setUp() + self.drivers_patch = mock.patch.object(drivers, 'register').start() + self.compat_patch = mock.patch.object( + trunk_plugin.TrunkPlugin, 'check_compatibility').start() + self.trunk_plugin = trunk_plugin.TrunkPlugin() + self.trunk_plugin.add_segmentation_type('vlan', lambda x: True) + self.plugin_driver = mock.Mock() + self.trunk_handler = trunk_driver.OVNTrunkHandler(self.plugin_driver) + + def _create_test_trunk(self, port, subports=None): + subports = subports if subports else [] + trunk = {'port_id': port['port']['id'], + 'project_id': 'test_tenant', + 'sub_ports': subports} + response = ( + self.trunk_plugin.create_trunk(self.context, {'trunk': trunk})) + return response + + def _get_trunk_obj(self, trunk_id): + return trunk_objects.Trunk.get_object(self.context, id=trunk_id) + + def test_parent_active_triggers_trunk_active(self): + with self.port() as new_parent: + new_parent['status'] = nlib_consts.PORT_STATUS_ACTIVE + old_parent = {'status': nlib_consts.PORT_STATUS_DOWN} + old_trunk = self._create_test_trunk(new_parent) + old_trunk = self._get_trunk_obj(old_trunk['id']) + old_trunk.update(status=trunk_consts.TRUNK_DOWN_STATUS) + trunk_details = {'trunk_id': old_trunk.id} + new_parent['trunk_details'] = trunk_details + old_parent['trunk_details'] = trunk_details + self.trunk_handler.port_updated( + resources.PORT, + events.AFTER_UPDATE, + None, + payload=events.DBEventPayload( + self.context, states=(old_parent, new_parent))) + new_trunk = self._get_trunk_obj(old_trunk.id) + self.assertEqual( + trunk_consts.TRUNK_ACTIVE_STATUS, new_trunk.status) + + def test_parent_build_does_not_trigger_trunk_active(self): + with self.port() as new_parent: + new_parent['status'] = nlib_consts.PORT_STATUS_BUILD + old_parent = {'status': nlib_consts.PORT_STATUS_DOWN} + old_trunk = self._create_test_trunk(new_parent) + old_trunk = self._get_trunk_obj(old_trunk['id']) + old_trunk.update(status=trunk_consts.TRUNK_DOWN_STATUS) + trunk_details = {'trunk_id': old_trunk.id} + new_parent['trunk_details'] = trunk_details + old_parent['trunk_details'] = trunk_details + self.trunk_handler.port_updated( + resources.PORT, + events.AFTER_UPDATE, + None, + payload=events.DBEventPayload( + self.context, states=(old_parent, new_parent))) + new_trunk = self._get_trunk_obj(old_trunk.id) + self.assertNotEqual( + trunk_consts.TRUNK_ACTIVE_STATUS, new_trunk.status) + + class TestTrunkDriver(base.BaseTestCase): def test_is_loaded(self): driver = trunk_driver.OVNTrunkDriver.create(mock.Mock()) diff --git a/neutron/tests/unit/services/trunk/test_plugin.py b/neutron/tests/unit/services/trunk/test_plugin.py index f5598260e9a..b631ef3d7e9 100644 --- a/neutron/tests/unit/services/trunk/test_plugin.py +++ b/neutron/tests/unit/services/trunk/test_plugin.py @@ -19,7 +19,6 @@ from neutron_lib.api.definitions import portbindings from neutron_lib.callbacks import events from neutron_lib.callbacks import registry from neutron_lib.callbacks import resources -from neutron_lib import constants as neutron_const from neutron_lib.plugins import directory from neutron_lib.services.trunk import constants import testtools @@ -286,32 +285,6 @@ class TrunkPluginTestCase(test_plugin.Ml2PluginV2TestCase): {'sub_ports': [{'port_id': subport['port']['id']}]}) self.assertEqual(constants.TRUNK_DOWN_STATUS, trunk['status']) - def test__trigger_trunk_status_change_parent_port_status_down(self): - callback = register_mock_callback(resources.TRUNK, events.AFTER_UPDATE) - with self.port() as parent: - parent['status'] = neutron_const.PORT_STATUS_DOWN - original_port = {'status': neutron_const.PORT_STATUS_DOWN} - _, _ = ( - self._test__trigger_trunk_status_change( - parent, original_port, - constants.TRUNK_DOWN_STATUS, - constants.TRUNK_DOWN_STATUS)) - callback.assert_not_called() - - def test__trigger_trunk_status_change_parent_port_status_up(self): - callback = register_mock_callback(resources.TRUNK, events.AFTER_UPDATE) - with self.port() as parent: - parent['status'] = neutron_const.PORT_STATUS_ACTIVE - original_port = {'status': neutron_const.PORT_STATUS_DOWN} - _, _ = ( - self._test__trigger_trunk_status_change( - parent, original_port, - constants.TRUNK_DOWN_STATUS, - constants.TRUNK_ACTIVE_STATUS)) - callback.assert_called_once_with( - resources.TRUNK, events.AFTER_UPDATE, - self.trunk_plugin, payload=mock.ANY) - def test__trigger_trunk_status_change_vif_type_changed_unbound(self): callback = register_mock_callback(resources.TRUNK, events.AFTER_UPDATE) with self.port() as parent: