From 43c1f98f074549611db90b6c333d2c54d7e4d4b3 Mon Sep 17 00:00:00 2001 From: Assaf Muller Date: Thu, 22 May 2014 14:38:30 +0300 Subject: [PATCH] Move ARP responder test to sanity command Additionally, the patch improves the check itself: To check if the currently installed OVS supports the ARP responder feature, we try to add a flow that references an OpenFlow ARP extension via ofctl. The test may fail due to an (expected) Runtime error, or due to some other unexpected error. In such a case the error was previously masked and tossed away. * Clean up ARP responder unit test * Extract ARP responder flow actions to be used by the unit tests, functional test as well as the ARP responder code itself After this patch, if the sanity check returned False but the user never ran it or ignored its results, the OVS agent will output errors to the log every time an ARP entry is (attempted) to be added or removed from the flow table. Closes-Bug: #1323718 Change-Id: I428c954d6561cd398a1e580804a9482969a154af --- neutron/agent/linux/ovs_lib.py | 24 ---------- neutron/cmd/sanity/checks.py | 47 +++++++++++++++++++ neutron/cmd/sanity_check.py | 14 ++++++ .../openvswitch/agent/ovs_neutron_agent.py | 29 ++---------- .../plugins/openvswitch/common/constants.py | 9 ++++ .../tests/functional/sanity/test_sanity.py | 3 ++ .../tests/unit/agent/linux/test_ovs_lib.py | 32 ------------- .../openvswitch/test_ovs_neutron_agent.py | 28 ++--------- .../tests/unit/openvswitch/test_ovs_tunnel.py | 6 --- 9 files changed, 80 insertions(+), 112 deletions(-) diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py index 78967d1f3a3..c8421a2124a 100644 --- a/neutron/agent/linux/ovs_lib.py +++ b/neutron/agent/linux/ovs_lib.py @@ -18,7 +18,6 @@ from oslo.config import cfg from neutron.agent.linux import ip_lib from neutron.agent.linux import utils from neutron.common import exceptions -from neutron.common import utils as common_utils from neutron.openstack.common import excutils from neutron.openstack.common import jsonutils from neutron.openstack.common import log as logging @@ -551,26 +550,3 @@ def _build_flow_expr_str(flow_dict, cmd): flow_expr_arr.append(actions) return ','.join(flow_expr_arr) - - -def ofctl_arg_supported(root_helper, cmd, args): - '''Verify if ovs-ofctl binary supports command with specific args. - - :param root_helper: utility to use when running shell cmds. - :param cmd: ovs-vsctl command to use for test. - :param args: arguments to test with command. - :returns: a boolean if the args supported. - ''' - supported = True - br_name = 'br-test-%s' % common_utils.get_random_string(6) - test_br = OVSBridge(br_name, root_helper) - test_br.reset_bridge() - - full_args = ["ovs-ofctl", cmd, test_br.br_name] + args - try: - utils.execute(full_args, root_helper=root_helper) - except Exception: - supported = False - - test_br.destroy() - return supported diff --git a/neutron/cmd/sanity/checks.py b/neutron/cmd/sanity/checks.py index 60081573e0d..68cf32f226e 100644 --- a/neutron/cmd/sanity/checks.py +++ b/neutron/cmd/sanity/checks.py @@ -13,11 +13,17 @@ # License for the specific language governing permissions and limitations # under the License. +import netaddr + from neutron.agent.linux import ovs_lib +from neutron.agent.linux import utils as agent_utils from neutron.common import utils +from neutron.openstack.common import log as logging from neutron.plugins.common import constants as const from neutron.plugins.openvswitch.common import constants as ovs_const +LOG = logging.getLogger(__name__) + def vxlan_supported(root_helper, from_ip='192.0.2.1', to_ip='192.0.2.2'): name = "vxlantest-" + utils.get_random_string(6) @@ -42,3 +48,44 @@ def nova_notify_supported(): return True except ImportError: return False + + +def ofctl_arg_supported(root_helper, cmd, **kwargs): + """Verify if ovs-ofctl binary supports cmd with **kwargs. + + :param root_helper: utility to use when running shell commands. + :param cmd: ovs-ofctl command to use for test. + :param **kwargs: arguments to test with the command. + :returns: a boolean if the supplied arguments are supported. + """ + br_name = 'br-test-%s' % utils.get_random_string(6) + with ovs_lib.OVSBridge(br_name, root_helper) as test_br: + full_args = ["ovs-ofctl", cmd, test_br.br_name, + ovs_lib._build_flow_expr_str(kwargs, cmd.split('-')[0])] + try: + agent_utils.execute(full_args, root_helper=root_helper) + except RuntimeError as e: + LOG.debug("Exception while checking supported feature via " + "command %s. Exception: %s" % (full_args, e)) + return False + except Exception: + LOG.exception(_("Unexpected exception while checking supported" + " feature via command: %s") % full_args) + return False + else: + return True + + +def arp_responder_supported(root_helper): + mac = netaddr.EUI('dead:1234:beef', dialect=netaddr.mac_unix) + ip = netaddr.IPAddress('240.0.0.1') + actions = ovs_const.ARP_RESPONDER_ACTIONS % {'mac': mac, 'ip': ip} + + return ofctl_arg_supported(root_helper, + cmd='add-flow', + table=21, + priority=1, + proto='arp', + dl_vlan=42, + nw_dst='%s' % ip, + actions=actions) diff --git a/neutron/cmd/sanity_check.py b/neutron/cmd/sanity_check.py index 16e65f80be1..b6522d6d79e 100644 --- a/neutron/cmd/sanity_check.py +++ b/neutron/cmd/sanity_check.py @@ -61,6 +61,16 @@ def check_nova_notify(): return result +def check_arp_responder(): + result = checks.arp_responder_supported( + root_helper=cfg.CONF.AGENT.root_helper) + if not result: + LOG.error(_('Check for Open vSwitch ARP responder support failed. ' + 'Please ensure that the version of openvswitch ' + 'being used has ARP flows support.')) + return result + + # Define CLI opts to test specific features, with a calback for the test OPTS = [ BoolOptCallback('ovs_vxlan', check_ovs_vxlan, default=False, @@ -69,6 +79,8 @@ OPTS = [ help=_('Check for patch port support')), BoolOptCallback('nova_notify', check_nova_notify, default=False, help=_('Check for nova notification support')), + BoolOptCallback('arp_responder', check_arp_responder, default=False, + help=_('Check for ARP responder support')), ] @@ -87,6 +99,8 @@ def enable_tests_from_config(): if (cfg.CONF.notify_nova_on_port_status_changes or cfg.CONF.notify_nova_on_port_data_changes): cfg.CONF.set_override('nova_notify', True) + if cfg.CONF.AGENT.arp_responder: + cfg.CONF.set_override('arp_responder', True) def all_tests_passed(): diff --git a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py index 956ef708238..d2eec998a62 100644 --- a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py @@ -172,12 +172,10 @@ class OVSNeutronAgent(n_rpc.RpcCallback, q_const.MAX_VLAN_TAG)) self.tunnel_types = tunnel_types or [] self.l2_pop = l2_population - # TODO(ethuleau): Initially, local ARP responder is be dependent to the + # TODO(ethuleau): Change ARP responder so it's not dependent on the # ML2 l2 population mechanism driver. - self.arp_responder_enabled = (arp_responder and - self._check_arp_responder_support() and - self.l2_pop) self.enable_distributed_routing = enable_distributed_routing + self.arp_responder_enabled = arp_responder and self.l2_pop self.agent_state = { 'binary': 'neutron-openvswitch-agent', 'host': cfg.CONF.host, @@ -251,20 +249,6 @@ class OVSNeutronAgent(n_rpc.RpcCallback, self.iter_num = 0 self.run_daemon_loop = True - def _check_arp_responder_support(self): - '''Check if OVS supports to modify ARP headers. - - This functionality is only available since the development branch 2.1. - ''' - args = ['arp,action=load:0x2->NXM_OF_ARP_OP[],' - 'move:NXM_NX_ARP_SHA[]->NXM_NX_ARP_THA[],' - 'move:NXM_OF_ARP_SPA[]->NXM_OF_ARP_TPA[]'] - supported = ovs_lib.ofctl_arg_supported(self.root_helper, 'add-flow', - args) - if not supported: - LOG.warning(_('OVS version can not support ARP responder.')) - return supported - def _report_state(self): # How many devices are likely used by a VM self.agent_state.get('configurations')['devices'] = ( @@ -477,14 +461,7 @@ class OVSNeutronAgent(n_rpc.RpcCallback, ip = netaddr.IPAddress(ip_str) if action == 'add': - actions = ('move:NXM_OF_ETH_SRC[]->NXM_OF_ETH_DST[],' - 'mod_dl_src:%(mac)s,' - 'load:0x2->NXM_OF_ARP_OP[],' - 'move:NXM_NX_ARP_SHA[]->NXM_NX_ARP_THA[],' - 'move:NXM_OF_ARP_SPA[]->NXM_OF_ARP_TPA[],' - 'load:%(mac)#x->NXM_NX_ARP_SHA[],' - 'load:%(ip)#x->NXM_OF_ARP_SPA[],' - 'in_port' % {'mac': mac, 'ip': ip}) + actions = constants.ARP_RESPONDER_ACTIONS % {'mac': mac, 'ip': ip} self.tun_br.add_flow(table=constants.ARP_RESPONDER, priority=1, proto='arp', diff --git a/neutron/plugins/openvswitch/common/constants.py b/neutron/plugins/openvswitch/common/constants.py index b74242b0b06..5fe702484dd 100644 --- a/neutron/plugins/openvswitch/common/constants.py +++ b/neutron/plugins/openvswitch/common/constants.py @@ -68,3 +68,12 @@ INVALID_OFPORT = '-1' # Represent invalid OF Port OFPORT_INVALID = -1 + +ARP_RESPONDER_ACTIONS = ('move:NXM_OF_ETH_SRC[]->NXM_OF_ETH_DST[],' + 'mod_dl_src:%(mac)s,' + 'load:0x2->NXM_OF_ARP_OP[],' + 'move:NXM_NX_ARP_SHA[]->NXM_NX_ARP_THA[],' + 'move:NXM_OF_ARP_SPA[]->NXM_OF_ARP_TPA[],' + 'load:%(mac)#x->NXM_NX_ARP_SHA[],' + 'load:%(ip)#x->NXM_OF_ARP_SPA[],' + 'in_port') diff --git a/neutron/tests/functional/sanity/test_sanity.py b/neutron/tests/functional/sanity/test_sanity.py index 2873dc7a586..bbe8911e534 100644 --- a/neutron/tests/functional/sanity/test_sanity.py +++ b/neutron/tests/functional/sanity/test_sanity.py @@ -49,3 +49,6 @@ class SanityTestCaseRoot(functional_base.BaseSudoTestCase): def test_ovs_patch_support_runs(self): checks.patch_supported(self.root_helper) + + def test_arp_responder_runs(self): + checks.arp_responder_supported(self.root_helper) diff --git a/neutron/tests/unit/agent/linux/test_ovs_lib.py b/neutron/tests/unit/agent/linux/test_ovs_lib.py index e60e62cbdbc..24ae10c3f0f 100644 --- a/neutron/tests/unit/agent/linux/test_ovs_lib.py +++ b/neutron/tests/unit/agent/linux/test_ovs_lib.py @@ -932,35 +932,3 @@ class OVS_Lib_Test(base.BaseTestCase): data = [[["map", external_ids], "tap99", 1]] self.assertIsNone(self._test_get_vif_port_by_id('tap99id', data, "br-ext")) - - def test_ofctl_arg_supported(self): - with mock.patch('neutron.common.utils.get_random_string') as utils: - utils.return_value = 'test' - supported = ovs_lib.ofctl_arg_supported(self.root_helper, 'cmd', - ['args']) - self.execute.assert_has_calls([ - mock.call(['ovs-vsctl', self.TO, '--', '--if-exists', 'del-br', - 'br-test-test'], root_helper=self.root_helper), - mock.call(['ovs-vsctl', self.TO, '--', '--may-exist', 'add-br', - 'br-test-test'], root_helper=self.root_helper), - mock.call(['ovs-ofctl', 'cmd', 'br-test-test', 'args'], - root_helper=self.root_helper), - mock.call(['ovs-vsctl', self.TO, '--', '--if-exists', 'del-br', - 'br-test-test'], root_helper=self.root_helper) - ]) - self.assertTrue(supported) - - self.execute.side_effect = Exception - supported = ovs_lib.ofctl_arg_supported(self.root_helper, 'cmd', - ['args']) - self.execute.assert_has_calls([ - mock.call(['ovs-vsctl', self.TO, '--', '--if-exists', 'del-br', - 'br-test-test'], root_helper=self.root_helper), - mock.call(['ovs-vsctl', self.TO, '--', '--may-exist', 'add-br', - 'br-test-test'], root_helper=self.root_helper), - mock.call(['ovs-ofctl', 'cmd', 'br-test-test', 'args'], - root_helper=self.root_helper), - mock.call(['ovs-vsctl', self.TO, '--', '--if-exists', 'del-br', - 'br-test-test'], root_helper=self.root_helper) - ]) - self.assertFalse(supported) diff --git a/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py b/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py index 5aa43f0e256..084bd020e99 100644 --- a/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py @@ -137,10 +137,7 @@ class TestOvsNeutronAgent(base.BaseTestCase): 'get_bridges'), mock.patch('neutron.openstack.common.loopingcall.' 'FixedIntervalLoopingCall', - new=MockFixedIntervalLoopingCall), - mock.patch('neutron.plugins.openvswitch.agent.ovs_neutron_agent.' - 'OVSNeutronAgent._check_arp_responder_support', - return_value=True)): + new=MockFixedIntervalLoopingCall)): self.agent = ovs_neutron_agent.OVSNeutronAgent(**kwargs) self.agent.tun_br = mock.Mock() self.agent.sg_agent = mock.Mock() @@ -1022,14 +1019,7 @@ class TestOvsNeutronAgent(base.BaseTestCase): ) as (add_flow_fn, mod_flow_fn, add_tun_fn): self.agent.fdb_add(None, fdb_entry) self.assertFalse(add_tun_fn.called) - actions = ('move:NXM_OF_ETH_SRC[]->NXM_OF_ETH_DST[],' - 'mod_dl_src:%(mac)s,' - 'load:0x2->NXM_OF_ARP_OP[],' - 'move:NXM_NX_ARP_SHA[]->NXM_NX_ARP_THA[],' - 'move:NXM_OF_ARP_SPA[]->NXM_OF_ARP_TPA[],' - 'load:%(mac)#x->NXM_NX_ARP_SHA[],' - 'load:%(ip)#x->NXM_OF_ARP_SPA[],' - 'in_port' % + actions = (constants.ARP_RESPONDER_ACTIONS % {'mac': netaddr.EUI(FAKE_MAC, dialect=netaddr.mac_unix), 'ip': netaddr.IPAddress(FAKE_IP1)}) add_flow_fn.assert_has_calls([ @@ -1121,14 +1111,7 @@ class TestOvsNeutronAgent(base.BaseTestCase): mock.patch.object(self.agent.tun_br, 'delete_flows') ) as (add_flow_fn, del_flow_fn): self.agent.fdb_update(None, fdb_entries) - actions = ('move:NXM_OF_ETH_SRC[]->NXM_OF_ETH_DST[],' - 'mod_dl_src:%(mac)s,' - 'load:0x2->NXM_OF_ARP_OP[],' - 'move:NXM_NX_ARP_SHA[]->NXM_NX_ARP_THA[],' - 'move:NXM_OF_ARP_SPA[]->NXM_OF_ARP_TPA[],' - 'load:%(mac)#x->NXM_NX_ARP_SHA[],' - 'load:%(ip)#x->NXM_OF_ARP_SPA[],' - 'in_port' % + actions = (constants.ARP_RESPONDER_ACTIONS % {'mac': netaddr.EUI(FAKE_MAC, dialect=netaddr.mac_unix), 'ip': netaddr.IPAddress(FAKE_IP2)}) add_flow_fn.assert_called_once_with(table=constants.ARP_RESPONDER, @@ -1400,10 +1383,7 @@ class AncillaryBridgesTest(base.BaseTestCase): return_value=bridges), mock.patch( 'neutron.agent.linux.ovs_lib.get_bridge_external_bridge_id', - side_effect=pullup_side_effect), - mock.patch('neutron.plugins.openvswitch.agent.ovs_neutron_agent.' - 'OVSNeutronAgent._check_arp_responder_support', - return_value=True)): + side_effect=pullup_side_effect)): self.agent = ovs_neutron_agent.OVSNeutronAgent(**self.kwargs) self.assertEqual(len(ancillary), len(self.agent.ancillary_brs)) if ancillary: diff --git a/neutron/tests/unit/openvswitch/test_ovs_tunnel.py b/neutron/tests/unit/openvswitch/test_ovs_tunnel.py index 095f761c426..8ddfa64be77 100644 --- a/neutron/tests/unit/openvswitch/test_ovs_tunnel.py +++ b/neutron/tests/unit/openvswitch/test_ovs_tunnel.py @@ -78,12 +78,6 @@ class TunnelTest(base.BaseTestCase): 'neutron.openstack.common.rpc.impl_fake') cfg.CONF.set_override('report_interval', 0, 'AGENT') - check_arp_responder_str = ('neutron.plugins.openvswitch.agent.' - 'ovs_neutron_agent.OVSNeutronAgent.' - '_check_arp_responder_support') - self.mock_check_arp_resp = mock.patch(check_arp_responder_str).start() - self.mock_check_arp_resp.return_value = True - self.INT_BRIDGE = 'integration_bridge' self.TUN_BRIDGE = 'tunnel_bridge' self.MAP_TUN_BRIDGE = 'tun_br_map'