> Hi, > > Thanks for the review. > > Em qui., 11 de set. de 2025 às 12:23, Lorenzo Bianconi < > [email protected]> escreveu: > > > > Logical router adv in ovn-ic the sb learned routes from > > > dynamic routing. Also, it doesn't learn route in ovn-ic > > > if prefix is already learned in sb learned route table. > > > > Hi Lucas, > > > > I think the patch is fine, just few nit inline. > > > > Regards, > > Lorenzo > > > > > > > > Signed-off-by: Lucas Vargas Dias <[email protected]> > > > --- > > > ic/ovn-ic.c | 102 +++++++++++++++++++++++++++++++++---- > > > tests/ovn-ic.at | 130 ++++++++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 223 insertions(+), 9 deletions(-) > > > > > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > > > index d1a59d372..5bf576eac 100644 > > > --- a/ic/ovn-ic.c > > > +++ b/ic/ovn-ic.c > > > @@ -70,6 +70,7 @@ struct ic_context { > > > struct ovsdb_idl_index *nbrec_port_by_name; > > > struct ovsdb_idl_index *sbrec_chassis_by_name; > > > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > > + struct ovsdb_idl_index *sbrec_datapath_binding_by_nb_uuid; > > > struct ovsdb_idl_index *sbrec_service_monitor_by_remote_type; > > > struct ovsdb_idl_index *sbrec_service_monitor_by_ic_learned; > > > struct ovsdb_idl_index > > *sbrec_service_monitor_by_remote_type_logical_port; > > > @@ -513,6 +514,21 @@ find_sb_pb_by_name(struct ovsdb_idl_index > > *sbrec_port_binding_by_name, > > > return pb; > > > } > > > > > > +static const struct sbrec_datapath_binding * > > > +find_dp_by_uuid(struct ovsdb_idl_index *sbrec_datapath_binding, > > > + const struct uuid *nb_uuid) > > > +{ > > > + const struct sbrec_datapath_binding *key = > > > + sbrec_datapath_binding_index_init_row(sbrec_datapath_binding); > > > + sbrec_datapath_binding_set_nb_uuid(key, nb_uuid, 1); > > > + > > > + const struct sbrec_datapath_binding *dp = > > > + sbrec_datapath_binding_index_find(sbrec_datapath_binding, key); > > > + sbrec_datapath_binding_index_destroy_row(key); > > > + > > > + return dp; > > > +} > > > + > > > static const struct sbrec_port_binding * > > > find_peer_port(struct ic_context *ctx, > > > const struct sbrec_port_binding *sb_pb) > > > @@ -1191,7 +1207,7 @@ add_to_routes_ad(struct hmap *routes_ad, const > > struct in6_addr prefix, > > > const struct nbrec_load_balancer *nb_lb, > > > const char *route_tag) > > > { > > > - ovs_assert(nb_route || nb_lrp || nb_lb); > > > + ovs_assert(nb_route || nb_lrp || nb_lb || nb_lr); > > > > > > if (route_table == NULL) { > > > route_table = ""; > > > @@ -1223,9 +1239,12 @@ add_to_routes_ad(struct hmap *routes_ad, const > > struct in6_addr prefix, > > > } else if (nb_lb) { > > > VLOG_WARN_RL(&rl, msg_fmt, origin, "loadbalancer", > > > UUID_ARGS(&nb_lb->header_.uuid)); > > > - } else { > > > + } else if (nb_lrp){ > > > VLOG_WARN_RL(&rl, msg_fmt, origin, "lrp", > > > UUID_ARGS(&nb_lrp->header_.uuid)); > > > + } else { > > > + VLOG_WARN_RL(&rl, msg_fmt, origin, "route", > > > + UUID_ARGS(&nb_lr->header_.uuid)); > > > } > > > } > > > } > > > @@ -1301,8 +1320,16 @@ add_network_to_routes_ad(struct hmap *routes_ad, > > const char *network, > > > > > > if (!route_need_advertise(NULL, &prefix, plen, nb_options, > > > nb_lr, ts_lrp)) { > > > - VLOG_DBG("Route ad: skip network %s of lrp %s.", > > > - network, nb_lrp->name); > > > + if (VLOG_IS_DBG_ENABLED()) { > > > + struct ds msg = DS_EMPTY_INITIALIZER; > > > + ds_put_format(&msg, "Route ad: skip network %s", network); > > > + if (nb_lrp) { > > > + ds_put_format(&msg, " of lrp %s", nb_lrp->name); > > > + } > > > + ds_put_format(&msg, "."); > > > + VLOG_DBG("%s", ds_cstr(&msg)); > > > + ds_destroy(&msg); > > > + } > > > return; > > > } > > > > > > @@ -1316,8 +1343,12 @@ add_network_to_routes_ad(struct hmap *routes_ad, > > const char *network, > > > struct ds msg = DS_EMPTY_INITIALIZER; > > > > > > ds_put_format(&msg, "Adding direct network route to <main> > > routing " > > > - "table: %s of lrp %s, nexthop ", network, > > nb_lrp->name); > > > + "table: %s", network); > > > > > > + if (nb_lrp) { > > > + ds_put_format(&msg, " of lrp %s,", nb_lrp->name); > > > + } > > > + ds_put_format(&msg, " nexthop "); > > > if (IN6_IS_ADDR_V4MAPPED(&nexthop)) { > > > ds_put_format(&msg, IP_FMT, > > > IP_ARGS(in6_addr_get_mapped_ipv4(&nexthop))); > > > @@ -1498,7 +1529,8 @@ route_matches_local_lb(const struct > > nbrec_load_balancer *nb_lb, > > > } > > > > > > static bool > > > -route_need_learn(const struct nbrec_logical_router *lr, > > > +route_need_learn(struct ic_context *ctx, > > > + const struct nbrec_logical_router *lr, > > > const struct icsbrec_route *isb_route, > > > struct in6_addr *prefix, unsigned int plen, > > > const struct smap *nb_options, > > > @@ -1564,6 +1596,23 @@ route_need_learn(const struct > > nbrec_logical_router *lr, > > > } > > > } > > > > > > + const struct sbrec_datapath_binding *dp = > > > + find_dp_by_uuid(ctx->sbrec_datapath_binding_by_nb_uuid, > > > + &lr->header_.uuid); > > > + if (!dp) { > > > + return true; > > > + } > > > + const struct sbrec_learned_route_table *sbrec_learned_route_table = > > > + > > sbrec_learned_route_table_get(ctx->ovnsb_idl); > > > + const struct sbrec_learned_route *sb_route; > > > > Maybe we can try to use an index here to speed-up the lookup? Not sure if > > it > > is worth doing so. What do you think? > > > > I didn't understand what type of lookup you mention. Maybe you have more > experience than I do, > but I didn't find an example that searches for an entry in sbrec and > returns a list of entries. > > If there exists a way to find sbrec_learned_routes by datapath, it will be > possible to speed-up the lookup.
Hi Lucas, sorry for the late reply. What I mean here is you can create an index (something similar to sbrec_learned_route_index_by_datapath index in ovn-controller [0]) in order to speed-up the lookup of learned routes for a specific datapath. Regards, Lorenzo [0] https://github.com/ovn-org/ovn/blob/main/controller/ovn-controller.c#L6700 > > Best, > Lucas > > > > > > > + SBREC_LEARNED_ROUTE_TABLE_FOR_EACH_SAFE (sb_route, > > > + sbrec_learned_route_table) > > { > > > + if (uuid_equals(&sb_route->datapath->header_.uuid, > > &dp->header_.uuid) > > > + && !strcmp(isb_route->ip_prefix, sb_route->ip_prefix)) { > > > + return false; > > > + } > > > + } > > > + > > > return true; > > > } > > > > > > @@ -1686,7 +1735,7 @@ sync_learned_routes(struct ic_context *ctx, > > > const struct nbrec_logical_router_port *lrp; > > > VECTOR_FOR_EACH (&ic_lr->isb_pbs, isb_pb) { > > > lrp_name = get_lrp_name_by_ts_port_name(ctx, > > isb_pb->logical_port); > > > - lrp = get_lrp_by_lrp_name(ctx, lrp_name); > > > + lrp = lrp_name ? get_lrp_by_lrp_name(ctx, lrp_name) : NULL; > > > if (lrp) { > > > ts_route_table = smap_get_def(&lrp->options, "route_table", > > ""); > > > route_filter_tag = smap_get_def(&lrp->options, > > > @@ -1745,7 +1794,7 @@ sync_learned_routes(struct ic_context *ctx, > > > isb_route->nexthop); > > > continue; > > > } > > > - if (!route_need_learn(ic_lr->lr, isb_route, &prefix, plen, > > > + if (!route_need_learn(ctx, ic_lr->lr, isb_route, &prefix, > > plen, > > > &nb_global->options, lrp, &nexthop)) { > > > continue; > > > } > > > @@ -1816,7 +1865,8 @@ ad_route_sync_external_ids(const struct > > ic_route_info *route_adv, > > > smap_get_uuid(&isb_route->external_ids, "lr-id", &isb_ext_lr_id); > > > 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; > > > + route_adv->nb_lrp ? route_adv->nb_lrp->header_.uuid : > > > + route_adv->nb_lr->header_.uuid; > > > > > > lr_id = route_adv->nb_lr->header_.uuid; > > > if (!uuid_equals(&isb_ext_id, &nb_id)) { > > > @@ -2008,6 +2058,26 @@ build_ts_routes_to_adv(struct ic_context *ctx, > > > } > > > } > > > } > > > + > > > + const struct sbrec_datapath_binding *dp = > > > + find_dp_by_uuid(ctx->sbrec_datapath_binding_by_nb_uuid, > > > + &lr->header_.uuid); > > > + if (!dp) { > > > + return; > > > + } > > > + const struct sbrec_learned_route_table *sbrec_learned_route_table = > > > + > > sbrec_learned_route_table_get(ctx->ovnsb_idl); > > > + const struct sbrec_learned_route *sb_route; > > > + SBREC_LEARNED_ROUTE_TABLE_FOR_EACH_SAFE (sb_route, > > > + sbrec_learned_route_table) > > { > > > + if (uuid_equals(&sb_route->datapath->header_.uuid, > > > + &dp->header_.uuid)) { > > > + add_network_to_routes_ad(routes_ad, sb_route->ip_prefix, > > NULL, > > > + ts_port_addrs, > > > + &nb_global->options, > > > + lr, route_tag, ts_lrp); > > > + } > > > + } > > > } > > > > > > static void > > > @@ -3067,6 +3137,15 @@ main(int argc, char *argv[]) > > > ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > > &sbrec_service_monitor_col_options); > > > > > > + ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_learned_route); > > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > > + &sbrec_learned_route_col_nexthop); > > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > > + &sbrec_learned_route_col_ip_prefix); > > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > > + &sbrec_learned_route_col_logical_port); > > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > > + &sbrec_learned_route_col_datapath); > > > /* Create IDL indexes */ > > > struct ovsdb_idl_index *nbrec_ls_by_name > > > = ovsdb_idl_index_create1(ovnnb_idl_loop.idl, > > > @@ -3080,6 +3159,9 @@ 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_datapath_binding_by_nb_uuid > > > + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, > > > + &sbrec_datapath_binding_col_nb_uuid); > > > struct ovsdb_idl_index *sbrec_chassis_by_name > > > = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, > > > &sbrec_chassis_col_name); > > > @@ -3190,6 +3272,8 @@ main(int argc, char *argv[]) > > > .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, > > > + .sbrec_datapath_binding_by_nb_uuid = > > > + sbrec_datapath_binding_by_nb_uuid, > > > .sbrec_chassis_by_name = sbrec_chassis_by_name, > > > .sbrec_service_monitor_by_remote_type = > > > sbrec_service_monitor_by_remote_type, > > > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > > > index 368b6701b..cf280ea7b 100644 > > > --- a/tests/ovn-ic.at > > > +++ b/tests/ovn-ic.at > > > @@ -4047,3 +4047,133 @@ OVN_CLEANUP_IC([az1], [az2], [az3]) > > > AT_CLEANUP > > > ]) > > > > > > +OVN_FOR_EACH_NORTHD([ > > > +AT_SETUP([ovn-ic -- Check ovn-ic adv and learn from SB Learned Route]) > > > + > > > +ovn_init_ic_db > > > + > > > +for i in 1 2; do > > > + ovn_start az$i > > > + ovn_as az$i > > > + > > > + # Enable route learning at AZ level > > > + check ovn-nbctl set nb_global . options:ic-route-learn=true > > > + # Enable route advertising at AZ level > > > + check ovn-nbctl set nb_global . options:ic-route-adv=true > > > +done > > > + > > > +# Create new transit switches and LRs. Test topology is next: > > > +# > > > +# > > > > I guess you can drop a new-line here. > > > > > +# logical router (lr11) - transit switch (ts11) - logical router (lr12) > > > +# > > > +# > > > > same here. > > > > > + > > > +# Create lr11, lr12 and ts11 and connect them > > > +for i in 1 2; do > > > + ovn_as az$i > > > + > > > + lr=lr1$i > > > + check ovn-nbctl lr-add $lr > > > + > > > + for j in 1; do > > > > you can drop the for here. > > > > > + ts=ts1$j > > > + check ovn-ic-nbctl --wait=sb --may-exist ts-add $ts > > > + > > > + lrp=lrp-$lr-$ts > > > + lsp=lsp-$ts-$lr > > > + # Create LRP and connect to TS > > > + check ovn-nbctl lrp-add $lr $lrp aa:aa:aa:aa:a$j:0$i > > 169.254.10$j.$i/24 > > > + check ovn-nbctl lsp-add $ts $lsp \ > > > + -- lsp-set-addresses $lsp router \ > > > + -- lsp-set-type $lsp router \ > > > + -- lsp-set-options $lsp router-port=$lrp > > > + done > > > +done > > > + > > > +# Create directly-connected route in lr11 > > > +check ovn_as az2 ovn-nbctl lrp-add lr12 lrp-lr12 aa:aa:aa:aa:bb:01 " > > 192.168.0.1/24" > > > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep > > 192.168 | > > > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > > > +192.168.0.0/24 169.254.101.2 > > > +]) > > > + > > > +ovn_as az2 > > > +check ovn-nbctl --wait=sb set Logical_Router lr12 > > option:dynamic-routing=true \ > > > + option:dynamic-routing-redistribute="connected,static" > > > +check ovn_as az2 ovn-nbctl --wait=sb lrp-add lr12 lr12-dr1 > > 00:00:00:00:ff:01 10.0.0.1/24 > > > +dr1=$(fetch_column port_binding _uuid logical_port=lr12-dr1) > > > +datapath=$(fetch_column datapath_binding _uuid external_ids:name=lr12) > > > + > > > +check_uuid ovn-sbctl create Learned_Route \ > > > + datapath=$datapath \ > > > + logical_port=$dr1 \ > > > + ip_prefix=192.168.1.0/24 \ > > > + nexthop=10.0.0.20 > > > + > > > +# Check Learned_Route adv in ovn-ic > > > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep > > 192.168 | > > > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > > > +192.168.0.0/24 169.254.101.2 > > > +192.168.1.0/24 169.254.101.2 > > > +]) > > > + > > > +ovn_as az2 > > > +learned=$(fetch_column Learned_Route _uuid ip_prefix=192.168.1.0/24) > > > +check ovn-sbctl destroy Learned_Route $learned > > > + > > > +# Check Learned_Route removed is not adv and learned. > > > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep > > 192.168 | > > > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > > > +192.168.0.0/24 169.254.101.2 > > > +]) > > > + > > > > drop new-line here. > > > > > + > > > +# Duplicate routes, SB learn route by two paths. > > > +ovn_as az2 > > > +check_uuid ovn-sbctl create Learned_Route \ > > > + datapath=$datapath \ > > > + logical_port=$dr1 \ > > > + ip_prefix=192.168.1.0/24 \ > > > + nexthop=10.0.0.20 > > > + > > > +check ovn-nbctl --wait=sb lrp-add lr12 lr12-dr2 00:00:00:00:ff:02 > > 169.254.254.1/24 > > > +dr2=$(fetch_column port_binding _uuid logical_port=lr12-dr2) > > > + > > > +check_uuid ovn-sbctl create Learned_Route \ > > > + datapath=$datapath \ > > > + logical_port=$dr2 \ > > > + ip_prefix=192.168.1.0/24 \ > > > + nexthop=169.254.254.3 > > > + > > > + > > > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep > > 192.168 | > > > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > > > +192.168.0.0/24 169.254.101.2 > > > +192.168.1.0/24 169.254.101.2 > > > +]) > > > + > > > +# Insert the same route Learned Route in the lr11 and don't learn by > > ovn-ic. > > > +ovn_as az1 > > > +datapath_lr11=$(fetch_column datapath_binding _uuid > > external_ids:name=lr11) > > > +check ovn-nbctl --wait=sb lrp-add lr11 lr11-dr1 00:00:00:00:ff:03 > > 169.254.254.2/24 > > > +sw1_dr1=$(fetch_column port_binding _uuid logical_port=lr11-dr1) > > > +check_uuid ovn-sbctl create Learned_Route \ > > > + datapath=$datapath_lr11 \ > > > + logical_port=$sw1_dr1 \ > > > + ip_prefix=192.168.1.0/24 \ > > > + nexthop=169.254.254.3 > > > + > > > +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep > > 192.168 | > > > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > > > +192.168.0.0/24 169.254.101.2 > > > +]) > > > > I guess you can check we are using the 'local' route here. > > > > > + > > > +OVS_WAIT_FOR_OUTPUT([ovn_as az2 ovn-nbctl lr-route-list lr12 | grep > > 192.168 | > > > + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl > > > +]) > > > + > > > +OVN_CLEANUP_IC([az1], [az2]) > > > + > > > +AT_CLEANUP > > > +]) > > > -- > > > 2.34.1 > > > > > > > > > -- > > > > > > > > > > > > > > > _'Esta mensagem é direcionada apenas para os endereços constantes no > > > cabeçalho inicial. Se você não está listado nos endereços constantes no > > > cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa > > > mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas > > estão > > > imediatamente anuladas e proibidas'._ > > > > > > > > > * **'Apesar do Magazine Luiza tomar > > > todas as precauções razoáveis para assegurar que nenhum vírus esteja > > > presente nesse e-mail, a empresa não poderá aceitar a responsabilidade > > por > > > quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.* > > > > > > > > > > > > _______________________________________________ > > > dev mailing list > > > [email protected] > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > -- > > > > > _‘Esta mensagem é direcionada apenas para os endereços constantes no > cabeçalho inicial. Se você não está listado nos endereços constantes no > cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa > mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão > imediatamente anuladas e proibidas’._ > > > * **‘Apesar do Magazine Luiza tomar > todas as precauções razoáveis para assegurar que nenhum vírus esteja > presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por > quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.* > > >
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
