diff mbox series

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

Message ID 20210507192802.26485-1-dceara@redhat.com
State Accepted
Headers show
Series [ovs-dev,v2] ovsdb-cs: Consider all tables when computing expected cond seqno. | expand

Commit Message

Dumitru Ceara May 7, 2021, 7:28 p.m. UTC
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")
Acked-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
V2:
- Addressed Ben's comments:
  - move hmap walk inside the 'if' branch.
  - don't use !! for bool.
- Added Ben's ack.
---
 lib/ovsdb-cs.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Comments

Ilya Maximets May 14, 2021, 12:34 p.m. UTC | #1
On 5/7/21 9:28 PM, 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")
> Acked-by: Ben Pfaff <blp@ovn.org>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
> V2:
> - Addressed Ben's comments:
>   - move hmap walk inside the 'if' branch.
>   - don't use !! for bool.
> - Added Ben's ack.
> ---
>  lib/ovsdb-cs.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)

Thanks!  Applied to master and branch-2.15.

Could you, please, prepare backports for 2.14 and 2.13?

Best regards, Ilya Maximets.
Dumitru Ceara May 17, 2021, 9:32 a.m. UTC | #2
On 5/14/21 2:34 PM, Ilya Maximets wrote:
> On 5/7/21 9:28 PM, 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")
>> Acked-by: Ben Pfaff <blp@ovn.org>
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>> ---
>> V2:
>> - Addressed Ben's comments:
>>   - move hmap walk inside the 'if' branch.
>>   - don't use !! for bool.
>> - Added Ben's ack.
>> ---
>>  lib/ovsdb-cs.c | 23 +++++++++++++++++++++--
>>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> Thanks!  Applied to master and branch-2.15.
> 
> Could you, please, prepare backports for 2.14 and 2.13?
> 
> Best regards, Ilya Maximets.
> 

I sent a backport for 2.14 that should apply cleanly to branch 2.13 too:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=244128

Thanks!
diff mbox series

Patch

diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
index 6f9f912ac4..911b71dd4f 100644
--- a/lib/ovsdb-cs.c
+++ b/lib/ovsdb-cs.c
@@ -903,8 +903,27 @@  ovsdb_cs_db_set_condition(struct ovsdb_cs_db *db, const char *table,
     }
 
     /* Conditions will be up to date when we receive replies for already
-     * requested and new conditions, if any. */
-    return db->cond_seqno + (t->new_cond ? 1 : 0) + (t->req_cond ? 1 : 0);
+     * requested and new conditions, if any.  This includes condition change
+     * requests for other tables too.
+     */
+    if (t->new_cond) {
+        /* New condition will be sent out after all already requested ones
+         * are acked.
+         */
+        bool any_req_cond = false;
+        HMAP_FOR_EACH (t, hmap_node, &db->tables) {
+            if (t->req_cond) {
+                any_req_cond = true;
+                break;
+            }
+        }
+        return db->cond_seqno + any_req_cond + 1;
+    } else {
+        /* Already requested conditions should be up to date at
+         * db->cond_seqno + 1 while acked conditions are already up to date.
+         */
+        return db->cond_seqno + !!t->req_cond;
+    }
 }
 
 /* Sets the replication condition for 'tc' in 'cs' to 'condition' and arranges