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 |
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.
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 --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