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.


>  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.


>      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.


>  {
>      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.


> +        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.

+            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.


>      /* 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.


>          /* 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.

+        }
> +
> +        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.


>              } 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.


> +    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.


> +    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]"

+        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.


> +    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.


> +        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.

+
> +    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.


> +
> +
> +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'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.


> +            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,
> +                    &current_isb_pb->nb_ic_uuid,
> current_isb_pb->logical_port);
> +                ic_pb_free(pb_info);
> +                pb_info = ic_pb_create(current_isb_pb);
> +                ic_pb_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,
> +                    &current_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.


> +
> +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.


> +    "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.


> -    "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.

+                "nb_ic_uuid": {"type": {"key": {"type": "uuid"},
> +                                     "min": 1,
> +                                     "max": 1}},
>

Same here, the min should be 0.


>                  "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".


> +                "nb_ic_uuid": {"type": {"key": {"type": "uuid"},
> +                                     "min": 1,
> +                                     "max": 1}},
>

Same here with the min.


>                  "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.


>                  "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.


> 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.


> +
> +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
>

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.


> +
> +
> +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.


> +
> +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.


> +
> +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.


> +    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()".

+
> +    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.

+
> +    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.

+
> +    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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to