From 3719a1add73b860c50d85fad0b270c1b69fb9147 Mon Sep 17 00:00:00 2001

From: Dumitru Ceara <dceara@redhat.com>

Date: Wed, 13 Jan 2021 10:23:32 +0100

Subject: [PATCH 3/3] binding: Set Logical_Switch_Port.up when all OVS flows

 are installed.

Using the ofctrl-seqno generic barrier, register a new type of

notifications for Port_Bindings.  This allows us to delay setting

the Logical_Switch_Port.up field until all OVS flows corresponding

to the logical port and underlying OVS interface have been installed.

This commit also marks the OVS interface as "installed by OVN" by

setting a new "ovn-installed" external-id in the OVS Interface record

when the port is fully wired by OVN.

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

Acked-by: Mark Michelson <mmichels@redhat.com>

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

(cherry picked from upstream master commit 4d3cb42b076bb58fd8f01ab8ad146ffd539f2152)

Change-Id: Id8e4fc3b28110cab2571d4ff8bc5ef81ae9b88c4

---

 NEWS                        |   6 ++

 controller/binding.c        | 181 ++++++++++++++++++++++++++++++++++++++++++++

 controller/binding.h        |   5 ++

 controller/ovn-controller.c |  14 +++-

 northd/ovn-northd.c         |   6 +-

 ovn-sb.ovsschema            |   5 +-

 ovn-sb.xml                  |   8 ++

 tests/ovn.at                |  32 ++++++++

 utilities/ovn-sbctl.c       |   4 +

 9 files changed, 254 insertions(+), 7 deletions(-)

diff --git a/NEWS b/NEWS

index 7d2ba56..e89c5f4 100644

--- a/NEWS

+++ b/NEWS

@@ -4,6 +4,12 @@ Post-v20.12.0

    - BFD protocol support according to RFC880 [0]. Introduce next-hop BFD

      availability check for OVN static routes.

      [0] https://tools.ietf.org/html/rfc5880)

+  - Change the semantic of the "Logical_Switch_Port.up" field such that it is

+    set to "true" only when all corresponding OVS openflow operations have

+    been processed.  This also introduces a new "OVS.Interface.external-id",

+    "ovn-installed".  This external-id is set by ovn-controller only after all

+    openflow operations corresponding to the OVS interface being added have

+    been processed.

 OVN v20.12.0 - 18 Dec 2020

 --------------------------

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

index c8e8591..d44f635 100644

--- a/controller/binding.c

+++ b/controller/binding.c

@@ -18,6 +18,7 @@

 #include "ha-chassis.h"

 #include "lflow.h"

 #include "lport.h"

+#include "ofctrl-seqno.h"

 #include "patch.h"

 #include "lib/bitmap.h"

@@ -34,6 +35,38 @@

 VLOG_DEFINE_THIS_MODULE(binding);

+/* External ID to be set in the OVS.Interface record when the OVS interface

+ * is ready for use, i.e., is bound to an OVN port and its corresponding

+ * flows have been installed.

+ */

+#define OVN_INSTALLED_EXT_ID "ovn-installed"

+

+/* Set of OVS interface IDs that have been released in the most recent

+ * processing iterations.  This gets updated in release_lport() and is

+ * periodically emptied in binding_seqno_run().

+ */

+static struct sset binding_iface_released_set =

+    SSET_INITIALIZER(&binding_iface_released_set);

+

+/* Set of OVS interface IDs that have been bound in the most recent

+ * processing iterations.  This gets updated in release_lport() and is

+ * periodically emptied in binding_seqno_run().

+ */

+static struct sset binding_iface_bound_set =

+    SSET_INITIALIZER(&binding_iface_bound_set);

+

+static void

+binding_iface_released_add(const char *iface_id)

+{

+    sset_add(&binding_iface_released_set, iface_id);

+}

+

+static void

+binding_iface_bound_add(const char *iface_id)

+{

+    sset_add(&binding_iface_bound_set, iface_id);

+}

+

 #define OVN_QOS_TYPE "linux-htb"

 struct qos_queue {

@@ -845,6 +878,7 @@ claim_lport(const struct sbrec_port_binding *pb,

             return false;

         }

+        binding_iface_bound_add(pb->logical_port);

         if (pb->chassis) {

             VLOG_INFO("Changing chassis for lport %s from %s to %s.",

                     pb->logical_port, pb->chassis->name,

@@ -908,7 +942,9 @@ 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);

     update_lport_tracking(pb, tracked_datapaths);

+    binding_iface_released_add(pb->logical_port);

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

     return true;

 }

@@ -2358,3 +2394,148 @@ delete_done:

     destroy_qos_map(&qos_map);

     return handled;

 }

+

+/* Registered ofctrl seqno type for port_binding flow installation. */

+static size_t binding_seq_type_pb_cfg;

+

+/* Binding specific seqno to be acked by ofctrl when flows for new interfaces

+ * have been installed.

+ */

+static uint32_t binding_iface_seqno = 0;

+

+/* Map indexed by iface-id containing the sequence numbers that when acked

+ * indicate that the OVS flows for the iface-id have been installed.

+ */

+static struct simap binding_iface_seqno_map =

+    SIMAP_INITIALIZER(&binding_iface_seqno_map);

+

+void

+binding_init(void)

+{

+    binding_seq_type_pb_cfg = ofctrl_seqno_add_type();

+}

+

+/* Processes new release/bind operations OVN ports.  For newly bound ports

+ * it creates ofctrl seqno update requests that will be acked when

+ * corresponding OVS flows have been installed.

+ *

+ * NOTE: Should be called only when valid SB and OVS transactions are

+ * available.

+ */

+void

+binding_seqno_run(struct shash *local_bindings)

+{

+    const char *iface_id;

+    const char *iface_id_next;

+

+    SSET_FOR_EACH_SAFE (iface_id, iface_id_next, &binding_iface_released_set) {

+        struct shash_node *lb_node = shash_find(local_bindings, iface_id);

+

+        /* If the local binding still exists (i.e., the OVS interface is

+         * still configured locally) then remove the external id and remove

+         * it from the in-flight seqno map.

+         */

+        if (lb_node) {

+            struct local_binding *lb = lb_node->data;

+

+            if (lb->iface && smap_get(&lb->iface->external_ids,

+                                      OVN_INSTALLED_EXT_ID)) {

+                ovsrec_interface_update_external_ids_delkey(

+                    lb->iface, OVN_INSTALLED_EXT_ID);

+            }

+        }

+        simap_find_and_delete(&binding_iface_seqno_map, iface_id);

+        sset_delete(&binding_iface_released_set,

+                    SSET_NODE_FROM_NAME(iface_id));

+    }

+

+    bool new_ifaces = false;

+    uint32_t new_seqno = binding_iface_seqno + 1;

+

+    SSET_FOR_EACH_SAFE (iface_id, iface_id_next, &binding_iface_bound_set) {

+        struct shash_node *lb_node = shash_find(local_bindings, iface_id);

+

+        if (lb_node) {

+            /* This is a newly bound interface, make sure we reset the

+             * Port_Binding 'up' field and the OVS Interface 'external-id'.

+             */

+            struct local_binding *lb = lb_node->data;

+

+            ovs_assert(lb->pb && lb->iface);

+            new_ifaces = true;

+

+            if (smap_get(&lb->iface->external_ids, OVN_INSTALLED_EXT_ID)) {

+                ovsrec_interface_update_external_ids_delkey(

+                    lb->iface, OVN_INSTALLED_EXT_ID);

+            }

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

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

+        }

+        sset_delete(&binding_iface_bound_set, SSET_NODE_FROM_NAME(iface_id));

+    }

+

+    /* Request a seqno update when the flows for new interfaces have been

+     * installed in OVS.

+     */

+    if (new_ifaces) {

+        binding_iface_seqno = new_seqno;

+        ofctrl_seqno_update_create(binding_seq_type_pb_cfg, new_seqno);

+    }

+}

+

+/* Processes ofctrl seqno ACKs for new bindings.  Sets the

+ * 'OVN_INSTALLED_EXT_ID' external-id in the OVS interface and the

+ * Port_Binding.up field for all ports for which OVS flows have been

+ * installed.

+ *

+ * NOTE: Should be called only when valid SB and OVS transactions are

+ * available.

+ */

+void

+binding_seqno_install(struct shash *local_bindings)

+{

+    struct ofctrl_acked_seqnos *acked_seqnos =

+            ofctrl_acked_seqnos_get(binding_seq_type_pb_cfg);

+    struct simap_node *node;

+    struct simap_node *node_next;

+

+    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;

+        }

+

+        struct local_binding *lb = lb_node->data;

+        if (!lb->pb || !lb->iface) {

+            goto del_seqno;

+        }

+

+        if (!ofctrl_acked_seqnos_contains(acked_seqnos, node->data)) {

+            continue;

+        }

+

+        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);

+        }

+

+del_seqno:

+        simap_delete(&binding_iface_seqno_map, node);

+    }

+

+    ofctrl_acked_seqnos_destroy(acked_seqnos);

+}

+

+void

+binding_seqno_flush(void)

+{

+    simap_clear(&binding_iface_seqno_map);

+}

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

index 2885971..c9ebef4 100644

--- a/controller/binding.h

+++ b/controller/binding.h

@@ -135,4 +135,9 @@ bool binding_handle_ovs_interface_changes(struct binding_ctx_in *,

 bool binding_handle_port_binding_changes(struct binding_ctx_in *,

                                          struct binding_ctx_out *);

 void binding_tracked_dp_destroy(struct hmap *tracked_datapaths);

+

+void binding_init(void);

+void binding_seqno_run(struct shash *local_bindings);

+void binding_seqno_install(struct shash *local_bindings);

+void binding_seqno_flush(void);

 #endif /* controller/binding.h */

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

index bb1c659..5599ea4 100644

--- a/controller/ovn-controller.c

+++ b/controller/ovn-controller.c

@@ -981,6 +981,7 @@ en_ofctrl_is_connected_run(struct engine_node *node, void *data)

         /* Flush ofctrl seqno requests when the ofctrl connection goes down. */

         if (!of_data->connected) {

             ofctrl_seqno_flush();

+            binding_seqno_flush();

         }

         engine_set_node_state(node, EN_UPDATED);

         return;

@@ -2404,13 +2405,14 @@ main(int argc, char *argv[])

     daemonize_complete();

+    /* Register ofctrl seqno types. */

+    ofctrl_seq_type_nb_cfg = ofctrl_seqno_add_type();

+

+    binding_init();

     patch_init();

     pinctrl_init();

     lflow_init();

-    /* Register ofctrl seqno types. */

-    ofctrl_seq_type_nb_cfg = ofctrl_seqno_add_type();

-

     /* Connect to OVS OVSDB instance. */

     struct ovsdb_idl_loop ovs_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(

         ovsdb_idl_create(ovs_remote, &ovsrec_idl_class, false, true));

@@ -2879,6 +2881,9 @@ main(int argc, char *argv[])

                                                        ovnsb_idl_loop.idl),

                                               ovnsb_cond_seqno,

                                               ovnsb_expected_cond_seqno));

+                    if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) {

+                        binding_seqno_run(&runtime_data->local_bindings);

+                    }

                     flow_output_data = engine_get_data(&en_flow_output);

                     if (flow_output_data && ct_zones_data) {

@@ -2889,6 +2894,9 @@ main(int argc, char *argv[])

                                    engine_node_changed(&en_flow_output));

                     }

                     ofctrl_seqno_run(ofctrl_get_cur_cfg());

+                    if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) {

+                        binding_seqno_install(&runtime_data->local_bindings);

+                    }

                 }

             }

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

index 9f8fb3b..307ee9c 100644

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

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

@@ -13049,7 +13049,7 @@ handle_port_binding_changes(struct northd_context *ctx, struct hmap *ports,

             continue;

         }

-        bool up = (sb->chassis || lsp_is_router(op->nbsp));

+        bool up = ((sb->up && (*sb->up)) || lsp_is_router(op->nbsp));

         if (!op->nbsp->up || *op->nbsp->up != up) {

             nbrec_logical_switch_port_set_up(op->nbsp, &up, 1);

         }

@@ -13197,7 +13197,7 @@ static const char *rbac_encap_update[] =

 static const char *rbac_port_binding_auth[] =

     {""};

 static const char *rbac_port_binding_update[] =

-    {"chassis"};

+    {"chassis", "up"};

 static const char *rbac_mac_binding_auth[] =

     {""};

@@ -13684,6 +13684,8 @@ main(int argc, char *argv[])

     ovsdb_idl_add_column(ovnsb_idl_loop.idl,

                          &sbrec_port_binding_col_virtual_parent);

     ovsdb_idl_add_column(ovnsb_idl_loop.idl,

+                         &sbrec_port_binding_col_up);

+    ovsdb_idl_add_column(ovnsb_idl_loop.idl,

                          &sbrec_gateway_chassis_col_chassis);

     ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_gateway_chassis_col_name);

     ovsdb_idl_add_column(ovnsb_idl_loop.idl,

diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema

index 97db6de..b418434 100644

--- a/ovn-sb.ovsschema

+++ b/ovn-sb.ovsschema

@@ -1,7 +1,7 @@

 {

     "name": "OVN_Southbound",

-    "version": "20.13.0",

-    "cksum": "3035725595 25676",

+    "version": "20.14.0",

+    "cksum": "1412040198 25748",

     "tables": {

         "SB_Global": {

             "columns": {

@@ -225,6 +225,7 @@

                 "nat_addresses": {"type": {"key": "string",

                                            "min": 0,

                                            "max": "unlimited"}},

+                "up": {"type": {"key": "boolean", "min": 0, "max": 1}},

                 "external_ids": {"type": {"key": "string",

                                  "value": "string",

                                  "min": 0,

diff --git a/ovn-sb.xml b/ovn-sb.xml

index eb440e4..4c82d51 100644

--- a/ovn-sb.xml

+++ b/ovn-sb.xml

@@ -2771,6 +2771,14 @@ tcp.flags = RST;

       </column>

+      <column name="up">

+        

+          This is set to true whenever all OVS flows

+          required by this Port_Binding have been installed.  This is

+          populated by ovn-controller.

+        

+      </column>

+

       <column name="tunnel_key">

           A number that represents the logical port in the key (e.g. STT key or

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

index a4fafa5..dfb94d2 100644

--- a/tests/ovn.at

+++ b/tests/ovn.at

@@ -23662,3 +23662,35 @@ as ovn-nb

 OVS_APP_EXIT_AND_WAIT([ovsdb-server])

 AT_CLEANUP

+

+AT_SETUP([ovn -- propagate Port_Binding.up to NB and OVS])

+ovn_start

+

+net_add n1

+sim_add hv1

+as hv1

+ovs-vsctl add-br br-phys

+ovn_attach n1 br-phys 192.168.0.1

+

+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 ovs-vsctl add-port br-int lsp1 -- set Interface lsp1 external-ids:iface-id=lsp1

+check_column "true" Port_Binding up logical_port=lsp1

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

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

+

+AS_BOX([add LSP for existing OVS port])

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

+check ovn-nbctl lsp-add ls lsp2

+check ovn-nbctl --wait=hv sync

+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"'])

+

+OVN_CLEANUP([hv1])

+AT_CLEANUP

diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c

index 0a1b9ff..c38e8ec 100644

--- a/utilities/ovn-sbctl.c

+++ b/utilities/ovn-sbctl.c

@@ -526,6 +526,7 @@ pre_get_info(struct ctl_context *ctx)

     ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_tunnel_key);

     ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_chassis);

     ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_datapath);

+    ovsdb_idl_add_column(ctx->idl, &sbrec_port_binding_col_up);

     ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_logical_datapath);

     ovsdb_idl_add_column(ctx->idl, &sbrec_logical_flow_col_logical_dp_group);

@@ -665,6 +666,7 @@ cmd_lsp_bind(struct ctl_context *ctx)

     struct sbctl_chassis *sbctl_ch;

     struct sbctl_port_binding *sbctl_bd;

     char *lport_name, *ch_name;

+    bool up = true;

     /* port_binding must exist, chassis must exist! */

     lport_name = ctx->argv[1];

@@ -683,6 +685,7 @@ cmd_lsp_bind(struct ctl_context *ctx)

         }

     }

     sbrec_port_binding_set_chassis(sbctl_bd->bd_cfg, sbctl_ch->ch_cfg);

+    sbrec_port_binding_set_up(sbctl_bd->bd_cfg, &up, 1);

     sbctl_context_invalidate_cache(ctx);

 }

@@ -699,6 +702,7 @@ cmd_lsp_unbind(struct ctl_context *ctx)

     sbctl_bd = find_port_binding(sbctl_ctx, lport_name, must_exist);

     if (sbctl_bd) {

         sbrec_port_binding_set_chassis(sbctl_bd->bd_cfg, NULL);

+        sbrec_port_binding_set_up(sbctl_bd->bd_cfg, NULL, 0);

     }

 }

-- 

1.8.3.1