Re: [ovs-dev] [PATCH v4 2/2] ovsdb-idl: Exclude missing tables and columns in the transaction.
On Thu, Aug 26, 2021 at 6:23 AM Ilya Maximets wrote:
>
> On 8/25/21 5:13 AM, num...@ovn.org wrote:
> > From: Numan Siddique
> >
> > In the cases where the C idl client is compiled with a newer schema
> > and the ovsdb-server is running with older schema, the IDL clients
> > can included tables or columns in the transaction which are missing
> > in the server schema. ovsdb-server will reject the transaction,
> > but the IDL client keeps on trying the transaction resulting
> > in a continuous loop. This patch fixes this issue by excluding
> > them for the jsonrpc transaction message.
> >
> > This patch chose to exclude the missing tables/columns from the
> > transaction instead of failing the transaction (TXN_ERROR) for
> > the following reasons:
> >
> > 1. IDL clients will not come to know the exact reason for the
> > transaction failure.
> >
> > 2. IDL client may not know all the transaction objects added in
> > its loop before calling ovsdb_idl_loop_commit_and_wait().
> >
> > 3. If the client has to figure out the reason by checking the
> > presence of tables/columns using the APIs added in the
> > previous commit, it can very well exclude such tables/columns
> > from the transaction.
> >
> > Relevant test cases are added to cover this case.
> >
> > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705
> > Signed-off-by: Numan Siddique
> > ---
> > lib/ovsdb-idl.c | 16
> > tests/idltest.ovsschema | 9 +++
> > tests/idltest2.ovsschema | 7 ++
> > tests/ovsdb-idl.at | 37
> > tests/test-ovsdb.c | 53
> > 5 files changed, 122 insertions(+)
> >
> > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> > index b2dfff46c..404cfc75a 100644
> > --- a/lib/ovsdb-idl.c
> > +++ b/lib/ovsdb-idl.c
> > @@ -3084,6 +3084,16 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
> > HMAP_FOR_EACH (row, txn_node, >txn_rows) {
> > const struct ovsdb_idl_table_class *class = row->table->class_;
> >
> > +if (!row->table->in_server_schema) {
> > +/* The table is not present in the server schema. Do not
> > + * include it in the transaction. */
> > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +VLOG_WARN_RL(, "%s database lacks %s table, excluding from "
> > + "the txn.", row->table->idl->class_->database,
> > + row->table->class_->name);
> > +continue;
>
> This check is only for data, but we should, probably, exclude this
> table from prerequisits too.
In my opinion it will be complicated and we might miss out on conditions like
this if we add the inserted row to the txn->txn_rows if the row is for
the missing table.
So I thought of creating a new hmap in the txn struct called 'excluded_rows'.
Such rows will be added in this hmap and will be cleaned up when the
txn object is destroyed.
This is what is in my mind -
https://github.com/numansiddique/ovs/commit/3ac1db8eb4f14f9648ab19e9a57c3948364de613
I need to spend some more time on this to address this issue completely.
So I've dropped patch 2 from v6 for now. I'll submit this patch once I
test thoroughly.
Hope this is fine and patch 1 can be considered if it is good in a
shape (to unblock OVN :)).
If you've any comments on the above commit link I shared please do share.
Thanks for the reviews
Numan
>
> > +}
> > +
> > if (!row->new_datum) {
> > if (class->is_root) {
> > struct json *op = json_object_create();
> > @@ -3382,6 +3392,12 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row
> > *row_,
> > }
> >
> > class = row->table->class_;
> > +
> > +if (!ovsdb_idl_has_column_in_table(row->table->idl, class->name,
> > + column->name)) {
>
> Should we warn a user about missing columns? And again, do we need
> to add a check to the ovsdb_idl_txn_verify() ?
>
> Would be great to have a _verify() function in the test case too.
>
> > +goto discard_datum;
> > +}
> > +
> > column_idx = column - class->columns;
> > write_only = row->table->modes[column_idx] == OVSDB_IDL_MONITOR;
> >
> > diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
> > index 3ddb612b0..65eadb961 100644
> > --- a/tests/idltest.ovsschema
> > +++ b/tests/idltest.ovsschema
> > @@ -210,6 +210,15 @@
> >},
> >"isRoot": true
> > },
> > +"simple7" : {
> > + "columns" : {
> > +"name" : {
> > + "type": "string"
> > +},
> > +"id": {"type": "string"}
> > + },
> > + "isRoot" : true
> > +},
> > "singleton" : {
> >"columns" : {
> > "name" : {
> > diff --git a/tests/idltest2.ovsschema b/tests/idltest2.ovsschema
> > index 210e4c389..a8199e56c 100644
> > --- a/tests/idltest2.ovsschema
> >
Re: [ovs-dev] [PATCH v4 2/2] ovsdb-idl: Exclude missing tables and columns in the transaction.
On 8/25/21 5:13 AM, num...@ovn.org wrote:
> From: Numan Siddique
>
> In the cases where the C idl client is compiled with a newer schema
> and the ovsdb-server is running with older schema, the IDL clients
> can included tables or columns in the transaction which are missing
> in the server schema. ovsdb-server will reject the transaction,
> but the IDL client keeps on trying the transaction resulting
> in a continuous loop. This patch fixes this issue by excluding
> them for the jsonrpc transaction message.
>
> This patch chose to exclude the missing tables/columns from the
> transaction instead of failing the transaction (TXN_ERROR) for
> the following reasons:
>
> 1. IDL clients will not come to know the exact reason for the
> transaction failure.
>
> 2. IDL client may not know all the transaction objects added in
> its loop before calling ovsdb_idl_loop_commit_and_wait().
>
> 3. If the client has to figure out the reason by checking the
> presence of tables/columns using the APIs added in the
> previous commit, it can very well exclude such tables/columns
> from the transaction.
>
> Relevant test cases are added to cover this case.
>
> Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705
> Signed-off-by: Numan Siddique
> ---
> lib/ovsdb-idl.c | 16
> tests/idltest.ovsschema | 9 +++
> tests/idltest2.ovsschema | 7 ++
> tests/ovsdb-idl.at | 37
> tests/test-ovsdb.c | 53
> 5 files changed, 122 insertions(+)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index b2dfff46c..404cfc75a 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -3084,6 +3084,16 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
> HMAP_FOR_EACH (row, txn_node, >txn_rows) {
> const struct ovsdb_idl_table_class *class = row->table->class_;
>
> +if (!row->table->in_server_schema) {
> +/* The table is not present in the server schema. Do not
> + * include it in the transaction. */
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +VLOG_WARN_RL(, "%s database lacks %s table, excluding from "
> + "the txn.", row->table->idl->class_->database,
> + row->table->class_->name);
> +continue;
This check is only for data, but we should, probably, exclude this
table from prerequisits too.
> +}
> +
> if (!row->new_datum) {
> if (class->is_root) {
> struct json *op = json_object_create();
> @@ -3382,6 +3392,12 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
> }
>
> class = row->table->class_;
> +
> +if (!ovsdb_idl_has_column_in_table(row->table->idl, class->name,
> + column->name)) {
Should we warn a user about missing columns? And again, do we need
to add a check to the ovsdb_idl_txn_verify() ?
Would be great to have a _verify() function in the test case too.
> +goto discard_datum;
> +}
> +
> column_idx = column - class->columns;
> write_only = row->table->modes[column_idx] == OVSDB_IDL_MONITOR;
>
> diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
> index 3ddb612b0..65eadb961 100644
> --- a/tests/idltest.ovsschema
> +++ b/tests/idltest.ovsschema
> @@ -210,6 +210,15 @@
>},
>"isRoot": true
> },
> +"simple7" : {
> + "columns" : {
> +"name" : {
> + "type": "string"
> +},
> +"id": {"type": "string"}
> + },
> + "isRoot" : true
> +},
> "singleton" : {
>"columns" : {
> "name" : {
> diff --git a/tests/idltest2.ovsschema b/tests/idltest2.ovsschema
> index 210e4c389..a8199e56c 100644
> --- a/tests/idltest2.ovsschema
> +++ b/tests/idltest2.ovsschema
> @@ -139,6 +139,13 @@
>"type": "string"
> }
>}
> +},
> +"simple7" : {
> + "columns" : {
> +"name" : {
> + "type": "string"
> +}
> + }
> }
>}
> }
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index b11fabe70..26212f265 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -999,6 +999,7 @@ test-ovsdb|ovsdb_idl|idltest database lacks simple5 table
> (database needs upgrad
> test-ovsdb|ovsdb_idl|idltest database lacks simple6 table (database needs
> upgrade?)
> test-ovsdb|ovsdb_idl|idltest database lacks singleton table (database needs
> upgrade?)
> test-ovsdb|ovsdb_idl|link1 table in idltest database lacks l2 column
> (database needs upgrade?)
> +test-ovsdb|ovsdb_idl|simple7 table in idltest database lacks id column
> (database needs upgrade?)
> ])
>
> # Check that ovsdb-idl sent on "monitor" request and that it didn't
> @@ -2410,3 +2411,39 @@ column l2 in table link1 is present
>
> OVSDB_SERVER_SHUTDOWN
>
[ovs-dev] [PATCH v4 2/2] ovsdb-idl: Exclude missing tables and columns in the transaction.
From: Numan Siddique
In the cases where the C idl client is compiled with a newer schema
and the ovsdb-server is running with older schema, the IDL clients
can included tables or columns in the transaction which are missing
in the server schema. ovsdb-server will reject the transaction,
but the IDL client keeps on trying the transaction resulting
in a continuous loop. This patch fixes this issue by excluding
them for the jsonrpc transaction message.
This patch chose to exclude the missing tables/columns from the
transaction instead of failing the transaction (TXN_ERROR) for
the following reasons:
1. IDL clients will not come to know the exact reason for the
transaction failure.
2. IDL client may not know all the transaction objects added in
its loop before calling ovsdb_idl_loop_commit_and_wait().
3. If the client has to figure out the reason by checking the
presence of tables/columns using the APIs added in the
previous commit, it can very well exclude such tables/columns
from the transaction.
Relevant test cases are added to cover this case.
Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705
Signed-off-by: Numan Siddique
---
lib/ovsdb-idl.c | 16
tests/idltest.ovsschema | 9 +++
tests/idltest2.ovsschema | 7 ++
tests/ovsdb-idl.at | 37
tests/test-ovsdb.c | 53
5 files changed, 122 insertions(+)
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index b2dfff46c..404cfc75a 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -3084,6 +3084,16 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
HMAP_FOR_EACH (row, txn_node, >txn_rows) {
const struct ovsdb_idl_table_class *class = row->table->class_;
+if (!row->table->in_server_schema) {
+/* The table is not present in the server schema. Do not
+ * include it in the transaction. */
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "%s database lacks %s table, excluding from "
+ "the txn.", row->table->idl->class_->database,
+ row->table->class_->name);
+continue;
+}
+
if (!row->new_datum) {
if (class->is_root) {
struct json *op = json_object_create();
@@ -3382,6 +3392,12 @@ ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
}
class = row->table->class_;
+
+if (!ovsdb_idl_has_column_in_table(row->table->idl, class->name,
+ column->name)) {
+goto discard_datum;
+}
+
column_idx = column - class->columns;
write_only = row->table->modes[column_idx] == OVSDB_IDL_MONITOR;
diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
index 3ddb612b0..65eadb961 100644
--- a/tests/idltest.ovsschema
+++ b/tests/idltest.ovsschema
@@ -210,6 +210,15 @@
},
"isRoot": true
},
+"simple7" : {
+ "columns" : {
+"name" : {
+ "type": "string"
+},
+"id": {"type": "string"}
+ },
+ "isRoot" : true
+},
"singleton" : {
"columns" : {
"name" : {
diff --git a/tests/idltest2.ovsschema b/tests/idltest2.ovsschema
index 210e4c389..a8199e56c 100644
--- a/tests/idltest2.ovsschema
+++ b/tests/idltest2.ovsschema
@@ -139,6 +139,13 @@
"type": "string"
}
}
+},
+"simple7" : {
+ "columns" : {
+"name" : {
+ "type": "string"
+}
+ }
}
}
}
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index b11fabe70..26212f265 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -999,6 +999,7 @@ test-ovsdb|ovsdb_idl|idltest database lacks simple5 table
(database needs upgrad
test-ovsdb|ovsdb_idl|idltest database lacks simple6 table (database needs
upgrade?)
test-ovsdb|ovsdb_idl|idltest database lacks singleton table (database needs
upgrade?)
test-ovsdb|ovsdb_idl|link1 table in idltest database lacks l2 column (database
needs upgrade?)
+test-ovsdb|ovsdb_idl|simple7 table in idltest database lacks id column
(database needs upgrade?)
])
# Check that ovsdb-idl sent on "monitor" request and that it didn't
@@ -2410,3 +2411,39 @@ column l2 in table link1 is present
OVSDB_SERVER_SHUTDOWN
AT_CLEANUP
+
+AT_SETUP([idl transaction handling of missing tables and columns - C])
+AT_KEYWORDS([ovsdb client idl txn])
+
+# idltest2.ovsschema is the same as idltest.ovsschema, except that
+# few tables and columns are missing. This test checks that idl doesn't
+# include the missing tables and columns in the transactions.
+# idl-missing-table-column-txn inserts
+# - a row for table - 'simple'
+# - a row for table - 'simple5' which is missing. This should not be
+#included in the transaction.
+# - a row for table - 'simple7 with the missing column
