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.