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 8c74661c154..ad0e80c4f6c 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 @@ -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'] 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 75b814dad6d..2a26c691689 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/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_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 f2db59d45ed..40645df1357 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 @@ -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, - # 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 e05c32547f8..031ab2b632a 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/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): 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 dbc9f274e9c..0788576f105 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 @@ -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 - # 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_localnet_port_has_learn_fdb(self): cfg.CONF.set_override('localnet_learn_fdb', 'True', 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.