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

@@ -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

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)

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():

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[],'
+            actions = constants.ARP_RESPONDER_ACTIONS % {'mac': mac, 'ip': ip}
-                       '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})
             self.tun_br.add_flow(table=constants.ARP_RESPONDER,
                                  priority=1,
                                  proto='arp',

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')

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)

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)

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),
+                       new=MockFixedIntervalLoopingCall)):
-            mock.patch('neutron.plugins.openvswitch.agent.ovs_neutron_agent.'
-                       'OVSNeutronAgent._check_arp_responder_support',
-                       return_value=True)):
             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[],'
+            actions = (constants.ARP_RESPONDER_ACTIONS %
-                       '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': 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[],'
+            actions = (constants.ARP_RESPONDER_ACTIONS %
-                       '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': 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),
+                side_effect=pullup_side_effect)):
-            mock.patch('neutron.plugins.openvswitch.agent.ovs_neutron_agent.'
-                       'OVSNeutronAgent._check_arp_responder_support',
-                       return_value=True)):
             self.agent = ovs_neutron_agent.OVSNeutronAgent(**self.kwargs)
             self.assertEqual(len(ancillary), len(self.agent.ancillary_brs))
             if ancillary:

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'