From 7fa6c1bf39b25da9eb345a40defe61c9a12134d5 Mon Sep 17 00:00:00 2001 From: Lucas Alvares Gomes Date: Fri, 31 Jan 2020 13:10:11 +0000 Subject: [PATCH] [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 Signed-off-by: Lucas Alvares Gomes --- neutron/common/ovn/constants.py | 4 ++ .../conf/plugins/ml2/drivers/ovn/ovn_conf.py | 7 ++++ .../ml2/drivers/ovn/mech_driver/ovsdb/api.py | 13 ------ .../drivers/ovn/mech_driver/ovsdb/commands.py | 25 ------------ .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 4 -- .../ovn/mech_driver/ovsdb/maintenance.py | 21 ++++++++++ .../ovn/mech_driver/ovsdb/ovn_client.py | 24 ++++++----- .../ovn/mech_driver/ovsdb/test_maintenance.py | 28 +++++++++++++ neutron/tests/unit/fake_resources.py | 1 - .../ovn/mech_driver/ovsdb/test_commands.py | 37 ----------------- .../ovn/mech_driver/ovsdb/test_maintenance.py | 40 ++++++++++++++++++- .../ovn/mech_driver/test_mech_driver.py | 18 +++++++++ ...gmp-snooping-support-1a6ec8e703311fce.yaml | 5 +++ 13 files changed, 137 insertions(+), 90 deletions(-) create mode 100644 releasenotes/notes/ovn-igmp-snooping-support-1a6ec8e703311fce.yaml diff --git a/neutron/common/ovn/constants.py b/neutron/common/ovn/constants.py index 4d8136ad609..8f61eb3f094 100644 --- a/neutron/common/ovn/constants.py +++ b/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' diff --git a/neutron/conf/plugins/ml2/drivers/ovn/ovn_conf.py b/neutron/conf/plugins/ml2/drivers/ovn/ovn_conf.py index fe3f7957157..ded41c5920c 100644 --- a/neutron/conf/plugins/ml2/drivers/ovn/ovn_conf.py +++ b/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 diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py index b33143e5521..7d2793cf6e6 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/api.py +++ b/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): diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py index 525f1737632..22c4d34993f 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/commands.py +++ b/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) 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 7fff3a2c355..cb79fd5ffee 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 @@ -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, 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 655344f6f78..276888c2dee 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py +++ b/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): 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 a3e4d2ffa89..18afcf36578 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 @@ -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 diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py index a15ea19c2c0..3bace092ca6 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py +++ b/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]) diff --git a/neutron/tests/unit/fake_resources.py b/neutron/tests/unit/fake_resources.py index 343a8dbb3f4..3181790bafa 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/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() diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py index 91c397727ba..76b612a73f8 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_commands.py +++ b/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): 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 a99b6b3e6fe..70d338647a8 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 @@ -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) 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 fc0da610ef3..97b9b3c8c41 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 @@ -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: diff --git a/releasenotes/notes/ovn-igmp-snooping-support-1a6ec8e703311fce.yaml b/releasenotes/notes/ovn-igmp-snooping-support-1a6ec8e703311fce.yaml new file mode 100644 index 00000000000..9c4b8809314 --- /dev/null +++ b/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.