On 12/6/22 11:20, Vladislav Odintsov wrote:
> Signed-off-by: Vladislav Odintsov <[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.
> + }
> +}
> +
> +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.
> "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]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
