Fix IGMP inconsistency across drivers

Prior to this patch, ML2/OVS and ML2/OVN had inconsistent IGMP
configurations. Neutron only exposed one configuration option for IGMP:
igmp_snooping_enabled.

Other features such as IGMP flood, IGMP flood reports and IGMP flood
unregistered were hardcoded differently on each driver (see LP#2044272
for a more details).

These hardcoded values has led to many changes over the years tweaking
them to work on different scenarios but they were never final because
the fix for one case would break the other.

This patch introduces 3 new configuration options for these other IGMP
features that can be enabled or disabled on both backends. Operators
can now fine tune their deployments in the way that will work for them.

As a consequence of the hardcoded values for each driver we had to break
some defaults and, in the case of ML2/OVS, if operators want to keep
things as they were before this patch they will need to enable the new
mcast_flood and mcast_flood_unregistered configuration options.

That said, the for ML2/OVS there was also an inconsistency with the help
string of igmp_snooping_enabled configuration option as it mentioned
that enabling snooping would disable flooding to unregistered ports but
that was not true anymore after the fix [0].

[0] https://bugs.launchpad.net/neutron/+bug/1884723

Closes-Bug: #2044272
Change-Id: Ic4dde46aa0ea2b03362329c87341c83b24d32176
Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>
This commit is contained in:
Lucas Alvares Gomes 2023-11-22 13:05:24 +00:00
parent dcccd7cabe
commit 114ca0f1be
15 changed files with 200 additions and 63 deletions

View File

@ -36,8 +36,7 @@ OVN Database information
The ``igmp_snooping_enable`` configuration from Neutron is translated
into the ``mcast_snoop`` option set in the ``other_config`` column
from the ``Logical_Switch`` table in the OVN Northbound Database
(``mcast_flood_unregistered`` is always "false"):
from the ``Logical_Switch`` table in the OVN Northbound Database:
.. code-block:: bash
@ -71,7 +70,6 @@ command below (populated only when igmp_snooping_enable is True):
groups and broadcast all the multicast traffic. This behavior can
impact when updating/upgrading the OVN services.
Extra information
~~~~~~~~~~~~~~~~~
@ -92,5 +90,11 @@ The permutations from different configurations are:
* With IGMP snooping enabled and multicast group address **is in**
the 224.0.0.X range: IP Multicast traffic **is** flooded.
* Apart from the ``igmp_snooping_enable`` configuration option mentioned
before, there are 3 other configuration options supported by the OVN
driver: ``igmp_flood``, ``igmp_flood_reports`` and
``igmp_flood_unregistered``. Check the :ref:`ML2 configuration
reference page <ML2_CONF>` for more information.
.. _`RFC 4541 session 2.1.2`: https://tools.ietf.org/html/rfc4541

View File

@ -1,3 +1,5 @@
.. _ML2_CONF:
============
ml2_conf.ini
============

View File

@ -334,8 +334,14 @@ class OVSBridge(BaseOVS):
def set_igmp_snooping_state(self, state):
state = bool(state)
# NOTE(lucasagomes): The mcast-snooping-disable-flood-unregistered
# has the opposite value of the config in Neutron. That's because
# IGMP Neutron configs are more value consistent using True to
# enable a feature and False to disable it.
flood_value = ('false' if
cfg.CONF.OVS.igmp_flood_unregistered else 'true')
other_config = {
'mcast-snooping-disable-flood-unregistered': 'false'}
'mcast-snooping-disable-flood-unregistered': flood_value}
with self.ovsdb.transaction() as txn:
txn.add(
self.ovsdb.db_set('Bridge', self.br_name,
@ -344,11 +350,10 @@ class OVSBridge(BaseOVS):
self.ovsdb.db_set('Bridge', self.br_name,
('other_config', other_config)))
def set_igmp_snooping_flood(self, port_name, state):
state = str(state)
def set_igmp_snooping_flood(self, port_name):
other_config = {
'mcast-snooping-flood-reports': state,
'mcast-snooping-flood': state}
'mcast-snooping-flood-reports': ovs_conf.get_igmp_flood_reports(),
'mcast-snooping-flood': ovs_conf.get_igmp_flood()}
self.ovsdb.db_set(
'Port', port_name,
('other_config', other_config)).execute(

View File

@ -229,6 +229,8 @@ class CoreChecks(base.BaseChecks):
self.extra_dhcp_options_check),
(_('OVN support for BM provisioning over IPv6 check'),
self.ovn_for_bm_provisioning_over_ipv6_check),
(_('ML2/OVS IGMP Flood check'),
self.ml2_ovs_igmp_flood_check),
]
@staticmethod
@ -639,3 +641,31 @@ class CoreChecks(base.BaseChecks):
'c5fd51bd154147a567097eaf61fbebc0b5b39e28 which added '
'support for iPXE over IPv6. It is available in '
'OVN >= 23.06.0.'))
@staticmethod
def ml2_ovs_igmp_flood_check(checker):
"""Check for IGMP related traffic behavior changes for ML2/OVS
Since LP#2044272, the default behavior of IGMP related traffic has
changed for the ML2/OVS driver. This check raises a warning and
instruct the user how to configure IGMP to keep the same behavior
as prior to the upgrade.
"""
# NOTE(lucasagomes): igmp_flood_reports is not checked as part
# of this function because its default is already True.
if ('ovn' not in cfg.CONF.ml2.mechanism_drivers and
cfg.CONF.OVS.igmp_snooping_enable and
not cfg.CONF.OVS.igmp_flood_unregistered and
not cfg.CONF.OVS.igmp_flood):
return upgradecheck.Result(
upgradecheck.Code.WARNING,
_('For non-ML2/OVN deployments where ``igmp_snooping_enable`` '
'is enabled, the default behavior of IGMP related traffic '
'has changed after LP#2044272. To keep the same behavior '
'as before please ensure that the configuration options: '
'``igmp_flood_unregistered`` and ``igmp_flood`` are also '
'enabled in the [OVS] section of the configuration file.'))
return upgradecheck.Result(
upgradecheck.Code.SUCCESS,
_('IGMP related traffic configuration is not affected.'))

View File

@ -37,15 +37,40 @@ OPTS = [
help=_('Enable IGMP snooping for integration bridge. If this '
'option is set to True, support for Internet Group '
'Management Protocol (IGMP) is enabled in integration '
'bridge. '
'Setting this option to True will also enable the Open '
'vSwitch mcast-snooping-disable-flood-unregistered '
'flag. This option will disable flooding of '
'bridge.')),
cfg.BoolOpt('igmp_flood', default=False,
help=_('Multicast packets (except reports) are '
'unconditionally forwarded to the ports bridging a '
'logical network to a physical network.')),
cfg.BoolOpt('igmp_flood_reports', default=True,
help=_('Multicast reports are unconditionally forwarded '
'to the ports bridging a logical network to a '
'physical network.')),
cfg.BoolOpt('igmp_flood_unregistered', default=False,
help=_('This option enables or disables flooding of '
'unregistered multicast packets to all ports. '
'The switch will send unregistered multicast packets '
'only to ports connected to multicast routers.')),
'If False, The switch will send unregistered '
'multicast packets only to ports connected to '
'multicast routers.')),
]
def register_ovs_agent_opts(cfg=cfg.CONF):
cfg.register_opts(OPTS, 'OVS')
def get_igmp_snooping_enabled():
return str(cfg.CONF.OVS.igmp_snooping_enable).lower()
def get_igmp_flood():
return str(cfg.CONF.OVS.igmp_flood).lower()
def get_igmp_flood_reports():
return str(cfg.CONF.OVS.igmp_flood_reports).lower()
def get_igmp_flood_unregistered():
return str(cfg.CONF.OVS.igmp_flood_unregistered).lower()

View File

@ -370,10 +370,6 @@ def is_ovn_emit_need_to_frag_enabled():
return cfg.CONF.ovn.ovn_emit_need_to_frag
def is_igmp_snooping_enabled():
return cfg.CONF.OVS.igmp_snooping_enable
def is_ovn_dhcp_disabled_for_baremetal():
return cfg.CONF.ovn.disable_ovn_dhcp_for_baremetal_ports

View File

@ -1558,9 +1558,7 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
"version of OVS does not support tunnels or patch "
"ports. Agent terminated!")
sys.exit(1)
self.int_br.set_igmp_snooping_flood(
self.conf.OVS.int_peer_patch_port,
self.conf.OVS.igmp_snooping_enable)
self.int_br.set_igmp_snooping_flood(self.conf.OVS.int_peer_patch_port)
if self.conf.AGENT.drop_flows_on_start:
self.tun_br.uninstall_flows(cookie=ovs_lib.COOKIE_ANY)
@ -1691,8 +1689,7 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
else:
int_ofport = self.int_br.add_patch_port(
int_if_name, ovs_const.NONEXISTENT_PEER)
self.int_br.set_igmp_snooping_flood(
int_if_name, self.conf.OVS.igmp_snooping_enable)
self.int_br.set_igmp_snooping_flood(int_if_name)
if br.port_exists(phys_if_name):
phys_ofport = br.get_port_ofport(phys_if_name)
else:

View File

@ -34,6 +34,7 @@ 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
from neutron.conf.agent import ovs_conf
from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
from neutron.db import l3_attrs_db
from neutron.db import ovn_hash_ring_db as hash_ring_db
@ -471,18 +472,28 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
# once per lock due to the use of periodics.NeverAgain().
@has_lock_periodic(spacing=600, run_immediately=True)
def check_for_igmp_snoop_support(self):
with self._nb_idl.transaction(check_error=True) as txn:
value = ('true' if ovn_conf.is_igmp_snooping_enabled()
else 'false')
snooping_conf = ovs_conf.get_igmp_snooping_enabled()
flood_conf = ovs_conf.get_igmp_flood_unregistered()
cmds = []
for ls in self._nb_idl.ls_list().execute(check_error=True):
if (ls.other_config.get(ovn_const.MCAST_SNOOP,
None) == value or not ls.name):
snooping = ls.other_config.get(ovn_const.MCAST_SNOOP)
flood = ls.other_config.get(ovn_const.MCAST_FLOOD_UNREGISTERED)
if (not ls.name or (snooping == snooping_conf and
flood == flood_conf)):
continue
txn.add(self._nb_idl.db_set(
cmds.append(self._nb_idl.db_set(
'Logical_Switch', ls.name,
('other_config', {
ovn_const.MCAST_SNOOP: value,
ovn_const.MCAST_FLOOD_UNREGISTERED: 'false'})))
ovn_const.MCAST_SNOOP: snooping_conf,
ovn_const.MCAST_FLOOD_UNREGISTERED: flood_conf})))
if cmds:
with self._nb_idl.transaction(check_error=True) as txn:
for cmd in cmds:
txn.add(cmd)
raise periodics.NeverAgain()
@ -557,12 +568,16 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
# once per lock due to the use of periodics.NeverAgain().
@has_lock_periodic(spacing=600, run_immediately=True)
def check_for_mcast_flood_reports(self):
mcast_flood_conf = ovs_conf.get_igmp_flood()
mcast_flood_reports_conf = ovs_conf.get_igmp_flood_reports()
cmds = []
for port in self._nb_idl.lsp_list().execute(check_error=True):
port_type = port.type.strip()
options = port.options
mcast_flood_reports_value = options.get(
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS)
mcast_flood_value = options.get(
ovn_const.LSP_OPTIONS_MCAST_FLOOD)
if self._ovn_client.is_mcast_flood_broken:
if port_type in ("vtep", ovn_const.LSP_TYPE_LOCALPORT,
@ -591,6 +606,15 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
'Logical_Switch_Port', port.name, 'options',
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS, if_exists=True))
elif (port_type == ovn_const.LSP_TYPE_LOCALNET and (
mcast_flood_conf != mcast_flood_value or
mcast_flood_reports_conf != mcast_flood_reports_value)):
options.update({
ovn_const.LSP_OPTIONS_MCAST_FLOOD: mcast_flood_conf,
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS:
mcast_flood_reports_conf})
cmds.append(self._nb_idl.lsp_set_options(port.name, **options))
if cmds:
with self._nb_idl.transaction(check_error=True) as txn:
for cmd in cmds:

View File

@ -47,6 +47,7 @@ from neutron.common.ovn import acl as ovn_acl
from neutron.common.ovn import constants as ovn_const
from neutron.common.ovn import utils
from neutron.common import utils as common_utils
from neutron.conf.agent import ovs_conf
from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
from neutron.db import ovn_revision_numbers_db as db_rev
from neutron.db import segments_db
@ -1889,9 +1890,12 @@ class OVNClient(object):
physnet = segment.get(segment_def.PHYSICAL_NETWORK)
fdb_enabled = ('true' if ovn_conf.is_learn_fdb_enabled()
else 'false')
options = {'network_name': physnet,
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true',
ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'false',
options = {
'network_name': physnet,
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS:
ovs_conf.get_igmp_flood_reports(),
ovn_const.LSP_OPTIONS_MCAST_FLOOD:
ovs_conf.get_igmp_flood(),
ovn_const.LSP_OPTIONS_LOCALNET_LEARN_FDB: fdb_enabled}
cmd = self._nb_idl.create_lswitch_port(
lport_name=utils.ovn_provnet_port_name(segment['id']),
@ -1920,11 +1924,13 @@ class OVNClient(object):
','.join(common_utils.get_az_hints(network))}}
# Enable IGMP snooping if igmp_snooping_enable is enabled in Neutron
value = 'true' if ovn_conf.is_igmp_snooping_enabled() else 'false'
vlan_transparent = (
'true' if network.get('vlan_transparent') else 'false')
params['other_config'] = {ovn_const.MCAST_SNOOP: value,
ovn_const.MCAST_FLOOD_UNREGISTERED: 'false',
params['other_config'] = {
ovn_const.MCAST_SNOOP:
ovs_conf.get_igmp_snooping_enabled(),
ovn_const.MCAST_FLOOD_UNREGISTERED:
ovs_conf.get_igmp_flood_unregistered(),
ovn_const.VLAN_PASSTHRU: vlan_transparent}
if utils.is_provider_network(network):
params['other_config'][ovn_const.LS_OPTIONS_FDB_AGE_THRESHOLD] = (

View File

@ -19,6 +19,7 @@ from unittest import mock
from neutron_lib import constants as p_const
from neutron_lib.plugins.ml2 import ovs_constants
from neutron_lib.services.qos import constants as qos_constants
from oslo_config import cfg
from oslo_utils import uuidutils
from ovsdbapp.backend.ovs_idl import event
@ -638,7 +639,11 @@ class BaseOVSTestCase(base.BaseSudoTestCase):
port_name = 'test_output_port_2'
self._create_bridge()
self._create_port(port_name)
self.ovs.set_igmp_snooping_flood(port_name, True)
# Enable flood
cfg.CONF.set_override('igmp_flood', True, group='OVS')
cfg.CONF.set_override('igmp_flood_reports', True, group='OVS')
self.ovs.set_igmp_snooping_flood(port_name)
ports_other_config = self.ovs.db_get_val('Port', port_name,
'other_config')
self.assertEqual(
@ -648,7 +653,10 @@ class BaseOVSTestCase(base.BaseSudoTestCase):
'true',
ports_other_config.get('mcast-snooping-flood-reports', '').lower())
self.ovs.set_igmp_snooping_flood(port_name, False)
# Disable flood
cfg.CONF.set_override('igmp_flood', False, group='OVS')
cfg.CONF.set_override('igmp_flood_reports', False, group='OVS')
self.ovs.set_igmp_snooping_flood(port_name)
ports_other_config = self.ovs.db_get_val('Port', port_name,
'other_config')
self.assertEqual(

View File

@ -205,8 +205,10 @@ class OVSBridgeTestCase(OVSBridgeTestBase):
br_other_config = self.ovs.ovsdb.db_find(
'Bridge', ('name', '=', self.br.br_name), columns=['other_config']
).execute()[0]['other_config']
expected_flood_value = ('false' if
cfg.CONF.OVS.igmp_flood_unregistered else 'true')
self.assertEqual(
'false',
expected_flood_value,
br_other_config.get(
'mcast-snooping-disable-flood-unregistered', '').lower())

View File

@ -1684,8 +1684,7 @@ class TestOvsNeutronAgent(object):
'int-br-eth', ovs_constants.NONEXISTENT_PEER),
]
expected_calls += [
mock.call.int_br.set_igmp_snooping_flood(
'int-br-eth', igmp_snooping_enabled),
mock.call.int_br.set_igmp_snooping_flood('int-br-eth'),
mock.call.phys_br.port_exists('phy-br-eth'),
]
if port_exists:
@ -1778,8 +1777,7 @@ class TestOvsNeutronAgent(object):
'int-br-eth', ovs_constants.NONEXISTENT_PEER),
]
expected_calls += [
mock.call.int_br.set_igmp_snooping_flood(
'int-br-eth', False),
mock.call.int_br.set_igmp_snooping_flood('int-br-eth'),
mock.call.phys_br.port_exists('phy-br-eth'),
]
if port_exists:

View File

@ -228,8 +228,7 @@ class TunnelTest(object):
mock.call.port_exists('int-%s' % self.MAP_TUN_BRIDGE),
mock.call.add_patch_port('int-%s' % self.MAP_TUN_BRIDGE,
ovs_constants.NONEXISTENT_PEER),
mock.call.set_igmp_snooping_flood('int-%s' % self.MAP_TUN_BRIDGE,
igmp_snooping),
mock.call.set_igmp_snooping_flood('int-%s' % self.MAP_TUN_BRIDGE),
]
self.mock_int_bridge_expected += [
@ -259,7 +258,7 @@ class TunnelTest(object):
self.mock_int_bridge_expected += [
mock.call.port_exists('patch-tun'),
mock.call.add_patch_port('patch-tun', 'patch-int'),
mock.call.set_igmp_snooping_flood('patch-tun', igmp_snooping),
mock.call.set_igmp_snooping_flood('patch-tun'),
]
self.mock_int_bridge_expected += [
mock.call.get_vif_ports((ovs_lib.INVALID_OFPORT,

View File

@ -52,6 +52,7 @@ from neutron.common.ovn import constants as ovn_const
from neutron.common.ovn import exceptions as ovn_exceptions
from neutron.common.ovn import hash_ring_manager
from neutron.common.ovn import utils as ovn_utils
from neutron.conf.agent import ovs_conf
from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
from neutron.db import db_base_plugin_v2
from neutron.db import ovn_revision_numbers_db
@ -881,9 +882,12 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase):
external_ids={},
lport_name=ovn_utils.ovn_provnet_port_name(segments[0]['id']),
lswitch_name=ovn_utils.ovn_name(net['id']),
options={'network_name': 'physnet1',
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true',
ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'false',
options={
'network_name': 'physnet1',
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS:
ovs_conf.get_igmp_flood_reports(),
ovn_const.LSP_OPTIONS_MCAST_FLOOD:
ovs_conf.get_igmp_flood(),
ovn_const.LSP_OPTIONS_LOCALNET_LEARN_FDB: 'false'},
tag=2,
type='localnet')
@ -3190,9 +3194,12 @@ class TestOVNMechanismDriverSegment(MechDriverSetupBase,
external_ids={},
lport_name=ovn_utils.ovn_provnet_port_name(new_segment['id']),
lswitch_name=ovn_utils.ovn_name(net['id']),
options={'network_name': 'physnet1',
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true',
ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'false',
options={
'network_name': 'physnet1',
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS:
ovs_conf.get_igmp_flood_reports(),
ovn_const.LSP_OPTIONS_MCAST_FLOOD:
ovs_conf.get_igmp_flood(),
ovn_const.LSP_OPTIONS_LOCALNET_LEARN_FDB: 'false'},
tag=200,
type='localnet')
@ -3205,9 +3212,12 @@ class TestOVNMechanismDriverSegment(MechDriverSetupBase,
external_ids={},
lport_name=ovn_utils.ovn_provnet_port_name(new_segment['id']),
lswitch_name=ovn_utils.ovn_name(net['id']),
options={'network_name': 'physnet2',
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true',
ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'false',
options={
'network_name': 'physnet2',
ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS:
ovs_conf.get_igmp_flood_reports(),
ovn_const.LSP_OPTIONS_MCAST_FLOOD:
ovs_conf.get_igmp_flood(),
ovn_const.LSP_OPTIONS_LOCALNET_LEARN_FDB: 'false'},
tag=300,
type='localnet')

View File

@ -0,0 +1,31 @@
---
prelude: >
The ML2/OVS and ML2/OVN drivers had inconsistencies on how IGMP was
configured. Prior to this patch Neutron only exposed a single IGMP
configuration option (``igmp_snooping_enabled``) and other features
such as flooding IGMP packets, flooding reports and flooding to
unregistered ports were hard coded differently in each driver.
This patch introduces Neutron configuration options for those listed
IGMP features.
As part of this work, default values in the IGMP snooping
configuration had to be changed for the ML2/OVS backend. Please
check in the following sections for more details.
features:
- |
New configuration options for IGMP were added in the [OVS] section:
``igmp_flood``, ``igmp_flood_reports`` and ``igmp_flood_unregistered``.
This gives operators full control on how IGMP should be configured for
their deployments.
upgrade:
- |
In order to make IGMP configuration consistent across drivers
some defaults had to be changed for ML2/OVS. This change did not
affect the defaults in the ML2/OVN driver.
If ML2/OVS is used and ``igmp_snooping_enable`` is enabled, in order
to make the IGMP related traffic run as before please ensure that
the following configuration options are enabled after the upgrade.
``[OVS]/igmp_flood_unregistered`` = True
``[OVS]/igmp_flood`` = True
``[OVS]/igmp_flood_reports`` = True