[ovs-dev] [PATCH ovn] ofctrl: Don't assert while merging tracked flows.

Han Zhou hzhou at ovn.org
Wed Apr 28 07:31:18 UTC 2021


Thanks Numan for working on this issue!

On Mon, Apr 26, 2021 at 3:53 AM <numans at ovn.org> wrote:
>
> From: Numan Siddique <numans at ovn.org>
>
> Presently we do ovs_assert(del_f->installed_flows) in
> merged_tracked_flows() of ofctrl.c if 'del_f' and flow 'f' are equal
> and if 'del_f' is not installed.  But there are a couple of scenarios
> this can happen:
>
> 1. A flow 'F1' was added to the desired flows, but ofctrl_put() couldn't
> install the flow (because of the rconn queue is full) and an SB update
> caused 'F1' to be removed and re-added as 'F2'.
>

This seems not possible, because del_f->installed_flow is set whenever a
"installed_flow" is added to the installed_flows table, regardless of
whether it is sent to OVS or not.

> 2. In a single engine run, a flow 'F1' was added to the desired flow,
> removed from the desired flow and re-added as 'F2'.  This could
> happen after the commit [1].

In theory this should not happen either, because if F1 was added and then
removed before it was installed, it would have been removed in
track_flow_del():

        if (!f->installed_flow) {
            /* If it is not installed yet, simply destroy it. */
            desired_flow_destroy(f);
            return;
        }

In fact this is what the comment is supposed to say, but the comment had a
typo (my bad):

            /* del_f must have been installed, otherwise it should have been
            * removed during track_flow_add_or_modify. */

s/track_flow_add_or_modify/track_flow_del

In practice, if commit [1] did trigger this bt, then I'd look deeper into
the logic, but I think removing the assert may just hide the problem.

>
> The likelihood of (2) seems to be higher with datapath groups enabled.
>
> The assertion - ovs_assert(del_f->installed_flows) is observed in
> few deployments after the commit [1].  This patch addressed this issue
> by removing that assertion.  Removing this assertion should not have
> any side effects as the flow 'F2' will be installed.
>
> This patch is proposed based on the code inspection and the possibility
> that the above mentioned scenarios can happen.  The patch doesn't have
> any test cases to reproduce these scenarios.
>
> [1] - c6c61b4e3462("ofctrl: Fix the assert seen when flood removing flows
with conj actions.")
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1951502
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
>  controller/ofctrl.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index c29c3d180..9e960b034 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -1931,11 +1931,29 @@ merge_tracked_flows(struct ovn_desired_flow_table
*flow_table)
>                  continue;
>              }
>
> -            /* del_f must have been installed, otherwise it should have
been
> -             * removed during track_flow_add_or_modify. */
> -            ovs_assert(del_f->installed_flow);
> +            if (!del_f->installed_flow) {
> +                /* del_f must have been installed, otherwise it should
have
> +                 * been removed during track_flow_add_or_modify.
> +                 *
> +                 * But however there are a couple of scenarios this may
not
> +                 * happen.
> +                 *
> +                 * Scenario 1:  A flow was added to the desired flows,
but
> +                 *              ofctrl_put() couldn't install the flow
and
> +                 *              an SB update caused the 'del_f' to be
removed
> +                 *              and re-added as 'f'.
> +                 *
> +                 * Scenario 2:  In a  single engine run, a flow 'del_f'
was
> +                 *              added to the desired flow, removed from
the
> +                 *              desired flow and re-added as 'f'.  This
could
> +                 *              happen after the commit c6c61b4e3462.
> +                 *
> +                 * Treat both the scenarios as valid scenarios and just
remove
> +                 * 'del_f' from the hmap - deleted_flows.
> +                 * update_installed_flows_by_track() will install 'f'.
> +                 */
>
> -            if (!f->installed_flow) {
> +            } else if (!f->installed_flow) {
>                  /* f is not installed yet. */
>                  replace_installed_to_desired(del_f->installed_flow,
del_f, f);
>              } else {
> --
> 2.29.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list