On Tue, Dec 3, 2019 at 5:38 AM Han Zhou <zhou...@gmail.com> wrote:
>
> On Mon, Dec 2, 2019 at 12:41 AM Numan Siddique <num...@ovn.org> wrote:
> >
> > On Mon, Dec 2, 2019 at 1:53 PM Han Zhou <zhou...@gmail.com> wrote:
> > >
> > > On Sun, Dec 1, 2019 at 11:59 PM Numan Siddique <num...@ovn.org> wrote:
> > > >
> > > > On Mon, Dec 2, 2019 at 12:44 PM Han Zhou <zhou...@gmail.com> wrote:
> > > > >
> > > > > On Fri, Nov 29, 2019 at 1:08 AM <num...@ovn.org> wrote:
> > > > > >
> > > > > > From: Numan Siddique <num...@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 principle, yes this would work. But I am not sure how to associate
> > the logical flow/OF flows
> > related to mc_group.
> >
> > The action - "outport = _MC_Flood" (or any other _MC_*) can be used in
> > any pipeline of the
> > logical switches/logical routers. I couldn't figure out how to apply
> > incremental processing
> > for mc_group changes. The approach I have taken now is to recalculate
> > flows for only the
> > relevant datapaths (based on the datapath column of Multicast_Group
> table).
> >
>
> I think I have some idea on this. Currently,
> flow_output_sb_multicast_group_handler() handles multicast_group changes,
> but it only computes physical flows related to the multicast group change.
> Since datapath changes always trigger recompute, this is not a problem.
> Now that we want to handle datapath changes incrementally, since logical
> flow compute also depends on the multicast group (unless we find a way to
> decouple it), we need to add the handling for logical flows, too. The
> relationship between logical flows and MC groups can be tracked and handled
> similar to how address-set/port-group is handled, using lflow_resource_ref.
> I think the framework can be reused, with below difference considered:
> 1. The MC name is in the action instead of match. We need to add the
> reference between logical flow and the MC when parsing the actions.
> 2. The MC name is not globally unique, but this can be handled simply by
> adding the datapath uuid as a prefix to the MC name.

Agree this should work. I was thinking yesterday on similar lines to
address this.


>
> I'd like to know though, is it really that useful to handle datapath
> changes incrementally? I am not aware of any real world use case that
> requires adding/deleting datapaths frequently. It seems not a low hanging
> fruit to me since we need to handle the incremental processing of
> runtime_data, i.e. local-datapaths/bindings. Of course it would be great if
> this can be achieved without introducing too much complexity. Moreover, if
> this can be handled, the port-binding changes on local HV can be handled
> incrementally, too, since I feel it should be simpler (I didn't work on it
> yet and maybe I am wrong). I think handling port-binding changes on local
> HV incrementally may be more important since it reduces end-to-end latency
> for port creation/deletion, which are more frequent operations.

I want to submit a patch series, which handles incremental processing for
OVS conf changes, port binding changes and also to improve the time taken
for lflow_run() to complete. In one of our deployments we are seeing lflow_run()
taking > 80 seconds to complete. This is causing ovs-vswitchd to break
the openflow
connection (as vswitchd sends echo request every 60 seconds and this
is not configurable yet).

And we have seen customer deployments (with openstack) where they use
heat stack and create
lot of networks, ports and VMs and this takes a lot of time.

The whole idea for me to work on this is to reduce
 - the time spent on lflow_run.
 - incrementally handle port binding changes
 - incrementally handle any OVS conf db changes.

And I think handling datapath changes incrementally would help in
handling the above points.
For some reason even if we can't handle port binding changes
incrementally, at least we can limit
flow calculations only for the datapath to which the port belongs too.

Right now I am working on these things and I will submit RFC patches along with
the scale/performance testing numbers.

Thanks
Numan




>
> >
> > > 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.
> > >
> >
> > Suppose if CMS has added below ACLs
> >
> > match - "tcp.src > 0 && tcp.dst < 34555" action = "drop"
> > match - "tcp.src > 0 && tcp.dst < 34555" action = "allow"
> >
> > In the present code, we log the 2nd flow as duplicate and ignore it.
> > In the proposed patch, we override the first flow with the 2nd one.
> > Either way, it's not gonna work as expected. And CMS should correct it.
> >
> > But I tend to agree with you. Which makes me now to think of a better
> > way to address this :).
> >
> > Thanks
> > Numan
> >
> > > Thanks,
> > > Han
> > > >
> > > > > > Signed-off-by: Numan Siddique <num...@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
> > > > > > d...@openvswitch.org
> > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > > _______________________________________________
> > > > > dev mailing list
> > > > > d...@openvswitch.org
> > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > >
> > > _______________________________________________
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to