diff mbox series

[ovs-dev] ovsdb-idl : Add APIs to query if a table and a column is present or not.

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

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Numan Siddique Aug. 19, 2021, 12:42 a.m. UTC 
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(+)

Comments

Michael Santana Aug. 20, 2021, 2:23 p.m. UTC | #1 
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
>
Numan Siddique Aug. 20, 2021, 3:59 p.m. UTC | #2 
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
>
Ben Pfaff Aug. 20, 2021, 4:42 p.m. UTC | #3 
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.
Numan Siddique Aug. 20, 2021, 5:49 p.m. UTC | #4 
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
>
Numan Siddique Aug. 23, 2021, 4:13 p.m. UTC | #5 
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 mbox series

Patch

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