| Message ID | 1456187294-96467-1-git-send-email-azhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
"dev" <dev-bounces@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@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 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... 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@il.ibm.com>
On Tue, Feb 23, 2016 at 12:14 AM, Liran Schour <LIRANS@il.ibm.com> wrote: > "dev" <dev-bounces@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@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@il.ibm.com> Thanks for the review!
Andy Zhou <azhou@ovn.org> wrote on 23/02/2016 11:21:31 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@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. > No do not see other issues. The patch is good and will reduce some computation on the server side. > > > 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@il.ibm.com> > Thanks for the review!
On Tue, Feb 23, 2016 at 10:14:02AM +0200, Liran Schour wrote: > 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... That's a correct result. Multiple transactions can be combined before the client receives them. If the combination is a null change, then there's nothing to send the client.
On Mon, Feb 22, 2016 at 04:28:12PM -0800, Andy Zhou wrote: > 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@ovn.org> > > --- > v1->v2: > *Explain the bug in more detail in the commit message. > *roll back the n_transactions if possible. Acked-by: Ben Pfaff <blp@ovn.org>
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c index 5ae9cdb..0fe2bd6 100644 --- a/ovsdb/monitor.c +++ b/ovsdb/monitor.c @@ -1121,11 +1121,24 @@ ovsdb_monitor_commit(struct ovsdb_replica *replica, struct ovsdb_monitor_aux aux; ovsdb_monitor_init_aux(&aux, m); + /* Update ovsdb_monitor's transaction number for + * each transaction, before calling ovsdb_monitor_change_cb(). */ + m->n_transactions++; ovsdb_txn_for_each_change(txn, ovsdb_monitor_change_cb, &aux); - if (aux.efficacy == OVSDB_CHANGES_REQUIRE_EXTERNAL_UPDATE) { + switch(aux.efficacy) { + case OVSDB_CHANGES_NO_EFFECT: + /* The transaction is ignored by the monitor. + * Roll back the 'n_transactions' as if the transaction + * has never happened. */ + m->n_transactions--; + break; + case OVSDB_CHANGES_REQUIRE_INTERNAL_UPDATE: + /* Nothing. */ + break; + case OVSDB_CHANGES_REQUIRE_EXTERNAL_UPDATE: ovsdb_monitor_json_cache_flush(m); - m->n_transactions++; + break; } return NULL;
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@ovn.org> --- v1->v2: *Explain the bug in more detail in the commit message. *roll back the n_transactions if possible. --- ovsdb/monitor.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)