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
