On 6/27/25 4:58 PM, Lorenzo Bianconi via dev wrote:
>> Prior to this commit, if you wanted to find the corresponding northbound
>> UUID for a southbound Logical Datapath, you could find it in one of two
>> places:
>>
>> For logical switches, it was in external-ids:logical-switch.
>> For logical routers, it was in external-ids:logical-router.
>>
>> With this commit, we are separating the type and UUID into separate
>> fields. This way, no matter the type of the datapath, you can find the
>> UUID. And you can find the type of the datapath without having to
>> potentially check multiple external-ids keys to do so.
>>
>> These fields are going to be used pretty heavily by northd in upcoming
>> patches, so instead of making them external-ids, these are now
>> full-fledged columns on the southbound Datapath_Binding. The UUID of the
>> northbound logical datapath is in a column called "nb_uuid" and the type
>> of the logical datapath is stored in an enumerated column called "type".
>>
>> Note that there is a seemingly-unrelated change in the check packet
>> length test in tests/ovn.at. This test started failing because its use
>> of `grep` was picking up the new "nb_uuid" column accidentally. The
>> change to the test uses `fetch_column` since it is more precise.
>>
>> Signed-off-by: Mark Michelson <[email protected]>
>
> Acked-by: Lorenzo Bianconi <[email protected]>
>
Hi Mark, Lorenzo,
Thanks for the patch and review! I have some comments of my own too.
>> ---
>> v9
>> * Rebased.
>>
>> v8
>> * Rebased.
>> * Changed the types of the new columns. The nb_uuid is type "uuid" and
>> the type is an enum.
>>
>> v7
>> * Rebased.
>> * Changed from using external-ids for the nb_uuid and type to using
>> new columns in the Datapath_Binding table.
>>
>> v6
>> * This is the first version of the series to contain this patch.
>> ---
>>
>> controller/local_data.c | 2 +-
>> ic/ovn-ic.c | 9 +++++++--
>> northd/northd.c | 38 +++++++++++++++++++-------------------
>> northd/northd.h | 5 +++--
>> ovn-sb.ovsschema | 11 +++++++++--
>> ovn-sb.xml | 21 +++++++++++----------
>> tests/ovn-controller.at | 4 ++++
>> tests/ovn-northd.at | 6 +++---
>> tests/ovn.at | 6 ++----
>> utilities/ovn-sbctl.c | 4 ++--
>> utilities/ovn-trace.c | 5 +----
>> 11 files changed, 62 insertions(+), 49 deletions(-)
>>
>> diff --git a/controller/local_data.c b/controller/local_data.c
>> index 00383d091..49b5ca9df 100644
>> --- a/controller/local_data.c
>> +++ b/controller/local_data.c
>> @@ -708,7 +708,7 @@ local_datapath_peer_port_add(struct local_datapath *ld,
>> static bool
>> datapath_is_switch(const struct sbrec_datapath_binding *ldp)
>> {
>> - return smap_get(&ldp->external_ids, "logical-switch") != NULL;
>> + return strcmp(ldp->type, "logical-switch") == 0;
>> }
>>
This worries me a bit. Our recommended upgrade procedure is to upgrade
ovn-controllers first and then central components (SB, NB and
ovn-northd). Upgrades can take a significant amount of time so there's
a high chance that in large deployments newer ovn-controllers run with
"old" SB databases for a while.
If users follow that then this function will always return "false" for
all datapath_binding records (regardless if they're switches or not).
I think we should maintain backwards compatibility with the old
external_ids in ovn-controller in order to not disrupt upgrades.
>> static bool
>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>> index caffa6fe0..44a6aa5ac 100644
>> --- a/ic/ovn-ic.c
>> +++ b/ic/ovn-ic.c
>> @@ -613,8 +613,11 @@ get_router_uuid_by_sb_pb(struct ic_context *ctx,
>> return NULL;
>> }
>>
>> - return smap_get_uuid(&router_pb->datapath->external_ids,
>> "logical-router",
>> - router_uuid);
>> + if (!router_pb->datapath->nb_uuid) {
>> + return false;
>> + }
>> + *router_uuid = *router_pb->datapath->nb_uuid;
>> + return true;
I don't think we explicitly mention ovn-ic in the recommended upgrade
path but I'd say it's similar to ovn-controller so we should probably
maintain backwards compatibility here too.
>> }
>>
>> static void
>> @@ -2582,6 +2585,8 @@ main(int argc, char *argv[])
>> ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_datapath_binding);
>> ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> &sbrec_datapath_binding_col_external_ids);
>> + ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> + &sbrec_datapath_binding_col_nb_uuid);
>>
>> ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_port_binding);
>> ovsdb_idl_add_column(ovnsb_idl_loop.idl,
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 9b21786fa..49ed36df7 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -577,20 +577,21 @@ ovn_datapath_from_sbrec(const struct hmap
>> *ls_datapaths,
>> const struct hmap *lr_datapaths,
>> const struct sbrec_datapath_binding *sb)
>> {
>> - struct uuid key;
>> const struct hmap *dps;
>>
>> - if (smap_get_uuid(&sb->external_ids, "logical-switch", &key)) {
>> + if (!strcmp(sb->type, "logical-switch")) {
>> dps = ls_datapaths;
>> - } else if (smap_get_uuid(&sb->external_ids, "logical-router", &key)) {
>> + } else if (!strcmp(sb->type, "logical-router")) {
>> dps = lr_datapaths;
>> } else {
>> return NULL;
>> }
>> - if (!dps) {
>> +
>> + if (!sb->nb_uuid) {
>> return NULL;
>> }
>> - struct ovn_datapath *od = ovn_datapath_find_(dps, &key);
>> +
>> + struct ovn_datapath *od = ovn_datapath_find_(dps, sb->nb_uuid);
>> if (od && (od->sb == sb)) {
>> return od;
>> }
>> @@ -749,11 +750,9 @@ store_mcast_info_for_switch_datapath(const struct
>> sbrec_ip_multicast *sb,
>> static void
>> ovn_datapath_update_external_ids(struct ovn_datapath *od)
>> {
>> - /* Get the logical-switch or logical-router UUID to set in
>> - * external-ids. */
>> + /* Get the NB UUID to set in external-ids. */
>> char uuid_s[UUID_LEN + 1];
>> sprintf(uuid_s, UUID_FMT, UUID_ARGS(&od->key));
>> - const char *key = od->nbs ? "logical-switch" : "logical-router";
>>
>> /* Get names to set in external-ids. */
>> const char *name = od->nbs ? od->nbs->name : od->nbr->name;
>> @@ -765,7 +764,6 @@ ovn_datapath_update_external_ids(struct ovn_datapath *od)
>>
>> /* Set external-ids. */
>> struct smap ids = SMAP_INITIALIZER(&ids);
>> - smap_add(&ids, key, uuid_s);
>> smap_add(&ids, "name", name);
>> if (name2 && name2[0]) {
>> smap_add(&ids, "name2", name2);
>> @@ -892,30 +890,26 @@ join_datapaths(const struct nbrec_logical_switch_table
>> *nbrec_ls_table,
>>
>> const struct sbrec_datapath_binding *sb;
>> SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_SAFE (sb, sbrec_dp_table) {
>> - struct uuid key;
>> - if (!smap_get_uuid(&sb->external_ids, "logical-switch", &key) &&
>> - !smap_get_uuid(&sb->external_ids, "logical-router", &key)) {
>> + if (!sb->nb_uuid) {
>> ovsdb_idl_txn_add_comment(
>> ovnsb_txn,
>> - "deleting Datapath_Binding "UUID_FMT" that lacks "
>> - "external-ids:logical-switch and "
>> - "external-ids:logical-router",
>> + "deleting Datapath_Binding "UUID_FMT" that lacks nb_uuid",
Similar to the previous comments above, the first time ovn-northd starts
up (after upgrade) and connects to an already existing SB database, it
will remove all datapath bindings, which will likely cause dataplane
disruption.
I think we should detect that the existing SB record had external_ids
set and update it so that the new nb_uuid is populated (without deleting
the external_ids or datapath record yet).
>> UUID_ARGS(&sb->header_.uuid));
>> sbrec_datapath_binding_delete(sb);
>> continue;
>> }
>>
>> - if (ovn_datapath_find_(datapaths, &key)) {
>> + if (ovn_datapath_find_(datapaths, sb->nb_uuid)) {
>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> VLOG_INFO_RL(
>> &rl, "deleting Datapath_Binding "UUID_FMT" with "
>> - "duplicate external-ids:logical-switch/router "UUID_FMT,
>> - UUID_ARGS(&sb->header_.uuid), UUID_ARGS(&key));
>> + "duplicate nb_uuid "UUID_FMT,
>> + UUID_ARGS(&sb->header_.uuid), UUID_ARGS(sb->nb_uuid));
>> sbrec_datapath_binding_delete(sb);
>> continue;
>> }
>>
>> - struct ovn_datapath *od = ovn_datapath_create(datapaths, &key,
>> + struct ovn_datapath *od = ovn_datapath_create(datapaths,
>> sb->nb_uuid,
>> NULL, NULL, sb);
>> ovs_list_push_back(sb_only, &od->list);
>> }
>> @@ -1117,11 +1111,17 @@ build_datapaths(struct ovsdb_idl_txn *ovnsb_txn,
>> sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key);
>> }
>> ovn_datapath_update_external_ids(od);
>> + sbrec_datapath_binding_set_nb_uuid(od->sb, &od->key, 1);
>> + sbrec_datapath_binding_set_type(od->sb, od->nbs ? "logical-switch" :
>> + "logical-router");
>> }
>> LIST_FOR_EACH (od, list, &nb_only) {
>> od->sb = sbrec_datapath_binding_insert(ovnsb_txn);
>> ovn_datapath_update_external_ids(od);
>> sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key);
>> + sbrec_datapath_binding_set_nb_uuid(od->sb, &od->key, 1);
>> + sbrec_datapath_binding_set_type(od->sb, od->nbs ? "logical-switch" :
>> + "logical-router");
>> }
>> ovn_destroy_tnlids(&dp_tnlids);
>>
>> diff --git a/northd/northd.h b/northd/northd.h
>> index 084634328..2b8f7fdee 100644
>> --- a/northd/northd.h
>> +++ b/northd/northd.h
>> @@ -347,7 +347,7 @@ DRR_MODES
>> #undef DRR_MODE
>>
>> /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or
>> - * sb->external_ids:logical-switch. */
>> + * sb->nb_uuid. */
>> struct ovn_datapath {
>> struct hmap_node key_node; /* Index on 'key'. */
>> struct uuid key; /* (nbs/nbr)->header_.uuid. */
>> @@ -449,7 +449,8 @@ ovn_datapath_is_stale(const struct ovn_datapath *od)
>> /* The two purposes for which ovn-northd uses OVN logical datapaths. */
>> enum ovn_datapath_type {
>> DP_SWITCH, /* OVN logical switch. */
>> - DP_ROUTER /* OVN logical router. */
>> + DP_ROUTER, /* OVN logical router. */
>> + DP_MAX,
>> };
>>
>> /* Returns an "enum ovn_stage" built from the arguments.
>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>> index 4c24f5b51..8bc7bbba3 100644
>> --- a/ovn-sb.ovsschema
>> +++ b/ovn-sb.ovsschema
>> @@ -1,7 +1,7 @@
>> {
>> "name": "OVN_Southbound",
>> - "version": "21.2.0",
>> - "cksum": "29145795 34859",
>> + "version": "21.3.0",
>> + "cksum": "3179330761 35286",
>> "tables": {
>> "SB_Global": {
>> "columns": {
>> @@ -196,6 +196,13 @@
>> "load_balancers": {"type": {"key": {"type": "uuid"},
>> "min": 0,
>> "max": "unlimited"}},
>> + "type": {"type": {"key": {"type": "string",
>> + "enum": ["set",
>> + ["logical-switch",
>> +
>> "logical-router"]]}}},
>> + "nb_uuid": {"type": {"key": {"type": "uuid"},
>> + "min": 0,
>> + "max": 1}},
>> "external_ids": {
>> "type": {"key": "string", "value": "string",
>> "min": 0, "max": "unlimited"}}},
>> diff --git a/ovn-sb.xml b/ovn-sb.xml
>> index db5faac66..ad09a472e 100644
>> --- a/ovn-sb.xml
>> +++ b/ovn-sb.xml
>> @@ -3236,18 +3236,19 @@ tcp.flags = RST;
>> db="OVN_Northbound"/> database.
>> </p>
>>
>> - <column name="external_ids" key="logical-switch" type='{"type":
>> "uuid"}'>
>> - For a logical datapath that represents a logical switch,
>> - <code>ovn-northd</code> stores in this key the UUID of the
>> - corresponding <ref table="Logical_Switch" db="OVN_Northbound"/> row
>> in
>> - the <ref db="OVN_Northbound"/> database.
>> + <column name="type" type='{"type": "string"}'>
>> + This represents the type of the northbound logical datapath that is
>> + represented by this southbound <code>Datapath_Binding</code>. The
>> + possible values for this are:
>> + <ul>
>> + <li><code>logical-switch</code></li>
>> + <li><code>logical-router</code></li>
>> + </ul>
>> </column>
>>
>> - <column name="external_ids" key="logical-router" type='{"type":
>> "uuid"}'>
>> - For a logical datapath that represents a logical router,
>> - <code>ovn-northd</code> stores in this key the UUID of the
>> - corresponding <ref table="Logical_Router" db="OVN_Northbound"/> row
>> in
>> - the <ref db="OVN_Northbound"/> database.
>> + <column name="nb_uuid" type='{"type": "string"}'>
>> + This is the UUID of the corresponding northbound logical datapath
>> + represented by this southbound <code>Datapath_Binding</code>.
>> </column>
>>
>> <column name="external_ids" key="interconn-ts" type='{"type":
>> "string"}'>
>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
>> index 6007a7454..4e73dcdcf 100644
>> --- a/tests/ovn-controller.at
>> +++ b/tests/ovn-controller.at
>> @@ -123,6 +123,7 @@ check_patches
>> # another logical port bound to this chassis.
>> check_uuid ovn-sbctl \
>> -- --id=@dp101 create Datapath_Binding tunnel_key=101
>> external_ids:name=dp101 \
>> + type=logical-switch \
>> -- create Port_Binding datapath=@dp101 logical_port=localnet1
>> tunnel_key=1 \
>> type=localnet options:network_name=physnet1
>> check_patches
>> @@ -131,6 +132,7 @@ check_patches
>> # Now we should get some patch ports created.
>> check_uuid ovn-sbctl \
>> -- --id=@dp102 create Datapath_Binding tunnel_key=102
>> external_ids:name=dp102 \
>> + type=logical-switch \
>> -- create Port_Binding datapath=@dp102 logical_port=localnet2
>> tunnel_key=1 \
>> type=localnet options:network_name=physnet1 \
>> -- create Port_Binding datapath=@dp102 logical_port=localvif2
>> tunnel_key=2
>> @@ -145,7 +147,9 @@ check_patches \
>> # the set of OVS patch ports doesn't change.
>> AT_CHECK([ovn-sbctl \
>> -- --id=@dp1 create Datapath_Binding tunnel_key=1 external_ids:name=dp1
>> \
>> + type=logical-switch \
>> -- --id=@dp2 create Datapath_Binding tunnel_key=2 external_ids:name=dp2
>> \
>> + type=logical-switch \
>> -- create Port_Binding datapath=@dp1 logical_port=foo tunnel_key=1
>> type=patch options:peer=bar \
>> -- create Port_Binding datapath=@dp2 logical_port=bar tunnel_key=2
>> type=patch options:peer=foo \
>> -- create Port_Binding datapath=@dp1 logical_port=dp1vif tunnel_key=3 \
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 5e0747fcc..33cd8a4ae 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -1893,8 +1893,8 @@ check ovn-nbctl --wait=sb \
>>
>> check_row_count Datapath_Binding 1
>>
>> -nb_uuid=$(ovn-sbctl get Datapath_Binding . external_ids:logical-router)
>> -lr_uuid=\"$(ovn-nbctl get Logical_Router . _uuid)\"
>> +nb_uuid=$(ovn-sbctl get Datapath_Binding . nb_uuid)
>> +lr_uuid=$(ovn-nbctl get Logical_Router . _uuid)
>> echo nb_uuid="$nb_uuid" lr_uuid="$lr_uuid"
>> AT_CHECK([test "${nb_uuid}" = "${lr_uuid}"])
>>
>> @@ -7582,7 +7582,7 @@ ls1_uuid=$(fetch_column nb:Logical_Switch _uuid)
>>
>> # create a duplicated sb datapath (and an IP_Mulicast record that references
>> # it) on purpose.
>> -AT_CHECK([ovn-sbctl --id=@dp create Datapath_Binding
>> external_ids:logical-switch=$ls1_uuid external_ids:name=ls1 tunnel_key=123
>> -- create IP_Multicast datapath=@dp], [0], [ignore])
>> +AT_CHECK([ovn-sbctl --id=@dp create Datapath_Binding nb_uuid=$ls1_uuid
>> type=logical-switch external_ids:name=ls1 tunnel_key=123 -- create
>> IP_Multicast datapath=@dp], [0], [ignore])
>>
>> # northd should delete one of the datapaths in the end
>> wait_row_count Datapath_Binding 1
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 108de260e..a6c7ae359 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -22194,8 +22194,7 @@ AT_CAPTURE_FILE([sbflows])
>> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int \
>> | grep "check_pkt_larger" | wc -l], [0], [[0
>> ]])
>> -dp_uuid=$(ovn-sbctl find datapath_binding | grep lr0 -B2 | grep _uuid | \
>> -awk '{print $3}')
>> +dp_uuid=$(fetch_column Datapath_Binding _uuid external-ids:name=lr0)
>> check_uuid ovn-sbctl create MAC_Binding ip=172.168.0.3 datapath=$dp_uuid \
>> logical_port=lr0-public mac="00\:00\:00\:12\:af\:11"
>>
>> @@ -22265,8 +22264,7 @@ check ovn-nbctl lr-nat-add lr1 snat 172.168.0.100
>> 10.0.0.0/24
>> check ovn-nbctl lr-nat-add lr1 snat 2000::1 1000::/64
>> check ovn-nbctl --wait=sb sync
>>
>> -dp_uuid=$(ovn-sbctl find datapath_binding | grep lr1 -B2 | grep _uuid | \
>> -awk '{print $3}')
>> +dp_uuid=$(fetch_column Datapath_Binding _uuid external-ids:name=lr1)
>> check_uuid ovn-sbctl create MAC_Binding ip=172.168.0.3 datapath=$dp_uuid \
>> logical_port=lr1-public mac="00\:00\:00\:12\:af\:11"
>>
>> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
>> index 60cc39149..74f759699 100644
>> --- a/utilities/ovn-sbctl.c
>> +++ b/utilities/ovn-sbctl.c
>> @@ -382,6 +382,7 @@ pre_get_info(struct ctl_context *ctx)
>> ovsdb_idl_add_column(ctx->idl, &sbrec_logical_dp_group_col_datapaths);
>>
>> ovsdb_idl_add_column(ctx->idl,
>> &sbrec_datapath_binding_col_external_ids);
>> + ovsdb_idl_add_column(ctx->idl, &sbrec_datapath_binding_col_nb_uuid);
>>
>> ovsdb_idl_add_column(ctx->idl, &sbrec_ip_multicast_col_datapath);
>> ovsdb_idl_add_column(ctx->idl, &sbrec_ip_multicast_col_seq_no);
>> @@ -1493,8 +1494,7 @@ static const struct ctl_table_class
>> tables[SBREC_N_TABLES] = {
>> [SBREC_TABLE_DATAPATH_BINDING].row_ids
>> = {{&sbrec_datapath_binding_col_external_ids, "name", NULL},
>> {&sbrec_datapath_binding_col_external_ids, "name2", NULL},
>> - {&sbrec_datapath_binding_col_external_ids, "logical-switch", NULL},
>> - {&sbrec_datapath_binding_col_external_ids, "logical-router", NULL}},
>> + {&sbrec_datapath_binding_col_nb_uuid, NULL, NULL}},
>>
>> [SBREC_TABLE_PORT_BINDING].row_ids
>> = {{&sbrec_port_binding_col_logical_port, NULL, NULL},
>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
>> index 0331eac21..fc79c27ed 100644
>> --- a/utilities/ovn-trace.c
>> +++ b/utilities/ovn-trace.c
>> @@ -703,10 +703,7 @@ read_datapaths(void)
>> const struct smap *ids = &sbdb->external_ids;
>>
>> dp->sb_uuid = sbdb->header_.uuid;
>> - if (!smap_get_uuid(ids, "logical-switch", &dp->nb_uuid) &&
>> - !smap_get_uuid(ids, "logical-router", &dp->nb_uuid)) {
>> - dp->nb_uuid = dp->sb_uuid;
>> - }
>> + dp->nb_uuid = sbdb->nb_uuid ? *sbdb->nb_uuid : dp->sb_uuid;
In theory ovn-trace doesn't need ovn-northd to run, it just needs a
conenction to the SB. I personally use it often (compiled from latest
source) and point it to Southbound databases from deployments with older
OVN versions. In line with my other comments above, I think we could
also ensure backwards compatibility here too.
>>
>> const char *name = smap_get(ids, "name");
>> dp->name = (name
>> --
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
