Nhg ip #5669
Merged
Nhg ip #5669
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
polychaeta
reviewed
Jan 13, 2020
There was a problem hiding this comment.
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 |
polychaeta
reviewed
Jan 15, 2020
There was a problem hiding this comment.
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 |
polychaeta
reviewed
Jan 17, 2020
There was a problem hiding this comment.
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/
|
riw777
approved these changes
Jan 28, 2020
polychaeta
reviewed
Jan 28, 2020
There was a problem hiding this comment.
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 warningsWarnings 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 |
mjstapp
reviewed
Jan 30, 2020
| 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.
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.
it's not a convention.
There was a problem hiding this comment.
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.
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.
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:
|
polychaeta
reviewed
Jan 30, 2020
There was a problem hiding this comment.
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 foundWarnings 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 warningsCLANG 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 warningsWarnings 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>
polychaeta
reviewed
Jan 31, 2020
There was a problem hiding this comment.
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 warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: clang_check |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.