[ovs-dev] [PATCH ovn branch-20.06 2/2] binding: Always delete child port bindings first.

[Dumitru Ceara]

When incrementally processing changes, child Port Bindings (container
and virtual ports) must be deleted first, before their parents, because
they need to be removed from their parent's children hash to avoid
parents with children with NULL port bindings.

Signed-off-by: Dumitru Ceara <dceara at redhat.com>

(cherry-picked from master commit de8030e6abc7b51dd2ee48bbe2c76592ef8b064c)
---
 controller/binding.c |   75 +++++++++++++++++++++++++++++++++++++++++++-------
 tests/ovn.at         |   18 ++++++++++++
 2 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index 3512a1d..c8e8591 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -2147,13 +2147,26 @@ bool
 binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
                                     struct binding_ctx_out *b_ctx_out)
 {
-    bool handled = true;
+    /* Run the tracked port binding loop twice to ensure correctness:
+     * 1. First to handle deleted changes.  This is split in four sub-parts
+     *    because child local bindings must be cleaned up first:
+     *    a. Container ports first.
+     *    b. Then virtual ports.
+     *    c. Then regular VIFs.
+     *    d. Last other ports.
+     * 2. Second to handle add/update changes.
+     */
+    struct shash deleted_container_pbs =
+        SHASH_INITIALIZER(&deleted_container_pbs);
+    struct shash deleted_virtual_pbs =
+        SHASH_INITIALIZER(&deleted_virtual_pbs);
+    struct shash deleted_vif_pbs =
+        SHASH_INITIALIZER(&deleted_vif_pbs);
+    struct shash deleted_other_pbs =
+        SHASH_INITIALIZER(&deleted_other_pbs);
     const struct sbrec_port_binding *pb;
+    bool handled = true;
 
-    /* Run the tracked port binding loop twice. One to handle deleted
-     * changes. And another to handle add/update changes.
-     * This will ensure correctness.
-     */
     SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
                                                b_ctx_in->port_binding_table) {
         if (!sbrec_port_binding_is_deleted(pb)) {
@@ -2161,18 +2174,60 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
         }
 
         enum en_lport_type lport_type = get_lport_type(pb);
-        if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) {
-            handled = handle_deleted_vif_lport(pb, lport_type, b_ctx_in,
-                                               b_ctx_out);
+
+        if (lport_type == LP_VIF) {
+            if (is_lport_container(pb)) {
+                shash_add(&deleted_container_pbs, pb->logical_port, pb);
+            } else {
+                shash_add(&deleted_vif_pbs, pb->logical_port, pb);
+            }
+        } else if (lport_type == LP_VIRTUAL) {
+            shash_add(&deleted_virtual_pbs, pb->logical_port, pb);
         } else {
-            handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
+            shash_add(&deleted_other_pbs, pb->logical_port, pb);
+        }
+    }
+
+    struct shash_node *node;
+    struct shash_node *node_next;
+    SHASH_FOR_EACH_SAFE (node, node_next, &deleted_container_pbs) {
+        handled = handle_deleted_vif_lport(node->data, LP_VIF, b_ctx_in,
+                                           b_ctx_out);
+        shash_delete(&deleted_container_pbs, node);
+        if (!handled) {
+            goto delete_done;
         }
+    }
 
+    SHASH_FOR_EACH_SAFE (node, node_next, &deleted_virtual_pbs) {
+        handled = handle_deleted_vif_lport(node->data, LP_VIRTUAL, b_ctx_in,
+                                           b_ctx_out);
+        shash_delete(&deleted_virtual_pbs, node);
         if (!handled) {
-            break;
+            goto delete_done;
+        }
+    }
+
+    SHASH_FOR_EACH_SAFE (node, node_next, &deleted_vif_pbs) {
+        handled = handle_deleted_vif_lport(node->data, LP_VIF, b_ctx_in,
+                                           b_ctx_out);
+        shash_delete(&deleted_vif_pbs, node);
+        if (!handled) {
+            goto delete_done;
         }
     }
 
+    SHASH_FOR_EACH_SAFE (node, node_next, &deleted_other_pbs) {
+        handle_deleted_lport(node->data, b_ctx_in, b_ctx_out);
+        shash_delete(&deleted_other_pbs, node);
+    }
+
+delete_done:
+    shash_destroy(&deleted_container_pbs);
+    shash_destroy(&deleted_virtual_pbs);
+    shash_destroy(&deleted_vif_pbs);
+    shash_destroy(&deleted_other_pbs);
+
     if (!handled) {
         return false;
     }
diff --git a/tests/ovn.at b/tests/ovn.at
index 812df1d..16ccf63 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -20160,6 +20160,24 @@ check ovn-nbctl lsp-add ls2 lsp-cont1 lsp1 1
 check ovn-nbctl --wait=hv sync
 check_row_count Port_Binding 1 logical_port=lsp-cont1 chassis=$ch
 
+AS_BOX([delete both OVN VIF and OVN container port])
+as hv1 ovn-appctl -t ovn-controller debug/pause
+check ovn-nbctl lsp-del lsp1 \
+    -- lsp-del lsp-cont1
+check ovn-nbctl --wait=sb sync
+as hv1 ovn-appctl -t ovn-controller debug/resume
+
+AS_BOX([readd both OVN VIF and OVN container port])
+as hv1 ovn-appctl -t ovn-controller debug/pause
+check ovn-nbctl lsp-add ls1 lsp1 \
+    -- lsp-add ls2 lsp-cont1 lsp1 1
+check ovn-nbctl --wait=sb sync
+as hv1 ovn-appctl -t ovn-controller debug/resume
+
+check ovn-nbctl --wait=hv sync
+wait_row_count Port_Binding 1 logical_port=lsp1 chassis=$ch
+wait_row_count Port_Binding 1 logical_port=lsp-cont1 chassis=$ch
+
 OVN_CLEANUP([hv1])
 AT_CLEANUP