[ovs-dev] [PATCH] Copy external_ids from Logical_Switch_Port to SB database
Russell Bryant
russell at ovn.org
Mon Jun 5 12:06:27 UTC 2017
On Thu, Jun 1, 2017 at 4:33 AM, Daniel Alvarez Sanchez
<dalvarez at 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 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?
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 at openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>>
>> --
>> Russell Bryant
>
>
--
Russell Bryant
More information about the dev
mailing list