From 47afa0664d6a41d0a75a65f0ba927974d957cb62 Mon Sep 17 00:00:00 2001

From: Dumitru Ceara <dceara@redhat.com>

Date: Mon, 18 Jan 2021 17:50:33 +0100

Subject: [PATCH 2/2] binding: Always delete child port bindings first.

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@redhat.com>

Signed-off-by: Numan Siddique <numans@ovn.org>

(cherry picked from master commit de8030e6abc7b51dd2ee48bbe2c76592ef8b064c)

Change-Id: I382f463b757df1ff0f3b5b0e6ec2050d4e7811ed

---

 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 2cdc036..e2d2d8a 100644

--- a/tests/ovn.at

+++ b/tests/ovn.at

@@ -22194,6 +22194,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

-- 

1.8.3.1