Merge "Limit trunk ACTIVE state hack to OVN" into unmaintained/2023.1

Author: Zuul

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
@@ -25,7 +26,9 @@
 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 = (
@@ -200,6 +203,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
@@ -225,6 +268,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

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
@@ -464,19 +463,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}})

neutron/tests/unit/services/trunk/drivers/ovn/test_trunk_driver.py

@@ -18,16 +18,21 @@
 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
 
 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()

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
@@ -286,32 +285,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: