From f28079a77719d283f5b474bdfd8d0a235f37694d 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 Signed-off-by: Bence Romsics (cherry picked from commit e69505d293ef12999f948531acbde75e16a65cd4) (cherry picked from commit 2224c9ea1e5a35acaad753a7f677d6788a612ec8) (cherry picked from commit fd9d90706da7965dabfc0521bdc15b468f6af899) (cherry picked from commit 24c492cf394ed23e431c72ac0c87f8a0246ee007) (cherry picked from commit 52aa69d16c92afab0cb45df95f217a6ce69716b2) --- .../trunk/drivers/ovn/trunk_driver.py | 47 +++++++++++++ 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, 117 insertions(+), 36 deletions(-) diff --git a/neutron/services/trunk/drivers/ovn/trunk_driver.py b/neutron/services/trunk/drivers/ovn/trunk_driver.py index fcc260ff4e0..637aa6de8b0 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.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 @@ -26,6 +27,7 @@ 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 @@ -216,6 +218,46 @@ def subport_event(self, resource, event, trunk_plugin, payload): self.subports_deleted(payload.states[0], payload.metadata['subports']) + 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 @@ -242,6 +284,11 @@ def register(self, resource, event, trigger, payload=None): resources.SUBPORTS, _event) + 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 ffcf1a8a2c3..fe314cbc2b9 100644 --- a/neutron/services/trunk/plugin.py +++ b/neutron/services/trunk/plugin.py @@ -21,7 +21,6 @@ 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 @@ -465,19 +464,12 @@ def _trigger_trunk_status_change(self, resource, event, trigger, payload): 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 894520bf860..a4f703fd3e3 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.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 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 TestTrunkHandler(base.BaseTestCase): @@ -363,6 +368,70 @@ def test_subport_event_invalid(self, s_deleted, s_added): s_deleted.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 setUp(self): super(TestTrunkDriver, self).setUp() diff --git a/neutron/tests/unit/services/trunk/test_plugin.py b/neutron/tests/unit/services/trunk/test_plugin.py index 271c16e66db..ca7f07e9de4 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.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 @@ -287,32 +286,6 @@ def test_remove_subports_trunk_goes_to_down(self): {'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: