Hi Dumitru, 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"} Alternatively we can not to add this index for now and add it later (when?). 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. 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). What do you think? > >> "isRoot": true}, >> "Connection": { >> "columns": { >> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at >> index e234b7fb9..ceee45092 100644 >> --- a/tests/ovn-ic.at >> +++ b/tests/ovn-ic.at >> @@ -194,6 +194,66 @@ OVN_CLEANUP_IC >> AT_CLEANUP >> ]) >> >> +OVN_FOR_EACH_NORTHD([ >> +AT_SETUP([ovn-ic -- duplicate NB route adv/learn]) >> + >> +ovn_init_ic_db >> +net_add n1 >> + >> +# 1 GW per AZ >> +for i in 1 2; do >> + az=az$i >> + ovn_start $az >> + sim_add gw-$az >> + as gw-$az >> + check ovs-vsctl add-br br-phys >> + ovn_az_attach $az n1 br-phys 192.168.1.$i >> + check ovs-vsctl set open . external-ids:ovn-is-interconn=true >> + check ovn-nbctl set nb-global . \ >> + options:ic-route-adv=true \ >> + options:ic-route-adv-default=true \ >> + options:ic-route-learn=true \ >> + options:ic-route-learn-default=true >> +done >> + >> +ovn_as az1 >> + >> +# create transit switch and connect to LR >> +check ovn-ic-nbctl ts-add ts1 >> +for i in 1 2; do >> + ovn_as az$i >> + >> + check ovn-nbctl lr-add lr1 >> + check ovn-nbctl lrp-add lr1 lrp$i 00:00:00:00:0$i:01 10.0.$i.1/24 >> + check ovn-nbctl lrp-set-gateway-chassis lrp$i gw-az$i >> + >> + check ovn-nbctl lsp-add ts1 lsp$i -- \ >> + lsp-set-addresses lsp$i router -- \ >> + lsp-set-type lsp$i router -- \ >> + lsp-set-options lsp$i router-port=lrp$i >> +done >> + >> +ovn_as az1 >> + >> +ovn-nbctl \ >> + --id=@id create logical-router-static-route ip_prefix=1.1.1.1/32 >> nexthop=10.0.1.10 -- \ >> + add logical-router lr1 static_routes @id >> +ovn-nbctl \ >> + --id=@id create logical-router-static-route ip_prefix=1.1.1.1/32 >> nexthop=10.0.1.10 -- \ >> + add logical-router lr1 static_routes @id >> + >> +wait_row_count ic-sb:route 1 ip_prefix=1.1.1.1/32 >> + >> +for i in 1 2; do >> + az=az$i >> + OVN_CLEANUP_SBOX(gw-$az) >> + OVN_CLEANUP_AZ([$az]) >> +done >> + >> +OVN_CLEANUP_IC >> +AT_CLEANUP >> +]) >> + >> OVN_FOR_EACH_NORTHD([ >> AT_SETUP([ovn-ic -- gateway sync]) >> > > Thanks, > Dumitru > > _______________________________________________ > dev mailing list > [email protected] <mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
