From fb5f8f2a2234329774ab50581197ea2b8dad1cfb Mon Sep 17 00:00:00 2001 From: Lucas Alvares Gomes Date: Tue, 11 Jul 2023 11:29:13 +0100 Subject: [PATCH] [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. Conflicts: neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py neutron/tests/unit/fake_resources.py Change-Id: I8f3f0c2d516e37145eb298b8f51d92fe9905158a Closes-Bug: #2026825 Signed-off-by: Lucas Alvares Gomes (cherry picked from commit 06dbc5227b9208887bab9bc6623098c80156f36d) --- .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 4 + .../ovn/mech_driver/ovsdb/maintenance.py | 41 ++++++--- .../ovn/mech_driver/ovsdb/ovn_client.py | 24 ++++-- neutron/tests/unit/fake_resources.py | 2 + .../ovn/mech_driver/ovsdb/test_maintenance.py | 83 +++++++++++++++++-- ...-mcast_flood_reports-4eee20856ccfc7d7.yaml | 7 ++ 6 files changed, 137 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/ovn-mcast_flood_reports-4eee20856ccfc7d7.yaml diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py index 548cdd43999..3d51f67c395 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py @@ -160,6 +160,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'] 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 756949db66b..32f99043399 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -705,7 +705,7 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase): txn.add(cmd) 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) @@ -716,21 +716,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_value = options.get( + mcast_flood_reports_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'}) - cmds.append(self._nb_idl.lsp_set_options(port.name, **options)) + if self._ovn_client.is_mcast_flood_broken: + 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: 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 6319b433be0..40414623aff 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 @@ -39,6 +39,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 @@ -92,6 +93,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() binding_prof = utils.validate_and_get_data_from_binding_profile(port) @@ -430,12 +446,8 @@ class OVNClient(object): options.update({'requested-chassis': port.get(portbindings.HOST_ID, '')}) - # 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', '') diff --git a/neutron/tests/unit/fake_resources.py b/neutron/tests/unit/fake_resources.py index b9458422498..46aa75dc76a 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/neutron/tests/unit/fake_resources.py @@ -159,6 +159,7 @@ class FakeOvsdbNbOvnIdl(object): self.ha_chassis_group_add_chassis = mock.Mock() self.ha_chassis_group_del_chassis = mock.Mock() self.get_lrouter_port = mock.Mock() + self.get_schema_version = mock.Mock(return_value='3.6.0') class FakeOvsdbSbOvnIdl(object): @@ -184,6 +185,7 @@ class FakeOvsdbSbOvnIdl(object): self.chassis_list = mock.MagicMock() self.is_table_present = mock.Mock() self.is_table_present.return_value = False + self.get_schema_version = mock.Mock(return_value='3.6.0') class FakeOvsdbTransaction(object): 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 e1e772d2f6a..781ffefedf0 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 @@ -492,7 +492,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', @@ -533,14 +534,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 - # 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 diff --git a/releasenotes/notes/ovn-mcast_flood_reports-4eee20856ccfc7d7.yaml b/releasenotes/notes/ovn-mcast_flood_reports-4eee20856ccfc7d7.yaml new file mode 100644 index 00000000000..2b5dd4e0247 --- /dev/null +++ b/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.