[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