From eaf217d89da5184ff660accda76a8fe1de5a16d4 Mon Sep 17 00:00:00 2001 From: Luis Tomas Bolivar Date: Fri, 19 May 2023 07:38:40 +0200 Subject: [PATCH] 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 | 3 + ovn_bgp_agent/drivers/openstack/utils/ovn.py | 9 +- .../openstack/watchers/base_watcher.py | 15 +- .../openstack/watchers/nb_bgp_watcher.py | 151 ++++++++++++------ .../unit/drivers/openstack/utils/test_ovn.py | 25 ++- .../openstack/watchers/test_nb_bgp_watcher.py | 83 +++++++++- 6 files changed, 230 insertions(+), 56 deletions(-) diff --git a/ovn_bgp_agent/constants.py b/ovn_bgp_agent/constants.py index d6f9e940..3bb70103 100644 --- a/ovn_bgp_agent/constants.py +++ b/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' diff --git a/ovn_bgp_agent/drivers/openstack/utils/ovn.py b/ovn_bgp_agent/drivers/openstack/utils/ovn.py index 04dd5681..17b34d49 100644 --- a/ovn_bgp_agent/drivers/openstack/utils/ovn.py +++ b/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 - row.options.get('requested-chassis') == chassis): + if (row.external_ids and + 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 diff --git a/ovn_bgp_agent/drivers/openstack/watchers/base_watcher.py b/ovn_bgp_agent/drivers/openstack/watchers/base_watcher.py index 8548c66f..bd828546 100644 --- a/ovn_bgp_agent/drivers/openstack/watchers/base_watcher.py +++ b/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 diff --git a/ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py b/ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py index bf83d47e..a4643cb3 100644 --- a/ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py +++ b/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,28 +139,41 @@ 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, 'options'): - # check chassis change - 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'): - # check fips addition - old_port_fip = old.external_ids.get( - constants.OVN_FIP_EXT_ID_KEY) - if not old_port_fip or current_port_fip != old_port_fip: - return True 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'): + old_chassis, _ = self._get_chassis(old) + if not old_chassis or current_chassis != old_chassis: + return True + if hasattr(old, 'external_ids'): + # check fips addition + old_port_fip = old.external_ids.get( + constants.OVN_FIP_EXT_ID_KEY) + if not old_port_fip or current_port_fip != old_port_fip: + return True + else: # by default expect the chassis information at external-ids + if hasattr(old, 'external_ids'): + # 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 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,31 +211,50 @@ class LogicalSwitchPortFIPDeleteEvent(base_watcher.LSPChassisEvent): return True return False - if hasattr(old, 'options'): - # check chassis change - old_chassis = old.options.get(constants.OVN_REQUESTED_CHASSIS) - if (not old_chassis or old_chassis != self.agent.chassis): - return False - if current_chassis != old_chassis and current_port_fip: - return True - # There was no change in chassis, so only progress if the - # chassis matches - if current_chassis != self.agent.chassis: - return False - if hasattr(old, 'external_ids'): - # 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 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, _ = 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: + return True + # There was no change in chassis, so only progress if the + # chassis matches + if current_chassis != self.agent.chassis: + return False + if hasattr(old, 'external_ids'): + # 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 + else: # by default expect the chassis information at external-ids + if hasattr(old, 'external_ids'): + # check chassis change + 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: + 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 return False diff --git a/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_ovn.py b/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_ovn.py index 76f54829..fa982c6e 100644 --- a/ovn_bgp_agent/tests/unit/drivers/openstack/utils/test_ovn.py +++ b/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) diff --git a/ovn_bgp_agent/tests/unit/drivers/openstack/watchers/test_nb_bgp_watcher.py b/ovn_bgp_agent/tests/unit/drivers/openstack/watchers/test_nb_bgp_watcher.py index 4051412b..eb4d7bdb 100644 --- a/ovn_bgp_agent/tests/unit/drivers/openstack/watchers/test_nb_bgp_watcher.py +++ b/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'],