[ovs-dev] [PATCH] Copy external_ids from Logical_Switch_Port to SB database

[Daniel Alvarez Sanchez]

Thanks a lot Russell for taking the time to review this :)

On Wed, May 31, 2017 at 8:48 PM, Russell Bryant <russell at ovn.org> wrote:

> On Wed, May 31, 2017 at 12:18 PM, Daniel Alvarez <dalvarez at redhat.com>
> wrote:
> > This patch makes ovn-northd copy all string-string pairs in
> > external_ids column of the Logical_Switch_Port table in Northbound
> > database to the equivalent column of the Port_Binding table in
> > Southbound database.
> >
> > OpenStack Neutron will add some useful data to NB database that can be
> > later read by networking-ovn-metadata-agent without the need of
> > maintaining a connection to NB database. This data would include
> > the CIDR's of a port or the project and device ID's which are needed
> > when talking to Nova to request metadata.
> > ---
> >  ovn/northd/ovn-northd.c | 12 ++++++++----
> >  ovn/ovn-nb.xml          | 11 ++++++++++-
> >  2 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 5914988..d4c5f3a 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -1814,10 +1814,14 @@ ovn_port_update_sbrec(const struct ovn_port *op,
> >                                     op->nbsp->n_addresses);
> >
> >          struct smap ids = SMAP_INITIALIZER(&ids);
> > -        const char *name = smap_get(&op->nbsp->external_ids,
> > -                                    "neutron:port_name");
> > -        if (name && name[0]) {
> > -            smap_add(&ids, "name", name);
> > +        struct smap_node *id;
> > +        SMAP_FOR_EACH (id, &op->nbsp->external_ids) {
> > +            if(!strcmp(id->key, "neutron:port_name")) {
> > +                if(id->value[0])
> > +                    smap_add(&ids, "name", id->value);
> > +            }
> > +            else
> > +                smap_add(&ids, id->key, id->value);
>
> An easier way to do this would be ...
>
> struct smap ids;
> smap_clone(&ids, &op->nbsp->external_ids);
>
> >          }
> >          sbrec_port_binding_set_external_ids(op->sb, &ids);
> >      }
>
> or even easier, if we're just blind copying everything:
>
> sbrec_port_binding_set_external_ids(op->sb, &op->nbsp->external_ids);
>

Yes, the reason I didn't do that is because the 'neutron:port_name' key is
renamed into 'name' so we can't just blind copy it. I might clone the smap
and then rename (remove and add) the 'neutron:port_name' key if present.
But performance wise it won't buy us much since smap_clone internally
does a SMAP_FOR_EACH plus we need to find the element afterwards.
If you think it's way more clear then I'd rewrite it into something like:

struct smap ids = SMAP_INITIALIZER(&ids);



smap_clone(&ids, &op->nbsp->external_ids);


const char *name = smap_get(&ids, "neutron:port_name");


if(name) {


    smap_remove(&ids, "neutron:port_name");
    if(name[0])
         smap_add(&ids, "name", name);
}

Is the above preferred?

There's a more general question of whether we want to copy everything,
> or make sure we have an explicit understanding of external-ids we know
> about.  Copying everything is really convenient, but it's also nice to
> have a conversation about each use case in case there's something
> better we can do that would benefit other projects as well.
>
> In this case, based on our IRC conversation, there are 3 pieces of
> information you'd like to have associated with a Port_Binding:
>
> 1) network prefix length for each IP address associated with the port
>
> 2) a "project ID" -- an openstack tenancy identifier
>
> 3) a "device ID" -- an ID for the openstack VM the port is associated with
>
> #1 seems like something that could be more generally useful.  We
> discussed this on IRC, but perhaps we should just update the addresses
> column to optionally allow you specify a prefix length with an
> address?
>
> #2 and #3 seem OpenStack specific, and just leaving them as
> external-ids that get copied over seems fine.  I'm also OK with a bulk
> copy.  It may be nice to document the specific external-ids you plan
> to use, just so people have a reference that explains what they are
> when they go to debug an environment.
>

I planned to document that in networking-ovn since it's CMS specific and
neither ovn-northd nor ovn-controller use those.
However,  where do you think this documentation should go? I still think
that if someone debugs an environment they'll only find those keys when
using OpenStack and, therefore, they should go and look at networking-ovn
documentation where I should definitely document what those keys are used
for.

Thanks!
Daniel


> > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> > index eb348fe..7bb322f 100644
> > --- a/ovn/ovn-nb.xml
> > +++ b/ovn/ovn-nb.xml
> > @@ -848,7 +848,16 @@
> >
> >      <group title="Common Columns">
> >        <column name="external_ids">
> > -        See <em>External IDs</em> at the beginning of this document.
> > +        <p>
> > +          See <em>External IDs</em> at the beginning of this document.
> > +        </p>
> > +
> > +        <p>
> > +          The <code>ovn-northd</code> program copies all these pairs
> into the
> > +          <ref column="external_ids"/> column of the
> > +          <ref table="Port_Binding"/> table in <ref
> db="OVN_Southbound"/>
> > +          database.
> > +        </p>
> >        </column>
> >      </group>
> >    </table>
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
> --
> Russell Bryant
>