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 <jpet...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
