[ovs-dev] [PATCH] ovsdb-cs: Consider all tables when computing expected cond seqno.

[Dumitru Ceara]

On 5/7/21 7:55 PM, Ben Pfaff wrote:
> On Fri, May 07, 2021 at 06:47:06PM +0200, Dumitru Ceara wrote:
>> In ovsdb_cs_db_set_condition(), take into account all pending condition
>> changes for all tables when computing the db->cond_seqno at which the
>> monitor is expected to be updated.
>>
>> In the following scenario, with two tables, A and B, the old code
>> performed the following steps:
>> 1. Initial db->cond_seqno = X.
>> 2. Client changes condition for table A:
>>    - A->new_cond gets set
>>    - expected cond seqno returned to the client: X + 1
>> 3. ovsdb-cs sends the monitor_cond_change for table A
>>    - A->req_cond <- A->new_cond
>> 4. Client changes condition for table B:
>>    - B->new_cond gets set
>>    - expected cond seqno returned to the client: X + 1
>>    - however, because the conditon change at step 3 is still not replied
>>      to, table B's monitor_cond_change request is not sent yet.
>> 5. ovsdb-cs receives the reply for the condition change at step 3:
>>    - db->cond_seqno <- X + 1
>> 6. ovsdb-cs sends the monitor_cond_change for table B
>> 7. ovsdb-cs receives the reply for the condition change at step 6:
>>   - db->cond_seqno <- X + 2
>>
>> The client was incorrectly informed that it will have all relevant
>> updates for table B at seqno X + 1 while actually that happens later, at
>> seqno X + 2.
>>
>> Fixes: 46437c5232bd ("ovsdb-idl: Enhance conditional monitoring API")
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> 
> It's not very often that we get a bug fix for a 4 1/2 year old patch.  I
> hope you're willing to backport.

Sure, no problem.  I can send backports for all relevant branches once
this is accepted.

I think it wasn't really spotted before because there aren't so many
users of the IDL/CS modules that use the return value of
ovsdb_idl_set_condition()/ovsdb_cs_set_condition().

OVN started doing that since be4b7719dc0d ("ovsdb-idlc: Return expected
sequence number while setting conditions.") and 58e5d32b011b
("ovn-controller: Fix nb_cfg update with monitor_cond_change in flight.").

> 
> I thought about asking for a test but this seems really challenging to
> test.  Distributed systems are hard.
> 
> I've stared at this and thought about it and it seems correct.  Here's
> my ack:
> Acked-by: Ben Pfaff <blp at ovn.org>
> 
> I have tiny comments on the code itself.  One is that any_req_cond is
> only used in one branch of the 'if', so it could be calculated just on
> that branch.  The other is that any_req_cond is a bool so that
> the !! in !!any_req_cond can be removed.

Ah, I was under the (wrong) impression that there's no guarantee in the
standard about bool having only 0 and 1 as possible values.  I'll send a
v2 taking care of your comments.

Thanks for the review!

Regards,
Dumitru