Hi Numan, Dumitru, I forgot one thing about a possible upgrade recommendation for the users. Fix can be applied much easier: just upgrading all ovn-ic in all AZs first and then converting DB schema. This will remove all duplicates. The latter can be requested manually invoking systemctl restart ovn-ic-db (at least for RH, there is a separate systemd unit for IC databases) or calling ovsdb-client convert … command directly.
Would it be an acceptable compromise for this situation? Regards, Vladislav Odintsov > On 12 Dec 2022, at 16:20, Numan Siddique <[email protected]> wrote: > > On Mon, Dec 12, 2022 at 5:41 AM Dumitru Ceara <[email protected]> wrote: >> >> On 12/9/22 19:42, Vladislav Odintsov wrote: >>> Hi Dumitru, >> >> Hi Vladislav, >> >>> >>> please see answers inline. >>> >>> Regards, >>> Vladislav Odintsov >>> >>>> On 9 Dec 2022, at 17:37, Dumitru Ceara <[email protected]> wrote: >>>> >>>> On 12/6/22 11:20, Vladislav Odintsov wrote: >>>>> Signed-off-by: Vladislav Odintsov <[email protected] >>>>> <mailto:[email protected]>> >>>>> --- >>>> >>>> Hi Vladislav, >>>> >>>>> ic/ovn-ic.c | 83 +++++++++++++++++++++++++++------------------ >>>>> ovn-ic-sb.ovsschema | 6 ++-- >>>>> tests/ovn-ic.at | 60 ++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 114 insertions(+), 35 deletions(-) >>>>> >>>>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c >>>>> index 9e2369fef..64307d8c4 100644 >>>>> --- a/ic/ovn-ic.c >>>>> +++ b/ic/ovn-ic.c >>>>> @@ -884,10 +884,12 @@ 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, char *route_table) >>>>> + const char *origin, const char *route_table, uint32_t hash) >>>>> { >>>>> struct ic_route_info *r; >>>>> - uint32_t hash = ic_route_hash(prefix, plen, nexthop, origin, >>>>> route_table); >>>>> + if (!hash) { >>>>> + hash = ic_route_hash(prefix, plen, nexthop, origin, route_table); >>>>> + } >>>>> HMAP_FOR_EACH_WITH_HASH (r, node, hash, routes) { >>>>> if (ipv6_addr_equals(&r->prefix, prefix) && >>>>> r->plen == plen && >>>>> @@ -945,8 +947,8 @@ add_to_routes_learned(struct hmap *routes_learned, >>>>> } >>>>> const char *origin = smap_get_def(&nb_route->options, "origin", ""); >>>>> if (ic_route_find(routes_learned, &prefix, plen, &nexthop, origin, >>>>> - nb_route->route_table)) { >>>>> - /* Route is already added to learned in previous iteration. */ >>>>> + nb_route->route_table, 0)) { >>>>> + /* Route was added to learned on previous iteration. */ >>>>> return true; >>>>> } >>>>> >>>>> @@ -1093,10 +1095,42 @@ route_need_advertise(const char *policy, >>>>> } >>>>> >>>>> static void >>>>> -add_to_routes_ad(struct hmap *routes_ad, >>>>> - const struct nbrec_logical_router_static_route >>>>> *nb_route, >>>>> - const struct lport_addresses *nexthop_addresses, >>>>> - const struct smap *nb_options, const char *route_table) >>>>> +add_to_routes_ad(struct hmap *routes_ad, const struct in6_addr prefix, >>>>> + unsigned int plen, const struct in6_addr nexthop, >>>>> + const char *origin, const char *route_table, >>>>> + const struct nbrec_logical_router_port *nb_lrp, >>>>> + const struct nbrec_logical_router_static_route >>>>> *nb_route) >>>>> +{ >>>>> + if (route_table == NULL) { >>>>> + route_table = ""; >>>>> + } >>>>> + >>>>> + uint hash = ic_route_hash(&prefix, plen, &nexthop, origin, >>>>> route_table); >>>>> + >>>>> + if (!ic_route_find(routes_ad, &prefix, plen, &nexthop, origin, >>>>> route_table, >>>>> + hash)) { >>>>> + struct ic_route_info *ic_route = xzalloc(sizeof *ic_route); >>>>> + ic_route->prefix = prefix; >>>>> + ic_route->plen = plen; >>>>> + ic_route->nexthop = nexthop; >>>>> + ic_route->nb_route = nb_route; >>>>> + ic_route->origin = origin; >>>>> + ic_route->route_table = route_table; >>>>> + ic_route->nb_lrp = nb_lrp; >>>>> + hmap_insert(routes_ad, &ic_route->node, hash); >>>>> + } else { >>>>> + VLOG_WARN("Duplicate route advertisement was suppressed! NB >>>>> route " >>>>> + "uuid: "UUID_FMT, >>>>> + UUID_ARGS(&nb_route->header_.uuid)); >>>> >>>> I think this should be rate limited. >>> >>> Agree, will fix this in v3. >>> >>>> >>>>> + } >>>>> +} >>>>> + >>>>> +static void >>>>> +add_static_to_routes_ad( >>>>> + struct hmap *routes_ad, >>>>> + const struct nbrec_logical_router_static_route *nb_route, >>>>> + const struct lport_addresses *nexthop_addresses, >>>>> + const struct smap *nb_options, const char *route_table) >>>>> { >>>>> if (strcmp(route_table, nb_route->route_table)) { >>>>> if (VLOG_IS_DBG_ENABLED()) { >>>>> @@ -1145,16 +1179,8 @@ add_to_routes_ad(struct hmap *routes_ad, >>>>> ds_destroy(&msg); >>>>> } >>>>> >>>>> - struct ic_route_info *ic_route = xzalloc(sizeof *ic_route); >>>>> - ic_route->prefix = prefix; >>>>> - ic_route->plen = plen; >>>>> - ic_route->nexthop = nexthop; >>>>> - ic_route->nb_route = nb_route; >>>>> - ic_route->origin = ROUTE_ORIGIN_STATIC; >>>>> - ic_route->route_table = nb_route->route_table; >>>>> - hmap_insert(routes_ad, &ic_route->node, >>>>> - ic_route_hash(&prefix, plen, &nexthop, >>>>> ROUTE_ORIGIN_STATIC, >>>>> - nb_route->route_table)); >>>>> + add_to_routes_ad(routes_ad, prefix, plen, nexthop, >>>>> ROUTE_ORIGIN_STATIC, >>>>> + nb_route->route_table, NULL, nb_route); >>>>> } >>>>> >>>>> static void >>>>> @@ -1198,18 +1224,9 @@ add_network_to_routes_ad(struct hmap *routes_ad, >>>>> const char *network, >>>>> ds_destroy(&msg); >>>>> } >>>>> >>>>> - struct ic_route_info *ic_route = xzalloc(sizeof *ic_route); >>>>> - ic_route->prefix = prefix; >>>>> - ic_route->plen = plen; >>>>> - ic_route->nexthop = nexthop; >>>>> - ic_route->nb_lrp = nb_lrp; >>>>> - ic_route->origin = ROUTE_ORIGIN_CONNECTED; >>>>> - >>>>> /* directly-connected routes go to <main> route table */ >>>>> - ic_route->route_table = NULL; >>>>> - hmap_insert(routes_ad, &ic_route->node, >>>>> - ic_route_hash(&prefix, plen, &nexthop, >>>>> - ROUTE_ORIGIN_CONNECTED, "")); >>>>> + add_to_routes_ad(routes_ad, prefix, plen, nexthop, >>>>> ROUTE_ORIGIN_CONNECTED, >>>>> + NULL, nb_lrp, NULL); >>>>> } >>>>> >>>>> static bool >>>>> @@ -1369,7 +1386,7 @@ 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); >>>>> + isb_route->route_table, 0); >>>>> if (route_learned) { >>>>> /* Sync external-ids */ >>>>> struct uuid ext_id; >>>>> @@ -1468,7 +1485,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); >>>>> + isb_route->origin, isb_route->route_table, 0); >>>>> if (!route_adv) { >>>>> /* Delete the extra route from IC-SB. */ >>>>> VLOG_DBG("Delete route %s -> %s from IC-SB, which is not found" >>>>> @@ -1550,8 +1567,8 @@ build_ts_routes_to_adv(struct ic_context *ctx, >>>>> } >>>>> } else { >>>>> /* It may be a route to be advertised */ >>>>> - add_to_routes_ad(routes_ad, nb_route, ts_port_addrs, >>>>> - &nb_global->options, ts_route_table); >>>>> + add_static_to_routes_ad(routes_ad, nb_route, ts_port_addrs, >>>>> + &nb_global->options, ts_route_table); >>>>> } >>>>> } >>>>> >>>>> diff --git a/ovn-ic-sb.ovsschema b/ovn-ic-sb.ovsschema >>>>> index 72c9d3f3e..1d60b36d1 100644 >>>>> --- a/ovn-ic-sb.ovsschema >>>>> +++ b/ovn-ic-sb.ovsschema >>>>> @@ -1,7 +1,7 @@ >>>>> { >>>>> "name": "OVN_IC_Southbound", >>>>> - "version": "1.1.0", >>>>> - "cksum": "2309827842 6784", >>>>> + "version": "1.1.1", >>>>> + "cksum": "3684563024 6914", >>>>> "tables": { >>>>> "IC_SB_Global": { >>>>> "columns": { >>>>> @@ -101,6 +101,8 @@ >>>>> "external_ids": { >>>>> "type": {"key": "string", "value": "string", >>>>> "min": 0, "max": "unlimited"}}}, >>>>> + "indexes": [["transit_switch", "availability_zone", >>>>> "route_table", >>>>> + "ip_prefix", "nexthop"]], >>>> >>>> Unfortunately we can't just add an index. This will break backwards >>>> compatibility. I'm afraid we just have to deal with duplicates in this >>>> table (doesn't your patch already warn for that?). Or maybe we should >>>> find a way to transition (recommend an upgrade path?) to a version where >>>> we can enforce the index. >>> >>> What did you mean by breaking backward compatibility here? >>> I see that in worst case ovn-ic database restart will fail with the schema >>> convert attempt in case there are records which violate this index. >>> Can we just notice in NEWS file that user must ensure before the upgrade >>> that there are no multiple records with same availability zone, transit >>> switch, route table, ip_prefix and nexthop? >>> >>> I created multiple same routes in ic sb and tried to apply new schema: >>> # ovsdb-client convert unix://var/run/ovn/ovn_ic_sb_db.sock >>> /usr/share/ovn/ovn-ic-sb.ovsschema >>> 2022-12-09T18:37:36Z|00001|ovsdb|WARN|/usr/share/ovn/ovn-ic-sb.ovsschema: >>> changed 2 columns in 'OVN_IC_Southbound' database from ephemeral to >>> persistent, including 'status' column in 'Connection' table, because >>> clusters do not support ephemeral columns >>> ovsdb-client: transaction returned error: {"details":"Transaction causes >>> multiple rows in \"Route\" table to have identical values >>> (\"668552d5-45ce-4e51-b728-118ee9a103b1\", >>> be9ecbbd-ffbe-4e40-afd9-8c53fee2b39b, \"1\", \"1.1.1.1/32\", and \"\") for >>> index on columns \"transit_switch\", \"availability_zone\", >>> \"route_table\", \"ip_prefix\", and \"nexthop\". First row, with UUID >>> a4b905bd-3d26-4d78-804e-e51dd54429ea, was inserted by this transaction. >>> Second row, with UUID 426d4795-8e1b-432f-9cf1-4a0f10818265, was inserted by >>> this transaction.","error":"constraint violation"} >>> >> >> This is what I meant by "breaking backwards compatibility". >> >>> Alternatively we can not to add this index for now and add it later (when?). >> >> This is what I was suggesting as a "way to transition to a version where >> we can enforce the index". So have ovn-ic clean up and reconcile the >> DBs properly in the next version. We could potentially also add a note >> to NEWS saying that an index will be enforced later. >> >>> But this also doesn’t guarantee that end user will upgrade from affected >>> version through version with the fix in code but without index change and >>> next upgrade to the version with applied new index. >>> >> >> True, but what if we document in the ovn-upgrades.rst manual that ovn-ic >> users should upgrade from versions pre-v22.09 in two stages: first to >> 22.12.latest and then to whatever newer release they need? >> >>> And the third option I see is just not to add an index at all. But I don’t >>> like it though. Having an index is a good approach, which prevents any >>> possible future (or existent in nowadays) bugs, which lead to creation of >>> duplicated records somehow. >>> I guess it’s not too hard to check the duplicates - convert command returns >>> uuid of duplicated records, so a slight upgrade difficulties doesn’t look >>> to me like a reason for not to use this ability (new index). >> >> I have the impression that this is not that easy. Doesn't it require to >> stop ovn-ic daemons while running the cleanup script? Otherwise the >> dups might be reinserted. >> >>> What do you think? >>> >> >> I think I might be OK with enforcing the index now too if we properly >> document it. It also depends on how many users we have out there that >> might suffer from these duplicated routes being advertised (I'm guessing >> not too many). CC-ing more maintainers to see what others think about this. > > Would it work if we add a new table instead ? "Route_v2" with indexing > and deprecating the older one ? > > Thanks > Numan > >> >> Regards, >> Dumitru >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
