diff mbox series

[ovs-dev,3/3] binding: Set Logical_Switch_Port.up when all OVS flows are installed.

Message ID 20210106163734.20347.51643.stgit@dceara.remote.csb
State Superseded
Headers show
Series controller: Notify when an OVN port is ready to use. | expand

Commit Message

Dumitru Ceara Jan. 6, 2021, 4:37 p.m. UTC
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>
---
 NEWS                        |    6 +
 controller/binding.c        |  174 +++++++++++++++++++++++++++++++++++++++++++
 controller/binding.h        |    5 +
 controller/ovn-controller.c |   14 +++
 northd/ovn-northd.c         |    4 +
 ovn-sb.ovsschema            |    5 +
 ovn-sb.xml                  |    8 ++
 utilities/ovn-sbctl.c       |    4 +
 8 files changed, 214 insertions(+), 6 deletions(-)
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index f0ac94b..058ebe4 100644
--- a/NEWS
+++ b/NEWS
@@ -1,6 +1,12 @@ 
 Post-v20.12.0
 -------------------------
   - Support ECMP multiple nexthops for reroute router policies.
+  - 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 - xx xxx xxxx
 --------------------------
diff --git a/controller/binding.c b/controller/binding.c
index e632203..8582e45 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 {
@@ -837,6 +870,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,
@@ -900,7 +934,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;
 }
@@ -2287,3 +2323,141 @@  binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
     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 (lb->iface && smap_get(&lb->iface->external_ids,
+                                      OVN_INSTALLED_EXT_ID)) {
+                ovsrec_interface_update_external_ids_delkey(
+                    lb->iface, OVN_INSTALLED_EXT_ID);
+            }
+            if (lb->pb) {
+                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) {
+        if (!ofctrl_acked_seqnos_contains(acked_seqnos, node->data)) {
+            continue;
+        }
+
+        struct shash_node *lb_node = shash_find(local_bindings, node->name);
+        struct local_binding *lb = lb_node->data;
+        bool up = true;
+
+        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);
+        }
+        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 c974056..3bb1690 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -134,4 +134,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 7a7825d..f3951b6 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));
@@ -2878,6 +2880,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) {
@@ -2888,6 +2893,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 f6acdc1..a9e8edf 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -12678,7 +12678,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);
         }
@@ -13315,6 +13315,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 5228839..b47879a 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Southbound",
-    "version": "20.12.0",
-    "cksum": "3969471120 24441",
+    "version": "20.12.1",
+    "cksum": "1448228268 24513",
     "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 c139948..744bbfc 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -2771,6 +2771,14 @@  tcp.flags = RST;
         </p>
       </column>
 
+      <column name="up">
+        <p>
+          This is set to <code>true</code> whenever all OVS flows
+          required by this Port_Binding have been installed.  This is
+          populated by <code>ovn-controller</code>.
+        </p>
+      </column>
+
       <column name="tunnel_key">
         <p>
           A number that represents the logical port in the key (e.g. STT key or
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);
     }
 }