[ovs-dev] Let's talk the NB DB IDL Part I - things we've see scaling the networking-ovn to NB DB connection

[Ryan Moats]

"dev" <dev-bounces at openvswitch.org> wrote on 08/02/2016 10:56:07 PM:

> From: Ryan Moats/Omaha/IBM at IBMUS
> To: Ben Pfaff <blp at ovn.org>
> Cc: ovs-dev <dev at openvswitch.org>
> Date: 08/02/2016 10:56 PM
> Subject: Re: [ovs-dev] Let's talk the NB DB IDL Part I - things
> we've see scaling the networking-ovn to NB DB connection
> Sent by: "dev" <dev-bounces at openvswitch.org>
>
> Ben Pfaff <blp at ovn.org> wrote on 08/02/2016 10:14:46 PM:
>
> > From: Ben Pfaff <blp at ovn.org>
> > To: Ryan Moats/Omaha/IBM at IBMUS
> > Cc: ovs-dev <dev at openvswitch.org>
> > Date: 08/02/2016 10:15 PM
> > Subject: Re: [ovs-dev] Let's talk the NB DB IDL Part I - things
> > we've see scaling the networking-ovn to NB DB connection
> >
> > On Tue, Aug 02, 2016 at 04:25:03PM -0500, Ryan Moats wrote:
> > > First, this code from ./ovsdb/execution.c (L693-707) looks
> > > a bit funny:
> > >
> > > if (!strcmp(json_string(until), "==") != equal) {
> > >     if (timeout && x->elapsed_msec >= timeout_msec) {
> > >         if (x->elapsed_msec) {
> > >             error = ovsdb_error("timed out",
> > >                                 "\"wait\" timed out after %lld ms",
> > >                                 x->elapsed_msec);
> > >         } else {
> > >             error = ovsdb_error("timed out", "\"wait\" timed out");
> > >         }
> > >     } else {
> > >         /* ovsdb_execute() will change this, if triggers really are
> > >          * supported. */
> > >         error = ovsdb_error("not supported", "triggers not
supported");
> > >     }
> > > }
> > >
> > > Specifically, returning a message for "timed out" when a
> > > timeout of 0 seconds has been specified and the conditional
> > > has not matched is misleading at best. I think it would make
> > > more sense to say "wait condition not met" or something like
> > > that.
> >
> > The second string in the ovsdb_error() call can be whatever we want,
and
> > if you think that something else is a better explanation then I'm happy
> > to take it, but RFC 7047 requires the string "timed out" for the error
> > type (see section 5.2.6 "Wait").
>
> It was the second string that was bothering me - I get the first is
> fixed by the RFC.  I'll spin a patch suggesting a change tomorrow.
>
> > > The second issue is a bit more serious.  Right now, before the
> > > networking-ovn IDL will add a new logical switch port and
> > > associated ACLs (and if my memory serves me, it creates seven
> > > for each port), there are a pair of wait clauses that have to
> > > be met.  The first of these is that the ports and ACLs for
> > > the Logical Switch have to have a particular list of UUIDs
> > > in each, and each entire list is sent in the transaction.
> > >
> > > Is this strictly necessary?  I ask because I'm looking at
> > > scaling to a point where the wait condition will have
> > > 8000 uuids for the logical switch ports part and
> > > 56000 uuids for the ACLs part and I haven't yet figured
> > > out why it is needed...
> >
> > Presumably this means that networking-ovn is calling "verify" on the
> > column in question.  Probably, networking-ovn should use the partial
map
> > update functionality introduced in commit f199df26e8e28 "ovsdb-idl: Add
> > partial map updates functionality."  I don't know whether it's in the
> > Python IDL yet.
>
> Indeed they are and thanks for the pointer to the commit - I'll dig
> into it tomorrow and see if that code is reflected in the Python
> IDL via that or another commit.  If it is, great.  If not, there
> will likely also be a patch adding it so that we can move along.

Hmm, maybe I'm misreading something, but I don't thing that's going
to work without some additional modifications - the partial map commit
currently codes for columns that have a particular value type defined
by the schema.  The problem we are seeing is with the ports and acls
columns of the logical switch table, which are lists of strong
references.  Since they don't have a defined value, the generated IDL
code doesn't provide hooks for using partial map operations and we default
back to update/verify with the given above results.

Now, I think this an oversight, because I can argue that since these
are strong references, I should be able to use partial maps to update
them as keys with a null value.  Does this make sense or am I breaking
something if I look at going this route?