On 25 August 2016 at 13:04, Guru Shetty <g...@ovn.org> wrote:
>
>
> On 24 August 2016 at 18:30, Ryan Moats <rmo...@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 <rmo...@us.ibm.com>
>> Co-authored-by: Guru Shetty <g...@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>)
])
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
