On 1/30/25 8:29 PM, [email protected] wrote: > Hi Felix, Hi Martin,
> I noticed that the system tests seem to leave 'ovnvrf1337' in the > system after they finish. The weird thing is that it happens whether > they pass or fail and it's always 1337, even though both tests use > other VRFs as well. Maybe it's the fact that we don't cleanup when a dynamic-routing-vrf-name configuration changes? I think that's what I pointed out in my review of patch 5/11: https://mail.openvswitch.org/pipermail/ovs-dev/2025-February/420400.html The relevant comment was: " > + * also not delete it even if we created it previously. */ Why not? ovn-controller owns that VRF, doesn't it? I mean, it created it so why not delete it if not needed anymore? > + sset_find_and_delete(&_maintained_vrfs, vrf_name); > + sset_find_and_delete(&old_maintained_vrfs, vrf_name); > + } " > > It's not a big deal for the test env, since other tests can use > VRF_RESERVE to cleanup system before they run, but it strikes me as odd > that the 1337 always persists, so probably worth looking into. > > Also small inconsistency, these tests use VRF tables 1337,1338 and > 1339, but they don't run VRF_RESERVE for all of them. > > Martin. > Regards, Dumitru > On Wed, 2025-01-29 at 12:15 +0100, Felix Huettner via dev wrote: >> When we stop ovn-controller without immediately restarting it we now >> cleanup routes. >> This allows the routing agents to stop advertising this chassis to >> the >> fabric. >> >> Signed-off-by: Felix Huettner <[email protected]> >> --- >> v3->v4: >> - addressed review comments. >> >> controller/route-exchange-netlink.c | 34 +++++++++++---- >> controller/route-exchange-netlink.h | 2 + >> controller/route-exchange.c | 66 >> +++++++++++++++++++++++++++- >> tests/system-ovn.at | 67 >> ++++++++++++++++++++++++++++- >> 4 files changed, 159 insertions(+), 10 deletions(-) >> >> diff --git a/controller/route-exchange-netlink.c b/controller/route- >> exchange-netlink.c >> index 6a9612a7e..93b3057c8 100644 >> --- a/controller/route-exchange-netlink.c >> +++ b/controller/route-exchange-netlink.c >> @@ -223,6 +223,9 @@ handle_route_msg(const struct route_table_msg >> *msg, void *data) >> >> /* This route is not from us, so we learn it. */ >> if (rd->rtm_protocol != RTPROT_OVN) { >> + if (!handle_data->learned_routes) { >> + return; >> + } >> if (prefix_is_link_local(&rd->rta_dst, rd->rtm_dst_len)) { >> return; >> } >> @@ -244,14 +247,16 @@ handle_route_msg(const struct route_table_msg >> *msg, void *data) >> return; >> } >> >> - struct hmapx_node *hn; >> - HMAPX_FOR_EACH_SAFE (hn, handle_data->routes_to_advertise) { >> - ar = hn->data; >> - if (ipv6_addr_equals(&ar->addr, &rd->rta_dst) >> - && ar->plen == rd->rtm_dst_len >> - && ar->priority == rd->rta_priority) { >> - hmapx_delete(handle_data->routes_to_advertise, hn); >> - return; >> + if (handle_data->routes_to_advertise) { >> + struct hmapx_node *hn; >> + HMAPX_FOR_EACH_SAFE (hn, handle_data->routes_to_advertise) { >> + ar = hn->data; >> + if (ipv6_addr_equals(&ar->addr, &rd->rta_dst) >> + && ar->plen == rd->rtm_dst_len >> + && ar->priority == rd->rta_priority) { >> + hmapx_delete(handle_data->routes_to_advertise, hn); >> + return; >> + } >> } >> } >> >> @@ -309,3 +314,16 @@ re_nl_sync_routes(uint32_t table_id, const >> struct hmap *routes, >> } >> hmapx_destroy(&routes_to_advertise); >> } >> + >> +void >> +re_nl_cleanup_routes(uint32_t table_id) >> +{ >> + /* Remove routes from the system that are not in the host_routes >> hmap and >> + * remove entries from host_routes hmap that match routes >> already installed >> + * in the system. */ >> + struct route_msg_handle_data data = { >> + .routes_to_advertise = NULL, >> + .learned_routes = NULL, >> + }; >> + route_table_dump_one_table(table_id, handle_route_msg, &data); >> +} >> diff --git a/controller/route-exchange-netlink.h b/controller/route- >> exchange-netlink.h >> index b930f05a2..7a3ae1dda 100644 >> --- a/controller/route-exchange-netlink.h >> +++ b/controller/route-exchange-netlink.h >> @@ -55,4 +55,6 @@ void re_nl_sync_routes(uint32_t table_id, const >> struct hmap *routes, >> struct ovs_list *learned_routes, >> const struct sbrec_datapath_binding *db); >> >> +void re_nl_cleanup_routes(uint32_t table_id); >> + >> #endif /* route-exchange-netlink.h */ >> diff --git a/controller/route-exchange.c b/controller/route- >> exchange.c >> index e7a85eecf..47ff2350e 100644 >> --- a/controller/route-exchange.c >> +++ b/controller/route-exchange.c >> @@ -19,6 +19,7 @@ >> >> #include <errno.h> >> #include <net/if.h> >> +#include <stdbool.h> >> >> #include "openvswitch/vlog.h" >> #include "openvswitch/list.h" >> @@ -37,6 +38,13 @@ >> VLOG_DEFINE_THIS_MODULE(route_exchange); >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); >> >> +struct maintained_route_table_entry { >> + struct hmap_node node; >> + uint32_t table_id; >> +}; >> + >> +static struct hmap _maintained_route_tables = HMAP_INITIALIZER( >> + &_maintained_route_tables); >> static struct sset _maintained_vrfs = >> SSET_INITIALIZER(&_maintained_vrfs); >> >> struct route_entry { >> @@ -45,6 +53,38 @@ struct route_entry { >> const struct sbrec_learned_route *sb_route; >> }; >> >> +static uint32_t >> +maintained_route_table_hash(uint32_t table_id) >> +{ >> + return hash_int(table_id, 0); >> +} >> + >> +static bool >> +maintained_route_table_contains(uint32_t table_id) >> +{ >> + uint32_t hash = maintained_route_table_hash(table_id); >> + struct maintained_route_table_entry *mrt; >> + HMAP_FOR_EACH_WITH_HASH (mrt, node, hash, >> + &_maintained_route_tables) { >> + if (mrt->table_id == table_id) { >> + return true; >> + } >> + } >> + return false; >> +} >> + >> +static void >> +maintained_route_table_add(uint32_t table_id) >> +{ >> + if (maintained_route_table_contains(table_id)) { >> + return; >> + } >> + uint32_t hash = maintained_route_table_hash(table_id); >> + struct maintained_route_table_entry *mrt = >> xzalloc(sizeof(*mrt)); >> + mrt->table_id = table_id; >> + hmap_insert(&_maintained_route_tables, &mrt->node, hash); >> +} >> + >> static struct route_entry * >> route_alloc_entry(struct hmap *routes, >> const struct sbrec_learned_route *sb_route) >> @@ -174,6 +214,9 @@ route_exchange_run(struct route_exchange_ctx_in >> *r_ctx_in, >> { >> struct sset old_maintained_vrfs = >> SSET_INITIALIZER(&old_maintained_vrfs); >> sset_swap(&_maintained_vrfs, &old_maintained_vrfs); >> + struct hmap old_maintained_route_table = HMAP_INITIALIZER( >> + &old_maintained_route_table); >> + hmap_swap(&_maintained_route_tables, >> &old_maintained_route_table); >> >> const struct advertise_datapath_entry *ad; >> HMAP_FOR_EACH (ad, node, r_ctx_in->announce_routes) { >> @@ -201,9 +244,10 @@ route_exchange_run(struct route_exchange_ctx_in >> *r_ctx_in, >> sset_find_and_delete(&old_maintained_vrfs, vrf_name); >> } >> >> + maintained_route_table_add(table_id); >> + >> struct ovs_list received_routes = OVS_LIST_INITIALIZER( >> &received_routes); >> - >> re_nl_sync_routes(ad->db->tunnel_key, &ad->routes, >> &received_routes, ad->db); >> >> @@ -218,6 +262,15 @@ route_exchange_run(struct route_exchange_ctx_in >> *r_ctx_in, >> re_nl_learned_routes_destroy(&received_routes); >> } >> >> + /* Remove routes in tables previously maintained by us. */ >> + struct maintained_route_table_entry *mrt; >> + HMAP_FOR_EACH_POP (mrt, node, &old_maintained_route_table) { >> + if (!maintained_route_table_contains(mrt->table_id)) { >> + re_nl_cleanup_routes(mrt->table_id); >> + } >> + free(mrt); >> + } >> + hmap_destroy(&old_maintained_route_table); >> >> /* Remove VRFs previously maintained by us not found in the >> above loop. */ >> const char *vrf_name; >> @@ -233,6 +286,11 @@ route_exchange_run(struct route_exchange_ctx_in >> *r_ctx_in, >> void >> route_exchange_cleanup_vrfs(void) >> { >> + struct maintained_route_table_entry *mrt; >> + HMAP_FOR_EACH (mrt, node, &_maintained_route_tables) { >> + re_nl_cleanup_routes(mrt->table_id); >> + } >> + >> const char *vrf_name; >> SSET_FOR_EACH (vrf_name, &_maintained_vrfs) { >> re_nl_delete_vrf(vrf_name); >> @@ -242,10 +300,16 @@ route_exchange_cleanup_vrfs(void) >> void >> route_exchange_destroy(void) >> { >> + struct maintained_route_table_entry *mrt; >> + HMAP_FOR_EACH_POP (mrt, node, &_maintained_route_tables) { >> + free(mrt); >> + } >> + >> const char *vrf_name; >> SSET_FOR_EACH_SAFE (vrf_name, &_maintained_vrfs) { >> sset_delete(&_maintained_vrfs, >> SSET_NODE_FROM_NAME(vrf_name)); >> } >> >> sset_destroy(&_maintained_vrfs); >> + hmap_destroy(&_maintained_route_tables); >> } >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> index 3f5672dad..963a67e0c 100644 >> --- a/tests/system-ovn.at >> +++ b/tests/system-ovn.at >> @@ -15090,8 +15090,39 @@ check ovn-nbctl --wait=hv set >> Logical_Router_Port internet-phys \ >> options:dynamic-routing-ifname=lo >> check_row_count Learned_Route 1 >> >> - >> +# Stopping the ovn-controller will clean up the route entries >> created by it. >> +# We first need to unset dynamic-routing-maintain-vrf as otherwise >> it will >> +# delete the whole vrf. >> +check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \ >> + options:dynamic-routing-maintain- >> vrf=false >> OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> +AT_CHECK([ip route list vrf ovnvrf1337 | awk '{$1=$1};1'], [0], [dnl >> +233.252.0.0/24 via 192.168.10.10 dev lo onlink >> +]) >> + >> +# Starting it again will add the routes again. >> +# The 2 sync commands ensure that we wait until the routes are >> actually >> +# installed. Otherwise this is racy. >> +start_daemon ovn-controller >> +OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" >> == "running"]) >> +OVS_WAIT_UNTIL_EQUAL([ip route list vrf ovnvrf1337 | awk >> '{$1=$1};1'], [dnl >> +blackhole 192.0.2.1 proto 84 metric 1000 >> +blackhole 192.0.2.2 proto 84 metric 100 >> +blackhole 192.0.2.3 proto 84 metric 100 >> +blackhole 192.0.2.10 proto 84 metric 100 >> +blackhole 198.51.100.0/24 proto 84 metric 1000 >> +233.252.0.0/24 via 192.168.10.10 dev lo onlink]) >> + >> +# Stoping with --restart will not touch the routes. >> +check ovn-appctl -t ovn-controller exit --restart >> +OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" >> != "running"]) >> +OVS_WAIT_UNTIL_EQUAL([ip route list vrf ovnvrf1337 | awk >> '{$1=$1};1'], [dnl >> +blackhole 192.0.2.1 proto 84 metric 1000 >> +blackhole 192.0.2.2 proto 84 metric 100 >> +blackhole 192.0.2.3 proto 84 metric 100 >> +blackhole 192.0.2.10 proto 84 metric 100 >> +blackhole 198.51.100.0/24 proto 84 metric 1000 >> +233.252.0.0/24 via 192.168.10.10 dev lo onlink]) >> >> as ovn-sb >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> @@ -15337,6 +15368,40 @@ check ovn-nbctl --wait=hv set >> Logical_Router_Port internet-phys \ >> options:dynamic-routing-ifname=lo >> check_row_count Learned_Route 1 >> >> +# Stopping the ovn-controller will clean up the route entries >> created by it. >> +# We first need to unset dynamic-routing-maintain-vrf as otherwise >> it will >> +# delete the whole vrf >> +check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \ >> + options:dynamic-routing-maintain- >> vrf=false >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) >> +AT_CHECK([ip route list vrf ovnvrf1337 | awk '{$1=$1};1'], [0], [dnl >> +233.252.0.0/24 via 192.168.10.10 dev lo onlink >> +]) >> + >> +# Starting it again will add the routes again. >> +# The 2 sync commands ensure that we wait until the routes are >> actually >> +# installed. Otherwise this is racy. >> +start_daemon ovn-controller >> +OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" >> == "running"]) >> +OVS_WAIT_UNTIL_EQUAL([ip route list vrf ovnvrf1337 | awk >> '{$1=$1};1'], [dnl >> +blackhole 192.0.2.1 proto 84 metric 100 >> +blackhole 192.0.2.2 proto 84 metric 100 >> +blackhole 192.0.2.3 proto 84 metric 100 >> +blackhole 192.0.2.10 proto 84 metric 100 >> +blackhole 198.51.100.0/24 proto 84 metric 1000 >> +233.252.0.0/24 via 192.168.10.10 dev lo onlink]) >> + >> +# Stoping with --restart will not touch the routes. >> +check ovn-appctl -t ovn-controller exit --restart >> +OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" >> != "running"]) >> +OVS_WAIT_UNTIL_EQUAL([ip route list vrf ovnvrf1337 | awk >> '{$1=$1};1'], [dnl >> +blackhole 192.0.2.1 proto 84 metric 100 >> +blackhole 192.0.2.2 proto 84 metric 100 >> +blackhole 192.0.2.3 proto 84 metric 100 >> +blackhole 192.0.2.10 proto 84 metric 100 >> +blackhole 198.51.100.0/24 proto 84 metric 1000 >> +233.252.0.0/24 via 192.168.10.10 dev lo onlink]) >> + >> as ovn-sb >> OVS_APP_EXIT_AND_WAIT([ovsdb-server]) >> > > _______________________________________________ > 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
