Fix issue with virtual ports not being exposed on time

The requested-chassis information is only added by neutron
for "" port types. The virtual ports chassis is decided by OVN
not by neutron, and neutron only update that information in the
periodic maintenance task.

As part of [1] information about the virtual port chassis is being
added to the external_ids, as well as for "" ports. This patch is
adapting the NB DB driver to consume this new source of information
and being able to expose those ports when OVN associates them to a
node.

Depends-On: https://review.opendev.org/c/openstack/neutron/+/882705

Closes-Bug: #2020157

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

Change-Id: I9abf987004370d0c3ec6a152bb28ab450a9af2c2

ovn_bgp_agent/constants.py

@@ -67,6 +67,9 @@ EXPOSE = "expose"
 WITHDRAW = "withdraw"
 
 OVN_REQUESTED_CHASSIS = "requested-chassis"
+OVN_HOST_ID_EXT_ID_KEY = "neutron:host_id"
+OVN_CHASSIS_AT_OPTIONS = "options"
+OVN_CHASSIS_AT_EXT_IDS = "external-ids"
 
 # Exposing method names
 EXPOSE_METHOD_UNDERLAY = 'underlay'

ovn_bgp_agent/drivers/openstack/utils/ovn.py

@@ -216,8 +216,13 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend):
         ports = []
         cmd = self.db_find_rows('Logical_Switch_Port', ('up', '=', True))
         for row in cmd.execute(check_error=True):
-            if (row.options and
+            if (row.external_ids and
-                    row.options.get('requested-chassis') == chassis):
+                    row.external_ids.get(
+                        constants.OVN_HOST_ID_EXT_ID_KEY) == chassis):
+                ports.append(row)
+            elif (row.options and
+                    row.options.get(
+                        constants.OVN_REQUESTED_CHASSIS) == chassis):
                 ports.append(row)
         return ports
 

ovn_bgp_agent/drivers/openstack/watchers/base_watcher.py

@@ -13,9 +13,11 @@
 # limitations under the License.
 
 from oslo_log import log as logging
-
 from ovsdbapp.backend.ovs_idl import event as row_event
 
+from ovn_bgp_agent import constants
+
+
 LOG = logging.getLogger(__name__)
 
 
@@ -59,3 +61,14 @@ class LSPChassisEvent(Event):
 
     def _check_ip_associated(self, mac):
         return len(mac.strip().split(' ')) > 1
+
+    def _get_chassis(self, row):
+        if (hasattr(row, 'external_ids') and
+                row.external_ids.get(constants.OVN_HOST_ID_EXT_ID_KEY)):
+            return (row.external_ids[constants.OVN_HOST_ID_EXT_ID_KEY],
+                    constants.OVN_CHASSIS_AT_EXT_IDS)
+        elif (hasattr(row, 'options') and
+                row.options.get(constants.OVN_REQUESTED_CHASSIS)):
+            return (row.options[constants.OVN_REQUESTED_CHASSIS],
+                    constants.OVN_CHASSIS_AT_OPTIONS)
+        return None, None

ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py

@@ -35,19 +35,28 @@ class LogicalSwitchPortProviderCreateEvent(base_watcher.LSPChassisEvent):
             if not self._check_ip_associated(row.addresses[0]):
                 return False
 
-            current_chassis = row.options.get(constants.OVN_REQUESTED_CHASSIS)
+            current_chassis, chassis_location = self._get_chassis(row)
             if current_chassis != self.agent.chassis:
                 return False
             if not bool(row.up[0]):
                 return False
 
-            if hasattr(old, 'options'):
-                old_chassis = old.options.get(constants.OVN_REQUESTED_CHASSIS)
-                if not old_chassis or current_chassis != old_chassis:
-                    return True
             if hasattr(old, 'up'):
                 if not bool(old.up[0]):
                     return True
+
+            # NOTE(ltomasbo): This can be updated/removed once neutron has
+            # chassis information on external ids
+            if chassis_location == constants.OVN_CHASSIS_AT_OPTIONS:
+                if hasattr(old, 'options'):
+                    old_chassis, _ = self._get_chassis(old)
+                    if not old_chassis or current_chassis != old_chassis:
+                        return True
+            else:
+                if hasattr(old, 'external_ids'):
+                    old_chassis, _ = self._get_chassis(old)
+                    if not old_chassis or current_chassis != old_chassis:
+                        return True
         except (IndexError, AttributeError):
             return False
 
@@ -72,24 +81,40 @@ class LogicalSwitchPortProviderDeleteEvent(base_watcher.LSPChassisEvent):
             if not self._check_ip_associated(row.addresses[0]):
                 return False
 
-            current_chassis = row.options.get(constants.OVN_REQUESTED_CHASSIS)
+            current_chassis, chassis_location = self._get_chassis(row)
             if event == self.ROW_DELETE:
-                return current_chassis == self.agent.chassis
+                return current_chassis == self.agent.chassis and row.up
 
             # ROW_UPDATE EVENT
-            if hasattr(old, 'options'):
-                # check chassis change
-                old_chassis = old.options.get(constants.OVN_REQUESTED_CHASSIS)
-                if old_chassis != self.agent.chassis:
-                    return False
-                if not current_chassis or current_chassis != old_chassis:
-                    return True
-
             if hasattr(old, 'up'):
                 if not bool(old.up[0]):
                     return False
                 if not bool(row.up[0]):
                     return True
+            else:
+                # If there is no change on the status, and it was already down
+                # there is no need to remove it again
+                if not bool(row.up[0]):
+                    return False
+
+            # NOTE(ltomasbo): This can be updated/removed once neutron has
+            # chassis information on external ids
+            if chassis_location == constants.OVN_CHASSIS_AT_OPTIONS:
+                if hasattr(old, 'options'):
+                    # check chassis change
+                    old_chassis, _ = self._get_chassis(old)
+                    if old_chassis != self.agent.chassis:
+                        return False
+                    if not current_chassis or current_chassis != old_chassis:
+                        return True
+            else:
+                if hasattr(old, 'external_ids'):
+                    # check chassis change
+                    old_chassis, _ = self._get_chassis(old)
+                    if old_chassis != self.agent.chassis:
+                        return False
+                    if not current_chassis or current_chassis != old_chassis:
+                        return True
         except (IndexError, AttributeError):
             return False
 
@@ -114,16 +139,23 @@ class LogicalSwitchPortFIPCreateEvent(base_watcher.LSPChassisEvent):
             if not self._check_ip_associated(row.addresses[0]):
                 return False
 
-            current_chassis = row.options.get(constants.OVN_REQUESTED_CHASSIS)
+            current_chassis, chassis_location = self._get_chassis(row)
             current_port_fip = row.external_ids.get(
                 constants.OVN_FIP_EXT_ID_KEY)
             if (current_chassis != self.agent.chassis or not bool(row.up[0]) or
                     not current_port_fip):
                 return False
 
+            if hasattr(old, 'up'):
+                # check port status change
+                if not bool(old.up[0]):
+                    return True
+
+            # NOTE(ltomasbo): This can be updated/removed once neutron has
+            # chassis information on external ids
+            if chassis_location == constants.OVN_CHASSIS_AT_OPTIONS:
                 if hasattr(old, 'options'):
-                # check chassis change
+                    old_chassis, _ = self._get_chassis(old)
-                old_chassis = old.options.get(constants.OVN_REQUESTED_CHASSIS)
                     if not old_chassis or current_chassis != old_chassis:
                         return True
                 if hasattr(old, 'external_ids'):
@@ -132,9 +164,15 @@ class LogicalSwitchPortFIPCreateEvent(base_watcher.LSPChassisEvent):
                         constants.OVN_FIP_EXT_ID_KEY)
                     if not old_port_fip or current_port_fip != old_port_fip:
                         return True
-            if hasattr(old, 'up'):
+            else:  # by default expect the chassis information at external-ids
-                # check port status change
+                if hasattr(old, 'external_ids'):
-                if not bool(old.up[0]):
+                    # note the whole extenal-ids are included, even if only
+                    # one field inside it is updated
+                    old_chassis, _ = self._get_chassis(old)
+                    old_port_fip = old.external_ids.get(
+                        constants.OVN_FIP_EXT_ID_KEY)
+                    if (current_chassis != old_chassis or
+                            current_port_fip != old_port_fip):
                         return True
         except (IndexError, AttributeError):
             return False
@@ -164,7 +202,7 @@ class LogicalSwitchPortFIPDeleteEvent(base_watcher.LSPChassisEvent):
             if not self._check_ip_associated(row.addresses[0]):
                 return False
 
-            current_chassis = row.options.get(constants.OVN_REQUESTED_CHASSIS)
+            current_chassis, chassis_location = self._get_chassis(row)
             current_port_fip = row.external_ids.get(
                 constants.OVN_FIP_EXT_ID_KEY)
             if event == self.ROW_DELETE:
@@ -173,9 +211,19 @@ class LogicalSwitchPortFIPDeleteEvent(base_watcher.LSPChassisEvent):
                     return True
                 return False
 
+            if hasattr(old, 'up'):
+                # check port status change
+                if not bool(old.up[0]):
+                    return False
+                if not bool(row.up[0]) and current_port_fip:
+                    return True
+
+            # NOTE(ltomasbo): This can be updated/removed once neutron has
+            # chassis information on external ids
+            if chassis_location == constants.OVN_CHASSIS_AT_OPTIONS:
                 if hasattr(old, 'options'):
                     # check chassis change
-                old_chassis = old.options.get(constants.OVN_REQUESTED_CHASSIS)
+                    old_chassis, _ = self._get_chassis(old)
                     if (not old_chassis or old_chassis != self.agent.chassis):
                         return False
                     if current_chassis != old_chassis and current_port_fip:
@@ -192,11 +240,20 @@ class LogicalSwitchPortFIPDeleteEvent(base_watcher.LSPChassisEvent):
                         return False
                     if old_port_fip != current_port_fip:
                         return True
-            if hasattr(old, 'up'):
+            else:  # by default expect the chassis information at external-ids
-                # check port status change
+                if hasattr(old, 'external_ids'):
-                if not bool(old.up[0]):
+                    # check chassis change
+                    old_chassis, _ = self._get_chassis(old)
+                    if (not old_chassis or old_chassis != self.agent.chassis):
                         return False
-                if not bool(row.up[0]) and current_port_fip:
+                    if current_chassis != old_chassis and current_port_fip:
+                        return True
+                    # check fips deletion
+                    old_port_fip = old.external_ids.get(
+                        constants.OVN_FIP_EXT_ID_KEY)
+                    if not old_port_fip:
+                        return False
+                    if old_port_fip != current_port_fip:
                         return True
         except (IndexError, AttributeError):
             return False

ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_ovn.py

@@ -99,12 +99,31 @@ class TestOvsdbNbOvnIdl(test_base.TestCase):
             'NAT',
             ('logical_port', '=', logical_port))
 
-    def test_get_active_ports_on_chassis(self):
+    def test_get_active_ports_on_chassis_options(self):
         chassis = 'local_chassis'
         row1 = fakes.create_object({
-            'options': {'requested-chassis': chassis}})
+            'options': {'requested-chassis': chassis},
+            'external_ids': {}})
         row2 = fakes.create_object({
-            'options': {'requested-chassis': 'other_chassis'}})
+            'options': {'requested-chassis': 'other_chassis'},
+            'external_ids': {}})
+        self.nb_idl.db_find_rows.return_value.execute.return_value = [
+            row1, row2]
+        ret = self.nb_idl.get_active_ports_on_chassis(chassis)
+
+        self.assertEqual([row1], ret)
+        self.nb_idl.db_find_rows.assert_called_once_with(
+            'Logical_Switch_Port',
+            ('up', '=', True))
+
+    def test_get_active_ports_on_chassis_external_ids(self):
+        chassis = 'local_chassis'
+        row1 = fakes.create_object({
+            'options': {},
+            'external_ids': {'neutron:host_id': chassis}})
+        row2 = fakes.create_object({
+            'options': {},
+            'external_ids': {'neutron:host_id': 'other_chassis'}})
         self.nb_idl.db_find_rows.return_value.execute.return_value = [
             row1, row2]
         ret = self.nb_idl.get_active_ports_on_chassis(chassis)

ovn_bgp_agent/tests/unit/drivers/openstack/watchers/test_nb_bgp_watcher.py

@@ -36,12 +36,30 @@ class TestLogicalSwitchPortProviderCreateEvent(test_base.TestCase):
                                addresses=['mac 192.168.0.1'],
                                options={'requested-chassis': self.chassis},
                                up=[True])
-        old = utils.create_row(options={}, up=True)
+        old = utils.create_row(options={}, up=[True])
+        self.assertTrue(self.event.match_fn(mock.Mock(), row, old))
+
+    def test_match_fn_port_up(self):
+        row = utils.create_row(type=constants.OVN_VM_VIF_PORT_TYPE,
+                               addresses=['mac 192.168.0.1'],
+                               options={'requested-chassis': self.chassis},
+                               up=[True])
+        old = utils.create_row(type=constants.OVN_VM_VIF_PORT_TYPE,
+                               addresses=['mac 192.168.0.1'],
+                               options={'requested-chassis': self.chassis},
+                               up=[False])
+        self.assertTrue(self.event.match_fn(mock.Mock(), row, old))
+
+    def test_match_fn_external_id(self):
+        row = utils.create_row(type=constants.OVN_VM_VIF_PORT_TYPE,
+                               addresses=['mac 192.168.0.1'],
+                               external_ids={'neutron:host_id': self.chassis},
+                               up=[True])
+        old = utils.create_row(external_ids={}, up=[True])
         self.assertTrue(self.event.match_fn(mock.Mock(), row, old))
 
     def test_match_fn_exception(self):
         row = utils.create_row(type=constants.OVN_VM_VIF_PORT_TYPE,
-                               addresses=['mac 192.168.0.1'],
                                up=[False])
         self.assertFalse(self.event.match_fn(mock.Mock(), row, mock.Mock()))
 
@@ -112,9 +130,18 @@ class TestLogicalSwitchPortProviderDeleteEvent(test_base.TestCase):
                                up=[True])
         self.assertTrue(self.event.match_fn(event, row, old))
 
-    def test_match_fn_exception(self):
+    def test_match_fn_update_chassis(self):
+        event = self.event.ROW_UPDATE
         row = utils.create_row(type=constants.OVN_VM_VIF_PORT_TYPE,
                                addresses=['mac 192.168.0.1'],
+                               external_ids={'neutron:host_id': 'chassis2'},
+                               up=[True])
+        old = utils.create_row(external_ids={'neutron:host_id': self.chassis},
+                               up=[True])
+        self.assertTrue(self.event.match_fn(event, row, old))
+
+    def test_match_fn_exception(self):
+        row = utils.create_row(type=constants.OVN_VM_VIF_PORT_TYPE,
                                up=[False])
         self.assertFalse(self.event.match_fn(mock.Mock(), row, mock.Mock()))
 
@@ -179,6 +206,17 @@ class TestLogicalSwitchPortFIPCreateEvent(test_base.TestCase):
         old = utils.create_row(options={}, up=[True])
         self.assertTrue(self.event.match_fn(mock.Mock(), row, old))
 
+    def test_match_fn_chassis_change_external_ids(self):
+        row = utils.create_row(
+            type=constants.OVN_VM_VIF_PORT_TYPE,
+            addresses=['mac 192.168.0.1'],
+            external_ids={
+                constants.OVN_HOST_ID_EXT_ID_KEY: self.chassis,
+                constants.OVN_FIP_EXT_ID_KEY: 'fip-ip'},
+            up=[True])
+        old = utils.create_row(external_ids={}, up=[True])
+        self.assertTrue(self.event.match_fn(mock.Mock(), row, old))
+
     def test_match_fn_status_change(self):
         row = utils.create_row(type=constants.OVN_VM_VIF_PORT_TYPE,
                                addresses=['mac 192.168.0.1'],
@@ -304,6 +342,45 @@ class TestLogicalSwitchPortFIPDeleteEvent(test_base.TestCase):
         old = utils.create_row(up=[True])
         self.assertTrue(self.event.match_fn(event, row, old))
 
+    def test_match_fn_update_external_id(self):
+        event = self.event.ROW_UPDATE
+        row = utils.create_row(
+            type=constants.OVN_VM_VIF_PORT_TYPE,
+            addresses=['mac 192.168.0.1'],
+            external_ids={
+                constants.OVN_HOST_ID_EXT_ID_KEY: 'other-chassis',
+                constants.OVN_FIP_EXT_ID_KEY: 'fip-ip'},
+            up=[True])
+        old = utils.create_row(external_ids={
+            constants.OVN_HOST_ID_EXT_ID_KEY: self.chassis,
+            constants.OVN_FIP_EXT_ID_KEY: 'fip-ip'})
+        self.assertTrue(self.event.match_fn(event, row, old))
+
+    def test_match_fn_update_external_id_remove_fip(self):
+        event = self.event.ROW_UPDATE
+        row = utils.create_row(
+            type=constants.OVN_VM_VIF_PORT_TYPE,
+            addresses=['mac 192.168.0.1'],
+            external_ids={
+                constants.OVN_HOST_ID_EXT_ID_KEY: self.chassis},
+            up=[True])
+        old = utils.create_row(external_ids={
+            constants.OVN_HOST_ID_EXT_ID_KEY: self.chassis,
+            constants.OVN_FIP_EXT_ID_KEY: 'fip-ip'})
+        self.assertTrue(self.event.match_fn(event, row, old))
+
+    def test_match_fn_update_external_id_no_fip(self):
+        event = self.event.ROW_UPDATE
+        row = utils.create_row(
+            type=constants.OVN_VM_VIF_PORT_TYPE,
+            addresses=['mac 192.168.0.1'],
+            external_ids={
+                constants.OVN_HOST_ID_EXT_ID_KEY: self.chassis},
+            up=[True])
+        old = utils.create_row(external_ids={
+            constants.OVN_HOST_ID_EXT_ID_KEY: self.chassis})
+        self.assertFalse(self.event.match_fn(event, row, old))
+
     def test_match_fn_exception(self):
         row = utils.create_row(type=constants.OVN_VM_VIF_PORT_TYPE,
                                addresses=['mac 192.168.0.1'],