| Message ID | 20170503154600.27401-3-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, May 3, 2017 at 8:45 AM, Ben Pfaff <blp@ovn.org> wrote: > It's much easier to see what's going on in the southbound database if > human-friendly names are available. > > Really it's too bad that we didn't put the human-friendly name in "name" > and the UUID in something like "external_ids:neutron-uuid", but it'll take > more coordination to change that at this point and it may not be worth it. > > Signed-off-by: Ben Pfaff <blp@ovn.org> Acked-by: Andy Zhou <azhou@ovn.org> > --- > ovn/northd/ovn-northd.c | 10 +++++++++- > ovn/ovn-sb.xml | 28 ++++++++++++++++++++++------ > 2 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index d393e544c308..3680433d26fa 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -725,8 +725,13 @@ build_datapaths(struct northd_context *ctx, struct hmap *datapaths) > sprintf(uuid_s, UUID_FMT, UUID_ARGS(&od->key)); > const char *key = od->nbs ? "logical-switch" : "logical-router"; > > - /* Get name to set in external-ids. */ > + /* Get names to set in external-ids. */ > const char *name = od->nbs ? od->nbs->name : od->nbr->name; > + const char *name2 = (od->nbs > + ? smap_get(&od->nbs->external_ids, > + "neutron:network_name") > + : smap_get(&od->nbr->external_ids, > + "neutron:router_name")); > > /* Set external-ids. */ > struct smap ids = SMAP_INITIALIZER(&ids); > @@ -734,6 +739,9 @@ build_datapaths(struct northd_context *ctx, struct hmap *datapaths) > if (*name) { > smap_add(&ids, "name", name); > } In the definition of both nbrec_loical_router and nbrec_logical_switch, the comment of the 'name' filed says it should nonnull. If that is true, then we don't need this check? > + if (name2 && name2[0]) { > + smap_add(&ids, *name ? "name2" : "name", name2); > + } > sbrec_datapath_binding_set_external_ids(od->sb, &ids); > smap_destroy(&ids); > > diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml > index 8bb0f3ddebc9..387adb8069d3 100644 > --- a/ovn/ovn-sb.xml > +++ b/ovn/ovn-sb.xml > @@ -1691,12 +1691,28 @@ tcp.flags = RST; > the <ref db="OVN_Northbound"/> database. > </column> > > - <column name="external_ids" key="name"> > - <code>ovn-northd</code> copies this from the <ref > - table="Logical_Router" db="OVN_Northbound"/> or <ref > - table="Logical_Switch" db="OVN_Northbound"/> table in the <ref > - db="OVN_Northbound"/> database, when that column is nonempty. > - </column> > + <group title="Naming"> > + <p> > + <code>ovn-northd</code> copies these from the name fields in the <ref > + db="OVN_Northbound"/> database, either from <ref > + table="Logical_Router" db="OVN_Northbound" column="name"/> and <ref > + table="Logical_Router" db="OVN_Northbound" column="external_ids" > + key="neutron:router_name"/> in the <ref table="Logical_Router" > + db="OVN_Northbound"/> table or from <ref table="Logical_Switch" > + db="OVN_Northbound" column="name"/> and <ref table="Logical_Switch" > + db="OVN_Northbound" column="external_ids" > + key="neutron:network_name"/> in the <ref table="Logical_Switch" > + db="OVN_Northbound"/> table. > + </p> > + > + <column name="external_ids" key="name"> > + A name for the logical datapath. > + </column> > + > + <column name="external_ids" key="name2"> > + Another name for the logical datapath. > + </column> > + </group> > </group> > > <group title="Common Columns"> > -- > 2.10.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, May 03, 2017 at 02:59:15PM -0700, Andy Zhou wrote: > On Wed, May 3, 2017 at 8:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > It's much easier to see what's going on in the southbound database if > > human-friendly names are available. > > > > Really it's too bad that we didn't put the human-friendly name in "name" > > and the UUID in something like "external_ids:neutron-uuid", but it'll take > > more coordination to change that at this point and it may not be worth it. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > Acked-by: Andy Zhou <azhou@ovn.org> Thanks! > > @@ -734,6 +739,9 @@ build_datapaths(struct northd_context *ctx, struct hmap *datapaths) > > if (*name) { > > smap_add(&ids, "name", name); > > } > In the definition of both nbrec_loical_router and > nbrec_logical_switch, the comment of the 'name' filed > says it should nonnull. If that is true, then we don't need this check? Oh, thanks for noticing. I simplified this code.
On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > It's much easier to see what's going on in the southbound database if > human-friendly names are available. > > Really it's too bad that we didn't put the human-friendly name in "name" > and the UUID in something like "external_ids:neutron-uuid", but it'll take > more coordination to change that at this point and it may not be worth it. Definitely a fair criticism. I had to think a bit about how we ended up here ... The original OVN integration used ovn-nbctl instead of the Python ovs library. I found it a quick and easy way to get the first POC driver working. It was later converted to use the Python lib and we dropped ovn-nbctl usage. When we create a resource in OVN, we have nowhere in the Neutron DB to stash OVN's UUID, so we need to look up the OVN resource using Neutron's UUID (name's are not unique). With ovn-nbctl, we only supporting looking up by UUID or "name", so I had to use Neutron's UUID there. So, this is a case of a quick decision made in the first POC driver sticking and taking over 2 years for someone to notice the odd choice and question it. I imagine everyone assumed it was for good reason and that OVN required unique names. Oops. I see how you've made the best of it in your series and I really appreciate that effort.
On Wed, May 03, 2017 at 07:42:46PM -0400, Russell Bryant wrote: > On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > It's much easier to see what's going on in the southbound database if > > human-friendly names are available. > > > > Really it's too bad that we didn't put the human-friendly name in "name" > > and the UUID in something like "external_ids:neutron-uuid", but it'll take > > more coordination to change that at this point and it may not be worth it. > > Definitely a fair criticism. I had to think a bit about how we ended > up here ... > > The original OVN integration used ovn-nbctl instead of the Python ovs > library. I found it a quick and easy way to get the first POC driver > working. It was later converted to use the Python lib and we dropped > ovn-nbctl usage. > > When we create a resource in OVN, we have nowhere in the Neutron DB to > stash OVN's UUID, so we need to look up the OVN resource using > Neutron's UUID (name's are not unique). With ovn-nbctl, we only > supporting looking up by UUID or "name", so I had to use Neutron's > UUID there. So, this is a case of a quick decision made in the first > POC driver sticking and taking over 2 years for someone to notice the > odd choice and question it. I imagine everyone assumed it was for > good reason and that OVN required unique names. Oops. > > I see how you've made the best of it in your series and I really > appreciate that effort. I thought that it was probably something like that. It's a very minor thing and I think that this is an OK workaround.
On Wed, May 3, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, May 03, 2017 at 07:42:46PM -0400, Russell Bryant wrote: > > On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > > It's much easier to see what's going on in the southbound database if > > > human-friendly names are available. > > > > > > Really it's too bad that we didn't put the human-friendly name in "name" > > > and the UUID in something like "external_ids:neutron-uuid", but it'll take Does OVSDB schema support index on map keys inside a column?
Isn't neutron:network_name and neutron:router_name very openstack specific? May we introduce more generic names in a way that other systems could eventually use them too? On May 4, 2017 8:43 AM, "Han Zhou" <zhouhan@gmail.com> wrote: > On Wed, May 3, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Wed, May 03, 2017 at 07:42:46PM -0400, Russell Bryant wrote: > > > On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > It's much easier to see what's going on in the southbound database if > > > > human-friendly names are available. > > > > > > > > Really it's too bad that we didn't put the human-friendly name in > "name" > > > > and the UUID in something like "external_ids:neutron-uuid", but it'll > take > > Does OVSDB schema support index on map keys inside a column? > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, May 03, 2017 at 11:43:15PM -0700, Han Zhou wrote: > On Wed, May 3, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Wed, May 03, 2017 at 07:42:46PM -0400, Russell Bryant wrote: > > > On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > It's much easier to see what's going on in the southbound database if > > > > human-friendly names are available. > > > > > > > > Really it's too bad that we didn't put the human-friendly name in > "name" > > > > and the UUID in something like "external_ids:neutron-uuid", but it'll > take > > Does OVSDB schema support index on map keys inside a column? No. For cases where the database needs to maintain uniqueness, a dedicated column is needed. But that is not always the case here.
Yes, I'm in favor of having generic names available. On Thu, May 04, 2017 at 09:49:30AM +0200, Miguel Angel Ajo Pelayo wrote: > Isn't neutron:network_name and neutron:router_name very openstack specific? > > May we introduce more generic names in a way that other systems could > eventually use them too? > > On May 4, 2017 8:43 AM, "Han Zhou" <zhouhan@gmail.com> wrote: > > > On Wed, May 3, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > On Wed, May 03, 2017 at 07:42:46PM -0400, Russell Bryant wrote: > > > > On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > It's much easier to see what's going on in the southbound database if > > > > > human-friendly names are available. > > > > > > > > > > Really it's too bad that we didn't put the human-friendly name in > > "name" > > > > > and the UUID in something like "external_ids:neutron-uuid", but it'll > > take > > > > Does OVSDB schema support index on map keys inside a column? > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Thu, May 4, 2017 at 10:05 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, May 03, 2017 at 11:43:15PM -0700, Han Zhou wrote: > > On Wed, May 3, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > On Wed, May 03, 2017 at 07:42:46PM -0400, Russell Bryant wrote: > > > > On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > It's much easier to see what's going on in the southbound database if > > > > > human-friendly names are available. > > > > > > > > > > Really it's too bad that we didn't put the human-friendly name in > > "name" > > > > > and the UUID in something like "external_ids:neutron-uuid", but it'll > > take > > > > Does OVSDB schema support index on map keys inside a column? > > No. For cases where the database needs to maintain uniqueness, a > dedicated column is needed. But that is not always the case here. Yes, it doesn't matter for this patch. I just try to understand what should be the desired design, if putting neutron-uuid in name was a bad idea. The case in Neutron plugin (or maybe also in some other CMS systems) is that it tries to avoid storing any ovn uuid's. When it needs to get corresponding ovn object, it queries OVN NB by neutron-uuid. At least in Logical_Switch_Port it seems nature to put it in "name" column, because in the schema, name is unique index in Logical_Switch_Port, so just act as an ID, although "name" suggests something else. If we put it in external_ids:neutron-uuid, it is not indexable. Maybe it is not a big problem because the "index" here in ovsdb may be not that critical as in traditional db, because what matters is IDL cache, but still it seems not right if some unique key can't be indexed. Another thing is, we have the "name" in most of the OVN tables, some are index (e.g. the one in Logical_Switch_Port), some are not. This may be confusing, too.
On Thu, May 04, 2017 at 10:34:38AM -0700, Han Zhou wrote: > On Thu, May 4, 2017 at 10:05 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Wed, May 03, 2017 at 11:43:15PM -0700, Han Zhou wrote: > > > On Wed, May 3, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > On Wed, May 03, 2017 at 07:42:46PM -0400, Russell Bryant wrote: > > > > > On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > It's much easier to see what's going on in the southbound > database if > > > > > > human-friendly names are available. > > > > > > > > > > > > Really it's too bad that we didn't put the human-friendly name in > > > "name" > > > > > > and the UUID in something like "external_ids:neutron-uuid", but > it'll > > > take > > > > > > Does OVSDB schema support index on map keys inside a column? > > > > No. For cases where the database needs to maintain uniqueness, a > > dedicated column is needed. But that is not always the case here. > > Yes, it doesn't matter for this patch. I just try to understand what should > be the desired design, if putting neutron-uuid in name was a bad idea. The > case in Neutron plugin (or maybe also in some other CMS systems) is that it > tries to avoid storing any ovn uuid's. When it needs to get corresponding > ovn object, it queries OVN NB by neutron-uuid. At least in > Logical_Switch_Port it seems nature to put it in "name" column, because in > the schema, name is unique index in Logical_Switch_Port, so just act as an > ID, although "name" suggests something else. If we put it in > external_ids:neutron-uuid, it is not indexable. Maybe it is not a big > problem because the "index" here in ovsdb may be not that critical as in > traditional db, because what matters is IDL cache, but still it seems not > right if some unique key can't be indexed. Another thing is, we have the > "name" in most of the OVN tables, some are index (e.g. the one in > Logical_Switch_Port), some are not. This may be confusing, too. I agree that there are inconsistencies in the design. Some are easy to fix, some are not, and I think that we need to decided which are important enough to fix.
On Thu, May 4, 2017 at 10:58 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, May 04, 2017 at 10:34:38AM -0700, Han Zhou wrote: > > On Thu, May 4, 2017 at 10:05 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > On Wed, May 03, 2017 at 11:43:15PM -0700, Han Zhou wrote: > > > > On Wed, May 3, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > > > On Wed, May 03, 2017 at 07:42:46PM -0400, Russell Bryant wrote: > > > > > > On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > > It's much easier to see what's going on in the southbound > > database if > > > > > > > human-friendly names are available. > > > > > > > > > > > > > > Really it's too bad that we didn't put the human-friendly name in > > > > "name" > > > > > > > and the UUID in something like "external_ids:neutron-uuid", but > > it'll > > > > take > > > > > > > > Does OVSDB schema support index on map keys inside a column? > > > > > > No. For cases where the database needs to maintain uniqueness, a > > > dedicated column is needed. But that is not always the case here. > > > > Yes, it doesn't matter for this patch. I just try to understand what should > > be the desired design, if putting neutron-uuid in name was a bad idea. The > > case in Neutron plugin (or maybe also in some other CMS systems) is that it > > tries to avoid storing any ovn uuid's. When it needs to get corresponding > > ovn object, it queries OVN NB by neutron-uuid. At least in > > Logical_Switch_Port it seems nature to put it in "name" column, because in > > the schema, name is unique index in Logical_Switch_Port, so just act as an > > ID, although "name" suggests something else. If we put it in > > external_ids:neutron-uuid, it is not indexable. Maybe it is not a big > > problem because the "index" here in ovsdb may be not that critical as in > > traditional db, because what matters is IDL cache, but still it seems not > > right if some unique key can't be indexed. Another thing is, we have the > > "name" in most of the OVN tables, some are index (e.g. the one in > > Logical_Switch_Port), some are not. This may be confusing, too. > > I agree that there are inconsistencies in the design. Some are easy to > fix, some are not, and I think that we need to decided which are > important enough to fix. I am not questioning the fix here (except that the string "neutron" could be something more generic). I am trying to understand why using "name" to store unique ids from client system and putting meaningful names in external_ids was a bad idea. If it is not a bad idea, then may be we can just change the document so that it is not just a human readable name, but can be anything unique from a client system. We can change "names" in all tables to unique index, and then the design will be consistent. I understand this may have impact to existing deployments, but I think it is not a bad idea. Being able to specify the unique id by client is a desirable feature. One benefit is, it makes object creation idempotent in case of timeout and retries. P.S. we are not alone for using "name" as unique id, e.g. https://kubernetes.io/docs/concepts/overview/working-with-objects/names/ Of course a namespace mechanism would be better.
On Thu, May 04, 2017 at 11:14:01AM -0700, Han Zhou wrote: > On Thu, May 4, 2017 at 10:58 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Thu, May 04, 2017 at 10:34:38AM -0700, Han Zhou wrote: > > > On Thu, May 4, 2017 at 10:05 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > On Wed, May 03, 2017 at 11:43:15PM -0700, Han Zhou wrote: > > > > > On Wed, May 3, 2017 at 4:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > > > > > On Wed, May 03, 2017 at 07:42:46PM -0400, Russell Bryant wrote: > > > > > > > On Wed, May 3, 2017 at 11:45 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > It's much easier to see what's going on in the southbound > > > database if > > > > > > > > human-friendly names are available. > > > > > > > > > > > > > > > > Really it's too bad that we didn't put the human-friendly > name in > > > > > "name" > > > > > > > > and the UUID in something like "external_ids:neutron-uuid", > but > > > it'll > > > > > take > > > > > > > > > > Does OVSDB schema support index on map keys inside a column? > > > > > > > > No. For cases where the database needs to maintain uniqueness, a > > > > dedicated column is needed. But that is not always the case here. > > > > > > Yes, it doesn't matter for this patch. I just try to understand what > should > > > be the desired design, if putting neutron-uuid in name was a bad idea. > The > > > case in Neutron plugin (or maybe also in some other CMS systems) is > that it > > > tries to avoid storing any ovn uuid's. When it needs to get > corresponding > > > ovn object, it queries OVN NB by neutron-uuid. At least in > > > Logical_Switch_Port it seems nature to put it in "name" column, because > in > > > the schema, name is unique index in Logical_Switch_Port, so just act as > an > > > ID, although "name" suggests something else. If we put it in > > > external_ids:neutron-uuid, it is not indexable. Maybe it is not a big > > > problem because the "index" here in ovsdb may be not that critical as in > > > traditional db, because what matters is IDL cache, but still it seems > not > > > right if some unique key can't be indexed. Another thing is, we have the > > > "name" in most of the OVN tables, some are index (e.g. the one in > > > Logical_Switch_Port), some are not. This may be confusing, too. > > > > I agree that there are inconsistencies in the design. Some are easy to > > fix, some are not, and I think that we need to decided which are > > important enough to fix. > > I am not questioning the fix here (except that the string "neutron" could > be something more generic). I am trying to understand why using "name" to > store unique ids from client system and putting meaningful names in > external_ids was a bad idea. It was a bad idea because it was documented to be a human-friendly name. That's all. > If it is not a bad idea, then may be we can just change the document > so that it is not just a human readable name, but can be anything > unique from a client system. We can change "names" in all tables to > unique index, and then the design will be consistent. I understand > this may have impact to existing deployments, but I think it is not a > bad idea. Being able to specify the unique id by client is a desirable > feature. One benefit is, it makes object creation idempotent in case > of timeout and retries. > > P.S. we are not alone for using "name" as unique id, e.g. > https://kubernetes.io/docs/concepts/overview/working-with-objects/names/ > Of course a namespace mechanism would be better.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index d393e544c308..3680433d26fa 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -725,8 +725,13 @@ build_datapaths(struct northd_context *ctx, struct hmap *datapaths) sprintf(uuid_s, UUID_FMT, UUID_ARGS(&od->key)); const char *key = od->nbs ? "logical-switch" : "logical-router"; - /* Get name to set in external-ids. */ + /* Get names to set in external-ids. */ const char *name = od->nbs ? od->nbs->name : od->nbr->name; + const char *name2 = (od->nbs + ? smap_get(&od->nbs->external_ids, + "neutron:network_name") + : smap_get(&od->nbr->external_ids, + "neutron:router_name")); /* Set external-ids. */ struct smap ids = SMAP_INITIALIZER(&ids); @@ -734,6 +739,9 @@ build_datapaths(struct northd_context *ctx, struct hmap *datapaths) if (*name) { smap_add(&ids, "name", name); } + if (name2 && name2[0]) { + smap_add(&ids, *name ? "name2" : "name", name2); + } sbrec_datapath_binding_set_external_ids(od->sb, &ids); smap_destroy(&ids); diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml index 8bb0f3ddebc9..387adb8069d3 100644 --- a/ovn/ovn-sb.xml +++ b/ovn/ovn-sb.xml @@ -1691,12 +1691,28 @@ tcp.flags = RST; the <ref db="OVN_Northbound"/> database. </column> - <column name="external_ids" key="name"> - <code>ovn-northd</code> copies this from the <ref - table="Logical_Router" db="OVN_Northbound"/> or <ref - table="Logical_Switch" db="OVN_Northbound"/> table in the <ref - db="OVN_Northbound"/> database, when that column is nonempty. - </column> + <group title="Naming"> + <p> + <code>ovn-northd</code> copies these from the name fields in the <ref + db="OVN_Northbound"/> database, either from <ref + table="Logical_Router" db="OVN_Northbound" column="name"/> and <ref + table="Logical_Router" db="OVN_Northbound" column="external_ids" + key="neutron:router_name"/> in the <ref table="Logical_Router" + db="OVN_Northbound"/> table or from <ref table="Logical_Switch" + db="OVN_Northbound" column="name"/> and <ref table="Logical_Switch" + db="OVN_Northbound" column="external_ids" + key="neutron:network_name"/> in the <ref table="Logical_Switch" + db="OVN_Northbound"/> table. + </p> + + <column name="external_ids" key="name"> + A name for the logical datapath. + </column> + + <column name="external_ids" key="name2"> + Another name for the logical datapath. + </column> + </group> </group> <group title="Common Columns">
It's much easier to see what's going on in the southbound database if human-friendly names are available. Really it's too bad that we didn't put the human-friendly name in "name" and the UUID in something like "external_ids:neutron-uuid", but it'll take more coordination to change that at this point and it may not be worth it. Signed-off-by: Ben Pfaff <blp@ovn.org> --- ovn/northd/ovn-northd.c | 10 +++++++++- ovn/ovn-sb.xml | 28 ++++++++++++++++++++++------ 2 files changed, 31 insertions(+), 7 deletions(-)