| Message ID | 20220128090845.30497-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Commit | 28f36edd197b017e650a35e3203fd6b96e8c5bec |
| Headers | show |
| Series | [ovs-dev] ovsdb-idl: Only process successful txn in ovsdb_idl_loop_run. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 1/28/22 10:08, Dumitru Ceara wrote: > Otherwise we hide the transaction result from the user. This may cause > problems as the user will not detect error cases. For example, if the > server refuses a transaction with "constraint violation", the user > should be notified because the transaction might need to be retried. > > For clients that process database changes incrementally (using change > tracking) this lack of failure notification creates a problem if it > occurs while no other database changes happen. In that case: > - ovsdb_idl_loop_run() silently consumes the failure, initializes a > new transaction. > - no other table update was received from the server so the user will > not add anything to the new transaction. > - ovsdb_idl_loop_commit_and_wait() will "succeed" as nothing changed > from the client's perspective. > In reality, the first transaction failed and the client wasn't given > the chance to handle the failure. > > Commit 0401cf5f9e06 ("ovsdb idl: Try committing the pending txn in > ovsdb_idl_loop_run.") tried to optimize for the common, successful > case. Maintain the same approach and optimize for transactions that > succeeded but fall back to the old mechanism of processing failures > within ovsdb_idl_loop_commit_and_wait() instead. > > Fixes: 0401cf5f9e06 ("ovsdb idl: Try committing the pending txn in ovsdb_idl_loop_run.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > OVN test results using this patch: > https://github.com/dceara/ovn/actions/runs/1752945463 > --- Thanks! Applied and backported down to 2.14. Best regards, Ilya Maximets.
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index b7ba25eca6b5..8b9eab0336ec 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -4251,8 +4251,8 @@ ovsdb_idl_loop_run(struct ovsdb_idl_loop *loop) { ovsdb_idl_run(loop->idl); - /* See if we can commit the loop->committing_txn. */ - if (loop->committing_txn) { + /* See if the 'committing_txn' succeeded in the meantime. */ + if (loop->committing_txn && loop->committing_txn->status == TXN_SUCCESS) { ovsdb_idl_try_commit_loop_txn(loop, NULL); }
Otherwise we hide the transaction result from the user. This may cause problems as the user will not detect error cases. For example, if the server refuses a transaction with "constraint violation", the user should be notified because the transaction might need to be retried. For clients that process database changes incrementally (using change tracking) this lack of failure notification creates a problem if it occurs while no other database changes happen. In that case: - ovsdb_idl_loop_run() silently consumes the failure, initializes a new transaction. - no other table update was received from the server so the user will not add anything to the new transaction. - ovsdb_idl_loop_commit_and_wait() will "succeed" as nothing changed from the client's perspective. In reality, the first transaction failed and the client wasn't given the chance to handle the failure. Commit 0401cf5f9e06 ("ovsdb idl: Try committing the pending txn in ovsdb_idl_loop_run.") tried to optimize for the common, successful case. Maintain the same approach and optimize for transactions that succeeded but fall back to the old mechanism of processing failures within ovsdb_idl_loop_commit_and_wait() instead. Fixes: 0401cf5f9e06 ("ovsdb idl: Try committing the pending txn in ovsdb_idl_loop_run.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- OVN test results using this patch: https://github.com/dceara/ovn/actions/runs/1752945463 --- lib/ovsdb-idl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)