[ovs-dev,v2] ovn-controller: Change strategy for gateway conntrack zone allocation.
diff mbox

Message ID 1467977223-20158-1-git-send-email-guru@ovn.org
State Accepted
Headers show

Commit Message

Gurucharan Shetty July 8, 2016, 11:27 a.m. UTC
Commit 263064aeaa31e7 (Convert binding_run to incremental processing.)
changed the way patched_datapaths were handled. Previously we would
destroy the datastructure in every run and re-create it fresh. The new
way causes problems with the way conntrack zones are allocated as now
we can have stale port_binding entries causing segmentation faults.

With this commit, we simply don't depend on port_binding records in
conntrack zone allocation and instead store the UUID as a string in
the patch_datapath datastructure.

(The test enhanced with this commit would fail without the changes
in the commit. i.e. ovn-controller would crash. )

Signed-off-by: Gurucharan Shetty <guru@ovn.org>
---
This applies on top of https://patchwork.ozlabs.org/patch/646268/.
I will rebase this if the dependent patch changes.
---
 ovn/controller/ovn-controller.c |  4 ++--
 ovn/controller/ovn-controller.h |  2 +-
 ovn/controller/patch.c          |  4 +++-
 ovn/controller/physical.c       |  8 ++++++--
 ovn/lib/ovn-util.c              |  8 +++-----
 ovn/lib/ovn-util.h              |  3 +--
 tests/ovn.at                    | 21 +++++++++++++++++++++
 7 files changed, 37 insertions(+), 13 deletions(-)

Comments

Ryan Moats July 8, 2016, 11:10 p.m. UTC | #1
"dev" <dev-bounces@openvswitch.org> wrote on 07/08/2016 06:27:03 AM:

> From: Gurucharan Shetty <guru@ovn.org>
> To: dev@openvswitch.org
> Date: 07/08/2016 04:25 PM
> Subject: [ovs-dev] [PATCH v2] ovn-controller: Change strategy for
> gateway conntrack zone allocation.
> Sent by: "dev" <dev-bounces@openvswitch.org>
>
> Commit 263064aeaa31e7 (Convert binding_run to incremental processing.)
> changed the way patched_datapaths were handled. Previously we would
> destroy the datastructure in every run and re-create it fresh. The new
> way causes problems with the way conntrack zones are allocated as now
> we can have stale port_binding entries causing segmentation faults.
>
> With this commit, we simply don't depend on port_binding records in
> conntrack zone allocation and instead store the UUID as a string in
> the patch_datapath datastructure.
>
> (The test enhanced with this commit would fail without the changes
> in the commit. i.e. ovn-controller would crash. )
>
> Signed-off-by: Gurucharan Shetty <guru@ovn.org>
> ---

I've tried this out and verified that the enhanced test does fail/crash
w/o the patch and passes w/ it so...

Acked-by: Ryan Moats <rmoats@us.ibm.com>
Gurucharan Shetty July 9, 2016, 4:42 p.m. UTC | #2
On 8 July 2016 at 16:10, Ryan Moats <rmoats@us.ibm.com> wrote:

> "dev" <dev-bounces@openvswitch.org> wrote on 07/08/2016 06:27:03 AM:
>
> > From: Gurucharan Shetty <guru@ovn.org>
> > To: dev@openvswitch.org
> > Date: 07/08/2016 04:25 PM
> > Subject: [ovs-dev] [PATCH v2] ovn-controller: Change strategy for
> > gateway conntrack zone allocation.
> > Sent by: "dev" <dev-bounces@openvswitch.org>
> >
> > Commit 263064aeaa31e7 (Convert binding_run to incremental processing.)
> > changed the way patched_datapaths were handled. Previously we would
> > destroy the datastructure in every run and re-create it fresh. The new
> > way causes problems with the way conntrack zones are allocated as now
> > we can have stale port_binding entries causing segmentation faults.
> >
> > With this commit, we simply don't depend on port_binding records in
> > conntrack zone allocation and instead store the UUID as a string in
> > the patch_datapath datastructure.
> >
> > (The test enhanced with this commit would fail without the changes
> > in the commit. i.e. ovn-controller would crash. )
> >
> > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
> > ---
>
> I've tried this out and verified that the enhanced test does fail/crash
> w/o the patch and passes w/ it so...
>
> Acked-by: Ryan Moats <rmoats@us.ibm.com>
>
Thank you, applied!

Patch
diff mbox

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 8471f64..28ee13e 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -251,8 +251,8 @@  update_ct_zones(struct sset *lports, struct hmap *patched_datapaths,
             continue;
         }
 
-        char *dnat = alloc_nat_zone_key(pd->port_binding, "dnat");
-        char *snat = alloc_nat_zone_key(pd->port_binding, "snat");
+        char *dnat = alloc_nat_zone_key(pd->key, "dnat");
+        char *snat = alloc_nat_zone_key(pd->key, "snat");
         sset_add(&all_users, dnat);
         sset_add(&all_users, snat);
         free(dnat);
diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h
index f3edc43..470b1f5 100644
--- a/ovn/controller/ovn-controller.h
+++ b/ovn/controller/ovn-controller.h
@@ -51,7 +51,7 @@  struct local_datapath *get_local_datapath(const struct hmap *,
  * with at least one logical patch port binding. */
 struct patched_datapath {
     struct hmap_node hmap_node;
-    const struct sbrec_port_binding *port_binding;
+    char *key;  /* Holds the uuid of the corresponding datapath. */
     bool local; /* 'True' if the datapath is for gateway router. */
     bool stale; /* 'True' if the datapath is not referenced by any patch
                  * port. */
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 3825f31..9a5c96f 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -263,7 +263,8 @@  add_patched_datapath(struct hmap *patched_datapaths,
 
     pd = xzalloc(sizeof *pd);
     pd->local = local;
-    pd->port_binding = binding_rec;
+    pd->key = xasprintf(UUID_FMT,
+                        UUID_ARGS(&binding_rec->datapath->header_.uuid));
     /* stale is set to false. */
     hmap_insert(patched_datapaths, &pd->hmap_node,
                 binding_rec->datapath->tunnel_key);
@@ -291,6 +292,7 @@  add_logical_patch_ports_postprocess(struct hmap *patched_datapaths)
                         patched_datapaths) {
         if (pd_cur_node->stale == true) {
             hmap_remove(patched_datapaths, &pd_cur_node->hmap_node);
+            free(pd_cur_node->key);
             free(pd_cur_node);
         }
     }
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index f7389ea..d1b40c2 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -297,8 +297,12 @@  consider_port_binding(struct hmap *flow_table,
         }
 
         int zone_id_dnat, zone_id_snat;
-        char *dnat = alloc_nat_zone_key(binding, "dnat");
-        char *snat = alloc_nat_zone_key(binding, "snat");
+        char *key = xasprintf(UUID_FMT,
+                              UUID_ARGS(&binding->datapath->header_.uuid));
+        char *dnat = alloc_nat_zone_key(key, "dnat");
+        char *snat = alloc_nat_zone_key(key, "snat");
+        free(key);
+
         zone_id_dnat = simap_get(ct_zones, dnat);
         if (zone_id_dnat) {
             put_load(zone_id_dnat, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c
index 6113087..6e94083 100644
--- a/ovn/lib/ovn-util.c
+++ b/ovn/lib/ovn-util.c
@@ -105,13 +105,11 @@  extract_lsp_addresses(char *address, struct lport_addresses *laddrs,
 }
 
 /* Allocates a key for NAT conntrack zone allocation for a provided
- * 'port_binding' record and a 'type'.
+ * 'key' record and a 'type'.
  *
  * It is the caller's responsibility to free the allocated memory. */
 char *
-alloc_nat_zone_key(const struct sbrec_port_binding *port_binding,
-                   const char *type)
+alloc_nat_zone_key(const char *key, const char *type)
 {
-    return xasprintf(UUID_FMT"_%s",
-                     UUID_ARGS(&port_binding->datapath->header_.uuid), type);
+    return xasprintf("%s_%s", key, type);
 }
diff --git a/ovn/lib/ovn-util.h b/ovn/lib/ovn-util.h
index d23f4af..98b1426 100644
--- a/ovn/lib/ovn-util.h
+++ b/ovn/lib/ovn-util.h
@@ -43,6 +43,5 @@  extract_lsp_addresses(char *address, struct lport_addresses *laddrs,
                       bool store_ipv6);
 
 char *
-alloc_nat_zone_key(const struct sbrec_port_binding *port_binding,
-                   const char *type);
+alloc_nat_zone_key(const char *key, const char *type);
 #endif
diff --git a/tests/ovn.at b/tests/ovn.at
index 3f2e779..a1c7af0 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3085,6 +3085,27 @@  $PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv2/vif1-tx.pcap | trim_zeros > rece
 echo $expected | trim_zeros > expout
 AT_CHECK([cat received1.packets], [0], [expout])
 
+# Delete the router and re-create it. Things should work as before.
+ovn-nbctl  lr-del R2
+ovn-nbctl create Logical_Router name=R2 options:chassis="hv2"
+# Connect alice to R2
+ovn-nbctl lrp-add R2 alice 00:00:02:01:02:03 172.16.1.1/24
+# Connect R2 to join
+ovn-nbctl lrp-add R2 R2_join 00:00:04:01:02:04 20.0.0.2/24
+
+ovn-nbctl -- --id=@lrt create Logical_Router_Static_Route \
+ip_prefix=192.168.1.0/24 nexthop=20.0.0.1 -- add Logical_Router \
+R2 static_routes @lrt
+
+# Wait for ovn-controller to catch up.
+sleep 1
+
+# Send the packet again.
+as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet
+$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv2/vif1-tx.pcap | trim_zeros > received1.packets
+echo $expected | trim_zeros >> expout
+AT_CHECK([cat received1.packets], [0], [expout])
+
 OVN_CLEANUP([hv1],[hv2])
 
 AT_CLEANUP