Message ID | 20210819004247.3369975-1-numans@ovn.org |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev] ovsdb-idl : Add APIs to query if a table and a column is present or not. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Wed, Aug 18, 2021 at 8:43 PM <numans@ovn.org> wrote: > > From: Numan Siddique <nusiddiq@redhat.com> > > This patch adds 2 new APIs in the ovsdb-idl client library > - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to > query if a table and a column is present in the IDL or not. This > is required for scenarios where the server schema is old and > missing a table or column and the client (built with a new schema > version) does a transaction with the missing table or column. This > results in a continuous loop of transaction failures. > > OVN would require the API - ovsdb_idl_has_table() to address this issue > when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' > table missing. A recent commit in OVN creates a 'datapath' row > in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be > useful to have. > > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > --- > lib/ovsdb-idl.c | 25 +++++++++++++++++++++++++ > lib/ovsdb-idl.h | 4 ++++ > 2 files changed, 29 insertions(+) > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index 2198c69c6..c5795fb7f 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -4256,3 +4256,28 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop) > > return retval; > } > + > +bool > +ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name) > +{ > + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, > + table_name); > + return table ? true: false; > +} > + > +bool > +ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name, > + const char *column_name) > +{ > + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, > + table_name); > + if (!table) { > + return false; > + } > + > + if (shash_find_data(&table->columns, column_name)) { > + return true; > + } > + > + return false; Why not use ovsdb_idl_has_table you created and make this function more concise? I would have done: if (ovsdb_idl_has_table(idl, table_name) && shash_find_data(&table->columns, column_name)) return true; return false; I dont know if this breaks any coding conventions though :) Otherwise LGTM > +} > diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h > index d93483245..216db4c6d 100644 > --- a/lib/ovsdb-idl.h > +++ b/lib/ovsdb-idl.h > @@ -474,6 +474,10 @@ void ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *); > > struct ovsdb_idl_row *ovsdb_idl_cursor_data(struct ovsdb_idl_cursor *); > > +bool ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name); > +bool ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, > + const char *table_name, > + const char *column_name); > #ifdef __cplusplus > } > #endif > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Aug 20, 2021 at 10:23 AM Michael Santana <msantana@redhat.com> wrote: > > On Wed, Aug 18, 2021 at 8:43 PM <numans@ovn.org> wrote: > > > > From: Numan Siddique <nusiddiq@redhat.com> > > > > This patch adds 2 new APIs in the ovsdb-idl client library > > - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to > > query if a table and a column is present in the IDL or not. This > > is required for scenarios where the server schema is old and > > missing a table or column and the client (built with a new schema > > version) does a transaction with the missing table or column. This > > results in a continuous loop of transaction failures. > > > > OVN would require the API - ovsdb_idl_has_table() to address this issue > > when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' > > table missing. A recent commit in OVN creates a 'datapath' row > > in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be > > useful to have. > > > > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > --- > > lib/ovsdb-idl.c | 25 +++++++++++++++++++++++++ > > lib/ovsdb-idl.h | 4 ++++ > > 2 files changed, 29 insertions(+) > > > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > index 2198c69c6..c5795fb7f 100644 > > --- a/lib/ovsdb-idl.c > > +++ b/lib/ovsdb-idl.c > > @@ -4256,3 +4256,28 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop) > > > > return retval; > > } > > + > > +bool > > +ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name) > > +{ > > + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, > > + table_name); > > + return table ? true: false; > > +} > > + > > +bool > > +ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name, > > + const char *column_name) > > +{ > > + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, > > + table_name); > > + if (!table) { > > + return false; > > + } > > + > > + if (shash_find_data(&table->columns, column_name)) { > > + return true; > > + } > > + > > + return false; > Why not use ovsdb_idl_has_table you created and make this function > more concise? I would have done: > > if (ovsdb_idl_has_table(idl, table_name) && > shash_find_data(&table->columns, column_name)) Thanks for the reviews. This would result in one extra lookup in the shash right ? > return true; > return false; > > I dont know if this breaks any coding conventions though :) Just using ovsdb_idl_has_table() would not be sufficient right ? We still need to get the table from the 'idl->table_from_name' shash. I thought about doing something like below ----------------- static struct ovsdb_idl_table* ovsdb_idl_get_table(struct ovsdb_idl *idl, const char *table_name) { return shash_find_data(&idl->table_by_name, table_name); } bool ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name) { return ovsdb_idl_get_table(idl, table_name) ? true: false; } bool ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name, const char *column_name) { struct ovsdb_idl_table *table = ovsdb_idl_get_table(idl, table_name); if (table && shash_find_data(&table->columns, column_name)) { return true; } return false; } ------------------------ Then I dropped it as this would just add an extra function overhead just to do 'shash_find_data()'. I'm fine if this improves readability. Thanks Numan > > Otherwise LGTM > > +} > > diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h > > index d93483245..216db4c6d 100644 > > --- a/lib/ovsdb-idl.h > > +++ b/lib/ovsdb-idl.h > > @@ -474,6 +474,10 @@ void ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *); > > > > struct ovsdb_idl_row *ovsdb_idl_cursor_data(struct ovsdb_idl_cursor *); > > > > +bool ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name); > > +bool ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, > > + const char *table_name, > > + const char *column_name); > > #ifdef __cplusplus > > } > > #endif > > -- > > 2.31.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Aug 18, 2021 at 08:42:47PM -0400, numans@ovn.org wrote: > From: Numan Siddique <nusiddiq@redhat.com> > > This patch adds 2 new APIs in the ovsdb-idl client library > - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to > query if a table and a column is present in the IDL or not. This > is required for scenarios where the server schema is old and > missing a table or column and the client (built with a new schema > version) does a transaction with the missing table or column. This > results in a continuous loop of transaction failures. > > OVN would require the API - ovsdb_idl_has_table() to address this issue > when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' > table missing. A recent commit in OVN creates a 'datapath' row > in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be > useful to have. > > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> In the OVN meeting, I suggested that this is a bug in the ovsdb_idl code for transactions: it shouldn't try to transact on a column that it knows doesn't exist. It might still make sense to add this API, though.
On Fri, Aug 20, 2021 at 12:43 PM Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Aug 18, 2021 at 08:42:47PM -0400, numans@ovn.org wrote: > > From: Numan Siddique <nusiddiq@redhat.com> > > > > This patch adds 2 new APIs in the ovsdb-idl client library > > - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to > > query if a table and a column is present in the IDL or not. This > > is required for scenarios where the server schema is old and > > missing a table or column and the client (built with a new schema > > version) does a transaction with the missing table or column. This > > results in a continuous loop of transaction failures. > > > > OVN would require the API - ovsdb_idl_has_table() to address this issue > > when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' > > table missing. A recent commit in OVN creates a 'datapath' row > > in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be > > useful to have. > > > > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > In the OVN meeting, I suggested that this is a bug in the ovsdb_idl code > for transactions: it shouldn't try to transact on a column that it knows > doesn't exist. It might still make sense to add this API, though. Thanks. I'll look into that and propose a patch for the transaction issue. Meanwhile this is required in OVN so that ovn-controller can avoid creating transactions for missing tables/columns. So it would be great if this patch can be considered before I propose the transaction patch. Thanks Numan > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Aug 20, 2021 at 1:49 PM Numan Siddique <numans@ovn.org> wrote: > > On Fri, Aug 20, 2021 at 12:43 PM Ben Pfaff <blp@ovn.org> wrote: > > > > On Wed, Aug 18, 2021 at 08:42:47PM -0400, numans@ovn.org wrote: > > > From: Numan Siddique <nusiddiq@redhat.com> > > > > > > This patch adds 2 new APIs in the ovsdb-idl client library > > > - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to > > > query if a table and a column is present in the IDL or not. This > > > is required for scenarios where the server schema is old and > > > missing a table or column and the client (built with a new schema > > > version) does a transaction with the missing table or column. This > > > results in a continuous loop of transaction failures. > > > > > > OVN would require the API - ovsdb_idl_has_table() to address this issue > > > when an old ovsdb-server is used (OVS 2.11) which has the 'datapath' > > > table missing. A recent commit in OVN creates a 'datapath' row > > > in the local ovsdb-server. ovsdb_idl_has_column_in_table() would be > > > useful to have. > > > > > > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705 > > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > > > In the OVN meeting, I suggested that this is a bug in the ovsdb_idl code > > for transactions: it shouldn't try to transact on a column that it knows > > doesn't exist. It might still make sense to add this API, though. > > Thanks. I'll look into that and propose a patch for the transaction issue. > Meanwhile this is required in OVN so that ovn-controller can avoid creating > transactions for missing tables/columns. > > So it would be great if this patch can be considered before I propose > the transaction patch. I'll submit v2 addressing the transaction handling issue in some time. so please ignore v2. Thanks Numan > > Thanks > Numan > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 2198c69c6..c5795fb7f 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -4256,3 +4256,28 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop) return retval; } + +bool +ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name) +{ + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, + table_name); + return table ? true: false; +} + +bool +ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name, + const char *column_name) +{ + struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name, + table_name); + if (!table) { + return false; + } + + if (shash_find_data(&table->columns, column_name)) { + return true; + } + + return false; +} diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h index d93483245..216db4c6d 100644 --- a/lib/ovsdb-idl.h +++ b/lib/ovsdb-idl.h @@ -474,6 +474,10 @@ void ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *); struct ovsdb_idl_row *ovsdb_idl_cursor_data(struct ovsdb_idl_cursor *); +bool ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name); +bool ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, + const char *table_name, + const char *column_name); #ifdef __cplusplus } #endif