Re: [ovs-dev] [PATCH 3/4] ovsdb-idl: Tolerate missing tables and columns.
On Tue, Mar 31, 2015 at 4:11 PM, Ben Pfaff wrote:
> 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.
>
>
Thx for the explanation, makes sense~ I'm good~
> > > @@ -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.
>
>
This is clear now, thx!
> > > 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?)
> > > +])
>
Re: [ovs-dev] [PATCH 3/4] ovsdb-idl: Tolerate missing tables and columns.
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:
Re: [ovs-dev] [PATCH 3/4] ovsdb-idl: Tolerate missing tables and columns.
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;
+}
> @@ -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?
> 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(
