Hi Maxim, thanks for the patch.
When submitting follow-up patches, please be sure to indicate the
version of the patch in the subject line (e.g. "PATCH ovn v2"). I'm
using the email date to assume this is the set of patches I should be
reviewing.
I have a couple of findings down below.
On 8/1/23 03:22, Maxim Korezkij via dev wrote:
previously if multiple routers in the same az are connected to the same
transit switch then ovn-ic would only propagate the routes of one of
these routers to the ic-sb.
This commit fixes this behaviour and allows multiple routers in a single
az to use route advertisements.
Signed-off-by: Maxim Korezkij <maxim.korezkij@mail.schwarz>
Signed-off-by: Felix Huettner <felix.huettner@mail.schwarz>
---
ic/ovn-ic.c | 26 +++++++++++----
tests/ovn-ic.at | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 105 insertions(+), 7 deletions(-)
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 6f31037ec..49850954f 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -1583,8 +1583,8 @@ build_ts_routes_to_adv(struct ic_context *ctx,
static void
advertise_lr_routes(struct ic_context *ctx,
Patch 2 changes the name of this function to "collect_lr_routes". I
think that change should be done in this patch since
"advertise_lr_routes" isn't actually advertising the routes anymore.
- const struct icsbrec_availability_zone *az,
- struct ic_router_info *ic_lr)
+ struct ic_router_info *ic_lr,
+ struct shash *routes_ad_by_ts)
{
const struct nbrec_nb_global *nb_global =
nbrec_nb_global_first(ctx->ovnnb_idl);
@@ -1595,7 +1595,7 @@ advertise_lr_routes(struct ic_context *ctx,
struct lport_addresses ts_port_addrs;
const struct icnbrec_transit_switch *key;
- struct hmap routes_ad = HMAP_INITIALIZER(&routes_ad);
+ struct hmap *routes_ad;
for (int i = 0; i < ic_lr->n_isb_pbs; i++) {
isb_pb = ic_lr->isb_pbs[i];
key = icnbrec_transit_switch_index_init_row(
@@ -1604,6 +1604,12 @@ advertise_lr_routes(struct ic_context *ctx,
ts_name = icnbrec_transit_switch_index_find(
ctx->icnbrec_transit_switch_by_name, key)->name;
icnbrec_transit_switch_index_destroy_row(key);
+ routes_ad = shash_find_data(routes_ad_by_ts, ts_name);
+ if (!routes_ad) {
+ routes_ad = xzalloc(sizeof *routes_ad);
+ hmap_init(routes_ad);
+ shash_add(routes_ad_by_ts, ts_name, routes_ad);
+ }
if (!extract_lsp_addresses(isb_pb->address, &ts_port_addrs)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
@@ -1615,12 +1621,10 @@ advertise_lr_routes(struct ic_context *ctx,
}
lrp_name = get_lrp_name_by_ts_port_name(ctx, isb_pb->logical_port);
route_table = get_route_table_by_lrp_name(ctx, lrp_name);
- build_ts_routes_to_adv(ctx, ic_lr, &routes_ad, &ts_port_addrs,
+ build_ts_routes_to_adv(ctx, ic_lr, routes_ad, &ts_port_addrs,
nb_global, route_table);
- advertise_routes(ctx, az, ts_name, &routes_ad);
destroy_lport_addresses(&ts_port_addrs);
}
- hmap_destroy(&routes_ad);
}
static void
@@ -1721,14 +1725,22 @@ route_run(struct ic_context *ctx,
icsbrec_port_binding_index_destroy_row(isb_pb_key);
struct ic_router_info *ic_lr;
+ struct shash routes_ad_by_ts = SHASH_INITIALIZER(&routes_ad_by_ts);
HMAP_FOR_EACH_SAFE (ic_lr, node, &ic_lrs) {
- advertise_lr_routes(ctx, az, ic_lr);
+ advertise_lr_routes(ctx, ic_lr, &routes_ad_by_ts);
sync_learned_routes(ctx, az, ic_lr);
free(ic_lr->isb_pbs);
hmap_destroy(&ic_lr->routes_learned);
hmap_remove(&ic_lrs, &ic_lr->node);
free(ic_lr);
}
+ struct shash_node *node;
+ SHASH_FOR_EACH_SAFE (node, &routes_ad_by_ts) {
+ advertise_routes(ctx, az, node->name, node->data);
+ hmap_destroy(node->data);
+ shash_delete(&routes_ad_by_ts, node);
+ }
You need to free(node->data) in this loop. Or as an alternative, you can
call shash_delete_free_data() instead of shash_delete() at the end of
the loop.
+ shash_destroy(&routes_ad_by_ts);
hmap_destroy(&ic_lrs);
}
diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
index ceee45092..4b470d5c8 100644
--- a/tests/ovn-ic.at
+++ b/tests/ovn-ic.at
@@ -1159,3 +1159,89 @@ AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr12 | grep
dst-ip | sort], [0], [d
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-ic -- route sync -- multiple logical routers])
+
+ovn_init_ic_db
+ovn-ic-nbctl ts-add ts1
+
+for i in 1 2; do
+ ovn_start az$i
+ ovn_as az$i
+
+ # Enable route learning at AZ level
+ ovn-nbctl set nb_global . options:ic-route-learn=true
+ # Enable route advertising at AZ level
+ ovn-nbctl set nb_global . options:ic-route-adv=true
+done
+
+# Create new transit switches and LRs. Test topology is next:
+#
+# logical router (lr11) - transit switch (ts1) - logical router (lr21)
+# \- logical router (lr22)
+#
+# each LR has one connected subnet except TS port
+
+
+# create lr11, lr21, lr22, ts1 and connect them
+ovn-ic-nbctl ts-add ts1
+
+ovn_as az1
+
+lr=lr11
+ovn-nbctl lr-add $lr
+
+lrp=lrp-$lr-ts1
+lsp=lsp-ts1-$lr
+# Create LRP and connect to TS
+ovn-nbctl lrp-add $lr $lrp aa:aa:aa:aa:a1:01 169.254.10.11/24
+ovn-nbctl lsp-add ts1 $lsp \
+ -- lsp-set-addresses $lsp router \
+ -- lsp-set-type $lsp router \
+ -- lsp-set-options $lsp router-port=$lrp
+
+ovn_as az2
+for i in 1 2; do
+ lr=lr2$i
+ ovn-nbctl lr-add $lr
+
+ lrp=lrp-$lr-ts1
+ lsp=lsp-ts1-$lr
+ # Create LRP and connect to TS
+ ovn-nbctl lrp-add $lr $lrp aa:aa:aa:aa:a2:0$i 169.254.10.2$i/24
+ ovn-nbctl lsp-add ts1 $lsp \
+ -- lsp-set-addresses $lsp router \
+ -- lsp-set-type $lsp router \
+ -- lsp-set-options $lsp router-port=$lrp
+done
+
+
+# Create directly-connected routes
+ovn_as az1 ovn-nbctl lrp-add lr11 lrp-lr11 aa:aa:aa:aa:bb:01 "192.168.0.1/24"
+ovn_as az2 ovn-nbctl lrp-add lr21 lrp-lr21 aa:aa:aa:aa:bc:01 "192.168.1.1/24"
+ovn_as az2 ovn-nbctl lrp-add lr22 lrp-lr22 aa:aa:aa:aa:bc:02 "192.168.2.1/24"
+
+# Test direct routes from lr21 and lr22 were learned to lr11
+AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr11 | grep 192.168 |
+ grep learned | awk '{print $1, $2}' | sort ], [0], [dnl
+192.168.1.0/24 169.254.10.21
+192.168.2.0/24 169.254.10.22
+])
+
+# Test direct routes from lr11 and lr22 were learned to lr21
+AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr21 | grep 192.168 |
+ grep learned | awk '{print $1, $2}' | sort ], [0], [dnl
+192.168.0.0/24 169.254.10.11
+])
+
+# Test direct routes from lr11 and lr21 were learned to lr22
+AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr22 | grep 192.168 |
+ grep learned | awk '{print $1, $2}' | sort ], [0], [dnl
+192.168.0.0/24 169.254.10.11
+])
+
+OVN_CLEANUP_IC([az1], [az2])
+
+AT_CLEANUP
+])
\ No newline at end of file
--
2.34.1
Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für die
Verwertung durch den vorgesehenen Empfänger bestimmt.
Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender bitte
unverzüglich in Kenntnis und löschen diese E Mail.
Hinweise zum Datenschutz finden Sie hier<https://www.datenschutz.schwarz>.
This e-mail may contain confidential content and is intended only for the
specified recipient/s.
If you are not the intended recipient, please inform the sender immediately and
delete this e-mail.
Information on data protection can be found
here<https://www.datenschutz.schwarz>.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev