On Tue, Mar 31, 2015 at 12:37:11PM -0700, Alex Wang wrote:
> Thx Ben for fixing this!~ Have few comments:
>
> @@ -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);
> >
> >
> Could this be simplified as follow?
>
> @@ -562,12 +699,31 @@ 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);
> +
> + if (!table_schema) {
> + VLOG_WARN("%s database lacks %s table "
> + "(database needs upgrade?)",
> + idl->class->database, table->class->name);
> + continue;
> + }
> +
> 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 (!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 the IDL isn't configured to monitor any of the columns in a table
at all, and table->need_table is false, then I don't want to log
anything at all, because the IDL doesn't care about that table at all;
that's why it's written the way I did it.
> > @@ -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;
> >
>
>
> I'm not very sure about this. Since when the idl is first created,
> idl->state
> will be 0 (IDL_S_SCHEMA_REQUESTED). So, the very first call to
> ovsdb_idl_update_has_lock() will not update the change_seqno. And this
> could be problematic?
A transition to IDL_S_MONITORING will increment change_seqno (right?),
so there's no need to increment it here unless we're already in that
state. A bit stronger: we should not increment here unless we're in
that state, because clients assume that when change_seqno is nonzero
that the database has been populated.
> > 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
> >
> >
> Test seems really complicated, but looks good to me~
Yeah, sorry about the complication.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
