diff mbox series

[ovs-dev,v1] ovn: Add sanity check when adding tunnel port

Message ID 20180108113622.14686-1-sysugaozhenyu@gmail.com
State Changes Requested
Headers show
Series [ovs-dev,v1] ovn: Add sanity check when adding tunnel port | expand

Commit Message

Gao Zhenyu Jan. 8, 2018, 11:36 a.m. UTC 
1. ovs cannot create two ports with same tunnel type and options:remote_ip
2. add santity check to detect if two chassises have same encap ip
3. add a sset to store tunnel ports' type & encap ip information

Signed-off-by: Zhenyu Gao <sysugaozhenyu@gmail.com>
---
 ovn/controller/encaps.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Ben Pfaff Jan. 8, 2018, 7:10 p.m. UTC | #1 
On Mon, Jan 08, 2018 at 11:36:22AM +0000, Zhenyu Gao wrote:
> 1. ovs cannot create two ports with same tunnel type and options:remote_ip
> 2. add santity check to detect if two chassises have same encap ip
> 3. add a sset to store tunnel ports' type & encap ip information
> 
> Signed-off-by: Zhenyu Gao <sysugaozhenyu@gmail.com>

Thanks for working on improving OVN.

This code leaks memory because it never destroys tunnel_type_ips.

I am unsure whether this handles changes properly.  Deletions happen
only at the end of encaps_run().  Insertions happen earlier.  What would
happen if a pass through encaps_run() was supposed to swap two tunnels
that would otherwise conflict?

Thanks,

Ben.
Gao Zhenyu Jan. 15, 2018, 3:38 a.m. UTC | #2 
Thanks for the review! The swapping may break it. I would try to revise it.

Thanks
Zhenyu Gao

2018-01-09 3:10 GMT+08:00 Ben Pfaff <blp@ovn.org>:

> On Mon, Jan 08, 2018 at 11:36:22AM +0000, Zhenyu Gao wrote:
> > 1. ovs cannot create two ports with same tunnel type and
> options:remote_ip
> > 2. add santity check to detect if two chassises have same encap ip
> > 3. add a sset to store tunnel ports' type & encap ip information
> >
> > Signed-off-by: Zhenyu Gao <sysugaozhenyu@gmail.com>
>
> Thanks for working on improving OVN.
>
> This code leaks memory because it never destroys tunnel_type_ips.
>
> I am unsure whether this handles changes properly.  Deletions happen
> only at the end of encaps_run().  Insertions happen earlier.  What would
> happen if a pass through encaps_run() was supposed to swap two tunnels
> that would otherwise conflict?
>
> Thanks,
>
> Ben.
>
diff mbox series

Patch

diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index f187a8f..06bffe8 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -50,6 +50,10 @@  struct tunnel_ctx {
      * adding a new tunnel. */
     struct sset port_names;
 
+    /* IP,type of all tunnel ports in bridge, to allow checking uniqueness
+     * when adding a new tunnel. */
+    struct sset tunnel_type_ips;
+
     struct ovsdb_idl_txn *ovs_txn;
     const struct ovsrec_bridge *br_int;
 };
@@ -78,6 +82,19 @@  tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id)
     return NULL;
 }
 
+static char*
+create_tunnel_type_ip_key(struct tunnel_ctx *tc, const char *type,
+                          const char *ip)
+{
+    char *tunnel_type_ip = xasprintf("%s-%s", type, ip);
+    if (!sset_contains(&tc->tunnel_type_ips, tunnel_type_ip)) {
+        return tunnel_type_ip;
+    }
+
+    free(tunnel_type_ip);
+    return NULL;
+}
+
 static void
 tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id,
            const struct sbrec_encap *encap)
@@ -116,6 +133,15 @@  tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id,
         goto exit;
     }
 
+    char *tunnel_type_ip = create_tunnel_type_ip_key(tc, encap->type,
+                                                     encap->ip);
+    if (!tunnel_type_ip) {
+        VLOG_WARN("Unable to allocate unique remote_ip for '%s' %s tunnel",
+                  encap->type, new_chassis_id);
+        free(port_name);
+        goto exit;
+    }
+
     struct ovsrec_interface *iface = ovsrec_interface_insert(tc->ovs_txn);
     ovsrec_interface_set_name(iface, port_name);
     ovsrec_interface_set_type(iface, encap->type);
@@ -130,6 +156,7 @@  tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id,
     ovsrec_bridge_update_ports_addvalue(tc->br_int, port);
 
     sset_add_and_free(&tc->port_names, port_name);
+    sset_add_and_free(&tc->tunnel_type_ips, tunnel_type_ip);
 
 exit:
     smap_destroy(&options);
@@ -166,6 +193,7 @@  encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
     struct tunnel_ctx tc = {
         .chassis = SHASH_INITIALIZER(&tc.chassis),
         .port_names = SSET_INITIALIZER(&tc.port_names),
+        .tunnel_type_ips = SSET_INITIALIZER(&tc.tunnel_type_ips),
         .br_int = br_int
     };
 
@@ -194,6 +222,24 @@  encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
                      * to delete this one. */
                     ovsrec_bridge_update_ports_delvalue(br, port);
                 }
+                for (size_t i = 0; i< port->n_interfaces; i++) {
+                    const struct ovsrec_interface *iface = port->interfaces[i];
+                    if (strcmp(iface->type, "geneve")
+                        && strcmp(iface->type, "stt")
+                        && strcmp(iface->type, "vxlan")) {
+                        continue;
+                    }
+                    const char *if_remote_ip = smap_get(&iface->options,
+                                                        "remote_ip");
+                    char *tunnel_type_ip;
+                    tunnel_type_ip = create_tunnel_type_ip_key(&tc,
+                                                               iface->type,
+                                                               if_remote_ip);
+                    if (!tunnel_type_ip) {
+                        continue;
+                    }
+                    sset_add_and_free(&tc.tunnel_type_ips, tunnel_type_ip);
+                }
             }
         }
     }