Merge "[OVN] Set the Neutron port status based on "lsp.up" and "lsp.enabled"" into stable/2023.1

Author: Zuul

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