[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
Tue Dec 3 17:32:39 UTC 2019
On Tue, Dec 3, 2019 at 5:18 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Tue, Dec 3, 2019 at 5:38 AM Han Zhou <zhouhan at gmail.com> wrote:
> >
> > On Mon, Dec 2, 2019 at 12:41 AM Numan Siddique <numans at ovn.org> wrote:
> > >
> > > On Mon, Dec 2, 2019 at 1:53 PM Han Zhou <zhouhan at gmail.com> wrote:
> > > >
> > > > 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 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.
>
Sounds great! Looking forward to it.
> 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 <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
> > > > > >
> > > > _______________________________________________
> > > > 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