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

[Ben Pfaff]

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.

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.