[OVN] Add IGMP snooping support

This patch is ading IGMP snooping support in the OVN driver. Multicast
support has been introduced in core OVN upstream.

Also, the patch always sets the "mcast_flood_unregistered" config in
the OVN Logical_Switch to the same value as the "mcast_snoop" config.
This is so that OVN matches the OVS behavior which is to enable IGMP
flooding by default [0] (in OVN, by default it's false).

[0] http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt (grep
for "mcast-snooping-disable")

Change-Id: I32f61ba3dd06d7eacf76a74c5c44e1286f90e584
Co-Authored-By: Daniel Alvarez <dalvarez@redhat.com>
Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>

neutron/common/ovn/constants.py

@@ -203,3 +203,7 @@ METADATA_DEFAULT_IP = '169.254.169.254'
 METADATA_DEFAULT_CIDR = '%s/%d' % (METADATA_DEFAULT_IP,
                                    METADATA_DEFAULT_PREFIX)
 METADATA_PORT = 80
+
+# OVN igmp options
+MCAST_SNOOP = 'mcast_snoop'
+MCAST_FLOOD_UNREGISTERED = 'mcast_flood_unregistered'

neutron/conf/plugins/ml2/drivers/ovn/ovn_conf.py

@@ -16,6 +16,7 @@ from oslo_log import log as logging
 from ovsdbapp.backend.ovs_idl import vlog
 
 from neutron._i18n import _
+from neutron.conf.agent import ovs_conf
 
 LOG = logging.getLogger(__name__)
 
@@ -193,11 +194,13 @@ ovn_opts = [
 ]
 
 cfg.CONF.register_opts(ovn_opts, group='ovn')
+ovs_conf.register_ovs_agent_opts()
 
 
 def list_opts():
     return [
         ('ovn', ovn_opts),
+        ('ovs', ovs_conf.OPTS)
     ]
 
 
@@ -291,3 +294,7 @@ def get_global_dhcpv6_opts():
 
 def is_ovn_emit_need_to_frag_enabled():
     return cfg.CONF.ovn.ovn_emit_need_to_frag
+
+
+def is_igmp_snooping_enabled():
+    return cfg.CONF.OVS.igmp_snooping_enable

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py

@@ -23,19 +23,6 @@ from neutron.common.ovn import constants as ovn_const
 @six.add_metaclass(abc.ABCMeta)
 class API(api.API):
 
-    @abc.abstractmethod
-    def set_lswitch_ext_ids(self, name, ext_ids, if_exists=True):
-        """Create a command to set OVN lswitch external ids
-
-        :param name:      The name of the lswitch
-        :type name:       string
-        :param ext_ids    The external ids to set for the lswitch
-        :type ext_ids:    dictionary
-        :param if_exists: Do not fail if lswitch does not exist
-        :type if_exists:  bool
-        :returns:        :class:`Command` with no result
-        """
-
     @abc.abstractmethod
     def create_lswitch_port(self, lport_name, lswitch_name, may_exist=True,
                             **columns):

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py

@@ -101,31 +101,6 @@ class CheckLivenessCommand(command.BaseCommand):
         self.result = self.api.nb_global.nb_cfg
 
 
-class LSwitchSetExternalIdsCommand(command.BaseCommand):
-    def __init__(self, api, name, ext_ids, if_exists):
-        super(LSwitchSetExternalIdsCommand, self).__init__(api)
-        self.name = name
-        self.ext_ids = ext_ids
-        self.if_exists = if_exists
-
-    def run_idl(self, txn):
-        try:
-            lswitch = idlutils.row_by_value(self.api.idl, 'Logical_Switch',
-                                            'name', self.name)
-
-        except idlutils.RowNotFound:
-            if self.if_exists:
-                return
-            msg = _("Logical Switch %s does not exist") % self.name
-            raise RuntimeError(msg)
-
-        lswitch.verify('external_ids')
-        external_ids = getattr(lswitch, 'external_ids', {})
-        for key, value in self.ext_ids.items():
-            external_ids[key] = value
-        lswitch.external_ids = external_ids
-
-
 class AddLSwitchPortCommand(command.BaseCommand):
     def __init__(self, api, lport, lswitch, may_exist, **columns):
         super(AddLSwitchPortCommand, self).__init__(api)

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py

@@ -185,10 +185,6 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend):
         except ovn_exc.RevisionConflict as e:
             LOG.info('Transaction aborted. Reason: %s', e)
 
-    def set_lswitch_ext_ids(self, lswitch_id, ext_ids, if_exists=True):
-        return cmd.LSwitchSetExternalIdsCommand(self, lswitch_id, ext_ids,
-                                                if_exists)
-
     def create_lswitch_port(self, lport_name, lswitch_name, may_exist=True,
                             **columns):
         return cmd.AddLSwitchPortCommand(self, lport_name, lswitch_name,

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py

@@ -459,6 +459,27 @@ class DBInconsistenciesPeriodics(object):
 
         raise periodics.NeverAgain()
 
+    # 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_igmp_snoop_support(self):
+        if not self.has_lock:
+            return
+
+        with self._nb_idl.transaction(check_error=True) as txn:
+            value = ('true' if ovn_conf.is_igmp_snooping_enabled()
+                     else 'false')
+            for ls in self._nb_idl.ls_list().execute(check_error=True):
+                if ls.other_config.get(ovn_const.MCAST_SNOOP, None) == value:
+                    continue
+                txn.add(self._nb_idl.db_set(
+                    'Logical_Switch', ls.name,
+                    ('other_config', {
+                        ovn_const.MCAST_SNOOP: value,
+                        ovn_const.MCAST_FLOOD_UNREGISTERED: value})))
+
+        raise periodics.NeverAgain()
+
 
 class HashRingHealthCheckPeriodics(object):
 

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py

@@ -1719,31 +1719,36 @@ class OVNClient(object):
             tag=tag if tag else [],
             options={'network_name': physnet}))
 
-    def _gen_network_external_ids(self, network):
-        ext_ids = {
+    def _gen_network_parameters(self, network):
+        params = {'external_ids': {
             ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: network['name'],
             ovn_const.OVN_NETWORK_MTU_EXT_ID_KEY: str(network['mtu']),
             ovn_const.OVN_REV_NUM_EXT_ID_KEY: str(
-                utils.get_revision_number(network, ovn_const.TYPE_NETWORKS))}
+                utils.get_revision_number(network, ovn_const.TYPE_NETWORKS))}}
 
         # NOTE(lucasagomes): There's a difference between the
         # "qos_policy_id" key existing and it being None, the latter is a
         # valid value. Since we can't save None in OVSDB, we are converting
         # it to "null" as a placeholder.
         if 'qos_policy_id' in network:
-            ext_ids[ovn_const.OVN_QOS_POLICY_EXT_ID_KEY] = (
+            params['external_ids'][ovn_const.OVN_QOS_POLICY_EXT_ID_KEY] = (
                 network['qos_policy_id'] or 'null')
-        return ext_ids
+
+        # Enable IGMP snooping if igmp_snooping_enable is enabled in Neutron
+        value = 'true' if ovn_conf.is_igmp_snooping_enabled() else 'false'
+        params['other_config'] = {ovn_const.MCAST_SNOOP: value,
+                                  ovn_const.MCAST_FLOOD_UNREGISTERED: value}
+        return params
 
     def create_network(self, network):
         # Create a logical switch with a name equal to the Neutron network
         # UUID.  This provides an easy way to refer to the logical switch
         # without having to track what UUID OVN assigned to it.
         admin_context = n_context.get_admin_context()
-        ext_ids = self._gen_network_external_ids(network)
+        lswitch_params = self._gen_network_parameters(network)
         lswitch_name = utils.ovn_name(network['id'])
         with self._nb_idl.transaction(check_error=True) as txn:
-            txn.add(self._nb_idl.ls_add(lswitch_name, external_ids=ext_ids,
+            txn.add(self._nb_idl.ls_add(lswitch_name, **lswitch_params,
                                         may_exist=True))
             physnet = network.get(pnet.PHYSICAL_NETWORK)
             if physnet:
@@ -1832,9 +1837,10 @@ class OVNClient(object):
         admin_context = n_context.get_admin_context()
         with self._nb_idl.transaction(check_error=True) as txn:
             txn.add(check_rev_cmd)
-            ext_ids = self._gen_network_external_ids(network)
+            lswitch_params = self._gen_network_parameters(network)
             lswitch = self._nb_idl.get_lswitch(lswitch_name)
-            txn.add(self._nb_idl.set_lswitch_ext_ids(lswitch_name, ext_ids))
+            txn.add(self._nb_idl.db_set(
+                'Logical_Switch', lswitch_name, *lswitch_params.items()))
             # Check if previous mtu is different than current one,
             # checking will help reduce number of operations
             if (not lswitch or

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py

@@ -14,6 +14,7 @@
 #    under the License.
 
 import mock
+from oslo_config import cfg
 
 from futurist import periodics
 from neutron_lib.api.definitions import external_net as extnet_apidef
@@ -798,3 +799,30 @@ class TestMaintenance(_TestMaintenanceHelper):
         # for the port
         self.assertTrue(ovn_port['port_security'])
         self.assertNotIn('unknown', ovn_port['addresses'])
+
+    def test_check_for_igmp_snooping_enabled(self):
+        cfg.CONF.set_override('igmp_snooping_enable', False, group='OVS')
+        net = self._create_network('net')
+        ls = self.nb_api.db_find('Logical_Switch',
+            ('name', '=', utils.ovn_name(net['id']))).execute(
+            check_error=True)[0]
+
+        self.assertEqual('false', ls['other_config'][ovn_const.MCAST_SNOOP])
+        self.assertEqual(
+            'false', ls['other_config'][ovn_const.MCAST_FLOOD_UNREGISTERED])
+
+        # Change the value of the configuration
+        cfg.CONF.set_override('igmp_snooping_enable', True, group='OVS')
+
+        # Call the maintenance task and check that the value has been
+        # updated in the Logical Switch
+        self.assertRaises(periodics.NeverAgain,
+                          self.maint.check_for_igmp_snoop_support)
+
+        ls = self.nb_api.db_find('Logical_Switch',
+            ('name', '=', utils.ovn_name(net['id']))).execute(
+            check_error=True)[0]
+
+        self.assertEqual('true', ls['other_config'][ovn_const.MCAST_SNOOP])
+        self.assertEqual(
+            'true', ls['other_config'][ovn_const.MCAST_FLOOD_UNREGISTERED])

neutron/tests/unit/fake_resources.py

@@ -52,7 +52,6 @@ class FakeOvsdbNbOvnIdl(object):
         self.transaction = mock.MagicMock()
         self.create_transaction = mock.MagicMock()
         self.ls_add = mock.Mock()
-        self.set_lswitch_ext_ids = mock.Mock()
         self.ls_del = mock.Mock()
         self.create_lswitch_port = mock.Mock()
         self.set_lswitch_port = mock.Mock()

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py

@@ -94,43 +94,6 @@ class TestCheckLivenessCommand(TestBaseCommand):
         self.assertNotEqual(cmd.result, old_ng_cfg)
 
 
-class TestLSwitchSetExternalIdsCommand(TestBaseCommand):
-
-    def _test_lswitch_extid_update_no_exist(self, if_exists=True):
-        with mock.patch.object(idlutils, 'row_by_value',
-                               side_effect=idlutils.RowNotFound):
-            cmd = commands.LSwitchSetExternalIdsCommand(
-                self.ovn_api, 'fake-lswitch',
-                {ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: 'neutron-network'},
-                if_exists=if_exists)
-            if if_exists:
-                cmd.run_idl(self.transaction)
-            else:
-                self.assertRaises(RuntimeError, cmd.run_idl, self.transaction)
-
-    def test_lswitch_no_exist_ignore(self):
-        self._test_lswitch_extid_update_no_exist(if_exists=True)
-
-    def test_lswitch_no_exist_fail(self):
-        self._test_lswitch_extid_update_no_exist(if_exists=False)
-
-    def test_lswitch_extid_update(self):
-        network_name = 'private'
-        new_network_name = 'private-new'
-        ext_ids = {ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: network_name}
-        new_ext_ids = {ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: new_network_name}
-        fake_lswitch = fakes.FakeOvsdbRow.create_one_ovsdb_row(
-            attrs={'external_ids': ext_ids})
-        with mock.patch.object(idlutils, 'row_by_value',
-                               return_value=fake_lswitch):
-            cmd = commands.LSwitchSetExternalIdsCommand(
-                self.ovn_api, fake_lswitch.name,
-                {ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: new_network_name},
-                if_exists=True)
-            cmd.run_idl(self.transaction)
-            self.assertEqual(new_ext_ids, fake_lswitch.external_ids)
-
-
 class TestAddLSwitchPortCommand(TestBaseCommand):
 
     def test_lswitch_not_found(self):

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py

@@ -17,6 +17,7 @@ import mock
 
 from futurist import periodics
 from neutron_lib import context
+from oslo_config import cfg
 
 from neutron.common.ovn import constants
 from neutron.common.ovn import utils
@@ -24,6 +25,7 @@ from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
 from neutron.db import ovn_revision_numbers_db
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import maintenance
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_db_sync
+from neutron.tests.unit import fake_resources as fakes
 from neutron.tests.unit.plugins.ml2 import test_security_group as test_sg
 from neutron.tests.unit import testlib_api
 
@@ -39,7 +41,7 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight,
             self.fmt, name='net1', admin_state_up=True)['network']
         self.port = self._make_port(
             self.fmt, self.net['id'], name='port1')['port']
-        self.fake_ovn_client = mock.Mock()
+        self.fake_ovn_client = mock.MagicMock()
         self.periodic = maintenance.DBInconsistenciesPeriodics(
             self.fake_ovn_client)
         self.ctx = context.get_admin_context()
@@ -269,3 +271,39 @@ class TestDBInconsistenciesPeriodics(testlib_api.SqlTestCaseLight,
         incst = [mock.Mock(resource_type=constants.TYPE_NETWORKS)] * 2
         self.periodic._log_maintenance_inconsistencies(incst, [])
         self.assertFalse(mock_log.called)
+
+    def test_check_for_igmp_snoop_support(self):
+        cfg.CONF.set_override('igmp_snooping_enable', True, group='OVS')
+        nb_idl = self.fake_ovn_client._nb_idl
+        ls0 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'ls0',
+                   'other_config': {
+                       constants.MCAST_SNOOP: 'false',
+                       constants.MCAST_FLOOD_UNREGISTERED: 'false'}})
+        ls1 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'ls1',
+                   'other_config': {}})
+        ls2 = fakes.FakeOvsdbRow.create_one_ovsdb_row(
+            attrs={'name': 'ls2',
+                   'other_config': {
+                        constants.MCAST_SNOOP: 'true',
+                        constants.MCAST_FLOOD_UNREGISTERED: 'true'}})
+
+        nb_idl.ls_list.return_value.execute.return_value = [ls0, ls1, ls2]
+
+        self.assertRaises(periodics.NeverAgain,
+                          self.periodic.check_for_igmp_snoop_support)
+
+        # "ls2" is not part of the transaction because it already
+        # have the right value set
+        expected_calls = [
+            mock.call('Logical_Switch', 'ls0',
+                      ('other_config', {
+                           constants.MCAST_SNOOP: 'true',
+                           constants.MCAST_FLOOD_UNREGISTERED: 'true'})),
+            mock.call('Logical_Switch', 'ls1',
+                      ('other_config', {
+                           constants.MCAST_SNOOP: 'true',
+                           constants.MCAST_FLOOD_UNREGISTERED: 'true'})),
+        ]
+        nb_idl.db_set.assert_has_calls(expected_calls)

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py

@@ -576,6 +576,24 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
                           self.mech_driver.update_network_precommit,
                           fake_network_context)
 
+    def _create_network_igmp_snoop(self, enabled):
+        cfg.CONF.set_override('igmp_snooping_enable', enabled, group='OVS')
+        nb_idl = self.mech_driver._ovn_client._nb_idl
+        net = self._make_network(self.fmt, name='net1',
+                                 admin_state_up=True)['network']
+        value = 'true' if enabled else 'false'
+        nb_idl.ls_add.assert_called_once_with(
+            ovn_utils.ovn_name(net['id']), external_ids=mock.ANY,
+            may_exist=True,
+            other_config={ovn_const.MCAST_SNOOP: value,
+                          ovn_const.MCAST_FLOOD_UNREGISTERED: value})
+
+    def test_create_network_igmp_snoop_enabled(self):
+        self._create_network_igmp_snoop(enabled=True)
+
+    def test_create_network_igmp_snoop_disabled(self):
+        self._create_network_igmp_snoop(enabled=False)
+
     def test_create_port_without_security_groups(self):
         kwargs = {'security_groups': []}
         with self.network(set_context=True, tenant_id='test') as net1:

releasenotes/notes/ovn-igmp-snooping-support-1a6ec8e703311fce.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - |
+    Adds support for IGMP snooping (Multicast) in the OVN driver. Defaults
+    to False. IGMP snooping requires OVN version 2.12 or above.