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 <[email protected]>
(cherry picked from commit e69505d)
(cherry picked from commit 2224c9e)
(cherry picked from commit fd9d907)
(cherry picked from commit 24c492c)
(cherry picked from commit 52aa69d)

Author: rubasov

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

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}})

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
@@ -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: