[ovs-dev] [PATCH v4] ovn-controller: Back out incremental processing

Guru Shetty guru at ovn.org
Thu Aug 25 20:11:42 UTC 2016


On 25 August 2016 at 13:04, Guru Shetty <guru at ovn.org> wrote:

>
>
> On 24 August 2016 at 18:30, Ryan Moats <rmoats at 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 at us.ibm.com>
>> Co-authored-by: Guru Shetty <guru at 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.
>
> 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).
>


Ignore the incremental below this in the previous mail. It got cut. I am
pasting a fresh one here:

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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
 ])

 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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
])

 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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
 ])


@@ -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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
 ])

 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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
-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=<cleared>),protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
+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=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>)
 ])



More information about the dev mailing list