]> git.proxmox.com Git - ovs.git/commitdiff
ovn-controller: Make encap processing more robust against changes.
authorJesse Gross <jesse@kernel.org>
Thu, 28 Jul 2016 22:03:56 +0000 (15:03 -0700)
committerJesse Gross <jesse@kernel.org>
Sun, 14 Aug 2016 05:37:51 +0000 (22:37 -0700)
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 <jesse@kernel.org>
Acked-by: Ryan Moats <rmoats@us.ibm.com>
Acked-by: Ben Pfaff <blp@ovn.org>
ovn/controller/encaps.c
ovn/controller/ovn-controller.c
tests/ovn-controller.at

index 6623f19ad71e6f9a13faa1d0a4141fbc00ef8b75..1ae3200b052665460dda238a9a8777875b884ac4 100644 (file)
@@ -20,6 +20,7 @@
 #include "lport.h"
 
 #include "lib/hash.h"
+#include "lib/poll-loop.h"
 #include "lib/sset.h"
 #include "lib/util.h"
 #include "lib/vswitch-idl.h"
@@ -35,26 +36,23 @@ encaps_register_ovs_idl(struct ovsdb_idl *ovs_idl)
     ovsdb_idl_add_table(ovs_idl, &ovsrec_table_bridge);
     ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_ports);
     ovsdb_idl_add_table(ovs_idl, &ovsrec_table_port);
-    ovsdb_idl_add_column(ovs_idl, &ovsrec_port_col_name);
-    ovsdb_idl_add_column(ovs_idl, &ovsrec_port_col_interfaces);
-    ovsdb_idl_add_column(ovs_idl, &ovsrec_port_col_external_ids);
+    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name);
+    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces);
+    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids);
     ovsdb_idl_add_table(ovs_idl, &ovsrec_table_interface);
-    ovsdb_idl_add_column(ovs_idl, &ovsrec_interface_col_name);
-    ovsdb_idl_add_column(ovs_idl, &ovsrec_interface_col_type);
-    ovsdb_idl_add_column(ovs_idl, &ovsrec_interface_col_options);
+    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
+    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type);
+    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options);
 }
 
 /* Enough context to create a new tunnel, using tunnel_add(). */
 struct tunnel_ctx {
-    /* Contains "struct port_hash_node"s.  Used to figure out what
-     * existing tunnels should be deleted: we index all of the OVN encap
-     * rows into this data structure, then as existing rows are
-     * generated we remove them.  After generating all the rows, any
-     * remaining in 'tunnel_hmap' must be deleted from the database. */
-    struct hmap tunnel_hmap;
+    /* Reverse mapping from an encap entry to it's parent chassis to
+     * allow updating the overall tunnel when any property changes. */
+    struct hmap encap_chassis_hmap;
 
-    /* Only valid within the process_full_encaps case in encaps_run(). */
-    struct hmap tunnel_hmap_by_uuid;
+    /* Table of chassis (indexed by chassis ID) to their tunnel ports in OVS. */
+    struct hmap chassis_hmap;
 
     /* Names of all ports in the bridge, to allow checking uniqueness when
      * adding a new tunnel. */
@@ -65,51 +63,15 @@ struct tunnel_ctx {
 };
 
 static struct tunnel_ctx tc = {
-    .tunnel_hmap = HMAP_INITIALIZER(&tc.tunnel_hmap),
-    .tunnel_hmap_by_uuid = HMAP_INITIALIZER(&tc.tunnel_hmap_by_uuid),
+    .encap_chassis_hmap = HMAP_INITIALIZER(&tc.encap_chassis_hmap),
+    .chassis_hmap = HMAP_INITIALIZER(&tc.chassis_hmap),
     .port_names = SSET_INITIALIZER(&tc.port_names),
 };
 
-static bool process_full_encaps = false;
-
-struct port_hash_node {
-    struct hmap_node node;
-    struct hmap_node uuid_node;
-    struct uuid uuid;
-    const struct ovsrec_port *port;
-    const struct ovsrec_bridge *bridge;
-};
-
-static size_t
-port_hash(const char *chassis_id, const char *type, const char *ip)
-{
-    size_t hash = hash_string(chassis_id, 0);
-    hash = hash_string(type, hash);
-    return hash_string(ip, hash);
-}
-
-static size_t
-port_hash_rec(const struct ovsrec_port *port)
-{
-    const char *chassis_id, *ip;
-    const struct ovsrec_interface *iface;
-
-    chassis_id = smap_get(&port->external_ids, "ovn-chassis-id");
-
-    if (!chassis_id || !port->n_interfaces) {
-        /* This should not happen for an OVN-created port. */
-        return 0;
-    }
-
-    iface = port->interfaces[0];
-    ip = smap_get(&iface->options, "remote_ip");
-    if (!ip) {
-        /* This should not happen for an OVN-created port. */
-        return 0;
-    }
-
-    return port_hash(chassis_id, iface->type, ip);
-}
+/* Iterate over the full set of tunnels in both the OVS and SB databases on
+ * the next wakeup. This is necessary when we add or remove a port in OVS to
+ * handle the case where validation fails. */
+static bool process_full_bridge = false;
 
 static char *
 tunnel_create_name(const char *chassis_id)
@@ -130,79 +92,100 @@ tunnel_create_name(const char *chassis_id)
     return NULL;
 }
 
-static struct port_hash_node *
-port_lookup_by_uuid(struct hmap *hmap_p, const struct uuid *uuid)
+struct encap_hash_node {
+    struct hmap_node node;
+    struct uuid encap_uuid;
+
+    char *chassis_id;
+    struct uuid chassis_uuid;
+};
+
+static struct encap_hash_node *
+lookup_encap_uuid(const struct uuid *uuid)
 {
-    struct port_hash_node *answer;
-    HMAP_FOR_EACH_WITH_HASH (answer, uuid_node, uuid_hash(uuid),
-                             hmap_p) {
-        if (uuid_equals(uuid, &answer->uuid)) {
-            return answer;
+    struct encap_hash_node *hash_node;
+    HMAP_FOR_EACH_WITH_HASH (hash_node, node, uuid_hash(uuid),
+                             &tc.encap_chassis_hmap) {
+        if (uuid_equals(uuid, &hash_node->encap_uuid)) {
+            return hash_node;
         }
     }
     return NULL;
 }
 
-static struct port_hash_node *
-port_lookup_by_port(const struct ovsrec_port *port)
+static void
+insert_encap_uuid(const struct uuid *uuid,
+                  const struct sbrec_chassis *chassis_rec)
 {
-    struct port_hash_node *answer;
-    HMAP_FOR_EACH_WITH_HASH (answer, node, port_hash_rec(port),
-                             &tc.tunnel_hmap) {
-        if (port == answer->port) {
-            return answer;
-        }
-    }
-    return NULL;
+    struct encap_hash_node *hash_node = xmalloc(sizeof *hash_node);
+
+    hash_node->encap_uuid = *uuid;
+    hash_node->chassis_id = xstrdup(chassis_rec->name);
+    hash_node->chassis_uuid = chassis_rec->header_.uuid;
+    hmap_insert(&tc.encap_chassis_hmap, &hash_node->node,
+                uuid_hash(&hash_node->encap_uuid));
 }
 
 static void
-tunnel_add(const struct sbrec_chassis *chassis_rec,
-           const struct sbrec_encap *encap)
+delete_encap_uuid(struct encap_hash_node *encap_hash_node)
 {
-    struct port_hash_node *hash_node;
-    const char *new_chassis_id = chassis_rec->name;
-
-    /* Check whether such a row already exists in OVS. If so, update
-     * the uuid field and insert into the by uuid hashmap. If not,
-     * create the tunnel. */
-
-    HMAP_FOR_EACH_WITH_HASH (hash_node, node,
-                             port_hash(new_chassis_id,
-                                       encap->type, encap->ip),
-                             &tc.tunnel_hmap) {
-        const struct ovsrec_port *port = hash_node->port;
-        const char *chassis_id = smap_get(&port->external_ids,
-                                          "ovn-chassis-id");
-        const struct ovsrec_interface *iface;
-        const char *ip;
-
-        if (!chassis_id || !port->n_interfaces) {
-            continue;
-        }
+    hmap_remove(&tc.encap_chassis_hmap, &encap_hash_node->node);
+    free(encap_hash_node->chassis_id);
+    free(encap_hash_node);
+}
 
-        iface = port->interfaces[0];
-        ip = smap_get(&iface->options, "remote_ip");
-        if (!ip) {
-            continue;
-        }
+struct chassis_hash_node {
+    struct hmap_node node;
+    char *chassis_id;
 
-        if (!strcmp(new_chassis_id, chassis_id)
-            && !strcmp(encap->type, iface->type)
-            && !strcmp(encap->ip, ip)) {
+    char *port_name;
+    struct uuid chassis_uuid;
+    struct uuid port_uuid;
+};
 
-            memcpy(&hash_node->uuid, &chassis_rec->header_.uuid,
-                   sizeof hash_node->uuid);
-            if (!port_lookup_by_uuid(&tc.tunnel_hmap_by_uuid,
-                                     &hash_node->uuid)) {
-                hmap_insert(&tc.tunnel_hmap_by_uuid, &hash_node->uuid_node,
-                            uuid_hash(&hash_node->uuid));
-            }
-            return;
+static struct chassis_hash_node *
+lookup_chassis_id(const char *chassis_id)
+{
+    struct chassis_hash_node *hash_node;
+    HMAP_FOR_EACH_WITH_HASH (hash_node, node, hash_string(chassis_id, 0),
+                             &tc.chassis_hmap) {
+        if (!strcmp(chassis_id, hash_node->chassis_id)) {
+            return hash_node;
         }
     }
+    return NULL;
+}
+
+static void
+insert_chassis_id(const char *chassis_id, const char *port_name,
+                  const struct uuid *chassis_uuid)
+{
+    struct chassis_hash_node *hash_node = xmalloc(sizeof *hash_node);
+
+    hash_node->chassis_id = xstrdup(chassis_id);
+    hash_node->port_name = xstrdup(port_name);
+    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. */
+    uuid_zero(&hash_node->port_uuid);
+    hmap_insert(&tc.chassis_hmap, &hash_node->node, hash_string(chassis_id, 0));
+}
+
+static void
+delete_chassis_id(struct chassis_hash_node *chassis_hash_node)
+{
+    hmap_remove(&tc.chassis_hmap, &chassis_hash_node->node);
+    free(chassis_hash_node->chassis_id);
+    free(chassis_hash_node->port_name);
+    free(chassis_hash_node);
+}
 
-    /* No such port, so add one. */
+static void
+tunnel_add(const struct sbrec_chassis *chassis_rec,
+           const struct sbrec_encap *encap)
+{
+    const char *new_chassis_id = chassis_rec->name;
     struct smap options = SMAP_INITIALIZER(&options);
     struct ovsrec_port *port, **ports;
     struct ovsrec_interface *iface;
@@ -238,6 +221,7 @@ tunnel_add(const struct sbrec_chassis *chassis_rec,
     ovsrec_bridge_verify_ports(tc.br_int);
     ovsrec_bridge_set_ports(tc.br_int, ports, tc.br_int->n_ports + 1);
 
+    insert_chassis_id(new_chassis_id, port_name, &chassis_rec->header_.uuid);
     sset_add(&tc.port_names, port_name);
     free(port_name);
     free(ports);
@@ -245,25 +229,37 @@ tunnel_add(const struct sbrec_chassis *chassis_rec,
     lport_index_reset();
     mcgroup_index_reset();
     lflow_reset_processing();
-    process_full_encaps = true;
+    process_full_bridge = true;
 }
 
 static void
 bridge_delete_port(const struct ovsrec_bridge *br,
-                   const struct ovsrec_port *port)
+                   const struct ovsrec_port *port,
+                   struct chassis_hash_node *chassis_hash_node)
 {
-    struct ovsrec_port **ports;
-    size_t i, n;
+    if (chassis_hash_node) {
+        sset_find_and_delete(&tc.port_names, chassis_hash_node->port_name);
+        delete_chassis_id(chassis_hash_node);
+    }
+
+    if (port) {
+        struct ovsrec_port **ports;
+        size_t i, n;
 
-    ports = xmalloc(sizeof *br->ports * br->n_ports);
-    for (i = n = 0; i < br->n_ports; i++) {
-        if (br->ports[i] != port) {
-            ports[n++] = br->ports[i];
+        ports = xmalloc(sizeof *br->ports * br->n_ports);
+        for (i = n = 0; i < br->n_ports; i++) {
+            if (br->ports[i] != port) {
+                ports[n++] = br->ports[i];
+            }
         }
+        ovsrec_bridge_verify_ports(br);
+        ovsrec_bridge_set_ports(br, ports, n);
+        free(ports);
+
+        binding_reset_processing();
+        lflow_reset_processing();
+        process_full_bridge = true;
     }
-    ovsrec_bridge_verify_ports(br);
-    ovsrec_bridge_set_ports(br, ports, n);
-    free(ports);
 }
 
 static struct sbrec_encap *
@@ -285,9 +281,9 @@ preferred_encap(const struct sbrec_chassis *chassis_rec)
 
 static bool
 check_and_add_tunnel(const struct sbrec_chassis *chassis_rec,
-                     const char *chassis_id)
+                     const char *local_chassis_id)
 {
-    if (strcmp(chassis_rec->name, chassis_id)) {
+    if (strcmp(chassis_rec->name, local_chassis_id)) {
         const struct sbrec_encap *encap = preferred_encap(chassis_rec);
         if (!encap) {
             VLOG_INFO("No supported encaps for '%s'", chassis_rec->name);
@@ -300,140 +296,201 @@ check_and_add_tunnel(const struct sbrec_chassis *chassis_rec,
 }
 
 static void
-check_and_update_tunnel(const struct sbrec_chassis *chassis_rec)
+check_and_update_tunnel(const struct ovsrec_port *port,
+                        const struct sbrec_chassis *chassis_rec)
 {
-    struct port_hash_node *port_node;
-    port_node = port_lookup_by_uuid(&tc.tunnel_hmap_by_uuid,
-                                    &chassis_rec->header_.uuid);
-    if (port_node) {
-        const struct sbrec_encap *encap = preferred_encap(chassis_rec);
-        const struct ovsrec_port *port = port_node->port;
-        const struct ovsrec_interface *iface = port->interfaces[0];
+    const struct sbrec_encap *encap = preferred_encap(chassis_rec);
+    const struct ovsrec_interface *iface = port->interfaces[0];
 
-        if (strcmp(encap->type, iface->type)) {
-            ovsrec_interface_set_type(iface, encap->type);
-        }
-        const char *ip = smap_get(&iface->options, "remote_ip");
-        if (!ip || strcmp(encap->ip, ip)) {
-            struct smap options = SMAP_INITIALIZER(&options);
-            smap_add(&options, "remote_ip", encap->ip);
-            smap_add(&options, "key", "flow");
-            ovsrec_interface_set_options(iface, &options);
-            smap_destroy(&options);
-        }
+    if (strcmp(encap->type, iface->type)) {
+        ovsrec_interface_set_type(iface, encap->type);
+    }
+    const char *ip = smap_get(&iface->options, "remote_ip");
+    if (!ip || strcmp(encap->ip, ip)) {
+        struct smap options = SMAP_INITIALIZER(&options);
+        smap_add(&options, "remote_ip", encap->ip);
+        smap_add(&options, "key", "flow");
+        ovsrec_interface_set_options(iface, &options);
+        smap_destroy(&options);
+    }
 
-        const char *chassis = smap_get_def(&port->external_ids,
-                                           "ovn-chassis-id", "");
-        if (strcmp(chassis_rec->name, chassis)) {
-            const struct smap id = SMAP_CONST1(&id, "ovn-chassis-id",
-                                               chassis_rec->name);
-            ovsrec_port_set_external_ids(port, &id);
-        }
-    } else {
-        /* This tunnel has been lost and shouldn't have been, so
-         * warn the operator of that fact. */
-        VLOG_WARN("Unable to find tunnel for chassis '%s'",
-                  chassis_rec->name);
+    const char *chassis = smap_get_def(&port->external_ids,
+                                       "ovn-chassis-id", "");
+    if (strcmp(chassis_rec->name, chassis)) {
+        const struct smap id = SMAP_CONST1(&id, "ovn-chassis-id",
+                                           chassis_rec->name);
+        ovsrec_port_set_external_ids(port, &id);
     }
 }
 
 void
 encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
-           const char *chassis_id)
+           const char *local_chassis_id)
 {
     if (!ctx->ovs_idl_txn || !br_int) {
         return;
     }
 
     const struct sbrec_chassis *chassis_rec;
-    const struct ovsrec_bridge *br;
+    const struct sbrec_encap *encap_rec;
 
     tc.br_int = br_int;
     tc.ovs_txn = ctx->ovs_idl_txn;
     ovsdb_idl_txn_add_comment(tc.ovs_txn,
                               "ovn-controller: modifying OVS tunnels '%s'",
-                              chassis_id);
+                              local_chassis_id);
 
-    /* Collect all port names into tc.port_names.
+    /* Generally speaking, the changes that we're interested in and commonly
+     * occur happen in the Encap table, so we interate over any changed rows
+     * there. Tunnels are set up based on entries in the Chassis table though,
+     * so it is necessary to maintain a backwards mapping.
      *
-     * Collect all the OVN-created tunnels into tc.tunnel_hmap. */
-    OVSREC_BRIDGE_FOR_EACH(br, ctx->ovs_idl) {
-        size_t i;
+     * In addition to OVN Southbound database changes, it's also possible
+     * that OVS's database can get out of sync. This could happen if a
+     * port is manually modified or one of our previous transactions failed.
+     * If we detect changes to the database and after every attempted
+     * transaction, we iterate over all of the ports currently in the database
+     * as well as the ones we expect to be there and attempt any repairs. We
+     * don't bother to try to do this incrementally since changes are less
+     * common. It would also require more bookkeeping to match up ports and
+     * interfaces. */
+
+    if (process_full_bridge || ovsrec_port_track_get_first(ctx->ovs_idl) ||
+        ovsrec_interface_track_get_first(ctx->ovs_idl)) {
+        const struct ovsrec_port *port_rec;
+        struct chassis_hash_node *chassis_node, *next;
+
+        process_full_bridge = false;
+        sset_clear(&tc.port_names);
+
+        /* Find all of the tunnel ports to remote chassis.
+         * Delete the tunnel ports from 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++) {
+                sset_add(&tc.port_names, port_rec->interfaces[i]->name);
+            }
 
-        for (i = 0; i < br->n_ports; i++) {
-            const struct ovsrec_port *port = br->ports[i];
+            const char *chassis_id = smap_get(&port_rec->external_ids,
+                                              "ovn-chassis-id");
+            if (chassis_id) {
+                chassis_node = lookup_chassis_id(chassis_id);
+                if (chassis_node) {
+                    /* Populate the port's UUID the first time we see it after
+                     * the port was added. */
+                    if (uuid_is_zero(&chassis_node->port_uuid)) {
+                        chassis_node->port_uuid = port_rec->header_.uuid;
+                    }
+                } else {
+                    for (int i = 0; i < port_rec->n_interfaces; i++) {
+                        sset_find_and_delete(&tc.port_names,
+                                             port_rec->interfaces[i]->name);
+                    }
+                    sset_find_and_delete(&tc.port_names, port_rec->name);
+                    bridge_delete_port(tc.br_int, port_rec, NULL);
+                }
+            }
+        }
 
-            sset_add(&tc.port_names, port->name);
+        /* For each chassis that we previously created, check that both the
+         * chassis and port still exist and are current. */
+        HMAP_FOR_EACH_SAFE (chassis_node, next, node, &tc.chassis_hmap) {
+            chassis_rec = sbrec_chassis_get_for_uuid(ctx->ovnsb_idl,
+                                                   &chassis_node->chassis_uuid);
+            port_rec = ovsrec_port_get_for_uuid(ctx->ovs_idl,
+                                                &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;
+            }
 
-            const char *chassis_id = smap_get(&port->external_ids,
-                                              "ovn-chassis-id");
-            if (chassis_id && !port_lookup_by_port(port)) {
-                struct port_hash_node *hash_node =
-                    xzalloc(sizeof *hash_node);
-                hash_node->bridge = br;
-                hash_node->port = port;
-                hmap_insert(&tc.tunnel_hmap, &hash_node->node,
-                            port_hash_rec(port));
-                process_full_encaps = true;
+            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);
             }
         }
     }
 
-    if (process_full_encaps) {
-        /* Create tunnels to the other chassis. */
-        struct hmap keep_tunnel_hmap_by_uuid =
-            HMAP_INITIALIZER(&keep_tunnel_hmap_by_uuid);
-        SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) {
-            check_and_add_tunnel(chassis_rec, chassis_id);
-            struct port_hash_node *hash_node = xzalloc(sizeof *hash_node);
-            memcpy(&hash_node->uuid, &chassis_rec->header_.uuid,
-                   sizeof hash_node->uuid);
-            hmap_insert(&keep_tunnel_hmap_by_uuid, &hash_node->uuid_node,
-                        uuid_hash(&hash_node->uuid));
+    /* Maintain a mapping backwards from encap entries to their parent
+     * chassis. Most changes happen at the encap row entry but tunnels need
+     * to be established on the basis of the overall chassis. */
+    SBREC_CHASSIS_FOR_EACH_TRACKED (chassis_rec, ctx->ovnsb_idl) {
+        /* Defer deletion of mapping until we have cleaned up associated
+         * ports. */
+        if (!sbrec_chassis_is_deleted(chassis_rec)) {
+            for (int i = 0; i < chassis_rec->n_encaps; i++) {
+                encap_rec = chassis_rec->encaps[i];
+
+                struct encap_hash_node *encap_hash_node;
+                encap_hash_node = lookup_encap_uuid(&encap_rec->header_.uuid);
+                if (encap_hash_node) {
+                    /* A change might have invalidated our mapping. Process the
+                     * new version and then iterate over everything to see if it
+                     * is OK. */
+                    delete_encap_uuid(encap_hash_node);
+                    process_full_bridge = true;
+                    poll_immediate_wake();
+                }
+
+                insert_encap_uuid(&encap_rec->header_.uuid, chassis_rec);
+            }
         }
+    }
 
-        /* Delete any tunnels that weren't recreated above. */
-        struct port_hash_node *old_hash_node, *next_hash_node;
-        HMAP_FOR_EACH_SAFE (old_hash_node, next_hash_node,
-                            node, &tc.tunnel_hmap) {
-            if (!uuid_is_zero(&old_hash_node->uuid)
-                && !port_lookup_by_uuid(&keep_tunnel_hmap_by_uuid,
-                                        &old_hash_node->uuid)) {
-                bridge_delete_port(old_hash_node->bridge, old_hash_node->port);
-                sset_find_and_delete(&tc.port_names,
-                                     old_hash_node->port->name);
-                hmap_remove(&tc.tunnel_hmap, &old_hash_node->node);
-                hmap_remove(&tc.tunnel_hmap_by_uuid,
-                            &old_hash_node->uuid_node);
-                free(old_hash_node);
+    /* Update tunnels based on changes to the Encap table. Once we've detected
+     * a change to an encap, we map it back to the parent chassis and add or
+     * update the required tunnel. That means that a given changed encap row
+     * might actually result in the creation of a different type tunnel if
+     * that type is preferred. That's OK - when we process the other encap
+     * rows, we'll just skip over the new tunnels. */
+    SBREC_ENCAP_FOR_EACH_TRACKED (encap_rec, ctx->ovnsb_idl) {
+        struct encap_hash_node *encap_hash_node;
+        struct chassis_hash_node *chassis_hash_node;
+        const struct ovsrec_port *port_rec = NULL;
+
+        encap_hash_node = lookup_encap_uuid(&encap_rec->header_.uuid);
+        if (!encap_hash_node) {
+            continue;
+        }
+        chassis_rec = sbrec_chassis_get_for_uuid(ctx->ovnsb_idl,
+                                                &encap_hash_node->chassis_uuid);
+
+        chassis_hash_node = lookup_chassis_id(encap_hash_node->chassis_id);
+        if (chassis_hash_node) {
+            /* If the UUID is zero it means that we just added the chassis on
+             * this spin through encaps_run() - perhaps there are two possible
+             * encaps. Presumably the information we had then is the same as
+             * we have now, so just skip this. */
+            if (uuid_is_zero(&chassis_hash_node->port_uuid)) {
+                continue;
             }
+
+            port_rec = ovsrec_port_get_for_uuid(ctx->ovs_idl,
+                                                &chassis_hash_node->port_uuid);
         }
-        hmap_destroy(&keep_tunnel_hmap_by_uuid);
-        process_full_encaps = false;
-    } else {
-        SBREC_CHASSIS_FOR_EACH_TRACKED (chassis_rec, ctx->ovnsb_idl) {
-            if (sbrec_chassis_is_deleted(chassis_rec)) {
-                /* Lookup the tunnel by row uuid and remove it. */
-                struct port_hash_node *port_hash =
-                    port_lookup_by_uuid(&tc.tunnel_hmap_by_uuid,
-                                        &chassis_rec->header_.uuid);
-                if (port_hash) {
-                    bridge_delete_port(port_hash->bridge, port_hash->port);
-                    sset_find_and_delete(&tc.port_names,
-                                         port_hash->port->name);
-                    hmap_remove(&tc.tunnel_hmap, &port_hash->node);
-                    hmap_remove(&tc.tunnel_hmap_by_uuid,
-                                &port_hash->uuid_node);
-                    free(port_hash);
-                    binding_reset_processing();
-                    lflow_reset_processing();
-                }
-            } else if (sbrec_chassis_is_new(chassis_rec)) {
-                check_and_add_tunnel(chassis_rec, chassis_id);
+
+        /* Create, update and delete the actual tunnel ports as necessary. */
+        if (!port_rec) {
+            if (chassis_rec) {
+                check_and_add_tunnel(chassis_rec, local_chassis_id);
+            }
+        } else {
+            if (chassis_rec) {
+                check_and_update_tunnel(port_rec, chassis_rec);
             } else {
-                check_and_update_tunnel(chassis_rec);
+                bridge_delete_port(tc.br_int, port_rec, chassis_hash_node);
             }
         }
+
+        if (!chassis_rec) {
+            delete_encap_uuid(encap_hash_node);
+        }
     }
 }
 
index 4694a31e3afed837bffa6f38a8e3ef2bae23cc2d..66a364fd099d67938213587a78237d7fde72e9cb 100644 (file)
@@ -482,6 +482,7 @@ main(int argc, char *argv[])
         ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
         ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
         ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
+        ovsdb_idl_track_clear(ovs_idl_loop.idl);
         poll_block();
         if (should_service_stop()) {
             exiting = true;
index af64804e03fac2c439ed9fb2706997a28a86c76a..983f676d2a26451f6609937e74673c528c70898f 100644 (file)
@@ -216,3 +216,51 @@ as ovn-sb
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 
 AT_CLEANUP
+
+# Checks that ovn-controller correctly maintains the mapping from the Encap
+# table in the Southbound database to OVS in the face of changes on both sides
+AT_SETUP([ovn-controller - change Encap properties])
+AT_KEYWORDS([ovn])
+ovn_init_db ovn-sb
+
+net_add n1
+sim_add hv
+as hv
+ovs-vsctl \
+    -- add-br br-phys \
+    -- add-br br-eth0 \
+    -- add-br br-eth1 \
+    -- add-br br-eth2
+ovn_attach n1 br-phys 192.168.0.1
+
+check_tunnel_property () {
+    test "`ovs-vsctl get interface ovn-fakech-0 $1`" = "$2"
+}
+
+# Start off with a remote chassis supporting STT
+ovn-sbctl chassis-add fakechassis stt 192.168.0.2
+OVS_WAIT_UNTIL([check_tunnel_property type stt])
+
+# See if we switch to Geneve as the first choice when it is available
+encap_uuid=$(ovn-sbctl add chassis fakechassis encaps @encap -- --id=@encap create encap type=geneve ip="127.0.0.1")
+OVS_WAIT_UNTIL([check_tunnel_property type geneve])
+
+# Check that changes within an encap row are propagated
+ovn-sbctl set encap ${encap_uuid} ip=192.168.0.2
+OVS_WAIT_UNTIL([check_tunnel_property options:remote_ip "\"192.168.0.2\""])
+
+# Change the type on the OVS side and check than OVN fixes it
+ovs-vsctl set interface ovn-fakech-0 type=vxlan
+OVS_WAIT_UNTIL([check_tunnel_property type geneve])
+
+# Delete the port entirely and it should be resurrected
+ovs-vsctl del-port ovn-fakech-0
+OVS_WAIT_UNTIL([check_tunnel_property type geneve])
+
+# Gracefully terminate daemons
+OVN_CLEANUP_SBOX([hv])
+OVN_CLEANUP_VSWITCH([main])
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+AT_CLEANUP