Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zebra: Don't display the vrf if not using namespace based vrfs #16750

Merged
merged 2 commits into from
Nov 5, 2024
Merged

zebra: Don't display the vrf if not using namespace based vrfs #16750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f51d2a6
zebra: Don't display the vrf if not using namespace based vrfs
donaldsharp Sep 5, 2024
e88cbd6
zebra: Remove large indentation level in do_show_route_helper
donaldsharp Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions zebra/table_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ extern "C" {
#if !defined(GNU_LINUX)
/* BSD systems
*/
#define RT_TABLE_ID_MAIN 0
#else
/* Linux Systems
*/
Expand Down
2 changes: 2 additions & 0 deletions zebra/zebra_vrf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ vrf_id_t zebra_vrf_lookup_by_table(uint32_t table_id, ns_id_t ns_id)

RB_FOREACH (vrf, vrf_id_head, &vrfs_by_id) {
zvrf = vrf->info;

if (zvrf == NULL)
continue;
/* case vrf with netns : match the netnsid */
Expand All @@ -408,6 +409,7 @@ vrf_id_t zebra_vrf_lookup_by_table(uint32_t table_id, ns_id_t ns_id)
/* VRF is VRF_BACKEND_VRF_LITE */
if (zvrf->table_id != table_id)
continue;

return zvrf_id(zvrf);
}
}
Expand Down
35 changes: 24 additions & 11 deletions zebra/zebra_vty.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,27 @@ static void vty_show_ip_route_detail_json(struct vty *vty,
vty_json(vty, json);
}

static void zebra_vty_display_vrf_header(struct vty *vty, struct zebra_vrf *zvrf, uint32_t tableid)
{
if (!tableid)
vty_out(vty, "VRF %s:\n", zvrf_name(zvrf));
else {
if (vrf_is_backend_netns())
vty_out(vty, "VRF %s table %u:\n", zvrf_name(zvrf), tableid);
else {
vrf_id_t vrf = zebra_vrf_lookup_by_table(tableid, zvrf->zns->ns_id);

if (vrf == VRF_DEFAULT && tableid != RT_TABLE_ID_MAIN)
vty_out(vty, "table %u:\n", tableid);
else {
struct zebra_vrf *zvrf2 = zebra_vrf_lookup_by_id(vrf);

vty_out(vty, "VRF %s table %u:\n", zvrf_name(zvrf2), tableid);
}
}
}
}

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,
Expand Down Expand Up @@ -937,17 +958,9 @@ static void do_show_route_helper(struct vty *vty, struct zebra_vrf *zvrf,
}
if (ctx->multi && ctx->header_done)
vty_out(vty, "\n");
if (ctx->multi || zvrf_id(zvrf) != VRF_DEFAULT
|| tableid) {
if (!tableid)
vty_out(vty, "VRF %s:\n",
zvrf_name(zvrf));
else
vty_out(vty,
"VRF %s table %u:\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm - maybe I'm naive, but ... when I was poking around with this stuff, I thought it was nice that the vrf/table relationship was being displayed - it was just wrong sometimes. would it be bad to show the vrf each time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tables have a dual nature in linux. It's a table and that table also can be a vrf. I don't think that it's a distinction that we should be making, though. If I have table 99 and it also is being used by vrf GREEN. I don't think when I say display table 99 that we should say it's vrf GREEN in that case, the operator didn't ask for that data. I don't think it makes sense in the l3mdev based vrf case that we should be displaying anything about a vrf when you display a specific table.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I say, maybe I'm being naive - but aren't there just two possibilities? either a table is in the default vrf, or it's in a different vrf. why not show that association? we're willing to show the vrf and table in the output of show vrf, and that seems ... fine. why not here too?

Copy link
Member

@Jafaral Jafaral Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda like the fact that if the table happens to be a vrf, then show me that info too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an important piece of info. I am with @mjstapp here, I think we should keep it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't do that Jafar. If using l3mdev it always displays Default. My point is that default is nonsensical. The fact that we are asking about a table means that we are not talking about vrfs at all. In linux they just happen to have taken the l3mdev device and overlayed it over the table id space. But table 3939 is not in the default vrf, as that it has no relation to the default vrf at all, hence my removal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gosh - we seem to be crossed-up here.
none of us thinks it's correct to print "default vrf" when ... it's not - but it would be nice to print the correct vrf, whether default or not (imo).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, the operator has asked for a table not a vrf in this case. Don't display anything about a vrf then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropping default, and keeping the correct vrf when there is actually one is the right approach IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

zvrf_name(zvrf),
tableid);
}
if (ctx->multi || zvrf_id(zvrf) != VRF_DEFAULT || tableid)
zebra_vty_display_vrf_header(vty, zvrf, tableid);

ctx->header_done = true;
first = 0;
}
Expand Down
Loading