On 1/29/25 12:15 PM, 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]>
> ---
Hi Felix,
This patch looks OK to me (except the few small nits I mentioned below).
I won't ack it in this version though because we probably need to
figure out the reason why some vrfs don't get cleaned up in the system
tests (reported by Martin earlier on this thread).
In any case, I'll have another look at this patch in v6.
Thanks,
Dumitru
> 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);
Nit: I think I prefer:
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);
Similar here:
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
