diff mbox series

[ovs-dev,v3,3/5] ic: prevent advertising/learning multiple same routes

Message ID 20221215170219.3072151-4-odivlad@gmail.com
State Accepted
Headers show
Series OVN IC multiple same routes fixes | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Vladislav Odintsov Dec. 15, 2022, 5:02 p.m. UTC
Advertize one route (ip_prefix, nexthop, route_table, transit_switch,
availability_zone) only once.

Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
---
 ic/ovn-ic.c     | 84 ++++++++++++++++++++++++++++++-------------------
 tests/ovn-ic.at | 60 +++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 33 deletions(-)
diff mbox series

Patch

diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 0d5e5f5d9..518796149 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,43 @@  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 {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+        VLOG_WARN_RL(&rl, "Duplicate route advertisement was suppressed! NB "
+                     "route uuid: "UUID_FMT,
+                     UUID_ARGS(&nb_route->header_.uuid));
+    }
+}
+
+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 +1180,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 +1225,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 +1387,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 +1486,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 +1568,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/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])