| Message ID | 20191129090735.587301-1-numans@ovn.org |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [ovs-dev,ovn] ofctrl_check_and_add_flow: Replace the actions of an existing flow if actions have changed. | expand |
On Fri, Nov 29, 2019 at 1:08 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@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. > Signed-off-by: Numan Siddique <numans@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Mon, Dec 2, 2019 at 12:44 PM Han Zhou <zhouhan@gmail.com> wrote: > > On Fri, Nov 29, 2019 at 1:08 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@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@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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Sun, Dec 1, 2019 at 11:59 PM Numan Siddique <numans@ovn.org> wrote: > > On Mon, Dec 2, 2019 at 12:44 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Fri, Nov 29, 2019 at 1:08 AM <numans@ovn.org> wrote: > > > > > > From: Numan Siddique <numans@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 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. Thanks, Han > > > > Signed-off-by: Numan Siddique <numans@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@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Mon, Dec 2, 2019 at 1:53 PM Han Zhou <zhouhan@gmail.com> wrote: > > On Sun, Dec 1, 2019 at 11:59 PM Numan Siddique <numans@ovn.org> wrote: > > > > On Mon, Dec 2, 2019 at 12:44 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > On Fri, Nov 29, 2019 at 1:08 AM <numans@ovn.org> wrote: > > > > > > > > From: Numan Siddique <numans@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). > 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@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@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, Dec 2, 2019 at 12:41 AM Numan Siddique <numans@ovn.org> wrote: > > On Mon, Dec 2, 2019 at 1:53 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Sun, Dec 1, 2019 at 11:59 PM Numan Siddique <numans@ovn.org> wrote: > > > > > > On Mon, Dec 2, 2019 at 12:44 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Fri, Nov 29, 2019 at 1:08 AM <numans@ovn.org> wrote: > > > > > > > > > > From: Numan Siddique <numans@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. 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. > > > 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@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@openvswitch.org > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Tue, Dec 3, 2019 at 5:38 AM Han Zhou <zhouhan@gmail.com> wrote: > > On Mon, Dec 2, 2019 at 12:41 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Mon, Dec 2, 2019 at 1:53 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > On Sun, Dec 1, 2019 at 11:59 PM Numan Siddique <numans@ovn.org> wrote: > > > > > > > > On Mon, Dec 2, 2019 at 12:44 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > On Fri, Nov 29, 2019 at 1:08 AM <numans@ovn.org> wrote: > > > > > > > > > > > > From: Numan Siddique <numans@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 <numans@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@openvswitch.org > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > > > > > dev mailing list > > > > > dev@openvswitch.org > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Dec 3, 2019 at 5:18 AM Numan Siddique <numans@ovn.org> wrote: > > On Tue, Dec 3, 2019 at 5:38 AM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Mon, Dec 2, 2019 at 12:41 AM Numan Siddique <numans@ovn.org> wrote: > > > > > > On Mon, Dec 2, 2019 at 1:53 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Sun, Dec 1, 2019 at 11:59 PM Numan Siddique <numans@ovn.org> wrote: > > > > > > > > > > On Mon, Dec 2, 2019 at 12:44 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > On Fri, Nov 29, 2019 at 1:08 AM <numans@ovn.org> wrote: > > > > > > > > > > > > > > From: Numan Siddique <numans@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@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@openvswitch.org > > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > > > > > > dev mailing list > > > > > > dev@openvswitch.org > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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;