From 9f9ee4663fe08d8f3936b6c4f10e51b6363605cc Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 4 Dec 2024 14:02:02 +0000 Subject: [PATCH] [OVN] Set always the GW LRP "gateway_mtu" option NOTE: stable/2024.1 only, as this is a combination of two commits in one to not introduce a regression by doing them separately. There were small changes required to unit tests as the code had changed in 2025.1 where this was originally merged. First commit: If the configuration option "ovn.ovn_emit_need_to_frag" is set, write always the "LSP.options.gateway_mtu" value, using the minimum MTU value of all networks connected to the router. That will limit the N/S traffic MTU regardless if the gateway network MTU is greater or less than the internal network MTU. Conflicts: neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py neutron/tests/unit/services/ovn_l3/test_plugin.py (cherry picked from commit d080b35782c705b105da61af3949d67832efd2c8) Second commit: Revert the network IDs filter in ``_gen_router_port_options`` In [1], a change in the ``network_ids`` filter was introduced. This filter is used to retrieve the networks belonging to a router. However this is used in two places: * To set the gateway MTU. Here all the networks attached to the router must be present. * To calculate the "redirect-type" value in the Logical_Router_Port. Only the internal interfaces must be present; the gateway port should be discarded. NOTE: patch [1] is present since 2025.1 (Epoxy). Conflicts: neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py [1]https://review.opendev.org/c/openstack/neutron/+/937026 Closes-Bug: #2105383 Related-Bug: #2032817 Change-Id: I9cd6e77e47ce16dcf2fc46fc076f1d7ee7a4f9d3 (cherry picked from commit db030749278324b52b6d09f4d55a5d1e288d0abe) (cherry picked from commit 2489d8009e5202af0c93c473c6771f0b6eaacb1b) (cherry picked from commit 197c920cdb75418fa4072e1e175789a61eaab804) Signed-off-by: Brian Haley --- doc/source/admin/ovn/routing.rst | 18 ++++ .../ovn/mech_driver/ovsdb/ovn_client.py | 21 +++-- .../ovn/mech_driver/ovsdb/test_ovn_client.py | 91 +++++++++++++++++++ .../ovn/mech_driver/test_mech_driver.py | 7 ++ .../tests/unit/services/ovn_l3/test_plugin.py | 89 +++++++++++++++++- ...n_router_gateway_mtu-ac69e07b30093193.yaml | 7 ++ 6 files changed, 218 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/ovn_router_gateway_mtu-ac69e07b30093193.yaml diff --git a/doc/source/admin/ovn/routing.rst b/doc/source/admin/ovn/routing.rst index c081ff02766..d8edd4a3fa8 100644 --- a/doc/source/admin/ovn/routing.rst +++ b/doc/source/admin/ovn/routing.rst @@ -180,3 +180,21 @@ encapsulation) it's been included for completeness. Traffic goes directly from instance to instance through br-int in the case of both instances living in the same host (VM1 and VM2), or via encapsulation when living on different hosts (VM3 and VM4). + + +Packet fragmentation +~~~~~~~~~~~~~~~~~~~~ + +The Neutron configuration variable ``[ovn]ovn_emit_need_to_frag`` configures +OVN to emit the "need to frag" packets in case of MTU mismatches. This +configuration option allows Neutron to set, in the router gateway +``Logical_Router_Port``, the option "gateway_mtu". If a packet from any +network reaches the gateway ``Logical_Router_Port``, OVN will send the "need +for frag" message. + +In order to allow any E/W or N/S traffic to cross the router, the value of +"gateway_mtu" will have the lowest MTU value off all networks connected to the +router. This could impact the performance of the traffic using the networks +connected to the router if the MTU defined is low. But the user can unset the +Neutron configuration flag in order to avoid the fragmentation, at the cost +of limiting the communication between networks with different MTUs. 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 9165b96ff90..4cdc719fdfe 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 @@ -1699,8 +1699,6 @@ class OVNClient(object): ls_name = utils.ovn_name(port['network_id']) ls = self._nb_idl.ls_get(ls_name).execute(check_error=True) network_type = ls.external_ids[ovn_const.OVN_NETTYPE_EXT_ID_KEY] - network_mtu = int( - ls.external_ids[ovn_const.OVN_NETWORK_MTU_EXT_ID_KEY]) # For provider networks (VLAN, FLAT types) we need to set the # "reside-on-redirect-chassis" option so the routing for this # logical router port is centralized in the chassis hosting the @@ -1724,15 +1722,16 @@ class OVNClient(object): LOG.debug("Router %s not found", port['device_id']) else: network_ids = {port['network_id'] for port in router_ports} - networks = self._plugin.get_networks( - admin_context, filters={'id': network_ids}) if ovn_conf.is_ovn_emit_need_to_frag_enabled(): - for net in networks: - if net['mtu'] > network_mtu: - options[ - ovn_const.OVN_ROUTER_PORT_GW_MTU_OPTION] = str( - network_mtu) - break + # If this method is called during a port creation, the port + # won't be present yet in the router ports list. It is + # needed not to modify the ``network_ids`` set. + _network_ids = network_ids.union({port['network_id']}) + networks = self._plugin.get_networks( + admin_context, filters={'id': _network_ids}) + # Set the lower MTU of all networks connected to the router + min_mtu = str(min(net['mtu'] for net in networks)) + options[ovn_const.OVN_ROUTER_PORT_GW_MTU_OPTION] = min_mtu if ovn_conf.is_ovn_distributed_floating_ip(): # NOTE(ltomasbo): For VLAN type networks connected through # the gateway port there is a need to set the redirect-type @@ -1741,6 +1740,8 @@ class OVNClient(object): # If there are no VLAN type networks attached we need to # still make it centralized. enable_redirect = False + networks = self._plugin.get_networks( + admin_context, filters={'id': network_ids}) if networks: enable_redirect = all( net.get(pnet.NETWORK_TYPE) in [const.TYPE_VLAN, 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 6595eb5f8a7..6268fccb78c 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 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 from neutron_lib import constants from oslo_config import cfg @@ -150,6 +152,95 @@ class TestOVNClient(testlib_api.MySQLTestCaseMixin, def test_router_reside_chassis_redirect_non_dvr_geneve_net(self): self._test_router_reside_chassis_redirect(False, 'geneve') + def _check_gw_lrp_mtu(self, router_id, mtu): + # Find gateway LRP and check the MTU value. + lr = self.nb_api.lookup('Logical_Router', + ovn_utils.ovn_name(router_id)) + for lrp in lr.ports: + if strutils.bool_from_string( + lrp.external_ids[ + ovn_const.OVN_ROUTER_IS_EXT_GW]): + self.assertEqual(mtu, int(lrp.options['gateway_mtu'])) + return + + self.fail('Gateway Logical_Router_Port not found for ' + 'router %s' % router_id) + + def test_update_network_lrp_mtu_updated(self): + cfg.CONF.set_override('ovn_emit_need_to_frag', True, group='ovn') + self.add_fake_chassis('host1', enable_chassis_as_gw=True, azs=[]) + net_ext_args = {provider_net.NETWORK_TYPE: 'geneve', + external_net.EXTERNAL: True, + mtu_def.MTU: 1300} + net_int_args = {provider_net.NETWORK_TYPE: 'geneve', + mtu_def.MTU: 1400} + with self.network( + 'test-ext-net', as_admin=True, + arg_list=tuple(net_ext_args.keys()), **net_ext_args) as \ + net_ext, self.network( + 'test-int-net', as_admin=True, + arg_list=tuple(net_int_args.keys()), **net_int_args) as \ + net_int: + with self.subnet(net_ext, cidr='10.1.0.0/24'), \ + self.subnet(net_int, cidr='10.2.0.0/24') as snet_int: + ext_gw = {'network_id': net_ext['network']['id']} + with self.router(external_gateway_info=ext_gw) as router: + router_id = router['router']['id'] + self._router_interface_action( + 'add', router_id, snet_int['subnet']['id'], + None) + + self._check_gw_lrp_mtu(router_id, 1300) + + # Update external network MTU. + net_ext_args = {'network': {mtu_def.MTU: 1350}} + req = self.new_update_request('networks', net_ext_args, + net_ext['network']['id']) + req.get_response(self.api) + self._check_gw_lrp_mtu(router_id, 1350) + + def test_add_new_router_interfaces_lrp_mtu_updated(self): + mtu_ext = 1350 + internal_interfaces = [{'cidr': '10.2.0.0/24', 'mtu': mtu_ext - 10}, + {'cidr': '10.3.0.0/24', 'mtu': mtu_ext + 10}, + {'cidr': '10.4.0.0/24', 'mtu': mtu_ext - 20}] + cfg.CONF.set_override('ovn_emit_need_to_frag', True, group='ovn') + self.add_fake_chassis('host1', enable_chassis_as_gw=True, azs=[]) + net_ext_args = {provider_net.NETWORK_TYPE: 'geneve', + external_net.EXTERNAL: True, + mtu_def.MTU: mtu_ext} + # Store the network's MTUs connected to the router. + router_attached_net_mtus = {mtu_ext} + with self.network( + 'test-ext-net', as_admin=True, + arg_list=tuple(net_ext_args.keys()), **net_ext_args) as \ + net_ext: + ext_gw = {'network_id': net_ext['network']['id']} + with self.subnet(net_ext, cidr='10.1.0.0/24'), \ + self.router(external_gateway_info=ext_gw) as router: + router_id = router['router']['id'] + for _int in internal_interfaces: + # Attach a new internal interface to the router. If the new + # network MTU is lower than the minimum MTU connected, the + # GW LRP.options.gateway_mtu will be updated. + net_int_args = {provider_net.NETWORK_TYPE: 'geneve', + mtu_def.MTU: _int['mtu']} + router_attached_net_mtus.add(_int['mtu']) + net_int = self._make_network( + self.fmt, name='nettest', admin_state_up=True, + **net_int_args) + snet_int = self._make_subnet( + self.fmt, net_int, constants.ATTR_NOT_SPECIFIED, + _int['cidr']) + self._router_interface_action( + 'add', router_id, snet_int['subnet']['id'], + None) + + # The GW LRP "gateway_mtu" value should be the minimum + # MTU connected to the router. + self._check_gw_lrp_mtu(router_id, + min(router_attached_net_mtus)) + def test_process_address_group(self): def _find_address_set_for_ag(): as_v4 = self.nb_api.lookup( 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 a2d762f96a4..ff6711b04fb 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 @@ -60,6 +60,7 @@ from neutron.db import ovn_revision_numbers_db from neutron.db import provisioning_blocks from neutron.db import securitygroups_db from neutron.db import segments_db +from neutron.objects import network as network_obj from neutron.plugins.ml2.drivers.ovn.agent import neutron_agent from neutron.plugins.ml2.drivers.ovn.mech_driver import mech_driver from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import impl_idl_ovn @@ -2603,6 +2604,12 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): with self.subnet(network=network) as subnet: with self.port(subnet=subnet, device_owner=const.DEVICE_OWNER_ROUTER_GW) as port: + # Manually set the network DB register MTU without calling + # the network update API. + net_obj = network_obj.Network.get_object( + self.context, id=network['network']['id']) + net_obj.mtu = new_mtu + net_obj.update() grps.return_value = [{'port_id': port['port']['id'], 'network_id':network['network']['id']}] diff --git a/neutron/tests/unit/services/ovn_l3/test_plugin.py b/neutron/tests/unit/services/ovn_l3/test_plugin.py index 859a1fff59f..9e576274aa2 100644 --- a/neutron/tests/unit/services/ovn_l3/test_plugin.py +++ b/neutron/tests/unit/services/ovn_l3/test_plugin.py @@ -76,10 +76,16 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): revision_plugin.RevisionPlugin() # MTU needs to be 1442 instead of 1500 because GENEVE headers size # must be at least 38 when using OVN - network_attrs = {external_net.EXTERNAL: True, 'mtu': 1442} + default_mtu = 1442 + network_attrs = {external_net.EXTERNAL: True, 'mtu': default_mtu} self.fake_network = \ fake_resources.FakeNetwork.create_one_network( attrs=network_attrs).info() + network_attrs.update({'provider:network_type': 'vlan', + 'provider:physical_network': 'physnet1'}) + self.fake_ext_network = \ + fake_resources.FakeNetwork.create_one_network( + attrs=network_attrs).info() self.fake_router_port = {'device_id': '', 'network_id': self.fake_network['id'], 'tenant_id': 'tenant-id', @@ -168,7 +174,10 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): ovn_const.OVN_ROUTER_IS_EXT_GW: 'True', ovn_const.OVN_ROUTER_NAME_EXT_ID_KEY: 'router-id', }, - 'options': {}} + 'options': { + ovn_const.OVN_ROUTER_PORT_GW_MTU_OPTION: str(default_mtu) + } + } self.fake_floating_ip_attrs = {'floating_ip_address': '192.168.0.10', 'fixed_ip_address': '10.0.0.10'} self.fake_floating_ip = fake_resources.FakeFloatingIp.create_one_fip( @@ -251,9 +260,12 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): 'neutron.services.ovn_l3.plugin.OVNL3RouterPlugin._sb_ovn', new_callable=mock.PropertyMock, return_value=fake_resources.FakeOvsdbSbOvnIdl()) - self._start_mock( + self._get_network = self._start_mock( 'neutron.plugins.ml2.plugin.Ml2Plugin.get_network', return_value=self.fake_network) + self._get_networks = self._start_mock( + 'neutron.plugins.ml2.plugin.Ml2Plugin.get_networks', + return_value=[self.fake_network]) self.get_port = self._start_mock( 'neutron.db.db_base_plugin_v2.NeutronDbPluginV2.get_port', return_value=self.fake_router_port) @@ -623,6 +635,8 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.' 'ovn_client.OVNClient._get_v4_network_of_all_router_ports') def test_create_router_with_ext_gw(self, get_rps): + self.fake_ext_gw_port_assert['options'] = {} + self._get_network.return_value = self.fake_ext_network self.l3_inst._nb_ovn.is_col_present.return_value = True self.get_subnet.return_value = self.fake_ext_subnet self.get_port.return_value = self.fake_ext_gw_port @@ -807,6 +821,8 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): @mock.patch('neutron.db.extraroute_db.ExtraRoute_dbonly_mixin.' 'update_router') def test_update_router_with_ext_gw(self, ur, grps): + self.fake_ext_gw_port_assert['options'] = {} + self._get_network.return_value = self.fake_ext_network self.l3_inst._nb_ovn.is_col_present.return_value = True ur.return_value = self.fake_router_with_ext_gw self.get_subnet.side_effect = lambda ctx, sid: { @@ -846,6 +862,8 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): 'update_router') def test_update_router_ext_gw_change_subnet(self, ur, grps, mock_get_gw): + self.fake_ext_gw_port_assert['options'] = {} + self._get_network.return_value = self.fake_ext_network self.l3_inst._nb_ovn.is_col_present.return_value = True mock_get_gw.return_value = [mock.sentinel.GwRoute] fake_old_ext_subnet = {'id': 'old-ext-subnet-id', @@ -919,6 +937,8 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): 'update_router') def test_update_router_ext_gw_change_ip_address(self, ur, grps, mock_get_gw): + self.fake_ext_gw_port_assert['options'] = {} + self._get_network.return_value = self.fake_ext_network self.l3_inst._nb_ovn.is_col_present.return_value = True mock_get_gw.return_value = [mock.sentinel.GwRoute] # Old gateway info with same subnet and different ip address @@ -1928,13 +1948,15 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): grps.return_value = [interface_info] self.get_router.return_value = self.fake_router_with_ext_gw mtu = 1200 - network_attrs = {'id': 'prov-net', 'mtu': mtu} + network_attrs = {'id': 'prov-net', 'mtu': 1200, + 'provider:network_type': 'vlan', + 'provider:physical_network': 'physnet1'} prov_net = fake_resources.FakeNetwork.create_one_network( attrs=network_attrs).info() self.fake_router_port['device_owner'] = ( constants.DEVICE_OWNER_ROUTER_GW) gn.return_value = prov_net - gns.return_value = [self.fake_network] + gns.return_value = [self.fake_network, prov_net] ext_ids = { ovn_const.OVN_NETTYPE_EXT_ID_KEY: constants.TYPE_GENEVE, ovn_const.OVN_NETWORK_MTU_EXT_ID_KEY: mtu, @@ -1969,6 +1991,63 @@ class TestOVNL3RouterPlugin(test_mech_driver.Ml2PluginV2TestCase): mock.ANY, self.fake_router_port, ovn_const.TYPE_ROUTER_PORTS) + @mock.patch('neutron.plugins.ml2.plugin.Ml2Plugin.get_network') + @mock.patch('neutron.plugins.ml2.plugin.Ml2Plugin.get_networks') + @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.' + 'ovn_client.OVNClient._get_router_ports') + @mock.patch('neutron.db.l3_db.L3_NAT_dbonly_mixin.add_router_interface') + def test_add_router_interface_need_to_frag_disabled( + self, ari, grps, gns, gn): + config.cfg.CONF.set_override( + 'ovn_emit_need_to_frag', False, group='ovn') + router_id = 'router-id' + interface_info = {'port_id': 'router-port-id', + 'network_id': 'priv-net'} + ari.return_value = self.fake_router_interface_info + grps.return_value = [interface_info] + self.get_router.return_value = self.fake_router_with_ext_gw + mtu = 1200 + network_attrs = {'id': 'prov-net', 'mtu': 1200, + 'provider:network_type': 'vlan', + 'provider:physical_network': 'physnet1'} + prov_net = fake_resources.FakeNetwork.create_one_network( + attrs=network_attrs).info() + self.fake_router_port['device_owner'] = ( + constants.DEVICE_OWNER_ROUTER_GW) + gn.return_value = prov_net + gns.return_value = [self.fake_network, prov_net] + ext_ids = { + ovn_const.OVN_NETTYPE_EXT_ID_KEY: constants.TYPE_GENEVE, + ovn_const.OVN_NETWORK_MTU_EXT_ID_KEY: mtu, + } + self.l3_inst._nb_ovn.ls_get.return_value.execute.return_value = ( + mock.Mock(external_ids=ext_ids)) + + payload = self._create_payload_for_router_interface(router_id) + self.ovn_drv._process_add_router_interface(resources.ROUTER_INTERFACE, + events.AFTER_CREATE, + self, payload) + + # Make sure that the "gateway_mtu" option was not set to router port + fake_router_port_assert = self.fake_router_port_assert + fake_router_port_assert['options'] = {} + fake_router_port_assert['external_ids'][ + ovn_const.OVN_ROUTER_IS_EXT_GW] = 'True' + + self.l3_inst._nb_ovn.add_lrouter_port.assert_called_once_with( + **fake_router_port_assert) + self.l3_inst._nb_ovn.set_lrouter_port_in_lswitch_port.\ + assert_called_once_with( + 'router-port-id', 'lrp-router-port-id', is_gw_port=True, + lsp_address=ovn_const.DEFAULT_ADDR_FOR_LSP_WITH_PEER) + self.l3_inst._nb_ovn.add_nat_rule_in_lrouter.assert_called_once_with( + 'neutron-router-id', logical_ip='10.0.0.0/24', + external_ip='192.168.1.1', type='snat') + + self.bump_rev_p.assert_called_with( + mock.ANY, self.fake_router_port, + ovn_const.TYPE_ROUTER_PORTS) + @mock.patch('neutron.plugins.ml2.plugin.Ml2Plugin.get_network') @mock.patch('neutron.plugins.ml2.plugin.Ml2Plugin.get_networks') @mock.patch('neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.' diff --git a/releasenotes/notes/ovn_router_gateway_mtu-ac69e07b30093193.yaml b/releasenotes/notes/ovn_router_gateway_mtu-ac69e07b30093193.yaml new file mode 100644 index 00000000000..c807296ddc8 --- /dev/null +++ b/releasenotes/notes/ovn_router_gateway_mtu-ac69e07b30093193.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + Now if the configuration option ``[ovn]ovn_emit_need_to_frag`` is set, + OVN will always set the "gateway_mtu" option in the gateway + ``Logical_Router_Port``. The value defined will be the lowest MTU of all + networks connected to this router.