-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Nhg ip #5669
Nhg ip #5669
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/daf918bcaf3d1bbca364bf85a849ddb2/raw/47b974f2be61f7bcec9008dda0b54ff34f21f45e/cr_5669_1578950346.diff | git apply
diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c
index de98ad629..41fa8c1cc 100644
--- a/zebra/zebra_vty.c
+++ b/zebra/zebra_vty.c
@@ -1386,25 +1386,22 @@ static int show_nexthop_group_helper(struct vty *vty, uint32_t id,
return CMD_SUCCESS;
}
-DEFPY (show_nexthop_group_vrf,
- show_nexthop_group_vrf_cmd,
- "show nexthop-group rib <(0-4294967295)$id [vrf <NAME$vrf_name|all$vrf_all>]>",
- SHOW_STR
- "Show Nexthop Groups\n"
- "RIB information\n"
- "Nexthop Group ID\n"
- VRF_FULL_CMD_HELP_STR)
+DEFPY(show_nexthop_group_vrf, show_nexthop_group_vrf_cmd,
+ "show nexthop-group rib <(0-4294967295)$id [vrf <NAME$vrf_name|all$vrf_all>]>",
+ SHOW_STR
+ "Show Nexthop Groups\n"
+ "RIB information\n"
+ "Nexthop Group ID\n" VRF_FULL_CMD_HELP_STR)
{
return show_nexthop_group_helper(vty, id, vrf_all, vrf_name);
}
-DEFPY (show_nexthop_group,
- show_nexthop_group_cmd,
- "show nexthop-group rib <(0-4294967295)$id>",
- SHOW_STR
- "Show Nexthop Groups\n"
- "RIB information\n"
- "Nexthop Group ID\n")
+DEFPY(show_nexthop_group, show_nexthop_group_cmd,
+ "show nexthop-group rib <(0-4294967295)$id>",
+ SHOW_STR
+ "Show Nexthop Groups\n"
+ "RIB information\n"
+ "Nexthop Group ID\n")
{
return show_nexthop_group_helper(vty, id, NULL, VRF_DEFAULT_NAME);
}
If you are a new contributor to FRR, please see our contributing guidelines.
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedCentOS 7 rpm pkg check: Failed (click for details)CentOS 7 rpm pkg check: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10384/artifact/CENTOS7RPM/ErrorLog/log_restart.txt CentOS 7 rpm pkg check: No useful log foundIPv4 ldp protocol on Ubuntu 16.04: Failed (click for details)Successful on other platforms
CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/87c0c5acaa8ac0e3bca67b071565765c/raw/47b974f2be61f7bcec9008dda0b54ff34f21f45e/cr_5669_1579094938.diff | git apply
diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c
index de98ad629..41fa8c1cc 100644
--- a/zebra/zebra_vty.c
+++ b/zebra/zebra_vty.c
@@ -1386,25 +1386,22 @@ static int show_nexthop_group_helper(struct vty *vty, uint32_t id,
return CMD_SUCCESS;
}
-DEFPY (show_nexthop_group_vrf,
- show_nexthop_group_vrf_cmd,
- "show nexthop-group rib <(0-4294967295)$id [vrf <NAME$vrf_name|all$vrf_all>]>",
- SHOW_STR
- "Show Nexthop Groups\n"
- "RIB information\n"
- "Nexthop Group ID\n"
- VRF_FULL_CMD_HELP_STR)
+DEFPY(show_nexthop_group_vrf, show_nexthop_group_vrf_cmd,
+ "show nexthop-group rib <(0-4294967295)$id [vrf <NAME$vrf_name|all$vrf_all>]>",
+ SHOW_STR
+ "Show Nexthop Groups\n"
+ "RIB information\n"
+ "Nexthop Group ID\n" VRF_FULL_CMD_HELP_STR)
{
return show_nexthop_group_helper(vty, id, vrf_all, vrf_name);
}
-DEFPY (show_nexthop_group,
- show_nexthop_group_cmd,
- "show nexthop-group rib <(0-4294967295)$id>",
- SHOW_STR
- "Show Nexthop Groups\n"
- "RIB information\n"
- "Nexthop Group ID\n")
+DEFPY(show_nexthop_group, show_nexthop_group_cmd,
+ "show nexthop-group rib <(0-4294967295)$id>",
+ SHOW_STR
+ "Show Nexthop Groups\n"
+ "RIB information\n"
+ "Nexthop Group ID\n")
{
return show_nexthop_group_helper(vty, id, NULL, VRF_DEFAULT_NAME);
}
If you are a new contributor to FRR, please see our contributing guidelines.
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10425/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/699d9a650be1f6c09175118c32e97588/raw/47b974f2be61f7bcec9008dda0b54ff34f21f45e/cr_5669_1579227144.diff | git apply
diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c
index de98ad629..41fa8c1cc 100644
--- a/zebra/zebra_vty.c
+++ b/zebra/zebra_vty.c
@@ -1386,25 +1386,22 @@ static int show_nexthop_group_helper(struct vty *vty, uint32_t id,
return CMD_SUCCESS;
}
-DEFPY (show_nexthop_group_vrf,
- show_nexthop_group_vrf_cmd,
- "show nexthop-group rib <(0-4294967295)$id [vrf <NAME$vrf_name|all$vrf_all>]>",
- SHOW_STR
- "Show Nexthop Groups\n"
- "RIB information\n"
- "Nexthop Group ID\n"
- VRF_FULL_CMD_HELP_STR)
+DEFPY(show_nexthop_group_vrf, show_nexthop_group_vrf_cmd,
+ "show nexthop-group rib <(0-4294967295)$id [vrf <NAME$vrf_name|all$vrf_all>]>",
+ SHOW_STR
+ "Show Nexthop Groups\n"
+ "RIB information\n"
+ "Nexthop Group ID\n" VRF_FULL_CMD_HELP_STR)
{
return show_nexthop_group_helper(vty, id, vrf_all, vrf_name);
}
-DEFPY (show_nexthop_group,
- show_nexthop_group_cmd,
- "show nexthop-group rib <(0-4294967295)$id>",
- SHOW_STR
- "Show Nexthop Groups\n"
- "RIB information\n"
- "Nexthop Group ID\n")
+DEFPY(show_nexthop_group, show_nexthop_group_cmd,
+ "show nexthop-group rib <(0-4294967295)$id>",
+ SHOW_STR
+ "Show Nexthop Groups\n"
+ "RIB information\n"
+ "Nexthop Group ID\n")
{
return show_nexthop_group_helper(vty, id, NULL, VRF_DEFAULT_NAME);
}
If you are a new contributor to FRR, please see our contributing guidelines.
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopology tests on Ubuntu 16.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-10472/test Topology Tests failed for Topology tests on Ubuntu 16.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10472/artifact/TOPOU1604/ErrorLog/log_topotests.txt Topotest tests on Ubuntu 16.04 i386: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-10472/test Topology Tests failed for Topotest tests on Ubuntu 16.04 i386:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10472/artifact/TOPOI386/ErrorLog/log_topotests.txt Topology tests on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-10472/test Topology Tests failed for Topology tests on Ubuntu 18.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10472/artifact/TOPOU1804/ErrorLog/log_topotests.txt Successful on other platforms
Topology Tests memory analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10472/artifact/TOPOU1604/MemoryLeaks/
|
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopology tests on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-10486/test Topology Tests failed for Topology tests on Ubuntu 18.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10486/artifact/TOPOU1804/ErrorLog/log_topotests.txt Topology tests on Ubuntu 16.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-10486/test Topology Tests failed for Topology tests on Ubuntu 16.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10486/artifact/TOPOU1604/ErrorLog/log_topotests.txt Topotest tests on Ubuntu 16.04 i386: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-10486/test Topology Tests failed for Topotest tests on Ubuntu 16.04 i386:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10486/artifact/TOPOI386/ErrorLog/log_topotests.txt Successful on other platforms
Topology Tests memory analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10486/artifact/TOPOU1804/MemoryLeaks/
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/9e23a2903ea4edfa63633edb5da550a5/raw/ac828f29ff2f191be5dbde455472d5d4ca681e74/cr_5669_1580235866.diff | git apply
diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c
index 9858a39fa..16abbacc7 100644
--- a/zebra/zebra_vty.c
+++ b/zebra/zebra_vty.c
@@ -1345,14 +1345,12 @@ DEFPY (show_interface_nexthop_group,
return CMD_SUCCESS;
}
-DEFPY (show_nexthop_group,
- show_nexthop_group_cmd,
- "show nexthop-group rib <(0-4294967295)$id [vrf <NAME$vrf_name|all$vrf_all>]>",
- SHOW_STR
- "Show Nexthop Groups\n"
- "RIB information\n"
- "Nexthop Group ID\n"
- VRF_FULL_CMD_HELP_STR)
+DEFPY(show_nexthop_group, show_nexthop_group_cmd,
+ "show nexthop-group rib <(0-4294967295)$id [vrf <NAME$vrf_name|all$vrf_all>]>",
+ SHOW_STR
+ "Show Nexthop Groups\n"
+ "RIB information\n"
+ "Nexthop Group ID\n" VRF_FULL_CMD_HELP_STR)
{
struct zebra_vrf *zvrf = NULL;
@@ -1361,7 +1359,8 @@ DEFPY (show_nexthop_group,
return show_nexthop_group_id_cmd_helper(vty, id);
if (vrf_is_backend_netns() && (vrf_name || vrf_all)) {
- vty_out(vty, "VRF subcommand does not make any sense in l3mdev based vrf's");
+ vty_out(vty,
+ "VRF subcommand does not make any sense in l3mdev based vrf's");
return CMD_WARNING;
}
If you are a new contributor to FRR, please see our contributing guidelines.
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10553/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
else if (v6) | ||
afi = AFI_IP6; | ||
if (vrf_is_backend_netns() && (vrf_name || vrf_all)) { | ||
vty_out(vty, "VRF subcommand does not make any sense in l3mdev based vrf's"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a terminating newline? and aren't errors usually preceded with "%%
- or is that not a convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not a convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't a newline ... reasonable?
the bigger question is the one about the treatment of singleton nhes - it does seem (to me) ok to want to see them by vrf. is there a specific problem introduced by offering that filter?
@@ -591,10 +589,11 @@ zebra_nhg_find_nexthop(uint32_t id, struct nexthop *nh, afi_t afi, int type) | |||
{ | |||
struct nhg_hash_entry *nhe = NULL; | |||
struct nexthop_group nhg = {}; | |||
vrf_id_t vrf_id = !vrf_is_backend_netns() ? VRF_DEFAULT : nh->vrf_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble following this: don't we have to include the nexthop's vrf in this lookup? I agree that it's awkward that sometimes there's a value in an nhe that's copied out of a single nexthop, and sometimes not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nexthop groups are namespace based not vrf based. So if you are not using namespaced backends for a vrf you need to make this distinction. Else you end up with a some scenarios where we are reporting a nexthop group in VRF X and that means nothing if you are using l3mdev's. I would rather just say they are in the default vrf.
offline conversation w/ Mark summary:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/c5227a50146358446abae690152a0c71/raw/ffbada57a090afc9d033dab963e618c3a7796a93/cr_5669_1580403541.diff | git apply
diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c
index 78001da17..101f0e8bc 100644
--- a/zebra/zebra_vty.c
+++ b/zebra/zebra_vty.c
@@ -1349,17 +1349,13 @@ DEFPY (show_interface_nexthop_group,
return CMD_SUCCESS;
}
-DEFPY (show_nexthop_group,
- show_nexthop_group_cmd,
- "show nexthop-group rib <(0-4294967295)$id|[singleton <ip$v4|ipv6$v6>] [vrf <NAME$vrf_name|all$vrf_all>]>",
- SHOW_STR
- "Show Nexthop Groups\n"
- "RIB information\n"
- "Nexthop Group ID\n"
- "Show Singleton Nexthop-Groups\n"
- IP_STR
- IP6_STR
- VRF_FULL_CMD_HELP_STR)
+DEFPY(show_nexthop_group, show_nexthop_group_cmd,
+ "show nexthop-group rib <(0-4294967295)$id|[singleton <ip$v4|ipv6$v6>] [vrf <NAME$vrf_name|all$vrf_all>]>",
+ SHOW_STR
+ "Show Nexthop Groups\n"
+ "RIB information\n"
+ "Nexthop Group ID\n"
+ "Show Singleton Nexthop-Groups\n" IP_STR IP6_STR VRF_FULL_CMD_HELP_STR)
{
struct zebra_vrf *zvrf = NULL;
@@ -1374,7 +1370,8 @@ DEFPY (show_nexthop_group,
afi = AFI_IP6;
if (vrf_is_backend_netns() && (vrf_name || vrf_all)) {
- vty_out(vty, "VRF subcommand does not make any sense in l3mdev based vrf's");
+ vty_out(vty,
+ "VRF subcommand does not make any sense in l3mdev based vrf's");
return CMD_WARNING;
}
If you are a new contributor to FRR, please see our contributing guidelines.
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 11 amd64 build: Failed (click for details)FreeBSD 11 amd64 build: No useful log foundSuccessful on other platforms
Warnings Generated during build:Checkout code: Successful with additional warningsFreeBSD 11 amd64 build: Failed (click for details)FreeBSD 11 amd64 build: No useful log found
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
ci:rerun |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10570/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10568/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
The zebra implementation of nexthop groups has two types of nexthops groups currently. Singleton objects which have afi's and combined nexthop groups that do not. Specifically call this out in the code to make this distinction. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Nexthop groups as a whole do not make sense to have a vrf'ness As that you can have a arbitrary number of nexthops that point to separate vrf's. Modify the code to make this distinction, by clearly delineating the line between the nhg and the nexthop a bit better. Nexthop groups having a vrf_id only make sense if you are using network namespaces to represent them. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/840d3830238356a6711635ae6de41502/raw/ffbada57a090afc9d033dab963e618c3a7796a93/cr_5669_1580478366.diff | git apply
diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c
index 78001da17..101f0e8bc 100644
--- a/zebra/zebra_vty.c
+++ b/zebra/zebra_vty.c
@@ -1349,17 +1349,13 @@ DEFPY (show_interface_nexthop_group,
return CMD_SUCCESS;
}
-DEFPY (show_nexthop_group,
- show_nexthop_group_cmd,
- "show nexthop-group rib <(0-4294967295)$id|[singleton <ip$v4|ipv6$v6>] [vrf <NAME$vrf_name|all$vrf_all>]>",
- SHOW_STR
- "Show Nexthop Groups\n"
- "RIB information\n"
- "Nexthop Group ID\n"
- "Show Singleton Nexthop-Groups\n"
- IP_STR
- IP6_STR
- VRF_FULL_CMD_HELP_STR)
+DEFPY(show_nexthop_group, show_nexthop_group_cmd,
+ "show nexthop-group rib <(0-4294967295)$id|[singleton <ip$v4|ipv6$v6>] [vrf <NAME$vrf_name|all$vrf_all>]>",
+ SHOW_STR
+ "Show Nexthop Groups\n"
+ "RIB information\n"
+ "Nexthop Group ID\n"
+ "Show Singleton Nexthop-Groups\n" IP_STR IP6_STR VRF_FULL_CMD_HELP_STR)
{
struct zebra_vrf *zvrf = NULL;
@@ -1374,7 +1370,8 @@ DEFPY (show_nexthop_group,
afi = AFI_IP6;
if (vrf_is_backend_netns() && (vrf_name || vrf_all)) {
- vty_out(vty, "VRF subcommand does not make any sense in l3mdev based vrf's");
+ vty_out(vty,
+ "VRF subcommand does not make any sense in l3mdev based vrf's");
return CMD_WARNING;
}
If you are a new contributor to FRR, please see our contributing guidelines.
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-10580/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
clang_check |
No description provided.