Hi Dumitru, Thanks for review Em qui., 20 de mar. de 2025 às 10:00, Dumitru Ceara <dce...@redhat.com> escreveu:
> On 3/7/25 7:36 PM, Lucas Vargas Dias via dev wrote: > > Check if lsp exists in TS, if router-port of lsp exist > > and lrp has the same network address of adversing. > > If some of conditions is false, remove the route of routes > > advertising. > > Also, check if lrp that's learning a new route has address in the > > same subnet of neighbor. > > These verifications avoid loop that generates a high CPU load viewed in > > ovsdb-server. > > > > Reported-at: > https://www.mail-archive.com/ovs-discuss@openvswitch.org/msg10304.html > > Signed-off-by: Lucas Vargas Dias <lucas.vd...@luizalabs.com> > > --- > > Hi Lucas, > > Thanks for the patch! And sorry for the delay in reviews. > > > ic/ovn-ic.c | 113 ++++++++++++++++++++++++- > > tests/ovn-ic.at | 220 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 331 insertions(+), 2 deletions(-) > > > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > > index c8796680b..ee5e7dbf4 100644 > > --- a/ic/ovn-ic.c > > +++ b/ic/ovn-ic.c > > @@ -1315,11 +1315,63 @@ route_has_local_gw(const struct > nbrec_logical_router *lr, > > return false; > > } > > > > +static bool > > +lrp_has_neighbor_in_ts(const struct nbrec_logical_router_port *lrp, > > + struct in6_addr *nexthop) > > +{ > > + bool has_neighbor_in_same_tw = false; > > + struct in6_addr lrp_address; > > + unsigned int lrp_mask_len; > > + if (!lrp || !nexthop) { > > + return false; > > + } > > + > > + for (size_t n_addr = 0; n_addr < lrp->n_networks; n_addr++) { > > + char *network = lrp->networks[n_addr]; > > + if (!network) { > > + continue; > > + } > > + > > + if (!ip46_parse_cidr(network, &lrp_address, &lrp_mask_len)) { > > + continue; > > + } > > + > > + if (IN6_IS_ADDR_V4MAPPED(&lrp_address) != > > + IN6_IS_ADDR_V4MAPPED(nexthop)) { > > + continue; > > + } > > + > > + if (IN6_IS_ADDR_V4MAPPED(&lrp_address)) { > > + ovs_be32 neigh_prefix_v4 = > in6_addr_get_mapped_ipv4(nexthop); > > + ovs_be32 prefix_v4 = in6_addr_get_mapped_ipv4(&lrp_address); > > + ovs_be32 mask = be32_prefix_mask(lrp_mask_len); > > + if ((prefix_v4 & mask) == (neigh_prefix_v4 & mask)) { > > + has_neighbor_in_same_tw = true; > > No need for the local variable, we could just return true here. > > > + break; > > + } > > + } else { > > + struct in6_addr mask = ipv6_create_mask(lrp_mask_len); > > + struct in6_addr lrp_ipv6_prefix = > ipv6_addr_bitand(&lrp_address, > > + &mask); > > + struct in6_addr neihgbor_ipv6_prefix = > ipv6_addr_bitand(nexthop, > > + > &mask); > > + if (ipv6_addr_equals(&lrp_ipv6_prefix, > &neihgbor_ipv6_prefix)) { > > + has_neighbor_in_same_tw = true; > > + break; > > Same here. > > > + } > > + } > > + } > > + > > + return has_neighbor_in_same_tw; > > And here return false. > > However, we already have the extract_lrp_networks() function in > ovn-util.c. Please use that one instead of manually parsing > NB.Logical_Router_Port networks. > I agree. > > +} > > + > > static bool > > route_need_learn(const struct nbrec_logical_router *lr, > > const struct icsbrec_route *isb_route, > > struct in6_addr *prefix, unsigned int plen, > > - const struct smap *nb_options) > > + const struct smap *nb_options, > > + const struct nbrec_logical_router_port *ts_lrp, > > + struct in6_addr *nexthop) > > { > > if (!smap_get_bool(nb_options, "ic-route-learn", false)) { > > return false; > > @@ -1344,6 +1396,10 @@ route_need_learn(const struct > nbrec_logical_router *lr, > > return false; > > } > > > > + if (!lrp_has_neighbor_in_ts(ts_lrp, nexthop)) { > > + return false; > > + } > > + > > return true; > > } > > > > @@ -1467,7 +1523,7 @@ sync_learned_routes(struct ic_context *ctx, > > continue; > > } > > if (!route_need_learn(ic_lr->lr, isb_route, &prefix, plen, > > - &nb_global->options)) { > > + &nb_global->options, lrp, &nexthop)) { > > continue; > > } > > > > @@ -1480,6 +1536,7 @@ sync_learned_routes(struct ic_context *ctx, > > struct uuid ext_id; > > smap_get_uuid(&route_learned->nb_route->external_ids, > > "ic-learned-route", &ext_id); > > + > > if (!uuid_equals(&ext_id, &isb_route->header_.uuid)) { > > char *uuid_s = > > xasprintf(UUID_FMT, > > @@ -1785,6 +1842,58 @@ delete_orphan_ic_routes(struct ic_context *ctx, > > isb_route->nexthop, isb_route->origin, > > isb_route->route_table, > isb_route->transit_switch); > > icsbrec_route_delete(isb_route); > > + continue; > > + } > > + > > + const struct nbrec_logical_router_port *lrp; > > + const struct nbrec_logical_switch *ls; > > + ls = find_ts_in_nb(ctx, isb_route->transit_switch); > > + if (ls) { > > + bool found_lrp_in_ts = false; > > + struct in6_addr lrp_address, nexthop; > > + unsigned int prefix_len; > > + if (!ip46_parse(isb_route->nexthop, &nexthop)) { > > + continue; > > + } > > + > > + for (size_t i = 0; i < ls->n_ports; i++) { > > + char * lsp_name = ls->ports[i]->name; > > Nit: should be "char *lsp_name". > > I agree. > > + const char *lrp_name = get_lrp_name_by_ts_port_name(ctx, > > + > lsp_name); > > + if (lrp_name) { > > + lrp = get_lrp_by_lrp_name(ctx, lrp_name); > > + size_t n_networks = 0; > > + if (lrp) { > > + n_networks = lrp->n_networks; > > + } > > + for (size_t n_addr = 0; n_addr < n_networks; > n_addr++) { > > + char *network = lrp->networks[n_addr]; > > + if (!network) { > > + continue; > > + } > > + > > + if (!ip46_parse_cidr(network, &lrp_address, > > + &prefix_len)) { > > + continue; > > + } > > + > > + if (ipv6_addr_equals(&lrp_address, &nexthop)) { > > + found_lrp_in_ts = true; > > + break; > > + } > > + } > > + } > > + } > > + > > + if (!found_lrp_in_ts) { > > + static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 1); > > + VLOG_INFO_RL(&rl, "Deleting orphan ICDB:Route: %s->%s " > > + " (%s, rtb:%s, transit switch: %s)", > > + isb_route->ip_prefix, isb_route->nexthop, > > + isb_route->origin, isb_route->route_table, > > + isb_route->transit_switch); > > + icsbrec_route_delete(isb_route); > > + } > > Most of this chunk should probably be extracted into a separate > function. It would also allow you to avoid the found_lrp_in_ts variable > and just return early. > > Also, I have the same comment about not parsing the LRP networks > manually, please use extract_lrp_networks(). > > I agree, I will use the extract_lrp_networks function. In this part of the code, using other function will be better to find lrp. > > } > > } > > icsbrec_route_index_destroy_row(isb_route_key); > > diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at > > index 0fd38acb2..acc8c68f5 100644 > > --- a/tests/ovn-ic.at > > +++ b/tests/ovn-ic.at > > @@ -3001,3 +3001,223 @@ OVN_CHECK_PACKETS([hv3/vif5-tx.pcap], > [expected-vif5]) > > OVN_CLEANUP_IC([az1], [az2], [az3]) > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn-ic -- Delete route lsp orphan]) > > + > > +ovn_init_ic_db > > +ovn-ic-nbctl ts-add ts1 > > + > > +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: > > +# > > +# / transit switch (ts11) \ > > +# logical router (lr11) - - logical router (lr12) > > +# \ transit switch (ts12) / > > +# > > + > > +# VPC1 > > +# create lr11, lr12, ts11, ts12 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 2; do > > + ts=ts1$j > > + 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 VPC1 > > +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 > > +192.168.0.0/24 169.254.102.2 > > +]) > > + > > +check ovn_as az2 ovn-nbctl lrp-del lrp-lr12-ts12 > > + > > +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_CLEANUP_IC([az1], [az2]) > > + > > +AT_CLEANUP > > +]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn-ic -- Delete route lrp orphan]) > > + > > +ovn_init_ic_db > > +ovn-ic-nbctl ts-add ts1 > > + > > +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: > > +# > > +# / transit switch (ts11) \ > > +# logical router (lr11) - - logical router (lr12) > > +# \ transit switch (ts12) / > > +# > > + > > +# VPC1 > > +# create lr11, lr12, ts11, ts12 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 2; do > > + ts=ts1$j > > + 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 VPC1 > > +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 > > +192.168.0.0/24 169.254.102.2 > > +]) > > + > > +check ovn_as az2 ovn-nbctl lsp-del lsp-ts12-lr12 > > + > > +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_CLEANUP_IC([az1], [az2]) > > + > > +AT_CLEANUP > > +]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn-ic -- Check loop with lsp orphan in same subnet of new > lsp in other TS]) > > + > > +ovn_init_ic_db > > +ovn-ic-nbctl ts-add ts1 > > + > > +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: > > +# > > +# / transit switch (ts11) \ > > +# logical router (lr11) - - logical router (lr12) > > +# \ transit switch (ts12) / > > +# > > + > > +# 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 > > + ts=ts1$j > > + 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 VPC1 > > +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 > > +]) > > + > > +# Delete LRP connection from lr11 to ts11, keep lsp orphan > > +check ovn_as az1 ovn-nbctl lrp-del lrp-lr11-ts11 > > + > > +# create ts12, and connect lr11 and lr21 to it in the same subnet of > ts11 where > > +# exist one lsp orphan > > +for i in 1 2; do > > + ovn_as az$i > > + ts=ts12 > > + > > + lr=lr1$i > > + for j in 1; do > > + 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$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 > > + > > +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.22 > > +]) > > + > > +OVN_CLEANUP_IC([az1], [az2]) > > + > > +AT_CLEANUP > > +]) > > Regards, > Dumitru > > > I will adjust the code based on comments and submit a new version. Regards, Lucas -- _‘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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
