On Thu, Aug 11, 2016 at 05:20:33PM -0700, Jesse Gross wrote:
> Originally, processing of encapsulations simply iterated over all tables on
> every wakeup and would replace anything that changed. This is somewhat
> inefficient but it captured all changes.
>
> Incremental processing avoided the need to do so much work but it could
> miss several types of changes. In particular, it only monitored the chassis
> table in the southbound database, so other changes (particularly in the
> encap table) were not reflected. In addition, while it corrected some
> changes to its data in OVS, others could go unnoticed.
>
> This attempts to fix those issues by reflecting the most recent updates
> to the southbound database in OVS at all times. It also increases safety
> by avoiding the possibility of dangling pointers to old database rows and
> eliminates the need to traverse the OVS database at all during most wakeups.
>
> Fixes: 1d45d5a9 ("ovn-controller: Change encaps_run to work incrementally.")
> Signed-off-by: Jesse Gross <je...@kernel.org>
Thanks for working on this.
This could use a few more high-level code comments. It's a little hard
to follow encaps_run(). I had to study it. I'm still not entirely sure
I understand all of it.
Usually, your code is super-robust, so some of this is on faith:
Acked-by: Ben Pfaff <b...@ovn.org>
I'm appending some suggestions.
diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index 52348c3..94290a7 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -119,10 +119,9 @@ insert_encap_uuid(const struct uuid *uuid,
{
struct encap_hash_node *hash_node = xmalloc(sizeof *hash_node);
- memcpy(&hash_node->encap_uuid, uuid, sizeof hash_node->encap_uuid);
+ hash_node->encap_uuid = *uuid;
hash_node->chassis_id = xstrdup(chassis_rec->name);
- memcpy(&hash_node->chassis_uuid, &chassis_rec->header_.uuid,
- sizeof hash_node->chassis_uuid);
+ hash_node->chassis_uuid = chassis_rec->header_.uuid;
hmap_insert(&tc.encap_chassis_hmap, &hash_node->node,
uuid_hash(&hash_node->encap_uuid));
}
@@ -165,12 +164,11 @@ insert_chassis_id(const char *chassis_id, const char
*port_name,
hash_node->chassis_id = xstrdup(chassis_id);
hash_node->port_name = xstrdup(port_name);
- memcpy(&hash_node->chassis_uuid, chassis_uuid,
- sizeof hash_node->chassis_uuid);
+ hash_node->chassis_uuid = *chassis_uuid;
/* We don't know the port's UUID until it has been inserted into the
* database, store zeros for now. It will get updated the next time we
* wake up. */
- memset(&hash_node->port_uuid, 0, sizeof hash_node->port_uuid);
+ uuid_zero(&hash_node->port_uuid);
hmap_insert(&tc.chassis_hmap, &hash_node->node, hash_string(chassis_id,
0));
}
@@ -350,6 +348,8 @@ encaps_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
process_full_bridge = false;
sset_clear(&tc.port_names);
+ /* Find all of the tunnel ports to remote chassis.
+ * Delete the tunnel ports fro unknown remote chassis. */
OVSREC_PORT_FOR_EACH (port_rec, ctx->ovs_idl) {
sset_add(&tc.port_names, port_rec->name);
for (int i = 0; i < port_rec->n_interfaces; i++) {
@@ -362,9 +362,7 @@ encaps_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
chassis_node = lookup_chassis_id(chassis_id);
if (chassis_node) {
if (uuid_is_zero(&chassis_node->port_uuid)) {
- memcpy(&chassis_node->port_uuid,
- &port_rec->header_.uuid,
- sizeof chassis_node->port_uuid);
+ chassis_node->port_uuid = port_rec->header_.uuid;
}
} else {
for (int i = 0; i < port_rec->n_interfaces; i++) {
@@ -384,14 +382,17 @@ encaps_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
&chassis_node->port_uuid);
if (!chassis_rec) {
+ /* Delete tunnel port (if present) for missing chassis. */
bridge_delete_port(tc.br_int, port_rec, chassis_node);
continue;
}
if (!port_rec) {
+ /* Delete our representation of the chassis, then add back. */
bridge_delete_port(tc.br_int, NULL, chassis_node);
check_and_add_tunnel(chassis_rec, local_chassis_id);
} else {
+ /* Update tunnel. */
check_and_update_tunnel(port_rec, chassis_rec);
}
}
@@ -420,6 +421,7 @@ encaps_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
}
}
+ /* Update tunnels based on changes to the Encap table. */
SBREC_ENCAP_FOR_EACH_TRACKED (encap_rec, ctx->ovnsb_idl) {
struct encap_hash_node *encap_hash_node;
struct chassis_hash_node *chassis_hash_node;
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
