From 3de7959b9018f53abd06320bc7f1a43ec216db7e Mon Sep 17 00:00:00 2001

From: Dumitru Ceara <dceara@redhat.com>

Date: Wed, 3 Feb 2021 20:36:41 +0100

Subject: [PATCH 2/4] binding: Set Port_Binding.up only if supported.

The supported upgrade procedure is to always upgrade ovn-controllers

first and OVN DBs and ovn-northd later.  This leads however to the

situation when ovn-controller might try to set the Port_Binding.up field

while the Southbound DB isn't yet aware of this field which would

trigger transaction failures and control plane/data plane outages.

To avoid such situations ovn-controller only sets the Port_Binding.up

field if it was explicitly set to 'false'.  This ensures that the SB

database was already upgraded.

On the ovn-northd side, as soon as ovn-northd is upgraded it will update

all existing Port_Bindings and explicitly set 'Port_Binding.up' to

false, implicitly notifying ovn-controller that it is safe to write to

the field.

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

Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")

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

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

(cherry picked from upstream commit 8b45fc9b2269df68566e47eee9cdd5f043b595d3)

Change-Id: I6fb8b59419466bbe43f0d993966d8316320c6327

---

 controller-vtep/binding.c |  6 ++++--

 controller/binding.c      | 33 ++++++++++++++++++++++----------

 northd/ovn-northd.c       |  8 ++++++++

 tests/ovn.at              | 48 ++++++++++++++++++++++++++++++++++++++++++++++-

 4 files changed, 82 insertions(+), 13 deletions(-)

diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c

index d28a598..01d5a16 100644

--- a/controller-vtep/binding.c

+++ b/controller-vtep/binding.c

@@ -110,9 +110,11 @@ update_pb_chassis(const struct sbrec_port_binding *port_binding_rec,

                      chassis_rec->name);

         }

-        bool up = true;

         sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec);

-        sbrec_port_binding_set_up(port_binding_rec, &up, 1);

+        if (port_binding_rec->n_up) {

+            bool up = true;

+            sbrec_port_binding_set_up(port_binding_rec, &up, 1);

+        }

     }

 }

diff --git a/controller/binding.c b/controller/binding.c

index 353debe..efaa109 100644

--- a/controller/binding.c

+++ b/controller/binding.c

@@ -870,6 +870,11 @@ get_lport_type(const struct sbrec_port_binding *pb)

  *   container and virtual ports).

  * Otherwise request a notification to be sent when the OVS flows

  * corresponding to 'pb' have been installed.

+ *

+ * Note:

+ *   Updates (directly or through a notification) the 'pb->up' field only if

+ *   it's explicitly set to 'false'.

+ *   This is to ensure compatibility with older versions of ovn-northd.

  */

 static void

 claimed_lport_set_up(const struct sbrec_port_binding *pb,

@@ -885,7 +890,7 @@ claimed_lport_set_up(const struct sbrec_port_binding *pb,

         return;

     }

-    if (pb->chassis != chassis_rec) {

+    if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) {

         binding_iface_bound_add(pb->logical_port);

     }

 }

@@ -973,7 +978,10 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,

         sbrec_port_binding_set_virtual_parent(pb, NULL);

     }

-    sbrec_port_binding_set_up(pb, NULL, 0);

+    if (pb->n_up) {

+        bool up = false;

+        sbrec_port_binding_set_up(pb, &up, 1);

+    }

     update_lport_tracking(pb, tracked_datapaths);

     binding_iface_released_add(pb->logical_port);

     VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);

@@ -2503,7 +2511,10 @@ binding_seqno_run(struct shash *local_bindings)

                 ovsrec_interface_update_external_ids_delkey(

                     lb->iface, OVN_INSTALLED_EXT_ID);

             }

-            sbrec_port_binding_set_up(lb->pb, NULL, 0);

+            if (lb->pb->n_up) {

+                bool up = false;

+                sbrec_port_binding_set_up(lb->pb, &up, 1);

+            }

             simap_put(&binding_iface_seqno_map, lb->name, new_seqno);

         }

         sset_delete(&binding_iface_bound_set, SSET_NODE_FROM_NAME(iface_id));

@@ -2536,7 +2547,6 @@ binding_seqno_install(struct shash *local_bindings)

     SIMAP_FOR_EACH_SAFE (node, node_next, &binding_iface_seqno_map) {

         struct shash_node *lb_node = shash_find(local_bindings, node->name);

-        bool up = true;

         if (!lb_node) {

             goto del_seqno;

@@ -2554,12 +2564,15 @@ binding_seqno_install(struct shash *local_bindings)

         ovsrec_interface_update_external_ids_setkey(lb->iface,

                                                     OVN_INSTALLED_EXT_ID,

                                                     "true");

-        sbrec_port_binding_set_up(lb->pb, &up, 1);

-

-        struct shash_node *child_node;

-        SHASH_FOR_EACH (child_node, &lb->children) {

-            struct local_binding *lb_child = child_node->data;

-            sbrec_port_binding_set_up(lb_child->pb, &up, 1);

+        if (lb->pb->n_up) {

+            bool up = true;

+

+            sbrec_port_binding_set_up(lb->pb, &up, 1);

+            struct shash_node *child_node;

+            SHASH_FOR_EACH (child_node, &lb->children) {

+                struct local_binding *lb_child = child_node->data;

+                sbrec_port_binding_set_up(lb_child->pb, &up, 1);

+            }

         }

 del_seqno:

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c

index 307ee9c..0dc920b 100644

--- a/northd/ovn-northd.c

+++ b/northd/ovn-northd.c

@@ -3329,6 +3329,14 @@ ovn_port_update_sbrec(struct northd_context *ctx,

     if (op->tunnel_key != op->sb->tunnel_key) {

         sbrec_port_binding_set_tunnel_key(op->sb, op->tunnel_key);

     }

+

+    /* ovn-controller will update 'Port_Binding.up' only if it was explicitly

+     * set to 'false'.

+     */

+    if (!op->sb->n_up) {

+        bool up = false;

+        sbrec_port_binding_set_up(op->sb, &up, 1);

+    }

 }

 /* Remove mac_binding entries that refer to logical_ports which are

diff --git a/tests/ovn.at b/tests/ovn.at

index 1956f5c..2ef056b 100644

--- a/tests/ovn.at

+++ b/tests/ovn.at

@@ -23697,7 +23697,7 @@ check ovn-nbctl ls-add ls

 AS_BOX([add OVS port for existing LSP])

 check ovn-nbctl lsp-add ls lsp1

 check ovn-nbctl --wait=hv sync

-check_column "[]" Port_Binding up logical_port=lsp1

+check_column "false" Port_Binding up logical_port=lsp1

 check ovs-vsctl add-port br-int lsp1 -- set Interface lsp1 external-ids:iface-id=lsp1

 check_column "true" Port_Binding up logical_port=lsp1

@@ -23712,5 +23712,51 @@ check_column "true" Port_Binding up logical_port=lsp2

 wait_column "true" nb:Logical_Switch_Port up name=lsp2

 OVS_WAIT_UNTIL([test `ovs-vsctl get Interface lsp2 external_ids:ovn-installed` = '"true"'])

+AS_BOX([ovn-controller should not reset Port_Binding.up without northd])

+# Pause northd and clear the "up" field to simulate older ovn-northd

+# versions writing to the Southbound DB.

+as northd ovn-appctl -t ovn-northd pause

+as northd-backup ovn-appctl -t ovn-northd pause

+

+as hv1 ovn-appctl -t ovn-controller debug/pause

+check ovn-sbctl clear Port_Binding lsp1 up

+as hv1 ovn-appctl -t ovn-controller debug/resume

+

+# Forcefully release the Port_Binding so ovn-controller reclaims it.

+# Make sure the Port_Binding.up field is not updated though.

+check ovn-sbctl clear Port_Binding lsp1 chassis

+hv1_uuid=$(fetch_column Chassis _uuid name=hv1)

+wait_column "$hv1_uuid" Port_Binding chassis logical_port=lsp1

+check_column "" Port_Binding up logical_port=lsp1

+

+# Once northd should explicitly set the Port_Binding.up field to 'false' and

+# ovn-controller sets it to 'true' as soon as the update is processed.

+as northd ovn-appctl -t ovn-northd resume

+as northd-backup ovn-appctl -t ovn-northd resume

+wait_column "true" Port_Binding up logical_port=lsp1

+wait_column "true" nb:Logical_Switch_Port up name=lsp1

+

+AS_BOX([ovn-controller should reset Port_Binding.up - from NULL])

+# If Port_Binding.up is cleared externally, ovn-northd resets it to 'false'

+# and ovn-controller finally sets it to 'true' once the update is processed.

+as hv1 ovn-appctl -t ovn-controller debug/pause

+check ovn-sbctl clear Port_Binding lsp1 up

+check ovn-nbctl --wait=sb sync

+wait_column "false" nb:Logical_Switch_Port up name=lsp1

+as hv1 ovn-appctl -t ovn-controller debug/resume

+wait_column "true" Port_Binding up logical_port=lsp1

+wait_column "true" nb:Logical_Switch_Port up name=lsp1

+

+AS_BOX([ovn-controller should reset Port_Binding.up - from false])

+# If Port_Binding.up is externally set to 'false', ovn-controller should sets

+# it to 'true' once the update is processed.

+as hv1 ovn-appctl -t ovn-controller debug/pause

+check ovn-sbctl set Port_Binding lsp1 up=false

+check ovn-nbctl --wait=sb sync

+wait_column "false" nb:Logical_Switch_Port up name=lsp1

+as hv1 ovn-appctl -t ovn-controller debug/resume

+wait_column "true" Port_Binding up logical_port=lsp1

+wait_column "true" nb:Logical_Switch_Port up name=lsp1

+

 OVN_CLEANUP([hv1])

 AT_CLEANUP

-- 

1.8.3.1