On Tue, Oct 21, 2025 at 8:03 PM Mairtin O'Loingsigh via dev < [email protected]> wrote:
> This patch adds support for managing transit routers ports. > Ports create on a transit router are remote for all availability zones > except the zone containing the chassis. > 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, I think we are getting close, just some last small comments. > NEWS | 3 + > ic/ovn-ic.c | 399 ++++++++++++++++++++++++++++++--------- > tests/ovn-ic-nbctl.at | 29 +++ > tests/ovn-ic.at | 204 ++++++++++++++++++++ > utilities/ovn-ic-nbctl.c | 194 +++++++++++++++++++ > 5 files changed, 744 insertions(+), 85 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 c331910c8..47dbe2df3 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -66,6 +66,7 @@ struct ic_context { > struct ovsdb_idl_txn *ovnisb_txn; > const struct icsbrec_availability_zone *runned_az; > struct ovsdb_idl_index *nbrec_ls_by_name; > + struct ovsdb_idl_index *nbrec_lr_by_name; > struct ovsdb_idl_index *nbrec_lrp_by_name; > struct ovsdb_idl_index *nbrec_port_by_name; > struct ovsdb_idl_index *sbrec_chassis_by_name; > @@ -75,8 +76,10 @@ struct ic_context { > struct ovsdb_idl_index > *sbrec_service_monitor_by_remote_type_logical_port; > struct ovsdb_idl_index *icnbrec_transit_switch_by_name; > 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_tr; > 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; > @@ -91,6 +94,7 @@ struct ic_state { > }; > > enum ic_datapath_type { IC_SWITCH, IC_ROUTER, IC_DATAPATH_MAX }; > +enum ic_port_binding_type { IC_SWITCH_PORT, IC_ROUTER_PORT, IC_PORT_MAX }; > > static const char *ovnnb_db; > static const char *ovnsb_db; > @@ -215,6 +219,16 @@ ic_dp_get_type(const struct icsbrec_datapath_binding > *isb_dp) > return IC_SWITCH; > } > > +static enum ic_port_binding_type > +ic_pb_get_type(const struct icsbrec_port_binding *isb_pb) > +{ > + if (isb_pb->type && !strcmp(isb_pb->type, "transit-router-port")) { > + return IC_ROUTER_PORT; > + } > + > + return IC_SWITCH_PORT; > +} > + > static void > enumerate_datapaths(struct ic_context *ctx, struct hmap *dp_tnlids, > struct shash *isb_ts_dps, struct shash *isb_tr_dps) > @@ -428,8 +442,6 @@ tr_run(struct ic_context *ctx, struct hmap *dp_tnlids, > > struct shash_node *node; > SHASH_FOR_EACH_SAFE (node, isb_tr_dps) { > - struct icsbrec_datapath_binding *isb_dp = node->data; > - ovn_free_tnlid(dp_tnlids, isb_dp->tunnel_key); > Ah I see the comment on 2/4 is addressed here, regardless let's move it to 2/4 then. > icsbrec_datapath_binding_delete(node->data); > shash_delete(isb_tr_dps, node); > } > @@ -606,6 +618,30 @@ 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) > @@ -723,6 +759,21 @@ 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, > @@ -860,6 +911,52 @@ sync_remote_port(struct ic_context *ctx, > } > } > > +/* For each remote port: > + * - Sync from ISB to NB > + */ > +static void > +sync_router_port(const struct icsbrec_port_binding *isb_pb, > + const struct icnbrec_transit_router_port *trp, > + const struct nbrec_logical_router_port *lrp) > +{ > + /* Sync from ICNB to NB */ > + if (trp->chassis[0]) { > + const char *chassis_name = > + smap_get(&lrp->options, "requested-chassis"); > nit: You could use smap_get_def with default being empty string, this way you could do direct strcmp without the NULL check. > + if (!chassis_name || strcmp(trp->chassis, chassis_name)) { > + nbrec_logical_router_port_update_options_setkey( > + lrp, "requested-chassis", trp->chassis); > + } > + } else { > + nbrec_logical_router_port_update_options_delkey( > + lrp, "requested-chassis"); > + } > + > + if (strcmp(trp->mac, lrp->mac)) { > + nbrec_logical_router_port_set_mac(lrp, trp->mac); > + } > + > + bool sync_networks = false; > + if (trp->n_networks != lrp->n_networks) { > + sync_networks = true; > + } else { > + for (size_t i = 0; i < trp->n_networks; i++) { > + if (strcmp(trp->networks[i], lrp->networks[i])) { > + sync_networks |= true; > + break; > + } > + } > + } > + > + if (sync_networks) { > + nbrec_logical_router_port_set_networks( > + lrp, (const char **) trp->networks, trp->n_networks); > + } > + > + /* Sync tunnel key from ISB to NB */ > + sync_lrp_tnl_key(lrp, isb_pb->tunnel_key); > Let's inline that, there will be only a single caller. > +} > + > static void > create_nb_lsp(struct ic_context *ctx, > const struct icsbrec_port_binding *isb_pb, > @@ -881,53 +978,88 @@ create_nb_lsp(struct ic_context *ctx, > nbrec_logical_switch_update_ports_addvalue(ls, lsp); > } > > -static void > -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) > +static const struct sbrec_port_binding * > +find_lp_in_sb(struct ic_context *ctx, const char *name) > +{ > + return find_sb_pb_by_name(ctx->sbrec_port_binding_by_name, name); > +} > + > +static uint32_t > +allocate_port_key(struct hmap *pb_tnlids) > +{ > + static uint32_t hint; > + return ovn_allocate_tnlid(pb_tnlids, "transit port", > + 1, (1u << 15) - 1, &hint); > +} > + > +static const struct icsbrec_port_binding * > +create_isb_pb(struct ic_context *ctx, const char *logical_port, > + const struct icsbrec_availability_zone *az, const char > *ts_name, > + const struct uuid *nb_ic_uuid, const char *type, > + struct hmap *pb_tnlids) > { > + uint32_t pb_tnl_key = allocate_port_key(pb_tnlids); > + if (pb_tnl_key == 0) { > + return NULL; > + } > + > const struct icsbrec_port_binding *isb_pb = > icsbrec_port_binding_insert(ctx->ovnisb_txn); > icsbrec_port_binding_set_availability_zone(isb_pb, az); > 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_logical_port(isb_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); > + return isb_pb; > +} > > - const char *address = get_lp_address_for_sb_pb(ctx, sb_pb); > - if (address) { > - icsbrec_port_binding_set_address(isb_pb, address); > - } > - > - const struct sbrec_port_binding *crp = find_crp_for_sb_pb(ctx, sb_pb); > - if (crp && crp->chassis) { > - icsbrec_port_binding_set_gateway(isb_pb, crp->chassis->name); > - } > - > - update_isb_pb_external_ids(ctx, sb_pb, isb_pb); > +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); > > - /* XXX: Sync encap so that multiple encaps can be used for the same > - * gateway. However, it is not needed for now, since we don't yet > - * support specifying encap type/ip for gateway chassis or ha-chassis > - * for logical router port in NB DB, and now encap should always be > - * empty. The sync can be added if we add such support for gateway > - * chassis/ha-chassis in NB DB. */ > + return lrp; > } > > -static const struct sbrec_port_binding * > -find_lsp_in_sb(struct ic_context *ctx, > - const struct nbrec_logical_switch_port *lsp) > +static bool > +trp_port_is_remote(struct ic_context *ctx, const char *chassis_name) > { > - return find_sb_pb_by_name(ctx->sbrec_port_binding_by_name, lsp->name); > + 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; > } > > -static uint32_t > -allocate_port_key(struct hmap *pb_tnlids) > +static struct nbrec_logical_router_port * > +trp_port_create(struct ic_context *ctx, const struct nbrec_logical_router > *lr, > + const struct icsbrec_port_binding *isb_pb, > + const struct icnbrec_transit_router_port *trp) > { > - static uint32_t hint; > - return ovn_allocate_tnlid(pb_tnlids, "transit port", > - 1, (1u << 15) - 1, &hint); > + struct nbrec_logical_router_port *lrp = > + nbrec_logical_router_port_insert(ctx->ovnnb_txn); > + nbrec_logical_router_port_set_name(lrp, trp->name); > + if (strcmp("", trp->chassis)) { > + nbrec_logical_router_port_update_options_setkey( > + lrp, "requested-chassis", trp->chassis); > + } > This is not needed here, it will be taken care of by the sync_router_port. > + > + nbrec_logical_router_port_update_options_setkey(lrp, "interconn-tr", > + trp->name); > + nbrec_logical_router_update_ports_addvalue(lr, lrp); > + sync_lrp_tnl_key(lrp, isb_pb->tunnel_key); > This is also not needed here. > + return lrp; > } > > static void > @@ -937,18 +1069,24 @@ port_binding_run(struct ic_context *ctx) > return; > } > > - struct shash isb_all_local_pbs = > SHASH_INITIALIZER(&isb_all_local_pbs); > - struct shash_node *node; > + struct shash switch_all_local_pbs = > + SHASH_INITIALIZER(&switch_all_local_pbs); > + struct shash router_all_local_pbs = > + SHASH_INITIALIZER(&router_all_local_pbs); > + struct hmap pb_tnlids = HMAP_INITIALIZER(&pb_tnlids); > > 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); > - > 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); > + ic_pb_get_type(isb_pb) != IC_ROUTER_PORT > + ? shash_add(&switch_all_local_pbs, isb_pb->logical_port, > isb_pb) > + : shash_add(&router_all_local_pbs, isb_pb->logical_port, > isb_pb); > + > + ovn_add_tnlid(&pb_tnlids, isb_pb->tunnel_key); > } > icsbrec_port_binding_index_destroy_row(isb_pb_key); > > @@ -962,21 +1100,23 @@ port_binding_run(struct ic_context *ctx) > } > 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); > > - ICSBREC_PORT_BINDING_FOR_EACH_EQUAL (isb_pb, isb_pb_key, > - > ctx->icsbrec_port_binding_by_ts) { > + 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"); > This has an issue on upgrade, we should search by the transit_switch column here. > + 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) { > - shash_add(&local_pbs, isb_pb->logical_port, isb_pb); > - shash_find_and_delete(&isb_all_local_pbs, > + shash_find_and_delete(&switch_all_local_pbs, > isb_pb->logical_port); > + shash_add(&local_pbs, isb_pb->logical_port, isb_pb); > } else { > - shash_add(&remote_pbs, isb_pb->logical_port, isb_pb); > + if (!shash_find_data(&remote_pbs, isb_pb->logical_port)) { > + shash_add(&remote_pbs, isb_pb->logical_port, isb_pb); > + } > } > - ovn_add_tnlid(&pb_tnlids, isb_pb->tunnel_key); > } > icsbrec_port_binding_index_destroy_row(isb_pb_key); > > @@ -987,25 +1127,40 @@ 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); > 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); > + isb_pb = create_isb_pb( > + ctx, sb_pb->logical_port, ctx->runned_az, > ts->name, > + &ts->header_.uuid, "transit-switch-port", > &pb_tnlids); > + const char *address = get_lp_address_for_sb_pb(ctx, > sb_pb); > + if (address) { > + icsbrec_port_binding_set_address(isb_pb, address); > + } > + > + const struct sbrec_port_binding *crp = > + find_crp_for_sb_pb(ctx, sb_pb); > + if (crp && crp->chassis) { > + icsbrec_port_binding_set_gateway(isb_pb, > + > crp->chassis->name); > + } > + > + update_isb_pb_external_ids(ctx, sb_pb, isb_pb); > The whole block after create_isb_pb should be probably in a separate function e.g. sync_ts_isb_pb(). > } else { > sync_local_port(ctx, isb_pb, sb_pb, lsp); > } > + > nit: Extra line. > } else if (!strcmp(lsp->type, "remote")) { > /* The port is remote. */ > isb_pb = shash_find_and_delete(&remote_pbs, lsp->name); > 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; > } > @@ -1017,26 +1172,101 @@ 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 shash_node *node; > + SHASH_FOR_EACH_SAFE (node, &local_pbs) { > + isb_pb = node->data; > + shash_delete(&local_pbs, node); > + 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); > + SHASH_FOR_EACH_SAFE (node, &remote_pbs) { > + isb_pb = node->data; > + shash_delete(&remote_pbs, node); > + create_nb_lsp(ctx, isb_pb, ls); > } > No need for shash_delete in both of those loops. > > shash_destroy(&local_pbs); > shash_destroy(&remote_pbs); > - ovn_destroy_tnlids(&pb_tnlids); > } > > - SHASH_FOR_EACH (node, &isb_all_local_pbs) { > - icsbrec_port_binding_delete(node->data); > + struct shash_node *node; > + SHASH_FOR_EACH_SAFE (node, &switch_all_local_pbs) { > + isb_pb = node->data; > + icsbrec_port_binding_delete(isb_pb); > + shash_delete(&switch_all_local_pbs, node); > Same here, no need for shash_delete. > } > + shash_destroy(&switch_all_local_pbs); > > - shash_destroy(&isb_all_local_pbs); > + const struct icnbrec_transit_router *tr; > + ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->ovninb_idl) { > + struct shash nb_ports = SHASH_INITIALIZER(&nb_ports); > + 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; > + } > + > + for (size_t i = 0; i < tr->n_ports; i++) { > + const struct icnbrec_transit_router_port *trp = tr->ports[i]; > + isb_pb = shash_find_and_delete(&router_all_local_pbs, > trp->name); > + if (!isb_pb) { > + if (!trp_port_is_remote(ctx, trp->chassis)) { > + isb_pb = create_isb_pb(ctx, trp->name, ctx->runned_az, > + tr->name, &tr->header_.uuid, > + "transit-router-port", > &pb_tnlids); > + icsbrec_port_binding_set_address(isb_pb, trp->mac); > We should continue here and synchronize the NB entries only after IC PB was commited to avoid synchronizing keys that might fail the transaction. + } else { > The else here should be just continue, we don't want to sync from ovn-ic that is not "owner" of the TRP. Only the ovn-ic where the TRP chassis resides should take care of that. > + isb_pb_key = icsbrec_port_binding_index_init_row( > + ctx->icsbrec_port_binding_by_tr); > + > icsbrec_port_binding_index_set_transit_switch(isb_pb_key, > + > tr->name); > + ICSBREC_PORT_BINDING_FOR_EACH_EQUAL ( > + isb_pb, isb_pb_key, > ctx->icsbrec_port_binding_by_tr) { > + if (!strcmp(trp->name, isb_pb->logical_port)) { > + break; > + } + } > + icsbrec_port_binding_index_destroy_row(isb_pb_key); > + } > + } > + /* Don't allow remote ports to create NB LRP until ICSB entry > is > + * created in the appropriate AZ. */ > + if (isb_pb) { > + const struct nbrec_logical_router_port *lrp = > + get_lrp_by_lrp_name(ctx, trp->name); > + > + if (!lrp) { > + lrp = trp_port_create(ctx, lr, isb_pb, trp); > + } else { > No need for else, it should always be synchronized. > + sync_router_port(isb_pb, trp, lrp); > + } > + > + shash_add(&nb_ports, trp->name, lrp); > + } > + } > + > + for (size_t i = 0; i < lr->n_ports; i++) { > + const struct nbrec_logical_router_port *lrp = lr->ports[i]; > + if (!shash_find_and_delete(&nb_ports, lrp->name)) { > + if (smap_get(&lrp->options, "interconn-tr")) { > nit: This can be a single if no need to nest. > + nbrec_logical_router_port_delete(lrp); > + nbrec_logical_router_update_ports_delvalue(lr, lrp); > + } > + } > + } > + > + shash_destroy(&nb_ports); > + } > + ovn_destroy_tnlids(&pb_tnlids); > + > + SHASH_FOR_EACH_SAFE (node, &router_all_local_pbs) { > + isb_pb = node->data; > + icsbrec_port_binding_delete(isb_pb); > + shash_delete(&router_all_local_pbs, node); > + } > + > + shash_destroy(&router_all_local_pbs); > } > > struct ic_router_info { > @@ -1717,20 +1947,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) > @@ -2267,6 +2483,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 (ic_pb_get_type(isb_pb) == IC_ROUTER_PORT) { > + continue; > + } > const struct nbrec_logical_switch_port *nb_lsp; > > nb_lsp = get_lsp_by_ts_port_name(ctx, isb_pb->logical_port); > @@ -3245,6 +3464,8 @@ main(int argc, char *argv[]) > struct ovsdb_idl_index *nbrec_ls_by_name > = ovsdb_idl_index_create1(ovnnb_idl_loop.idl, > &nbrec_logical_switch_col_name); > + struct ovsdb_idl_index *nbrec_lr_by_name = ovsdb_idl_index_create1( > + ovnnb_idl_loop.idl, &nbrec_logical_router_col_name); > struct ovsdb_idl_index *nbrec_port_by_name > = ovsdb_idl_index_create1(ovnnb_idl_loop.idl, > &nbrec_logical_switch_port_col_name); > @@ -3279,14 +3500,18 @@ 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_tr > = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, > > &icsbrec_port_binding_col_transit_switch); > > - struct ovsdb_idl_index *icsbrec_port_binding_by_ts_az > - = ovsdb_idl_index_create2(ovnisb_idl_loop.idl, > - > &icsbrec_port_binding_col_transit_switch, > - > &icsbrec_port_binding_col_availability_zone); > + struct ovsdb_idl_index *icsbrec_port_binding_by_nb_ic_uuid = > + ovsdb_idl_index_create1(ovnisb_idl_loop.idl, > + &icsbrec_port_binding_col_nb_ic_uuid); > + > + struct ovsdb_idl_index *icsbrec_port_binding_by_nb_ic_uuid_type = > + ovsdb_idl_index_create2(ovnisb_idl_loop.idl, > + &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, > @@ -3361,6 +3586,7 @@ main(int argc, char *argv[]) > .ovnisb_idl = ovnisb_idl_loop.idl, > .ovnisb_txn = ovsdb_idl_loop_run(&ovnisb_idl_loop), > .nbrec_ls_by_name = nbrec_ls_by_name, > + .nbrec_lr_by_name = nbrec_lr_by_name, > .nbrec_lrp_by_name = nbrec_lrp_by_name, > .nbrec_port_by_name = nbrec_port_by_name, > .sbrec_port_binding_by_name = sbrec_port_binding_by_name, > @@ -3374,8 +3600,11 @@ 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_tr = icsbrec_port_binding_by_tr, > + .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/tests/ovn-ic-nbctl.at b/tests/ovn-ic-nbctl.at > index 7aa3a46d8..4c5269784 100644 > --- a/tests/ovn-ic-nbctl.at > +++ b/tests/ovn-ic-nbctl.at > @@ -97,5 +97,34 @@ 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 chassis=chassis], [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 5 arguments > +]) > + > +AT_CHECK([ovn-ic-nbctl trp-add tr0 tr0-p0 00:11:22:11:22:33 > 192.168.10.10/24 chassis=chassis]) > +AT_CHECK([ovn-ic-nbctl trp-add tr0 tr0-p0 00:11:22:11:22:33 > 192.168.10.10/24 chassis=chassis], [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 chassis=chassis]) > + > +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]) > + > +AT_CHECK([ovn-ic-nbctl --if-exists tr-del tr0]) > + > OVN_IC_NBCTL_TEST_STOP > AT_CLEANUP > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > index 3d36226f6..083973f74 100644 > --- a/tests/ovn-ic.at > +++ b/tests/ovn-ic.at > @@ -4336,3 +4336,207 @@ wait_row_count Datapath_Binding 0 > 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 > +wait_row_count Port_Binding 1 > + > +# Check that port type is correctly set to remote > +check_row_count sb:Port_Binding 1 logical_port=tr0-p0 > +check_row_count sb:Port_Binding 1 type=remote > +az1_tunnel_key=$(ovn-sbctl --bare --columns=tunnel_key find Port_Binding) > +az1_mac=$(ovn-sbctl --bare --columns=mac find Port_Binding) > +port_name=$(ovn-nbctl --bare --columns=name find Logical_Router_Port > options:requested-tnl-key="${az1_tunnel_key}") > nit: This can changed to check_row_count nb:Logical_Router_Port 1 options:requested-tnl-key="${az1_tunnel_key}" +check test "${port_name}"==tr0-p0 > + > +ovn_as az2 > +wait_row_count Port_Binding 1 > +# Check that port type is correctly set to local > +check_row_count sb:Port_Binding 1 logical_port=tr0-p0 > +check_row_count sb:Port_Binding 1 type=patch > +az2_tunnel_key=$(ovn-sbctl --bare --columns=tunnel_key find Port_Binding) > +az2_mac=$(ovn-sbctl --bare --columns=mac find Port_Binding) > +# Check that nbctl port tunnel_key matches sbctl > +port_name=$(ovn-nbctl --bare --columns=name find Logical_Router_Port > options:requested-tnl-key="${az1_tunnel_key}") > nit: Same here. > +check test "${port_name}"==tr0-p0 > +check test "${az1_tunnel_key}"=="${az2_tunnel_key}" > +check test "${az1_mac}"=="${az2_mac}" > +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 > + > +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 --wait=sb tr-add tr0 > +check ovn-ic-nbctl --wait=sb trp-add tr0 tr0-p0 00:00:00:11:22:00 > 192.168.10.10/24 192.168.10.20/24 chassis=chassis-az2 > +wait_row_count Port_Binding 1 > +check_row_count sb:Port_Binding 1 logical_port=tr0-p0 > + > +ovn_as az2 > +wait_row_count Port_Binding 1 > +check_row_count sb:Port_Binding 1 logical_port=tr0-p0 > + > +ovn_as az1 > +check ovn-ic-nbctl --wait=sb trp-del tr0-p0 > +wait_row_count Port_Binding 0 > + > +ovn_as az2 > +wait_row_count Port_Binding 0 > + > +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 > + > +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 --wait=sb tr-add tr0 > +check ovn-ic-nbctl --wait=sb trp-add tr0 tr0-p0 00:00:00:11:22:00 > 192.168.10.10/24 192.168.10.20/24 chassis=chassis-az2 > +wait_row_count Port_Binding 1 > +check_row_count sb:Port_Binding 1 logical_port=tr0-p0 > + > +ovn_as az2 > +wait_row_count Port_Binding 1 > +check_row_count sb:Port_Binding 1 logical_port=tr0-p0 > + > +ovn_as az1 > +check ovn-ic-nbctl tr-del tr0 > +wait_row_count Port_Binding 0 > + > +ovn_as az2 > +wait_row_count Port_Binding 0 > + > +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 > +]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-ic -- Duplicate port addresses]) > + > +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 --wait=sb tr-add tr0 > +check ovn-ic-nbctl --wait=sb trp-add tr0 tr0-p0 00:00:00:11:22:00 \ > + 192.168.10.10/24 192.168.10.20/24 192.168.10.10/24 192.168.10.20/24 \ > + chassis=chassis-az2 > +wait_row_count Port_Binding 1 logical_port=tr0-p0 > +check_row_count nb:Logical_Router_Port 1 networks="192.168.10.10/24 > 192.168.10.20/24" > + > +ovn_as az2 > +wait_row_count Port_Binding 1 logical_port=tr0-p0 > +check_row_count nb:Logical_Router_Port 1 networks="192.168.10.10/24 > 192.168.10.20/24" > + > +ovn_as az1 > +check ovn-ic-nbctl --wait=sb tr-del tr0 > +check_row_count sb:Datapath_Binding 0 > + > +ovn_as az2 > +check_row_count sb:Datapath_Binding 0 > + > +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 > + > +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 > +wait_row_count Datapath_Binding 1 > +wait_row_count Port_Binding 1 logical_port=tr0-p0 > +check_row_count sb:Port_Binding 1 logical_port=tr0-p0 > + > +ovn_as az2 > +wait_row_count Port_Binding 1 logical_port=tr0-p0 > +check_row_count sb:Port_Binding 1 logical_port=tr0-p0 > + > +ovn_as az1 > +check ovn-ic-nbctl tr-del tr0 > +wait_row_count ic-nb:Transit_Router_Port 0 > + > +AT_CHECK([ovn-ic-nbctl list Transit_Router], [0], [dnl > +]) > + > +OVN_CLEANUP_IC([az1], [az2]) > +AT_CLEANUP > +]) > diff --git a/utilities/ovn-ic-nbctl.c b/utilities/ovn-ic-nbctl.c > index 6a94f863e..24d376559 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\ > @@ -581,6 +584,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) > { > @@ -600,6 +635,38 @@ 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; > + > + ctx->error = trp_by_name_or_uuid(ctx, trp_name, must_exist, &trp); > + if (ctx->error) { > + return; > + } > + > + if (!trp) { > + return; > + } > + > + const struct icnbrec_transit_router *tr = NULL; > + char *tr_uuid = uuid_to_string(&trp->tr_uuid); > + ctx->error = tr_by_name_or_uuid(ctx, tr_uuid, true, &tr); > + free(tr_uuid); > + if (ctx->error) { > + return; > + } > + > + if (!trp) { > + return; > + } > + > + icnbrec_transit_router_update_ports_delvalue(tr, trp); > + icnbrec_transit_router_port_delete(trp); > +} > + > static void > ic_nbctl_tr_list(struct ctl_context *ctx) > { > @@ -621,6 +688,123 @@ 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; > + > + ctx->error = tr_by_name_or_uuid(ctx, tr_name, true, &tr); > + if (ctx->error) { > + return; > + } > + > + const struct icnbrec_transit_router_port *trp; > + ctx->error = trp_by_name_or_uuid(ctx, trp_name, false, &trp); > + if (ctx->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; > + char *error = ip_parse_cidr(networks[i], &ipv4, &plen); > + if (error) { > + free(error); > + struct in6_addr ipv6; > + error = ipv6_parse_cidr(networks[i], &ipv6, &plen); > + 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_name(trp, trp_name); > + icnbrec_transit_router_port_set_mac(trp, mac); > + icnbrec_transit_router_port_set_tr_uuid(trp, tr->header_.uuid); > + icnbrec_transit_router_port_set_networks(trp, networks, n_networks); > + for (int i = 0; i < n_settings; i++) { > + ctx->error = ctl_set_column("Transit_Router_Port", &trp->header_, > + settings[i], ctx->symtab); > + if (ctx->error) { > + return; > + } > + } > + > + icnbrec_transit_router_update_ports_addvalue(tr, trp); > +} > + > static void > verify_connections(struct ctl_context *ctx) > { > @@ -831,6 +1015,11 @@ static const struct ctl_table_class > tables[ICNBREC_N_TABLES] = { > > [ICNBREC_TABLE_TRANSIT_ROUTER].row_ids[0] = > {&icnbrec_transit_router_col_name, NULL, NULL}, > + > + [ICNBREC_TABLE_TRANSIT_ROUTER_PORT].row_ids = > + {{&icnbrec_transit_router_port_col_name, NULL, NULL}, > + {&icnbrec_transit_router_port_col_tr_uuid, NULL, NULL}, > + {&icnbrec_transit_router_port_col_mac, NULL, NULL}}, > }; > > > @@ -1138,6 +1327,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", 5, 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, "", > -- > 2.51.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
