[ovs-dev] [PATCH 2/3] ovsdb-idl: Properly handle conditional monitor update error

Andy Zhou azhou at ovn.org
Fri Jan 6 22:40:15 UTC 2017


On Wed, Jan 4, 2017 at 3:55 PM, Ben Pfaff <blp at ovn.org> wrote:

> On Thu, Dec 22, 2016 at 12:12:05PM -0800, Andy Zhou wrote:
> > On Thu, Dec 22, 2016 at 9:21 AM, Ben Pfaff <blp at ovn.org> wrote:
> >
> > > On Tue, Dec 20, 2016 at 01:47:16AM -0800, Andy Zhou wrote:
> > > > From: andy zhou <azhou at ovn.org>
> > > >
> > > > When generating conditional monitoring update request, current code
> > > > failed to update idl's 'request-id'.  This bug causes the reply
> > > > message of the update request, regardless an ACK or a NACK, be
> > > > logged as an unexpected message at the debug level and ignored by
> > > > the core idl logic.
> > > >
> > > > In addition, the idl should not generate another conditional
> > > > monitoring update request when there is an outstanding request.
> > > > So that the requests and their reply are properly serialized.
> > > >
> > > > When the conditional monitoring is nacked by the server, drop idl
> > > > into a client visible error state.
> > > >
> > > > Signed-off-by: Andy Zhou <azhou at ovn.org>
> > >
> > > When the condition is changed when an update request is already in
> > > flight, and the update request completes, will the code properly send
> > > a new condition change at that point?
> > >
> >
> > When an ack is received, the request_id will be freed and set to NULL,
> thus
> > unblocking ovsdb_idl_send_cond_changge() to send out the next condition
> > change.
> >
> > Am I still missing something?
>
> I asked the question because I hadn't looked, and it seemed like an easy
> mistake to make.  I agree that it seems OK.  However, I think that the
> loop in ovsdb_idl_run() can reset request_id without calling
> ovsdb_idl_send_cond_change() afterward.  That'll happen on the next trip
> through the main loop but at least in theory that could happen much
> later, so perhaps we should call it right away, e.g.:
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index 341951d..82c8584 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -458,6 +458,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
>
>              case IDL_S_MONITORING_COND:
>                  /* Conditional monitor clauses were updated. */
> +                ovsdb_idl_send_cond_change(idl);
>                  break;
>
>              case IDL_S_MONITORING:
>
> Probably the VLOG calls should be rate-limited.
>

Thanks for the suggestion. I folded this change into v2.


More information about the dev mailing list