[ovs-dev] [PATCH ovn v3] ovn-controller: Fix nb_cfg update with monitor_cond_change in flight.

Dumitru Ceara dceara at redhat.com
Mon Nov 30 15:45:45 UTC 2020


On 11/30/20 9:43 AM, Han Zhou wrote:
> 
> 
> On Tue, Nov 24, 2020 at 5:51 AM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> 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 <blp at ovn.org <mailto:blp at ovn.org>>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>>
>>
>> ---
>> 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 <hzhou at ovn.org <mailto:hzhou at ovn.org>>
> 
> Thanks,
> Han

Thanks,
Dumitru



More information about the dev mailing list