From 9736efd891b68605151e88ae08caedbef15d9d38 Mon Sep 17 00:00:00 2001 From: Lucas Alvares Gomes Date: Mon, 8 Mar 2021 11:44:24 +0000 Subject: [PATCH] [OVN] Set mcast_flood_reports on LSPs This patch enables the "mcast_flood_reports" and "mcast_flood" (on provnet ports only) options in the Logical Switch Ports in the OVN driver. Without these options, the ovn-controller will consume the IGMP queries and won't send it to the LSP ports, meaning that external IGMP queries will never arrive to the VMs. In talks to the core OVN team, it was suggested [0] to enable the "mcast_flood_reports" option by default in the OVN driver (at least until fixed in core OVN) as a workaround to this problem. And, to avoid having to update all ports (which can be many) based on the igmp_snooping_enable configuration option, we are always setting "mcast_flood_reports" to "true" in the LSPs. This won't cause any harm (also confirmed by core OVN developers [0]) since it will be ignored if multicast snoop is disabled. [0] https://bugzilla.redhat.com/show_bug.cgi?id=1933990#c3 Closes-Bug: #1918108 Change-Id: I99a60b9af94b8208b5818b035e189822981bb269 Signed-off-by: Lucas Alvares Gomes (cherry picked from commit b04d64b90f2822222f96ddd726d379a2f19b539e) --- neutron/common/ovn/constants.py | 2 + .../ovn/mech_driver/ovsdb/maintenance.py | 28 ++++++++++++++ .../ovn/mech_driver/ovsdb/ovn_client.py | 13 ++++++- .../ovn/mech_driver/test_mech_driver.py | 6 +-- .../ovn/mech_driver/ovsdb/test_maintenance.py | 37 +++++++++++++++++++ .../ovn/mech_driver/test_mech_driver.py | 12 ++++-- ...-mcast-flood-reports-80fb529120f2af1c.yaml | 7 ++++ 7 files changed, 97 insertions(+), 8 deletions(-) create mode 100644 releasenotes/notes/ovn-mcast-flood-reports-80fb529120f2af1c.yaml diff --git a/neutron/common/ovn/constants.py b/neutron/common/ovn/constants.py index 403ffdedb2b..d7263443f19 100644 --- a/neutron/common/ovn/constants.py +++ b/neutron/common/ovn/constants.py @@ -275,6 +275,8 @@ LSP_TYPE_VIRTUAL = 'virtual' LSP_TYPE_EXTERNAL = 'external' LSP_OPTIONS_VIRTUAL_PARENTS_KEY = 'virtual-parents' LSP_OPTIONS_VIRTUAL_IP_KEY = 'virtual-ip' +LSP_OPTIONS_MCAST_FLOOD_REPORTS = 'mcast_flood_reports' +LSP_OPTIONS_MCAST_FLOOD = 'mcast_flood' HA_CHASSIS_GROUP_DEFAULT_NAME = 'default_ha_chassis_group' HA_CHASSIS_GROUP_HIGHEST_PRIORITY = 32767 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 e5a858be6e8..5c626f9fa15 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py @@ -644,6 +644,34 @@ class DBInconsistenciesPeriodics(SchemaAwarePeriodicsBase): txn.add(cmd) raise periodics.NeverAgain() + # TODO(lucasagomes): Remove this in the Y 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) + 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", "localport", "router"): + continue + + options = port.options + if ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS in options: + continue + + options.update({ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true'}) + if port_type == ovn_const.LSP_TYPE_LOCALNET: + options.update({ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'true'}) + + cmds.append(self._nb_idl.lsp_set_options(port.name, **options)) + + if cmds: + with self._nb_idl.transaction(check_error=True) as txn: + for cmd in cmds: + txn.add(cmd) + + raise periodics.NeverAgain() + class HashRingHealthCheckPeriodics(object): 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 59addca9bd8..dea729913f6 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 @@ -294,6 +294,14 @@ 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', 'localport', 'router'): + options.update({ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true'}) + device_owner = port.get('device_owner', '') sg_ids = ' '.join(utils.get_lsp_security_groups(port)) return OvnPortInfo(port_type, options, addresses, port_security, @@ -1549,6 +1557,9 @@ class OVNClient(object): def create_provnet_port(self, network_id, segment, txn=None): tag = segment.get(segment_def.SEGMENTATION_ID, []) physnet = segment.get(segment_def.PHYSICAL_NETWORK) + options = {'network_name': physnet, + ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true', + ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'true'} cmd = self._nb_idl.create_lswitch_port( lport_name=utils.ovn_provnet_port_name(segment['id']), lswitch_name=utils.ovn_name(network_id), @@ -1556,7 +1567,7 @@ class OVNClient(object): external_ids={}, type=ovn_const.LSP_TYPE_LOCALNET, tag=tag, - options={'network_name': physnet}) + options=options) self._transaction([cmd], txn=txn) def delete_provnet_port(self, network_id, segment): diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index 397a4b3cee4..14da5ebbbd7 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -660,13 +660,11 @@ class TestProvnetPorts(base.TestOVNFunctionalBase): ovn_localnetport = self._find_port_row_by_name( utils.ovn_provnet_port_name(seg_db[0]['id'])) self.assertEqual(ovn_localnetport.tag, [100]) - self.assertEqual(ovn_localnetport.options, - {'network_name': 'physnet1'}) + self.assertEqual(ovn_localnetport.options['network_name'], 'physnet1') seg_2 = self.create_segment(n1['id'], 'physnet2', '222') ovn_localnetport = self._find_port_row_by_name( utils.ovn_provnet_port_name(seg_2['id'])) - self.assertEqual(ovn_localnetport.options, - {'network_name': 'physnet2'}) + self.assertEqual(ovn_localnetport.options['network_name'], 'physnet2') self.assertEqual(ovn_localnetport.tag, [222]) # Delete segments and ensure that localnet 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 c5c1e9825fa..8e0ec4a3378 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 @@ -395,3 +395,40 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight, priority=constants.HA_CHASSIS_GROUP_HIGHEST_PRIORITY - 1) ] nb_idl.ha_chassis_group_add_chassis.assert_has_calls(expected_calls) + + def test_check_for_mcast_flood_reports(self): + nb_idl = self.fake_ovn_client._nb_idl + lsp0 = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={'name': 'lsp0', + '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': {}, + 'type': "vtep"}) + lsp3 = fakes.FakeOvsdbRow.create_one_ovsdb_row( + attrs={'name': 'lsp3', 'options': {}, + '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': 'localnet'}) + + nb_idl.lsp_list.return_value.execute.return_value = [ + lsp0, lsp1, lsp2, lsp3, lsp4, lsp5] + + # 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 lsp1 and lsp5 were called because they are the only + # ones meeting the criteria ("mcast_flood_reports" not yet set, + # and type "" or localnet) + expected_calls = [ + mock.call('lsp1', mcast_flood_reports='true'), + mock.call('lsp5', mcast_flood_reports='true', mcast_flood='true')] + + nb_idl.lsp_set_options.assert_has_calls(expected_calls) diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index 18936ca2ed5..18ce385731a 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -671,7 +671,9 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase): external_ids={}, lport_name=ovn_utils.ovn_provnet_port_name(segments[0]['id']), lswitch_name=ovn_utils.ovn_name(net['id']), - options={'network_name': 'physnet1'}, + options={'network_name': 'physnet1', + ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true', + ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'true'}, tag=2, type='localnet') @@ -2075,7 +2077,9 @@ class TestOVNMechanismDriverSegment(test_segment.HostSegmentMappingTestCase): external_ids={}, lport_name=ovn_utils.ovn_provnet_port_name(new_segment['id']), lswitch_name=ovn_utils.ovn_name(net['id']), - options={'network_name': 'phys_net1'}, + options={'network_name': 'phys_net1', + ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true', + ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'true'}, tag=200, type='localnet') ovn_nb_api.create_lswitch_port.reset_mock() @@ -2087,7 +2091,9 @@ class TestOVNMechanismDriverSegment(test_segment.HostSegmentMappingTestCase): external_ids={}, lport_name=ovn_utils.ovn_provnet_port_name(new_segment['id']), lswitch_name=ovn_utils.ovn_name(net['id']), - options={'network_name': 'phys_net2'}, + options={'network_name': 'phys_net2', + ovn_const.LSP_OPTIONS_MCAST_FLOOD_REPORTS: 'true', + ovn_const.LSP_OPTIONS_MCAST_FLOOD: 'true'}, tag=300, type='localnet') segments = segments_db.get_network_segments( diff --git a/releasenotes/notes/ovn-mcast-flood-reports-80fb529120f2af1c.yaml b/releasenotes/notes/ovn-mcast-flood-reports-80fb529120f2af1c.yaml new file mode 100644 index 00000000000..621d2b14bb0 --- /dev/null +++ b/releasenotes/notes/ovn-mcast-flood-reports-80fb529120f2af1c.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes a configuration problem in the OVN driver that prevented + external IGMP queries from reaching the Virtual Machines. See + `bug 1918108 `_ + for details.