Clean MAC_Binding entries when (dis)associating a FIP

When a FIP is assigned to a port, it may happen that it was
previously used by another FIP or gateway port and an entry
in MAC_Binding table exits. If that's the case, the ARP
responder will answer with the wrong MAC address and the FIP
will then be unreachable.

In order to be on the safe side, this patch is deleting all
MAC_Binding entries upon association or disassociation of a
FIP as a workaround. The proper way to fix this should be
making ovn-northd or ovn-controller clearing/updating the
stale entries upon new NAT rules creation.

Details about the bug reported in OVS ML at [0].

[0] https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047604.html

Change-Id: Ic04fcbe332adf030ab0ee3aad3a94ad874b18c9a
Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>

3 files changed, 135 insertions, 1 deletions

diff --git a/networking_ovn/ml2/mech_driver.py b/networking_ovn/ml2/mech_driver.py
index 17b7d7a..92b3d0c 100644
--- a/networking_ovn/ml2/mech_driver.py
+++ b/networking_ovn/ml2/mech_driver.py
@@ -801,6 +801,13 @@ class OVNMechanismDriver(api.MechanismDriver):
             LOG.debug("Port not found during OVN status down report: %s",
                       port_id)
 
+    def delete_mac_binding_entries(self, external_ip):
+        """Delete all MAC_Binding entries associated to this IP address"""
+        mac_binds = self._sb_ovn.db_find_rows(
+            'MAC_Binding', ('ip', '=', external_ip)).execute() or []
+        for entry in mac_binds:
+            self._sb_ovn.db_destroy('MAC_Binding', entry.uuid).execute()
+
     def update_segment_host_mapping(self, host, phy_nets):
         """Update SegmentHostMapping in DB"""
         if not host:
diff --git a/networking_ovn/ovsdb/ovsdb_monitor.py b/networking_ovn/ovsdb/ovsdb_monitor.py
index 7790cd7..eb99b14 100644
--- a/networking_ovn/ovsdb/ovsdb_monitor.py
+++ b/networking_ovn/ovsdb/ovsdb_monitor.py
@@ -199,6 +199,27 @@ class LogicalSwitchPortUpdateDownEvent(row_event.RowEvent):
         self.driver.set_port_status_down(row.name)
 
 
+class FIPAddDeleteEvent(row_event.RowEvent):
+    """Row event - NAT 'dnat_and_snat' entry added or deleted
+
+    This happens when a FIP is created or removed.
+    """
+    def __init__(self, driver):
+        self.driver = driver
+        table = 'NAT'
+        events = (self.ROW_CREATE, self.ROW_DELETE)
+        super(FIPAddDeleteEvent, self).__init__(
+            events, table, (('type', '=', 'dnat_and_snat'),))
+        self.event_name = 'FIPAddDeleteEvent'
+
+    def run(self, event, row, old):
+        # When a FIP is added or deleted, we will delete all entries in the
+        # MAC_Binding table of SB OVSDB corresponding to that IP Address.
+        # TODO(dalvarez): Remove this workaround once fixed in core OVN:
+        # https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047604.html
+        self.driver.delete_mac_binding_entries(row.external_ip)
+
+
 class OvnDbNotifyHandler(event.RowEventHandler):
     def __init__(self, driver):
         super(OvnDbNotifyHandler, self).__init__()
@@ -279,11 +300,13 @@ class OvnNbIdl(OvnIdl):
         self._lsp_update_down_event = LogicalSwitchPortUpdateDownEvent(driver)
         self._lsp_create_up_event = LogicalSwitchPortCreateUpEvent(driver)
         self._lsp_create_down_event = LogicalSwitchPortCreateDownEvent(driver)
+        self._fip_create_delete_event = FIPAddDeleteEvent(driver)
 
         self.notify_handler.watch_events([self._lsp_create_up_event,
                                           self._lsp_create_down_event,
                                           self._lsp_update_up_event,
-                                          self._lsp_update_down_event])
+                                          self._lsp_update_down_event,
+                                          self._fip_create_delete_event])
 
     @classmethod
     def from_server(cls, connection_string, schema_name, driver):
@@ -323,6 +346,7 @@ class OvnSbIdl(OvnIdl):
         helper.register_table('Encap')
         helper.register_table('Port_Binding')
         helper.register_table('Datapath_Binding')
+        helper.register_table('MAC_Binding')
         _idl = cls(driver, connection_string, helper)
         _idl.set_lock(_idl.event_lock_name)
         return _idl
diff --git a/networking_ovn/tests/functional/test_ovsdb_monitor.py b/networking_ovn/tests/functional/test_ovsdb_monitor.py
index 0e61248..c8bde1f 100644
--- a/networking_ovn/tests/functional/test_ovsdb_monitor.py
+++ b/networking_ovn/tests/functional/test_ovsdb_monitor.py
@@ -12,6 +12,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import threading
+
 import mock
 from oslo_utils import uuidutils
 
@@ -19,7 +21,31 @@ from networking_ovn.ovsdb import ovsdb_monitor
 from networking_ovn.tests.functional import base
 from neutron.common import utils as n_utils
 from neutron_lib.api.definitions import portbindings
+from neutron_lib.plugins import constants as plugin_constants
 from neutron_lib.plugins import directory
+from ovsdbapp.backend.ovs_idl import event
+
+
+class WaitForMACBindingDeleteEvent(event.RowEvent):
+    # TODO(dalvarez): Use WaitEvent from ovsdbapp once this patch
+    # https://review.openstack.org/#/c/613121 merges.
+    event_name = 'WaitForMACBindingDeleteEvent'
+    ONETIME = True
+
+    def __init__(self, entry):
+        self.event = threading.Event()
+        self.timeout = 15
+        table = 'MAC_Binding'
+        events = (self.ROW_DELETE)
+        conditions = (('_uuid', '=', entry),)
+        super(WaitForMACBindingDeleteEvent, self).__init__(
+            events, table, conditions)
+
+    def run(self, event, row, old):
+        self.event.set()
+
+    def wait(self):
+        return self.event.wait(self.timeout)
 
 
 class TestNBDbMonitor(base.TestOVNFunctionalBase):
@@ -27,6 +53,7 @@ class TestNBDbMonitor(base.TestOVNFunctionalBase):
     def setUp(self):
         super(TestNBDbMonitor, self).setUp(ovn_worker=True)
         self.chassis = self.add_fake_chassis('ovs-host1')
+        self.l3_plugin = directory.get_plugin(plugin_constants.L3)
 
     def create_port(self):
         net = self._make_network(self.fmt, 'net1', True)
@@ -41,6 +68,82 @@ class TestNBDbMonitor(base.TestOVNFunctionalBase):
         port = self.deserialize(self.fmt, port_res)['port']
         return port
 
+    def _create_fip(self, port, fip_address):
+        e1 = self._make_network(self.fmt, 'e1', True,
+                                arg_list=('router:external',
+                                          'provider:network_type',
+                                          'provider:physical_network'),
+                                **{'router:external': True,
+                                   'provider:network_type': 'flat',
+                                   'provider:physical_network': 'public'})
+        res = self._create_subnet(self.fmt, e1['network']['id'],
+                                  '100.0.0.0/24', gateway_ip='100.0.0.254',
+                                  allocation_pools=[{'start': '100.0.0.2',
+                                                     'end': '100.0.0.253'}],
+                                  enable_dhcp=False)
+        e1_s1 = self.deserialize(self.fmt, res)
+        r1 = self.l3_plugin.create_router(
+            self.context,
+            {'router': {
+                'name': 'r1', 'admin_state_up': True,
+                'tenant_id': self._tenant_id,
+                'external_gateway_info': {
+                    'enable_snat': True,
+                    'network_id': e1['network']['id'],
+                    'external_fixed_ips': [
+                        {'ip_address': '100.0.0.2',
+                         'subnet_id': e1_s1['subnet']['id']}]}}})
+        self.l3_plugin.add_router_interface(
+            self.context, r1['id'],
+            {'subnet_id': port['fixed_ips'][0]['subnet_id']})
+        r1_f2 = self.l3_plugin.create_floatingip(
+            self.context, {'floatingip': {
+                'tenant_id': self._tenant_id,
+                'floating_network_id': e1['network']['id'],
+                'subnet_id': None,
+                'floating_ip_address': fip_address,
+                'port_id': port['id']}})
+        return r1_f2
+
+    def test_floatingip_mac_bindings(self):
+        """Check that MAC_Binding entries are cleared on FIP add/removal
+
+        This test will:
+        * Create a MAC_Binding entry for an IP address on the
+        'network1' datapath.
+        * Create a FIP with that same IP address on an external.
+        network and associate it to a Neutron port on a private network.
+        * Check that the MAC_Binding entry gets deleted.
+        * Create a new MAC_Binding entry for the same IP address.
+        * Delete the FIP.
+        * Check that the MAC_Binding entry gets deleted.
+        """
+        self._make_network(self.fmt, 'network1', True)
+        dp = self.sb_api.db_find(
+            'Datapath_Binding',
+            ('external_ids', '=', {'name2': 'network1'})).execute()
+        macb_id = self.sb_api.db_create('MAC_Binding', datapath=dp[0]['_uuid'],
+                                        ip='100.0.0.21').execute()
+        port = self.create_port()
+
+        # Ensure that the MAC_Binding entry gets deleted after creating a FIP
+        row_event = WaitForMACBindingDeleteEvent(macb_id)
+        self.mech_driver._sb_ovn.idl.notify_handler.watch_event(row_event)
+        fip = self._create_fip(port, '100.0.0.21')
+        self.assertTrue(row_event.wait())
+
+        # Now that the FIP is created, add a new MAC_Binding entry with the
+        # same IP address
+
+        macb_id = self.sb_api.db_create('MAC_Binding', datapath=dp[0]['_uuid'],
+                                        ip='100.0.0.21').execute()
+
+        # Ensure that the MAC_Binding entry gets deleted after deleting the FIP
+        row_event = WaitForMACBindingDeleteEvent(macb_id)
+        self.mech_driver._sb_ovn.idl.notify_handler.watch_event(row_event)
+        self.l3_plugin.delete_floatingip(self.context, fip['id'])
+        self.assertTrue(row_event.wait())
+
     def _test_port_binding_and_status(self, port_id, action, status):
         # This function binds or unbinds port to chassis and
         # checks if port status matches with input status