[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