From e94e7447aa621792b7c31d71d1c581d473128ec4 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 24 Sep 2025 07:02:01 +0000 Subject: [PATCH] [OVN] The external networks GW chassis must the same as the GW LRP The external networks use ``HA_Chassis_Group`` to bind the port to a chassis. The gateway ``Logical_Router_Port`` registers use ``Gateway_Chassis``. In order to route the external ports traffic through a router gateway port, it is needed that both the external port and the gateway port are collocated in the same chassis. This patch detects when a network (private interface in a router) is connected to a router and if this network has external ports. In this case, the Neutron API changes the external ports ``HA_Chassis_Group`` to match the set of ``Gateway_Chassis`` used in the gateway port. The mechanism used to detect any port addition or deletion to a router is the OVN event ``LogicalRouterPortEvent``, loaded by the OVN L3 plugin. NOTE: along with this patch it is needed a mechanism to permanently sync any change in the ``Logical_Router`` ``Gateway_Chassis`` set and the associated ``HA_Chassis_Group`` of the networks connected to this router. This will be done in a follow up patch in order to submit smaller and better reviewable patches. Related-Bug: #2125553 Signed-off-by: Rodolfo Alonso Hernandez Change-Id: I125d87ffbb33541ff3a4bb2a90915d20d0a171ad --- neutron/common/ovn/utils.py | 30 ++++- .../ovn/mech_driver/ovsdb/ovn_client.py | 98 +++++++++++++++++ neutron/services/ovn_l3/ovsdb_monitor.py | 77 +++++++++++++ neutron/services/ovn_l3/plugin.py | 6 + .../ovn/mech_driver/ovsdb/test_ovn_client.py | 37 +++++++ .../services/ovn_l3/test_ovsdb_monitor.py | 103 ++++++++++++++++++ .../ovn/mech_driver/test_mech_driver.py | 2 + 7 files changed, 351 insertions(+), 2 deletions(-) create mode 100644 neutron/services/ovn_l3/ovsdb_monitor.py create mode 100644 neutron/tests/functional/services/ovn_l3/test_ovsdb_monitor.py diff --git a/neutron/common/ovn/utils.py b/neutron/common/ovn/utils.py index 778d443cb41..2f04a5146f5 100644 --- a/neutron/common/ovn/utils.py +++ b/neutron/common/ovn/utils.py @@ -1094,6 +1094,18 @@ def get_subnets_address_scopes(context, subnets_by_id, fixed_ips, ml2_plugin): return address4_scope_id, address6_scope_id +def get_high_prio_chassis_in_ha_chassis_group(ha_chassis_group): + """Returns (name, priority) of the highest priority HA_Chassis""" + hc_list = [] + for ha_chassis in ha_chassis_group.ha_chassis: + hc_list.append((ha_chassis.chassis_name, ha_chassis.priority)) + hc_list = sorted(hc_list, key=lambda x: x[1], reverse=True) + try: + return hc_list[0] + except IndexError: + return None, None + + def _filter_candidates_for_ha_chassis_group(hcg_info): """Filter a list of chassis candidates for a given HA Chassis Group. @@ -1157,6 +1169,10 @@ def _sync_ha_chassis_group(nb_idl, hcg_info, txn): ha_ch_grp_cmd = txn.add(nb_idl.ha_chassis_group_add( hcg_info.group_name, may_exist=True, external_ids=hcg_info.external_ids)) + else: + # Update the external_ids. + txn.add(nb_idl.db_set('HA_Chassis_Group', hcg_info.group_name, + ('external_ids', hcg_info.external_ids))) max_chassis_number = constants.MAX_CHASSIS_IN_HA_GROUP priority = constants.HA_CHASSIS_GROUP_HIGHEST_PRIORITY @@ -1231,7 +1247,18 @@ def sync_ha_chassis_group_router(context, nb_idl, sb_idl, router_id, txn): @ovn_context(idl_var_name='nb_idl') def sync_ha_chassis_group_network(context, nb_idl, sb_idl, port_id, network_id, txn): - """Syncs the HA Chassis Group for a given network""" + """Syncs the HA_Chassis_Group for a given network""" + # If there is a network associated HA_Chassis_Group, the port will use it + # instead of creating a new one or updating it. + group_name = ovn_name(network_id) + hcg = nb_idl.lookup('HA_Chassis_Group', group_name, default=None) + if hcg: + router_id = hcg.external_ids.get(constants.OVN_ROUTER_ID_EXT_ID_KEY) + if router_id: + # If the HA_Chassis_Group is linked to a router, do not modify it. + ch_name, _ = get_high_prio_chassis_in_ha_chassis_group(hcg) + return hcg.uuid, ch_name + # If there are Chassis marked for hosting external ports create a HA # Chassis Group per external port, otherwise do it at the network # level @@ -1247,7 +1274,6 @@ def sync_ha_chassis_group_network(context, nb_idl, sb_idl, port_id, else: chassis_list = sb_idl.get_gateway_chassis_from_cms_options( name_only=False) - group_name = ovn_name(network_id) ignore_chassis = set() LOG.debug('HA Chassis Group %s is based on network %s', group_name, network_id) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py index 6c5215bf7f1..4bcc2d3c0a1 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py @@ -1599,6 +1599,29 @@ class OVNClient: 'Error: %(error)s', {'router': router_id, 'error': e}) + def update_router_ha_chassis_group(self, context, router_id): + """If the router has GW, bind all external ports to the same GW chassis + + If a router receives or removes the gateway, this method checks all + the connected internal ports and collects its networks. Then it updates + each network, depending on the presence or not of the router gateway. + See LP#2125553. + """ + # Retrieve all internal networks (aka: ext_ids=neutron:is_ext_gw=False) + # connected to this router. + lr_name = utils.ovn_name(router_id) + lr = self._nb_idl.lr_get(lr_name).execute(check_error=True) + network_ids = set() + for lrp in lr.ports: + ext_gw = lrp.external_ids.get(ovn_const.OVN_ROUTER_IS_EXT_GW) + if not strutils.bool_from_string(ext_gw): + net_name = lrp.external_ids[ + ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY] + network_ids.add(utils.get_neutron_name(net_name)) + + for network_id in network_ids: + self.link_network_ha_chassis_group(context, network_id, router_id) + def delete_router(self, context, router_id): """Delete a logical router.""" lrouter_name = utils.ovn_name(router_id) @@ -1810,6 +1833,21 @@ class OVNClient: lsp_address=lsp_address)) self._transaction(commands, txn=txn) + def get_router_port(self, port_id): + try: + return self._nb_idl.lrp_get( + utils.ovn_lrouter_port_name(port_id)).execute( + check_errors=True) + except idlutils.RowNotFound: + return + + def get_router_gateway_ports(self, router_id): + lrps = self._nb_idl.lrp_list(utils.ovn_name(router_id)).execute( + check_errors=True) + return [lrp for lrp in lrps if + strutils.bool_from_string( + lrp.external_ids.get(ovn_const.OVN_ROUTER_IS_EXT_GW))] + def create_router_port(self, context, router_id, router_interface): port = self._plugin.get_port(context, router_interface['port_id']) router = self._l3_plugin.get_router(context, router_id) @@ -2284,6 +2322,66 @@ class OVNClient: if check_rev_cmd.result == ovn_const.TXN_COMMITTED: db_rev.bump_revision(context, network, ovn_const.TYPE_NETWORKS) + def unlink_network_ha_chassis_group(self, network_id): + """Unlink the network HCG to the router + + If the network (including all subnets) has been detached from the + router, the "HA_Chassis_Group" in unlinked from the router by removing + the router_id tag from the external_ids dictionary. + """ + name = utils.ovn_name(network_id) + hcg = self._nb_idl.lookup('HA_Chassis_Group', name, default=None) + if hcg: + self._nb_idl.db_remove( + 'HA_Chassis_Group', name, 'external_ids', + ovn_const.OVN_ROUTER_ID_EXT_ID_KEY).execute( + check_error=True) + + def link_network_ha_chassis_group(self, context, network_id, router_id): + """Link a unified HCG for all network ext. ports if connected to router + + If a network is connected to a router, this method checks if the router + has a gateway port and the corresponding "Gateway_Chassis" registers. + In that case, it creates a unified "HA_Chassis_Group" for this network + and assign it to all external ports. That will collocate the external + ports in the same gateway chassis as the router gateway port, allowing + N/S communication. See LP#2125553 + """ + gw_lrps = self.get_router_gateway_ports(router_id) + if not gw_lrps: + # The router has no GW ports. Remove the "neutron:router_id" tag + # from the "HA_Chassis_Group" associated, if any. + self.unlink_network_ha_chassis_group(network_id) + return + + # Retrieve all "Gateway_Chassis" and build the "chassis_prio" + # dictionary. + chassis_prio = {} + for gc in gw_lrps[0].gateway_chassis: + chassis_prio[gc.chassis_name] = gc.priority + + with self._nb_idl.transaction(check_error=True) as txn: + # Create the "HA_Chassis_Group" associated to this network. + hcg, _ = utils.sync_ha_chassis_group_network_unified( + context, self._nb_idl, self._sb_idl, network_id, router_id, + chassis_prio, txn) + + # Retrieve all LSPs from external ports in this network. + ls = self._nb_idl.lookup('Logical_Switch', + utils.ovn_name(network_id)) + for lsp in (lsp for lsp in ls.ports if + lsp.type == ovn_const.LSP_TYPE_EXTERNAL): + # NOTE(ralonsoh): this is a protection check but all external + # ports must have "HA_Chassis_Group". If the "HA_Chassis_Group" + # register is for this port only, remove it. + group_name = utils.ovn_extport_chassis_group_name(lsp.name) + if (lsp.ha_chassis_group and + lsp.ha_chassis_group[0].name == group_name): + txn.add(self._nb_idl.ha_chassis_group_del( + lsp.ha_chassis_group[0].name, if_exists=True)) + txn.add(self._nb_idl.db_set('Logical_Switch_Port', lsp.uuid, + ('ha_chassis_group', hcg))) + def _add_subnet_dhcp_options(self, context, subnet, network, ovn_dhcp_options=None): if utils.is_dhcp_options_ignored(subnet): diff --git a/neutron/services/ovn_l3/ovsdb_monitor.py b/neutron/services/ovn_l3/ovsdb_monitor.py new file mode 100644 index 00000000000..69a5cfe2f7e --- /dev/null +++ b/neutron/services/ovn_l3/ovsdb_monitor.py @@ -0,0 +1,77 @@ +# Copyright 2025 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. + +from neutron_lib import context as neutron_context +from neutron_lib.plugins import constants +from neutron_lib.plugins import directory +from oslo_utils import strutils +from ovsdbapp.backend.ovs_idl import event as row_event + +from neutron.common.ovn import constants as ovn_const +from neutron.common.ovn import utils + + +class LogicalRouterPortEvent(row_event.RowEvent): + """Logical_Router_Port create/delete event. + + If a Logical_Router_Port is deleted or added, first check if this LRP is a + gateway port or not. Then update the corresponding network (or networks) + HA_Chassis_Group, matching the Logical_Router Gateway_Chassis. + See LP#2125553. + """ + def __init__(self, driver): + self.driver = driver + self.l3_plugin = directory.get_plugin(constants.L3) + self.admin_context = neutron_context.get_admin_context() + table = 'Logical_Router_Port' + events = (self.ROW_CREATE, self.ROW_DELETE) + super().__init__(events, table, None) + + def match_fn(self, event, row, old): + if event == self.ROW_DELETE: + # Check if the LR has another port in the same network. If that is + # the case, do nothing. + ls_name = row.external_ids[ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY] + lr_name = row.external_ids[ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY] + lr = self.driver._nb_ovn.lookup('Logical_Router', lr_name) + for lrp in (lrp for lrp in lr.ports if lrp.name != row.name): + if (ls_name == lrp.external_ids[ + ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY]): + return False + return True + + # event == self.ROW_CREATE + return True + + def run(self, event, row, old=None): + ext_gw = row.external_ids.get(ovn_const.OVN_ROUTER_IS_EXT_GW) + router_id = utils.get_neutron_name( + row.external_ids[ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY]) + net_id = utils.get_neutron_name( + row.external_ids[ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY]) + if event == self.ROW_DELETE: + if not strutils.bool_from_string(ext_gw): # LRP internal port. + self.l3_plugin._ovn_client.unlink_network_ha_chassis_group( + net_id) + else: # LRP gateway port. + self.l3_plugin._ovn_client.update_router_ha_chassis_group( + self.admin_context, router_id) + + else: # event == self.ROW_CREATE + if not strutils.bool_from_string(ext_gw): # LRP internal port. + self.l3_plugin._ovn_client.link_network_ha_chassis_group( + self.admin_context, net_id, router_id) + else: # LRP gateway port. + self.l3_plugin._ovn_client.update_router_ha_chassis_group( + self.admin_context, router_id) diff --git a/neutron/services/ovn_l3/plugin.py b/neutron/services/ovn_l3/plugin.py index 56eba404707..67249aaa276 100644 --- a/neutron/services/ovn_l3/plugin.py +++ b/neutron/services/ovn_l3/plugin.py @@ -51,6 +51,7 @@ from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client from neutron.quota import resource_registry from neutron.scheduler import l3_ovn_scheduler from neutron.services.ovn_l3 import exceptions as ovn_l3_exc +from neutron.services.ovn_l3 import ovsdb_monitor from neutron.services.ovn_l3.service_providers import driver_controller from neutron.services.portforwarding.drivers.ovn import driver \ as port_forwarding @@ -175,6 +176,11 @@ class OVNL3RouterPlugin(service_base.ServicePluginBase, if not self._nb_ovn or not self._sb_ovn: raise ovn_l3_exc.MechanismDriverOVNNotReady() + # Register needed events. + self._nb_ovn.idl.notify_handler.watch_events([ + ovsdb_monitor.LogicalRouterPortEvent(self), + ]) + def _add_neutron_router_interface(self, context, router_id, interface_info): try: diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py index e8d728eb8b5..98861b71f68 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +from unittest import mock + from neutron_lib.api.definitions import external_net from neutron_lib.api.definitions import network_mtu as mtu_def from neutron_lib.api.definitions import provider_net @@ -499,3 +501,38 @@ class TestOVNClient(base.TestOVNFunctionalBase, dhcp_options.options['ntp_server']) self.assertEqual('1.2.3.6', dhcp_options.options['wpad']) + + def test_update_ha_chassis_group_linked_to_router(self): + # Create a router with multiple networks (internal, external). The + # method `link_network_ha_chassis_group` must be called for all + # internal networks. + num_private_subnets = 5 + ovn_client = self.mech_driver._ovn_client + net_arg = {provider_net.NETWORK_TYPE: 'geneve', + external_net.EXTERNAL: True} + with self.network('external', as_admin=True, + arg_list=tuple(net_arg.keys()), **net_arg) as net: + with self.subnet(net, cidr='10.100.0.0/24'): + ext_gw = {'network_id': net['network']['id']} + with self.router(external_gateway_info=ext_gw) as router: + router_id = router['router']['id'] + + net_ids = [] + for idx in range(num_private_subnets): + with self.network('internal' + str(idx)) as net: + net_ids.append(net['network']['id']) + with self.subnet(net, cidr=f'10.{idx}.0.0/24') as subnet: + subnet_id = subnet['subnet']['id'] + self._router_interface_action( + 'add', router_id, subnet_id, None) + + lr_name = ovn_utils.ovn_name(router_id) + lr = self.nb_api.lookup('Logical_Router', lr_name) + self.assertEqual(num_private_subnets + 1, len(lr.ports)) + with mock.patch.object(ovn_client, 'link_network_ha_chassis_group') as\ + mock_link: + ovn_client.update_router_ha_chassis_group(self.context, router_id) + calls = [mock.call(self.context, net_id, router_id) + for net_id in net_ids] + self.assertEqual(num_private_subnets, len(mock_link.mock_calls)) + mock_link.assert_has_calls(calls, any_order=True) diff --git a/neutron/tests/functional/services/ovn_l3/test_ovsdb_monitor.py b/neutron/tests/functional/services/ovn_l3/test_ovsdb_monitor.py new file mode 100644 index 00000000000..0a0338f642d --- /dev/null +++ b/neutron/tests/functional/services/ovn_l3/test_ovsdb_monitor.py @@ -0,0 +1,103 @@ +# Copyright 2025 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. + +from unittest import mock + +from neutron_lib.api.definitions import external_net +from neutron_lib.plugins import constants as plugin_constants +from neutron_lib.plugins import directory + +from neutron.common import utils as n_utils +from neutron.tests.functional import base +from neutron.tests.unit.api import test_extensions +from neutron.tests.unit.extensions import test_l3 + + +class TestLogicalRouterPortEvent( + base.TestOVNFunctionalBase, + test_l3.L3NatTestCaseMixin): + + def setUp(self, **kwargs): + super().setUp(**kwargs) + self.chassis = self.add_fake_chassis('ovs-host1') + self.l3_plugin = directory.get_plugin(plugin_constants.L3) + self.l3_plugin._post_fork_initialize(mock.ANY, mock.ANY, mock.ANY) + self.ext_api = test_extensions.setup_extensions_middleware( + test_l3.L3TestExtensionManager()) + kwargs = {'arg_list': (external_net.EXTERNAL,), + external_net.EXTERNAL: True} + self.net_ext = self._make_network( + self.fmt, 'net_ext', True, as_admin=True, **kwargs) + self.subnet = self._make_subnet(self.fmt, self.net_ext, '20.0.10.1', + '20.0.10.0/24') + self.router = self._make_router(self.fmt, self._tenant_id) + self.router_id = self.router['router']['id'] + self.net_ext_id = self.net_ext['network']['id'] + self.subnet_id = self.subnet['subnet']['id'] + + def test_add_and_delete_gw_network(self): + def is_called(): + try: + mock_update_router.assert_called_once_with( + mock.ANY, self.router_id) + return True + except AssertionError: + return False + + with mock.patch.object( + self.l3_plugin._ovn_client, + 'update_router_ha_chassis_group') as mock_update_router: + self._add_external_gateway_to_router(self.router_id, + self.net_ext_id) + n_utils.wait_until_true(is_called, timeout=10) + mock_update_router.reset_mock() + self._remove_external_gateway_from_router( + self.router_id, self.net_ext_id, external_gw_info={}) + n_utils.wait_until_true(is_called, timeout=10) + + def test_add_private_network(self): + def is_called(): + try: + mock_link.assert_called_once_with( + mock.ANY, self.net_ext_id, self.router_id) + return True + except AssertionError: + return False + + with mock.patch.object( + self.l3_plugin._ovn_client, + 'link_network_ha_chassis_group') as mock_link: + self._router_interface_action( + 'add', self.router_id, self.subnet_id, None) + n_utils.wait_until_true(is_called, timeout=10) + + def test_delete_private_network(self): + def is_called(): + try: + mock_unlink.assert_called_once_with(self.net_ext_id) + return True + except AssertionError: + return False + + with mock.patch.object( + self.l3_plugin._ovn_client, + 'link_network_ha_chassis_group'), \ + mock.patch.object( + self.l3_plugin._ovn_client, + 'unlink_network_ha_chassis_group') as mock_unlink: + self._router_interface_action( + 'add', self.router_id, self.subnet_id, None) + self._router_interface_action( + 'remove', self.router_id, self.subnet_id, None) + n_utils.wait_until_true(is_called, timeout=10) diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index 152ba1f9e1b..f303d5404e5 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -2836,6 +2836,7 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): self.sb_ovn.get_chassis_host_for_port.return_value = { ch4.name, ch5.name} + self.nb_ovn.lookup.return_value = None ovn_utils.sync_ha_chassis_group_network( self.context, self.nb_ovn, self.sb_ovn, fake_port['id'], @@ -2879,6 +2880,7 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): self.sb_ovn.get_extport_chassis_from_cms_options.return_value = [] self.sb_ovn.get_gateway_chassis_from_cms_options.return_value = [ ch0, ch1, ch2, ch3, ch4, ch5] + self.nb_ovn.lookup.return_value = None ovn_utils.sync_ha_chassis_group_network( self.context, self.nb_ovn, self.sb_ovn, fake_port['id'],