[ovs-dev] [ovn 6/6] ovn-controller: Create tunnels based on Chassis configuration.

[Ben Pfaff]

On Mon, Apr 27, 2015 at 10:14:51PM -0700, Justin Pettit wrote:
> This creates a tunnel to each Chassis's specified Encaps.  In the
> future, we may want to limit this to only Chassis that share a logical
> datapath on the local system.
> 
> Signed-off-by: Justin Pettit <jpettit at nicira.com>

I had a little bit of a mental impedance matching problem with the
terminology used in this patch.  The patch seems to use the terms
"encaps(ulation)" and "tunnel" interchangeably.  I think of GRE, VXLAN,
Geneve, STT, etc., as "encapsulations", but instances of each of these
protocols between a pair of nodes as "tunnels".  This threw me for a bit
of a loop until I recognized it.

s/generarting/generating/ and s/remaing/remaining/:
> +     * generated we remove them.  After generarting all the rows, any
> +     * remaing in 'port_hmap' must be deleted from the database. */
> +    struct hmap port_hmap;

I noticed that the data values in port_names are only used for ports
that are also in a port_hash_node.  It seemed slightly counterintuitive
to me.  How about this incremental:

diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c
index 0148b67..d1ae85d 100644
--- a/ovn/controller/chassis.c
+++ b/ovn/controller/chassis.c
@@ -19,6 +19,7 @@
 #include "lib/hash.h"
 #include "lib/poll-loop.h"
 #include "lib/shash.h"
+#include "lib/sset.h"
 #include "lib/util.h"
 #include "lib/vswitch-idl.h"
 #include "openvswitch/vlog.h"
@@ -133,12 +134,9 @@ struct encap_ctx {
      * remaing in 'port_hmap' must be deleted from the database. */
     struct hmap port_hmap;
 
-    /* Contains a mapping of names of all ports on the switch,
-     * regardless of whether they are OVN, to their attached bridge.
-     * The names are used to make sure that any tunnel port we attempt
-     * to create is unique on the switch.  The bridge record values are
-     * used when tunnels are removed to know their attachment points. */
-    struct shash port_names;
+    /* Names of all ports in the bridge, to allow checking to uniqueness when
+     * adding a new tunnel. */
+    struct sset port_names;
 
     struct ovsdb_idl_txn *ovs_txn;
     const struct ovsrec_bridge *br_int;
@@ -147,6 +145,7 @@ struct encap_ctx {
 struct port_hash_node {
     struct hmap_node node;
     const struct ovsrec_port *port;
+    const struct ovsrec_bridge *bridge;
 };
 
 static size_t
@@ -185,7 +184,7 @@ encap_create_name(struct encap_ctx *ec, const char *chassis_id)
         char *port_name;
         port_name = xasprintf("ovn-%.6s-%x", chassis_id, i);
 
-        if (!shash_find(&ec->port_names, port_name)) {
+        if (!sset_contains(&ec->port_names, port_name)) {
             return port_name;
         }
 
@@ -266,7 +265,7 @@ encap_add(struct encap_ctx *ec, const char *new_chassis_id,
     ports[ec->br_int->n_ports] = port;
     ovsrec_bridge_set_ports(ec->br_int, ports, ec->br_int->n_ports + 1);
 
-    shash_add(&ec->port_names, port_name, ec->br_int);
+    sset_add(&ec->port_names, port_name);
     free(port_name);
     free(ports);
 }
@@ -313,7 +312,7 @@ update_encaps(struct controller_ctx *ctx)
 
     struct encap_ctx ec = {
         .port_hmap = HMAP_INITIALIZER(&ec.port_hmap),
-        .port_names = SHASH_INITIALIZER(&ec.port_names),
+        .port_names = SSET_INITIALIZER(&ec.port_names),
         .br_int = ctx->br_int
     };
 
@@ -322,16 +321,20 @@ update_encaps(struct controller_ctx *ctx)
                               "ovn-controller: modifying OVS encaps '%s'",
                               ctx->chassis_id);
 
+    /* Collect all port names into ec.port_names.
+     *
+     * Collect all the OVN-created tunnels into ec.port_hmap. */
     OVSREC_BRIDGE_FOR_EACH(br, ctx->ovs_idl) {
         size_t i;
 
         for (i = 0; i < br->n_ports; i++) {
             const struct ovsrec_port *port = br->ports[i];
 
-            shash_add(&ec.port_names, port->name, br);
+            sset_add(&ec.port_names, port->name);
 
             if (smap_get(&port->external_ids, "ovn-chassis-id")) {
                 struct port_hash_node *hash_node = xzalloc(sizeof *hash_node);
+                hash_node->bridge = br;
                 hash_node->port = port;
                 hmap_insert(&ec.port_hmap, &hash_node->node,
                             port_hash_rec(port));
@@ -353,14 +356,12 @@ update_encaps(struct controller_ctx *ctx)
     /* Delete any existing OVN encaps that were not still around. */
     struct port_hash_node *hash_node, *next_hash_node;
     HMAP_FOR_EACH_SAFE (hash_node, next_hash_node, node, &ec.port_hmap) {
-        const struct ovsrec_bridge *br = shash_find_data(&ec.port_names,
-                                                         hash_node->port->name);
         hmap_remove(&ec.port_hmap, &hash_node->node);
-        bridge_delete_port(br, hash_node->port);
+        bridge_delete_port(hash_node->bridge, hash_node->port);
         free(hash_node);
     }
     hmap_destroy(&ec.port_hmap);
-    shash_destroy(&ec.port_names);
+    sset_destroy(&ec.port_names);
 
     retval = ovsdb_idl_txn_commit_block(ec.ovs_txn);
     if (retval != TXN_SUCCESS && retval != TXN_UNCHANGED) {

Here in update_encaps(), I think that the !encap case here should not
call encap_add():

            /* Create tunnels to the other chassis. */
            const struct sbrec_encap *encap = preferred_tunnel(chassis_rec);
            if (!encap) {
                VLOG_INFO("No supported encaps for '%s'", chassis_rec->name);
            }
            encap_add(&ec, chassis_rec->name, encap);

I'm concerned about the growing number of blocking round-trips to the
database server.  I think it's OK for now, but I also think it will
eventually bite us.

I guess that preferred_tunnel() will prefer Geneve over STT since it's
first in alphabetical order and the IDL always sorts sets.  Maybe that
should be made more obvious?

Here in encap_add(), I think that the !port_name case should bail out
instead of continuing:
    port_name = encap_create_name(ec, new_chassis_id);
    if (!port_name) {
        VLOG_WARN("Unable to allocate unique name for '%s' encap",
                  new_chassis_id);
    }

I think that encap_add() leaks external_ids and options.

Thanks,

Ben.