[ovs-dev] [PATCH v4] ovn-controller: Back out incremental processing
Guru Shetty
guru at ovn.org
Thu Aug 25 20:04:10 UTC 2016
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).
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=<
])
More information about the dev
mailing list