The learned routes hash previously only used route parameters (prefix, nexthop, origin, route_table). This caused collisions when identical routes were learned through different transit switches, leading to infinite route re-learning.
While such configurations are invalid due to overlapping IP addressing - ICshould handle them without looping. Fix by including the IC route UUID in the hash to distinguish routes with identical parameters but different origins. As a result, routers will now learn the correct number of prefixes as ECMP routes, making the misconfiguration visible to the user. Signed-off-by: Alexandra Rukomoinikova <[email protected]> --- v1 --> v2: fixed typo in the tests v2 --> v3: fixed hash calculation --- ic/ovn-ic.c | 56 ++++++++++++++-------------- tests/ovn-ic.at | 98 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 27 deletions(-) diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c index 95d73cb4b..7d2f9442a 100644 --- a/ic/ovn-ic.c +++ b/ic/ovn-ic.c @@ -1286,6 +1286,7 @@ struct ic_route_info { const char *origin; const char *route_table; const char *route_tag; + struct uuid ic_route_uuid; const struct nbrec_logical_router *nb_lr; @@ -1305,9 +1306,11 @@ struct ic_route_info { static uint32_t ic_route_hash(const struct in6_addr *prefix, unsigned int plen, const struct in6_addr *nexthop, const char *origin, - const char *route_table) + const char *route_table, const struct uuid *ic_route_uuid) { - uint32_t basis = hash_bytes(prefix, sizeof *prefix, (uint32_t)plen); + uint32_t basis = ic_route_uuid ? uuid_hash(ic_route_uuid) : 0; + basis = hash_bytes(prefix, sizeof *prefix, basis); + basis = hash_int((uint32_t) plen, basis); basis = hash_string(origin, basis); basis = hash_string(route_table, basis); return hash_bytes(nexthop, sizeof *nexthop, basis); @@ -1316,18 +1319,22 @@ ic_route_hash(const struct in6_addr *prefix, unsigned int plen, static struct ic_route_info * ic_route_find(struct hmap *routes, const struct in6_addr *prefix, unsigned int plen, const struct in6_addr *nexthop, - const char *origin, const char *route_table, uint32_t hash) + const char *origin, const char *route_table, + const struct uuid *ic_route_uuid, uint32_t hash) { struct ic_route_info *r; if (!hash) { - hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); + hash = ic_route_hash(prefix, plen, nexthop, origin, route_table, + ic_route_uuid); } HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) { if (ipv6_addr_equals(&r->prefix, prefix) && r->plen == plen && ipv6_addr_equals(&r->nexthop, nexthop) && !strcmp(r->origin, origin) && - !strcmp(r->route_table ? r->route_table : "", route_table)) { + !strcmp(r->route_table ? r->route_table : "", route_table) && + (!ic_route_uuid || uuid_equals(&r->ic_route_uuid, + ic_route_uuid))) { return r; } } @@ -1370,7 +1377,8 @@ parse_route(const char *s_prefix, const char *s_nexthop, static bool add_to_routes_learned(struct hmap *routes_learned, const struct nbrec_logical_router_static_route *nb_route, - const struct nbrec_logical_router *nb_lr) + const struct nbrec_logical_router *nb_lr, + const struct uuid *ic_route_uuid) { struct in6_addr prefix, nexthop; unsigned int plen; @@ -1379,8 +1387,11 @@ add_to_routes_learned(struct hmap *routes_learned, return false; } const char *origin = smap_get_def(&nb_route->options, "origin", ""); + + uint32_t hash = ic_route_hash(&prefix, plen, &nexthop, origin, + nb_route->route_table, ic_route_uuid); if (ic_route_find(routes_learned, &prefix, plen, &nexthop, origin, - nb_route->route_table, 0)) { + nb_route->route_table, ic_route_uuid, hash)) { /* Route was added to learned on previous iteration. */ return true; } @@ -1393,9 +1404,9 @@ add_to_routes_learned(struct hmap *routes_learned, ic_route->origin = origin; ic_route->route_table = nb_route->route_table; ic_route->nb_lr = nb_lr; - hmap_insert(routes_learned, &ic_route->node, - ic_route_hash(&prefix, plen, &nexthop, origin, - nb_route->route_table)); + memcpy(&ic_route->ic_route_uuid, ic_route_uuid, sizeof(struct uuid)); + hmap_insert(routes_learned, &ic_route->node, hash); + return true; } @@ -1562,10 +1573,11 @@ add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix, route_table = ""; } - uint hash = ic_route_hash(&prefix, plen, &nexthop, origin, route_table); + uint hash = ic_route_hash(&prefix, plen, &nexthop, origin, + route_table, NULL); if (!ic_route_find(routes_ad, &prefix, plen, &nexthop, origin, route_table, - hash)) { + NULL, hash)) { struct ic_route_info *ic_route = xzalloc(sizeof *ic_route); ic_route->prefix = prefix; ic_route->plen = plen; @@ -2154,20 +2166,9 @@ sync_learned_routes(struct ic_context *ctx, struct ic_route_info *route_learned = ic_route_find(&ic_lr->routes_learned, &prefix, plen, &nexthop, isb_route->origin, - isb_route->route_table, 0); + isb_route->route_table, + &isb_route->header_.uuid, 0); if (route_learned) { - /* Sync external-ids */ - 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, - UUID_ARGS(&isb_route->header_.uuid)); - nbrec_logical_router_static_route_update_external_ids_setkey( - route_learned->nb_route, "ic-learned-route", uuid_s); - free(uuid_s); - } hmap_remove(&ic_lr->routes_learned, &route_learned->node); free(route_learned); } else { @@ -2275,7 +2276,7 @@ advertise_routes(struct ic_context *ctx, } struct ic_route_info *route_adv = ic_route_find(routes_ad, &prefix, plen, &nexthop, - isb_route->origin, isb_route->route_table, 0); + isb_route->origin, isb_route->route_table, NULL, 0); if (!route_adv) { /* Delete the extra route from IC-SB. */ VLOG_DBG("Delete route %s -> %s from IC-SB, which is not found" @@ -2349,7 +2350,8 @@ build_ts_routes_to_adv(struct ic_context *ctx, if (smap_get_uuid(&nb_route->external_ids, "ic-learned-route", &isb_uuid)) { /* It is a learned route */ - if (!add_to_routes_learned(&ic_lr->routes_learned, nb_route, lr)) { + if (!add_to_routes_learned(&ic_lr->routes_learned, nb_route, lr, + &isb_uuid)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, "Bad format of learned route in NB: " "%s -> %s. Delete it.", nb_route->ip_prefix, diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at index 1a826aa1c..c633cdeb4 100644 --- a/tests/ovn-ic.at +++ b/tests/ovn-ic.at @@ -4594,3 +4594,101 @@ OVN_CLEANUP_IC([az1], [az2]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-ic -- check loop with annonce duplicated prefixes - multiple ts]) + +ovn_init_ic_db +ovn-ic-nbctl ts-add ts11 +ovn-ic-nbctl ts-add ts12 + +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 + +for i in 1 2; do + ovn_as az$i + + check ovn-nbctl lr-add lr$i + + check ovn-nbctl lrp-add lr$i lr-lrp-$i aa:aa:aa:aa:a1:0$i 169.254.101.1/24 + check ovn-nbctl lsp-add-router-port ts11 lsp-az$i lr-lrp-$i + + # Create subnet for annonce. + check ovn-nbctl lrp-add lr$i lr-lrp-subnet-$i aa:aa:aa:aa:a2:0$i 192.168.0.$i/24 + + # Create one more lrp-ts port through another transit switch. + check ovn-nbctl lrp-add lr$i lr-lrp-dup$i aa:aa:aa:aa:a3:01 169.254.101.1/24 + check ovn-nbctl lsp-add-router-port ts12 lsp-dup-az$i lr-lrp-dup$i +done + +OVS_WAIT_FOR_OUTPUT([ovn_as az1 ovn-nbctl lr-route-list lr1 | grep 192.168 | + grep learned | awk '{print $1, $2}' | sort ], [0], [dnl +192.168.0.0/24 169.254.101.1 +192.168.0.0/24 169.254.101.1 +]) + +OVN_CLEANUP_IC([az1], [az2]) +AT_CLEANUP +]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-ic -- check loop with annonce duplicated prefixes - single ts]) + +ovn_init_ic_db + +for i in 1 2 3; 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: +# logical router (lr12) +# | +# logical router (lr11) - / transit switch (ts11) \ - logical router (lr13) +# + +# Create lr11, lr13, ts11 and connect them +for i in 1 3; do + ovn_as az$i + + check ovn-nbctl lr-add lr1$i + check ovn-ic-nbctl --wait=sb --may-exist ts-add ts11 + + # Create LRP and connect to TS + check ovn-nbctl lrp-add lr1$i lrp-lr1$i-ts11 aa:aa:aa:aa:a1:0$i 169.254.101.1/24 + check ovn-nbctl lsp-add-router-port ts11 lsp-ts11-lr1$i lrp-lr1$i-ts11 +done + +# Create lr12 and connect it to ts11 +ovn_as az2 +check ovn-nbctl lr-add lr12 + +# Create LRP and connect to TS +check ovn-nbctl lrp-add lr12 lrp-lr12-ts11 aa:aa:aa:aa:a1:03 169.254.101.2/24 +check ovn-nbctl lsp-add-router-port ts11 lsp-lr12-ts11 lrp-lr12-ts11 + +# Create directly-connected route in lr11 +check ovn_as az1 ovn-nbctl lrp-add lr11 lrp-lr11 aa:aa:aa:aa:bb:01 "192.168.0.1/24" +check ovn_as az3 ovn-nbctl lrp-add lr13 lrp-lr13 aa:aa:aa:aa:bb:03 "192.168.0.1/24" + +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 +192.168.0.0/24 169.254.101.1 +192.168.0.0/24 169.254.101.1 +]) + +OVN_CLEANUP_IC([az1], [az2], [az3]) +AT_CLEANUP +]) -- 2.48.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
