Until now, if ovs-vsctl (or another client of the C ovsdb-idl library) was
compiled against a schema that had a column or table that was not in the
database actually being used (e.g. during an upgrade), and the column or
table was selected for monitoring, then ovsdb-idl would fail to get any
data at all because ovsdb-server would report an error due to a request
about a column or a table it didn't know about.
This commit fixes the problem by making ovsdb-idl retrieve the database
schema from the database server and omit any tables or columns that don't
exist from its monitoring request. This works OK for the kinds of upgrades
that OVSDB otherwise supports gracefully because it will simply make the
missing columns or tables appear empty, which clients of the ovsdb-idl
library already have to tolerate.
VMware-BZ: #1413562
Reported-by: Alex Wang <al...@nicira.com>
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
lib/ovsdb-idl.c | 207 +++++++++++++++++++++++++++++++++++++++++------
lib/ovsdb-parser.c | 16 ++--
lib/ovsdb-parser.h | 5 +-
tests/automake.mk | 2 +-
tests/idltest2.ovsschema | 85 +++++++++++++++++++
tests/ovsdb-idl.at | 75 +++++++++++++++++
6 files changed, 358 insertions(+), 32 deletions(-)
create mode 100644 tests/idltest2.ovsschema
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 9c25dbc..b35e854 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -28,11 +28,15 @@
#include "fatal-signal.h"
#include "json.h"
#include "jsonrpc.h"
+#include "ovsdb/ovsdb.h"
+#include "ovsdb/table.h"
#include "ovsdb-data.h"
#include "ovsdb-error.h"
#include "ovsdb-idl-provider.h"
+#include "ovsdb-parser.h"
#include "poll-loop.h"
#include "shash.h"
+#include "sset.h"
#include "util.h"
#include "openvswitch/vlog.h"
@@ -71,16 +75,25 @@ struct ovsdb_idl_arc {
struct ovsdb_idl_row *dst; /* Destination row. */
};
+enum ovsdb_idl_state {
+ IDL_S_SCHEMA_REQUESTED,
+ IDL_S_MONITOR_REQUESTED,
+ IDL_S_MONITORING
+};
+
struct ovsdb_idl {
const struct ovsdb_idl_class *class;
struct jsonrpc_session *session;
struct shash table_by_name;
struct ovsdb_idl_table *tables; /* Contains "struct ovsdb_idl_table *"s.*/
- struct json *monitor_request_id;
- unsigned int last_monitor_request_seqno;
unsigned int change_seqno;
bool verify_write_only;
+ /* Session state. */
+ unsigned int state_seqno;
+ enum ovsdb_idl_state state;
+ struct json *request_id;
+
/* Database locking. */
char *lock_name; /* Name of lock we need, NULL if none. */
bool has_lock; /* Has db server told us we have the lock? */
@@ -124,7 +137,9 @@ static struct vlog_rate_limit syntax_rl =
VLOG_RATE_LIMIT_INIT(1, 5);
static struct vlog_rate_limit semantic_rl = VLOG_RATE_LIMIT_INIT(1, 5);
static void ovsdb_idl_clear(struct ovsdb_idl *);
-static void ovsdb_idl_send_monitor_request(struct ovsdb_idl *);
+static void ovsdb_idl_send_schema_request(struct ovsdb_idl *);
+static void ovsdb_idl_send_monitor_request(struct ovsdb_idl *,
+ const struct json *schema_json);
static void ovsdb_idl_parse_update(struct ovsdb_idl *, const struct json *);
static struct ovsdb_error *ovsdb_idl_parse_update__(struct ovsdb_idl *,
const struct json *);
@@ -214,7 +229,10 @@ ovsdb_idl_create(const char *remote, const struct
ovsdb_idl_class *class,
hmap_init(&table->rows);
table->idl = idl;
}
- idl->last_monitor_request_seqno = UINT_MAX;
+
+ idl->state_seqno = UINT_MAX;
+ idl->request_id = NULL;
+
hmap_init(&idl->outstanding_txns);
return idl;
@@ -239,7 +257,7 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl)
}
shash_destroy(&idl->table_by_name);
free(idl->tables);
- json_destroy(idl->monitor_request_id);
+ json_destroy(idl->request_id);
free(idl->lock_name);
json_destroy(idl->lock_request_id);
hmap_destroy(&idl->outstanding_txns);
@@ -298,14 +316,17 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
unsigned int seqno;
seqno = jsonrpc_session_get_seqno(idl->session);
- if (idl->last_monitor_request_seqno != seqno) {
- idl->last_monitor_request_seqno = seqno;
+ if (idl->state_seqno != seqno) {
+ idl->state_seqno = seqno;
+ json_destroy(idl->request_id);
+ idl->request_id = NULL;
ovsdb_idl_txn_abort_all(idl);
- ovsdb_idl_send_monitor_request(idl);
+
+ ovsdb_idl_send_schema_request(idl);
+ idl->state = IDL_S_SCHEMA_REQUESTED;
if (idl->lock_name) {
ovsdb_idl_send_lock_request(idl);
}
- break;
}
msg = jsonrpc_session_recv(idl->session);
@@ -321,14 +342,31 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
/* Database contents changed. */
ovsdb_idl_parse_update(idl, msg->params->u.array.elems[1]);
} else if (msg->type == JSONRPC_REPLY
- && idl->monitor_request_id
- && json_equal(idl->monitor_request_id, msg->id)) {
- /* Reply to our "monitor" request. */
- idl->change_seqno++;
- json_destroy(idl->monitor_request_id);
- idl->monitor_request_id = NULL;
- ovsdb_idl_clear(idl);
- ovsdb_idl_parse_update(idl, msg->result);
+ && idl->request_id
+ && json_equal(idl->request_id, msg->id)) {
+ switch (idl->state) {
+ case IDL_S_SCHEMA_REQUESTED:
+ /* Reply to our "get_schema" request. */
+ json_destroy(idl->request_id);
+ idl->request_id = NULL;
+ ovsdb_idl_send_monitor_request(idl, msg->result);
+ idl->state = IDL_S_MONITOR_REQUESTED;
+ break;
+
+ case IDL_S_MONITOR_REQUESTED:
+ /* Reply to our "monitor" request. */
+ idl->change_seqno++;
+ json_destroy(idl->request_id);
+ idl->request_id = NULL;
+ idl->state = IDL_S_MONITORING;
+ ovsdb_idl_clear(idl);
+ ovsdb_idl_parse_update(idl, msg->result);
+ break;
+
+ case IDL_S_MONITORING:
+ default:
+ OVS_NOT_REACHED();
+ }
} else if (msg->type == JSONRPC_REPLY
&& idl->lock_request_id
&& json_equal(idl->lock_request_id, msg->id)) {
@@ -551,8 +589,107 @@ ovsdb_idl_omit(struct ovsdb_idl *idl, const struct
ovsdb_idl_column *column)
}
static void
-ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl)
+ovsdb_idl_send_schema_request(struct ovsdb_idl *idl)
+{
+ struct jsonrpc_msg *msg;
+
+ json_destroy(idl->request_id);
+ msg = jsonrpc_create_request(
+ "get_schema",
+ json_array_create_1(json_string_create(idl->class->database)),
+ &idl->request_id);
+ jsonrpc_session_send(idl->session, msg);
+}
+
+static void
+log_error(struct ovsdb_error *error)
+{
+ char *s = ovsdb_error_to_string(error);
+ VLOG_WARN("error parsing database schema: %s", s);
+ free(s);
+ ovsdb_error_destroy(error);
+}
+
+/* Frees 'schema', which is in the format returned by parse_schema(). */
+static void
+free_schema(struct shash *schema)
+{
+ if (schema) {
+ struct shash_node *node, *next;
+
+ SHASH_FOR_EACH_SAFE (node, next, schema) {
+ struct sset *sset = node->data;
+ sset_destroy(sset);
+ free(sset);
+ shash_delete(schema, node);
+ }
+ shash_destroy(schema);
+ free(schema);
+ }
+}
+
+/* Parses 'schema_json', an OVSDB schema in JSON format as described in RFC
+ * 7047, to obtain the names of its rows and columns. If successful, returns
+ * an shash whose keys are table names and whose values are ssets, where each
+ * sset contains the names of its table's columns. On failure (due to a parse
+ * error), returns NULL.
+ *
+ * It would also be possible to use the general-purpose OVSDB schema parser in
+ * ovsdb-server, but that's overkill, possibly too strict for the current use
+ * case, and would require restructuring ovsdb-server to separate the schema
+ * code from the rest. */
+static struct shash *
+parse_schema(const struct json *schema_json)
+{
+ struct ovsdb_parser parser;
+ const struct json *tables_json;
+ struct ovsdb_error *error;
+ struct shash_node *node;
+ struct shash *schema;
+
+ ovsdb_parser_init(&parser, schema_json, "database schema");
+ tables_json = ovsdb_parser_member(&parser, "tables", OP_OBJECT);
+ error = ovsdb_parser_destroy(&parser);
+ if (error) {
+ log_error(error);
+ return NULL;
+ }
+
+ schema = xmalloc(sizeof *schema);
+ shash_init(schema);
+ SHASH_FOR_EACH (node, json_object(tables_json)) {
+ const char *table_name = node->name;
+ const struct json *json = node->data;
+ const struct json *columns_json;
+
+ ovsdb_parser_init(&parser, json, "table schema for table %s",
+ table_name);
+ columns_json = ovsdb_parser_member(&parser, "columns", OP_OBJECT);
+ error = ovsdb_parser_destroy(&parser);
+ if (error) {
+ log_error(error);
+ free_schema(schema);
+ return NULL;
+ }
+
+ struct sset *columns = xmalloc(sizeof *columns);
+ sset_init(columns);
+
+ struct shash_node *node2;
+ SHASH_FOR_EACH (node2, json_object(columns_json)) {
+ const char *column_name = node2->name;
+ sset_add(columns, column_name);
+ }
+ shash_add(schema, table_name, columns);
+ }
+ return schema;
+}
+
+static void
+ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl,
+ const struct json *schema_json)
{
+ struct shash *schema = parse_schema(schema_json);
struct json *monitor_requests;
struct jsonrpc_msg *msg;
size_t i;
@@ -562,12 +699,25 @@ ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl)
const struct ovsdb_idl_table *table = &idl->tables[i];
const struct ovsdb_idl_table_class *tc = table->class;
struct json *monitor_request, *columns;
+ const struct sset *table_schema;
size_t j;
+ table_schema = (schema
+ ? shash_find_data(schema, table->class->name)
+ : NULL);
+
columns = table->need_table ? json_array_create_empty() : NULL;
for (j = 0; j < tc->n_columns; j++) {
const struct ovsdb_idl_column *column = &tc->columns[j];
if (table->modes[j] & OVSDB_IDL_MONITOR) {
+ if (table_schema
+ && !sset_contains(table_schema, column->name)) {
+ VLOG_WARN("%s table in %s database lacks %s column "
+ "(database needs upgrade?)",
+ table->class->name, idl->class->database,
+ column->name);
+ continue;
+ }
if (!columns) {
columns = json_array_create_empty();
}
@@ -576,18 +726,27 @@ ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl)
}
if (columns) {
+ if (schema && !table_schema) {
+ VLOG_WARN("%s database lacks %s table "
+ "(database needs upgrade?)",
+ idl->class->database, table->class->name);
+ json_destroy(columns);
+ continue;
+ }
+
monitor_request = json_object_create();
json_object_put(monitor_request, "columns", columns);
json_object_put(monitor_requests, tc->name, monitor_request);
}
}
+ free_schema(schema);
- json_destroy(idl->monitor_request_id);
+ json_destroy(idl->request_id);
msg = jsonrpc_create_request(
"monitor",
json_array_create_3(json_string_create(idl->class->database),
json_null_create(), monitor_requests),
- &idl->monitor_request_id);
+ &idl->request_id);
jsonrpc_session_send(idl->session, msg);
}
@@ -2385,11 +2544,11 @@ static void
ovsdb_idl_update_has_lock(struct ovsdb_idl *idl, bool new_has_lock)
{
if (new_has_lock && !idl->has_lock) {
- if (!idl->monitor_request_id) {
+ if (idl->state == IDL_S_MONITORING) {
idl->change_seqno++;
} else {
- /* We're waiting for a monitor reply, so don't signal that the
- * database changed. The monitor reply will increment change_seqno
+ /* We're setting up a session, so don't signal that the database
+ * changed. Finalizing the session will increment change_seqno
* anyhow. */
}
idl->is_lock_contended = false;
diff --git a/lib/ovsdb-parser.c b/lib/ovsdb-parser.c
index f4642ad..3e44833 100644
--- a/lib/ovsdb-parser.c
+++ b/lib/ovsdb-parser.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2011, 2013 Nicira, Inc.
+/* Copyright (c) 2009, 2011, 2013, 2015 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -127,6 +127,15 @@ ovsdb_parser_has_error(const struct ovsdb_parser *parser)
}
struct ovsdb_error *
+ovsdb_parser_destroy(struct ovsdb_parser *parser)
+{
+ free(parser->name);
+ sset_destroy(&parser->used);
+
+ return parser->error;
+}
+
+struct ovsdb_error *
ovsdb_parser_finish(struct ovsdb_parser *parser)
{
if (!parser->error) {
@@ -157,8 +166,5 @@ ovsdb_parser_finish(struct ovsdb_parser *parser)
}
}
- free(parser->name);
- sset_destroy(&parser->used);
-
- return parser->error;
+ return ovsdb_parser_destroy(parser);
}
diff --git a/lib/ovsdb-parser.h b/lib/ovsdb-parser.h
index 8df9d52..2f9f483 100644
--- a/lib/ovsdb-parser.h
+++ b/lib/ovsdb-parser.h
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2015 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -72,7 +72,8 @@ bool ovsdb_parser_has_error(const struct ovsdb_parser *);
struct ovsdb_error *ovsdb_parser_get_error(const struct ovsdb_parser *);
struct ovsdb_error *ovsdb_parser_finish(struct ovsdb_parser *)
OVS_WARN_UNUSED_RESULT;
-void ovsdb_parser_destroy(struct ovsdb_parser *);
+struct ovsdb_error *ovsdb_parser_destroy(struct ovsdb_parser *)
+ OVS_WARN_UNUSED_RESULT;
bool ovsdb_parser_is_id(const char *string);
diff --git a/tests/automake.mk b/tests/automake.mk
index abbfcb5..c691cf9 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -235,7 +235,7 @@ tests_test_lib_LDADD = lib/libopenvswitch.la
# idltest schema and IDL
OVSIDL_BUILT += tests/idltest.c tests/idltest.h tests/idltest.ovsidl
IDLTEST_IDL_FILES = tests/idltest.ovsschema tests/idltest.ann
-EXTRA_DIST += $(IDLTEST_IDL_FILES)
+EXTRA_DIST += $(IDLTEST_IDL_FILES) tests/idltest2.ovsschema
tests/idltest.ovsidl: $(IDLTEST_IDL_FILES)
$(AM_V_GEN)$(OVSDB_IDLC) -C $(srcdir) annotate $(IDLTEST_IDL_FILES) >
$@.tmp && \
mv $@.tmp $@
diff --git a/tests/idltest2.ovsschema b/tests/idltest2.ovsschema
new file mode 100644
index 0000000..312c9cc
--- /dev/null
+++ b/tests/idltest2.ovsschema
@@ -0,0 +1,85 @@
+{
+ "name": "idltest",
+ "version": "1.2.3",
+ "tables": {
+ "link1": {
+ "columns": {
+ "i": {
+ "type": "integer"
+ },
+ "k": {
+ "type": {
+ "key": {
+ "type": "uuid",
+ "refTable": "link1"
+ }
+ }
+ },
+ "ka": {
+ "type": {
+ "key": {
+ "type": "uuid",
+ "refTable": "link1"
+ },
+ "max": "unlimited",
+ "min": 0
+ }
+ }
+ }
+ },
+ "simple": {
+ "columns": {
+ "b": {
+ "type": "boolean"
+ },
+ "ba": {
+ "type": {
+ "key": "boolean",
+ "max": 1,
+ "min": 0
+ }
+ },
+ "i": {
+ "type": "integer"
+ },
+ "ia": {
+ "type": {
+ "key": "integer",
+ "max": "unlimited",
+ "min": 0
+ }
+ },
+ "r": {
+ "type": "real"
+ },
+ "ra": {
+ "type": {
+ "key": "real",
+ "max": "unlimited",
+ "min": 0
+ }
+ },
+ "s": {
+ "type": "string"
+ },
+ "sa": {
+ "type": {
+ "key": "string",
+ "max": "unlimited",
+ "min": 0
+ }
+ },
+ "u": {
+ "type": "uuid"
+ },
+ "ua": {
+ "type": {
+ "key": "uuid",
+ "max": "unlimited",
+ "min": 0
+ }
+ }
+ }
+ }
+ }
+}
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index 89752f0..57642be 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -496,3 +496,78 @@ OVSDB_CHECK_IDL_PY([getattr idl, insert ops],
002: i=2 k=2 ka=[] l2= uuid=<0>
003: done
]])
+
+AT_SETUP([idl handling of missing tables and columns - C])
+AT_KEYWORDS([ovsdb server idl positive])
+OVS_RUNDIR=`pwd`; export OVS_RUNDIR
+
+# idltest2.ovsschema is the same as idltest.ovsschema, except that
+# table link2 and column l2 have been deleted. But the IDL still
+# expects them to be there, so this test checks that it properly
+# tolerates them being missing.
+AT_CHECK([ovsdb-tool create db $abs_srcdir/idltest2.ovsschema],
+ [0], [stdout], [ignore])
+AT_CHECK([ovsdb-server '-vPATTERN:console:ovsdb-server|%c|%m' --detach
--no-chdir --pidfile="`pwd`"/pid --remote=punix:socket
--unixctl="`pwd`"/unixctl db], [0], [ignore], [ignore])
+AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 idl
unix:socket ['["idltest",
+ {"op": "insert",
+ "table": "link1",
+ "row": {"i": 0, "k": ["named-uuid", "self"]},
+ "uuid-name": "self"}]' \
+ '["idltest",
+ {"op": "insert",
+ "table": "link1",
+ "row": {"i": 1, "k": ["named-uuid", "row2"]},
+ "uuid-name": "row1"},
+ {"op": "insert",
+ "table": "link1",
+ "row": {"i": 2, "k": ["named-uuid", "row1"]},
+ "uuid-name": "row2"}]' \
+ '["idltest",
+ {"op": "update",
+ "table": "link1",
+ "where": [["i", "==", 1]],
+ "row": {"k": ["uuid", "#1#"]}}]' \
+ '["idltest",
+ {"op": "update",
+ "table": "link1",
+ "where": [],
+ "row": {"k": ["uuid", "#0#"]}}]']],
+ [0], [stdout], [stderr], [kill `cat pid`])
+AT_CHECK([sort stdout | ${PERL} $srcdir/uuidfilt.pl], [0],
+ [[000: empty
+001: {"error":null,"result":[{"uuid":["uuid","<0>"]}]}
+002: i=0 k=0 ka=[] l2= uuid=<0>
+003: {"error":null,"result":[{"uuid":["uuid","<1>"]},{"uuid":["uuid","<2>"]}]}
+004: i=0 k=0 ka=[] l2= uuid=<0>
+004: i=1 k=2 ka=[] l2= uuid=<1>
+004: i=2 k=1 ka=[] l2= uuid=<2>
+005: {"error":null,"result":[{"count":1}]}
+006: i=0 k=0 ka=[] l2= uuid=<0>
+006: i=1 k=1 ka=[] l2= uuid=<1>
+006: i=2 k=1 ka=[] l2= uuid=<2>
+007: {"error":null,"result":[{"count":3}]}
+008: i=0 k=0 ka=[] l2= uuid=<0>
+008: i=1 k=0 ka=[] l2= uuid=<1>
+008: i=2 k=0 ka=[] l2= uuid=<2>
+009: done
+]], [], [kill `cat pid`])
+
+# Check that ovsdb-idl figured out that table link2 and column l2 are missing.
+AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl
+test-ovsdb|ovsdb_idl|idltest database lacks link2 table (database needs
upgrade?)
+test-ovsdb|ovsdb_idl|link1 table in idltest database lacks l2 column (database
needs upgrade?)
+])
+
+# Check that ovsdb-idl sent on "monitor" request and that it didn't
+# mention that table or column, and (for paranoia) that it did mention another
+# table and column.
+AT_CHECK([grep -c '"monitor"' stderr], [0], [1
+])
+AT_CHECK([grep '"monitor"' stderr | grep link2], [1])
+AT_CHECK([grep '"monitor"' stderr | grep l2], [1])
+AT_CHECK([grep '"monitor"' stderr | grep -c '"link1"'], [0], [1
+])
+AT_CHECK([grep '"monitor"' stderr | grep -c '"ua"'], [0], [1
+])
+OVSDB_SERVER_SHUTDOWN
+AT_CLEANUP
--
2.1.3
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
