From c246913b2f21b832989220bc7e7dac6442797040 Mon Sep 17 00:00:00 2001 From: Luis Tomas Bolivar Date: Thu, 23 Mar 2023 16:45:18 +0100 Subject: [PATCH] Ensure redirect-type=bridged not used for geneve networks As part of [1] the redirect-type=bridged flag was enabled by default. However this have the side effect of also decentralizing N/S traffic for geneve tenant networks, breaking the VM connectivity on them when it must be centralized, i.e., when no FIPs are associated to the VMs. This patch differentiates and only enable that flag when the networks conected through that router gateway port are of VLAN/FLAT type. In addition, to avoid MTU issues for the VLAN networks if there are also geneve networks connected to the same router, we re-take the approach on [2] to ensure the traffic is centralized but not tunneled [1] https://review.opendev.org/c/openstack/neutron/+/875644 [2] https://review.opendev.org/c/openstack/neutron/+/875676 Closes-Bug: #2012712 Conflicts: neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py Change-Id: I25e5ee2cf8daee52221a640faa7ac09679742707 (cherry picked from commit 0ec04dd638da8cc9f4d5ebb21a09ea5ccb05623c) --- neutron/common/ovn/constants.py | 2 + .../ovn/mech_driver/ovsdb/maintenance.py | 67 ++++++++++++++- .../ovn/mech_driver/ovsdb/ovn_client.py | 84 ++++++++++++++++--- .../ovn/mech_driver/ovsdb/test_maintenance.py | 69 +++++++++++++-- .../notes/redirect-type-f29e89ca97357fe9.yaml | 24 ++++++ 5 files changed, 227 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/redirect-type-f29e89ca97357fe9.yaml diff --git a/neutron/common/ovn/constants.py b/neutron/common/ovn/constants.py index 93e9035c7d7..8ac8bfc91af 100644 --- a/neutron/common/ovn/constants.py +++ b/neutron/common/ovn/constants.py @@ -372,6 +372,8 @@ LSP_OPTIONS_MCAST_FLOOD = 'mcast_flood' LRP_OPTIONS_RESIDE_REDIR_CH = 'reside-on-redirect-chassis' +LRP_OPTIONS_REDIRECT_TYPE = 'redirect-type' +BRIDGE_REDIRECT_TYPE = "bridged" # Port Binding types PB_TYPE_VIRTUAL = 'virtual' diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py index a6c1f1c628e..7da21a6e36c 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -27,6 +27,7 @@ from neutron_lib import context as n_context from neutron_lib.db import api as db_api from neutron_lib import exceptions as n_exc +from neutron_lib.exceptions import l3 as l3_exc from oslo_config import cfg from oslo_log import log from oslo_utils import timeutils @@ -816,6 +817,66 @@ def update_port_qos_with_external_ids_reference(self): txn.add(cmd) raise periodics.NeverAgain() + # A static spacing value is used here, but this method will only run + # once per lock due to the use of periodics.NeverAgain(). + @periodics.periodic(spacing=600, run_immediately=True) + def check_redirect_type_router_gateway_ports(self): + """Check OVN router gateway ports + Check for the option "redirect-type=bridged" value for + router gateway ports. + """ + if not self.has_lock: + return + context = n_context.get_admin_context() + cmds = [] + gw_ports = self._ovn_client._plugin.get_ports( + context, {'device_owner': [n_const.DEVICE_OWNER_ROUTER_GW]}) + for gw_port in gw_ports: + enable_redirect = False + if ovn_conf.is_ovn_distributed_floating_ip(): + try: + r_ports = self._ovn_client._get_router_ports( + context, gw_port['device_id']) + except l3_exc.RouterNotFound: + LOG.debug("No Router %s not found", gw_port['device_id']) + continue + else: + network_ids = {port['network_id'] for port in r_ports} + networks = self._ovn_client._plugin.get_networks( + context, filters={'id': network_ids}) + # NOTE(ltomasbo): For VLAN type networks connected through + # the gateway port there is a need to set the redirect-type + # option to bridge to ensure traffic is not centralized + # through the controller. + # If there are no VLAN type networks attached we need to + # still make it centralized. + if networks: + enable_redirect = all( + net.get(pnet.NETWORK_TYPE) in [n_const.TYPE_VLAN, + n_const.TYPE_FLAT] + for net in networks) + + lrp_name = utils.ovn_lrouter_port_name(gw_port['id']) + lrp = self._nb_idl.get_lrouter_port(lrp_name) + redirect_value = lrp.options.get( + ovn_const.LRP_OPTIONS_REDIRECT_TYPE) + if enable_redirect: + if redirect_value != ovn_const.BRIDGE_REDIRECT_TYPE: + opt = {ovn_const.LRP_OPTIONS_REDIRECT_TYPE: + ovn_const.BRIDGE_REDIRECT_TYPE} + cmds.append(self._nb_idl.db_set( + 'Logical_Router_Port', lrp_name, ('options', opt))) + else: + if redirect_value == ovn_const.BRIDGE_REDIRECT_TYPE: + cmds.append(self._nb_idl.db_remove( + 'Logical_Router_Port', lrp_name, 'options', + (ovn_const.LRP_OPTIONS_REDIRECT_TYPE))) + if cmds: + with self._nb_idl.transaction(check_error=True) as txn: + for cmd in cmds: + txn.add(cmd) + raise periodics.NeverAgain() + # A static spacing value is used here, but this method will only run # once per lock due to the use of periodics.NeverAgain(). @periodics.periodic(spacing=600, run_immediately=True) @@ -838,9 +899,11 @@ def check_vlan_distributed_ports(self): router_ports = self._ovn_client._plugin.get_ports( context, {'network_id': vlan_net_ids, 'device_owner': n_const.ROUTER_PORT_OWNERS}) - expected_value = ('false' if ovn_conf.is_ovn_distributed_floating_ip() - else 'true') + for rp in router_ports: + expected_value = ( + self._ovn_client._get_reside_redir_for_gateway_port( + rp['device_id'])) lrp_name = utils.ovn_lrouter_port_name(rp['id']) lrp = self._nb_idl.get_lrouter_port(lrp_name) if lrp.options.get( 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 984e88d6bbd..19cb2e81330 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 @@ -1317,6 +1317,11 @@ def update_router_routes(self, context, router_id, add, remove, nexthop=route['nexthop'])) self._transaction(commands, txn=txn) + def _get_router_gw_ports(self, context, router_id): + return self._plugin.get_ports(context, filters={ + 'device_owner': [const.DEVICE_OWNER_ROUTER_GW], + 'device_id': [router_id]}) + def _get_router_ports(self, context, router_id, get_gw_port=False): # _get_router() will raise a RouterNotFound error if there's no router # with the router_id @@ -1548,6 +1553,31 @@ def _gen_router_port_ext_ids(self, port): return ext_ids + def _get_reside_redir_for_gateway_port(self, device_id): + admin_context = n_context.get_admin_context() + reside_redir_ch = 'true' + if ovn_conf.is_ovn_distributed_floating_ip(): + reside_redir_ch = 'false' + try: + router_ports = self._get_router_ports(admin_context, device_id) + except l3_exc.RouterNotFound: + LOG.debug("No Router %s not found", device_id) + else: + network_ids = {port['network_id'] for port in router_ports} + networks = self._plugin.get_networks( + admin_context, filters={'id': network_ids}) + + # NOTE(ltomasbo): not all the networks connected to the router + # are of vlan type, so we won't set the redirect-type=bridged + # on the router gateway port, therefore we need to centralized + # the vlan traffic to avoid tunneling + if networks: + reside_redir_ch = 'true' if any( + net.get(pnet.NETWORK_TYPE) not in [const.TYPE_VLAN, + const.TYPE_FLAT] + for net in networks) else 'false' + return reside_redir_ch + def _gen_router_port_options(self, port, network=None): options = {} admin_context = n_context.get_admin_context() @@ -1562,14 +1592,15 @@ def _gen_router_port_options(self, port, network=None): # FIXME(ltomasbo): Once Bugzilla 2162756 is fixed the # is_provider_network check should be removed if network.get(pnet.NETWORK_TYPE) == const.TYPE_VLAN: - options[ovn_const.LRP_OPTIONS_RESIDE_REDIR_CH] = ( - 'false' if (ovn_conf.is_ovn_distributed_floating_ip() and - not utils.is_provider_network(network)) - else 'true') + reside_redir_ch = self._get_reside_redir_for_gateway_port( + port['device_id']) + options[ovn_const.LRP_OPTIONS_RESIDE_REDIR_CH] = reside_redir_ch is_gw_port = const.DEVICE_OWNER_ROUTER_GW == port.get( 'device_owner') - if is_gw_port and ovn_conf.is_ovn_emit_need_to_frag_enabled(): + + if is_gw_port and (ovn_conf.is_ovn_distributed_floating_ip() or + ovn_conf.is_ovn_emit_need_to_frag_enabled()): try: router_ports = self._get_router_ports(admin_context, port['device_id']) @@ -1578,12 +1609,32 @@ def _gen_router_port_options(self, port, network=None): LOG.debug("Router %s not found", port['device_id']) else: network_ids = {port['network_id'] for port in router_ports} - for net in self._plugin.get_networks(admin_context, - filters={'id': network_ids}): - if net['mtu'] > network['mtu']: - options[ovn_const.OVN_ROUTER_PORT_GW_MTU_OPTION] = str( - network['mtu']) - break + 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 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 + # option to bridge to ensure traffic is not centralized + # through the controller. + # If there are no VLAN type networks attached we need to + # still make it centralized. + enable_redirect = False + if networks: + enable_redirect = all( + net.get(pnet.NETWORK_TYPE) in [const.TYPE_VLAN, + const.TYPE_FLAT] + for net in networks) + if enable_redirect: + options[ovn_const.LRP_OPTIONS_REDIRECT_TYPE] = ( + ovn_const.BRIDGE_REDIRECT_TYPE) + return options def _create_lrouter_port(self, context, router, port, txn=None): @@ -1664,6 +1715,12 @@ def create_router_port(self, context, router_id, router_interface): if utils.is_snat_enabled(router) and cidr: self.update_nat_rules(router, networks=[cidr], enable_snat=True, txn=txn) + if ovn_conf.is_ovn_distributed_floating_ip(): + router_gw_ports = self._get_router_gw_ports(context, + router_id) + for router_port in router_gw_ports: + self._update_lrouter_port(context, router_port, + txn=txn) db_rev.bump_revision(context, port, ovn_const.TYPE_ROUTER_PORTS) @@ -1804,6 +1861,11 @@ def delete_router_port(self, context, port_id, router_id=None, self.update_nat_rules( router, networks=[cidr], enable_snat=False, txn=txn) + if ovn_conf.is_ovn_distributed_floating_ip(): + router_gw_ports = self._get_router_gw_ports(context, router_id) + for router_port in router_gw_ports: + self._update_lrouter_port(context, router_port, txn=txn) + # NOTE(mangelajo): If the port doesn't exist anymore, we # delete the router port as the last operation and update the # revision database to ensure consistency diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py index 3b52025b383..6245f9d791a 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py @@ -16,6 +16,7 @@ from unittest import mock from futurist import periodics +from neutron_lib import constants as n_const from neutron_lib import context from neutron_lib.db import api as db_api from oslo_config import cfg @@ -622,16 +623,76 @@ def test_update_port_qos_with_external_ids_reference(self): ('external_ids', external_ids))] nb_idl.db_set.assert_has_calls(expected_calls) + def _test_check_redirect_type_router_gateway_ports(self, networks, + redirect_value): + self.fake_ovn_client._plugin.get_ports.return_value = [{ + 'device_owner': n_const.DEVICE_OWNER_ROUTER_GW, + 'id': 'fake-id', + 'device_id': 'fake-device-id'}] + self.fake_ovn_client._get_router_ports.return_value = [] + self.fake_ovn_client._plugin.get_networks.return_value = networks + + lrp_redirect = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={ + 'options': {constants.LRP_OPTIONS_REDIRECT_TYPE: "bridged"}}) + lrp_no_redirect = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={ + 'options': {}}) + + # set the opossite so that the value is changed + if redirect_value: + self.fake_ovn_client._nb_idl.get_lrouter_port.return_value = ( + lrp_no_redirect) + else: + self.fake_ovn_client._nb_idl.get_lrouter_port.return_value = ( + lrp_redirect) + + self.assertRaises( + periodics.NeverAgain, + self.periodic.check_redirect_type_router_gateway_ports) + + if redirect_value: + expected_calls = [ + mock.call.db_set('Logical_Router_Port', + mock.ANY, + ('options', {'redirect-type': 'bridged'})) + ] + self.fake_ovn_client._nb_idl.db_set.assert_has_calls( + expected_calls) + else: + expected_calls = [ + mock.call.db_remove('Logical_Router_Port', mock.ANY, + 'options', 'redirect-type') + ] + self.fake_ovn_client._nb_idl.db_remove.assert_has_calls( + expected_calls) + + def test_check_redirect_type_router_gateway_ports_enable_redirect(self): + cfg.CONF.set_override('enable_distributed_floating_ip', 'True', + group='ovn') + networks = [{'network_id': 'foo', + 'provider:network_type': n_const.TYPE_VLAN}] + self._test_check_redirect_type_router_gateway_ports(networks, True) + + def test_check_redirect_type_router_gateway_ports_disable_redirect(self): + cfg.CONF.set_override('enable_distributed_floating_ip', 'True', + group='ovn') + networks = [{'network_id': 'foo', + 'provider:network_type': n_const.TYPE_GENEVE}] + self._test_check_redirect_type_router_gateway_ports(networks, False) + def _test_check_vlan_distributed_ports(self, opt_value=None): fake_net0 = {'id': 'net0'} fake_net1 = {'id': 'net1'} - fake_port0 = {'id': 'port0'} - fake_port1 = {'id': 'port1'} + fake_port0 = {'id': 'port0', 'device_id': 'device0'} + fake_port1 = {'id': 'port1', 'device_id': 'device1'} self.fake_ovn_client._plugin.get_networks.return_value = [ fake_net0, fake_net1] self.fake_ovn_client._plugin.get_ports.return_value = [ fake_port0, fake_port1] + (self.fake_ovn_client._get_reside_redir_for_gateway_port + .return_value) = 'true' fake_lrp = fakes.FakeOvsdbRow.create_one_ovsdb_row( attrs={ @@ -645,8 +706,6 @@ def _test_check_vlan_distributed_ports(self, opt_value=None): self.periodic.check_vlan_distributed_ports) def test_check_vlan_distributed_ports_expected_value(self): - cfg.CONF.set_override('enable_distributed_floating_ip', 'False', - group='ovn') self._test_check_vlan_distributed_ports(opt_value='true') # If the "reside-on-redirect-chassis" option value do match @@ -655,8 +714,6 @@ def test_check_vlan_distributed_ports_expected_value(self): self.fake_ovn_client._nb_idl.db_set.called) def test_check_vlan_distributed_ports_non_expected_value(self): - cfg.CONF.set_override('enable_distributed_floating_ip', 'False', - group='ovn') self._test_check_vlan_distributed_ports(opt_value='false') # If the "reside-on-redirect-chassis" option value does not match diff --git a/releasenotes/notes/redirect-type-f29e89ca97357fe9.yaml b/releasenotes/notes/redirect-type-f29e89ca97357fe9.yaml new file mode 100644 index 00000000000..3d5661b22c1 --- /dev/null +++ b/releasenotes/notes/redirect-type-f29e89ca97357fe9.yaml @@ -0,0 +1,24 @@ +--- +issues: + - | + The `redirect-type=bridged` option is only used if all the tenant networks + connected to the router are of type VLAN or FLAT. In this case their + traffic will be distributed. However, if there is a mix of VLAN/FLAT and + geneve networks connected to the same router, the redirect-type option is + not set, and therefore the traffic for the VLAN/FLAT networks will also be + centralized but not tunneled. +fixes: + - | + [`bug 2003455 `_] + As part of a previous commit + (https://review.opendev.org/c/openstack/neutron/+/875644) the + `redirect-type=bridged` option was set in all the router gateway ports + (cr-lrp ovn ports). However this was breaking the N/S traffic for geneve + tenant networks connected to the provider networks through those routers + with the redirect-type option enabled. To fix this we ensure that the + redirect-type option is only set if all the networks connected to the + router are of VLAN or FLAT type, otherwise we fall back to the default + option. This also means that if there is a mix of VLAN and geneve tenant + networks connected to the same router, the VLAN traffic will be centralized + (but not tunneled). If the traffic for the VLAN/FLAT needs to be + distributed, then it should use a different router.