On Tue, Dec 3, 2019 at 5:18 AM Numan Siddique <[email protected]> wrote: > > On Tue, Dec 3, 2019 at 5:38 AM Han Zhou <[email protected]> wrote: > > > > On Mon, Dec 2, 2019 at 12:41 AM Numan Siddique <[email protected]> wrote: > > > > > > On Mon, Dec 2, 2019 at 1:53 PM Han Zhou <[email protected]> wrote: > > > > > > > > On Sun, Dec 1, 2019 at 11:59 PM Numan Siddique <[email protected]> wrote: > > > > > > > > > > On Mon, Dec 2, 2019 at 12:44 PM Han Zhou <[email protected]> wrote: > > > > > > > > > > > > On Fri, Nov 29, 2019 at 1:08 AM <[email protected]> wrote: > > > > > > > > > > > > > > From: Numan Siddique <[email protected]> > > > > > > > > > > > > > > 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 <[email protected]> > > > > > > > --- > > > > > > > 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 > > > > > > > [email protected] > > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > > > > > > dev mailing list > > > > > > [email protected] > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > [email protected] > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
