| Message ID | 20180517171655.20550-1-mmichels@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovsdb-idl: Correct singleton insert logic | expand |
On Thu, May 17, 2018 at 01:16:55PM -0400, Mark Michelson wrote: > When inserting data into a "singleton" table (one that has maxRows == > 1), there is a check that ensures that the table is currently empty > before inserting the row. The intention is to prevent races where > multiple clients might attempt to insert rows at the same time. > > The problem is that this singleton check can cause legitimate > transactions to fail. Specifically, a transaction that attempts to > delete the current content of the table and insert new data will cause > the singleton check to fail since the table currently has data. > > This patch corrects the issue by keeping a count of the rows being > deleted and added to singleton tables. If the total is larger than zero, > then the net operation is attempting to insert rows. If the total is > less than zero, then the net operation is attempting to remove rows. If > the total is zero, then the operation is inserting and deleting an equal > number of rows (or is just updating rows). We only add the singleton > check if the total is larger than zero. > > This patch also includes a new test for singleton tables that ensures > that the maxRows constraint works as expected. > > Signed-off-by: Mark Michelson <mmichels@redhat.com> Good catch. (It's kind of weird to delete and repopulate a singleton table, but we should support it.) I think that the following patch achieves the same end with less bookkeeping overhead. What do you think? It doesn't break anything in the testsuite, but I didn't check that it actually achieves the purpose. diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index c6ff78e25a04..1453025acbd6 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -3929,6 +3929,39 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) /* Add updates. */ any_updates = false; + + /* For tables constrained to have only a single row (a fairly common OVSDB + * pattern for storing global data), identify whether we're inserting a + * row. If so, then verify that the table is empty before inserting the + * row. This gives us a clear verification-related failure if there was an + * insertion race with another client. */ + for (size_t i = 0; i < txn->db->class_->n_tables; i++) { + struct ovsdb_idl_table *table = &txn->db->tables[i]; + if (table->class_->is_singleton) { + /* Count the number of rows in the table before and after our + * transaction commits. This is O(n) in the number of rows in the + * table, but that's OK since we know that the table should only + * have one row. */ + size_t initial_rows = 0; + size_t final_rows = 0; + HMAP_FOR_EACH (row, hmap_node, &table->rows) { + initial_rows += row->old_datum != NULL; + final_rows += row->new_datum != NULL; + } + + if (initial_rows == 0 && final_rows == 1) { + struct json *op = json_object_create(); + json_array_add(operations, op); + json_object_put_string(op, "op", "wait"); + json_object_put_string(op, "table", table->class_->name); + json_object_put(op, "where", json_array_create_empty()); + json_object_put(op, "timeout", json_integer_create(0)); + json_object_put_string(op, "until", "=="); + json_object_put(op, "rows", json_array_create_empty()); + } + } + } + HMAP_FOR_EACH (row, txn_node, &txn->txn_rows) { const struct ovsdb_idl_table_class *class = row->table->class_; @@ -3947,23 +3980,6 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) struct json *row_json; size_t idx; - if (!row->old_datum && class->is_singleton) { - /* We're inserting a row into a table that allows only a - * single row. (This is a fairly common OVSDB pattern for - * storing global data.) Verify that the table is empty - * before inserting the row, so that we get a clear - * verification-related failure if there was an insertion - * race with another client. */ - struct json *op = json_object_create(); - json_array_add(operations, op); - json_object_put_string(op, "op", "wait"); - json_object_put_string(op, "table", class->name); - json_object_put(op, "where", json_array_create_empty()); - json_object_put(op, "timeout", json_integer_create(0)); - json_object_put_string(op, "until", "=="); - json_object_put(op, "rows", json_array_create_empty()); - } - struct json *op = json_object_create(); json_object_put_string(op, "op", row->old_datum ? "update" : "insert");
Hi Ben, Thanks for having a look and providing an update. See my replies in-line below. On 05/25/2018 06:31 PM, Ben Pfaff wrote: > On Thu, May 17, 2018 at 01:16:55PM -0400, Mark Michelson wrote: >> When inserting data into a "singleton" table (one that has maxRows == >> 1), there is a check that ensures that the table is currently empty >> before inserting the row. The intention is to prevent races where >> multiple clients might attempt to insert rows at the same time. >> >> The problem is that this singleton check can cause legitimate >> transactions to fail. Specifically, a transaction that attempts to >> delete the current content of the table and insert new data will cause >> the singleton check to fail since the table currently has data. >> >> This patch corrects the issue by keeping a count of the rows being >> deleted and added to singleton tables. If the total is larger than zero, >> then the net operation is attempting to insert rows. If the total is >> less than zero, then the net operation is attempting to remove rows. If >> the total is zero, then the operation is inserting and deleting an equal >> number of rows (or is just updating rows). We only add the singleton >> check if the total is larger than zero. >> >> This patch also includes a new test for singleton tables that ensures >> that the maxRows constraint works as expected. >> >> Signed-off-by: Mark Michelson <mmichels@redhat.com> > > Good catch. (It's kind of weird to delete and repopulate a singleton > table, but we should support it.) The use case that brought this to our attention was the "set-ssl" commands for ovn-nbctl and ovn-sbctl. Since the SSL table is a singleton, these operations delete the current row and repopulate with a new row. The existing check resulted in failure when when the transaction should have been allowed. > > I think that the following patch achieves the same end with less > bookkeeping overhead. What do you think? It doesn't break anything in > the testsuite, but I didn't check that it actually achieves the purpose. > I did some testing and debugging and this looks like it has the expected behavior, and I agree that this seems like a simpler approach. So Acked-by: Mark Michelson <mmichels@redhat.com> > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index c6ff78e25a04..1453025acbd6 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -3929,6 +3929,39 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) > > /* Add updates. */ > any_updates = false; > + > + /* For tables constrained to have only a single row (a fairly common OVSDB > + * pattern for storing global data), identify whether we're inserting a > + * row. If so, then verify that the table is empty before inserting the > + * row. This gives us a clear verification-related failure if there was an > + * insertion race with another client. */ > + for (size_t i = 0; i < txn->db->class_->n_tables; i++) { > + struct ovsdb_idl_table *table = &txn->db->tables[i]; > + if (table->class_->is_singleton) { > + /* Count the number of rows in the table before and after our > + * transaction commits. This is O(n) in the number of rows in the > + * table, but that's OK since we know that the table should only > + * have one row. */ > + size_t initial_rows = 0; > + size_t final_rows = 0; > + HMAP_FOR_EACH (row, hmap_node, &table->rows) { > + initial_rows += row->old_datum != NULL; > + final_rows += row->new_datum != NULL; > + } > + > + if (initial_rows == 0 && final_rows == 1) { > + struct json *op = json_object_create(); > + json_array_add(operations, op); > + json_object_put_string(op, "op", "wait"); > + json_object_put_string(op, "table", table->class_->name); > + json_object_put(op, "where", json_array_create_empty()); > + json_object_put(op, "timeout", json_integer_create(0)); > + json_object_put_string(op, "until", "=="); > + json_object_put(op, "rows", json_array_create_empty()); > + } > + } > + } > + > HMAP_FOR_EACH (row, txn_node, &txn->txn_rows) { > const struct ovsdb_idl_table_class *class = row->table->class_; > > @@ -3947,23 +3980,6 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) > struct json *row_json; > size_t idx; > > - if (!row->old_datum && class->is_singleton) { > - /* We're inserting a row into a table that allows only a > - * single row. (This is a fairly common OVSDB pattern for > - * storing global data.) Verify that the table is empty > - * before inserting the row, so that we get a clear > - * verification-related failure if there was an insertion > - * race with another client. */ > - struct json *op = json_object_create(); > - json_array_add(operations, op); > - json_object_put_string(op, "op", "wait"); > - json_object_put_string(op, "table", class->name); > - json_object_put(op, "where", json_array_create_empty()); > - json_object_put(op, "timeout", json_integer_create(0)); > - json_object_put_string(op, "until", "=="); > - json_object_put(op, "rows", json_array_create_empty()); > - } > - > struct json *op = json_object_create(); > json_object_put_string(op, "op", > row->old_datum ? "update" : "insert"); >
On Tue, May 29, 2018 at 09:15:52AM -0400, Mark Michelson wrote: > Hi Ben, > > Thanks for having a look and providing an update. See my replies in-line > below. > > On 05/25/2018 06:31 PM, Ben Pfaff wrote: > >On Thu, May 17, 2018 at 01:16:55PM -0400, Mark Michelson wrote: > >>When inserting data into a "singleton" table (one that has maxRows == > >>1), there is a check that ensures that the table is currently empty > >>before inserting the row. The intention is to prevent races where > >>multiple clients might attempt to insert rows at the same time. > >> > >>The problem is that this singleton check can cause legitimate > >>transactions to fail. Specifically, a transaction that attempts to > >>delete the current content of the table and insert new data will cause > >>the singleton check to fail since the table currently has data. > >> > >>This patch corrects the issue by keeping a count of the rows being > >>deleted and added to singleton tables. If the total is larger than zero, > >>then the net operation is attempting to insert rows. If the total is > >>less than zero, then the net operation is attempting to remove rows. If > >>the total is zero, then the operation is inserting and deleting an equal > >>number of rows (or is just updating rows). We only add the singleton > >>check if the total is larger than zero. > >> > >>This patch also includes a new test for singleton tables that ensures > >>that the maxRows constraint works as expected. > >> > >>Signed-off-by: Mark Michelson <mmichels@redhat.com> > > > >Good catch. (It's kind of weird to delete and repopulate a singleton > >table, but we should support it.) > > The use case that brought this to our attention was the "set-ssl" commands > for ovn-nbctl and ovn-sbctl. Since the SSL table is a singleton, these > operations delete the current row and repopulate with a new row. The > existing check resulted in failure when when the transaction should have > been allowed. > > > > >I think that the following patch achieves the same end with less > >bookkeeping overhead. What do you think? It doesn't break anything in > >the testsuite, but I didn't check that it actually achieves the purpose. > > > > I did some testing and debugging and this looks like it has the expected > behavior, and I agree that this seems like a simpler approach. So > > Acked-by: Mark Michelson <mmichels@redhat.com> Thanks. I applied this to master.
Hi Ben, This fixes an issue that is being experienced by users in version 2.9 as well. Can this please be backported there as well? Thanks, Mark Michelson On 06/15/2018 07:27 PM, Ben Pfaff wrote: > On Tue, May 29, 2018 at 09:15:52AM -0400, Mark Michelson wrote: >> Hi Ben, >> >> Thanks for having a look and providing an update. See my replies in-line >> below. >> >> On 05/25/2018 06:31 PM, Ben Pfaff wrote: >>> On Thu, May 17, 2018 at 01:16:55PM -0400, Mark Michelson wrote: >>>> When inserting data into a "singleton" table (one that has maxRows == >>>> 1), there is a check that ensures that the table is currently empty >>>> before inserting the row. The intention is to prevent races where >>>> multiple clients might attempt to insert rows at the same time. >>>> >>>> The problem is that this singleton check can cause legitimate >>>> transactions to fail. Specifically, a transaction that attempts to >>>> delete the current content of the table and insert new data will cause >>>> the singleton check to fail since the table currently has data. >>>> >>>> This patch corrects the issue by keeping a count of the rows being >>>> deleted and added to singleton tables. If the total is larger than zero, >>>> then the net operation is attempting to insert rows. If the total is >>>> less than zero, then the net operation is attempting to remove rows. If >>>> the total is zero, then the operation is inserting and deleting an equal >>>> number of rows (or is just updating rows). We only add the singleton >>>> check if the total is larger than zero. >>>> >>>> This patch also includes a new test for singleton tables that ensures >>>> that the maxRows constraint works as expected. >>>> >>>> Signed-off-by: Mark Michelson <mmichels@redhat.com> >>> >>> Good catch. (It's kind of weird to delete and repopulate a singleton >>> table, but we should support it.) >> >> The use case that brought this to our attention was the "set-ssl" commands >> for ovn-nbctl and ovn-sbctl. Since the SSL table is a singleton, these >> operations delete the current row and repopulate with a new row. The >> existing check resulted in failure when when the transaction should have >> been allowed. >> >>> >>> I think that the following patch achieves the same end with less >>> bookkeeping overhead. What do you think? It doesn't break anything in >>> the testsuite, but I didn't check that it actually achieves the purpose. >>> >> >> I did some testing and debugging and this looks like it has the expected >> behavior, and I agree that this seems like a simpler approach. So >> >> Acked-by: Mark Michelson <mmichels@redhat.com> > > Thanks. I applied this to master. >
Done. On Mon, Jun 25, 2018 at 08:08:02AM -0400, Mark Michelson wrote: > Hi Ben, > > This fixes an issue that is being experienced by users in version 2.9 as > well. Can this please be backported there as well? > > Thanks, > Mark Michelson > > On 06/15/2018 07:27 PM, Ben Pfaff wrote: > >On Tue, May 29, 2018 at 09:15:52AM -0400, Mark Michelson wrote: > >>Hi Ben, > >> > >>Thanks for having a look and providing an update. See my replies in-line > >>below. > >> > >>On 05/25/2018 06:31 PM, Ben Pfaff wrote: > >>>On Thu, May 17, 2018 at 01:16:55PM -0400, Mark Michelson wrote: > >>>>When inserting data into a "singleton" table (one that has maxRows == > >>>>1), there is a check that ensures that the table is currently empty > >>>>before inserting the row. The intention is to prevent races where > >>>>multiple clients might attempt to insert rows at the same time. > >>>> > >>>>The problem is that this singleton check can cause legitimate > >>>>transactions to fail. Specifically, a transaction that attempts to > >>>>delete the current content of the table and insert new data will cause > >>>>the singleton check to fail since the table currently has data. > >>>> > >>>>This patch corrects the issue by keeping a count of the rows being > >>>>deleted and added to singleton tables. If the total is larger than zero, > >>>>then the net operation is attempting to insert rows. If the total is > >>>>less than zero, then the net operation is attempting to remove rows. If > >>>>the total is zero, then the operation is inserting and deleting an equal > >>>>number of rows (or is just updating rows). We only add the singleton > >>>>check if the total is larger than zero. > >>>> > >>>>This patch also includes a new test for singleton tables that ensures > >>>>that the maxRows constraint works as expected. > >>>> > >>>>Signed-off-by: Mark Michelson <mmichels@redhat.com> > >>> > >>>Good catch. (It's kind of weird to delete and repopulate a singleton > >>>table, but we should support it.) > >> > >>The use case that brought this to our attention was the "set-ssl" commands > >>for ovn-nbctl and ovn-sbctl. Since the SSL table is a singleton, these > >>operations delete the current row and repopulate with a new row. The > >>existing check resulted in failure when when the transaction should have > >>been allowed. > >> > >>> > >>>I think that the following patch achieves the same end with less > >>>bookkeeping overhead. What do you think? It doesn't break anything in > >>>the testsuite, but I didn't check that it actually achieves the purpose. > >>> > >> > >>I did some testing and debugging and this looks like it has the expected > >>behavior, and I agree that this seems like a simpler approach. So > >> > >>Acked-by: Mark Michelson <mmichels@redhat.com> > > > >Thanks. I applied this to master. > > >
Thank you very much, Ben! On 06/25/2018 04:22 PM, Ben Pfaff wrote: > Done. > > On Mon, Jun 25, 2018 at 08:08:02AM -0400, Mark Michelson wrote: >> Hi Ben, >> >> This fixes an issue that is being experienced by users in version 2.9 as >> well. Can this please be backported there as well? >> >> Thanks, >> Mark Michelson >> >> On 06/15/2018 07:27 PM, Ben Pfaff wrote: >>> On Tue, May 29, 2018 at 09:15:52AM -0400, Mark Michelson wrote: >>>> Hi Ben, >>>> >>>> Thanks for having a look and providing an update. See my replies in-line >>>> below. >>>> >>>> On 05/25/2018 06:31 PM, Ben Pfaff wrote: >>>>> On Thu, May 17, 2018 at 01:16:55PM -0400, Mark Michelson wrote: >>>>>> When inserting data into a "singleton" table (one that has maxRows == >>>>>> 1), there is a check that ensures that the table is currently empty >>>>>> before inserting the row. The intention is to prevent races where >>>>>> multiple clients might attempt to insert rows at the same time. >>>>>> >>>>>> The problem is that this singleton check can cause legitimate >>>>>> transactions to fail. Specifically, a transaction that attempts to >>>>>> delete the current content of the table and insert new data will cause >>>>>> the singleton check to fail since the table currently has data. >>>>>> >>>>>> This patch corrects the issue by keeping a count of the rows being >>>>>> deleted and added to singleton tables. If the total is larger than zero, >>>>>> then the net operation is attempting to insert rows. If the total is >>>>>> less than zero, then the net operation is attempting to remove rows. If >>>>>> the total is zero, then the operation is inserting and deleting an equal >>>>>> number of rows (or is just updating rows). We only add the singleton >>>>>> check if the total is larger than zero. >>>>>> >>>>>> This patch also includes a new test for singleton tables that ensures >>>>>> that the maxRows constraint works as expected. >>>>>> >>>>>> Signed-off-by: Mark Michelson <mmichels@redhat.com> >>>>> >>>>> Good catch. (It's kind of weird to delete and repopulate a singleton >>>>> table, but we should support it.) >>>> >>>> The use case that brought this to our attention was the "set-ssl" commands >>>> for ovn-nbctl and ovn-sbctl. Since the SSL table is a singleton, these >>>> operations delete the current row and repopulate with a new row. The >>>> existing check resulted in failure when when the transaction should have >>>> been allowed. >>>> >>>>> >>>>> I think that the following patch achieves the same end with less >>>>> bookkeeping overhead. What do you think? It doesn't break anything in >>>>> the testsuite, but I didn't check that it actually achieves the purpose. >>>>> >>>> >>>> I did some testing and debugging and this looks like it has the expected >>>> behavior, and I agree that this seems like a simpler approach. So >>>> >>>> Acked-by: Mark Michelson <mmichels@redhat.com> >>> >>> Thanks. I applied this to master. >>> >>
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 9b99375da..943fa8f6d 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -3929,6 +3929,27 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) /* Add updates. */ any_updates = false; + struct shash singletons; + shash_init(&singletons); + /* This calculates the total number of rows being inserted or deleted + * from singleton tables in this transaction. + */ + HMAP_FOR_EACH (row, txn_node, &txn->txn_rows) { + const struct ovsdb_idl_table_class *class = row->table->class_; + if (class->is_singleton) { + int *class_insert_count = shash_find_data(&singletons, class->name); + if (!class_insert_count) { + class_insert_count = xzalloc(sizeof *class_insert_count); + shash_add(&singletons, class->name, class_insert_count); + } + if (!row->new_datum) { + (*class_insert_count)--; + } else if (!row->old_datum) { + (*class_insert_count)++; + } + } + } + HMAP_FOR_EACH (row, txn_node, &txn->txn_rows) { const struct ovsdb_idl_table_class *class = row->table->class_; @@ -3948,20 +3969,25 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) size_t idx; if (!row->old_datum && class->is_singleton) { - /* We're inserting a row into a table that allows only a - * single row. (This is a fairly common OVSDB pattern for - * storing global data.) Verify that the table is empty - * before inserting the row, so that we get a clear - * verification-related failure if there was an insertion - * race with another client. */ - struct json *op = json_object_create(); - json_array_add(operations, op); - json_object_put_string(op, "op", "wait"); - json_object_put_string(op, "table", class->name); - json_object_put(op, "where", json_array_create_empty()); - json_object_put(op, "timeout", json_integer_create(0)); - json_object_put_string(op, "until", "=="); - json_object_put(op, "rows", json_array_create_empty()); + int *class_insert_count = shash_find_data(&singletons, class->name); + ovs_assert(class_insert_count); + if (*class_insert_count > 0) { + /* We're inserting a row into a table that allows only a + * single row. (This is a fairly common OVSDB pattern for + * storing global data.) Verify that the table is empty + * before inserting the row, so that we get a clear + * verification-related failure if there was an insertion + * race with another client. + */ + struct json *op = json_object_create(); + json_array_add(operations, op); + json_object_put_string(op, "op", "wait"); + json_object_put_string(op, "table", class->name); + json_object_put(op, "where", json_array_create_empty()); + json_object_put(op, "timeout", json_integer_create(0)); + json_object_put_string(op, "until", "=="); + json_object_put(op, "rows", json_array_create_empty()); + } } struct json *op = json_object_create(); @@ -4047,6 +4073,7 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) } } } + shash_destroy_free_data(&singletons); /* Add increment. */ if (txn->inc_table && (any_updates || txn->inc_force)) { diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema index 57b6bde15..bee79fc50 100644 --- a/tests/idltest.ovsschema +++ b/tests/idltest.ovsschema @@ -170,6 +170,15 @@ } }, "isRoot" : false + }, + "singleton" : { + "columns" : { + "name" : { + "type": "string" + } + }, + "isRoot" : true, + "maxRows" : 1 } } } diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index 9caa02aad..9dcd34445 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -70,7 +70,7 @@ m4_define([OVSDB_CHECK_IDL_REGISTER_COLUMNS_PYN], AT_CHECK([ovsdb_start_idltest]) m4_if([$2], [], [], [AT_CHECK([ovsdb-client transact unix:socket $2], [0], [ignore], [ignore])]) - AT_CHECK([$8 $srcdir/test-ovsdb.py -t10 idl $srcdir/idltest.ovsschema unix:socket ?simple:b,ba,i,ia,r,ra,s,sa,u,ua?link1:i,k,ka,l2?link2:i,l1 $3], + AT_CHECK([$8 $srcdir/test-ovsdb.py -t10 idl $srcdir/idltest.ovsschema unix:socket ?simple:b,ba,i,ia,r,ra,s,sa,u,ua?link1:i,k,ka,l2?link2:i,l1?singleton:name $3], [0], [stdout], [ignore]) AT_CHECK([sort stdout | uuidfilt]m4_if([$6],,, [[| $6]]), [0], [$4]) @@ -776,6 +776,32 @@ OVSDB_CHECK_IDL([external-linking idl, consistent ops], 003: done ]]) +OVSDB_CHECK_IDL([singleton idl, constraints], + [], + [['["idltest", + {"op": "insert", + "table": "singleton", + "row": {"name": "foo"}}]' \ + '["idltest", + {"op": "insert", + "table": "singleton", + "row": {"name": "bar"}}]' \ + '+["idltest", + {"op": "delete", + "table": "singleton", + "where": [["_uuid", "==", ["uuid", "#0#"]]]}, + {"op": "insert", + "table": "singleton", + "row": {"name": "bar"}}]']], + [[000: empty +001: {"error":null,"result":[{"uuid":["uuid","<0>"]}]} +002: name=foo uuid=<0> +003: {"error":null,"result":[{"uuid":["uuid","<1>"]},{"details":"transaction causes \"singleton\" table to contain 2 rows, greater than the schema-defined limit of 1 row(s)","error":"constraint violation"}]} +004: {"error":null,"result":[{"count":1},{"uuid":["uuid","<2>"]}]} +005: name=bar uuid=<2> +006: done +]]) + OVSDB_CHECK_IDL_PY([external-linking idl, insert ops], [], [['linktest']], diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c index 05e97cb51..9676772c6 100644 --- a/tests/test-ovsdb.c +++ b/tests/test-ovsdb.c @@ -1907,6 +1907,26 @@ print_idl_row_updated_link2(const struct idltest_link2 *l2, int step) } } +static void +print_idl_row_updated_singleton(const struct idltest_singleton *sng, int step) +{ + size_t i; + bool updated = false; + + for (i = 0; i < IDLTEST_SINGLETON_N_COLUMNS; i++) { + if (idltest_singleton_is_updated(sng, i)) { + if (!updated) { + printf("%03d: updated columns:", step); + updated = true; + } + printf(" %s", idltest_singleton_columns[i].name); + } + } + if (updated) { + printf("\n"); + } +} + static void print_idl_row_simple(const struct idltest_simple *s, int step) { @@ -1974,12 +1994,21 @@ print_idl_row_link2(const struct idltest_link2 *l2, int step) print_idl_row_updated_link2(l2, step); } +static void +print_idl_row_singleton(const struct idltest_singleton *sng, int step) +{ + printf("%03d: name=%s", step, sng->name); + printf(" uuid="UUID_FMT"\n", UUID_ARGS(&sng->header_.uuid)); + print_idl_row_updated_singleton(sng, step); +} + static void print_idl(struct ovsdb_idl *idl, int step) { const struct idltest_simple *s; const struct idltest_link1 *l1; const struct idltest_link2 *l2; + const struct idltest_singleton *sng; int n = 0; IDLTEST_SIMPLE_FOR_EACH (s, idl) { @@ -1994,6 +2023,10 @@ print_idl(struct ovsdb_idl *idl, int step) print_idl_row_link2(l2, step); n++; } + IDLTEST_SINGLETON_FOR_EACH (sng, idl) { + print_idl_row_singleton(sng, step); + n++; + } if (!n) { printf("%03d: empty\n", step); } diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py index 6ed3949e7..ec6035447 100644 --- a/tests/test-ovsdb.py +++ b/tests/test-ovsdb.py @@ -240,6 +240,16 @@ def print_idl(idl, step): print(''.join(s)) n += 1 + if "singleton" in idl.tables: + sng = idl.tables["singleton"].rows + for row in six.itervalues(sng): + s = ["%03d:" % step] + s.append(" name=%s" % row.name) + if hasattr(row, "uuid"): + s.append(" uuid=%s" % row.uuid) + print(''.join(s)) + n += 1 + if not n: print("%03d: empty" % step) sys.stdout.flush()
When inserting data into a "singleton" table (one that has maxRows == 1), there is a check that ensures that the table is currently empty before inserting the row. The intention is to prevent races where multiple clients might attempt to insert rows at the same time. The problem is that this singleton check can cause legitimate transactions to fail. Specifically, a transaction that attempts to delete the current content of the table and insert new data will cause the singleton check to fail since the table currently has data. This patch corrects the issue by keeping a count of the rows being deleted and added to singleton tables. If the total is larger than zero, then the net operation is attempting to insert rows. If the total is less than zero, then the net operation is attempting to remove rows. If the total is zero, then the operation is inserting and deleting an equal number of rows (or is just updating rows). We only add the singleton check if the total is larger than zero. This patch also includes a new test for singleton tables that ensures that the maxRows constraint works as expected. Signed-off-by: Mark Michelson <mmichels@redhat.com> --- lib/ovsdb-idl.c | 55 ++++++++++++++++++++++++++++++++++++------------- tests/idltest.ovsschema | 9 ++++++++ tests/ovsdb-idl.at | 28 ++++++++++++++++++++++++- tests/test-ovsdb.c | 33 +++++++++++++++++++++++++++++ tests/test-ovsdb.py | 10 +++++++++ 5 files changed, 120 insertions(+), 15 deletions(-)