[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.

Closes-Bug: #2036705
Change-Id: I5b7b55b0b365df7246a571cea97a392cdf89bdb6

neutron/common/ovn/utils.py

@@ -301,6 +301,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 @@ class PortBindingChassisUpdateEvent(row_event.RowEvent):
             # ``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 @@ class PortBindingChassisEvent(row_event.RowEvent):
             router, host)
 
 
-class LogicalSwitchPortCreateUpEvent(row_event.RowEvent):
+class LogicalSwitchPortCreateEvent(row_event.RowEvent):
-    """Row create event - Logical_Switch_Port 'up' = True.
+    """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.
+    port that has been activated or 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(LogicalSwitchPortCreateUpEvent, self).__init__(
+        super().__init__(events, table, [])
-            events, table, (('up', '=', True),))
+        self.event_name = 'LogicalSwitchPortCreateEvent'
-        self.event_name = 'LogicalSwitchPortCreateUpEvent'
 
     def run(self, event, row, old):
-        self.driver.set_port_status_up(row.name)
+        if utils.is_lsp_up(row) and utils.is_lsp_enabled(row):
-
+            self.driver.set_port_status_up(row.name)
-
+        else:
-class LogicalSwitchPortCreateDownEvent(row_event.RowEvent):
+            self.driver.set_port_status_down(row.name)
-    """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)
 
 
 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),),
+            events, table, None)
-            old_conditions=(('up', '!=', True),))
         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.
+    This happens when the VM goes down or the port is disabled.
-    New value of Logical_Switch_Port 'up' will be False and the old value will
-    be True.
     """
     def __init__(self, driver):
         self.driver = driver
         table = 'Logical_Switch_Port'
         events = (self.ROW_UPDATE,)
         super(LogicalSwitchPortUpdateDownEvent, self).__init__(
-            events, table, (('up', '=', False),),
+            events, table, None)
-            old_conditions=(('up', '=', True),))
         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 @@ class OvnNbIdl(OvnIdlDistributedLock):
         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_event = LogicalSwitchPortCreateEvent(driver)
-        self._lsp_create_down_event = LogicalSwitchPortCreateDownEvent(driver)
         self._fip_create_delete_event = FIPAddDeleteEvent(driver)
 
-        self.notify_handler.watch_events([self._lsp_create_up_event,
+        self.notify_handler.watch_events([self._lsp_create_event,
-                                          self._lsp_create_down_event,
                                           self._lsp_update_up_event,
                                           self._lsp_update_down_event,
                                           self._fip_create_delete_event])
@@ -804,10 +805,8 @@ class OvnNbIdl(OvnIdlDistributedLock):
         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.notify_handler.unwatch_events([self._lsp_create_event])
-                                            self._lsp_create_down_event])
+        del self._lsp_create_event
-        self._lsp_create_up_event = None
-        self._lsp_create_down_event = None
 
     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 @@ OVN_NB_SCHEMA = {
                 "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 @@ class TestPortBindingChassisUpdateEvent(base.BaseTestCase):
         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 @@ class TestOvnNbIdlNotifyHandler(test_mech_driver.OVNMechanismDriverTestCase):
         # Execute the notifications queued
         self.idl.notify_handler.notify_loop()
 
-    def test_lsp_up_create_event(self):
+    def test_lsp_create_event(self):
-        row_data = {"up": True, "name": "foo-name"}
+        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):
+        # up and disabled
-        row_data = {"up": False, "name": "foo-name"}
+        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)
-
-    def test_lsp_up_not_set_event(self):
-        row_data = {"up": ['set', []], "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)
+        self.mech_driver.set_port_status_down.assert_called_once_with('foo')
+        self.mech_driver.set_port_status_down.reset_mock()
+
+        # 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.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,11 +476,10 @@ class TestOvnNbIdlNotifyHandler(test_mech_driver.OVNMechanismDriverTestCase):
 
     def test_post_connect(self):
         self.idl.post_connect()
-        self.assertIsNone(self.idl._lsp_create_up_event)
+        self.assertIsNone(getattr(self.idl, '_lsp_create_event', None))
-        self.assertIsNone(self.idl._lsp_create_down_event)
 
     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)
@@ -467,7 +487,7 @@ class TestOvnNbIdlNotifyHandler(test_mech_driver.OVNMechanismDriverTestCase):
         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 @@ class TestOvnNbIdlNotifyHandler(test_mech_driver.OVNMechanismDriverTestCase):
         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,8 @@
+---
+features:
+  - |
+    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".