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

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(-)

On Wed, May 31, 2017 at 12:18 PM, Daniel Alvarez <dalvarez@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);

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.

> 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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

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

> On Wed, May 31, 2017 at 12:18 PM, Daniel Alvarez <dalvarez@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@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
> --
> Russell Bryant
>

On Thu, Jun 1, 2017 at 4:33 AM, Daniel Alvarez Sanchez
<dalvarez@redhat.com> wrote:
> Thanks a lot Russell for taking the time to review this :)
>
> On Wed, May 31, 2017 at 8:48 PM, Russell Bryant <russell@ovn.org> wrote:
>>
>> On Wed, May 31, 2017 at 12:18 PM, Daniel Alvarez <dalvarez@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?

Ah, sorry, I didn't read close enough and missed that the "name" key
was changing.

I'm not sure if either approach will make a meaningful difference.  I
slightly prefer this latest version, though.

Please also have another look at the coding style doc:

http://docs.openvswitch.org/en/latest/internals/contributing/coding-style/

There should be spaces after "if" and braces should be used, even for
single statements.  FOr example:

    if (name[0]) {
        smap_add(...);
    }

>
>> 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.

networking-ovn is fine.

The "neutron:network_name" and "neutron:port_name" external IDs are
documented in OVN itself, but those are actually read by OVN code in
some cases for user friendliness.  It makes sense to me to only
document the ones used by OVN in the OVN docs then.

>
> 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@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>>
>> --
>> Russell Bryant
>
>

Thanks Russel, ack to your comments.
I've just sent the v2 version of the patch

On Mon, Jun 5, 2017 at 2:06 PM, Russell Bryant <russell@ovn.org> wrote:

> On Thu, Jun 1, 2017 at 4:33 AM, Daniel Alvarez Sanchez
> <dalvarez@redhat.com> wrote:
> > Thanks a lot Russell for taking the time to review this :)
> >
> > On Wed, May 31, 2017 at 8:48 PM, Russell Bryant <russell@ovn.org> wrote:
> >>
> >> On Wed, May 31, 2017 at 12:18 PM, Daniel Alvarez <dalvarez@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?
>
> Ah, sorry, I didn't read close enough and missed that the "name" key
> was changing.
>
> I'm not sure if either approach will make a meaningful difference.  I
> slightly prefer this latest version, though.
>
> Please also have another look at the coding style doc:
>
> http://docs.openvswitch.org/en/latest/internals/contributing/coding-style/
>
> There should be spaces after "if" and braces should be used, even for
> single statements.  FOr example:
>
>     if (name[0]) {
>         smap_add(...);
>     }
>
> >
> >> 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.
>
> networking-ovn is fine.
>
> The "neutron:network_name" and "neutron:port_name" external IDs are
> documented in OVN itself, but those are actually read by OVN code in
> some cases for user friendliness.  It makes sense to me to only
> document the ones used by OVN in the OVN docs then.
>
> >
> > 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@openvswitch.org
> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >>
> >>
> >> --
> >> Russell Bryant
> >
> >
>
>
>
> --
> Russell Bryant
>