Merge "[OVN] Disable the mcast_flood_reports option for LSPs" into stable/2023.1

Author: Zuul

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.