On Wed, Oct 01, 2025 at 12:17:30PM +0200, Ales Musil wrote:
> On Wed, Oct 1, 2025 at 12:56 AM Mairtin O'Loingsigh via dev <
> [email protected]> wrote:
>
> > This patch adds support for adding transit routers and transit router
> > ports. When created, a transit router creates logical routers in each AZ
> > with the same global datapath binding.
> >
> > 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 routers.
> >
> > Commands to add, delete and list transit routers.
> > $ ovn-ic-nbctl tr-add tr0
> > $ ovn-ic-nbctl tr-del tr0
> > $ ovn-ic-nbctl tr-list
> > 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-1476
> > 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 patch, the fact that it had to be squashed into single
> commit
> tells that there is something wrong with the schema. Also this
> change is pretty big on it's own, I believe that this could be split even
> further
> into:
> 1) Schema change
> 2) tr changes (nbctl + ovn-ic)
> 3) trp changes (nbctl + ovn-ic)
>
> I left some comments down below.
Ack, Ill split the patch
>
>
> > NEWS | 6 +
> > ic/ovn-ic.c | 1012 +++++++++++++++++++++++++++++++-------
> > ovn-architecture.7.xml | 11 +-
> > ovn-ic-nb.ovsschema | 32 +-
> > ovn-ic-nb.xml | 70 +++
> > ovn-ic-sb.ovsschema | 31 +-
> > ovn-ic-sb.xml | 28 ++
> > tests/ovn-ic-nbctl.at | 41 ++
> > tests/ovn-ic-sbctl.at | 2 +-
> > tests/ovn-ic.at | 201 +++++++-
> > utilities/ovn-ic-nbctl.c | 266 +++++++++-
> > 11 files changed, 1519 insertions(+), 181 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index f9ad8ae75..e97817e05 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -15,6 +15,12 @@ Post v25.09.0
> > (Static_)MAC_Bindings of adjacent logical routers over ARPs learned
> > through EVPN on the switch.
> >
> > + - Added Transit Router support:
> > + * Support the creation of Transit Routers.
> > + * Support the creation of Transit Router Ports with replication
> > + across availability zones.
> > + * Added new ovn-ic-nbctl
> > 'tr-add','tr-del','tr-list''trp-add','trp-del'
> > + to manage Transit Router.
> > 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 cea64588b..28ac43600 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,14 +76,14 @@ 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_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;
> > struct ovsdb_idl_index *icsbrec_service_monitor_by_source_az;
> > struct ovsdb_idl_index *icsbrec_service_monitor_by_target_az;
> > struct ovsdb_idl_index
> > *icsbrec_service_monitor_by_target_az_logical_port;
> > + struct ovsdb_idl_index *icsbrec_route_by_nb_ic_uuid_type;
> > + struct ovsdb_idl_index *icsbrec_route_by_nb_ic_az;
> > };
> >
> > struct ic_state {
> > @@ -190,31 +191,78 @@ az_run(struct ic_context *ctx)
> > ctx->runned_az = az;
> > return az;
> > }
> > +
> >
>
> nit: Unrelated change.
Ack, fine
>
>
> > return NULL;
> > }
> >
> > static uint32_t
> > -allocate_ts_dp_key(struct hmap *dp_tnlids, bool vxlan_mode)
> > +allocate_dp_key(struct hmap *dp_tnlids, bool vxlan_mode, const char *name)
> > {
> > uint32_t hint = vxlan_mode ? OVN_MIN_DP_VXLAN_KEY_GLOBAL
> > : OVN_MIN_DP_KEY_GLOBAL;
> > - return ovn_allocate_tnlid(dp_tnlids, "transit switch datapath", hint,
> > + return ovn_allocate_tnlid(dp_tnlids, name, hint,
> > vxlan_mode ? OVN_MAX_DP_VXLAN_KEY_GLOBAL :
> > OVN_MAX_DP_KEY_GLOBAL,
> > &hint);
> > }
> >
> > +
> > +struct ic_dp_info {
> > + struct hmap_node node;
> > + struct uuid uuid;
> > + const struct icsbrec_datapath_binding *isb_dp;
> > +};
> > +
> > +static struct ic_dp_info *
> > +ic_dp_find(struct hmap *dp_info, const struct uuid *uuid)
> > +{
> > + struct ic_dp_info *info;
> > + HMAP_FOR_EACH_WITH_HASH (info, node, uuid_hash(uuid),
> > + dp_info) {
> > + if (uuid_equals(&info->uuid, uuid)) {
> > + return info;
> > + }
> > +
> > + }
> > + return NULL;
> > +}
> > +
> > +static const struct icsbrec_datapath_binding *
> > +ic_dp_find_and_remove(struct hmap *dp_info, const struct uuid *uuid)
> > +{
> > + struct ic_dp_info *info = ic_dp_find(dp_info, uuid);
> > + if (info) {
> > + hmap_remove(dp_info, &info->node);
> > + const struct icsbrec_datapath_binding *isb_dp = info->isb_dp;
> > + free(info);
> > + return isb_dp;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > static void
> > -ts_run(struct ic_context *ctx)
> > +ts_run(struct ic_context *ctx, struct hmap *dp_tnlids,
> > + struct hmap *isb_ts_dps, struct hmap *isb_tr_dps)
> >
>
> nit: Unaligned arguments. Please check the other
> function definitions and calls.
>
Ack, ill fix
>
> > {
> > const struct icnbrec_transit_switch *ts;
> > bool dp_key_refresh = false;
> >
> > - struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids);
> > - struct shash isb_dps = SHASH_INITIALIZER(&isb_dps);
> > const struct icsbrec_datapath_binding *isb_dp;
> > ICSBREC_DATAPATH_BINDING_FOR_EACH (isb_dp, ctx->ovnisb_idl) {
> > - shash_add(&isb_dps, isb_dp->transit_switch, isb_dp);
> > - ovn_add_tnlid(&dp_tnlids, isb_dp->tunnel_key);
> > + ovn_add_tnlid(dp_tnlids, isb_dp->tunnel_key);
> > +
> > + struct ic_dp_info *dp_info;
> > + dp_info = xzalloc(sizeof *dp_info);
> >
>
> There doesn't seem to be a reason to do zero-alloc.
> Also it would be nice to use a designated initializer.
Ack, Ill change to designate initializer.
>
>
> > + dp_info->isb_dp = isb_dp;
> > + dp_info->uuid = isb_dp->nb_ic_uuid;
> > + if (isb_dp->type && !strcmp(isb_dp->type, "transit-router")) {
> >
>
> It actually might be beneficial to have an enum that and
> helper function that will return the enum type.
Ack, ill add a helper for the enum return type
>
> + hmap_insert(isb_tr_dps, &dp_info->node, uuid_hash(
> > + &isb_dp->nb_ic_uuid));
> > + continue;
> > + }
> > +
> > + hmap_insert(isb_ts_dps, &dp_info->node, uuid_hash(
> > + &isb_dp->nb_ic_uuid));
> > }
> >
> > bool vxlan_mode = false;
> > @@ -230,12 +278,10 @@ ts_run(struct ic_context *ctx)
> > }
> > }
> > }
> > -
> >
>
> nit: Unrelated change.
Ack,
>
>
> > /* Sync INB TS to AZ NB */
> > if (ctx->ovnnb_txn) {
> > struct shash nb_tses = SHASH_INITIALIZER(&nb_tses);
> > const struct nbrec_logical_switch *ls;
> > -
> >
>
> nit: Unrelated change.
Ack, ill remove
>
>
> > /* Get current NB Logical_Switch with other_config:interconn-ts */
> > NBREC_LOGICAL_SWITCH_FOR_EACH (ls, ctx->ovnnb_idl) {
> > const char *ts_name = smap_get(&ls->other_config,
> > "interconn-ts");
> > @@ -266,17 +312,18 @@ ts_run(struct ic_context *ctx)
> > }
> > }
> >
> > - isb_dp = shash_find_data(&isb_dps, ts->name);
> > - if (isb_dp) {
> > + struct ic_dp_info *dp_info = ic_dp_find(isb_ts_dps,
> > + &ts->header_.uuid);
> > + if (dp_info) {
> > int64_t nb_tnl_key = smap_get_int(&ls->other_config,
> > "requested-tnl-key",
> > 0);
> > - if (nb_tnl_key != isb_dp->tunnel_key) {
> > + if (nb_tnl_key != dp_info->isb_dp->tunnel_key) {
> > VLOG_DBG("Set other_config:requested-tnl-key %"PRId64
> > " for transit switch %s in NB.",
> > - isb_dp->tunnel_key, ts->name);
> > + dp_info->isb_dp->tunnel_key, ls->name);
> > char *tnl_key_str = xasprintf("%"PRId64,
> > - isb_dp->tunnel_key);
> > +
> > dp_info->isb_dp->tunnel_key);
> > nbrec_logical_switch_update_other_config_setkey(
> > ls, "requested-tnl-key", tnl_key_str);
> > free(tnl_key_str);
> > @@ -298,10 +345,11 @@ ts_run(struct ic_context *ctx)
> > if (ctx->ovnisb_txn) {
> > /* Create ISB Datapath_Binding */
> > ICNBREC_TRANSIT_SWITCH_FOR_EACH (ts, ctx->ovninb_idl) {
> > - isb_dp = shash_find_and_delete(&isb_dps, ts->name);
> > + isb_dp = ic_dp_find_and_remove(isb_ts_dps, &ts->header_.uuid);
> > if (!isb_dp) {
> > /* Allocate tunnel key */
> > - int64_t dp_key = allocate_ts_dp_key(&dp_tnlids,
> > vxlan_mode);
> > + int64_t dp_key = allocate_dp_key(dp_tnlids, vxlan_mode,
> > + "transit switch datapath");
> >
> if (!dp_key) {
> > continue;
> > }
> > @@ -309,23 +357,148 @@ ts_run(struct ic_context *ctx)
> > isb_dp = icsbrec_datapath_binding_insert(ctx->ovnisb_txn);
> > icsbrec_datapath_binding_set_transit_switch(isb_dp,
> > ts->name);
> > icsbrec_datapath_binding_set_tunnel_key(isb_dp, dp_key);
> > + icsbrec_datapath_binding_set_nb_ic_uuid(isb_dp,
> > + ts->header_.uuid);
> > + icsbrec_datapath_binding_set_type(isb_dp,
> > "transit-switch");
> > + ovn_add_tnlid(dp_tnlids, isb_dp->tunnel_key);
> > +
> > } else if (dp_key_refresh) {
> > - /* Refresh tunnel key since encap mode has changhed. */
> > - int64_t dp_key = allocate_ts_dp_key(&dp_tnlids,
> > vxlan_mode);
> > + /* Refresh tunnel key since encap mode has changed. */
> > + int64_t dp_key = allocate_dp_key(dp_tnlids, vxlan_mode,
> > + "transit switch datapath");
> > if (dp_key) {
> > + ovn_free_tnlid(dp_tnlids, isb_dp->tunnel_key);
> > icsbrec_datapath_binding_set_tunnel_key(isb_dp,
> > dp_key);
> > }
> > +
> > }
> > }
> >
> > - /* Delete extra ISB Datapath_Binding */
> > + struct ic_dp_info *dp_info;
> > + HMAP_FOR_EACH_POP (dp_info, node, isb_ts_dps) {
> > + icsbrec_datapath_binding_delete(dp_info->isb_dp);
> > + free(dp_info);
> > + }
> > + }
> > +}
> > +
> > +static const struct sbrec_chassis *
> > +find_sb_chassis(struct ic_context *ctx, const char *name)
> > +{
> > + const struct sbrec_chassis *key =
> > + sbrec_chassis_index_init_row(ctx->sbrec_chassis_by_name);
> > + sbrec_chassis_index_set_name(key, name);
> > +
> > + const struct sbrec_chassis *chassis =
> > + sbrec_chassis_index_find(ctx->sbrec_chassis_by_name, key);
> > + sbrec_chassis_index_destroy_row(key);
> > +
> > + return chassis;
> > +}
> > +
> > +static void
> > +tr_run(struct ic_context *ctx, struct hmap *dp_tnlids,
> > + struct hmap *isb_tr_dps)
> > +{
> > + struct shash nb_tres = SHASH_INITIALIZER(&nb_tres);
> > + const struct nbrec_logical_router *lr;
> > +
> > + if (ctx->ovnnb_txn) {
> > + NBREC_LOGICAL_ROUTER_FOR_EACH (lr, ctx->ovnnb_idl) {
> > + const char *tr_name = smap_get(&lr->options, "interconn-tr");
> > + if (tr_name) {
> > + shash_add(&nb_tres, tr_name, lr);
> > + }
> > +
> >
>
> nit: Extra empty line. Please check the rest of the file too.
Ack, remove extra line
>
> + }
> > +
> > + const struct icnbrec_transit_router *tr;
> > + ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->ovninb_idl) {
> > + lr = shash_find_data(&nb_tres, tr->name);
> > + if (!lr) {
> > + lr = nbrec_logical_router_insert(ctx->ovnnb_txn);
> > + nbrec_logical_router_set_name(lr, tr->name);
> > + nbrec_logical_router_update_options_setkey(lr,
> > "interconn-tr",
> > + tr->name);
> > + shash_add(&nb_tres, tr->name, lr);
> > + }
> > +
> > + struct ic_dp_info *dp_info = ic_dp_find(isb_tr_dps,
> > + &tr->header_.uuid);
> > + if (dp_info) {
> > + int64_t nb_tnl_key = smap_get_int(&lr->options,
> > + "requested-tnl-key",
> > + 0);
> > + if (nb_tnl_key != dp_info->isb_dp->tunnel_key) {
> > + VLOG_DBG("Set other_config:requested-tnl-key %"PRId64
> > + " for transit router %s in NB.",
> > + dp_info->isb_dp->tunnel_key, lr->name);
> > + char *tnl_key_str = xasprintf("%"PRId64,
> > +
> > dp_info->isb_dp->tunnel_key);
> > + nbrec_logical_router_update_options_setkey(
> > + lr, "requested-tnl-key", tnl_key_str);
> > + free(tnl_key_str);
> > + }
> > +
> > + }
> > + }
> > + }
> > +
> > + /* Sync TS between INB and ISB. This is performed after syncing with
> > AZ
> > + * SB, to avoid uncommitted ISB datapath tunnel key to be synced back
> > to
> > + * AZ. */
> > + if (ctx->ovnisb_txn) {
> > + /* Create ISB Datapath_Binding */
> > + const struct icnbrec_transit_router *tr;
> > + ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->ovninb_idl) {
> > + lr = shash_find_and_delete(&nb_tres, tr->name);
> > + struct ic_dp_info *dp_info = ic_dp_find(isb_tr_dps,
> > + &tr->header_.uuid);
> > + if (!dp_info) {
> > + int dp_key = smap_get_int(&tr->other_config,
> > + "requested-tnl-key", 0);
> > + if (!dp_key) {
> > + dp_key = allocate_dp_key(dp_tnlids, false,
> > + "transit router datapath");
> > + if (!dp_key) {
> > + VLOG_DBG("Set options:requested-tnl-key %"PRId32
> > + " for lr %s in ISB.", dp_key, tr->name);
> > + continue;
> > + }
> > +
> > + }
> > +
> > + struct icsbrec_datapath_binding *isb_dp;
> > + isb_dp = icsbrec_datapath_binding_insert(ctx->ovnisb_txn);
> > + icsbrec_datapath_binding_set_tunnel_key(isb_dp, dp_key);
> > + icsbrec_datapath_binding_set_nb_ic_uuid(isb_dp,
> > + tr->header_.uuid);
> > + icsbrec_datapath_binding_set_type(isb_dp,
> > "transit-router");
> > + } else {
> > + hmap_remove(isb_tr_dps, &dp_info->node);
> > + free(dp_info);
> > + }
> > +
> > + }
> > + struct ic_dp_info *dp_info;
> > + HMAP_FOR_EACH_POP (dp_info, node, isb_tr_dps) {
> > + icsbrec_datapath_binding_delete(dp_info->isb_dp);
> > + free(dp_info);
> > + }
> > struct shash_node *node;
> > - SHASH_FOR_EACH (node, &isb_dps) {
> > - icsbrec_datapath_binding_delete(node->data);
> > + SHASH_FOR_EACH (node, &nb_tres) {
> > + lr = node->data;
> > + nbrec_logical_router_delete(lr);
> > + const struct icnbrec_transit_router_port *trp;
> > + ICNBREC_TRANSIT_ROUTER_PORT_FOR_EACH (trp, ctx->ovninb_idl) {
> > + if (!strcmp(trp->transit_router, lr->name)) {
> > + icnbrec_transit_router_port_delete(trp);
> > + }
> > +
> > + }
> > }
> > }
> > - ovn_destroy_tnlids(&dp_tnlids);
> > - shash_destroy(&isb_dps);
> > + shash_destroy(&nb_tres);
> > }
> >
> > /* Returns true if any information in gw and chassis is different. */
> > @@ -451,6 +624,7 @@ gateway_run(struct ic_context *ctx)
> > gw = shash_find_and_delete(&remote_gws, chassis->name);
> > if (!gw) {
> > sbrec_chassis_delete(chassis);
> > + continue;
> >
>
> This continue doesn't do anything for the loop outcome.
Ack, continue removed
>
>
> > } else if (is_gateway_data_changed(gw, chassis)) {
> > sync_isb_gw_to_sb(ctx, gw, chassis);
> > }
> > @@ -498,6 +672,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)
> > @@ -584,20 +783,6 @@ get_lp_address_for_sb_pb(struct ic_context *ctx,
> > return peer->n_mac ? *peer->mac : NULL;
> > }
> >
> > -static const struct sbrec_chassis *
> > -find_sb_chassis(struct ic_context *ctx, const char *name)
> > -{
> > - const struct sbrec_chassis *key =
> > - sbrec_chassis_index_init_row(ctx->sbrec_chassis_by_name);
> > - sbrec_chassis_index_set_name(key, name);
> > -
> > - const struct sbrec_chassis *chassis =
> > - sbrec_chassis_index_find(ctx->sbrec_chassis_by_name, key);
> > - sbrec_chassis_index_destroy_row(key);
> > -
> > - return chassis;
> > -}
> > -
> > static void
> > sync_lsp_tnl_key(const struct nbrec_logical_switch_port *lsp,
> > int64_t isb_tnl_key)
> > @@ -615,6 +800,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,
> > @@ -701,6 +903,49 @@ 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;
> > +};
> > +
> > +/* For each local port:
> > + * - Sync from NB to ISB.
> > + * - Sync gateway from SB to ISB.
> > + * - Sync tunnel key from ISB to NB.
> > + */
> > +static void
> > +sync_local_router_port(struct ic_context *ctx,
> > + const struct icsbrec_port_binding *isb_pb,
> > + const struct sbrec_port_binding *sb_pb,
> > + struct ic_trp_data *trp_data)
> > +{
> > + (void) ctx;
> > + (void) sb_pb;
> >
>
> Please don't pass unused arguments.
> Also this function is a bit backwards isn't it?
> We are supposed to sync those thing from IC databases
> into NB.
Ack, Yes, both should sync from IC, ill change this.
>
>
> > + const struct nbrec_logical_router_port *lrp = trp_data->lrp;
> > +
> > + /* Sync address from NB to ISB */
> > + if (!strcmp(lrp->mac, isb_pb->address)) {
> > + icsbrec_port_binding_set_address(isb_pb, lrp->mac);
> > + }
> > +
> > + const char *chassis_name = smap_get(&lrp->options,
> > "requested-chassis");
> > + if (chassis_name) {
> > + if (!strcmp(chassis_name, trp_data->chassis)) {
> > + icnbrec_transit_router_port_set_chassis(trp_data->trp,
> > + chassis_name);
> > + }
> > +
> > + }
> > +
> > + /* 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
> > @@ -752,6 +997,32 @@ sync_remote_port(struct ic_context *ctx,
> > }
> > }
> >
> > +/* For each remote port:
> > + * - Sync from ISB to NB
> > + */
> > +static void
> > +sync_remote_router_port(struct ic_context *ctx OVS_UNUSED,
> > + const struct icsbrec_port_binding *isb_pb,
> > + const struct ic_trp_data *trp_data,
> > + const struct sbrec_port_binding *sb_pb OVS_UNUSED)
> > +{
> >
>
> The sync should actually be same for both
> local and remote TRPs. Only exception is the
> the "requested-chassis" option, remote should
> set it, local should clear it.
>
Ack, ill update syncs to match, unless requested-chassis is set
>
> > + const struct nbrec_logical_router_port *lrp = trp_data->lrp;
> > +
> > + /* Sync from ICNB to NB */
> > + if (strcmp(trp_data->chassis, "")) {
> >
>
> nit: "!trp_data->chassis[0]"
Ack, change to above
>
> + 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,
> > @@ -778,6 +1049,9 @@ 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,
> > uint32_t pb_tnl_key)
> > {
> > const struct icsbrec_port_binding *isb_pb =
> > @@ -786,10 +1060,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);
> > + 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")) {
> > + 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);
> > @@ -808,10 +1089,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
> > @@ -822,6 +1102,236 @@ 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 = xzalloc(sizeof *info);
> >
> > No need for zero alloc and please use designated initializer.
Ack, ill change to designated initializer
>
>
> > + info->isb_pb = isb_pb;
> > + info->uuid = isb_pb->nb_ic_uuid;
> > + info->logical_port = xstrdup(isb_pb->logical_port);
> > + return info;
> > +}
> > +
> > +static void
> > +ic_pb_insert(struct hmap *dp_info, struct ic_pb_info *info)
> > +{
> > + if (info) {
> >
>
> This check can be removed, there is always a non-NULL value passed.
Ack
>
>
> > + size_t hash = uuid_hash(&info->isb_pb->nb_ic_uuid);
> > + hash = hash_string(info->isb_pb->logical_port, hash);
> > + hmap_insert(dp_info, &info->node, hash);
> > + }
> > +
> > +}
> > +
> > +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 = uuid_hash(nb_ic_uuid);
> > + hash = hash_string(logical_port, hash);
> >
>
> It's usually a good idea to have a separate hash function
> when there are more things to hash.
Ack, ill add a hash function
>
> +
> > + 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;
> > +}
> > +
> > +static const struct icsbrec_port_binding *
> > +ic_pb_find_and_remove(struct hmap *dp_info, const struct uuid *nb_ic_uuid,
> > + const char *logical_port)
> > +{
> > + struct ic_pb_info *info = ic_pb_find(dp_info, nb_ic_uuid,
> > logical_port);
> > + if (info) {
> > + hmap_remove(dp_info, &info->node);
> > + const struct icsbrec_port_binding *isb_pb = info->isb_pb;
> > + free(info->logical_port);
> > + free(info);
> > + return isb_pb;
> > + }
> > +
> > + return NULL;
> > +}
>
> +
> > +static struct ic_pb_info *
> > +ic_pb_remove(struct hmap *dp_info, const struct uuid *nb_ic_uuid,
> > + const char *logical_port)
> > +{
> > + 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)) {
> > +
> > + 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)
> > +{
> > + 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 icsbrec_port_binding *
> > +ic_pb_remove_node_and_free(struct hmap *dp_info, struct ic_pb_info
> > *pb_info)
> > +{
> > + if (pb_info) {
> > + hmap_remove(dp_info, &pb_info->node);
> > + return ic_pb_free(pb_info);
> > + }
> > +
> > + return NULL;
> > +}
> >
>
> This feels like unnecessary amount of functions,
> we usually define the following for structs like these:
> *_add()
> *_find()
> *_destroy()
> *_hash()
>
> most of the logic is representable by the four above.
Ack, ill change to the above functions
>
>
> > +
> > +
> > +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)
> > +{
> > + 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 = xzalloc(sizeof *trp_data);
> > + trp_data->mac = xstrdup(trp->mac);
> > + trp_data->name = xstrdup(trp->name);
> > + trp_data->chassis = xstrdup(trp->chassis);
> > + trp_data->tr_name = xstrdup(trp->transit_router);
> > + trp_data->nb_ic_uuid = trp->nb_ic_uuid;
> > + trp_data->lrp = lrp;
> > + trp_data->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;
> > +}
> > +
> > +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)
> > +{
> >
>
I did not include non remote ports in the IC SB because they can have
different tunnel keys in different AZs, and I thought that would flood
the IC SB. Also, I saw the IC SB as a way to advertise information that
should be consistent across all AZ, since the keys change per AZ I left them
out.
> I'm little puzzled by the logic, why should only
> port with remote chassis in IC SB? Both of types in fact
> should be in IC SB.
>
> +
> > + const struct icsbrec_port_binding *isb_pb;
> > + bool is_remote_port = trp_port_is_remote(ctx, trp_data->lrp);
> > + if (!is_remote_port) {
> > + /* Only port with a remote chassis get an entry in ICSB */
> > + if (!!find_sb_chassis(ctx, trp_data->chassis)) {
> >
>
> nit: The double !! is not needed.
Ack, ill remove the double !!
>
>
> > + isb_pb = ic_pb_find_and_remove(local_pbs,
> > &trp_data->nb_ic_uuid,
> > + trp_data->name);
> > + if (!isb_pb) {
> > + uint32_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", trp_data->mac, pb_tnl_key);
> > + } else {
> > + sync_local_router_port(ctx, isb_pb, sb_pb, trp_data);
> > + }
> > + }
> > +
> > + } else {
> > + isb_pb = ic_pb_find_and_remove(remote_pbs, &trp_data->nb_ic_uuid,
> > + trp_data->name);
> > + if (isb_pb) {
> > + sb_pb = find_lp_in_sb(ctx, trp_data->name);
> > + if (!sb_pb) {
> > + return;
> > + }
> > +
> > + sync_remote_router_port(ctx, isb_pb, trp_data, sb_pb);
> > + }
> > +
> > + }
> > +}
> > +
> > static void
> > port_binding_run(struct ic_context *ctx)
> > {
> > @@ -829,46 +1339,54 @@ port_binding_run(struct ic_context *ctx)
> > return;
> > }
> >
> > - struct shash isb_all_local_pbs =
> > SHASH_INITIALIZER(&isb_all_local_pbs);
> > - struct shash_node *node;
> > -
> > - 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);
> > + 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_insert(&isb_all_local_pbs, pb_info);
> > + }
> >
> > - 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);
> > + ovn_add_tnlid(&pb_tnlids, current_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;
> > +
> > 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);
> > + icsbrec_port_binding_index_set_type(isb_pb_key, "transit-switch");
> > + 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,
> > + ¤t_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_insert(&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,
> > + ¤t_isb_pb->nb_ic_uuid,
> > current_isb_pb->logical_port);
> > + if (!pb_info) {
> > + pb_info = ic_pb_create(current_isb_pb);
> > + ic_pb_insert(&remote_pbs, pb_info);
> > + }
> > +
> > }
> > - ovn_add_tnlid(&pb_tnlids, isb_pb->tunnel_key);
> > }
> > icsbrec_port_binding_index_destroy_row(isb_pb_key);
> >
> > @@ -879,56 +1397,168 @@ 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;
> > + isb_pb = ic_pb_find_and_remove(&local_pbs,
> > &ts->header_.uuid,
> > + 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", 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);
> > + const struct icsbrec_port_binding *isb_pb;
> > + isb_pb = ic_pb_find_and_remove(&remote_pbs,
> > &ts->header_.uuid,
> > + 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;
> > }
> > sync_remote_port(ctx, isb_pb, lsp, sb_pb);
> > }
> > +
> > } else {
> > VLOG_DBG("Ignore lsp %s on ts %s with type %s.",
> > lsp->name, ts->name, lsp->type);
> > }
> > }
> >
> > - /* 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 */
> > - SHASH_FOR_EACH (node, &remote_pbs) {
> > - create_nb_lsp(ctx, node->data, ls);
> > + 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);
> > }
> >
> > - shash_destroy(&local_pbs);
> > - shash_destroy(&remote_pbs);
> > - ovn_destroy_tnlids(&pb_tnlids);
> > + hmap_destroy(&local_pbs);
> > + hmap_destroy(&remote_pbs);
> > }
> >
> > - SHASH_FOR_EACH (node, &isb_all_local_pbs) {
> > - icsbrec_port_binding_delete(node->data);
> > + /* Walk list of transit router ports */
> > + struct shash trp_ports = SHASH_INITIALIZER(&trp_ports);
> > + trp_parse(ctx, &trp_ports);
> > +
> > + 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);
> > + icsbrec_port_binding_index_set_type(isb_pb_key, "transit-router");
> > +
> > + 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_insert(&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_insert(&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);
> > + continue;
> > + } 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);
> > + }
> > +
> > + /* Create lsp in NB for remote ports */
> > + HMAP_FOR_EACH_POP (info, node, &remote_pbs) {
> > + isb_pb = ic_pb_free(info);
> > + }
> > +
> > + 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;
> > + isb_pb = ic_pb_remove_node_and_free(&isb_all_local_pbs, info);
> > + if (isb_pb) {
> > + icsbrec_port_binding_delete(isb_pb);
> > + }
> > +
> > }
> >
> > - 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 {
> > @@ -1609,20 +2239,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)
> > @@ -1728,12 +2344,14 @@ sync_learned_routes(struct ic_context *ctx,
> > route_filter_tag = "";
> > }
> >
> > - isb_route_key =
> > icsbrec_route_index_init_row(ctx->icsbrec_route_by_ts);
> > - icsbrec_route_index_set_transit_switch(isb_route_key,
> > - isb_pb->transit_switch);
> > + isb_route_key = icsbrec_route_index_init_row(
> > + ctx->icsbrec_route_by_nb_ic_uuid_type);
> > + icsbrec_route_index_set_nb_ic_uuid(isb_route_key,
> > + isb_pb->nb_ic_uuid);
> > + icsbrec_route_index_set_type(isb_route_key, isb_pb->type);
> >
> > ICSBREC_ROUTE_FOR_EACH_EQUAL (isb_route, isb_route_key,
> > - ctx->icsbrec_route_by_ts) {
> > +
> > ctx->icsbrec_route_by_nb_ic_uuid_type) {
> > const char *lr_id = smap_get(&isb_route->external_ids,
> > "lr-id");
> > if (lr_id == NULL) {
> > continue;
> > @@ -1875,22 +2493,41 @@ ad_route_sync_external_ids(const struct
> > ic_route_info *route_adv,
> > }
> > }
> >
> > +struct ic_route {
> > + struct hmap_node node;
> > + struct uuid nb_ic_uuid;
> > + struct hmap routes_ad;
> > + const char *ts_name;
> > + const char *type;
> > +};
> >
>
> As mentioned on the schema, we shouldn't change the
> routes at all for now.
Ack, Ill undo all route changes
>
>
> > +
> > +static struct ic_route *
> > +ic_route_ad_find(struct hmap *route, const struct uuid *nb_ic_uuid)
> > +{
> > + struct ic_route *ic_route;
> > + size_t hash = uuid_hash(nb_ic_uuid);
> > + HMAP_FOR_EACH_WITH_HASH (ic_route, node, hash, route) {
> > + if (uuid_equals(nb_ic_uuid, &ic_route->nb_ic_uuid)) {
> > + return ic_route;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > /* Sync routes from routes_ad to IC-SB. */
> > static void
> > advertise_routes(struct ic_context *ctx,
> > - const struct icsbrec_availability_zone *az,
> > - const char *ts_name,
> > - struct hmap *routes_ad)
> > + struct ic_route *route)
> > {
> > ovs_assert(ctx->ovnisb_txn);
> > const struct icsbrec_route *isb_route;
> > const struct icsbrec_route *isb_route_key =
> > - icsbrec_route_index_init_row(ctx->icsbrec_route_by_ts_az);
> > - icsbrec_route_index_set_transit_switch(isb_route_key, ts_name);
> > - icsbrec_route_index_set_availability_zone(isb_route_key, az);
> > + icsbrec_route_index_init_row(ctx->icsbrec_route_by_nb_ic_az);
> > + icsbrec_route_index_set_nb_ic_uuid(isb_route_key, route->nb_ic_uuid);
> > + icsbrec_route_index_set_availability_zone(isb_route_key,
> > ctx->runned_az);
> >
> > ICSBREC_ROUTE_FOR_EACH_EQUAL (isb_route, isb_route_key,
> > - ctx->icsbrec_route_by_ts_az) {
> > + ctx->icsbrec_route_by_nb_ic_az) {
> > struct in6_addr prefix, nexthop;
> > unsigned int plen;
> >
> > @@ -1904,7 +2541,7 @@ advertise_routes(struct ic_context *ctx,
> > continue;
> > }
> > struct ic_route_info *route_adv =
> > - ic_route_find(routes_ad, &prefix, plen, &nexthop,
> > + ic_route_find(&route->routes_ad, &prefix, plen, &nexthop,
> > isb_route->origin, isb_route->route_table, 0);
> > if (!route_adv) {
> > /* Delete the extra route from IC-SB. */
> > @@ -1915,7 +2552,7 @@ advertise_routes(struct ic_context *ctx,
> > } else {
> > ad_route_sync_external_ids(route_adv, isb_route);
> >
> > - hmap_remove(routes_ad, &route_adv->node);
> > + hmap_remove(&route->routes_ad, &route_adv->node);
> > free(route_adv);
> > }
> > }
> > @@ -1923,10 +2560,12 @@ advertise_routes(struct ic_context *ctx,
> >
> > /* Create the missing routes in IC-SB */
> > struct ic_route_info *route_adv;
> > - HMAP_FOR_EACH_SAFE (route_adv, node, routes_ad) {
> > + HMAP_FOR_EACH_SAFE (route_adv, node, &route->routes_ad) {
> > isb_route = icsbrec_route_insert(ctx->ovnisb_txn);
> > - icsbrec_route_set_transit_switch(isb_route, ts_name);
> > - icsbrec_route_set_availability_zone(isb_route, az);
> > + icsbrec_route_set_availability_zone(isb_route, ctx->runned_az);
> > + icsbrec_route_set_type(isb_route, route->type);
> > + icsbrec_route_set_transit_switch(isb_route, route->ts_name);
> > + icsbrec_route_set_nb_ic_uuid(isb_route, route->nb_ic_uuid);
> >
> > char *prefix_s, *nexthop_s;
> > if (IN6_IS_ADDR_V4MAPPED(&route_adv->prefix)) {
> > @@ -1954,7 +2593,7 @@ advertise_routes(struct ic_context *ctx,
> >
> > ad_route_sync_external_ids(route_adv, isb_route);
> >
> > - hmap_remove(routes_ad, &route_adv->node);
> > + hmap_remove(&route->routes_ad, &route_adv->node);
> > free(route_adv);
> > }
> > }
> > @@ -2045,19 +2684,19 @@ build_ts_routes_to_adv(struct ic_context *ctx,
> > static void
> > collect_lr_routes(struct ic_context *ctx,
> > struct ic_router_info *ic_lr,
> > - struct shash *routes_ad_by_ts)
> > + struct hmap *routes_ad_by_ts)
> > {
> > const struct nbrec_nb_global *nb_global =
> > nbrec_nb_global_first(ctx->ovnnb_idl);
> > ovs_assert(nb_global);
> >
> > const struct icsbrec_port_binding *isb_pb;
> > - const char *lrp_name, *ts_name, *route_table, *route_tag;
> > + const char *lrp_name, *route_table, *route_tag;
> > struct lport_addresses ts_port_addrs;
> > const struct icnbrec_transit_switch *key;
> > const struct nbrec_logical_router_port *lrp;
> >
> > - struct hmap *routes_ad;
> > + struct ic_route *routes_ad;
> > const struct icnbrec_transit_switch *t_sw;
> > VECTOR_FOR_EACH (&ic_lr->isb_pbs, isb_pb) {
> > key = icnbrec_transit_switch_index_init_row(
> > @@ -2069,12 +2708,16 @@ collect_lr_routes(struct ic_context *ctx,
> > if (!t_sw) {
> > continue;
> > }
> > - ts_name = t_sw->name;
> > - routes_ad = shash_find_data(routes_ad_by_ts, ts_name);
> > + routes_ad = ic_route_ad_find(routes_ad_by_ts,
> > &t_sw->header_.uuid);
> > +
> > if (!routes_ad) {
> > routes_ad = xzalloc(sizeof *routes_ad);
> > - hmap_init(routes_ad);
> > - shash_add(routes_ad_by_ts, ts_name, routes_ad);
> > + hmap_init(&routes_ad->routes_ad);
> > + routes_ad->nb_ic_uuid = isb_pb->nb_ic_uuid;
> > + routes_ad->ts_name = xstrdup(t_sw->name);
> > + routes_ad->type = xstrdup(isb_pb->type);
> > + hmap_insert(routes_ad_by_ts, &routes_ad->node,
> > + uuid_hash(&routes_ad->nb_ic_uuid));
> > }
> >
> > if (!extract_lsp_addresses(isb_pb->address, &ts_port_addrs)) {
> > @@ -2094,32 +2737,35 @@ collect_lr_routes(struct ic_context *ctx,
> > route_table = "";
> > route_tag = "";
> > }
> > - build_ts_routes_to_adv(ctx, ic_lr, routes_ad, &ts_port_addrs,
> > - nb_global, route_table, route_tag, lrp);
> > + build_ts_routes_to_adv(ctx, ic_lr, &routes_ad->routes_ad,
> > + &ts_port_addrs, nb_global, route_table, route_tag,
> > lrp);
> > destroy_lport_addresses(&ts_port_addrs);
> > }
> > }
> >
> > static void
> > -delete_orphan_ic_routes(struct ic_context *ctx,
> > - const struct icsbrec_availability_zone *az)
> > +delete_orphan_ic_routes(struct ic_context *ctx)
> > {
> > const struct icsbrec_route *isb_route, *isb_route_key =
> > icsbrec_route_index_init_row(ctx->icsbrec_route_by_az);
> > - icsbrec_route_index_set_availability_zone(isb_route_key, az);
> > -
> > - const struct icnbrec_transit_switch *t_sw, *t_sw_key;
> > + icsbrec_route_index_set_availability_zone(isb_route_key,
> > ctx->runned_az);
> >
> > ICSBREC_ROUTE_FOR_EACH_EQUAL (isb_route, isb_route_key,
> > ctx->icsbrec_route_by_az)
> > {
> > - t_sw_key = icnbrec_transit_switch_index_init_row(
> > - ctx->icnbrec_transit_switch_by_name);
> > - icnbrec_transit_switch_index_set_name(t_sw_key,
> > - isb_route->transit_switch);
> > - t_sw = icnbrec_transit_switch_index_find(
> > - ctx->icnbrec_transit_switch_by_name, t_sw_key);
> > - icnbrec_transit_switch_index_destroy_row(t_sw_key);
> > + if (isb_route->type && !strcmp(isb_route->type,
> > "transit-router")) {
> > + const struct icnbrec_transit_router *t_rt;
> > +
> > + t_rt = icnbrec_transit_router_get_for_uuid(ctx->ovninb_idl,
> > + &isb_route->nb_ic_uuid);
> > + if (!t_rt) {
> > + icsbrec_route_delete(isb_route);
> > + }
> > + continue;
> > + }
> > + const struct icnbrec_transit_switch *t_sw;
> > + t_sw = icnbrec_transit_switch_get_for_uuid(ctx->ovninb_idl,
> > + &isb_route->nb_ic_uuid);
> >
> > if (!t_sw || !find_lrp_of_nexthop(ctx, isb_route)) {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > @@ -2140,7 +2786,7 @@ route_run(struct ic_context *ctx)
> > return;
> > }
> >
> > - delete_orphan_ic_routes(ctx, ctx->runned_az);
> > + delete_orphan_ic_routes(ctx);
> >
> > struct hmap ic_lrs = HMAP_INITIALIZER(&ic_lrs);
> > const struct icsbrec_port_binding *isb_pb;
> > @@ -2156,8 +2802,10 @@ 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")) {
> > + continue;
> > + }
> > const struct nbrec_logical_switch_port *nb_lsp;
> > -
> > nb_lsp = get_lsp_by_ts_port_name(ctx, isb_pb->logical_port);
> > if (!strcmp(nb_lsp->type, "switch")) {
> > VLOG_DBG("IC-SB Port_Binding '%s' on ts '%s' corresponds to a
> > "
> > @@ -2204,7 +2852,7 @@ route_run(struct ic_context *ctx)
> > icsbrec_port_binding_index_destroy_row(isb_pb_key);
> >
> > struct ic_router_info *ic_lr;
> > - struct shash routes_ad_by_ts = SHASH_INITIALIZER(&routes_ad_by_ts);
> > + struct hmap routes_ad_by_ts = HMAP_INITIALIZER(&routes_ad_by_ts);
> > HMAP_FOR_EACH_SAFE (ic_lr, node, &ic_lrs) {
> > collect_lr_routes(ctx, ic_lr, &routes_ad_by_ts);
> > sync_learned_routes(ctx, ic_lr);
> > @@ -2213,12 +2861,17 @@ route_run(struct ic_context *ctx)
> > hmap_remove(&ic_lrs, &ic_lr->node);
> > free(ic_lr);
> > }
> > - struct shash_node *node;
> > - SHASH_FOR_EACH (node, &routes_ad_by_ts) {
> > - advertise_routes(ctx, ctx->runned_az, node->name, node->data);
> > - hmap_destroy(node->data);
> > - }
> > - shash_destroy_free_data(&routes_ad_by_ts);
> > +
> > + struct ic_route *node;
> > + HMAP_FOR_EACH_SAFE (node, node, &routes_ad_by_ts) {
> > + advertise_routes(ctx, node);
> > + hmap_destroy(&node->routes_ad);
> > + free((char *) node->ts_name);
> > + free((char *) node->type);
> > + free(node);
> > + };
> > +
> > + hmap_destroy(&routes_ad_by_ts);
> > hmap_destroy(&ic_lrs);
> > }
> >
> > @@ -2733,11 +3386,29 @@ update_sequence_numbers(struct ic_context *ctx,
> > static void
> > ovn_db_run(struct ic_context *ctx)
> > {
> > + struct hmap dp_tnlids = HMAP_INITIALIZER(&dp_tnlids);
> > + struct hmap isb_ts_dps = HMAP_INITIALIZER(&isb_ts_dps);
> > + struct hmap isb_tr_dps = HMAP_INITIALIZER(&isb_tr_dps);
> > +
> > gateway_run(ctx);
> > - ts_run(ctx);
> > + ts_run(ctx, &dp_tnlids, &isb_ts_dps, &isb_tr_dps);
> > + tr_run(ctx, &dp_tnlids, &isb_tr_dps);
> > port_binding_run(ctx);
> > route_run(ctx);
> > sync_service_monitor(ctx);
> > +
> > + ovn_destroy_tnlids(&dp_tnlids);
> > +
> > + struct ic_dp_info *dps;
> > + HMAP_FOR_EACH_SAFE (dps, node, &isb_ts_dps) {
> > + free(dps);
> > + }
> > + hmap_destroy(&isb_ts_dps);
> > +
> > + HMAP_FOR_EACH_SAFE (dps, node, &isb_tr_dps) {
> > + free(dps);
> > + }
> > + hmap_destroy(&isb_tr_dps);
> > }
> >
> > static void
> > @@ -3105,6 +3776,9 @@ 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);
> > @@ -3114,6 +3788,7 @@ main(int argc, char *argv[])
> > struct ovsdb_idl_index *sbrec_port_binding_by_name
> > = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> > &sbrec_port_binding_col_logical_port);
> > +
> > struct ovsdb_idl_index *sbrec_chassis_by_name
> > = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> > &sbrec_chassis_col_name);
> > @@ -3139,26 +3814,27 @@ 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,
> > &icsbrec_route_col_availability_zone);
> >
> > - struct ovsdb_idl_index *icsbrec_route_by_ts
> > - = ovsdb_idl_index_create1(ovnisb_idl_loop.idl,
> > - &icsbrec_route_col_transit_switch);
> > + struct ovsdb_idl_index *icsbrec_route_by_nb_ic_uuid_type
> > + = ovsdb_idl_index_create2(ovnisb_idl_loop.idl,
> > + &icsbrec_route_col_nb_ic_uuid,
> > + &icsbrec_route_col_type);
> >
> > - struct ovsdb_idl_index *icsbrec_route_by_ts_az
> > + struct ovsdb_idl_index *icsbrec_route_by_nb_ic_az
> > = ovsdb_idl_index_create2(ovnisb_idl_loop.idl,
> > - &icsbrec_route_col_transit_switch,
> > + &icsbrec_route_col_nb_ic_uuid,
> > &icsbrec_route_col_availability_zone);
> >
> > struct ovsdb_idl_index *icsbrec_service_monitor_by_source_az
> > @@ -3221,6 +3897,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,
> > @@ -3234,17 +3911,20 @@ 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,
> > .icsbrec_service_monitor_by_source_az =
> > icsbrec_service_monitor_by_source_az,
> > .icsbrec_service_monitor_by_target_az =
> > icsbrec_service_monitor_by_target_az,
> > .icsbrec_service_monitor_by_target_az_logical_port =
> > icsbrec_service_monitor_by_target_az_logical_port,
> > + .icsbrec_route_by_nb_ic_uuid_type =
> > + icsbrec_route_by_nb_ic_uuid_type,
> > + .icsbrec_route_by_nb_ic_az = icsbrec_route_by_nb_ic_az,
> > };
> >
> > if (!state.had_lock &&
> > ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> > diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml
> > index 11efdf9a2..9a932fef7 100644
> > --- a/ovn-architecture.7.xml
> > +++ b/ovn-architecture.7.xml
> > @@ -2372,17 +2372,20 @@
> > <p>
> > A global OVN Interconnection Northbound database is introduced for the
> > operator (probably through CMS systems) to configure transit logical
> > - switches that connect logical routers from different AZs. A transit
> > - switch is similar to a regular logical switch, but it is used for
> > + switches/routers that connect logical routers/switches from different
> > AZs.
> > + A transit switch is similar to a regular logical switch, but it is
> > used for
> > interconnection purpose only. Typically, each transit switch can be
> > used
> > to connect all logical routers that belong to same tenant across all
> > AZs.
> > + A transit router is similar to a regular logical router, but is used
> > for
> > + redirecting traffic to a specific AZ or cloning a router port across
> > + multiple AZs.
> > </p>
> >
> > <p>
> > A dedicated daemon process <code>ovn-ic</code>, OVN interconnection
> > controller, in each AZ will consume this data and populate
> > corresponding
> > - logical switches to their own northbound databases for each AZ, so
> > that
> > - logical routers can be connected to the transit switch by creating
> > + logical switches/routers to their own northbound databases for each
> > AZ. So
> > + that logical routers can be connected to the transit switch by
> > creating
> > patch port pairs in their northbound databases. Any router ports
> > connected to the transit switches are considered interconnection
> > ports,
> > which will be exchanged between AZs.
> > diff --git a/ovn-ic-nb.ovsschema b/ovn-ic-nb.ovsschema
> > index 8145b0335..c1eefb25f 100644
> > --- a/ovn-ic-nb.ovsschema
> > +++ b/ovn-ic-nb.ovsschema
> > @@ -1,7 +1,7 @@
> > {
> > "name": "OVN_IC_Northbound",
> > - "version": "1.2.0",
> > - "cksum": "4176728051 3557",
> > + "version": "1.2.1",
> >
>
> This is a pretty big change so we should bump the Y version e.g.
> 1.3.0.
Ack, Ill change the version
>
>
> > + "cksum": "471978705 4834",
> > "tables": {
> > "IC_NB_Global": {
> > "columns": {
> > @@ -35,6 +35,34 @@
> > "min": 0, "max": "unlimited"}}},
> > "isRoot": true,
> > "indexes": [["name"]]},
> > + "Transit_Router": {
> > + "columns": {
> > + "name": {"type": "string"},
> > + "other_config": {
> > + "type": {"key": "string", "value": "string",
> > + "min": 0, "max": "unlimited"}},
> > + "external_ids": {
> > + "type": {"key": "string", "value": "string",
> > + "min": 0, "max": "unlimited"}}},
> > + "isRoot": true,
> > + "indexes": [["name"]]},
> > + "Transit_Router_Port": {
> > + "columns": {
> > + "name": {"type": "string"},
> > + "other_config": {
> > + "type": {"key": "string", "value": "string",
> > + "min": 0, "max": "unlimited"}},
> > + "mac": {"type": "string"},
> > + "chassis": {"type": "string"},
> >
> + "networks": {"type": {"key": "string",
> > + "min": 0,
> > + "max": "unlimited"}},
> > + "nb_ic_uuid": {"type": {"key": {"type": "uuid"},
> > + "min": 1,
> > + "max": 1}},
> > + "transit_router": {"type": "string"}},
> > + "isRoot": true,
> > + "indexes": [["name"]]},
> > "Connection": {
> > "columns": {
> > "target": {"type": "string"},
> > diff --git a/ovn-ic-nb.xml b/ovn-ic-nb.xml
> > index 304e100ff..90d4694c4 100644
> > --- a/ovn-ic-nb.xml
> > +++ b/ovn-ic-nb.xml
> > @@ -120,6 +120,76 @@
> > </group>
> > </table>
> >
> > + <table name="Transit_Router" title="Transit logical router">
> > + <p>
> > + Each row represents one transit logical router for interconnection
> > + between different OVN deployments (availability zones).
> > + </p>
> > +
> > + <group title="Naming">
> > + <column name="name">
> > + A name that uniquely identifies the transit logical router.
> > + </column>
> > + </group>
> > +
> > + <group title="Common Columns">
> > + <column name="other_config"/>
> > + <column name="external_ids">
> > + See <em>External IDs</em> at the beginning of this document.
> > + </column>
> > + </group>
> > + </table>
> > +
> > + <table name="Transit_Router_Port" title="Transit logical router">
> > + <p>
> > + Each row represents one transit logical router for interconnection
> > + between different OVN deployments (availability zones).
> > + </p>
> > +
> > + <group title="Naming">
> > + <column name="name">
> > + A name that uniquely identifies the transit logical router port.
> > + </column>
> > + </group>
> > +
> > + <column name="mac">
> > + The Ethernet address that belongs to this router port.
> > + </column>
> > +
> > + <column name="chassis">
> > + The chassis this router port should be bound to.
> > + </column>
> > +
> > + <column name="nb_ic_uuid">
> > + This is the UUID of the northbound IC logical datapath this port is
> > + associated with.
> > + </column>
> > +
> > + <column name="transit_router">
> > + The name of the <code>Transit_Router</code> this port belongs to.
> > + </column>
> > +
> > + <column name="networks">
> > + <p>
> > + The IP addresses and netmasks of the router port. For example,
> > + <code>192.168.0.1/24</code> indicates that the router's IP
> > + address is 192.168.0.1 and that packets destined to
> > + 192.168.0.<var>x</var> should be routed to this port.
> > + These are optional.
> > + </p>
> > +
> > + <p>
> > + A logical router port always adds a link-local IPv6 address
> > + (fe80::/64) automatically generated from the interface's MAC
> > + address using the modified EUI-64 format.
> > + </p>
> > + </column>
> > +
> > + <group title="Common Columns">
> > + <column name="other_config"/>
> > + </group>
> > + </table>
> > +
> > <table name="SSL">
> > SSL/TLS configuration for ovn-nb database access.
> >
> > diff --git a/ovn-ic-sb.ovsschema b/ovn-ic-sb.ovsschema
> > index 34b5457bb..5880729e8 100644
> > --- a/ovn-ic-sb.ovsschema
> > +++ b/ovn-ic-sb.ovsschema
> > @@ -1,7 +1,7 @@
> > {
> > "name": "OVN_IC_Southbound",
> > "version": "2.2.0",
> >
>
> The version bump here is definitely required.
Ack, Ill change the version
>
>
> > - "cksum": "2294868959 8438",
> > + "cksum": "662060257 9716",
> > "tables": {
> > "IC_SB_Global": {
> > "columns": {
> > @@ -58,6 +58,13 @@
> > "Datapath_Binding": {
> > "columns": {
> > "transit_switch": {"type": "string"},
> > + "type": {"type": {"key": {"type": "string",
> > + "enum": ["set",
> > + ["transit-switch",
> > +
> > "transit-router"]]}}},
> >
>
> This suffers the same issue as the datapath refactor in SB.
> We should specify {"min": 0, "max": 1} to allow empty values.
Ack, ill add min/max to preserve backwards compatability
>
> + "nb_ic_uuid": {"type": {"key": {"type": "uuid"},
> > + "min": 1,
> > + "max": 1}},
> >
>
> Same here, the min should be 0.
Ack, ill add min/max to preserve backwards compatability
>
>
> > "tunnel_key": {
> > "type": {"key": {"type": "integer",
> > "minInteger": 1,
> > @@ -65,7 +72,7 @@
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > "min": 0, "max": "unlimited"}}},
> > - "indexes": [["tunnel_key"]],
> > + "indexes": [["nb_ic_uuid"],["tunnel_key"]],
> > "isRoot": true},
> > "Port_Binding": {
> > "columns": {
> > @@ -83,15 +90,29 @@
> > "refType": "weak"},
> > "min": 0, "max": 1}},
> > "address": {"type": "string"},
> > + "type": {"type": {"key": {"type": "string",
> > + "enum": ["set",
> > + ["transit-switch",
> > +
> > "transit-router"]]}}},
> >
>
> Same here, also I would prefer "transit-switch-port" and
> "transit-router-port".
Ack, Ill change to the names above
>
>
> > + "nb_ic_uuid": {"type": {"key": {"type": "uuid"},
> > + "min": 1,
> > + "max": 1}},
> >
>
> Same here with the min.
Ack, ill add min/max to preserve backwards compatability
>
>
> > "tunnel_key": {
> > "type": {"key": {"type": "integer",
> > "minInteger": 1,
> > @@ -65,7 +72,7 @@
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > "min": 0, "max": "unlimited"}}},
> > - "indexes": [["tunnel_key"]],
> > + "indexes": [["nb_ic_uuid"],["tunnel_key"]],
> > "isRoot": true},
> > "Port_Binding": {
> > "columns": {
> > @@ -83,15 +90,29 @@
> > "refType": "weak"},
> > "min": 0, "max": 1}},
> > "address": {"type": "string"},
> > + "type": {"type": {"key": {"type": "string",
> > + "enum": ["set",
> > + ["transit-switch",
> > +
> > "transit-router"]]}}},
> >
>
> Same here, also I would prefer "transit-switch-port" and
> "transit-router-port".
Ack, Ill change to the names above
>
>
> > + "nb_ic_uuid": {"type": {"key": {"type": "uuid"},
> > + "min": 1,
> > + "max": 1}},
> >
>
> Same here with the min.
Ack, ill add min/max to preserve backwards compatability
>
>
> > "external_ids": {"type": {"key": "string",
> > "value": "string",
> > "min": 0,
> > "max": "unlimited"}}},
>
>
> > "external_ids": {"type": {"key": "string",
> > "value": "string",
> > "min": 0,
> > "max": "unlimited"}}},
> > - "indexes": [["transit_switch", "tunnel_key"],
> > ["logical_port"]],
> > + "indexes": [["nb_ic_uuid", "tunnel_key"]],
> > "isRoot": true},
> > "Route": {
> > "columns": {
> > "transit_switch": {"type": "string"},
> > + "type": {"type": {"key": {"type": "string",
> > + "enum": ["set",
> > + ["transit-switch",
> > +
> > "transit-router"]]}}},
> > + "nb_ic_uuid": {"type": {"key": {"type": "uuid"},
> > + "min": 1,
> > + "max": 1}},
> >
>
> We shouldn't change the route at all for now.
Ack, Ill undo route changes
>
>
> > "availability_zone": {"type": {"key": {"type": "uuid",
> > "refTable": "Availability_Zone"}}},
> > "route_table": {"type": "string"},
> > @@ -104,8 +125,8 @@
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > "min": 0, "max": "unlimited"}}},
> > - "indexes": [["transit_switch", "availability_zone",
> > "route_table",
> > - "ip_prefix", "nexthop"]],
> > + "indexes": [["nb_ic_uuid", "type", "availability_zone",
> > + "route_table", "ip_prefix", "nexthop"]],
> > "isRoot": true},
> > "Connection": {
> > "columns": {
> > diff --git a/ovn-ic-sb.xml b/ovn-ic-sb.xml
> > index 35dc1f509..2e466271b 100644
> > --- a/ovn-ic-sb.xml
> > +++ b/ovn-ic-sb.xml
> > @@ -213,6 +213,15 @@
> > db="OVN_IC_Northbound"/> table.
> > </column>
> >
> > + <column name="type">
> > + Can be one of <code>transit-switch</code> or
> > <code>transit-router</code>
> > + </column>
> > +
> > + <column name="nb_ic_uuid" type='{"type": "string"}'>
> > + This is the UUID of the corresponding northbound IC logical datapath
> > + represented by this southbound <code>Datapath_Binding</code>.
> > + </column>
> > +
> > <column name="tunnel_key">
> > <p>
> > The tunnel key value to which the logical datapath is bound. The
> > key
> > @@ -302,6 +311,15 @@
> > </p>
> > </column>
> >
> > + <column name="type">
> > + Can be one of <code>switch-port</code> or <code>router-port</code>
> > + </column>
> > +
> > + <column name="nb_ic_uuid" type='{"type": "string"}'>
> > + This is the UUID of the corresponding northbound IC logical datapath
> > + represented by this southbound <code>Datapath_Binding</code>.
> > + </column>
> > +
> > </group>
> >
> > <group title="Common Columns">
> > @@ -323,6 +341,16 @@
> > The name of the transit switch, upon which the route is
> > advertised.
> > </column>
> >
> > + <column name="type">
> > + Can be one of <code>transit-switch</code> or
> > + <code>transit-router</code>
> > + </column>
> > +
> > + <column name="nb_ic_uuid" type='{"type": "string"}'>
> > + This is the UUID of the corresponding northbound IC logical
> > datapath
> > + represented by this southbound <code>Datapath_Binding</code>.
> > + </column>
> > +
> > <column name="availability_zone">
> > The availability zone that has advertised the route.
> > </column>
> > diff --git a/tests/ovn-ic-nbctl.at b/tests/ovn-ic-nbctl.at
> > index 2402d646c..058d66cb1 100644
> > --- a/tests/ovn-ic-nbctl.at
> > +++ b/tests/ovn-ic-nbctl.at
> > @@ -63,3 +63,44 @@ AT_CHECK([ovn-ic-nbctl --if-exists ts-del ts2])
> >
> > OVN_IC_NBCTL_TEST_STOP
> > AT_CLEANUP
> > +
> > +AT_SETUP([ovn-ic-nbctl])
> > +OVN_IC_NBCTL_TEST_START
> > +
> > +AT_CHECK([ovn-ic-nbctl tr-add tr0])
> > +AT_CHECK([ovn-ic-nbctl tr-list | uuidfilt], [0], [dnl
> > +<0> (tr0)
> > +])
> > +
> > +AT_CHECK([ovn-ic-nbctl tr-add tr1])
> > +AT_CHECK([ovn-ic-nbctl tr-list | uuidfilt], [0], [dnl
> > +<0> (tr0)
> > +<1> (tr1)
> > +])
> > +
> > +AT_CHECK([ovn-ic-nbctl tr-list | ovn-ic-nbctl tr-list | cut -d' ' -f2 |
> > sort], [0], [dnl
> > +(tr0)
> > +(tr1)
> > +])
> > +
> > +AT_CHECK([ovn-ic-nbctl tr-del tr1])
> > +AT_CHECK([ovn-ic-nbctl tr-list | uuidfilt], [0], [dnl
> > +<0> (tr0)
> > +])
> > +
> > +AT_CHECK([ovn-ic-nbctl tr-add tr0], [1], [],
> > + [ovn-ic-nbctl: tr0: a transit router with this name already exists
> > +])
> > +
> > +AT_CHECK([ovn-ic-nbctl --may-exist tr-add tr0])
> > +AT_CHECK([ovn-ic-nbctl tr-list | uuidfilt], [0], [dnl
> > +<0> (tr0)
> > +])
> > +
> > +AT_CHECK([ovn-ic-nbctl tr-del tr2], [1], [],
> > + [ovn-ic-nbctl: tr2: router name not found
> > +])
> > +
> > +AT_CHECK([ovn-ic-nbctl --if-exists tr-del tr2])
> > +OVN_IC_NBCTL_TEST_STOP
> > +AT_CLEANUP
> >
>
> This testing is also missing TRP related commands.
Ack, ill add TRP releated tests
>
>
> > diff --git a/tests/ovn-ic-sbctl.at b/tests/ovn-ic-sbctl.at
> > index 86d6b00f6..e61de5729 100644
> > --- a/tests/ovn-ic-sbctl.at
> > +++ b/tests/ovn-ic-sbctl.at
> > @@ -33,7 +33,7 @@ for az in 1 2; do
> > for pb in 1 2; do
> > ovn-ic-sbctl create port_binding logical_port=lp$az$gw$pb
> > transit_switch="ts$pb" \
> > address="\"aa:aa:aa:aa:0$az:$gw$pb 169.254.$pb.$az$gw/24\""
> > tunnel_key=$az$gw \
> > - availability_zone=$az_uuid gateway=gw$az$gw
> > + availability_zone=$az_uuid gateway=gw$az$gw type=transit-switch
> > nb_ic_uuid=b4b13365-2fef-4288-8e80-40663dc408$az$pb
> > done
> > done
> > done
> > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
> > index 2d92f3adf..d1fc56bf0 100644
> > --- a/tests/ovn-ic.at
> > +++ b/tests/ovn-ic.at
> > @@ -912,7 +912,7 @@ check ovn_as az1 ovn-nbctl lb-add lb_v6 [[4242::1]]:80
> > "[[4242::2]]:80"
> > check ovn_as az1 ovn-nbctl lr-lb-add lr1 lb_v6
> > OVS_WAIT_UNTIL([ovn_as az2 ovn-nbctl lr-route-list lr2 | grep learned |
> > grep 4242])
> >
> > -AT_CHECK([ovn-ic-sbctl list route | grep 'ip_prefix.*4242' -A 2], [0],
> > [dnl
> > +AT_CHECK([ovn-ic-sbctl list route | grep 'ip_prefix.*4242' -A 3 | sed
> > '/nb_ic_uuid/d'], [0], [dnl
> > ip_prefix : "4242::1/128"
> > nexthop : "2001:db8:1::1"
> > origin : loadbalancer
> > @@ -4248,6 +4248,205 @@ ovn_as az2 check ovn-nbctl remove
> > logical_router_port lrp-lr12-ts1 options ic-ro
> > ovn_as az1 check ovn-nbctl remove logical_router_port lrp-lr11-ts1
> > options ic-route-deny-learn
> >
> >
> > +OVN_CLEANUP_IC([az1], [az2])
> > +AT_CLEANUP
> > +])
> > +
> > +AT_BANNER([OVN Interconnection Controller Transit Router])
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ovn-ic -- Add transit router])
> > +
> > +ovn_init_ic_db
> > +ovn_start az1
> > +ovn_start az2
> > +
> > +OVS_WAIT_UNTIL([test 2 = `ovn-ic-sbctl show | wc -l`])
> >
>
> Please don't wait for a specific line count. This should be handled
> by the sync call below.
Ack, ill change this to a sync call
>
>
> > +
> > +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
> > +OVS_WAIT_UNTIL([test 6 = `ovn-sbctl list Datapath_Binding | wc -l`])
> >
>
> Wouldn't it be better to use wait_row_count?
> I won't repeat myself but this applies to all testing in this file.
>
> +AT_CHECK([ovn-nbctl lr-list | cut -c 38-], [0], [dnl
> >
>
Ack, Ill change to check_row_cout
> I would rather see the full list instead of using cut, the order
> can also change.
> Maybe even better would be to call:
> "check_row_count nb:Logical_Router 1 name=tr0".
> This also applied to the rest of the testing in this file.
>
>
> > +(tr0)
> > +])
> > +
> > +ovn_as az2
> > +OVS_WAIT_UNTIL([test 6 = `ovn-sbctl list Datapath_Binding | wc -l`])
> > +AT_CHECK([ovn-nbctl lr-list | cut -c 38-], [0], [dnl
> > +(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
> > +])
> >
>
> This test also doesn't check that the MAC and networks are
> populated correctly, it could be part of the check_row_count
> filter.
Ack, Ill add MAC and network checks
>
>
> > +
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ovn-ic -- Add transit router remote 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 az2
> > +check ovn-sbctl chassis-add chassis-az2 vxlan 192.168.0.2
> > +check ovn-nbctl --wait=sb sync
> > +
> > +ovn_as az1
> > +check ovn-sbctl chassis-add chassis-az2 vxlan 192.168.0.2 \
> > + -- set chassis chassis-az2 other_config:is-remote=true
> > +check ovn-nbctl --wait=sb sync
> > +
> > +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 -E '(tr0-p0|type)'], [0],
> > [dnl
> > +logical_port : tr0-p0
> > +type : patch
> > +])
> >
>
> This is not correct is it?
> The tr0-p0 is assigned to chassis-az2, so it should be
> type=remote on az1.
Ack, Yes, this should be remote
>
>
> > +
> > +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)'], [0],
> > [dnl
> > +logical_port : tr0-p0
> > +type : patch
> > +])
> > +
> > +OVN_CLEANUP_IC([az1], [az2])
> > +AT_CLEANUP
> > +])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ovn-ic -- Transit router delete port])
> >
>
> The deletion and creation could be part of a single test.
Ack, Ill merge cretion and deleation
>
>
> > +
> > +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
> > ])
> > diff --git a/utilities/ovn-ic-nbctl.c b/utilities/ovn-ic-nbctl.c
> > index 5819192fe..740c75bf1 100644
> > --- a/utilities/ovn-ic-nbctl.c
> > +++ b/utilities/ovn-ic-nbctl.c
> > @@ -337,6 +337,14 @@ Transit switch commands:\n\
> > ts-del SWITCH delete SWITCH\n\
> > ts-list print all transit switches\n\
> > \n\
> > +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\
> > del-connection delete the connections\n\
> > @@ -397,10 +405,20 @@ struct ic_nbctl_context {
> > static struct cmd_show_table cmd_show_tables[] = {
> > {&icnbrec_table_transit_switch,
> > &icnbrec_transit_switch_col_name,
> > - {NULL},
> > + {NULL, NULL, NULL},
> > + {NULL, NULL, NULL}},
> > +
> > + {&icnbrec_table_transit_router,
> > + &icnbrec_transit_router_col_name,
> > + {NULL, NULL, NULL},
> > + {NULL, NULL, NULL}},
> > +
> > + {&icnbrec_table_transit_router_port,
> > + &icnbrec_transit_router_port_col_name,
> > + {&icnbrec_transit_router_port_col_nb_ic_uuid, NULL, NULL},
> > {NULL, NULL, NULL}},
> >
> > - {NULL, NULL, {NULL, NULL, NULL}, {NULL, NULL, NULL}},
> > + {NULL, NULL, {NULL, NULL, NULL}, {NULL, NULL, NULL}}
> > };
> >
> > static void
> > @@ -507,6 +525,228 @@ ic_nbctl_ts_list(struct ctl_context *ctx)
> > smap_destroy(&switches);
> > free(nodes);
> > }
> > +
> > +static void
> > +ic_nbctl_tr_add(struct ctl_context *ctx)
> > +{
> > + if (!ovsdb_idl_server_has_table(ctx->idl,
> > &icnbrec_table_transit_router)) {
> > + VLOG_WARN("icnbrec_table_transit_router missing");
> > + }
> > +
> > + const char *tr_name = ctx->argv[1];
> > + bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > +
> > + const struct icnbrec_transit_router *tr;
> > + ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->idl) {
> > + if (!strcmp(tr->name, tr_name)) {
> > + if (may_exist) {
> > + return;
> > + }
> > +
> > + ctl_error(ctx, "%s: a transit router with this name already "
> > + "exists", tr_name);
> > + return;
> > + }
> > +
> > + }
> > + tr = icnbrec_transit_router_insert(ctx->txn);
> > +
> > + icnbrec_transit_router_set_name(tr, tr_name);
> > +}
> > +
> > +static char *
> > +tr_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool
> > must_exist,
> > + const struct icnbrec_transit_router **tr_p)
> > +{
> > + const struct icnbrec_transit_router *tr = NULL;
> > + *tr_p = NULL;
> > +
> > + struct uuid tr_uuid;
> > + bool is_uuid = uuid_from_string(&tr_uuid, id);
> > + if (is_uuid) {
> > + tr = icnbrec_transit_router_get_for_uuid(ctx->idl, &tr_uuid);
> > + }
> > +
> > + if (!tr) {
> > + const struct icnbrec_transit_router *iter;
> > +
> > + ICNBREC_TRANSIT_ROUTER_FOR_EACH (iter, ctx->idl) {
> > + if (!strcmp(iter->name, id)) {
> > + tr = iter;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (!tr && must_exist) {
> > + return xasprintf("%s: router %s not found",
> > + id, is_uuid ? "UUID" : "name");
> > + }
> > +
> > + *tr_p = tr;
> > + return NULL;
> > +}
> > +
> > +static void
> > +ic_nbctl_tr_del(struct ctl_context *ctx)
> > +{
> > + bool must_exist = !shash_find(&ctx->options, "--if-exists");
> > + const char *id = ctx->argv[1];
> > + const struct icnbrec_transit_router *tr = NULL;
> > +
> > + char *error = tr_by_name_or_uuid(ctx, id, must_exist, &tr);
> > + if (error) {
> > + ctx->error = error;
> > + return;
> > + }
> > +
> > + if (!tr) {
> > + return;
> > + }
> > +
> > + icnbrec_transit_router_delete(tr);
> > +}
> > +
> > +static void
> > +ic_nbctl_trp_del(struct ctl_context *ctx)
> > +{
> > + const char *port = ctx->argv[1];
> > + const struct icnbrec_transit_router_port *trp = NULL;
> > +
> > + const struct icnbrec_transit_router_port *iter;
> > +
> > + ICNBREC_TRANSIT_ROUTER_PORT_FOR_EACH (iter, ctx->idl) {
> > + if (!strcmp(iter->name, port)) {
> > + trp = iter;
> > + break;
> > + }
> > +
> > + }
> > +
> > + if (!trp) {
> > + ctx->error = xasprintf("%s: router port not found", port);
> > + return;
> > + }
> > +
> > + icnbrec_transit_router_port_delete(trp);
> > +}
> > +
> > +static void
> > +ic_nbctl_tr_list(struct ctl_context *ctx)
> > +{
> > + const struct icnbrec_transit_router *tr;
> > + struct smap routers;
> > +
> > + smap_init(&routers);
> > + ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->idl) {
> > + smap_add_format(&routers, tr->name, UUID_FMT " (%s)",
> > + UUID_ARGS(&tr->header_.uuid), tr->name);
> > + }
> > + const struct smap_node **nodes = smap_sort(&routers);
> > + for (size_t i = 0; i < smap_count(&routers); i++) {
> > + const struct smap_node *node = nodes[i];
> > + ds_put_format(&ctx->output, "%s\n", node->value);
> > + }
> > + smap_destroy(&routers);
> > + free(nodes);
> > +}
> > +
> > +static void
> > +ic_nbctl_trp_add(struct ctl_context *ctx)
> > +{
> >
>
> trp-add supports --may-exist option, but I don't
> see any check in the function.
Ack, include changes for the --may-exist check
>
>
> > + 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];
> > + bool found = false;
> > + const struct icnbrec_transit_router *tr;
> > + ICNBREC_TRANSIT_ROUTER_FOR_EACH (tr, ctx->idl) {
> > + if (!strcmp(tr->name, tr_name)) {
> > + found = true;
> > + break;
> > + }
> > +
> > + }
> >
>
> Please use "tr_by_name_or_uuid()".
Ack, Ill change to tr_by_name_or_uuid
>
> +
> > + if (!found) {
> > + ctl_error(ctx, "%s: transit router does not exist.", tr_name);
> > + return;
> > + }
> > +
> > + found = false;
> > + const struct icnbrec_transit_router_port *trp;
> > + ICNBREC_TRANSIT_ROUTER_PORT_FOR_EACH (trp, ctx->idl) {
> > + if (!strcmp(trp->name, trp_name)) {
> > + found = true;
> > + break;
> > + }
> > +
> > + }
> > +
> > + if (found) {
> > + ctl_error(ctx, "%s: a transit router port with this name already "
> > + "exists.", trp_name);
> > + return;
> > +
> > + }
> >
>
> It would be probably beneficial to also have "trp_by_name_or_uuid()".
> There is the same lookup for trp-del and it would also support
> specifying the uuid, not only the name.
>
Ack, Ill add a trp_by_name_or_uuid
> +
> > + struct eth_addr ea;
> > + if (!eth_addr_from_string(mac, &ea)) {
> > + ctl_error(ctx, "%s: MAC address is invalid.", mac);
> > + 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;
> > +
> > + for (int i = 0; i < n_networks; i++) {
> > + ovs_be32 ipv4;
> > + unsigned int plen;
> > + char *error;
> > + 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;
> > + }
> > +
> > + }
> > +
> > + }
> >
>
> Please take a look at nbctl_lrp_add, particularly the section
> with network parsing, we could utilize the same
> "lrp_network_sset()" function here too.
Ack, ill change to lrp_network_sset for parsing
>
> +
> > + 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);
> > + icnbrec_transit_router_port_set_networks(trp, networks, n_networks);
> > +
> > + for (int i = 0; i < n_settings; i++) {
> > + char *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)
> > {
> > @@ -714,6 +954,15 @@ cmd_set_ssl(struct ctl_context *ctx)
> > static const struct ctl_table_class tables[ICNBREC_N_TABLES] = {
> > [ICNBREC_TABLE_TRANSIT_SWITCH].row_ids[0] =
> > {&icnbrec_transit_switch_col_name, NULL, NULL},
> > +
> > + [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_nb_ic_uuid, NULL, NULL},
> > + {&icnbrec_transit_router_port_col_mac, NULL, NULL},
> > + {&icnbrec_transit_router_port_col_transit_router, NULL, NULL}},
> > };
> >
> >
> > @@ -1010,11 +1259,24 @@ ic_nbctl_exit(int status)
> > static const struct ctl_command_syntax ic_nbctl_commands[] = {
> > { "init", 0, 0, "", NULL, ic_nbctl_init, NULL, "", RW },
> > { "sync", 0, 0, "", ic_nbctl_pre_sync, NULL, NULL, "", RO },
> > +
> > /* transit switch commands. */
> > { "ts-add", 1, 1, "SWITCH", NULL, ic_nbctl_ts_add, NULL,
> > "--may-exist", RW },
> > { "ts-del", 1, 1, "SWITCH", NULL, ic_nbctl_ts_del, NULL,
> > "--if-exists", RW },
> > { "ts-list", 0, 0, "", NULL, ic_nbctl_ts_list, NULL, "", RO },
> >
> > + /* transit router commands. */
> > + { "tr-add", 1, 1, "ROUTER", NULL, ic_nbctl_tr_add, NULL,
> > "--may-exist",
> > + RW },
> > + { "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, "",
> > RO},
> > --
> > 2.50.0
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> I didn't check all logic in detail as quite a bit will probably
> change and it's quite hard to track what belongs where with
> a single patch.
>
> Thanks,
> Ales
Thank you for your review Ales, responses inline
Regards,
Mairtin
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
