From 0c6f14ec145342ce793db8ae4efa31adc2972e21 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Fri, 24 May 2024 16:46:17 +0200 Subject: [PATCH 1/5] Revert "vtysh, zebra: Fix malformed json output for multiple vrfs in command 'show ip route vrf all json'" This reverts commit 0e2fc3d67f1d358896a764373f41cb59c095eda9. This fix was correct but not optimal for memory consumption at scale. Signed-off-by: Louis Scalbert --- zebra/zebra_vty.c | 81 +++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 52 deletions(-) diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index 44720754ba..cc2d9b2164 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -60,8 +60,8 @@ struct route_show_ctx { }; static int do_show_ip_route(struct vty *vty, const char *vrf_name, afi_t afi, - safi_t safi, bool use_fib, json_object *vrf_json, - bool use_json, route_tag_t tag, + safi_t safi, bool use_fib, bool use_json, + route_tag_t tag, const struct prefix *longer_prefix_p, bool supernets_only, int type, unsigned short ospf_instance_id, uint32_t tableid, @@ -153,8 +153,8 @@ DEFPY (show_ip_rpf, }; return do_show_ip_route(vty, VRF_DEFAULT_NAME, ip ? AFI_IP : AFI_IP6, - SAFI_MULTICAST, false, NULL, uj, 0, NULL, false, - 0, 0, 0, false, &ctx); + SAFI_MULTICAST, false, uj, 0, NULL, false, 0, 0, + 0, false, &ctx); } DEFPY (show_ip_rpf_addr, @@ -858,13 +858,14 @@ static void vty_show_ip_route_detail_json(struct vty *vty, vty_json(vty, json); } -static void -do_show_route_helper(struct vty *vty, struct zebra_vrf *zvrf, - struct route_table *table, afi_t afi, bool use_fib, - json_object *vrf_json, route_tag_t tag, - const struct prefix *longer_prefix_p, bool supernets_only, - int type, unsigned short ospf_instance_id, bool use_json, - uint32_t tableid, bool show_ng, struct route_show_ctx *ctx) +static void do_show_route_helper(struct vty *vty, struct zebra_vrf *zvrf, + struct route_table *table, afi_t afi, + bool use_fib, route_tag_t tag, + const struct prefix *longer_prefix_p, + bool supernets_only, int type, + unsigned short ospf_instance_id, bool use_json, + uint32_t tableid, bool show_ng, + struct route_show_ctx *ctx) { struct route_node *rn; struct route_entry *re; @@ -886,7 +887,7 @@ do_show_route_helper(struct vty *vty, struct zebra_vrf *zvrf, * => display the VRF and table if specific */ - if (use_json && !vrf_json) + if (use_json) json = json_object_new_object(); /* Show all routes. */ @@ -961,11 +962,7 @@ do_show_route_helper(struct vty *vty, struct zebra_vrf *zvrf, if (json_prefix) { prefix2str(&rn->p, buf, sizeof(buf)); - if (!vrf_json) - json_object_object_add(json, buf, json_prefix); - else - json_object_object_add(vrf_json, buf, - json_prefix); + json_object_object_add(json, buf, json_prefix); json_prefix = NULL; } } @@ -974,15 +971,13 @@ do_show_route_helper(struct vty *vty, struct zebra_vrf *zvrf, * This is an extremely expensive operation at scale * and non-pretty reduces memory footprint significantly. */ - if (use_json && !vrf_json) { + if (use_json) vty_json_no_pretty(vty, json); - json = NULL; - } } static void do_show_ip_route_all(struct vty *vty, struct zebra_vrf *zvrf, - afi_t afi, bool use_fib, json_object *vrf_json, - bool use_json, route_tag_t tag, + afi_t afi, bool use_fib, bool use_json, + route_tag_t tag, const struct prefix *longer_prefix_p, bool supernets_only, int type, unsigned short ospf_instance_id, bool show_ng, @@ -1002,15 +997,15 @@ static void do_show_ip_route_all(struct vty *vty, struct zebra_vrf *zvrf, continue; do_show_ip_route(vty, zvrf_name(zvrf), afi, SAFI_UNICAST, - use_fib, vrf_json, use_json, tag, - longer_prefix_p, supernets_only, type, - ospf_instance_id, zrt->tableid, show_ng, ctx); + use_fib, use_json, tag, longer_prefix_p, + supernets_only, type, ospf_instance_id, + zrt->tableid, show_ng, ctx); } } static int do_show_ip_route(struct vty *vty, const char *vrf_name, afi_t afi, - safi_t safi, bool use_fib, json_object *vrf_json, - bool use_json, route_tag_t tag, + safi_t safi, bool use_fib, bool use_json, + route_tag_t tag, const struct prefix *longer_prefix_p, bool supernets_only, int type, unsigned short ospf_instance_id, uint32_t tableid, @@ -1045,7 +1040,7 @@ static int do_show_ip_route(struct vty *vty, const char *vrf_name, afi_t afi, return CMD_SUCCESS; } - do_show_route_helper(vty, zvrf, table, afi, use_fib, vrf_json, tag, + do_show_route_helper(vty, zvrf, table, afi, use_fib, tag, longer_prefix_p, supernets_only, type, ospf_instance_id, use_json, tableid, show_ng, ctx); @@ -1750,7 +1745,6 @@ DEFPY (show_route, struct route_show_ctx ctx = { .multi = vrf_all || table_all, }; - json_object *root_json = NULL; if (!vrf_is_backend_netns()) { if ((vrf_all || vrf_name) && (table || table_all)) { @@ -1772,42 +1766,25 @@ DEFPY (show_route, } if (vrf_all) { - if (!!json) - root_json = json_object_new_object(); RB_FOREACH (vrf, vrf_name_head, &vrfs_by_name) { - json_object *vrf_json = NULL; - if ((zvrf = vrf->info) == NULL || (zvrf->table[afi][SAFI_UNICAST] == NULL)) continue; - if (!!json) - vrf_json = json_object_new_object(); - if (table_all) do_show_ip_route_all(vty, zvrf, afi, !!fib, - vrf_json, !!json, tag, + !!json, tag, prefix_str ? prefix : NULL, !!supernets_only, type, ospf_instance_id, !!ng, &ctx); else do_show_ip_route(vty, zvrf_name(zvrf), afi, - SAFI_UNICAST, !!fib, vrf_json, - !!json, tag, - prefix_str ? prefix : NULL, + SAFI_UNICAST, !!fib, !!json, + tag, prefix_str ? prefix : NULL, !!supernets_only, type, ospf_instance_id, table, !!ng, &ctx); - - if (!!json) - json_object_object_add(root_json, - zvrf_name(zvrf), - vrf_json); - } - if (!!json) { - vty_json_no_pretty(vty, root_json); - root_json = NULL; } } else { vrf_id_t vrf_id = VRF_DEFAULT; @@ -1823,13 +1800,13 @@ DEFPY (show_route, return CMD_SUCCESS; if (table_all) - do_show_ip_route_all(vty, zvrf, afi, !!fib, NULL, !!json, - tag, prefix_str ? prefix : NULL, + do_show_ip_route_all(vty, zvrf, afi, !!fib, !!json, tag, + prefix_str ? prefix : NULL, !!supernets_only, type, ospf_instance_id, !!ng, &ctx); else do_show_ip_route(vty, vrf->name, afi, SAFI_UNICAST, - !!fib, NULL, !!json, tag, + !!fib, !!json, tag, prefix_str ? prefix : NULL, !!supernets_only, type, ospf_instance_id, table, !!ng, &ctx); From 03b1ee7a39d1ac166f4dff62c75374f85029c918 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Mon, 27 May 2024 10:04:14 +0200 Subject: [PATCH 2/5] lib: add helpers to print json keys Add helpers to print json keys in order to prepare the next commits. Signed-off-by: Louis Scalbert --- lib/vty.c | 15 +++++++++++++++ lib/vty.h | 2 ++ 2 files changed, 17 insertions(+) diff --git a/lib/vty.c b/lib/vty.c index 628c694e95..0dcd118a97 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -390,6 +390,21 @@ int vty_json_no_pretty(struct vty *vty, struct json_object *json) return vty_json_helper(vty, json, JSON_C_TO_STRING_NOSLASHESCAPE); } + +void vty_json_key(struct vty *vty, const char *key, bool *first_key) +{ + vty_out(vty, "%s\"%s\":", *first_key ? "{" : ",", key); + *first_key = false; +} + +void vty_json_close(struct vty *vty, bool first_key) +{ + if (first_key) + /* JSON was not opened */ + vty_out(vty, "{"); + vty_out(vty, "}\n"); +} + void vty_json_empty(struct vty *vty, struct json_object *json) { json_object *jsonobj = json; diff --git a/lib/vty.h b/lib/vty.h index a9570ef048..c336a816cc 100644 --- a/lib/vty.h +++ b/lib/vty.h @@ -378,6 +378,8 @@ extern bool vty_set_include(struct vty *vty, const char *regexp); */ extern int vty_json(struct vty *vty, struct json_object *json); extern int vty_json_no_pretty(struct vty *vty, struct json_object *json); +void vty_json_key(struct vty *vty, const char *key, bool *first_key); +void vty_json_close(struct vty *vty, bool first_key); extern void vty_json_empty(struct vty *vty, struct json_object *json); /* post fd to be passed to the vtysh client * fd is owned by the VTY code after this and will be closed when done From cb440058f2d85d1408f932ad820c38e8e1b17191 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Fri, 24 May 2024 17:06:59 +0200 Subject: [PATCH 3/5] zebra: fix show route vrf all memory consumption 0e2fc3d67f ("vtysh, zebra: Fix malformed json output for multiple vrfs in command 'show ip route vrf all json'") has been reverted in the previous commit. Although the fix was correct, it was consuming too muca memory when displaying large route tables. A root JSON object was storing all the JSON objects containing the route tables, each containing their respective prefixes in JSON objects. This resulted in excessive memory allocation for JSON objects, potentially leading to an out-of-memory error on the machine. To Fix the memory consumption issue for the "show ip[v6] route vrf all json" command, display the tables one by one and free the memory of each JSON object after it has been displayed. Signed-off-by: Louis Scalbert --- zebra/zebra_vty.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index cc2d9b2164..97245d55b9 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -1739,6 +1739,7 @@ DEFPY (show_route, "Nexthop Group Information\n") { afi_t afi = ipv4 ? AFI_IP : AFI_IP6; + bool first_vrf_json = true; struct vrf *vrf; int type = 0; struct zebra_vrf *zvrf; @@ -1770,7 +1771,9 @@ DEFPY (show_route, if ((zvrf = vrf->info) == NULL || (zvrf->table[afi][SAFI_UNICAST] == NULL)) continue; - + if (json) + vty_json_key(vty, zvrf_name(zvrf), + &first_vrf_json); if (table_all) do_show_ip_route_all(vty, zvrf, afi, !!fib, !!json, tag, @@ -1786,6 +1789,8 @@ DEFPY (show_route, ospf_instance_id, table, !!ng, &ctx); } + if (json) + vty_json_close(vty, first_vrf_json); } else { vrf_id_t vrf_id = VRF_DEFAULT; From 85eb60ffd6ce40428c3a99aeddf46b1b60d2e0a5 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Fri, 24 May 2024 16:34:23 +0200 Subject: [PATCH 4/5] zebra: fix show route memory consumption When displaying a route table in JSON, a table JSON object is storing all the prefix JSON objects containing the prefix information. This results in excessive memory allocation for JSON objects, potentially leading to an out-of-memory error on the machine with large routing tables. To Fix the memory consumption issue for the "show ip[v6] route [vrf XX] json" command, display the prefixes one by one and free the memory of each JSON object after it has been displayed. Signed-off-by: Louis Scalbert --- zebra/zebra_vty.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index 97245d55b9..c31218a7c3 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -869,9 +869,9 @@ static void do_show_route_helper(struct vty *vty, struct zebra_vrf *zvrf, { struct route_node *rn; struct route_entry *re; + bool first_json = true; int first = 1; rib_dest_t *dest; - json_object *json = NULL; json_object *json_prefix = NULL; uint32_t addr; char buf[BUFSIZ]; @@ -887,9 +887,6 @@ static void do_show_route_helper(struct vty *vty, struct zebra_vrf *zvrf, * => display the VRF and table if specific */ - if (use_json) - json = json_object_new_object(); - /* Show all routes. */ for (rn = route_top(table); rn; rn = srcdest_route_next(rn)) { dest = rib_dest_from_rnode(rn); @@ -962,17 +959,15 @@ static void do_show_route_helper(struct vty *vty, struct zebra_vrf *zvrf, if (json_prefix) { prefix2str(&rn->p, buf, sizeof(buf)); - json_object_object_add(json, buf, json_prefix); + vty_json_key(vty, buf, &first_json); + vty_json_no_pretty(vty, json_prefix); + json_prefix = NULL; } } - /* - * This is an extremely expensive operation at scale - * and non-pretty reduces memory footprint significantly. - */ if (use_json) - vty_json_no_pretty(vty, json); + vty_json_close(vty, first_json); } static void do_show_ip_route_all(struct vty *vty, struct zebra_vrf *zvrf, From 2d6dcc0c57ecb4a8d28bf8671e67c3e292f7adad Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Mon, 27 May 2024 10:35:26 +0200 Subject: [PATCH 5/5] tests: check show route vrf all json output Check that "show ip route vrf XXX json" and the JSON at key "XXX" of "show ip route vrf all json" gives the same output. Signed-off-by: Louis Scalbert --- .../test_bgp-vrf-route-leak-basic.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/topotests/bgp_vrf_route_leak_basic/test_bgp-vrf-route-leak-basic.py b/tests/topotests/bgp_vrf_route_leak_basic/test_bgp-vrf-route-leak-basic.py index 3938a966ee..6d4b436bcc 100644 --- a/tests/topotests/bgp_vrf_route_leak_basic/test_bgp-vrf-route-leak-basic.py +++ b/tests/topotests/bgp_vrf_route_leak_basic/test_bgp-vrf-route-leak-basic.py @@ -338,6 +338,21 @@ interface EVA result, diff = topotest.run_and_expect(test_func, None, count=10, wait=0.5) assert result, "BGP VRF DONNA check failed:\n{}".format(diff) + """ + Check that "show ip route vrf DONNA json" and the JSON at key "DONNA" of + "show ip route vrf all json" gives the same result. + """ + + def check_vrf_table(router, vrf, expect): + output = router.vtysh_cmd("show ip route vrf all json", isjson=True) + vrf_table = output.get(vrf, {}) + + return topotest.json_cmp(vrf_table, expect) + + test_func = partial(check_vrf_table, r1, "DONNA", expect) + result, diff = topotest.run_and_expect(test_func, None, count=10, wait=0.5) + assert result, "BGP VRF DONNA check failed:\n{}".format(diff) + def test_vrf_route_leak_donna_after_eva_up(): logger.info("Ensure that route states change after EVA interface goes up")