[OVN] Set the Neutron port status based on "lsp.up" and "lsp.enabled"

The Neutron "port.status" field ("ACTIVE", "DOWN") is set depending on
the Logical Switch Port "up" and "enabled" flags. Before this patch,
the "port.status" depended only on the "up" flag. However, the user
can set the "port.admin_state_up" field that will modify the
"lsp.enabled" flag. If the "port.admin_state_up" is DOWN, the port
does not transmit and the status should be "DOWN".

The OVN backend is correctly disabling the port transmission; what
was incorrect in the Neutron API is only the "port.status" field.

This is currently working in other mechanism drivers like LB or OVS.

NOTE: this backport also includes [1], that is updating the release
note section to "fixes".

[1]https://review.opendev.org/c/openstack/neutron/+/903863

Closes-Bug: #2036705
Change-Id: I5b7b55b0b365df7246a571cea97a392cdf89bdb6
(cherry picked from commit aead4aa)
(cherry picked from commit d084f27)

Author: ralonsoh

neutron/common/ovn/utils.py

@@ -299,6 +299,33 @@ def get_lsp_security_groups(port, skip_trusted_port=True):
                   ) else port.get('security_groups', [])
 
 
+def is_lsp_enabled(lsp):
+    """Return if a Logical Switch Port is enabled
+
+    This method mimics the OVN northd method ``lsp_is_enabled``
+    https://shorturl.at/rBSZ7. The "enabled" field can have three values:
+    * []: from older OVN versions, in this case the LSP is enabled by default.
+    * [True]: the LSP is enabled.
+    * [False]: the LSP is disabled.
+
+    :param lsp: ``ovs.db.Row`` with a Logical Switch Port register.
+    :return: True if the port is enabled, False if not.
+    """
+    return not lsp.enabled or lsp.enabled == [True]
+
+
+def is_lsp_up(lsp):
+    """Return if a Logical Switch Port is UP
+
+    This method mimics the OVN northd method ``lsp_is_up``
+    https://shorturl.at/aoKR6
+
+    :param lsp: ``ovs.db.Row`` with a Logical Switch Port register.
+    :return: True if the port is UP, False if not.
+    """
+    return lsp.up == [True]
+
+
 def is_snat_enabled(router):
     return router.get(l3.EXTERNAL_GW_INFO, {}).get('enable_snat', True)
 

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py

@@ -286,7 +286,7 @@ def match_fn(self, event, row, old=None):
             # ``LogicalSwitchPortUpdateUpEvent`` events.
             return False
 
-        return bool(lsp.up)
+        return utils.is_lsp_enabled(lsp) and utils.is_lsp_up(lsp)
 
     def run(self, event, row, old=None):
         self.driver.set_port_status_up(row.logical_port)
@@ -461,81 +461,84 @@ def run(self, event, row, old):
             router, host)
 
 
-class LogicalSwitchPortCreateUpEvent(row_event.RowEvent):
-    """Row create event - Logical_Switch_Port 'up' = True.
+class LogicalSwitchPortCreateEvent(row_event.RowEvent):
+    """Row create event - Checks Logical_Switch_Port is UP and enabled.
 
     On connection, we get a dump of all ports, so if there is a neutron
-    port that is down that has since been activated, we'll catch it here.
-    This event will not be generated for new ports getting created.
+    port that has been activated or deactivated, we'll catch it here.
     """
 
     def __init__(self, driver):
         self.driver = driver
         table = 'Logical_Switch_Port'
         events = (self.ROW_CREATE,)
-        super(LogicalSwitchPortCreateUpEvent, self).__init__(
-            events, table, (('up', '=', True),))
-        self.event_name = 'LogicalSwitchPortCreateUpEvent'
+        super().__init__(events, table, [])
+        self.event_name = 'LogicalSwitchPortCreateEvent'
 
     def run(self, event, row, old):
-        self.driver.set_port_status_up(row.name)
-
-
-class LogicalSwitchPortCreateDownEvent(row_event.RowEvent):
-    """Row create event - Logical_Switch_Port 'up' = False
-
-    On connection, we get a dump of all ports, so if there is a neutron
-    port that is up that has since been deactivated, we'll catch it here.
-    This event will not be generated for new ports getting created.
-    """
-    def __init__(self, driver):
-        self.driver = driver
-        table = 'Logical_Switch_Port'
-        events = (self.ROW_CREATE,)
-        super(LogicalSwitchPortCreateDownEvent, self).__init__(
-            events, table, (('up', '=', False),))
-        self.event_name = 'LogicalSwitchPortCreateDownEvent'
-
-    def run(self, event, row, old):
-        self.driver.set_port_status_down(row.name)
+        if utils.is_lsp_up(row) and utils.is_lsp_enabled(row):
+            self.driver.set_port_status_up(row.name)
+        else:
+            self.driver.set_port_status_down(row.name)
 
 
 class LogicalSwitchPortUpdateUpEvent(row_event.RowEvent):
-    """Row update event - Logical_Switch_Port 'up' going from False to True
+    """Row update event - Logical_Switch_Port UP or enabled going True
 
     This happens when the VM goes up.
-    New value of Logical_Switch_Port 'up' will be True and the old value will
-    be False.
     """
     def __init__(self, driver):
         self.driver = driver
         table = 'Logical_Switch_Port'
         events = (self.ROW_UPDATE,)
         super(LogicalSwitchPortUpdateUpEvent, self).__init__(
-            events, table, (('up', '=', True),),
-            old_conditions=(('up', '!=', True),))
+            events, table, None)
         self.event_name = 'LogicalSwitchPortUpdateUpEvent'
 
+    def match_fn(self, event, row, old):
+        if not (utils.is_lsp_up(row) and utils.is_lsp_enabled(row)):
+            return False
+
+        if hasattr(old, 'up') and not utils.is_lsp_up(old):
+            # The port has transitioned from DOWN to UP, and the admin state
+            # is UP (lsp.enabled=True)
+            return True
+        if hasattr(old, 'enabled') and not utils.is_lsp_enabled(old):
+            # The user has set the admin state to UP and the port is UP too.
+            return True
+        return False
+
     def run(self, event, row, old):
         self.driver.set_port_status_up(row.name)
 
 
 class LogicalSwitchPortUpdateDownEvent(row_event.RowEvent):
-    """Row update event - Logical_Switch_Port 'up' going from True to False
+    """Row update event - Logical_Switch_Port UP or enabled going to False
 
-    This happens when the VM goes down.
-    New value of Logical_Switch_Port 'up' will be False and the old value will
-    be True.
+    This happens when the VM goes down or the port is disabled.
     """
     def __init__(self, driver):
         self.driver = driver
         table = 'Logical_Switch_Port'
         events = (self.ROW_UPDATE,)
         super(LogicalSwitchPortUpdateDownEvent, self).__init__(
-            events, table, (('up', '=', False),),
-            old_conditions=(('up', '=', True),))
+            events, table, None)
         self.event_name = 'LogicalSwitchPortUpdateDownEvent'
 
+    def match_fn(self, event, row, old):
+        if (hasattr(old, 'up') and
+                utils.is_lsp_up(old) and
+                not utils.is_lsp_up(row)):
+            # If the port goes DOWN, update the port status to DOWN.
+            return True
+        if (hasattr(old, 'enabled') and
+                utils.is_lsp_enabled(old) and
+                not utils.is_lsp_enabled(row)):
+            # If the port is disabled by the user, update the port status to
+            # DOWN.
+            return True
+        return False
+
     def run(self, event, row, old):
         self.driver.set_port_status_down(row.name)
 
@@ -779,12 +782,10 @@ def __init__(self, driver, remote, schema):
         super(OvnNbIdl, self).__init__(driver, remote, schema)
         self._lsp_update_up_event = LogicalSwitchPortUpdateUpEvent(driver)
         self._lsp_update_down_event = LogicalSwitchPortUpdateDownEvent(driver)
-        self._lsp_create_up_event = LogicalSwitchPortCreateUpEvent(driver)
-        self._lsp_create_down_event = LogicalSwitchPortCreateDownEvent(driver)
+        self._lsp_create_event = LogicalSwitchPortCreateEvent(driver)
         self._fip_create_delete_event = FIPAddDeleteEvent(driver)
 
-        self.notify_handler.watch_events([self._lsp_create_up_event,
-                                          self._lsp_create_down_event,
+        self.notify_handler.watch_events([self._lsp_create_event,
                                           self._lsp_update_up_event,
                                           self._lsp_update_down_event,
                                           self._fip_create_delete_event])
@@ -804,10 +805,8 @@ def unwatch_logical_switch_port_create_events(self):
         After the startup, there is no need to watch these events.
         So unwatch these events.
         """
-        self.notify_handler.unwatch_events([self._lsp_create_up_event,
-                                            self._lsp_create_down_event])
-        self._lsp_create_up_event = None
-        self._lsp_create_down_event = None
+        self.notify_handler.unwatch_events([self._lsp_create_event])
+        del self._lsp_create_event
 
     def post_connect(self):
         self.unwatch_logical_switch_port_create_events()

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py

@@ -60,7 +60,9 @@
                 "port_security": {"type": {"key": "string",
                                            "min": 0,
                                            "max": "unlimited"}},
-                "up": {"type": {"key": "boolean", "min": 0, "max": 1}}},
+                "up": {"type": {"key": "boolean", "min": 0, "max": 1}},
+                "enabled": {"type": {"key": "boolean", "min": 0, "max": 1}},
+            },
             "indexes": [["name"]],
             "isRoot": False,
         },
@@ -366,7 +368,8 @@ def test_event_matches(self):
         pbtable = fakes.FakeOvsdbTable.create_one_ovsdb_table(
             attrs={'name': 'Port_Binding'})
         ovsdb_row = fakes.FakeOvsdbRow.create_one_ovsdb_row
-        self.driver.nb_ovn.lookup.return_value = ovsdb_row(attrs={'up': True})
+        self.driver.nb_ovn.lookup.return_value = ovsdb_row(
+            attrs={'up': True, 'enabled': True})
 
         # Port binding change.
         self._test_event(
@@ -422,28 +425,46 @@ def _test_lsp_helper(self, event, new_row_json, old_row_json=None,
         # Execute the notifications queued
         self.idl.notify_handler.notify_loop()
 
-    def test_lsp_up_create_event(self):
-        row_data = {"up": True, "name": "foo-name"}
+    def test_lsp_create_event(self):
+        row_data = {'name': 'foo'}
+
+        # up and enabled
+        row_data.update({'up': True, 'enabled': True})
         self._test_lsp_helper('create', row_data)
-        self.mech_driver.set_port_status_up.assert_called_once_with("foo-name")
+        self.mech_driver.set_port_status_up.assert_called_once_with('foo')
         self.assertFalse(self.mech_driver.set_port_status_down.called)
+        self.mech_driver.set_port_status_up.reset_mock()
 
-    def test_lsp_down_create_event(self):
-        row_data = {"up": False, "name": "foo-name"}
+        # up and disabled
+        row_data.update({'up': True, 'enabled': False})
         self._test_lsp_helper('create', row_data)
-        self.mech_driver.set_port_status_down.assert_called_once_with(
-            "foo-name")
         self.assertFalse(self.mech_driver.set_port_status_up.called)
+        self.mech_driver.set_port_status_down.assert_called_once_with('foo')
+        self.mech_driver.set_port_status_down.reset_mock()
 
-    def test_lsp_up_not_set_event(self):
-        row_data = {"up": ['set', []], "name": "foo-name"}
+        # down and enabled
+        row_data.update({'up': False, 'enabled': True})
         self._test_lsp_helper('create', row_data)
         self.assertFalse(self.mech_driver.set_port_status_up.called)
-        self.assertFalse(self.mech_driver.set_port_status_down.called)
+        self.mech_driver.set_port_status_down.assert_called_once_with('foo')
+        self.mech_driver.set_port_status_down.reset_mock()
+
+        # down and disabled
+        row_data.update({'up': False, 'enabled': False})
+        self._test_lsp_helper('create', row_data)
+        self.assertFalse(self.mech_driver.set_port_status_up.called)
+        self.mech_driver.set_port_status_down.assert_called_once_with('foo')
+        self.mech_driver.set_port_status_down.reset_mock()
+
+        # Not set to up
+        row_data.update({'up': ['set', []], 'enabled': True})
+        self._test_lsp_helper('create', row_data)
+        self.assertFalse(self.mech_driver.set_port_status_up.called)
+        self.mech_driver.set_port_status_down.assert_called_once_with('foo')
 
     def test_unwatch_logical_switch_port_create_events(self):
         self.idl.unwatch_logical_switch_port_create_events()
-        row_data = {"up": True, "name": "foo-name"}
+        row_data = {'up': False, 'name': 'foo-name'}
         self._test_lsp_helper('create', row_data)
         self.assertFalse(self.mech_driver.set_port_status_up.called)
         self.assertFalse(self.mech_driver.set_port_status_down.called)
@@ -455,19 +476,18 @@ def test_unwatch_logical_switch_port_create_events(self):
 
     def test_post_connect(self):
         self.idl.post_connect()
-        self.assertIsNone(self.idl._lsp_create_up_event)
-        self.assertIsNone(self.idl._lsp_create_down_event)
+        self.assertIsNone(getattr(self.idl, '_lsp_create_event', None))
 
     def test_lsp_up_update_event(self):
-        new_row_json = {"up": True, "name": "foo-name"}
+        new_row_json = {'up': True, 'enabled': True, 'name': 'foo-name'}
         old_row_json = {"up": False}
         self._test_lsp_helper('update', new_row_json,
                               old_row_json=old_row_json)
         self.mech_driver.set_port_status_up.assert_called_once_with("foo-name")
         self.assertFalse(self.mech_driver.set_port_status_down.called)
 
     def test_lsp_down_update_event(self):
-        new_row_json = {"up": False, "name": "foo-name"}
+        new_row_json = {'up': False, 'enabled': False, 'name': 'foo-name'}
         old_row_json = {"up": True}
         self._test_lsp_helper('update', new_row_json,
                               old_row_json=old_row_json)
@@ -476,7 +496,7 @@ def test_lsp_down_update_event(self):
         self.assertFalse(self.mech_driver.set_port_status_up.called)
 
     def test_lsp_up_update_event_no_old_data(self):
-        new_row_json = {"up": True, "name": "foo-name"}
+        new_row_json = {'up': True, 'enabled': True, 'name': 'foo-name'}
         self._test_lsp_helper('update', new_row_json,
                               old_row_json=None)
         self.assertFalse(self.mech_driver.set_port_status_up.called)

releasenotes/notes/port_status_based_on_lsp_up_and_enabled-31c062fc7089f62a.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    [`bug 2036705 <https://bugs.launchpad.net/neutron/+bug/2036705>`_]
+    The Neutron ``port.status`` field ("ACTIVE", "DOWN") is now set based on
+    the ML2/OVN Logical Switch Port ``up`` and ``enabled`` flags. The user can
+    now set the ``port.admin_state_up``, that is replicated in the
+    ``lsp.enabled`` flag, to enable or disable the port. If the port is
+    disabled, the traffic is stopped and the ``port.status`` is set to "DOWN".