On Thu, Oct 9, 2025 at 5:49 PM Mairtin O'Loingsigh via dev < [email protected]> wrote:
> This patch adds support for managing transit routers ports. > Ports create on a transit router with be duplicated across all AZs. If > the port has a requested chassis, all ports except the port in the > chassis' AZ will be remote. > > This series also add support for commands to manage transit router ports > > Commands to add and delete a transit router port. > $ ovn-ic-nbctl trp-add tr0 tr0-p0 00:00:00:11:22:00 192.168.10.10/24 > 192.168.10.20/24 chassis=ovn-chassis-1 > $ ovn-ic-nbctl trp-del tr0-p0 > > Reported-at: https://issues.redhat.com/browse/FDP-1477 > Reported-at: https://issues.redhat.com/browse/FDP-1478 > Signed-off-by: Mairtin O'Loingsigh <[email protected]> > --- > Hi Mairtin, thank you for the v3. I have a couple of comments down below. Please separate the ovn-util into its own commit. > NEWS | 3 + > ic/ovn-ic.c | 623 ++++++++++++++++++++++++++++++++++----- > lib/ovn-util.c | 95 ++++++ > lib/ovn-util.h | 14 + > tests/ovn-ic-nbctl.at | 27 ++ > tests/ovn-ic.at | 171 +++++++++++ > utilities/ovn-ic-nbctl.c | 184 ++++++++++++ > utilities/ovn-nbctl.c | 97 ------ > 8 files changed, 1046 insertions(+), 168 deletions(-) > > diff --git a/NEWS b/NEWS > index 593af397d..4648d9d87 100644 > --- a/NEWS > +++ b/NEWS > @@ -49,6 +49,9 @@ Post v25.09.0 > * Support the creation of Transit Routers. > * Added new ovn-ic-nbctl 'tr-add','tr-del','tr-list' commands to > manage > Transit Router. > + * Support the creation of Transit Router Ports. > + * Added new ovn-ic-nbctl 'trp-add' and 'tpr-del' commands to manage > + Transit Router Ports. > OVN v25.09.0 - xxx xx xxxx > -------------------------- > - STT tunnels are no longer supported in ovn-encap-type. > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index cbd325a31..d650fc95d 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -78,6 +78,8 @@ struct ic_context { > struct ovsdb_idl_index *icsbrec_port_binding_by_az; > struct ovsdb_idl_index *icsbrec_port_binding_by_ts; > struct ovsdb_idl_index *icsbrec_port_binding_by_ts_az; > + struct ovsdb_idl_index *icsbrec_port_binding_by_nb_ic_uuid; > + struct ovsdb_idl_index *icsbrec_port_binding_by_nb_ic_uuid_type; > struct ovsdb_idl_index *icsbrec_route_by_az; > struct ovsdb_idl_index *icsbrec_route_by_ts; > struct ovsdb_idl_index *icsbrec_route_by_ts_az; > @@ -686,6 +688,31 @@ find_ts_in_nb(struct ic_context *ctx, char *ts_name) > return NULL; > } > > +static const struct nbrec_logical_router* > +find_tr_in_nb(struct ic_context *ctx, char *tr_name) > +{ > + const struct nbrec_logical_router *key = > + nbrec_logical_router_index_init_row(ctx->nbrec_lr_by_name); > + nbrec_logical_router_index_set_name(key, tr_name); > + > + const struct nbrec_logical_router *lr; > + bool found = false; > + NBREC_LOGICAL_ROUTER_FOR_EACH_EQUAL (lr, key, ctx->nbrec_lr_by_name) { > + if (smap_get(&lr->options, "interconn-tr")) { > + found = true; > + break; > + } > + > + } > + nbrec_logical_router_index_destroy_row(key); > + > + if (found) { > + return lr; > + } > + > + return NULL; > +} > + > static const struct sbrec_port_binding * > find_sb_pb_by_name(struct ovsdb_idl_index *sbrec_port_binding_by_name, > const char *name) > @@ -789,6 +816,23 @@ sync_lsp_tnl_key(const struct > nbrec_logical_switch_port *lsp, > > } > > +static void > +sync_lrp_tnl_key(const struct nbrec_logical_router_port *lrp, > + int64_t isb_tnl_key) > +{ > + int64_t tnl_key = smap_get_int(&lrp->options, "requested-tnl-key", 0); > + if (tnl_key != isb_tnl_key) { > + VLOG_DBG("Set options:requested-tnl-key %"PRId64 > + " for lrp %s in NB.", isb_tnl_key, lrp->name); > + char *tnl_key_str = xasprintf("%"PRId64, isb_tnl_key); > + nbrec_logical_router_port_update_options_setkey(lrp, > + > "requested-tnl-key", > + tnl_key_str); > + free(tnl_key_str); > + } > + > +} > + > static bool > get_router_uuid_by_sb_pb(struct ic_context *ctx, > const struct sbrec_port_binding *sb_pb, > @@ -875,6 +919,44 @@ sync_local_port(struct ic_context *ctx, > sync_lsp_tnl_key(lsp, isb_pb->tunnel_key); > } > > +struct ic_trp_data { > + const struct icnbrec_transit_router_port *trp; > + char *name; > + char *tr_name; > + char *mac; > + char *chassis; > + struct uuid nb_ic_uuid; > + const struct nbrec_logical_router_port *lrp; > +}; > This struct is not needed at all, the required info is in the ICNB LRP. > + > +/* For each local port: > + * - Sync from NB to ISB. > + * - Sync gateway from SB to ISB. > + * - Sync tunnel key from ISB to NB. > + */ > The comment is wrong. We are not syncing from NB to ISB, the requested chassis is from INB to NB. > +static void > +sync_local_router_port(const struct icsbrec_port_binding *isb_pb, > + struct ic_trp_data *trp_data) > The sync local_router_port and remote_router_port should be the same. The only difference is the chassis, if set (remote) set it to all NB LRP via "requested-chassis". If not set (local) remove the "requested-chassis" option from NB LRP. This is also not syncing networks but it should be. > +{ > + const struct nbrec_logical_router_port *lrp = trp_data->lrp; > + > + /* Sync address from NB to ISB */ > + if (strcmp(lrp->mac, isb_pb->address)) { > + nbrec_logical_router_port_set_mac(lrp, isb_pb->address); > + } > + > + const char *chassis_name = smap_get(&lrp->options, > "requested-chassis"); > + if (chassis_name) { > + if (!trp_data->chassis[0]) { > + nbrec_logical_router_port_update_options_setkey(lrp, > + "requested-chassis", trp_data->chassis); > + } > + } > + > + /* Sync back tunnel key from ISB to NB */ > + sync_lrp_tnl_key(lrp, isb_pb->tunnel_key); > +} > + > /* For each remote port: > * - Sync from ISB to NB > * - Sync gateway from ISB to SB > @@ -926,6 +1008,29 @@ sync_remote_port(struct ic_context *ctx, > } > } > > +/* For each remote port: > + * - Sync from ISB to NB > + */ > +static void > +sync_remote_router_port(const struct icsbrec_port_binding *isb_pb, > + const struct ic_trp_data *trp_data) > +{ > + const struct nbrec_logical_router_port *lrp = trp_data->lrp; > + > + /* Sync from ICNB to NB */ > + if (trp_data->chassis[0]) { > + const char *chassis_name = smap_get(&lrp->options, > + "requested-chassis"); > + if (!chassis_name || strcmp(trp_data->chassis, chassis_name)) { > + nbrec_logical_router_port_update_options_setkey(lrp, > + "requested-chassis", trp_data->chassis); > + } > + } > + > + /* Sync tunnel key from ISB to NB */ > + sync_lrp_tnl_key(lrp, isb_pb->tunnel_key); > +} > + > static void > create_nb_lsp(struct ic_context *ctx, > const struct icsbrec_port_binding *isb_pb, > @@ -952,7 +1057,10 @@ create_isb_pb(struct ic_context *ctx, > const struct sbrec_port_binding *sb_pb, > const struct icsbrec_availability_zone *az, > const char *ts_name, > - uint32_t pb_tnl_key) > + const struct uuid *nb_ic_uuid, > + const char *type, > + const char *mac, > + int64_t pb_tnl_key) > { > const struct icsbrec_port_binding *isb_pb = > icsbrec_port_binding_insert(ctx->ovnisb_txn); > @@ -960,10 +1068,17 @@ create_isb_pb(struct ic_context *ctx, > icsbrec_port_binding_set_transit_switch(isb_pb, ts_name); > icsbrec_port_binding_set_logical_port(isb_pb, sb_pb->logical_port); > icsbrec_port_binding_set_tunnel_key(isb_pb, pb_tnl_key); > + icsbrec_port_binding_set_nb_ic_uuid(isb_pb, nb_ic_uuid, 1); > + icsbrec_port_binding_set_type(isb_pb, type); > > - const char *address = get_lp_address_for_sb_pb(ctx, sb_pb); > - if (address) { > - icsbrec_port_binding_set_address(isb_pb, address); > + if (!strcmp(type, "transit-switch-port")) { > + const char *address = get_lp_address_for_sb_pb(ctx, sb_pb); > + if (address) { > + icsbrec_port_binding_set_address(isb_pb, address); > + } > + } else { > + icsbrec_port_binding_set_address(isb_pb, mac); > + return; > } > > const struct sbrec_port_binding *crp = find_crp_for_sb_pb(ctx, sb_pb); > @@ -982,10 +1097,9 @@ create_isb_pb(struct ic_context *ctx, > } > > static const struct sbrec_port_binding * > -find_lsp_in_sb(struct ic_context *ctx, > - const struct nbrec_logical_switch_port *lsp) > +find_lp_in_sb(struct ic_context *ctx, const char *name) > { > - return find_sb_pb_by_name(ctx->sbrec_port_binding_by_name, lsp->name); > + return find_sb_pb_by_name(ctx->sbrec_port_binding_by_name, name); > } > > static uint32_t > @@ -996,53 +1110,304 @@ allocate_port_key(struct hmap *pb_tnlids) > 1, (1u << 15) - 1, &hint); > } > > +struct ic_pb_info { > + struct hmap_node node; > + struct uuid uuid; > + const struct icsbrec_port_binding *isb_pb; > + char *logical_port; > +}; > + > +static struct ic_pb_info * > +ic_pb_create(const struct icsbrec_port_binding *isb_pb) > +{ > + struct ic_pb_info *info = xmalloc(sizeof *info); > + *info = (struct ic_pb_info) { > + .isb_pb = isb_pb, > + .uuid = *isb_pb->nb_ic_uuid, > + .logical_port = xstrdup(isb_pb->logical_port), > + }; > + return info; > +} > + > +static size_t > +ic_pb_hash(const struct uuid *nb_ic_uuid, const char *logical_port) > +{ > + size_t hash = uuid_hash(nb_ic_uuid); > + hash = hash_string(logical_port, hash); > + return hash; > +} > + > static void > -port_binding_run(struct ic_context *ctx) > +ic_pb_add(struct hmap *dp_info, struct ic_pb_info *info) > { > - if (!ctx->ovnisb_txn || !ctx->ovnnb_txn || !ctx->ovnsb_txn) { > - return; > + size_t hash = ic_pb_hash(info->isb_pb->nb_ic_uuid, > + info->isb_pb->logical_port); > + hmap_insert(dp_info, &info->node, hash); > +} > ic_pb_add and ic_pb_create can be combined into single function, it's always used together. > + > +static struct ic_pb_info * > +ic_pb_find(struct hmap *dp_info, const struct uuid *nb_ic_uuid, > + const char *logical_port) > +{ > + size_t hash = ic_pb_hash(nb_ic_uuid, logical_port); > + > + struct ic_pb_info *info; > + HMAP_FOR_EACH_WITH_HASH (info, node, hash, dp_info) { > + if (uuid_equals(&info->uuid, nb_ic_uuid) && > + !strcmp(info->logical_port, logical_port)) { > + return info; > + } > + > } > + return NULL; > +} > > - struct shash isb_all_local_pbs = > SHASH_INITIALIZER(&isb_all_local_pbs); > - struct shash_node *node; > +static struct ic_pb_info * > +ic_pb_remove(struct hmap *dp_info, const struct uuid *nb_ic_uuid, > + const char *logical_port) > +{ > find + hmap_remove will do the same trick, I don't think there is a need for special function like this. > + size_t hash = uuid_hash(nb_ic_uuid); > + hash = hash_string(logical_port, hash); + struct ic_pb_info *info; > + HMAP_FOR_EACH_WITH_HASH (info, node, hash, dp_info) { > + if (uuid_equals(&info->uuid, nb_ic_uuid) && > + !strcmp(info->logical_port, logical_port)) { > > - const struct icsbrec_port_binding *isb_pb; > - const struct icsbrec_port_binding *isb_pb_key = > - > icsbrec_port_binding_index_init_row(ctx->icsbrec_port_binding_by_az); > - icsbrec_port_binding_index_set_availability_zone(isb_pb_key, > - ctx->runned_az); > + hmap_remove(dp_info, &info->node); > + return info; > + } > + > + } > + return NULL; > +} > + > +static const struct icsbrec_port_binding * > +ic_pb_free(struct ic_pb_info *pb_info) > +{ > This should be called destroy and shouldn't return anything. To align with the rest of the codebase you can just get the isb_pb directly no need for a special function that does multiple things. > + if (pb_info) { > + const struct icsbrec_port_binding *isb_pb = pb_info->isb_pb; > + free(pb_info->logical_port); > + free(pb_info); > + return isb_pb; > + } > + > + return NULL; > +} > + > +static const struct nbrec_logical_router_port * > +get_lrp_by_lrp_name(struct ic_context *ctx, const char *lrp_name) > +{ > + const struct nbrec_logical_router_port *lrp; > + const struct nbrec_logical_router_port *lrp_key = > + nbrec_logical_router_port_index_init_row(ctx->nbrec_lrp_by_name); > + nbrec_logical_router_port_index_set_name(lrp_key, lrp_name); > + lrp = nbrec_logical_router_port_index_find(ctx->nbrec_lrp_by_name, > + lrp_key); > + nbrec_logical_router_port_index_destroy_row(lrp_key); > + > + return lrp; > +} > + > +static void > +trp_parse(struct ic_context *ctx, struct shash *trp_ports) > +{ > There is no need for this function whatsoever see down below. > + const struct icnbrec_transit_router_port *trp; > + ICNBREC_TRANSIT_ROUTER_PORT_FOR_EACH (trp, ctx->ovninb_idl) { > + const struct nbrec_logical_router *lr = find_tr_in_nb(ctx, > + trp->transit_router); > + if (!lr) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "Transit Router Port %s or lr: %s not > found.", > + trp->name, trp->transit_router); > + continue; > + } > + > + const struct nbrec_logical_router_port *lrp = > + get_lrp_by_lrp_name(ctx, trp->name); > + if (!lrp) { > + lrp = nbrec_logical_router_port_insert(ctx->ovnnb_txn); > + nbrec_logical_router_port_set_name(lrp, trp->name); > + nbrec_logical_router_port_set_mac(lrp, trp->mac); > + if (strcmp("", trp->chassis)) { > + nbrec_logical_router_port_update_options_setkey(lrp, > + "requested-chassis", trp->chassis); > + } > + > + nbrec_logical_router_port_set_networks(lrp, > + (const char **) trp->networks, trp->n_networks); > + > + nbrec_logical_router_port_update_options_setkey(lrp, > + "interconn-tr", lr->name); > + > + nbrec_logical_router_update_ports_addvalue(lr, lrp); > + } > + > + struct ic_trp_data *trp_data = shash_find_data(trp_ports, > + trp->name); > + if (!trp_data) { > + trp_data = xmalloc(sizeof *trp_data); > + *trp_data = (struct ic_trp_data) { > + .mac = xstrdup(trp->mac), > + .name = xstrdup(trp->name), > + .chassis = xstrdup(trp->chassis), > + .tr_name = xstrdup(trp->transit_router), > + .nb_ic_uuid = *trp->nb_ic_uuid, > + .lrp = lrp, > + .trp = trp, > + }; > + shash_add(trp_ports, trp->name, trp_data); > + } > > + } > +} > + > +static bool > +trp_port_is_remote(struct ic_context *ctx, > + const struct nbrec_logical_router_port *lrp) { > + > + const char *chassis_name = smap_get(&lrp->options, > "requested-chassis"); > + if (chassis_name) { > + const struct sbrec_chassis *chassis = find_sb_chassis(ctx, > + chassis_name); > + if (chassis) { > + return smap_get_bool(&chassis->other_config, "is-remote", > false); > + } > + > + } > + > + return false; > +} > Do we need this function? It's northd problem to solve this, if we only set the requested-chassis when chassis is set for TRP it's fine, nothing else to do. > + > +static const struct icsbrec_port_binding * > +trp_search_all_port_bindings(struct ic_context *ctx, > + struct ic_trp_data *trp_data) > +{ > + > + const struct icsbrec_port_binding *isb_pb_key; > + isb_pb_key = icsbrec_port_binding_index_init_row( > + ctx->icsbrec_port_binding_by_nb_ic_uuid_type); > + icsbrec_port_binding_index_set_nb_ic_uuid(isb_pb_key, > + &trp_data->nb_ic_uuid, 1); > + icsbrec_port_binding_index_set_type(isb_pb_key, > "transit-router-port"); > + > + const struct icsbrec_port_binding *isb_pb = NULL; > ICSBREC_PORT_BINDING_FOR_EACH_EQUAL (isb_pb, isb_pb_key, > - ctx->icsbrec_port_binding_by_az) > { > - shash_add(&isb_all_local_pbs, isb_pb->logical_port, isb_pb); > + ctx->icsbrec_port_binding_by_nb_ic_uuid_type) { > + if (!strcmp(isb_pb->logical_port, trp_data->name)) { > + break; > + } > } > icsbrec_port_binding_index_destroy_row(isb_pb_key); > > + return isb_pb; > +} > + > +static void > +trp_port_sync(struct ic_context *ctx, > + struct ic_trp_data *trp_data, > + const struct icsbrec_availability_zone *az, > + struct hmap *local_pbs, struct hmap *remote_pbs, > + struct hmap *pb_tnlids, > + const struct sbrec_port_binding *sb_pb) > +{ > + > + bool is_remote_port = trp_port_is_remote(ctx, trp_data->lrp); > + if (!is_remote_port) { > + const struct icsbrec_port_binding *isb_pb = NULL; > + struct ic_pb_info *info = ic_pb_find(local_pbs, > &trp_data->nb_ic_uuid, > + trp_data->name); > + if (info) { > + hmap_remove(local_pbs, &info->node); > + isb_pb = ic_pb_free(info); > + } else { > + isb_pb = trp_search_all_port_bindings(ctx, trp_data); > + } > + > + if (!isb_pb) { > + int64_t pb_tnl_key = allocate_port_key(pb_tnlids); > + create_isb_pb(ctx, sb_pb, az, trp_data->name, > + &trp_data->nb_ic_uuid, > + "transit-router-port", trp_data->mac, pb_tnl_key); > + } else { > + sync_local_router_port(isb_pb, trp_data); > + } + > + } else { > + const struct icsbrec_port_binding *isb_pb = NULL; > + struct ic_pb_info *info = ic_pb_find(remote_pbs, > &trp_data->nb_ic_uuid, > + trp_data->name); > + if (info) { > + hmap_remove(remote_pbs, &info->node); > + isb_pb = ic_pb_free(info); > + } > + > + if (isb_pb) { > + sb_pb = find_lp_in_sb(ctx, trp_data->name); > + if (!sb_pb) { > + return; > + } > + > + sync_remote_router_port(isb_pb, trp_data); > + } > + > + } > This should be greatly once we have only one sync function. > +} > + > +static void > +port_binding_run(struct ic_context *ctx) > +{ > + if (!ctx->ovnisb_txn || !ctx->ovnnb_txn || !ctx->ovnsb_txn) { > + return; > + } > + > + struct hmap isb_all_local_pbs = HMAP_INITIALIZER(&isb_all_local_pbs); > + const struct icsbrec_port_binding *current_isb_pb; > + struct hmap pb_tnlids = HMAP_INITIALIZER(&pb_tnlids); > + ICSBREC_PORT_BINDING_FOR_EACH (current_isb_pb, ctx->ovnisb_idl) { > + if (!strcmp(ctx->runned_az->name, > + current_isb_pb->availability_zone->name)) { > + struct ic_pb_info *pb_info = ic_pb_create(current_isb_pb); > + ic_pb_add(&isb_all_local_pbs, pb_info); > + } > + > + ovn_add_tnlid(&pb_tnlids, current_isb_pb->tunnel_key); > + } > + > const struct sbrec_port_binding *sb_pb; > const struct icnbrec_transit_switch *ts; > + > ICNBREC_TRANSIT_SWITCH_FOR_EACH (ts, ctx->ovninb_idl) { > const struct nbrec_logical_switch *ls = find_ts_in_nb(ctx, > ts->name); > if (!ls) { > VLOG_DBG("Transit switch %s not found in NB.", ts->name); > continue; > } > - struct shash local_pbs = SHASH_INITIALIZER(&local_pbs); > - struct shash remote_pbs = SHASH_INITIALIZER(&remote_pbs); > - struct hmap pb_tnlids = HMAP_INITIALIZER(&pb_tnlids); > - isb_pb_key = icsbrec_port_binding_index_init_row( > - ctx->icsbrec_port_binding_by_ts); > - icsbrec_port_binding_index_set_transit_switch(isb_pb_key, > ts->name); > + struct hmap local_pbs = HMAP_INITIALIZER(&local_pbs); > + struct hmap remote_pbs = HMAP_INITIALIZER(&remote_pbs); > > - ICSBREC_PORT_BINDING_FOR_EACH_EQUAL (isb_pb, isb_pb_key, > - > ctx->icsbrec_port_binding_by_ts) { > - if (isb_pb->availability_zone == ctx->runned_az) { > - shash_add(&local_pbs, isb_pb->logical_port, isb_pb); > - shash_find_and_delete(&isb_all_local_pbs, > - isb_pb->logical_port); > + const struct icsbrec_port_binding *isb_pb_key; > + isb_pb_key = icsbrec_port_binding_index_init_row( > + ctx->icsbrec_port_binding_by_nb_ic_uuid_type); > + icsbrec_port_binding_index_set_nb_ic_uuid(isb_pb_key, > + &ts->header_.uuid, 1); > + icsbrec_port_binding_index_set_type(isb_pb_key, > "transit-switch-port"); > + ICSBREC_PORT_BINDING_FOR_EACH_EQUAL (current_isb_pb, isb_pb_key, > + ctx->icsbrec_port_binding_by_nb_ic_uuid_type) { > + if (current_isb_pb->availability_zone == ctx->runned_az) { > + struct ic_pb_info *pb_info = > ic_pb_remove(&isb_all_local_pbs, > + current_isb_pb->nb_ic_uuid, > current_isb_pb->logical_port); > + ic_pb_free(pb_info); > + pb_info = ic_pb_create(current_isb_pb); > + ic_pb_add(&local_pbs, pb_info); > } else { > - shash_add(&remote_pbs, isb_pb->logical_port, isb_pb); > + struct ic_pb_info *pb_info = ic_pb_find(&remote_pbs, > + current_isb_pb->nb_ic_uuid, > current_isb_pb->logical_port); > + if (!pb_info) { > + pb_info = ic_pb_create(current_isb_pb); > + ic_pb_add(&remote_pbs, pb_info); > + } > + > nit: Extra empty line that also applies to the rest of the change. > } > - ovn_add_tnlid(&pb_tnlids, isb_pb->tunnel_key); > } > icsbrec_port_binding_index_destroy_row(isb_pb_key); > > @@ -1053,25 +1418,42 @@ port_binding_run(struct ic_context *ctx) > if (!strcmp(lsp->type, "router") > || !strcmp(lsp->type, "switch")) { > /* The port is local. */ > - sb_pb = find_lsp_in_sb(ctx, lsp); > + sb_pb = find_lp_in_sb(ctx, lsp->name); > if (!sb_pb) { > continue; > } > - isb_pb = shash_find_and_delete(&local_pbs, lsp->name); > + > + const struct icsbrec_port_binding *isb_pb = NULL; > + static struct ic_pb_info *pb_info; > + pb_info = ic_pb_find(&local_pbs, &ts->header_.uuid, > + lsp->name); > + if (pb_info) { > + hmap_remove(&local_pbs, &pb_info->node); > + isb_pb = ic_pb_free(pb_info); > + } > if (!isb_pb) { > uint32_t pb_tnl_key = allocate_port_key(&pb_tnlids); > - create_isb_pb(ctx, sb_pb, ctx->runned_az, > - ts->name, pb_tnl_key); > + create_isb_pb(ctx, sb_pb, ctx->runned_az, ts->name, > + &ts->header_.uuid, "transit-switch-port", NULL, > + pb_tnl_key); > } else { > sync_local_port(ctx, isb_pb, sb_pb, lsp); > } > + > } else if (!strcmp(lsp->type, "remote")) { > /* The port is remote. */ > - isb_pb = shash_find_and_delete(&remote_pbs, lsp->name); > + struct ic_pb_info *pb_info; > + pb_info = ic_pb_find(&remote_pbs, &ts->header_.uuid, > + lsp->name); > + const struct icsbrec_port_binding *isb_pb = NULL; > + if (pb_info) { > + hmap_remove(&remote_pbs, &pb_info->node); > + isb_pb = ic_pb_free(pb_info); > + } > if (!isb_pb) { > nbrec_logical_switch_update_ports_delvalue(ls, lsp); > } else { > - sb_pb = find_lsp_in_sb(ctx, lsp); > + sb_pb = find_lp_in_sb(ctx, lsp->name); > if (!sb_pb) { > continue; > } > @@ -1083,26 +1465,134 @@ port_binding_run(struct ic_context *ctx) > } > } > > - /* Delete extra port-binding from ISB */ > - SHASH_FOR_EACH (node, &local_pbs) { > - icsbrec_port_binding_delete(node->data); > + struct ic_pb_info *info; > + HMAP_FOR_EACH_POP (info, node, &local_pbs) { > + const struct icsbrec_port_binding *isb_pb = ic_pb_free(info); > + if (isb_pb) { > + icsbrec_port_binding_delete(isb_pb); > + } > + > + } > + > + /* Create lsp in NB for remote ports */ > + HMAP_FOR_EACH_POP (info, node, &remote_pbs) { > + const struct icsbrec_port_binding *isb_pb; > + isb_pb = ic_pb_free(info); > + create_nb_lsp(ctx, isb_pb, ls); > + } > + > + hmap_destroy(&local_pbs); > + hmap_destroy(&remote_pbs); > + } > + > + /* Walk list of transit router ports */ > + struct shash trp_ports = SHASH_INITIALIZER(&trp_ports); > + trp_parse(ctx, &trp_ports); > I think the approach here is wrong. We should be iterating over TRs and their ports (schema change). For each port we should create ICSB PB if it doesn't exist in current az. We should always sync the ICNB LRP to NB LR no matter if it existed or not (it could have changed). For that we probably require only shash TBF. So we can most likely completely remove the new "struct ic_pb_info". + > + const struct icnbrec_transit_router *tr; > + /* Transit Router port binding */ > + ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->ovninb_idl) { > + const struct nbrec_logical_router *lr = find_tr_in_nb(ctx, > tr->name); > + if (!lr) { > + VLOG_DBG("Transit router %s not found in NB.", tr->name); > + continue; > + } > + > + struct hmap local_pbs = HMAP_INITIALIZER(&local_pbs); > + struct hmap remote_pbs = HMAP_INITIALIZER(&remote_pbs); > + const struct icsbrec_port_binding *isb_pb_key; > + isb_pb_key = icsbrec_port_binding_index_init_row( > + ctx->icsbrec_port_binding_by_nb_ic_uuid_type); > + icsbrec_port_binding_index_set_nb_ic_uuid(isb_pb_key, > + &tr->header_.uuid, 1); > + icsbrec_port_binding_index_set_type(isb_pb_key, > "transit-router-port"); > + > + const struct icsbrec_port_binding *isb_pb; > + ICSBREC_PORT_BINDING_FOR_EACH_EQUAL (isb_pb, isb_pb_key, > + > ctx->icsbrec_port_binding_by_nb_ic_uuid_type) { > + if (isb_pb->availability_zone == ctx->runned_az) { > + struct ic_pb_info *pb_info = > ic_pb_remove(&isb_all_local_pbs, > + isb_pb->nb_ic_uuid, isb_pb->logical_port); > + ic_pb_add(&local_pbs, pb_info); > + > + } else { > + struct ic_pb_info *pb_info = ic_pb_find(&remote_pbs, > + isb_pb->nb_ic_uuid, isb_pb->logical_port); > + if (!pb_info) { > + pb_info = ic_pb_create(isb_pb); > + ic_pb_add(&remote_pbs, pb_info); > + } > + > + } > + } > + icsbrec_port_binding_index_destroy_row(isb_pb_key); > + > + for (size_t i = 0; i < lr->n_ports; i++) { > + const struct nbrec_logical_router_port *lrp; > + lrp = lr->ports[i]; > + > + sb_pb = find_lp_in_sb(ctx, lrp->name); > + if (!sb_pb) { > + continue; > + } > + > + /* Check if port came from ICNB */ > + struct ic_trp_data *trp_data = shash_find_data(&trp_ports, > + lrp->name); > + if (trp_data) { > + trp_port_sync(ctx, trp_data, ctx->runned_az, &local_pbs, > + &remote_pbs, &pb_tnlids, sb_pb); > + } else { > + if (smap_get(&lrp->options, "interconn-tr")) { > + nbrec_logical_router_port_delete(lrp); > + nbrec_logical_router_update_ports_delvalue(lr, lrp); > + } > + > + } > + > + } > + > + struct ic_pb_info *info; > + HMAP_FOR_EACH_POP (info, node, &local_pbs) { > + isb_pb = ic_pb_free(info); > + icsbrec_port_binding_delete(isb_pb); > } > > /* Create lsp in NB for remote ports */ > - SHASH_FOR_EACH (node, &remote_pbs) { > - create_nb_lsp(ctx, node->data, ls); > + HMAP_FOR_EACH_POP (info, node, &remote_pbs) { > + isb_pb = ic_pb_free(info); > } > > - shash_destroy(&local_pbs); > - shash_destroy(&remote_pbs); > - ovn_destroy_tnlids(&pb_tnlids); > + hmap_destroy(&local_pbs); > + hmap_destroy(&remote_pbs); > } > + ovn_destroy_tnlids(&pb_tnlids); > + > + struct ic_pb_info *info; > + HMAP_FOR_EACH_SAFE (info, node, &isb_all_local_pbs) { > + const struct icsbrec_port_binding *isb_pb; > + struct ic_pb_info *pb_info; > + pb_info = ic_pb_remove(&isb_all_local_pbs, &info->uuid, > + info->logical_port); > + isb_pb = ic_pb_free(pb_info); > + if (isb_pb) { > + icsbrec_port_binding_delete(isb_pb); > + } > > - SHASH_FOR_EACH (node, &isb_all_local_pbs) { > - icsbrec_port_binding_delete(node->data); > } > > - shash_destroy(&isb_all_local_pbs); > + hmap_destroy(&isb_all_local_pbs); > + > + struct shash_node *node; > + SHASH_FOR_EACH (node, &trp_ports) { > + struct ic_trp_data *trp_port = node->data; > + free(trp_port->name); > + free(trp_port->mac); > + free(trp_port->tr_name); > + free(trp_port->chassis); > + free(trp_port); > + } > + shash_destroy(&trp_ports); > } > > struct ic_router_info { > @@ -1783,20 +2273,6 @@ get_lrp_name_by_ts_port_name(struct ic_context > *ctx, const char *ts_port_name) > return smap_get(&nb_lsp->options, "router-port"); > } > > -static const struct nbrec_logical_router_port * > -get_lrp_by_lrp_name(struct ic_context *ctx, const char *lrp_name) > -{ > - const struct nbrec_logical_router_port *lrp; > - const struct nbrec_logical_router_port *lrp_key = > - nbrec_logical_router_port_index_init_row(ctx->nbrec_lrp_by_name); > - nbrec_logical_router_port_index_set_name(lrp_key, lrp_name); > - lrp = nbrec_logical_router_port_index_find(ctx->nbrec_lrp_by_name, > - lrp_key); > - nbrec_logical_router_port_index_destroy_row(lrp_key); > - > - return lrp; > -} > - > static const struct nbrec_logical_router_port * > find_lrp_of_nexthop(struct ic_context *ctx, > const struct icsbrec_route *isb_route) > @@ -2333,6 +2809,9 @@ route_run(struct ic_context *ctx) > ICSBREC_PORT_BINDING_FOR_EACH_EQUAL (isb_pb, isb_pb_key, > ctx->icsbrec_port_binding_by_az) > { > + if (isb_pb->type && !strcmp(isb_pb->type, "transit-router-port")) > { > + continue; > + } > const struct nbrec_logical_switch_port *nb_lsp; > > nb_lsp = get_lsp_by_ts_port_name(ctx, isb_pb->logical_port); > @@ -3347,14 +3826,14 @@ main(int argc, char *argv[]) > = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, > > &icsbrec_port_binding_col_availability_zone); > > - struct ovsdb_idl_index *icsbrec_port_binding_by_ts > + struct ovsdb_idl_index *icsbrec_port_binding_by_nb_ic_uuid > = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, > - > &icsbrec_port_binding_col_transit_switch); > + &icsbrec_port_binding_col_nb_ic_uuid); > > - struct ovsdb_idl_index *icsbrec_port_binding_by_ts_az > + struct ovsdb_idl_index *icsbrec_port_binding_by_nb_ic_uuid_type > = ovsdb_idl_index_create2(ovnisb_idl_loop.idl, > - > &icsbrec_port_binding_col_transit_switch, > - > &icsbrec_port_binding_col_availability_zone); > + &icsbrec_port_binding_col_nb_ic_uuid, > + &icsbrec_port_binding_col_type); > > struct ovsdb_idl_index *icsbrec_route_by_az > = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, > @@ -3443,8 +3922,10 @@ main(int argc, char *argv[]) > .icnbrec_transit_switch_by_name = > icnbrec_transit_switch_by_name, > .icsbrec_port_binding_by_az = icsbrec_port_binding_by_az, > - .icsbrec_port_binding_by_ts = icsbrec_port_binding_by_ts, > - .icsbrec_port_binding_by_ts_az = > icsbrec_port_binding_by_ts_az, > + .icsbrec_port_binding_by_nb_ic_uuid = > + icsbrec_port_binding_by_nb_ic_uuid, > + .icsbrec_port_binding_by_nb_ic_uuid_type = > + icsbrec_port_binding_by_nb_ic_uuid_type, > .icsbrec_route_by_az = icsbrec_route_by_az, > .icsbrec_route_by_ts = icsbrec_route_by_ts, > .icsbrec_route_by_ts_az = icsbrec_route_by_ts_az, > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index 910f67304..f0329aaec 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -1547,3 +1547,98 @@ ovn_is_valid_vni(int64_t vni) > { > return vni >= 0 && (vni <= (1 << 24) - 1); > } > + > +struct sset * > +lrp_network_sset(const char **networks, int n_networks) > +{ > + struct sset *network_set = xzalloc(sizeof *network_set); > + sset_init(network_set); > + for (int i = 0; i < n_networks; i++) { > + char *norm = normalize_prefix_str(networks[i]); > + if (!norm) { > + sset_destroy(network_set); > + free(network_set); > + return NULL; > + } > + sset_add_and_free(network_set, norm); > + } > + return network_set; > +} > + > +char * > +normalize_ipv4_prefix_str(const char *orig_prefix) > +{ > + unsigned int plen; > + ovs_be32 ipv4; > + char *error; > + > + error = ip_parse_cidr(orig_prefix, &ipv4, &plen); > + if (error) { > + free(error); > + return NULL; > + } > + return normalize_ipv4_prefix(ipv4, plen); > +} > + > +char * > +normalize_ipv6_prefix_str(const char *orig_prefix) > +{ > + unsigned int plen; > + struct in6_addr ipv6; > + char *error; > + > + error = ipv6_parse_cidr(orig_prefix, &ipv6, &plen); > + if (error) { > + free(error); > + return NULL; > + } > + return normalize_ipv6_prefix(&ipv6, plen); > +} > + > +char * > +normalize_prefix_str(const char *orig_prefix) > +{ > + char *ret; > + > + ret = normalize_ipv4_prefix_str(orig_prefix); > + if (!ret) { > + ret = normalize_ipv6_prefix_str(orig_prefix); > + } nit: Add empty line. > + return ret; > +} > + > +char * > +normalize_ipv4_addr_str(const char *orig_addr) > +{ > + ovs_be32 ipv4; > + > + if (!ip_parse(orig_addr, &ipv4)) { > + return NULL; > + } > + > + return normalize_ipv4_prefix(ipv4, 32); > +} > + > +char * > +normalize_ipv6_addr_str(const char *orig_addr) > +{ > + struct in6_addr ipv6; > + > + if (!ipv6_parse(orig_addr, &ipv6)) { > + return NULL; > + } > + > + return normalize_ipv6_prefix(&ipv6, 128); > +} > + > +OVS_UNUSED char * > Why is there OVS_UNUSED needed? > +normalize_addr_str(const char *orig_addr) > +{ > + char *ret; > + > + ret = normalize_ipv4_addr_str(orig_addr); > + if (!ret) { > + ret = normalize_ipv6_addr_str(orig_addr); > + } > nit: Add empty line. > + return ret; > +} > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index 3aca4ca75..26b7e133d 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -672,4 +672,18 @@ bool datapath_get_nb_uuid(const struct > sbrec_datapath_binding *sb, > > const char *datapath_get_nb_type(const struct sbrec_datapath_binding *sb); > > +struct sset *lrp_network_sset(const char **networks, int n_networks); > + > +char *normalize_ipv4_prefix_str(const char *orig_prefix); > + > +char *normalize_ipv6_prefix_str(const char *orig_prefix); > + > +char *normalize_prefix_str(const char *orig_prefix); > + > +char *normalize_ipv4_addr_str(const char *orig_addr); > + > +char *normalize_ipv6_addr_str(const char *orig_addr); > + > +OVS_UNUSED char *normalize_addr_str(const char *orig_addr); > + > #endif /* OVN_UTIL_H */ > diff --git a/tests/ovn-ic-nbctl.at b/tests/ovn-ic-nbctl.at > index 058d66cb1..31270b22c 100644 > --- a/tests/ovn-ic-nbctl.at > +++ b/tests/ovn-ic-nbctl.at > @@ -102,5 +102,32 @@ AT_CHECK([ovn-ic-nbctl tr-del tr2], [1], [], > ]) > > AT_CHECK([ovn-ic-nbctl --if-exists tr-del tr2]) > +AT_CHECK([ovn-ic-nbctl tr-del tr0]) > + > +AT_CHECK([ovn-ic-nbctl trp-add tr0 tr0-p0 00:11:22:11:22:33 > 192.168.10.10/24], [1], [], > + [ovn-ic-nbctl: tr0: router name not found > +]) > + > +AT_CHECK([ovn-ic-nbctl tr-add tr0]) > +AT_CHECK([ovn-ic-nbctl trp-add tr0 tr0-p0], [1], [], > + [ovn-ic-nbctl: 'trp-add' command requires at least 4 arguments > +]) > + > +AT_CHECK([ovn-ic-nbctl trp-add tr0 tr0-p0 00:11:22:11:22:33 > 192.168.10.10/24]) > +AT_CHECK([ovn-ic-nbctl trp-add tr0 tr0-p0 00:11:22:11:22:33 > 192.168.10.10/24], [1], [], > + [ovn-ic-nbctl: tr0-p0: a port with this name already exists > +]) > + > +AT_CHECK([ovn-ic-nbctl --may-exist trp-add tr0 tr0-p0 00:11:22:11:22:33 > 192.168.10.10/24]) > + > +AT_CHECK([ovn-ic-nbctl trp-del], [1], [], > + [ovn-ic-nbctl: 'trp-del' command requires at least 1 arguments > +]) > +AT_CHECK([ovn-ic-nbctl trp-del tr0-p0]) > +AT_CHECK([ovn-ic-nbctl trp-del tr0-p0], [1], [], > + [ovn-ic-nbctl: tr0-p0: router port name not found > +]) > +AT_CHECK([ovn-ic-nbctl --if-exists trp-del tr0-p0]) > + > This test is missing setting the chassis and it would be also nice to test out the mac and network collisions. OVN_IC_NBCTL_TEST_STOP > AT_CLEANUP > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > index 424e9be1c..103067410 100644 > --- a/tests/ovn-ic.at > +++ b/tests/ovn-ic.at > @@ -4331,3 +4331,174 @@ check_row_count nb:Logical_Router 1 name=tr0 > OVN_CLEANUP_IC([az1], [az2]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-ic -- Add transit router port]) > + > +ovn_init_ic_db > +ovn_start az1 > +ovn_start az2 > + > +OVS_WAIT_UNTIL([test 2 = `ovn-ic-sbctl show | wc -l`]) > + > +check ovn-ic-nbctl --wait=sb sync > +AT_CHECK([ovn-ic-sbctl show], [0], [dnl > +availability-zone az1 > +availability-zone az2 > +]) > + > +ovn_as az1 > +check ovn-ic-nbctl tr-add tr0 > +check ovn-ic-nbctl trp-add tr0 tr0-p0 00:00:00:11:22:00 192.168.10.10/24 > 192.168.10.20/24 > +OVS_WAIT_UNTIL([test 1 = `ovn-sbctl list Port_Binding | grep tr0-p0 | wc > -l`]) > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0-p0], [0], [dnl > +logical_port : tr0-p0 > +]) > + > +ovn_as az2 > +OVS_WAIT_UNTIL([test 1 = `ovn-sbctl list Port_Binding | grep tr0-p0 | wc > -l`]) > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0-p0], [0], [dnl > +logical_port : tr0-p0 > +]) > + > +OVN_CLEANUP_IC([az1], [az2]) > +AT_CLEANUP > +]) > + > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-ic -- Add transit router remote port]) > + > +ovn_init_ic_db > +net_add n1 > +net_add n2 > +ovn_start az1 > +ovn_start az2 > + > +OVS_WAIT_UNTIL([test 2 = `ovn-ic-sbctl show | wc -l`]) > + > +check ovn-ic-nbctl --wait=sb sync > +AT_CHECK([ovn-ic-sbctl show], [0], [dnl > +availability-zone az1 > +availability-zone az2 > +]) > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_az_attach az1 n1 br-phys 192.168.0.1 > +ovs-vsctl set open . external-ids:ovn-is-interconn=true > + > +sim_add hv2 > +as hv2 > +ovs-vsctl add-br br-phys > +ovn_az_attach az2 n2 br-phys 192.168.0.2 > +ovs-vsctl set open . external-ids:ovn-is-interconn=true > + > +ovn_as az1 > +check ovn-ic-nbctl tr-add tr0 > +check ovn-ic-nbctl trp-add tr0 tr0-p0 00:00:00:11:22:00 192.168.10.10/24 > 192.168.10.20/24 chassis=hv2 > +OVS_WAIT_UNTIL([test 1 = `ovn-sbctl list Port_Binding | grep tr0-p0 | wc > -l`]) > nit: wait_row_count here and in the rest of the newly added tests. > +AT_CHECK([ovn-sbctl list Port_Binding | grep -E '(tr0-p0|type|mac)'], > [0], [dnl > +logical_port : tr0-p0 > +mac : [["00:00:00:11:22:00 192.168.10.10/24 > 192.168.10.20/24"]] > +type : remote > +]) > nit: check_row_count here and and in the rest of the newly added tests. This should also check the chassis column. Same below. > + > +ovn_as az2 > +OVS_WAIT_UNTIL([test 1 = `ovn-sbctl list Port_Binding | grep tr0-p0 | wc > -l`]) > +AT_CHECK([ovn-sbctl list Port_Binding | grep -E '(tr0-p0|type|mac)'], [0], > [dnl > +logical_port : tr0-p0 > +mac : [["00:00:00:11:22:00 192.168.10.10/24 > 192.168.10.20/24"]] > +type : patch > +]) + > +OVN_CLEANUP_IC([az1], [az2]) > +AT_CLEANUP > +]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-ic -- Transit router delete port]) > + > +ovn_init_ic_db > +ovn_start az1 > +ovn_start az2 > + > +OVS_WAIT_UNTIL([test 2 = `ovn-ic-sbctl show | wc -l`]) > + > +check ovn-ic-nbctl --wait=sb sync > +AT_CHECK([ovn-ic-sbctl show], [0], [dnl > +availability-zone az1 > +availability-zone az2 > +]) > + > +ovn_as az1 > +check ovn-ic-nbctl tr-add tr0 > +check ovn-ic-nbctl trp-add tr0 tr0-p0 00:00:00:11:22:00 192.168.10.10/24 > 192.168.10.20/24 chassis=chassis-az2 > +OVS_WAIT_UNTIL([test 1 = `ovn-sbctl list Port_Binding | grep tr0-p0 | wc > -l`]) > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0-p0], [0], [dnl > +logical_port : tr0-p0 > +]) > + > +ovn_as az2 > +OVS_WAIT_UNTIL([test 1 = `ovn-sbctl list Port_Binding | grep tr0-p0 | wc > -l`]) > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0-p0], [0], [dnl > +logical_port : tr0-p0 > +]) > + > +ovn_as az1 > +check ovn-ic-nbctl trp-del tr0-p0 > +OVS_WAIT_UNTIL([test 0 = `ovn-sbctl list Port_Binding | grep tr0-p0 | wc > -l`]) > +AT_CHECK([ovn-sbctl list Port_Binding], [0], [dnl > +]) > + > +OVN_CLEANUP_IC([az1], [az2]) > +AT_CLEANUP > +]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-ic -- Port binding deletion upon transit router deletion]) > + > +ovn_init_ic_db > +ovn_start az1 > +ovn_start az2 > + > +OVS_WAIT_UNTIL([test 2 = `ovn-ic-sbctl show | wc -l`]) > + > +check ovn-ic-nbctl --wait=sb sync > +AT_CHECK([ovn-ic-sbctl show], [0], [dnl > +availability-zone az1 > +availability-zone az2 > +]) > + > +ovn_as az1 > +check ovn-ic-nbctl tr-add tr0 > +check ovn-ic-nbctl trp-add tr0 tr0-p0 00:00:00:11:22:00 192.168.10.10/24 > 192.168.10.20/24 chassis=chassis-az2 > +check ovn-ic-nbctl trp-add tr0 tr0-p1 00:00:00:11:22:00 192.168.10.10/24 > 192.168.10.20/24 > +OVS_WAIT_UNTIL([test 2 = `ovn-sbctl list Port_Binding | grep tr0 | wc > -l`]) > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0 | sort], [0], [dnl > +logical_port : tr0-p0 > +logical_port : tr0-p1 > +]) > + > +ovn_as az2 > +OVS_WAIT_UNTIL([test 2 = `ovn-sbctl list Port_Binding | grep tr0| wc -l`]) > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0 | sort], [0], [dnl > +logical_port : tr0-p0 > +logical_port : tr0-p1 > +]) > + > +ovn_as az1 > +check ovn-ic-nbctl tr-del tr0 > +OVS_WAIT_UNTIL([test 0 = `ovn-sbctl list Port_Binding | grep tr0| wc -l`]) > +AT_CHECK([ovn-sbctl list Port_Binding], [0], [dnl > +]) > + > +AT_CHECK([ovn-ic-nbctl list Transit_Router], [0], [dnl > +]) > +AT_CHECK([ovn-ic-nbctl list Transit_Router_Port], [0], [dnl > +]) > + > +OVN_CLEANUP_IC([az1], [az2]) > +AT_CLEANUP > +]) > I would also add checks for the proper tunnel keys for all TRP created in NB in all newly added tests. > diff --git a/utilities/ovn-ic-nbctl.c b/utilities/ovn-ic-nbctl.c > index 9d1784e86..4f709627e 100644 > --- a/utilities/ovn-ic-nbctl.c > +++ b/utilities/ovn-ic-nbctl.c > @@ -341,6 +341,9 @@ Transit router commands:\n\ > tr-add ROUTER create a transit router named ROUTER\n\ > tr-del ROUTER delete ROUTER\n\ > tr-list print all transit routers\n\ > + trp-add ROUTER PORT MAC [NETWORK]...[chassis=CHASSIS]\n\ > + add a transit router PORT\n\ > + trp-del PORT delete a transit router PORT\n\ > \n\ > Connection commands:\n\ > get-connection print the connections\n\ > @@ -582,6 +585,38 @@ ic_nbctl_tr_add(struct ctl_context *ctx) > icnbrec_transit_router_set_name(tr, tr_name); > } > > +static char * > +trp_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool > must_exist, > + const struct icnbrec_transit_router_port **trp_p) > +{ > + const struct icnbrec_transit_router_port *trp = NULL; > + *trp_p = NULL; > + > + struct uuid trp_uuid; > + bool is_uuid = uuid_from_string(&trp_uuid, id); > + if (is_uuid) { > + trp = icnbrec_transit_router_port_get_for_uuid(ctx->idl, > &trp_uuid); > + } > + > + if (!trp) { > + const struct icnbrec_transit_router_port *iter; > + > + ICNBREC_TRANSIT_ROUTER_PORT_FOR_EACH (iter, ctx->idl) { > + if (!strcmp(iter->name, id)) { > + trp = iter; > + break; > + } > + } > + } > + > + if (!trp && must_exist) { > + return xasprintf("%s: router port %s not found", > + id, is_uuid ? "UUID" : "name"); > + } > + > + *trp_p = trp; > + return NULL; > +} > > static void > ic_nbctl_tr_del(struct ctl_context *ctx) > @@ -603,6 +638,26 @@ ic_nbctl_tr_del(struct ctl_context *ctx) > icnbrec_transit_router_delete(tr); > } > > +static void > +ic_nbctl_trp_del(struct ctl_context *ctx) > +{ > + bool must_exist = !shash_find(&ctx->options, "--if-exists"); > + const char *trp_name = ctx->argv[1]; > + const struct icnbrec_transit_router_port *trp = NULL; > + > + char *error = trp_by_name_or_uuid(ctx, trp_name, must_exist, &trp); > + if (error) { > + ctx->error = error; > + return; > + } > + > + if (!trp) { > + return; > + } > + > + icnbrec_transit_router_port_delete(trp); > +} > + > static void > ic_nbctl_tr_list(struct ctl_context *ctx) > { > @@ -622,6 +677,130 @@ ic_nbctl_tr_list(struct ctl_context *ctx) > smap_destroy(&routers); > free(nodes); > } > + > +static void > +ic_nbctl_trp_add(struct ctl_context *ctx) > +{ > + bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL; > + const char *tr_name = ctx->argv[1]; > + const char *trp_name = ctx->argv[2]; > + const char *mac = ctx->argv[3]; > + const char **networks = (const char **) &ctx->argv[4]; > + const struct icnbrec_transit_router *tr; > + > + char *error = tr_by_name_or_uuid(ctx, tr_name, true, &tr); > + if (error) { > + ctx->error = error; > + return; > + } > + > + const struct icnbrec_transit_router_port *trp; > + error = trp_by_name_or_uuid(ctx, trp_name, false, &trp); > + if (error) { > + ctx->error = error; > + return; > + } > + > + /* Parse networks*/ > + int n_networks = ctx->argc - 4; > + for (int i = 4; i < ctx->argc; i++) { > + if (strchr(ctx->argv[i], '=')) { > + n_networks = i - 4; > + break; > + } > + > + } > + > + char **settings = (char **) &ctx->argv[n_networks + 4]; > + int n_settings = ctx->argc - 4 - n_networks; > + > + struct eth_addr ea; > + if (!eth_addr_from_string(mac, &ea)) { > + ctl_error(ctx, "%s: invalid mac address %s", trp_name, mac); > + return; > + } > + > + if (trp) { > + if (!may_exist) { > + ctl_error(ctx, "%s: a port with this name already exists", > + trp_name); > + return; > + } > + > + struct eth_addr lrp_ea; > + eth_addr_from_string(trp->mac, &lrp_ea); > + if (!eth_addr_equals(ea, lrp_ea)) { > + ctl_error(ctx, "%s: port already exists with mac %s", > trp_name, > + trp->mac); > + return; > + } > + > + struct sset *new_networks = lrp_network_sset(networks, > n_networks); > + if (!new_networks) { > + ctl_error(ctx, "%s: Invalid networks configured", trp_name); > + return; > + } > + struct sset *orig_networks = lrp_network_sset( > + (const char **) trp->networks, trp->n_networks); > + if (!orig_networks) { > + ctl_error(ctx, "%s: Existing port has invalid networks > configured", > + trp_name); > + sset_destroy(new_networks); > + free(new_networks); > + return; > + } > + > + bool same_networks = sset_equals(orig_networks, new_networks); > + sset_destroy(orig_networks); > + free(orig_networks); > + sset_destroy(new_networks); > + free(new_networks); > + if (!same_networks) { > + ctl_error(ctx, "%s: port already exists with different > network", > + trp_name); > + return; > + } > + > + return; > + } > + > + for (int i = 0; i < n_networks; i++) { > + ovs_be32 ipv4; > + unsigned int plen; > + error = ip_parse_cidr(networks[i], &ipv4, &plen); > + if (error) { > + free(error); > + struct in6_addr ipv6; > + error = ipv6_parse_cidr(networks[i], &ipv6, &plen); > It would be nice to add "ip46_parse_cidr" to avoid the double nesting here and in ovn-nbctl.c too. > + if (error) { > + free(error); > + ctl_error(ctx, "%s: invalid network address: %s", > trp_name, > + networks[i]); > + return; > + } > + > + } > + > + } > + > + trp = icnbrec_transit_router_port_insert(ctx->txn); > + icnbrec_transit_router_port_set_transit_router(trp, tr_name); > + icnbrec_transit_router_port_set_name(trp, trp_name); > + icnbrec_transit_router_port_set_mac(trp, mac); > + icnbrec_transit_router_port_set_nb_ic_uuid(trp, &tr->header_.uuid, 1); > + icnbrec_transit_router_port_set_networks(trp, networks, n_networks); > + > + for (int i = 0; i < n_settings; i++) { > + error = ctl_set_column("Transit_Router_Port", &trp->header_, > + settings[i], ctx->symtab); > + if (error) { > + ctx->error = error; > + return; > + } > + > + } > +} > + > static void > verify_connections(struct ctl_context *ctx) > { > @@ -1145,6 +1324,11 @@ static const struct ctl_command_syntax > ic_nbctl_commands[] = { > { "tr-del", 1, 1, "ROUTER", NULL, ic_nbctl_tr_del, NULL, > "--if-exists", > RW }, > { "tr-list", 0, 0, "", NULL, ic_nbctl_tr_list, NULL, "", RO }, > + { "trp-add", 4, INT_MAX, > + "ROUTER PORT MAC [NETWORK]...[COLUMN[:KEY]=VALUE]...", > + NULL, ic_nbctl_trp_add, NULL, "--may-exist", RW }, > + { "trp-del", 1, 1, "PORT", NULL, ic_nbctl_trp_del, NULL, > "--if-exists", > + RW }, > > /* Connection commands. */ > {"get-connection", 0, 0, "", pre_connection, cmd_get_connection, > NULL, "", > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 058ad8968..809f14518 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -4534,86 +4534,6 @@ nbctl_dhcp_options_list(struct ctl_context *ctx) > free(nodes); > } > > -static char * > -normalize_ipv4_prefix_str(const char *orig_prefix) > -{ > - unsigned int plen; > - ovs_be32 ipv4; > - char *error; > - > - error = ip_parse_cidr(orig_prefix, &ipv4, &plen); > - if (error) { > - free(error); > - return NULL; > - } > - return normalize_ipv4_prefix(ipv4, plen); > -} > - > -static char * > -normalize_ipv6_prefix_str(const char *orig_prefix) > -{ > - unsigned int plen; > - struct in6_addr ipv6; > - char *error; > - > - error = ipv6_parse_cidr(orig_prefix, &ipv6, &plen); > - if (error) { > - free(error); > - return NULL; > - } > - return normalize_ipv6_prefix(&ipv6, plen); > -} > - > -/* The caller must free the returned string. */ > -static char * > -normalize_prefix_str(const char *orig_prefix) > -{ > - char *ret; > - > - ret = normalize_ipv4_prefix_str(orig_prefix); > - if (!ret) { > - ret = normalize_ipv6_prefix_str(orig_prefix); > - } > - return ret; > -} > - > -static char * > -normalize_ipv4_addr_str(const char *orig_addr) > -{ > - ovs_be32 ipv4; > - > - if (!ip_parse(orig_addr, &ipv4)) { > - return NULL; > - } > - > - return normalize_ipv4_prefix(ipv4, 32); > -} > - > -static char * > -normalize_ipv6_addr_str(const char *orig_addr) > -{ > - struct in6_addr ipv6; > - > - if (!ipv6_parse(orig_addr, &ipv6)) { > - return NULL; > - } > - > - return normalize_ipv6_prefix(&ipv6, 128); > -} > - > -/* Similar to normalize_prefix_str but must be an un-masked address. > - * The caller must free the returned string. */ > -OVS_UNUSED static char * > -normalize_addr_str(const char *orig_addr) > -{ > - char *ret; > - > - ret = normalize_ipv4_addr_str(orig_addr); > - if (!ret) { > - ret = normalize_ipv6_addr_str(orig_addr); > - } > - return ret; > -} > > static bool > ip_in_lrp_networks(const struct nbrec_logical_router_port *lrp, > @@ -6552,23 +6472,6 @@ nbctl_lrp_get_gateway_chassis(struct ctl_context > *ctx) > free(gcs); > } > > -static struct sset * > -lrp_network_sset(const char **networks, int n_networks) > -{ > - struct sset *network_set = xzalloc(sizeof *network_set); > - sset_init(network_set); > - for (int i = 0; i < n_networks; i++) { > - char *norm = normalize_prefix_str(networks[i]); > - if (!norm) { > - sset_destroy(network_set); > - free(network_set); > - return NULL; > - } > - sset_add_and_free(network_set, norm); > - } > - return network_set; > -} > - > static void > nbctl_pre_lrp_add(struct ctl_context *ctx) > { > -- > 2.51.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
