[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 <[email protected]>
(cherry picked from commit 06dbc52)

Author: umago

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

@@ -159,6 +159,10 @@ def schema_helper(cls):
                                                         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

@@ -619,7 +619,7 @@ def check_for_ha_chassis_group(self):
 
         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)
@@ -630,21 +630,36 @@ def check_for_mcast_flood_reports(self):
         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_value = options.get(
-                    ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS)
-                if mcast_flood_value == 'false':
+            mcast_flood_reports_value = options.get(
+                ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS)
+
+            if self._ovn_client.is_mcast_flood_broken:
+                if port_type in ("vtep", ovn_const.LSP_TYPE_LOCALPORT,
+                                 "router"):
                     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'})
-            cmds.append(self._nb_idl.lsp_set_options(port.name, **options))
+                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

@@ -39,6 +39,7 @@
 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
 
@@ -96,6 +97,7 @@ def __init__(self, nb_idl, sb_idl):
 
         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)
@@ -298,6 +300,20 @@ def update_lsp_host_info(self, context, db_port, up=True):
 
         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)
@@ -439,12 +455,8 @@ def _get_port_options(self, port):
             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,
-        # according to core OVN developers it shouldn't cause any harm
-        # 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'):
+        if self.is_mcast_flood_broken and 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

@@ -164,6 +164,7 @@ def __init__(self, **kwargs):
         self.ha_chassis_group_add_chassis = mock.Mock()
         self.ha_chassis_group_del_chassis = mock.Mock()
         self.lrp_get = mock.Mock()
+        self.get_schema_version = mock.Mock(return_value='3.6.0')
 
 
 class FakeOvsdbSbOvnIdl(object):
@@ -188,6 +189,7 @@ def __init__(self, **kwargs):
         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

@@ -537,7 +537,8 @@ def test_check_port_has_address_scope(self):
             "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',
@@ -578,14 +579,86 @@ def test_check_for_mcast_flood_reports(self):
         self.assertRaises(periodics.NeverAgain,
                           self.periodic.check_for_mcast_flood_reports)
 
-        # Assert only lsp1, lsp5 and lsp6 were called because they are the
-        # only ones meeting the criteria
+        # Assert only lsp1 and lsp5 were called because they are the
+        # 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('lsp6', mcast_flood_reports='true', mcast_flood='false')]
+            mock.call('lsp5', mcast_flood_reports='true')]
 
         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_router_mac_binding_options(self):
         nb_idl = self.fake_ovn_client._nb_idl

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.