From 7339fca5eb4eedbba68e1d7786c92412605a763f Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Thu, 10 Dec 2020 00:10:38 +0100 Subject: [PATCH] Fix multicast traffic with IGMP snooping enabled In the ML2/OVS when igmp_snooping is enabled but there is no external querier multicast traffic will stop working after few minutes as packets will not be flooded to tunnel/external bridges. So this patch sets "mcast-snooping-disable-flood-unregistered" option of the br-int to False (default value) even when igmp_snooping is enabled in the neutron-ovs-agent's config file. Additionally it configures "mcast-snooping-flood-reports" and "mcast-snooping-flood" on patch ports in br-int to True. That way we can provide best effort snooping: multicast isolation where IGMP queriers are available and flood everywhere else? Conflicts: neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py neutron/tests/functional/agent/common/test_ovs_lib.py neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py Closes-Bug: #1884723 Change-Id: Iefa0044dba9e92592295a79448e5d57d9e14a40b (cherry picked from commit b4070c975274f53a4a2caaabeb5af55683232d3d) --- neutron/agent/common/ovs_lib.py | 12 +++++++++- .../openvswitch/agent/ovs_neutron_agent.py | 5 ++++ .../functional/agent/common/test_ovs_lib.py | 24 +++++++++++++++++++ .../tests/functional/agent/test_ovs_lib.py | 5 ++-- .../agent/test_ovs_neutron_agent.py | 15 +++++++++++- .../openvswitch/agent/test_ovs_tunnel.py | 10 ++++++-- 6 files changed, 65 insertions(+), 6 deletions(-) diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index 098a9443693..847d2b8d396 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -287,7 +287,7 @@ class OVSBridge(BaseOVS): def set_igmp_snooping_state(self, state): state = bool(state) other_config = { - 'mcast-snooping-disable-flood-unregistered': str(state)} + 'mcast-snooping-disable-flood-unregistered': 'false'} with self.ovsdb.transaction() as txn: txn.add( self.ovsdb.db_set('Bridge', self.br_name, @@ -296,6 +296,16 @@ class OVSBridge(BaseOVS): self.ovsdb.db_set('Bridge', self.br_name, ('other_config', other_config))) + def set_igmp_snooping_flood(self, port_name, state): + state = str(state) + other_config = { + 'mcast-snooping-flood-reports': state, + 'mcast-snooping-flood': state} + self.ovsdb.db_set( + 'Port', port_name, + ('other_config', other_config)).execute( + check_error=True, log_errors=True) + def create(self, secure_mode=False): other_config = { 'mac-table-size': str(cfg.CONF.OVS.bridge_mac_table_size)} diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py index cb61eb49316..93ffcbaf913 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py @@ -1226,6 +1226,9 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin, "version of OVS does not support tunnels or patch " "ports. Agent terminated!") sys.exit(1) + self.int_br.set_igmp_snooping_flood( + self.conf.OVS.int_peer_patch_port, + self.conf.OVS.igmp_snooping_enable) if self.conf.AGENT.drop_flows_on_start: self.tun_br.uninstall_flows(cookie=ovs_lib.COOKIE_ANY) @@ -1379,6 +1382,8 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin, phys_ofport = br.add_patch_port( phys_if_name, constants.NONEXISTENT_PEER) + self.int_br.set_igmp_snooping_flood( + int_if_name, self.conf.OVS.igmp_snooping_enable) self.int_ofports[physical_network] = int_ofport self.phys_ofports[physical_network] = phys_ofport diff --git a/neutron/tests/functional/agent/common/test_ovs_lib.py b/neutron/tests/functional/agent/common/test_ovs_lib.py index fb8afb1b5d4..2734afaa4dd 100644 --- a/neutron/tests/functional/agent/common/test_ovs_lib.py +++ b/neutron/tests/functional/agent/common/test_ovs_lib.py @@ -465,3 +465,27 @@ class BaseOVSTestCase(base.BaseSudoTestCase): self.assertEqual(p_const.TYPE_GRE, ipv4_port_type) self.assertEqual(ovs_lib.TYPE_GRE_IP6, ipv6_port_type) self.assertEqual('legacy_l2', ipv6_port_options.get('packet_type')) + + def test_set_igmp_snooping_flood(self): + port_name = 'test_output_port_2' + self._create_bridge() + self._create_port(port_name) + self.ovs.set_igmp_snooping_flood(port_name, True) + ports_other_config = self.ovs.db_get_val('Port', port_name, + 'other_config') + self.assertEqual( + 'true', + ports_other_config.get('mcast-snooping-flood', '').lower()) + self.assertEqual( + 'true', + ports_other_config.get('mcast-snooping-flood-reports', '').lower()) + + self.ovs.set_igmp_snooping_flood(port_name, False) + ports_other_config = self.ovs.db_get_val('Port', port_name, + 'other_config') + self.assertEqual( + 'false', + ports_other_config.get('mcast-snooping-flood', '').lower()) + self.assertEqual( + 'false', + ports_other_config.get('mcast-snooping-flood-reports', '').lower()) diff --git a/neutron/tests/functional/agent/test_ovs_lib.py b/neutron/tests/functional/agent/test_ovs_lib.py index 17404a1c072..88799890b77 100644 --- a/neutron/tests/functional/agent/test_ovs_lib.py +++ b/neutron/tests/functional/agent/test_ovs_lib.py @@ -196,8 +196,9 @@ class OVSBridgeTestCase(OVSBridgeTestBase): 'Bridge', ('name', '=', self.br.br_name), columns=['other_config'] ).execute()[0]['other_config'] self.assertEqual( - str(state), - br_other_config['mcast-snooping-disable-flood-unregistered']) + 'false', + br_other_config.get( + 'mcast-snooping-disable-flood-unregistered', '').lower()) def test_set_igmp_snooping_enabled(self): self._test_set_igmp_snooping_state(True) diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py index 91a3dca2ab9..d9b9434c204 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py @@ -1383,7 +1383,8 @@ class TestOvsNeutronAgent(object): self.assertNotIn('activated_port_id', port_info['added']) def _test_setup_physical_bridges(self, port_exists=False, - dvr_enabled=False): + dvr_enabled=False, + igmp_snooping_enabled=False): self.agent.enable_distributed_routing = dvr_enabled with mock.patch.object(ip_lib.IPDevice, "exists") as devex_fn,\ mock.patch.object(sys, "exit"),\ @@ -1441,6 +1442,8 @@ class TestOvsNeutronAgent(object): 'phy-br-eth', constants.NONEXISTENT_PEER), ] expected_calls += [ + mock.call.int_br.set_igmp_snooping_flood( + 'int-br-eth', igmp_snooping_enabled), mock.call.int_br.drop_port(in_port='int_ofport') ] if not dvr_enabled: @@ -1510,6 +1513,10 @@ class TestOvsNeutronAgent(object): int_br.add_port.assert_called_with("int-br-eth") phys_br.add_port.assert_called_with("phy-br-eth") + def test_setup_physical_bridges_igmp_snooping_enabled(self): + cfg.CONF.set_override('igmp_snooping_enable', True, 'OVS') + self._test_setup_physical_bridges(igmp_snooping_enabled=True) + def _test_setup_physical_bridges_change_from_veth_to_patch_conf( self, port_exists=False): with mock.patch.object(sys, "exit"),\ @@ -1565,6 +1572,8 @@ class TestOvsNeutronAgent(object): 'phy-br-eth', constants.NONEXISTENT_PEER), ] expected_calls += [ + mock.call.int_br.set_igmp_snooping_flood( + 'int-br-eth', False), mock.call.int_br.drop_port(in_port='int_ofport'), mock.call.phys_br.drop_port(in_port='phy_ofport'), mock.call.int_br.set_db_attribute('Interface', 'int-br-eth', @@ -1605,6 +1614,8 @@ class TestOvsNeutronAgent(object): return_value=False),\ mock.patch.object(self.agent.int_br, 'port_exists', return_value=False),\ + mock.patch.object(self.agent.int_br, + 'set_igmp_snooping_flood'),\ mock.patch.object(sys, "exit"): self.agent.setup_tunnel_br(None) self.agent.setup_tunnel_br() @@ -1629,6 +1640,8 @@ class TestOvsNeutronAgent(object): "add_patch_port") as int_patch_port,\ mock.patch.object(self.agent.tun_br, "add_patch_port") as tun_patch_port,\ + mock.patch.object(self.agent.int_br, + 'set_igmp_snooping_flood'),\ mock.patch.object(sys, "exit"): self.agent.setup_tunnel_br(None) self.agent.setup_tunnel_br() diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py index 25aa76f5752..a61b0a27a42 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_tunnel.py @@ -234,6 +234,8 @@ class TunnelTest(object): mock.call.port_exists('int-%s' % self.MAP_TUN_BRIDGE), mock.call.add_patch_port('int-%s' % self.MAP_TUN_BRIDGE, constants.NONEXISTENT_PEER), + mock.call.set_igmp_snooping_flood('int-%s' % self.MAP_TUN_BRIDGE, + igmp_snooping), ] self.mock_int_bridge_expected += [ @@ -263,6 +265,7 @@ class TunnelTest(object): self.mock_int_bridge_expected += [ mock.call.port_exists('patch-tun'), mock.call.add_patch_port('patch-tun', 'patch-int'), + mock.call.set_igmp_snooping_flood('patch-tun', igmp_snooping), ] self.mock_int_bridge_expected += [ mock.call.get_vif_ports((ovs_lib.INVALID_OFPORT, @@ -719,7 +722,9 @@ class TunnelTestUseVethInterco(TunnelTest): self.mock_int_bridge_expected += [ mock.call.db_get_val('Interface', 'int-%s' % self.MAP_TUN_BRIDGE, 'type', log_errors=False), - mock.call.add_port('int-%s' % self.MAP_TUN_BRIDGE) + mock.call.add_port('int-%s' % self.MAP_TUN_BRIDGE), + mock.call.set_igmp_snooping_flood('int-%s' % self.MAP_TUN_BRIDGE, + igmp_snooping), ] self.mock_int_bridge_expected += [ @@ -742,7 +747,8 @@ class TunnelTestUseVethInterco(TunnelTest): ] self.mock_int_bridge_expected += [ mock.call.port_exists('patch-tun'), - mock.call.add_patch_port('patch-tun', 'patch-int') + mock.call.add_patch_port('patch-tun', 'patch-int'), + mock.call.set_igmp_snooping_flood('patch-tun', igmp_snooping), ] self.mock_int_bridge_expected += [ mock.call.get_vif_ports((ovs_lib.INVALID_OFPORT,