Improve the PortBindingUpdateVirtualPortsEvent match filter

This patch improves the ``PortBindingUpdateVirtualPortsEvent``
match filter. These are the new conditions:
* Event delete: that happens when the port binding has been deleted
  because the port is no longer bound or the port has been
  deleted. That will remove the Neutron port host name.
  NOTE: in case the Neutron port has been deleted, the method
  ``Ml2Plugin.update_virtual_port_host`` won't update (create) a new
  PortBinding object.
* If the new register has virtual_parents but not the old one, that
  means the ovn-controller has received traffic with the VIP from
  this port. The port host ID must be set.
* If the virtual parents have changed, the port host ID must be
  updated.
* If the virtual parents have been removed, the port host ID must
  be removed too.

Newer versions of OVN [1] are handling the virtual port binding in
a different way. When the virtual parents are added or removed,
the related "Port_Binding" register is deleted and the created
again. This is why this new version includes the event "DELETE"
on the match method; when the register is deleted, the event
class considers that the port is no longer bound to a host and
removes the host name for the Neutron port.

[1]https://review.opendev.org/c/openstack/neutron/+/880890/

Change-Id: I34caf7d0212ccb4bd7259c4414e7c3994bd8da4d
(cherry picked from commit 2fbfe38)

Author: ralonsoh

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py

@@ -547,43 +547,42 @@ class PortBindingUpdateVirtualPortsEvent(row_event.RowEvent):
     def __init__(self, driver):
         self.driver = driver
         table = 'Port_Binding'
-        events = (self.ROW_UPDATE, )
+        events = (self.ROW_UPDATE, self.ROW_DELETE)
         super().__init__(events, table, None)
         self.event_name = 'PortBindingUpdateVirtualPortsEvent'
 
     def match_fn(self, event, row, old):
-        # This event should catch only those events from ports that are
-        # "virtual" or have been "virtual". The second happens when all virtual
-        # parent are disassociated; in the same transaction the
-        # "virtual-parents" list is removed from "options" and the type is set
-        # to "".
-        if (row.type != ovn_const.PB_TYPE_VIRTUAL and
-                getattr(old, 'type', None) != ovn_const.PB_TYPE_VIRTUAL):
-            return False
+        # This event should catch the events related to virtual parents (that
+        # are associated to virtual ports).
+        if event == self.ROW_DELETE:
+            # The port binding has been deleted, delete the host ID (if the
+            # port was not deleted before).
+            return True
 
         virtual_parents = (row.options or {}).get(
             ovn_const.LSP_OPTIONS_VIRTUAL_PARENTS_KEY)
         old_virtual_parents = getattr(old, 'options', {}).get(
             ovn_const.LSP_OPTIONS_VIRTUAL_PARENTS_KEY)
-        chassis = row.chassis
-        old_chassis = getattr(old, 'chassis', [])
-
-        if virtual_parents and chassis != old_chassis:
-            # That happens when the chassis is assigned (VIP is first detected
-            # in a port) or changed (the VIP changes of assigned port and
-            # host).
-            return True
-
-        if not virtual_parents and old_virtual_parents:
+        if virtual_parents != old_virtual_parents:
+            # 1) if virtual_parents and not old_virtual_parents:
+            # The port has received a virtual parent and now is bound.
+            # 2) elif (virtual_parents and old_virtual_parents and
+            #          old_virtual_parents != virtual_parents):
+            # If the port virtual parents have changed (the VIP is bound
+            # to another host because it's owned by another port).
+            # 3) if not virtual_parents and old_virtual_parents:
             # All virtual parent ports are removed, the VIP is unbound.
             return True
         return False
 
     def run(self, event, row, old):
-        virtual_parents = (row.options or {}).get(
-            ovn_const.LSP_OPTIONS_VIRTUAL_PARENTS_KEY)
-        chassis_uuid = (row.chassis[0].uuid if
-                        row.chassis and virtual_parents else None)
+        if event == self.ROW_DELETE:
+            chassis_uuid = None
+        else:
+            virtual_parents = (row.options or {}).get(
+                ovn_const.LSP_OPTIONS_VIRTUAL_PARENTS_KEY)
+            chassis_uuid = (row.chassis[0].uuid if
+                            row.chassis and virtual_parents else None)
         self.driver.update_virtual_port_host(row.logical_port, chassis_uuid)
 
 

neutron/plugins/ml2/plugin.py

@@ -2068,8 +2068,14 @@ def update_virtual_port_host(self, context, port_id, hostname):
         """
         hostname = hostname or ''
         with db_api.CONTEXT_WRITER.using(context):
-            for pb in ports_obj.PortBinding.get_objects(context,
-                                                        port_id=port_id):
+            pbindings = ports_obj.PortBinding.get_objects(context,
+                                                          port_id=port_id)
+            if not pbindings:
+                # The port has been deleted and there is no need to delete and
+                # create any port binding.
+                return
+
+            for pb in pbindings:
                 pb.delete()
 
             attrs = {'port_id': port_id,

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py

@@ -311,6 +311,17 @@ def _find_port_binding(self, port_id):
         rows = cmd.execute(check_error=True)
         return rows[0] if rows else None
 
+    def _set_port_binding_virtual_parent(self, port_id, parent_port_id):
+        pb_port_parent = self.sb_api.db_find_rows(
+            'Port_Binding', ('logical_port', '=', parent_port_id)).execute(
+            check_error=True)[0]
+        pb_port_vip = self.sb_api.db_find_rows(
+            'Port_Binding', ('logical_port', '=', port_id)).execute(
+            check_error=True)[0]
+        self.sb_api.db_set(
+            'Port_Binding', pb_port_vip.uuid,
+            ('virtual_parent', pb_port_parent.uuid)).execute(check_error=True)
+
     def _check_port_binding_type(self, port_id, port_type):
         def is_port_binding_type(port_id, port_type):
             bp = self._find_port_binding(port_id)
@@ -340,22 +351,54 @@ def test_virtual_port_host_update(self, mock_update_vip_host):
         port = self.create_port()
         self._check_port_binding_type(vip['id'], '')
 
+        # 1) Set the allowed address pairs.
         data = {'port': {'allowed_address_pairs': allowed_address_pairs}}
         req = self.new_update_request('ports', data, port['id'])
         req.get_response(self.api)
         # This test checks that the VIP "Port_Binding" register gets the type
         # and the corresponding "virtual-parents".
         self._check_port_binding_type(vip['id'], ovn_const.LSP_TYPE_VIRTUAL)
         self._check_port_virtual_parents(vip['id'], port['id'])
-        mock_update_vip_host.assert_not_called()
+        n_utils.wait_until_true(lambda: mock_update_vip_host.called,
+                                timeout=10)
+        # The "Port_Binding" has been deleted. Then the "Port_Binding" register
+        # is created again without virtual_parents, but this event doesn't
+        # call "update_virtual_port_host".
+        mock_update_vip_host.assert_called_once_with(vip['id'], None)
 
+        # 2) Unset the allowed address pairs.
+        # Assign the VIP again and delete the virtual port.
+        # Before unsetting the allowed address pairs, we first manually add
+        # the Port_Binding.virtual_parent of the virtual port. That happens
+        # when an ovn-controller detects traffic with the VIP and assign the
+        # port hosting the VIP as virtual parent.
+        self._set_port_binding_virtual_parent(vip['id'], port['id'])
+        mock_update_vip_host.reset_mock()
         data = {'port': {'allowed_address_pairs': []}}
         req = self.new_update_request('ports', data, port['id'])
         req.get_response(self.api)
         self._check_port_binding_type(vip['id'], '')
         self._check_port_virtual_parents(vip['id'], None)
         n_utils.wait_until_true(lambda: mock_update_vip_host.called,
                                 timeout=10)
+        # The virtual port is no longer considered as virtual. The
+        # "Port_Binding" register is deleted.
+        mock_update_vip_host.assert_called_once_with(vip['id'], None)
+
+        # 3) Set again the allowed address pairs.
+        mock_update_vip_host.reset_mock()
+        data = {'port': {'allowed_address_pairs': allowed_address_pairs}}
+        req = self.new_update_request('ports', data, port['id'])
+        req.get_response(self.api)
+        # This test checks that the VIP "Port_Binding" register gets the type
+        # and the corresponding "virtual-parents".
+        self._check_port_binding_type(vip['id'], ovn_const.LSP_TYPE_VIRTUAL)
+        self._check_port_virtual_parents(vip['id'], port['id'])
+        mock_update_vip_host.reset_mock()
+        self._delete('ports', vip['id'])
+        n_utils.wait_until_true(lambda: mock_update_vip_host.called,
+                                timeout=10)
+        # The virtual port is deleted and so the associated "Port_Binding".
         mock_update_vip_host.assert_called_once_with(vip['id'], None)