[ovs-dev] [PATCH v4] ovn-controller: Back out incremental processing
Ryan Moats
rmoats at us.ibm.com
Thu Aug 25 23:58:37 UTC 2016
Guru Shetty <guru at ovn.org> wrote on 08/25/2016 03:11:42 PM:
> From: Guru Shetty <guru at ovn.org>
> To: Ryan Moats/Omaha/IBM at IBMUS
> Cc: ovs dev <dev at openvswitch.org>
> Date: 08/25/2016 03:11 PM
> Subject: Re: [ovs-dev] [PATCH v4] ovn-controller: Back out
> incremental processing
>
> 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
isidentical
> * 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>)
> ])
Thanks, I'll roll these into v5 and post later tonight/tomorrow morning...
More information about the dev
mailing list