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

Numan Siddique numans at ovn.org
Mon Dec 2 07:58:57 UTC 2019


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.

Thanks
Numan


> > 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