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 <[email protected]> wrote: > On Thu, Jun 1, 2017 at 4:33 AM, Daniel Alvarez Sanchez > <[email protected]> wrote: > > Thanks a lot Russell for taking the time to review this :) > > > > On Wed, May 31, 2017 at 8:48 PM, Russell Bryant <[email protected]> wrote: > >> > >> On Wed, May 31, 2017 at 12:18 PM, Daniel Alvarez <[email protected]> > >> 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 > >> > [email protected] > >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > >> > >> -- > >> Russell Bryant > > > > > > > > -- > Russell Bryant > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
