[ovs-dev] [dev master 2/3] bridge: Make ovs-vswitchd run again if status_txn commit fails.

[Alex Wang]

On Mon, Jun 9, 2014 at 2:36 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Sun, Jun 01, 2014 at 10:04:29PM -0700, Alex Wang wrote:
> > This commit adds logic that checks the return value of status_txn
> > transaction and runs the update again if the transaction fails.
> >
> > Signed-off-by: Alex Wang <alexw at nicira.com>
>
> I am not sure that you understand what TXN_INCOMPLETE means.  It means
> that a transaction has been queued up to send to the database server,
> but the database server has not yet replied.  Some of the comments in
> this commit imply that we should retry the transaction in the case of
> TXN_INCOMPLETE, but no, that's not the case; we should just wait for
> the database server to respond.
>
> It seems to me that status_txn_try_again is only used after the
> transaction finishes, since if status == TXN_INCOMPLETE then we're
> still waiting for it to complete and status_txn is still nonnull.  So
> I would update this comment to remove the mention of "incomplete":
>



Thanks for correcting my understanding.  I forgot to follow the
instant_stats_wait() logic when removing it.  I'll adjust the code
accordingly.





> > + * If the previous database transaction was incomplete or failed (is not
> > + * 'TXN_SUCCESS' or 'TXN_UNCHANGED'), 'status_txn_try_again' is set to
> true,
> > + * which will cause the main thread wake up soon and retry the status
> update.
> >   */
> >  static struct ovsdb_idl_txn *status_txn;
> > +static bool status_txn_try_again;
>
> Also, here we really only care to set status_txn_try_again if the
> transaction is somehow complete, right?  Then I would move the new
> code inside the previous "if" block that only runs if status !=
> TXN_INCOMPLETE.  Also, TXN_UNCOMMITTED should be impossible here,
> since we called ovsdb_idl_txn_commit() a few lines earlier.
>


I'll do that.




> > @@ -2453,6 +2458,14 @@ bridge_run(void)
> >              ovsdb_idl_txn_destroy(status_txn);
> >              status_txn = NULL;
> >          }
> > +
> > +        /* Sets the 'status_txn_try_again' if the transaction fails or
> > +         * is still incomplete. */
> > +        if (status == TXN_SUCCESS || status == TXN_UNCOMMITTED) {
> > +            status_txn_try_again = false;
> > +        } else {
> > +            status_txn_try_again = true;
> > +        }
> >      }
> >
> >      run_system_stats();
>
> There seems to be some similar confusion here.  If the transaction is
> incomplete, then we shouldn't do any kind of timer-based waiting, we
> should instead wait for the transaction to complete:
>


I'll fix it.


> > @@ -2486,11 +2499,11 @@ bridge_wait(void)
> >          poll_timer_wait_until(stats_timer);
> >      }
> >
> > -    /* If the status database transaction is 'TXN_INCOMPLETE' in this
> run,
> > -     * register a timeout in 'STATUS_CHECK_AGAIN_MSEC'.  Else, wait on
> the
> > -     * global connectivity sequence number.  Note, this also helps batch
> > -     * multiple status changes into one transaction. */
> > -    if (status_txn) {
> > +    /* If the status update to database needs to be run again
> (transaction
> > +     * fails or incomplete), registers a timeout in
> 'STATUS_CHECK_AGAIN_MSEC'.
> > +     * Else, waits on the global connectivity sequence number.  Note,
> this also
> > +     * helps batch multiple status changes into one transaction. */
> > +    if (status_txn_try_again) {
> >          poll_timer_wait_until(time_msec() + STATUS_CHECK_AGAIN_MSEC);
> >      } else {
> >          seq_wait(connectivity_seq_get(), connectivity_seqno);
> > --
> > 1.7.9.5
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140609/75ef9cc8/attachment-0005.html>