On Wed, Apr 28, 2021 at 3:31 AM Han Zhou <[email protected]> wrote:
>
> Thanks Numan for working on this issue!
>
> On Mon, Apr 26, 2021 at 3:53 AM <[email protected]> wrote:
> >
> > From: Numan Siddique <[email protected]>
> >
> > 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.
This can happen if ofctrl_can_put() returns false in which case
ovn-controller will
not call ofctrl_put() at all. There is a theoretical possibility here for sure.
Let me know if you think otherwise.
We observed this assertion during the upgrades of openshift to an ovn version
and there were a lot of "unreasonable timeout" warnings.
The updated OVN version had the commit [1] which led me to think that
commit [1] can also cause this.
>
> > 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
This typo really confused me :). From your comments the scenario 2 seems
not possible.
But I still think scenario 1 can happen if ofctrl_can_put() returns FALSE.
What do you think ?
Thanks
Numan
>
> 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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
