On Fri, Oct 17, 2025 at 1:59 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, thank you for the v5. I have a few comments down below. > NEWS | 3 + > ic/ovn-ic.c | 354 ++++++++++++++++++++++++++++++++------- > tests/ovn-ic-nbctl.at | 29 ++++ > tests/ovn-ic.at | 219 ++++++++++++++++++++++++ > utilities/ovn-ic-nbctl.c | 194 +++++++++++++++++++++ > 5 files changed, 736 insertions(+), 63 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 28b0b81f3..0e5f09d0e 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; > @@ -77,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; > @@ -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) > @@ -356,8 +370,8 @@ ts_run(struct ic_context *ctx, struct hmap *dp_tnlids, > struct shash_node *node; > SHASH_FOR_EACH_SAFE (node, isb_ts_dps) { > const struct icsbrec_datapath_binding *isb_dp = node->data; > - shash_delete(isb_ts_dps, node); > icsbrec_datapath_binding_delete(isb_dp); > + shash_delete(isb_ts_dps, node); > nit: Unrelated change. > } > } > } > @@ -397,7 +411,7 @@ tr_run(struct ic_context *ctx, struct hmap *dp_tnlids, > /* Create ISB Datapath_Binding */ > const struct icnbrec_transit_router *tr; > ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->ovninb_idl) { > - shash_find_and_delete(&nb_tres, tr->name); > + lr = shash_find_and_delete(&nb_tres, tr->name); > char *uuid_str = uuid_to_string(&tr->header_.uuid); > struct icsbrec_datapath_binding *isb_dp = > shash_find_and_delete(isb_tr_dps, uuid_str); > @@ -416,11 +430,17 @@ tr_run(struct ic_context *ctx, struct hmap > *dp_tnlids, > icsbrec_datapath_binding_set_nb_ic_uuid(isb_dp, > > &tr->header_.uuid, 1); > icsbrec_datapath_binding_set_type(isb_dp, > "transit-router"); > + char *tnl_key_str = xasprintf("%"PRId64, > isb_dp->tunnel_key); > + nbrec_logical_router_update_options_setkey( > + lr, "requested-tnl-key", tnl_key_str); > + free(tnl_key_str); > This should be part of 2/4, also this isn't completely right, see feedback on 2/4. > } > } > > 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); > No need to free the tnlid here. We are at the end of assigning anyway. > icsbrec_datapath_binding_delete(node->data); > shash_delete(isb_tr_dps, node); > } > @@ -602,6 +622,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) > @@ -719,6 +763,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, > @@ -856,6 +915,44 @@ 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 icnbrec_transit_router_port *trp, > + const struct nbrec_logical_router_port *lrp) > +{ > nit: I would call it just sync_router_port. > + /* Sync from ICNB to NB */ > + if (trp->chassis[0]) { > + const char *chassis_name = > + smap_get(&lrp->options, "requested-chassis"); > + if (!chassis_name || strcmp(trp->chassis, chassis_name)) { > + nbrec_logical_router_port_update_options_setkey( > + lrp, "requested-chassis", trp->chassis); > + } > + } > There should be an else to delete the option if it's not set. > + > + 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; > nit: You can break here. > + } > + } > + } > + > + 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); > +} > Isn't this function also missing synchronization of mac? > + > static void > create_nb_lsp(struct ic_context *ctx, > const struct icsbrec_port_binding *isb_pb, > @@ -878,11 +975,10 @@ create_nb_lsp(struct ic_context *ctx, > } > > 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) > +create_isb_pb(struct ic_context *ctx, const struct sbrec_port_binding > *sb_pb, > + const struct icsbrec_availability_zone *az, const char > *ts_name, > + 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); > @@ -890,10 +986,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")) { > nit: Use the new helper here. > + 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); > @@ -912,10 +1015,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 > @@ -926,6 +1028,55 @@ allocate_port_key(struct hmap *pb_tnlids) > 1, (1u << 15) - 1, &hint); > } > > +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 bool > +trp_port_is_remote(struct ic_context *ctx, const char *chassis_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 struct nbrec_logical_router_port * > +trp_port_create(struct ic_context *ctx, const struct nbrec_logical_router > *lr, > + const struct icnbrec_transit_router_port *trp) > +{ > This function should add only things that are not synchronized with "sync_remote_router_port". + struct nbrec_logical_router_port *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", > + trp->name); > + nbrec_logical_router_update_ports_addvalue(lr, lrp); > + return lrp; > +} > + > static void > port_binding_run(struct ic_context *ctx) > { > @@ -933,23 +1084,30 @@ 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); > > const struct sbrec_port_binding *sb_pb; > const struct icnbrec_transit_switch *ts; > + > nit: Unrelated change. > ICNBREC_TRANSIT_SWITCH_FOR_EACH (ts, ctx->ovninb_idl) { > const struct nbrec_logical_switch *ls = find_ts_in_nb(ctx, > ts->name); > if (!ls) { > @@ -958,21 +1116,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"); > + ICSBREC_PORT_BINDING_FOR_EACH_EQUAL ( > + isb_pb, isb_pb_key, > ctx->icsbrec_port_binding_by_nb_ic_uuid_type) { > This suffers the same issue on upgrade as the TS in 2/4. We need to keep using the transit_switch column here. 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); > Not sure why the "ovn_add_tnlid()" was deleted here. > } > icsbrec_port_binding_index_destroy_row(isb_pb_key); > > @@ -983,25 +1143,28 @@ 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); > + 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); > 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; > } > @@ -1013,26 +1176,97 @@ 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); > } > > 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); > + } > + shash_destroy(&switch_all_local_pbs); > + > + /* Walk list of transit routers*/ > + 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]; > + > + const struct nbrec_logical_router_port *lrp = > + get_lrp_by_lrp_name(ctx, trp->name); > + if (!lrp) { > + lrp = trp_port_create(ctx, lr, trp); > + } > We need to sync the lrp here too. > + > + sb_pb = find_lp_in_sb(ctx, lrp->name); + if (!sb_pb) { > + continue; > + } > This is not right, we won't have SB port binding for that LRP. Same as for the TR, if the IC SB PB is not created yet we shouldn't sync the tunnel key, but we should still sync the rest. > + > + shash_add(&nb_ports, trp->name, lrp); > + isb_pb = shash_find_and_delete(&router_all_local_pbs, > trp->name); > + if (!trp_port_is_remote(ctx, trp->chassis)) { > + > + if (!isb_pb) { > + uint32_t pb_tnl_key = allocate_port_key(&pb_tnlids); > + create_isb_pb(ctx, sb_pb, ctx->runned_az, tr->name, > + &tr->header_.uuid, > "transit-router-port", > + trp->mac, pb_tnl_key); > I think we need to split the create_isb_pb a bit, we don't have the SB PB for LRP yet. > + continue; > + } > + } > + > + if (isb_pb) { > + sync_remote_router_port(isb_pb, trp, 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")) { > + nbrec_logical_router_port_delete(lrp); > + nbrec_logical_router_update_ports_delvalue(lr, lrp); > + } > + } > + } > + > + SHASH_FOR_EACH_SAFE (node, &nb_ports) { > + shash_delete(&nb_ports, node); > + } > Should be shash_destroy instead of the loop. We should also assert that the shash is empty here. > + } > + 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(&isb_all_local_pbs); > + shash_destroy(&router_all_local_pbs); > } > > struct ic_router_info { > @@ -1713,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) > @@ -2263,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 (isb_pb->type && !strcmp(isb_pb->type, "transit-router-port")) > { > + continue; > + } > nit: You have the helper `ic_pb_get_type`. > const struct nbrec_logical_switch_port *nb_lsp; > > nb_lsp = get_lsp_by_ts_port_name(ctx, isb_pb->logical_port); > @@ -3241,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); > @@ -3275,14 +3500,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 > - = ovsdb_idl_index_create1(ovnisb_idl_loop.idl, > - > &icsbrec_port_binding_col_transit_switch); > + 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_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_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, > @@ -3357,6 +3582,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, > @@ -3370,8 +3596,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/tests/ovn-ic-nbctl.at b/tests/ovn-ic-nbctl.at > index 058d66cb1..0f6290b95 100644 > --- a/tests/ovn-ic-nbctl.at > +++ b/tests/ovn-ic-nbctl.at > @@ -102,5 +102,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 dafad4fd0..a6173fa8e 100644 > --- a/tests/ovn-ic.at > +++ b/tests/ovn-ic.at > @@ -4335,3 +4335,222 @@ 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 > +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 > +]) > Please use check_row_count. > +az1_tunnel_key = $(ovn-sbctl --bare --columns=tunnel_key find > Port_Binding) > +az1_mac = $(ovn-sbctl --bare --columns=mac find Port_Binding) > + > +ovn_as az2 > +wait_row_count Port_Binding 1 > +# Check that port type is correctly set to local > +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 > +]) > Same here, please use check_row_count. > +az2_tunnel_key = $(ovn-sbctl --bare --columns=tunnel_key find > Port_Binding) > +az2_mac = $(ovn-sbctl --bare --columns=mac find Port_Binding) + > +AT_CHECK([test "$az1_tunnel_key" == "$az2_tunnel_key"]) > +AT_CHECK([test "$az1_mac" == "$az2_mac"]) > In addition we should also check that the requested-tnl-key is set for both. > + > +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 > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0-p0], [0], [dnl > +logical_port : tr0-p0 > +]) > I think the wait_row_count above is enough, you can filter by name there also. This applies to the rest of the tests. > + > +ovn_as az2 > +wait_row_count Port_Binding 1 > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0-p0], [0], [dnl > +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 > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0 | sort], [0], [dnl > +logical_port : tr0-p0 > +]) > + > +ovn_as az2 > +wait_row_count Port_Binding 1 > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0 | sort], [0], [dnl > +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 > +]) > I would replace those with check_row_count. > + > +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 > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0 | sort], [0], [dnl > +logical_port : tr0-p0 > +]) + > +ovn_as az2 > +wait_row_count Port_Binding 1 logical_port=tr0-p0 > +AT_CHECK([ovn-sbctl list Port_Binding | grep tr0 | sort], [0], [dnl > +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 bc480017c..20e86e28c 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; > + > nit: I would remove the empty space. > + 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) > { > @@ -602,6 +637,40 @@ 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); > nit: You can set the ctx->error directly. That applies to every change in ovn-ic-nbctl.c. > + if (error) { > + ctx->error = error; + return; > + } > + > + if (!trp) { > + return; > + } > + > + const struct icnbrec_transit_router *tr = NULL; > + char *tr_uuid = uuid_to_string(&trp->tr_uuid); > + error = tr_by_name_or_uuid(ctx, tr_uuid, true, &tr); > + free(tr_uuid); > + if (error) { > + ctx->error = 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) > { > @@ -623,6 +692,126 @@ 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); > + 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++) { > + error = ctl_set_column("Transit_Router_Port", &trp->header_, > + settings[i], ctx->symtab); > + if (error) { > + ctx->error = error; > + return; > + } > + } > + > + icnbrec_transit_router_update_ports_addvalue(tr, trp); > +} > + > static void > verify_connections(struct ctl_context *ctx) > { > @@ -1145,6 +1334,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
