[ovs-dev,v4] ovn-controller: Back out incremental processing
diff mbox

Message ID CAM_3v9KfWYx17YTTuA3oag7+AOA0b=FqXKCWgzFTjkLPUx+yNg@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Guru Shetty Aug. 25, 2016, 8:04 p.m. UTC
On 24 August 2016 at 18:30, Ryan Moats <rmoats@us.ibm.com> wrote:

> As [1] indicates, incremental processing hasn't resulted
> in an improvement worth the complexity it has added.
> This patch backs out all of the code specific to incremental
> processing, along with the persisting of OF flows,
> logical ports, multicast groups, all_lports, local and patched
> datapaths
>
> [1] http://openvswitch.org/pipermail/dev/2016-August/078272.html
>
> Signed-off-by: Ryan Moats <rmoats@us.ibm.com>
> Co-authored-by: Guru Shetty <guru@ovn.com>
>
I think you should remove the co-authored-by. I just gave a very silly test
that I don't think warrants a co-authored-by.

I think you should add the following incremental to the test to make it a
little better.

dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Patch
diff mbox

diff --git a/tests/ovn.at b/tests/ovn.at
index 30411d8..c161d07 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5015,7 +5015,7 @@  for i in `seq 1 3`; do
     -- lsp-set-addresses bar$i "f0:00:0a:01:02:$i 172.16.1.$ip"
 done

-OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int table=0 | grep REG13])
+OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int table=0 | grep REG13 |
wc -l` -eq 4])

 OVN_CLEANUP([hv1])


As expected, the load-balancer tests now fail. But in addition to the zone
allocation, there was another issue which was added as part of incremental
processing fixes that was causing consistent test failures. The following
incremental reverts it back to how it was before all the incremental
processing changes (There is still a corner case, but I don't think it was
caused by incremental processing, so I will send a separate fix for that).

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 9fc8a93..647b4f0 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -253,8 +253,6 @@  add_logical_flows(struct controller_ctx *ctx, const
struct lport_index *lports,
     uint32_t conj_id_ofs = 1;
     const struct sbrec_logical_flow *lflow;

-    ovn_group_table_clear(group_table, false);
-
     struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
     struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
     const struct sbrec_dhcp_options *dhcp_opt_row;
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 5c3125a..d8e111d 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -708,16 +708,6 @@  add_flow_mod(struct ofputil_flow_mod *fm, struct
ovs_list *msgs)
 ^L
 /* group_table. */

-static struct group_info *
-group_info_clone(struct group_info *source)
-{
-    struct group_info *clone = xmalloc(sizeof *clone);
-    ds_clone(&clone->group, &source->group);
-    clone->group_id = source->group_id;
-    clone->hmap_node.hash = source->hmap_node.hash;
-    return clone;
-}
-
 /* Finds and returns a group_info in 'existing_groups' whose key is
identical
  * to 'target''s key, or NULL if there is none. */
 static struct group_info *
@@ -770,7 +760,10 @@  add_group_mod(const struct ofputil_group_mod *gm,
struct ovs_list *msgs)
  * with ofctrl_add_flow().
  *
  * Replaces the group table on the switch, if possible, by the contents of
- * 'groups->desired_groups'.
+ * 'groups->desired_groups'.  Regardless of whether the group table
+ * is updated, this deletes all the groups from the
+ * 'groups->desired_groups' and frees them. (The hmap itself isn't
+ * destroyed.)
  *
  * This should be called after ofctrl_run() within the main loop. */
 void
@@ -929,10 +922,13 @@  ofctrl_put(struct hmap *flow_table, int64_t nb_cfg)
     /* Move the contents of desired_groups to existing_groups. */
     HMAP_FOR_EACH_SAFE(desired, next_group, hmap_node,
                        &groups->desired_groups) {
+        hmap_remove(&groups->desired_groups, &desired->hmap_node);
         if (!ovn_group_lookup(&groups->existing_groups, desired)) {
-            struct group_info *clone = group_info_clone(desired);
-            hmap_insert(&groups->existing_groups, &clone->hmap_node,
-                        clone->hmap_node.hash);
+            hmap_insert(&groups->existing_groups, &desired->hmap_node,
+                        desired->hmap_node.hash);
+        } else {
+           ds_destroy(&desired->group);
+           free(desired);
         }
     }
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index e267384..b9da189 100755
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -373,10 +373,11 @@  for i in `seq 1 20`; do
 done

 dnl Each server should have at least one connection.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1)], [0], [dnl
-tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.2,dst=192.168.1.2,sport=<cleared>,dport=<c
-tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.3,dst=192.168.1.2,sport=<cleared>,dport=<c
-tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.4,dst=192.168.1.2,sport=<cleared>,dport=<c
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.2,dst=192.168.1.2,sport=<cleared>,dport=<c
+tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.3,dst=192.168.1.2,sport=<cleared>,dport=<c
+tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.4,dst=192.168.1.2,sport=<cleared>,dport=<c
 ])

 dnl Should work with the virtual IP 30.0.0.3 address through NAT
@@ -386,10 +387,11 @@  for i in `seq 1 20`; do
 done

 dnl Each server should have at least one connection.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.3)], [0], [dnl
-tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.2,dst=192.168.1.2,sport=<cleared>,dport=<c
-tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.3,dst=192.168.1.2,sport=<cleared>,dport=<c
-tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.4,dst=192.168.1.2,sport=<cleared>,dport=<c
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.3) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.2,dst=192.168.1.2,sport=<cleared>,dport=<c
+tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.3,dst=192.168.1.2,sport=<cleared>,dport=<c
+tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.4,dst=192.168.1.2,sport=<cleared>,dport=<c
 ])

 dnl Test load-balancing that includes L4 ports in NAT.
@@ -399,10 +401,11 @@  for i in `seq 1 20`; do
 done

 dnl Each server should have at least one connection.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.2)], [0], [dnl
-tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.2,dst=192.168.1.2,sport=<cleared>,dport=<c
-tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.3,dst=192.168.1.2,sport=<cleared>,dport=<c
-tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.4,dst=192.168.1.2,sport=<cleared>,dport=<c
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.2) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.2,dst=192.168.1.2,sport=<cleared>,dport=<c
+tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.3,dst=192.168.1.2,sport=<cleared>,dport=<c
+tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.4,dst=192.168.1.2,sport=<cleared>,dport=<c
 ])


@@ -489,10 +492,11 @@  for i in `seq 1 20`; do
 done

 dnl Each server should have at least one connection.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1)], [0], [dnl
-tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.3,dst=192.168.1.2,sport=<cleared>,dport=<
-tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.4,dst=192.168.1.2,sport=<cleared>,dport=<
-tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.5,dst=192.168.1.2,sport=<cleared>,dport=<
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.3,dst=192.168.1.2,sport=<cleared>,dport=<
+tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.4,dst=192.168.1.2,sport=<cleared>,dport=<
+tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.5,dst=192.168.1.2,sport=<cleared>,dport=<
 ])

 dnl Test load-balancing that includes L4 ports in NAT.
@@ -502,10 +506,11 @@  for i in `seq 1 20`; do
 done

 dnl Each server should have at least one connection.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.2)], [0], [dnl
-tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.3,dst=192.168.1.2,sport=<cleared>,dport=<
-tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.4,dst=192.168.1.2,sport=<cleared>,dport=<
-tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.5,dst=192.168.1.2,sport=<cleared>,dport=<
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.2) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.3,dst=192.168.1.2,sport=<cleared>,dport=<
+tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.4,dst=192.168.1.2,sport=<cleared>,dport=<
+tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.5,dst=192.168.1.2,sport=<cleared>,dport=<
 ])
_______________________________________________
dev mailing list