[ovs-dev] [ovsdb monitor fix v2 1/3] ovsdb: Fix one off error in tracking monitor changes

[Andy Zhou]

On Tue, Feb 23, 2016 at 12:14 AM, Liran Schour <LIRANS at il.ibm.com> wrote:

> "dev" <dev-bounces at openvswitch.org> wrote on 23/02/2016 02:28:12 AM:
>
> > dbmon's changes should be stored with the next transaction number,
> > rather than the current transaction number.  This bug causes the
> > changes of a transaction stored in a monitor to be unnoticed by
> > the jsonrpc connections that is responsible for flush the monitor
> > content.
> >
> > However, the bug was not noticed until it was exposed by a later
> > optimization patch: "avoid unnecessary call to
> ovsdb_monitor_get_update()."
> > The lack of optimization means that the update is still generated
> > when 'unflushed' equals to n_transactions + 1, which should have
> > indicated the monitor has been flushed already.
> >
> > Signed-off-by: Andy Zhou <azhou at ovn.org>
> >
>
> This patch looks good. However the essence of this change is that in case
> of OVSDB_CHANGES_REQUIRE_INTERNAL_UPDATE n_transcations will be
> incremented. That prevents you from seeing the inconsistency in the tests I
> mentioned here:
> http://openvswitch.org/pipermail/dev/2016-February/066501.html


I agree that this can happen in theory, especially when the jsonrpc client
connection is slow, or stalled for some reason. I am just not not sure this
actually can happen in a unit test environment.

>
>
> I still think that it is an issue. If you do insert X and then delete X
> the client might not know about X ever or get a notification about X. We
> can not be sure. What we know for sure that X will not be in the client's
> replica at the end...


True.  Do you see an issue outside of unit test? It would be good to get
Ben's perspective on this also since he will review those patches as well.


>
> Look at the following code from monitor.c:
>
> ovsdb_monitor_changes_update(const struct ovsdb_row *old,
>                              const struct ovsdb_row *new,
>                              const struct ovsdb_monitor_table *mt,
>                              struct ovsdb_monitor_changes *changes)
> {
>     const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old);
>     struct ovsdb_monitor_row *change;
>
>     change = ovsdb_monitor_changes_row_find(changes, uuid);
>     if (!change) {
>         change = xzalloc(sizeof *change);
>         hmap_insert(&changes->rows, &change->hmap_node, uuid_hash(uuid));
>         change->uuid = *uuid;
>         change->old = clone_monitor_row_data(mt, old);
>         change->new = clone_monitor_row_data(mt, new);
>     } else {
>         if (new) {
>             update_monitor_row_data(mt, new, change->new);
>         } else {
>             free_monitor_row_data(mt, change->new);
>             change->new = NULL;
>
>             if (!change->old) {
>                 /* This row was added then deleted.  Forget about it. */
>                 hmap_remove(&changes->rows, &change->hmap_node);
>                 free(change);
>                 -----> Here we remove  the entire change and the client
> never been notified about that....
>             }
>         }
>     }
> }
>
> Otherwise this patch series looks good and do fix a problem.


>
> Acked-by: Liran Schour <lirans at il.ibm.com>

Thanks for the review!