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. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
