[ovs-dev] [PATCH ovn] ofctrl_check_and_add_flow: Replace the actions of an existing flow if actions have changed.

Han Zhou zhouhan at gmail.com
Mon Dec 2 08:23:14 UTC 2019


On Sun, Dec 1, 2019 at 11:59 PM Numan Siddique <numans at ovn.org> wrote:
>
> On Mon, Dec 2, 2019 at 12:44 PM Han Zhou <zhouhan at gmail.com> wrote:
> >
> > On Fri, Nov 29, 2019 at 1:08 AM <numans at ovn.org> wrote:
> > >
> > > From: Numan Siddique <numans at ovn.org>
> > >
> > > If ofctrl_check_and_add_flow(F') is called where flow F' has
> > match-actions (M, A2)
> > > and if there already exists a flow F with match-actions (M, A1) in the
> > desired flow
> > > table, then this function  doesn't update the existing flow F with new
> > actions
> > > actions A2.
> > >
> > > This patch fixes it. Presently we don't see any issues because of this
> > behaviour.
> > > But it's better to fix it.
> > >
> >
> > Hi Numan, could you explain why do you think the F' should override the
F?
> > For my understanding, this is a problem of duplicated logical flows
> > generated by ovn-northd and can't be solved in ovn-controller. The
desired
> > flows have conflict and there is no way to judge which one should be
> > applied.
> >
>
> Hi Han,
>
> I probably should have given the context in which I observed this issue.
> I am working on making incremental processing for datapath
additions/deletions.
>
> In my testing I observed that the test case -  84: ovn.at:10767
> ovn -- send gratuitous ARP for NAT rules on HA distributed router
> is failing 100% of the time.
>
> Upon investigation I found that it's failing for below logical flow
>
>  table=17(ls_in_l2_lkup      ), priority=80   , match=(eth.src == {
> 00:00:00:00:ff:01} && (arp.op == 1 || nd_ns)), action=(outport =
> "_MC_flood"; output;)
>
> If you see the test code here -
> https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L10901
>
> ovn-nbctl --wait=hv clear logical_switch_port lrp0-rp options
>
> When the above command is executed, the logical switch "ls0" is
> removed from the "local_datapaths" hmap.
> When ls0 is removed, ovsdb-server sends monitor condition to remove
> the "Multicast_Group" row ls0.
>
> Later when this command is executed  - ovn-nbctl --wait=hv
> lsp-set-options lrp0-rp router-port=lrp0 nat-addresses="router"
>
> ovn-controller gets this update from sb ovsdb-server and it adds back
> ls0 to "local_datapaths". At this point, "Multicast_Group" row
> for ls0 is empty and the above logical flow  translates to "set:0->reg15".
>
> Soon after ovn-controller receives monitor update message to add back
> the "Multicast_Group" row for ls0.
> With the patch I am working, calculates logical flows for the logical
> switch sw0. But it doesn't add the right flow. The action should have
> been set to "set:0x8000->reg15".
>
> The patch I am working on can be found here - [1] -
>
https://github.com/numansiddique/ovn/commit/024812e76f4d0628612b1885cca65302a64038c8
> https://github.com/numansiddique/ovn/tree/lflow_reloaded/v1/p1
>
> Please note I am still testing it out and doing some scale testing
> before submitting the patch for review.
> [1] adds  a new table - "Datapath_Logical_Flow" in south db which is a
> weak ref and its references in Datapath_Binding.
>
> We can discuss more about the approach later.
>
> But I think the proposed patch here makes sense. We don't hit this
> issue in the present code because when ever any datapath_binding
> change happens
> we do full recompute and this flushes out the old  logical flow in the
> desired_flow_table.
>
Hi Numan,

Thanks for explaining the context. The principle in I-P for handling
changes is always handling deletions first and then creations, and for
updates, we always delete the old entries and then add the new ones.
In your context, if a mc_group is updated, we should delete the old flows
related to that mc_group and then create the new flows. Would the problem
be solved if we follow this principle?
In my view, ofctrl_check_and_add_flow() is not the right place for this,
because it already lose the context whether it is duplicated logical flows
generated by northd (e.g. conflict ACLs, or bugs), or it is just transient
status during ovn-controller processing, which is the case you encountered.

Thanks,
Han
>
> > > Signed-off-by: Numan Siddique <numans at ovn.org>
> > > ---
> > >  controller/ofctrl.c | 23 ++++++++++++++++-------
> > >  1 file changed, 16 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > > index 10edd84fb..5a174da48 100644
> > > --- a/controller/ofctrl.c
> > > +++ b/controller/ofctrl.c
> > > @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct
> > ovn_desired_flow_table *flow_table,
> > >
> > >      ovn_flow_log(f, "ofctrl_add_flow");
> > >
> > > -    if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) {
> > > -        if (log_duplicate_flow) {
> > > -            static struct vlog_rate_limit rl =
VLOG_RATE_LIMIT_INIT(5,
> > 5);
> > > -            if (!VLOG_DROP_DBG(&rl)) {
> > > -                char *s = ovn_flow_to_string(f);
> > > -                VLOG_DBG("dropping duplicate flow: %s", s);
> > > -                free(s);
> > > +    struct ovn_flow *existing_f =
> > > +        ovn_flow_lookup(&flow_table->match_flow_table, f, true);
> > > +    if (existing_f) {
> > > +        if (ofpacts_equal(f->ofpacts, f->ofpacts_len,
> > > +                          existing_f->ofpacts,
existing_f->ofpacts_len))
> > {
> > > +            if (log_duplicate_flow) {
> > > +                static struct vlog_rate_limit rl =
> > VLOG_RATE_LIMIT_INIT(5, 5);
> > > +                if (!VLOG_DROP_DBG(&rl)) {
> > > +                    char *s = ovn_flow_to_string(f);
> > > +                    VLOG_DBG("dropping duplicate flow: %s", s);
> > > +                    free(s);
> > > +                }
> > >              }
> > > +        } else {
> > > +            free(existing_f->ofpacts);
> > > +            existing_f->ofpacts = xmemdup(f->ofpacts,
f->ofpacts_len);
> > > +            existing_f->ofpacts_len = f->ofpacts_len;
> > >          }
> > >          ovn_flow_destroy(f);
> > >          return;
> > > --
> > > 2.23.0
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list