From 62ecacc180caad93d416442a713729088dd558d6 Mon Sep 17 00:00:00 2001 From: Jakub Libosvar Date: Mon, 23 Sep 2024 17:29:01 -0400 Subject: [PATCH] nb driver: Don't expose FIP if the external_mac is not set If the external_mac in the NAT entry is not set then it means two things: 1) The DVR is disabled (see [2]) 2) The external_mac has not been set yet because Neutron doesn't populate the external_mac column of the FIP NAT entry until the associated port with the FIP is bound to a chassis. This is because of LB VIPs that don't get bound [1]. For case 1) we need to expose the FIP on the gateway node where the LRP is hosted. For case 2) we can ignore if the external_mac is not set but then we need another event that would re-act on setting the external_mac. This will be done in a followup patch. [1] https://bugs.launchpad.net/networking-ovn/+bug/1789686 [2] https://bugs.launchpad.net/ovn-bgp-agent/+bug/2056477 Related-Bug: #2073403 Change-Id: Ie24f1f370c44f95840af2d4d2010c20655738ebc Signed-off-by: Jakub Libosvar --- .../drivers/openstack/nb_exceptions.py | 17 +++++++ .../drivers/openstack/nb_ovn_bgp_driver.py | 49 +++++++++++++------ .../openstack/watchers/nb_bgp_watcher.py | 10 ++-- .../openstack/test_nb_ovn_bgp_driver.py | 20 ++++---- .../openstack/watchers/test_nb_bgp_watcher.py | 11 ++--- 5 files changed, 72 insertions(+), 35 deletions(-) create mode 100644 ovn_bgp_agent/drivers/openstack/nb_exceptions.py diff --git a/ovn_bgp_agent/drivers/openstack/nb_exceptions.py b/ovn_bgp_agent/drivers/openstack/nb_exceptions.py new file mode 100644 index 00000000..ed669b3e --- /dev/null +++ b/ovn_bgp_agent/drivers/openstack/nb_exceptions.py @@ -0,0 +1,17 @@ +# Copyright 2024 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +class NATNotFound(Exception): + pass diff --git a/ovn_bgp_agent/drivers/openstack/nb_ovn_bgp_driver.py b/ovn_bgp_agent/drivers/openstack/nb_ovn_bgp_driver.py index 9fdce3fb..b6339831 100644 --- a/ovn_bgp_agent/drivers/openstack/nb_ovn_bgp_driver.py +++ b/ovn_bgp_agent/drivers/openstack/nb_ovn_bgp_driver.py @@ -22,6 +22,7 @@ from oslo_log import log as logging from ovn_bgp_agent import constants from ovn_bgp_agent.drivers import driver_api +from ovn_bgp_agent.drivers.openstack import nb_exceptions from ovn_bgp_agent.drivers.openstack.utils import bgp as bgp_utils from ovn_bgp_agent.drivers.openstack.utils import driver_utils from ovn_bgp_agent.drivers.openstack.utils import ovn @@ -231,9 +232,12 @@ class NBOVNBGPDriver(driver_api.AgentDriverBase): def _ensure_lsp_exposed(self, port): port_fip = port.external_ids.get(constants.OVN_FIP_EXT_ID_KEY) if port_fip: - external_ip, external_mac, ls_name = ( - self.get_port_external_ip_and_ls(port.name)) - if not external_ip or not ls_name: + try: + external_ip, external_mac, ls_name = ( + self.get_port_external_ip_and_ls(port.name)) + except nb_exceptions.NATNotFound as e: + LOG.debug("Logical Switch Port %s does not have all data " + "required in its NAT entry: %s", port.name, e) return return self._expose_fip(external_ip, external_mac, ls_name, port) @@ -551,14 +555,26 @@ class NBOVNBGPDriver(driver_api.AgentDriverBase): def get_port_external_ip_and_ls(self, port): nat_entry = self.nb_idl.get_nat_by_logical_port(port) - if not nat_entry: - return None, None, None - net_id = nat_entry.external_ids.get(constants.OVN_FIP_NET_EXT_ID_KEY) - if not net_id: - return nat_entry.external_ip, nat_entry.external_mac[0], None - else: - ls_name = "neutron-{}".format(net_id) - return nat_entry.external_ip, nat_entry.external_mac[0], ls_name + try: + net_id = nat_entry.external_ids[constants.OVN_FIP_NET_EXT_ID_KEY] + except AttributeError: + # nat_entry is None and has no external_ids attribute + raise nb_exceptions.NATNotFound( + "NAT entry for port %s not found" % port) + except KeyError: + # network ID was not found in the NAT entry + raise nb_exceptions.NATNotFound( + "NAT entry for port %s does not contain network ID in its " + "external_ids" % port) + try: + external_mac = nat_entry.external_mac[0] + except IndexError: + raise nb_exceptions.NATNotFound( + "NAT entry for port %s does not have external_mac set" % ( + port)) + + ls_name = "neutron-{}".format(net_id) + return nat_entry.external_ip, external_mac, ls_name @lockutils.synchronized('nbbgp') def expose_fip(self, ip, mac, logical_switch, row): @@ -1052,11 +1068,12 @@ class NBOVNBGPDriver(driver_api.AgentDriverBase): LOG.debug("Something went wrong, VIP port %s not found", vip_port) return - external_ip, external_mac, ls_name = ( - self.get_port_external_ip_and_ls(vip_lsp.name)) - if not external_ip or not ls_name: - LOG.debug("Something went wrong, no NAT entry for the VIP %s", - vip_port) + try: + external_ip, external_mac, ls_name = ( + self.get_port_external_ip_and_ls(vip_lsp.name)) + except nb_exceptions.NATNotFound as e: + LOG.debug("Something went wrong with the VIP %s: %s", + vip_port, e) return self._expose_fip(external_ip, external_mac, ls_name, vip_lsp) 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 d3e234e7..39bdf12b 100644 --- a/ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py +++ b/ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py @@ -16,6 +16,7 @@ from oslo_concurrency import lockutils from oslo_log import log as logging from ovn_bgp_agent import constants +from ovn_bgp_agent.drivers.openstack import nb_exceptions from ovn_bgp_agent.drivers.openstack.utils import common as common_utils from ovn_bgp_agent.drivers.openstack.utils import driver_utils from ovn_bgp_agent.drivers.openstack.utils import loadbalancer as lb_utils @@ -235,9 +236,12 @@ class LogicalSwitchPortFIPCreateEvent(base_watcher.LSPChassisEvent): return False def _run(self, event, row, old): - external_ip, external_mac, ls_name = ( - self.agent.get_port_external_ip_and_ls(row.name)) - if not external_ip or not ls_name: + try: + external_ip, external_mac, ls_name = ( + self.agent.get_port_external_ip_and_ls(row.name)) + except nb_exceptions.NATNotFound as e: + LOG.debug("Logical Switch Port %s does not have all data required" + " in its NAT entry: %s", row.name, e) return with _SYNC_STATE_LOCK.read_lock(): diff --git a/ovn_bgp_agent/tests/unit/drivers/openstack/test_nb_ovn_bgp_driver.py b/ovn_bgp_agent/tests/unit/drivers/openstack/test_nb_ovn_bgp_driver.py index 6a56e133..4f74b6f2 100644 --- a/ovn_bgp_agent/tests/unit/drivers/openstack/test_nb_ovn_bgp_driver.py +++ b/ovn_bgp_agent/tests/unit/drivers/openstack/test_nb_ovn_bgp_driver.py @@ -18,6 +18,7 @@ from unittest import mock from oslo_config import cfg from ovn_bgp_agent import constants +from ovn_bgp_agent.drivers.openstack import nb_exceptions from ovn_bgp_agent.drivers.openstack import nb_ovn_bgp_driver from ovn_bgp_agent.drivers.openstack.utils import bgp as bgp_utils from ovn_bgp_agent.drivers.openstack.utils import driver_utils @@ -740,9 +741,9 @@ class TestNBOVNBGPDriver(test_base.TestCase): def test_get_port_external_ip_and_ls_no_nat_entry(self): self.nb_idl.get_nat_by_logical_port.return_value = None - ret = self.nb_bgp_driver.get_port_external_ip_and_ls('fake-port') - - self.assertEqual(ret, (None, None, None)) + self.assertRaises( + nb_exceptions.NATNotFound, + self.nb_bgp_driver.get_port_external_ip_and_ls, 'fake-port') def test_get_port_external_ip_and_ls_no_external_id(self): nat_entry = fakes.create_object({ @@ -751,10 +752,9 @@ class TestNBOVNBGPDriver(test_base.TestCase): 'external_mac': ['fake-mac']}) self.nb_idl.get_nat_by_logical_port.return_value = nat_entry - ret = self.nb_bgp_driver.get_port_external_ip_and_ls('fake-port') - - self.assertEqual( - ret, (nat_entry.external_ip, nat_entry.external_mac[0], None)) + self.assertRaises( + nb_exceptions.NATNotFound, + self.nb_bgp_driver.get_port_external_ip_and_ls, 'fake-port') def test_expose_fip(self): ip = '10.0.0.1' @@ -1706,9 +1706,9 @@ class TestNBOVNBGPDriver(test_base.TestCase): constants.OVN_LS_NAME_EXT_ID_KEY: 'net2' }) self.nb_idl.lsp_get.return_value.execute.return_value = vip_lsp - mock_get_port_external_ip_and_ls = mock.patch.object( - self.nb_bgp_driver, 'get_port_external_ip_and_ls').start() - mock_get_port_external_ip_and_ls.return_value = (None, None, None) + mock.patch.object( + self.nb_bgp_driver, 'get_port_external_ip_and_ls', + side_effect=nb_exceptions.NATNotFound).start() mock_expose_fip = mock.patch.object( self.nb_bgp_driver, '_expose_fip').start() 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 15b04f02..444d1b60 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 @@ -16,6 +16,7 @@ from unittest import mock from ovn_bgp_agent import constants +from ovn_bgp_agent.drivers.openstack import nb_exceptions from ovn_bgp_agent.drivers.openstack.watchers import nb_bgp_watcher from ovn_bgp_agent.tests import base as test_base from ovn_bgp_agent.tests import utils @@ -420,15 +421,13 @@ class TestLogicalSwitchPortFIPCreateEvent(test_base.TestCase): ls_name, row) def test_run_no_external_ip(self): - external_ip = None - ls_name = 'logical_switch' - self.agent.get_port_external_ip_and_ls.return_value = (external_ip, - 'mac', - ls_name) + self.agent.get_port_external_ip_and_ls.side_effect = ( + nb_exceptions.NATNotFound) + row = utils.create_row(type=constants.OVN_VM_VIF_PORT_TYPE, addresses=['mac 192.168.0.1'], name='net-id') - self.event.run(mock.Mock(), row, mock.Mock()) + self.event.run(mock.ANY, row, mock.ANY) self.agent.expose_fip.assert_not_called()