From patchwork Thu May 17 17:16:55 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Michelson X-Patchwork-Id: 915611 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 40myf63Js3z9s0q for ; Fri, 18 May 2018 03:17:02 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 24446E9E; Thu, 17 May 2018 17:17:00 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 761A8F72 for ; Thu, 17 May 2018 17:16:58 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 15297A3 for ; Thu, 17 May 2018 17:16:57 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 408D2805A531 for ; Thu, 17 May 2018 17:16:56 +0000 (UTC) Received: from monae.redhat.com (ovpn-122-194.rdu2.redhat.com [10.10.122.194]) by smtp.corp.redhat.com (Postfix) with ESMTP id 077ED2023582 for ; Thu, 17 May 2018 17:16:55 +0000 (UTC) From: Mark Michelson To: dev@openvswitch.org Date: Thu, 17 May 2018 13:16:55 -0400 Message-Id: <20180517171655.20550-1-mmichels@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 17 May 2018 17:16:56 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 17 May 2018 17:16:56 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'mmichels@redhat.com' RCPT:'' X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [PATCH] ovsdb-idl: Correct singleton insert logic X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org 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 Acked-by: Mark Michelson --- 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(-) 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()