[OVN] Disable the mcast_flood_reports option for LSPs

The mcast_flood_reports option was being enabled on LSPs as a workaround
for a problem in core OVN. The issue in core OVN has been fixed and this
workaround is now causing an increase in the number of actions on the
table 38 of OVN (at the risk of hitting a size limit).

This patch disables the mcast_flood_reports option on newer versions of
OVN while keeping the backward compatibility with the old ones.

Since the fix in core OVN does not expose any information to the CMS to
tell us that the issue is fixed this patch uses the NB DB schema version
to determine if this is an old or a new OVN version.

Change-Id: I8f3f0c2d516e37145eb298b8f51d92fe9905158a
Closes-Bug: #2026825
Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py

@@ -159,6 +159,10 @@ class Backend(ovs_idl.Backend):
                                                         cls.schema)
         return cls._schema_helper
 
+    @classmethod
+    def get_schema_version(cls):
+        return cls.schema_helper.schema_json['version']
+
     @classmethod
     def schema_has_table(cls, table_name):
         return table_name in cls.schema_helper.schema_json['tables']

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py

@@ -613,7 +613,7 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
 
         raise periodics.NeverAgain()
 
-    # TODO(lucasagomes): Remove this in the Z cycle
+    # TODO(lucasagomes): Remove this in the B+3 cycle
     # 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)
@@ -624,21 +624,36 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase):
         cmds = []
         for port in self._nb_idl.lsp_list().execute(check_error=True):
             port_type = port.type.strip()
-            if port_type in ("vtep", ovn_const.LSP_TYPE_LOCALPORT, "router"):
-                continue
-
             options = port.options
-            if port_type == ovn_const.LSP_TYPE_LOCALNET:
+            mcast_flood_reports_value = options.get(
-                mcast_flood_value = options.get(
                     ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS)
-                if mcast_flood_value == 'false':
-                    continue
-                options.update({ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'false'})
-            elif ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS in options:
-                continue
 
-            options.update({ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true'})
+            if self._ovn_client.is_mcast_flood_broken:
-            cmds.append(self._nb_idl.lsp_set_options(port.name, **options))
+                if port_type in ("vtep", ovn_const.LSP_TYPE_LOCALPORT,
+                                 "router"):
+                    continue
+
+                if port_type == ovn_const.LSP_TYPE_LOCALNET:
+                    mcast_flood_value = options.pop(
+                        ovn_const.LSP_OPTIONS_MCAST_FLOOD, None)
+                    if mcast_flood_value:
+                        cmds.append(self._nb_idl.db_remove(
+                            'Logical_Switch_Port', port.name, 'options',
+                            ovn_const.LSP_OPTIONS_MCAST_FLOOD,
+                            if_exists=True))
+
+                if mcast_flood_reports_value == 'true':
+                    continue
+
+                options.update(
+                    {ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true'})
+                cmds.append(self._nb_idl.lsp_set_options(port.name, **options))
+
+            elif (mcast_flood_reports_value and port_type !=
+                    ovn_const.LSP_TYPE_LOCALNET):
+                cmds.append(self._nb_idl.db_remove(
+                    'Logical_Switch_Port', port.name, 'options',
+                    ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS, if_exists=True))
 
         if cmds:
             with self._nb_idl.transaction(check_error=True) as txn:

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py

@@ -38,6 +38,7 @@ from oslo_config import cfg
 from oslo_log import log
 from oslo_utils import excutils
 from oslo_utils import timeutils
+from oslo_utils import versionutils
 from ovsdbapp.backend.ovs_idl import idlutils
 import tenacity
 
@@ -95,6 +96,7 @@ class OVNClient(object):
 
         self._plugin_property = None
         self._l3_plugin_property = None
+        self._is_mcast_flood_broken = None
 
         # TODO(ralonsoh): handle the OVN client extensions with an ext. manager
         self._qos_driver = qos_extension.OVNClientQosExtension(driver=self)
@@ -297,6 +299,20 @@ class OVNClient(object):
 
         self._transaction(cmd)
 
+    # TODO(lucasagomes): Remove this method and the logic around the broken
+    # mcast_flood_reports configuration option on any other port that is not
+    # type "localnet" when the fixed version of OVN becomes the norm.
+    # The commit in core OVN fixing this issue is the
+    # https://github.com/ovn-org/ovn/commit/6aeeccdf272bc60630581e46aa42d97f4f56d4fa
+    @property
+    def is_mcast_flood_broken(self):
+        if self._is_mcast_flood_broken is None:
+            schema_version = self._nb_idl.get_schema_version()
+            self._is_mcast_flood_broken = (
+                versionutils.convert_version_to_tuple(schema_version) <
+                (6, 3, 0))
+        return self._is_mcast_flood_broken
+
     def _get_port_options(self, port):
         context = n_context.get_admin_context()
         bp_info = utils.validate_and_get_data_from_binding_profile(port)
@@ -438,12 +454,8 @@ class OVNClient(object):
             if port_type != ovn_const.LSP_TYPE_VIRTUAL:
                 options[ovn_const.LSP_OPTIONS_REQUESTED_CHASSIS_KEY] = chassis
 
-        # TODO(lucasagomes): Enable the mcast_flood_reports by default,
+        if self.is_mcast_flood_broken and port_type not in (
-        # according to core OVN developers it shouldn't cause any harm
+                'vtep', ovn_const.LSP_TYPE_LOCALPORT, 'router'):
-        # and will be ignored when mcast_snoop is False. We can revise
-        # this once https://bugzilla.redhat.com/show_bug.cgi?id=1933990
-        # (see comment #3) is fixed in Core OVN.
-        if port_type not in ('vtep', ovn_const.LSP_TYPE_LOCALPORT, 'router'):
             options.update({ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true'})
 
         device_owner = port.get('device_owner', '')

neutron/tests/unit/fake_resources.py

@@ -165,6 +165,7 @@ class FakeOvsdbNbOvnIdl(object):
         self.ha_chassis_group_del_chassis = mock.Mock()
         self.get_lrouter_gw_ports = mock.Mock()
         self.lrp_get = mock.Mock()
+        self.get_schema_version = mock.Mock(return_value='3.6.0')
 
 
 class FakeOvsdbSbOvnIdl(object):
@@ -189,6 +190,7 @@ class FakeOvsdbSbOvnIdl(object):
         self.is_table_present = mock.Mock()
         self.is_table_present.return_value = False
         self.get_chassis_by_card_serial_from_cms_options = mock.Mock()
+        self.get_schema_version = mock.Mock(return_value='3.6.0')
 
 
 class FakeOvsdbTransaction(object):

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py

@@ -542,7 +542,8 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight,
             "lsp1", external_ids=external_ids
         )
 
-    def test_check_for_mcast_flood_reports(self):
+    def test_check_for_mcast_flood_reports_broken(self):
+        self.fake_ovn_client.is_mcast_flood_broken = True
         nb_idl = self.fake_ovn_client._nb_idl
         lsp0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
             attrs={'name': 'lsp0',
@@ -583,14 +584,86 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight,
         self.assertRaises(periodics.NeverAgain,
                           self.periodic.check_for_mcast_flood_reports)
 
-        # Assert only lsp1, lsp5 and lsp6 were called because they are the
+        # Assert only lsp1 and lsp5 were called because they are the
-        # only ones meeting the criteria
+        # only ones meeting to set mcast_flood_reports to 'true'
         expected_calls = [
             mock.call('lsp1', mcast_flood_reports='true'),
-            mock.call('lsp5', mcast_flood_reports='true', mcast_flood='false'),
+            mock.call('lsp5', mcast_flood_reports='true')]
-            mock.call('lsp6', mcast_flood_reports='true', mcast_flood='false')]
 
         nb_idl.lsp_set_options.assert_has_calls(expected_calls)
+        self.assertEqual(2, nb_idl.lsp_set_options.call_count)
+
+        # Assert only lsp6 and lsp7 were called because they are the
+        # only ones meeting to remove mcast_flood
+        expected_calls = [
+            mock.call('Logical_Switch_Port', 'lsp6', 'options',
+                      constants.LSP_OPTIONS_MCAST_FLOOD,
+                      if_exists=True),
+            mock.call('Logical_Switch_Port', 'lsp7', 'options',
+                      constants.LSP_OPTIONS_MCAST_FLOOD,
+                      if_exists=True)]
+
+        nb_idl.db_remove.assert_has_calls(expected_calls)
+        self.assertEqual(2, nb_idl.db_remove.call_count)
+
+    def test_check_for_mcast_flood_reports(self):
+        self.fake_ovn_client.is_mcast_flood_broken = False
+        nb_idl = self.fake_ovn_client._nb_idl
+
+        lsp0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp0',
+                   'options': {
+                       constants.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true'},
+                   'type': ""})
+        lsp1 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp1', 'options': {}, 'type': ""})
+        lsp2 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp2',
+                   'options': {
+                       constants.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true'},
+                   'type': "vtep"})
+        lsp3 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp3', 'options': {},
+                   'type': constants.LSP_TYPE_LOCALPORT})
+        lsp4 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp4', 'options': {},
+                   'type': "router"})
+        lsp5 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp5', 'options': {},
+                   'type': constants.LSP_TYPE_LOCALNET})
+        lsp6 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp6',
+                   'options': {
+                       constants.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true',
+                       constants.LSP_OPTIONS_MCAST_FLOOD: 'true'},
+                   'type': constants.LSP_TYPE_LOCALNET})
+        lsp7 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'lsp7',
+                   'options': {
+                       constants.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true',
+                       constants.LSP_OPTIONS_MCAST_FLOOD: 'false'},
+                   'type': constants.LSP_TYPE_LOCALNET})
+
+        nb_idl.lsp_list.return_value.execute.return_value = [
+            lsp0, lsp1, lsp2, lsp3, lsp4, lsp5, lsp6, lsp7]
+
+        # Invoke the periodic method, it meant to run only once at startup
+        # so NeverAgain will be raised at the end
+        self.assertRaises(periodics.NeverAgain,
+                          self.periodic.check_for_mcast_flood_reports)
+
+        # Assert only lsp0 and lsp2 were called because they are the
+        # only ones meeting the criteria
+        expected_calls = [
+            mock.call('Logical_Switch_Port', 'lsp0', 'options',
+                      constants.LSP_OPTIONS_MCAST_FLOOD_REPORTS,
+                      if_exists=True),
+            mock.call('Logical_Switch_Port', 'lsp2', 'options',
+                      constants.LSP_OPTIONS_MCAST_FLOOD_REPORTS,
+                      if_exists=True)]
+
+        nb_idl.db_remove.assert_has_calls(expected_calls)
+        self.assertEqual(2, nb_idl.db_remove.call_count)
 
     def test_check_localnet_port_has_learn_fdb(self):
         cfg.CONF.set_override('localnet_learn_fdb', 'True',

releasenotes/notes/ovn-mcast_flood_reports-4eee20856ccfc7d7.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    For OVN versions v22.09.0 and above, the ``mcast_flood_reports`` option
+    is now set to ``false`` on all ports except "localnet" types. In the past,
+    this option was set to ``true`` as a workaround for a bug in core OVN
+    multicast implementation.