Hi Lorenzo, Thanks for your review. Please let me know if you have any further comments on the updated patch.
Thanks Sragdhara On 5/2/25, 2:35 PM, "Lorenzo Bianconi" <lorenzo.bianc...@redhat.com> wrote: > In an interconnect topology, when a logical router (LR) is connected to a > transit-switch (TS), the static routes on the LR as well as routes for the > connected subnets get advertised to the other sites via the IC-SB. > > This commit adds two new nb_global options ic-route-adv-lb and > ic-route-learn-lb, which will be help propagate the LB VIP based routes. > Default value for both the options is false. Also these options come into > effect only if the exisiting options ic-route-adv and ic-route-learn are > true. > > ic-route-adv-lb: If this is set to true, add routes in the IC-SB for the > VIPs associated with the LB instances linked to the logical router. These > routes would have the VIP as destination and IP of the LRP connected to TS > as next hop The origin would be set as "loadbalancer". > > ic-route-learn-lb: If this is set to true, routes with origin loadbalancer > are synced from the IC-SB and applied to the LR as learned route. > > This change makes services hotes behind LB in one site become accessible by > clients in other locations, via interconnect. Hi Sragdhara, The patch seems fine to me, just minor comments inline. Regards, Lorenzo > > Signed-off-by: Sragdhara Datta Chaudhuri > <sragdha.chau...@nutanix.com<mailto:sragdha.chau...@nutanix.com>> > --- > ic/ovn-ic.c | 203 ++++++++++++++++++++++++++++-- > lib/ovn-util.h | 1 + > ovn-ic-sb.ovsschema | 7 +- > ovn-ic-sb.xml | 10 +- > ovn-nb.xml | 21 +++- > tests/ovn-ic.at | 296 ++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 523 insertions(+), 15 deletions(-) > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index c8796680b..8183030c4 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -939,14 +939,17 @@ struct ic_route_info { > > const struct nbrec_logical_router *nb_lr; > > - /* Either nb_route or nb_lrp is set and the other one must be NULL. > + /* One of nb_route, nb_lrp, nb_lb is set and the other ones must be NULL. > * - For a route that is learned from IC-SB, or a static route that is > * generated from a route that is configured in NB, the "nb_route" > * is set. > * - For a route that is generated from a direct-connect subnet of > - * a logical router port, the "nb_lrp" is set. */ > + * a logical router port, the "nb_lrp" is set. > + * - For a route that is generated from a load-balancer vip of > + * a logical router, the "nb_lb" is set. */ > const struct nbrec_logical_router_static_route *nb_route; > const struct nbrec_logical_router_port *nb_lrp; > + const struct nbrec_load_balancer *nb_lb; > }; > > static uint32_t > @@ -1163,9 +1166,10 @@ add_to_routes_ad(struct hmap *routes_ad, const struct > in6_addr prefix, > const struct nbrec_logical_router_port *nb_lrp, > const struct nbrec_logical_router_static_route *nb_route, > const struct nbrec_logical_router *nb_lr, > + const struct nbrec_load_balancer *nb_lb, > const char *route_tag) > { > - ovs_assert(nb_route || nb_lrp); > + ovs_assert(nb_route || nb_lrp || nb_lb); > > if (route_table == NULL) { > route_table = ""; > @@ -1184,6 +1188,7 @@ add_to_routes_ad(struct hmap *routes_ad, const struct > in6_addr prefix, > ic_route->route_table = route_table; > ic_route->nb_lrp = nb_lrp; > ic_route->nb_lr = nb_lr; > + ic_route->nb_lb = nb_lb; > ic_route->route_tag = route_tag; > hmap_insert(routes_ad, &ic_route->node, hash); > } else { > @@ -1193,6 +1198,9 @@ add_to_routes_ad(struct hmap *routes_ad, const struct > in6_addr prefix, > if (nb_route) { > VLOG_WARN_RL(&rl, msg_fmt, origin, "route", > UUID_ARGS(&nb_route->header_.uuid)); > + } else if (nb_lb) { > + VLOG_WARN_RL(&rl, msg_fmt, origin, "loadbalancer", > + UUID_ARGS(&nb_lb->header_.uuid)); > } else { > VLOG_WARN_RL(&rl, msg_fmt, origin, "lrp", > UUID_ARGS(&nb_lrp->header_.uuid)); > @@ -1248,7 +1256,8 @@ add_static_to_routes_ad( > } > > add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_STATIC, > - nb_route->route_table, NULL, nb_route, nb_lr, > route_tag); > + nb_route->route_table, NULL, nb_route, nb_lr, > + NULL, route_tag); > } > > static void > @@ -1296,7 +1305,68 @@ add_network_to_routes_ad(struct hmap *routes_ad, const > char *network, > > /* directly-connected routes go to <main> route table */ > add_to_routes_ad(routes_ad, prefix, plen, nexthop, > ROUTE_ORIGIN_CONNECTED, > - NULL, nb_lrp, NULL, nb_lr, route_tag); > + NULL, nb_lrp, NULL, nb_lr, NULL, route_tag); > +} > + > +static void > +add_lb_vip_to_routes_ad( > + struct hmap *routes_ad, > + const char *vip_key, > + const struct nbrec_load_balancer *nb_lb, > + const struct lport_addresses *nexthop_addresses, > + const struct smap *nb_options, > + const struct nbrec_logical_router *nb_lr, > + const char *route_tag) maybe something like: static void add_lb_vip_to_routes_ad(struct hmap *routes_ad, const char *vip_key, const struct nbrec_load_balancer *nb_lb, ... const char *route_tag) > +{ > + char *vip_str = NULL; > + struct in6_addr vip_ip, nexthop; > + uint16_t vip_port; > + int addr_family; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + > + if (!ip_address_and_port_from_lb_key(vip_key, &vip_str, &vip_ip, > + &vip_port, &addr_family)) { > + VLOG_WARN_RL(&rl, "Route ad: Parsing failed for lb vip %s", vip_key); > + return; > + } > + if (vip_str == NULL) { > + return; > + } > + unsigned int plen = (addr_family == AF_INET) ? 32 : 128; > + if (!route_need_advertise(NULL, &vip_ip, plen, nb_options)) { > + VLOG_DBG("Route ad: skip lb vip %s.", vip_key); > + free(vip_str); since you are freeing vip_str at the end of the routine I guess you can just add a 'goto out' here. > + return; > + } > + if (!get_nexthop_from_lport_addresses(IN6_IS_ADDR_V4MAPPED(&vip_ip), > + nexthop_addresses, > + &nexthop)) { > + VLOG_WARN_RL(&rl, "Route ad: failed to get nexthop for lb vip"); > + free(vip_str); ditto. > + return; > + } > + > + if (VLOG_IS_DBG_ENABLED()) { > + struct ds msg = DS_EMPTY_INITIALIZER; > + > + ds_put_format(&msg, "Adding lb vip route to <main> routing " > + "table: %s, nexthop ", vip_str); > + > + if (IN6_IS_ADDR_V4MAPPED(&nexthop)) { > + ds_put_format(&msg, IP_FMT, > + IP_ARGS(in6_addr_get_mapped_ipv4(&nexthop))); > + } else { > + ipv6_format_addr(&nexthop, &msg); > + } > + > + VLOG_DBG("%s", ds_cstr(&msg)); > + ds_destroy(&msg); > + } > + > + /* Lb vip routes go to <main> route table */ > + add_to_routes_ad(routes_ad, vip_ip, plen, nexthop, ROUTE_ORIGIN_LB, > + NULL, NULL, NULL, nb_lr, nb_lb, route_tag); out: > + free(vip_str); > } > > static bool > @@ -1315,6 +1385,44 @@ route_has_local_gw(const struct nbrec_logical_router > *lr, > return false; > } > > +static bool > +route_matches_local_lb(const struct nbrec_load_balancer *nb_lb, > + const char *ip_prefix) > +{ > + char *vip_str = NULL; > + struct in6_addr vip_ip; > + uint16_t vip_port; > + int addr_family; > + unsigned int plen; > + char vip_cidr[50]; > + struct smap_node *node; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + > + SMAP_FOR_EACH (node, &nb_lb->vips) { > + if (node->key) { > + if (ip_address_and_port_from_lb_key(node->key, &vip_str, > + &vip_ip, &vip_port, > + &addr_family)) { > + if (vip_str) { > + plen = (addr_family == AF_INET) ? 32 : 128; > + snprintf(vip_cidr, 50, "%s/%d", vip_str, plen); I guess it is better to do sizeof(vip_cidr) here. > + if (!strcmp(vip_cidr, ip_prefix)) { > + free(vip_str); > + return true; > + } > + } > + } else { > + VLOG_WARN_RL( > + &rl, > + "Route learn: Parsing failed for local lb vip %s", > + node->key); > + } > + } > + } > + free(vip_str); > + return false; > +} > + > static bool > route_need_learn(const struct nbrec_logical_router *lr, > const struct icsbrec_route *isb_route, > @@ -1330,6 +1438,11 @@ route_need_learn(const struct nbrec_logical_router *lr, > return false; > } > > + if (!strcmp(isb_route->origin, ROUTE_ORIGIN_LB) && > + !smap_get_bool(nb_options, "ic-route-learn-lb", false)) { > + return false; > + } > + > if (prefix_is_link_local(prefix, plen)) { > return false; > } > @@ -1344,6 +1457,29 @@ route_need_learn(const struct nbrec_logical_router *lr, > return false; > } > > + for (int i = 0; i < lr->n_load_balancer; i++) { > + if (route_matches_local_lb(lr->load_balancer[i], > + isb_route->ip_prefix)) { > + VLOG_DBG("Skip learning %s (rtb:%s) route, as we've got local" > + " LB with matching VIP", isb_route->ip_prefix, > + isb_route->route_table); > + return false; > + } > + } > + for (int i = 0; i < lr->n_load_balancer_group; i++) { > + const struct nbrec_load_balancer_group *nb_lbg; > + nb_lbg = lr->load_balancer_group[i]; > + for (int j = 0; j < nb_lbg->n_load_balancer; j++) { > + if (route_matches_local_lb(nb_lbg->load_balancer[j], > + isb_route->ip_prefix)) { > + VLOG_DBG("Skip learning %s (rtb:%s) route, as we've got > local" > + " LB with matching VIP", isb_route->ip_prefix, > + isb_route->route_table); > + return false; > + } > + } > + } > + > return true; > } > > @@ -1535,8 +1671,13 @@ ad_route_sync_external_ids(const struct ic_route_info > *route_adv, > const char *route_tag; > smap_get_uuid(&isb_route->external_ids, "nb-id", &isb_ext_id); > smap_get_uuid(&isb_route->external_ids, "lr-id", &isb_ext_lr_id); > - nb_id = route_adv->nb_route ? route_adv->nb_route->header_.uuid > - : route_adv->nb_lrp->header_.uuid; > + if (route_adv->nb_lb) { > + nb_id = route_adv->nb_lb->header_.uuid; > + } else { > + nb_id = route_adv->nb_route ? route_adv->nb_route->header_.uuid > + : route_adv->nb_lrp->header_.uuid; > + } nit: I guess you can do something like: nb_id = route_adv->nb_lb ? route_adv->nb_lb->header_.uuid : route_adv->nb_route ? route_adv->nb_route->header_.uuid : route_adv->nb_lrp->header_.uuid; > + > lr_id = route_adv->nb_lr->header_.uuid; > if (!uuid_equals(&isb_ext_id, &nb_id)) { > char *uuid_s = xasprintf(UUID_FMT, UUID_ARGS(&nb_id)); > @@ -1696,6 +1837,40 @@ build_ts_routes_to_adv(struct ic_context *ctx, > lrp->name); > } > } > + > + /* Check loadbalancers associated with the LR */ > + if (smap_get_bool(&nb_global->options, "ic-route-adv-lb", false)) { > + for (int i = 0; i < lr->n_load_balancer; i++) { > + const struct nbrec_load_balancer *nb_lb = lr->load_balancer[i]; > + struct smap_node *node; > + SMAP_FOR_EACH (node, &nb_lb->vips) { > + if (node->key) { > + add_lb_vip_to_routes_ad(routes_ad, node->key, nb_lb, > + ts_port_addrs, > + &nb_global->options, > + lr, route_tag); > + } > + } > + } > + > + for (int i = 0; i < lr->n_load_balancer_group; i++) { > + const struct nbrec_load_balancer_group *nb_lbg; > + nb_lbg = lr->load_balancer_group[i]; > + for (int j = 0; j < nb_lbg->n_load_balancer; j++) { > + const struct nbrec_load_balancer *nb_lb; > + nb_lb = nb_lbg->load_balancer[j]; > + struct smap_node *node; > + SMAP_FOR_EACH (node, &nb_lb->vips) { > + if (node->key) { > + add_lb_vip_to_routes_ad(routes_ad, node->key, nb_lb, > + ts_port_addrs, > + &nb_global->options, > + lr, route_tag); > + } > + } > + } > + } > + } > } > > static void > @@ -2213,6 +2388,10 @@ main(int argc, char *argv[]) > &nbrec_logical_router_col_options); > ovsdb_idl_add_column(ovnnb_idl_loop.idl, > &nbrec_logical_router_col_external_ids); > + ovsdb_idl_add_column(ovnnb_idl_loop.idl, > + &nbrec_logical_router_col_load_balancer); > + ovsdb_idl_add_column(ovnnb_idl_loop.idl, > + &nbrec_logical_router_col_load_balancer_group); > > ovsdb_idl_add_table(ovnnb_idl_loop.idl, > &nbrec_table_logical_router_port); > ovsdb_idl_add_column(ovnnb_idl_loop.idl, > @@ -2252,6 +2431,16 @@ main(int argc, char *argv[]) > ovsdb_idl_add_column(ovnnb_idl_loop.idl, > &nbrec_logical_switch_port_col_external_ids); > > + ovsdb_idl_add_table(ovnnb_idl_loop.idl, > + &nbrec_table_load_balancer); > + ovsdb_idl_add_column(ovnnb_idl_loop.idl, > + &nbrec_load_balancer_col_vips); > + > + ovsdb_idl_add_table(ovnnb_idl_loop.idl, > + &nbrec_table_load_balancer_group); > + ovsdb_idl_add_column(ovnnb_idl_loop.idl, > + &nbrec_load_balancer_group_col_load_balancer); > + > /* ovn-sb db. */ > struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER( > ovsdb_idl_create(ovnsb_db, &sbrec_idl_class, false, true)); > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index 0fff9b463..5db7cb8d4 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -30,6 +30,7 @@ > > #define ROUTE_ORIGIN_CONNECTED "connected" > #define ROUTE_ORIGIN_STATIC "static" > +#define ROUTE_ORIGIN_LB "loadbalancer" > > #define ETH_CRC_LENGTH 4 > #define ETHERNET_OVERHEAD (ETH_HEADER_LEN + ETH_CRC_LENGTH) > diff --git a/ovn-ic-sb.ovsschema b/ovn-ic-sb.ovsschema > index 459c3833e..e92cf8f58 100644 > --- a/ovn-ic-sb.ovsschema > +++ b/ovn-ic-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_IC_Southbound", > - "version": "2.0.0", [...] > +]) > + > +ovn_as az2 ovn-nbctl lb-add lb_temp 200.1.3.55:80 1.1.1.1:80,1.1.1.2:80 > +ovn_as az2 ovn-nbctl lr-lb-add lr2 lb_temp > +OVS_WAIT_WHILE([ovn_as az2 ovn-nbctl lr-route-list lr2 | grep learned | grep > 200.1.3.55]) > +AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr2], [0], [dnl > +IPv4 Routes > +Route Table <main>: > + 200.1.1.50 169.254.100.1 dst-ip (learned) > + 200.1.1.53 169.254.100.1 dst-ip (learned) > +]) > + > +ovn_as az2 ovn-nbctl lr-lb-del lr2 lb_temp > +ovn_as az2 ovn-nbctl lb-del lb_temp > +OVS_WAIT_UNTIL([ovn_as az2 ovn-nbctl lr-route-list lr2 | grep learned | grep > 200.1.3.55]) > +AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr2], [0], [dnl > +IPv4 Routes > +Route Table <main>: > + 200.1.1.50 169.254.100.1 dst-ip (learned) > + 200.1.1.53 169.254.100.1 dst-ip (learned) > + 200.1.3.55 169.254.100.1 dst-ip (learned) > +]) > +ovn_as az1 ovn-nbctl lr-route-del lr1 200.1.3.55 169.1.1.1 > +OVS_WAIT_WHILE([ovn_as az2 ovn-nbctl lr-route-list lr2 | grep learned | grep > 200.1.3.55]) > +AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr2], [0], [dnl > +IPv4 Routes > +Route Table <main>: > + 200.1.1.50 169.254.100.1 dst-ip (learned) > + 200.1.1.53 169.254.100.1 dst-ip (learned) > +]) > + > +# Create a third az > +i=3 > +ovn_start az$i > +ovn_as az$i I guess you can use '3' directly here, it is more readable. > +check ovn-ic-nbctl --wait=sb sync > +check ovn-nbctl set nb_global . options:ic-route-learn=true > +check ovn-nbctl set nb_global . options:ic-route-adv=true > + > +check ovn-nbctl set nb_global . options:ic-route-adv-lb=true > +check ovn-nbctl set nb_global . options:ic-route-learn-lb=true > + > +check ovn-nbctl lr-add lr$i > +check ovn-nbctl lrp-add lr$i lrp-lr$i-ts1 aa:aa:aa:aa:aa:0$i > 169.254.100.$i/24 2001:db8:1::$i/64 > +check ovn-nbctl lsp-add ts1 lsp-ts1-lr$i \ > + -- lsp-set-addresses lsp-ts1-lr$i router \ > + -- lsp-set-type lsp-ts1-lr$i router \ > + -- lsp-set-options lsp-ts1-lr$i router-port=lrp-lr$i-ts1 > + > +# Create LB in 3rd az and check that the route propagates to az1 LR > +ovn_as az3 ovn-nbctl lb-add lb3 200.1.3.50:80 1.1.3.1:80,1.1.3.2:80 > +ovn_as az3 ovn-nbctl lr-lb-add lr3 lb3 > +OVS_WAIT_UNTIL([ovn_as az2 ovn-nbctl lr-route-list lr2 | grep learned | grep > 200.1.3.50]) > +AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr2], [0], [dnl > +IPv4 Routes > +Route Table <main>: > + 200.1.1.50 169.254.100.1 dst-ip (learned) > + 200.1.1.53 169.254.100.1 dst-ip (learned) > + 200.1.3.50 169.254.100.3 dst-ip (learned) > +]) > + > +# Create LB in az3 with same VIP as az1, check for ECMP route in az2 LR > +ovn_as az3 ovn-nbctl lb-add lb4 200.1.1.50:80 1.1.3.1:80,1.1.3.2:80 > +ovn_as az3 ovn-nbctl lr-lb-add lr3 lb4 > +OVS_WAIT_UNTIL([ovn_as az2 ovn-nbctl lr-route-list lr2 | grep learned | grep > ecmp]) > +AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr2], [0], [dnl > +IPv4 Routes > +Route Table <main>: > + 200.1.1.50 169.254.100.1 dst-ip (learned) ecmp > + 200.1.1.50 169.254.100.3 dst-ip (learned) ecmp > + 200.1.1.53 169.254.100.1 dst-ip (learned) > + 200.1.3.50 169.254.100.3 dst-ip (learned) > +]) > + > +OVN_CLEANUP_IC([az1], [az2]) > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([ovn-ic -- route sync -- IPv6 route tables]) > AT_KEYWORDS([IPv6-route-sync]) > -- > 2.39.3 > > _______________________________________________ > dev mailing list > d...@openvswitch.org<mailto:d...@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
