On 11/30/20 9:43 AM, Han Zhou wrote: > > > On Tue, Nov 24, 2020 at 5:51 AM Dumitru Ceara <[email protected] > <mailto:[email protected]>> wrote: >> >> It is not correct for ovn-controller to pass the current SB_Global.nb_cfg >> value to ofctrl_put() if there are pending changes to conditional >> monitoring clauses (local or in flight). It might be that after the >> monitor condition is acked by the SB, records that were added to the SB >> before SB_Global.nb_cfg was set are now sent as updates to >> ovn-controller. These should be first installed in OVS before >> ovn-controller reports that it caught up with the current >> SB_Global.nb_cfg value. >> >> Also: >> - ofctrl_put should not advance cur_cfg if there are flow updates in >> flight. >> - ofctrl_put should be called after SB monitor conditions are updated. >> >> Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental > engine - quiet mode.") > > For my understanding, the above commit didn't introduce the problem. The > monitor condition change was never considered in nb_cfg update before, > so I think it is a day-one issue. > I was aware of the problem/limitation of the nb_cfg mechanism but I > didn't think of such a solution. I like the way this patch is addressing it! >
Thanks! > Or maybe the "Fixes" refers to this part of the patch: "ofctrl_put > should not advance cur_cfg if there are flow updates in flight". That > commit is the culprit for this. I'd suggest using a separate patch > because these are totally different problems. Right, the "Fixes" is only for the second part of the patch. I'll split it in a series. > >> Acked-by: Ben Pfaff <[email protected] <mailto:[email protected]>> >> Signed-off-by: Dumitru Ceara <[email protected] > <mailto:[email protected]>> >> >> --- >> V3: >> - move ofctrl_put() call after updating SB monitor conditions. >> - added Ben's ack. >> v2: >> - use new IDL *set_condition() return value. >> - fix ofctrl_put to not advance cur_cfg if there are flow updates in >> flight. >> --- >> controller/ofctrl.c | 2 +- >> controller/ovn-controller.c | 91 > ++++++++++++++++++++++++++++++--------------- >> 2 files changed, 62 insertions(+), 31 deletions(-) >> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c >> index c1bbc58..eac5305 100644 >> --- a/controller/ofctrl.c >> +++ b/controller/ofctrl.c >> @@ -2034,7 +2034,7 @@ ofctrl_put(struct ovn_desired_flow_table > *flow_table, >> need_put = true; >> } else if (nb_cfg != old_nb_cfg) { >> /* nb_cfg changed since last ofctrl_put() call */ >> - if (cur_cfg == old_nb_cfg) { >> + if (cur_cfg == old_nb_cfg && ovs_list_is_empty(&flow_updates)) { > > Good catch! > However, in the "else" branch it also sets the need_put to true, which > shouldn't be affected by the condition ovs_list_is_empty(&flow_updates). > Could you revise it to make the nb_cfg update accurate while not > impacting the need_put logic? > I see, I missed the fact that we used the same logic to update two different things. I'll fix it in v4. > Consider my conditional ack for the in-flight monitor condition change > part of this patch if you would split the in-flight flow-updates as a > separate patch. > Acked-by: Han Zhou <[email protected] <mailto:[email protected]>> > > Thanks, > Han Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
