Force arp_responder to True when DVR and tunneling enabled

After [1] and [2], the ARP responder needs to be enabled
if DVR and tunneling are enabled or ARP will not work. If
it is False we will log a message and force it to True.

[1] https://review.opendev.org/#/c/651905/
[2] https://review.opendev.org/#/c/653883/

Change-Id: I934062c970effe5194056b0786f84f3246850701
Related-bug: #1774459

neutron/conf/plugins/ml2/drivers/ovs_conf.py

@@ -143,7 +143,11 @@ agent_opts = [
                        "Requires OVS 2.1 and ML2 l2population driver. "
                        "Allows the switch (when supporting an overlay) "
                        "to respond to an ARP request locally without "
-                       "performing a costly ARP broadcast into the overlay.")),
+                       "performing a costly ARP broadcast into the overlay. "
+                       "NOTE: If enable_distributed_routing is set to True "
+                       "then arp_responder will automatically be set to True "
+                       "in the agent, regardless of the setting in the config "
+                       "file.")),
     cfg.BoolOpt('dont_fragment', default=True,
                 help=_("Set or un-set the don't fragment (DF) bit on "
                        "outgoing IP packet carrying GRE/VXLAN tunnel.")),

neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py

@@ -119,8 +119,7 @@ class OVSDVRNeutronAgent(object):
                  patch_int_ofport=constants.OFPORT_INVALID,
                  patch_tun_ofport=constants.OFPORT_INVALID,
                  host=None, enable_tunneling=False,
-                 enable_distributed_routing=False,
-                 arp_responder_enabled=False):
+                 enable_distributed_routing=False):
         self.context = context
         self.plugin_rpc = plugin_rpc
         self.host = host
@@ -134,7 +133,6 @@ class OVSDVRNeutronAgent(object):
                                   patch_int_ofport, patch_tun_ofport)
         self.reset_dvr_parameters()
         self.dvr_mac_address = None
-        self.arp_responder_enabled = arp_responder_enabled
         if self.enable_distributed_routing:
             self.get_dvr_mac_address()
 
@@ -425,15 +423,12 @@ class OVSDVRNeutronAgent(object):
                 gateway_mac=subnet_info['gateway_mac'],
                 dst_mac=comp_ovsport.get_mac(),
                 dst_port=comp_ovsport.get_ofport())
-        # Add the following flow rule only when ARP RESPONDER is
-        # enabled
-        if self.arp_responder_enabled:
-            self.int_br.install_dvr_dst_mac_for_arp(
-                lvm.network_type,
-                vlan_tag=lvm.vlan,
-                gateway_mac=port.vif_mac,
-                dvr_mac=self.dvr_mac_address,
-                rtr_port=port.ofport)
+        self.int_br.install_dvr_dst_mac_for_arp(
+            lvm.network_type,
+            vlan_tag=lvm.vlan,
+            gateway_mac=port.vif_mac,
+            dvr_mac=self.dvr_mac_address,
+            rtr_port=port.ofport)
 
         if lvm.network_type == n_const.TYPE_VLAN:
             # TODO(vivek) remove the IPv6 related flows once SNAT is not
@@ -628,16 +623,12 @@ class OVSDVRNeutronAgent(object):
                     network_type=network_type,
                     vlan_tag=vlan_to_use, dst_mac=comp_port.get_mac())
             ldm.remove_all_compute_ofports()
-            # If ARP Responder enabled, remove the rule that redirects
-            # the dvr_mac_address destination to the router port, since
-            # the router port is removed or unbound.
-            if self.arp_responder_enabled:
-                self.int_br.delete_dvr_dst_mac_for_arp(
-                    network_type=network_type,
-                    vlan_tag=vlan_to_use,
-                    gateway_mac=port.vif_mac,
-                    dvr_mac=self.dvr_mac_address,
-                    rtr_port=port.ofport)
+            self.int_br.delete_dvr_dst_mac_for_arp(
+                network_type=network_type,
+                vlan_tag=vlan_to_use,
+                gateway_mac=port.vif_mac,
+                dvr_mac=self.dvr_mac_address,
+                rtr_port=port.ofport)
             if ldm.get_csnat_ofport() == constants.OFPORT_INVALID:
                 # if there is no csnat port for this subnet, remove
                 # this subnet from local_dvr_map, as no dvr (or) csnat

neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py

@@ -162,17 +162,21 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
         self.available_local_vlans = set(six.moves.range(
             n_const.MIN_VLAN_TAG, n_const.MAX_VLAN_TAG + 1))
         self.tunnel_types = agent_conf.tunnel_types or []
+        self.enable_tunneling = bool(self.tunnel_types)
         self.l2_pop = agent_conf.l2_population
         # TODO(ethuleau): Change ARP responder so it's not dependent on the
         #                 ML2 l2 population mechanism driver.
         self.enable_distributed_routing = agent_conf.enable_distributed_routing
         self.arp_responder_enabled = agent_conf.arp_responder and self.l2_pop
+        if (self.enable_distributed_routing and self.enable_tunneling and
+                not self.arp_responder_enabled):
+            LOG.warning("ARP responder was not enabled but is required since "
+                        "DVR and tunneling are enabled, setting to True.")
+            self.arp_responder_enabled = True
 
         host = self.conf.host
         self.agent_id = 'ovs-agent-%s' % host
 
-        self.enable_tunneling = bool(self.tunnel_types)
-
         # Validate agent configurations
         self._check_agent_configurations()
 
@@ -252,8 +256,7 @@ class OVSNeutronAgent(l2population_rpc.L2populationRpcCallBackTunnelMixin,
             self.patch_tun_ofport,
             host,
             self.enable_tunneling,
-            self.enable_distributed_routing,
-            self.arp_responder_enabled)
+            self.enable_distributed_routing)
 
         if self.enable_distributed_routing:
             self.dvr_agent.setup_dvr_flows()

neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py

@@ -2922,14 +2922,38 @@ class TestOvsDvrNeutronAgent(object):
                                     'ip_address': '1.1.1.3'}]
 
     @staticmethod
-    def _expected_port_bound(port, lvid, is_dvr=True):
+    def _expected_port_bound(port, lvid, is_dvr=True,
+                             network_type=n_const.TYPE_VXLAN):
         resp = [
             mock.call.db_get_val('Port', port.port_name, 'other_config'),
             mock.call.set_db_attribute('Port', port.port_name, 'other_config',
                                        mock.ANY),
         ]
         if is_dvr:
-            resp = [mock.call.get_vifs_by_ids([])] + resp
+            resp = [
+                mock.call.get_vifs_by_ids([]),
+                mock.call.install_dvr_dst_mac_for_arp(
+                    network_type,
+                    dvr_mac=port.dvr_mac,
+                    gateway_mac=port.vif_mac,
+                    rtr_port=port.ofport,
+                    vlan_tag=lvid)
+            ] + resp
+        return resp
+
+    @staticmethod
+    def _expected_port_unbound(port, lvid, is_dvr=True,
+                               network_type=n_const.TYPE_VXLAN):
+        resp = []
+        if is_dvr:
+            resp = [
+                mock.call.delete_dvr_dst_mac_for_arp(
+                    network_type=network_type,
+                    dvr_mac=port.dvr_mac,
+                    gateway_mac=port.vif_mac,
+                    rtr_port=port.ofport,
+                    vlan_tag=lvid)
+            ]
         return resp
 
     def _expected_install_dvr_process(self, lvid, port, ip_version,
@@ -2964,6 +2988,7 @@ class TestOvsDvrNeutronAgent(object):
             gateway_ip = '2001:100::1'
             cidr = '2001:100::0/64'
         self._port.vif_mac = 'aa:bb:cc:11:22:33'
+        self._port.dvr_mac = self.agent.dvr_agent.dvr_mac_address
         gateway_mac = 'aa:bb:cc:66:66:66'
         self._compute_port.vif_mac = '77:88:99:00:11:22'
         physical_network = self._physical_network
@@ -3019,7 +3044,8 @@ class TestOvsDvrNeutronAgent(object):
                     lvid=lvid,
                     segmentation_id=segmentation_id,
                 ),
-            ] + self._expected_port_bound(self._port, lvid)
+            ] + self._expected_port_bound(self._port, lvid,
+                                          network_type=network_type)
             self.assertEqual(expected_on_int_br, int_br.mock_calls)
             self.assertEqual([], tun_br.mock_calls)
             self.assertEqual(expected_on_phys_br, phys_br.mock_calls)
@@ -3039,7 +3065,9 @@ class TestOvsDvrNeutronAgent(object):
                     dst_port=self._compute_port.ofport,
                     vlan_tag=segmentation_id,
                 ),
-            ] + self._expected_port_bound(self._compute_port, lvid, False)
+            ] + self._expected_port_bound(self._compute_port, lvid,
+                                          is_dvr=False,
+                                          network_type=network_type)
             self.assertEqual(expected_on_int_br, int_br.mock_calls)
             self.assertFalse([], tun_br.mock_calls)
             self.assertFalse([], phys_br.mock_calls)
@@ -3055,6 +3083,7 @@ class TestOvsDvrNeutronAgent(object):
             cidr = '2001:100::0/64'
         network_type = n_const.TYPE_VXLAN
         self._port.vif_mac = gateway_mac = 'aa:bb:cc:11:22:33'
+        self._port.dvr_mac = self.agent.dvr_agent.dvr_mac_address
         self._compute_port.vif_mac = '77:88:99:00:11:22'
         physical_network = self._physical_network
         segmentation_id = self._segmentation_id
@@ -3120,7 +3149,8 @@ class TestOvsDvrNeutronAgent(object):
                     dst_port=self._compute_port.ofport,
                     vlan_tag=lvid,
                 ),
-            ] + self._expected_port_bound(self._compute_port, lvid, False)
+            ] + self._expected_port_bound(self._compute_port, lvid, False,
+                                          network_type)
             self.assertEqual(expected_on_int_br, int_br.mock_calls)
             self.assertEqual([], tun_br.mock_calls)
             self.assertEqual([], phys_br.mock_calls)
@@ -3293,6 +3323,7 @@ class TestOvsDvrNeutronAgent(object):
         else:
             gateway_ip = '2001:100::1'
             cidr = '2001:100::0/64'
+        self._port.dvr_mac = self.agent.dvr_agent.dvr_mac_address
         gateway_mac = 'aa:bb:cc:11:22:33'
         int_br = mock.create_autospec(self.agent.int_br)
         tun_br = mock.create_autospec(self.agent.tun_br)
@@ -3383,12 +3414,16 @@ class TestOvsDvrNeutronAgent(object):
                     vif_mac=self._port.vif_mac),
             ])
             if network_type == 'vlan':
-                self.assertEqual([], int_br.mock_calls)
+                expected_unbound_dvr = self._expected_port_unbound(self._port,
+                    self._segmentation_id, network_type=network_type)
+                self.assertEqual(expected_unbound_dvr, int_br.mock_calls)
                 self.assertEqual([], tun_br.mock_calls)
                 self.assertEqual(expected, phys_br.mock_calls)
                 self.assertEqual({}, self.agent.dvr_agent.local_ports)
             else:
-                self.assertEqual([], int_br.mock_calls)
+                expected_unbound_dvr = self._expected_port_unbound(self._port,
+                    lvid, network_type=network_type)
+                self.assertEqual(expected_unbound_dvr, int_br.mock_calls)
                 self.assertEqual(expected, tun_br.mock_calls)
                 self.assertEqual([], phys_br.mock_calls)
 
@@ -3401,6 +3436,7 @@ class TestOvsDvrNeutronAgent(object):
         else:
             gateway_ip = '2001:100::1'
             cidr = '2001:100::0/64'
+        self._port.dvr_mac = self.agent.dvr_agent.dvr_mac_address
         gateway_mac = 'aa:bb:cc:11:22:33'
         int_br = mock.create_autospec(self.agent.int_br)
         tun_br = mock.create_autospec(self.agent.tun_br)

releasenotes/notes/force-arp-responder-true-for-dvr-5aabbfa51945dd5a.yaml

@@ -0,0 +1,8 @@
+---
+other:
+  - |
+    When the ``enable_distributed_routing`` (DVR) configuration option is set
+    to ``True`` and tunneling is enabled, the ``arp_responder`` option will
+    be forced to ``True`` since it is now required in order for ARP to work
+    properly. For more information, see bug
+    `1774459 <https://bugs.launchpad.net/neutron/+bug/1774459>`_.