-
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
isisd: add support for Topology Independent LFA (TI-LFA) #7011
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/8cd3d840373cbef517dbfb08b266f2d1/raw/44e1d51394644b0cceb2b2ed77b3c3fb65b985db/cr_7011_1598538347.diff | git apply
diff --git a/isisd/isis_route.h b/isisd/isis_route.h
index 611756742..fbb548a79 100644
--- a/isisd/isis_route.h
+++ b/isisd/isis_route.h
@@ -66,8 +66,7 @@ struct isis_route_info *isis_route_create(struct prefix *prefix,
/* Walk the given table and install new routes to zebra and remove old ones.
* route status is tracked using ISIS_ROUTE_FLAG_ACTIVE */
-void isis_route_verify_table(struct isis_area *area,
- struct route_table *table,
+void isis_route_verify_table(struct isis_area *area, struct route_table *table,
struct route_table *table_backup);
/* Same as isis_route_verify_table, but merge L1 and L2 routes before */
diff --git a/isisd/isis_sr.c b/isisd/isis_sr.c
index 02adad170..66cfd7ba8 100644
--- a/isisd/isis_sr.c
+++ b/isisd/isis_sr.c
@@ -1584,7 +1584,8 @@ void sr_adj_sid_add_single(struct isis_adjacency *adj, int family, bool backup,
sra->backup_nexthops = list_new();
for (ALL_LIST_ELEMENTS_RO(nexthops, node, vadj)) {
struct isis_adjacency *adj = vadj->sadj->adj;
- struct mpls_label_stack *label_stack = vadj->label_stack;
+ struct mpls_label_stack *label_stack =
+ vadj->label_stack;
adjinfo2nexthop(family, sra->backup_nexthops, adj,
label_stack);
diff --git a/isisd/isisd.c b/isisd/isisd.c
index 753fea516..f578dfcb4 100644
--- a/isisd/isisd.c
+++ b/isisd/isisd.c
@@ -1397,12 +1397,8 @@ DEFUN (no_debug_isis_srevents,
return CMD_SUCCESS;
}
-DEFUN (debug_isis_tilfa,
- debug_isis_tilfa_cmd,
- "debug " PROTO_NAME " ti-lfa",
- DEBUG_STR
- PROTO_HELP
- "IS-IS TI-LFA Events\n")
+DEFUN(debug_isis_tilfa, debug_isis_tilfa_cmd, "debug " PROTO_NAME " ti-lfa",
+ DEBUG_STR PROTO_HELP "IS-IS TI-LFA Events\n")
{
debug_tilfa |= DEBUG_TILFA;
print_debug(vty, DEBUG_TILFA, 1);
@@ -1410,13 +1406,9 @@ DEFUN (debug_isis_tilfa,
return CMD_SUCCESS;
}
-DEFUN (no_debug_isis_tilfa,
- no_debug_isis_tilfa_cmd,
- "no debug " PROTO_NAME " ti-lfa",
- NO_STR
- UNDEBUG_STR
- PROTO_HELP
- "IS-IS TI-LFA Events\n")
+DEFUN(no_debug_isis_tilfa, no_debug_isis_tilfa_cmd,
+ "no debug " PROTO_NAME " ti-lfa",
+ NO_STR UNDEBUG_STR PROTO_HELP "IS-IS TI-LFA Events\n")
{
debug_tilfa &= ~DEBUG_TILFA;
print_debug(vty, DEBUG_TILFA, 0);
diff --git a/isisd/isisd.h b/isisd/isisd.h
index bd1a9ae54..e3de6692f 100644
--- a/isisd/isisd.h
+++ b/isisd/isisd.h
@@ -289,7 +289,7 @@ extern unsigned long debug_tilfa;
#define DEBUG_BFD (1<<10)
#define DEBUG_TX_QUEUE (1<<11)
#define DEBUG_SR (1<<12)
-#define DEBUG_TILFA (1<<13)
+#define DEBUG_TILFA (1 << 13)
/* Debug related macro. */
#define IS_DEBUG_ADJ_PACKETS (debug_adj_pkt & DEBUG_ADJ_PACKETS)
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFedora 29 amd64 build: Failed (click for details)Make failed for Fedora 29 amd64 build:
Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13883/artifact/F29BUILD/config.status/config.status FreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build:
FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13883/artifact/CI009BUILD/config.status/config.status Debian 10 amd64 build: Failed (click for details)Make failed for Debian 10 amd64 build:
Debian 10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13883/artifact/DEB10BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13883/artifact/U2004AMD64BUILD/config.status/config.statusMake failed for Ubuntu 20.04 amd64 build:
Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsFedora 29 amd64 build: Failed (click for details)Make failed for Fedora 29 amd64 build:
Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13883/artifact/F29BUILD/config.status/config.status FreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build:
FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13883/artifact/CI009BUILD/config.status/config.status Debian 10 amd64 build: Failed (click for details)Make failed for Debian 10 amd64 build:
Debian 10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13883/artifact/DEB10BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13883/artifact/U2004AMD64BUILD/config.status/config.statusMake failed for Ubuntu 20.04 amd64 build:
|
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 2 on Ubuntu 18.04 arm8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO2U18ARM8-13885/test Topology Tests failed for Topo tests part 2 on Ubuntu 18.04 arm8:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13885/artifact/TOPO2U18ARM8/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopo tests part 2 on Ubuntu 18.04 arm8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO2U18ARM8-13885/test Topology Tests failed for Topo tests part 2 on Ubuntu 18.04 arm8:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13885/artifact/TOPO2U18ARM8/ErrorLog/log_topotests.txt
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
Continuous Integration Result: SUCCESSFULContinuous 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-13885/ 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:
|
This can't be broken into smaller commits? 46k of stuff and it's 2 commits? |
@donaldsharp the bulk of those 46k line changes are JSON data used by the TI-LFA topotest. This is because that topotest comprises of 9 testing steps, and for each step I'm using full snapshots of some "show" commands to check if things converged properly. To reduce the amount of data, I should change that topotest to use diffs instead of full snapshots. That should also make it easier to identify what are the expected changes between one testing step and the following one. The actual TI-LFA code changes are somewhat small ( |
e2aa2e9
to
1104397
Compare
Update:
|
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 2 on Ubuntu 18.04 arm8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO2U18ARM8-13938/test Topology Tests failed for Topo tests part 2 on Ubuntu 18.04 arm8:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13938/artifact/TOPO2U18ARM8/ErrorLog/log_topotests.txt Topo tests part 0 on Ubuntu 18.04 arm8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-13938/test Topology Tests failed for Topo tests part 0 on Ubuntu 18.04 arm8:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13938/artifact/TOPO0U18ARM8/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopo tests part 2 on Ubuntu 18.04 arm8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO2U18ARM8-13938/test Topology Tests failed for Topo tests part 2 on Ubuntu 18.04 arm8:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13938/artifact/TOPO2U18ARM8/ErrorLog/log_topotests.txt Topo tests part 0 on Ubuntu 18.04 arm8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-13938/test Topology Tests failed for Topo tests part 0 on Ubuntu 18.04 arm8:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-13938/artifact/TOPO0U18ARM8/ErrorLog/log_topotests.txt
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
Regarding the CI failure, Martin should be updating the ARM boxes soon to add the |
1104397
to
2b49d06
Compare
Outdated results 🚧Basic BGPD CI results: Partial FAILURE, 1 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous 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-14037/ 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:
|
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
2b49d06
to
86d103f
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous 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-14238/ 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:
|
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.
Just one question and a few nits... Otherwise looks good. I didn't spend a ton of time in the topotests changes, however.
86d103f
to
1a29607
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous 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-14392/ 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:
Ubuntu 18.04 amd64 build: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 amd64 build:
Ubuntu 18.04 arm8 build: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm8 build:
Ubuntu 18.04 arm7 build: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm7 build:
Debian 9 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 9 amd64 build:
Ubuntu 18.04 ppc64le build: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 ppc64le build:
|
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 amazing work to provide TI-LFA support.
However, I have some request changes:
- First, you remove the advertisement of Backup ADJ_SID if TI-LFA is not enable while Backup ADJ_SID is also used by other protection mechanism like MPLS FRR. So, it must be advertise independently of TI-LFA. And, if this later is on, push the backup stack of label.
- New TI-LFA CLI doesn't check if SR is enable while TI-LFA needs SR enable first. It will be safer to add verification if SR is enable or not in new functions introduce in isis_nb_config.c and raised a message / warning in isis_cli.c for new commands
- isis_lfa.c / isis_lfa.h lake of comment. It will be good for code maintainability if you could add doxygen documentation for new functions and add more comments or a dedicated developer doc to describe main process / implementation of TI-LFA
struct tilfa_find_qnode_adj_sid_args { | ||
const uint8_t *qnode_sysid; | ||
mpls_label_t label; | ||
}; | ||
|
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.
Even if it uses only in this file, it is better to move structure declaration in the corresponding .h file
void isis_spf_node_list_init(struct isis_spf_nodes *nodes); | ||
void isis_spf_node_list_clear(struct isis_spf_nodes *nodes); | ||
struct isis_spf_node *isis_spf_node_new(struct isis_spf_nodes *nodes, | ||
const uint8_t *sysid); | ||
struct isis_spf_node *isis_spf_node_find(const struct isis_spf_nodes *nodes, | ||
const uint8_t *sysid); | ||
bool isis_lfa_excise_adj_check(const struct isis_spftree *spftree, | ||
const uint8_t *id); | ||
bool isis_lfa_excise_node_check(const struct isis_spftree *spftree, | ||
const uint8_t *id); | ||
struct isis_spftree *isis_spf_reverse_run(const struct isis_spftree *spftree); | ||
int isis_spf_run_neighbors(struct isis_spftree *spftree); | ||
void isis_spf_run_lfa(struct isis_area *area, struct isis_spftree *spftree); | ||
int isis_lfa_check(struct isis_spftree *spftree, struct isis_vertex *vertex); | ||
struct isis_spftree * | ||
isis_tilfa_compute(struct isis_area *area, struct isis_spftree *spftree, | ||
struct isis_spftree *spftree_reverse, | ||
struct lfa_protected_resource *protected_resource); | ||
|
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.
Please add doxygen doc for each function prototype
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.
Done.
isisd/isis_sr.c
Outdated
if (backup) { | ||
struct isis_vertex_adj *vadj; | ||
struct listnode *node; | ||
|
||
sra->backup_nexthops = list_new(); | ||
for (ALL_LIST_ELEMENTS_RO(nexthops, node, vadj)) { | ||
struct isis_adjacency *adj = vadj->sadj->adj; | ||
struct mpls_label_stack *label_stack; | ||
|
||
label_stack = vadj->label_stack; | ||
adjinfo2nexthop(family, sra->backup_nexthops, adj, | ||
label_stack); | ||
} | ||
} |
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 will be safer to check that nexthops list is not null to avoid create unnecessary a new list for sra->backup_nexthops
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.
Done in preparation for the upcoming backup Adj-SID changes.
isisd/isis_sr.c
Outdated
if (sra->type == ISIS_SR_LAN_BACKUP) { | ||
sra->backup_nexthops->del = | ||
(void (*)(void *))isis_nexthop_delete; | ||
list_delete(&sra->backup_nexthops); | ||
} | ||
|
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.
Need to check that sra->backup_nexthops is not null in the case you install a backup ADJ_SID without TI-LFA (in accordance to my previous remark)
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.
Done in preparation for the upcoming backup Adj-SID changes.
isisd/isis_zebra.c
Outdated
#if 0 | ||
SET_FLAG(api.message, ZAPI_MESSAGE_DISTANCE); | ||
api.distance = route_info->depth; | ||
#endif |
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.
Please remove dead code
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.
Done. I tried to preserve that #if 0
block from the original code but in fact setting the admin distance that way doesn't make any sense :)
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.
looks good -- thanks for the cleanup!
Add CLI wrapper commands as well... Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
TI-LFA is a modern fast-reroute (FRR) solution that leverages Segment Routing to pre-compute backup nexthops for all destinations in the network, helping to reduce traffic restoration times whenever a failure occurs. The backup nexthops are expected to be installed in the FIB so that they can be activated as soon as a failure is detected, making sub-50ms recovery possible (assuming an hierarchical FIB). TI-LFA is a huge step forward compared to prior IP-FRR solutions, like classic LFA and Remote LFA, as it guarantees 100% coverage for all destinations. This is possible thanks to the source routing capabilities of SR, which allows the backup nexthops to steer traffic around the failures (using as many SIDs as necessary). In addition to that, the repair paths always follow the post-convergence SPF tree, which prevents transient congestions and suboptimal routing from happening. Deploying TI-LFA is very simple as it only requires a single configuration command for each interface that needs to be protected (both link protection and node protection are available). In addition to IPv4 and IPv6 routes, SR Prefix-SIDs and Adj-SIDs are also protected by the backup nexthops computed by the TI-LFA algorithms. Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
1a29607
to
d240e5c
Compare
💚 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-14761/ 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:
|
TI-LFA is a modern fast-reroute (FRR) solution that leverages Segment Routing to pre-compute backup nexthops for all destinations in the network, helping to reduce traffic restoration times whenever a failure occurs. The backup nexthops are expected to be installed in the FIB so that they can be activated as soon as a failure is detected, making sub-50ms recovery possible (assuming an hierarchical FIB).
TI-LFA is a huge step forward compared to prior IP-FRR solutions, like classic LFA and Remote LFA, as it guarantees 100% coverage for all destinations. This is possible thanks to the source routing capabilities of SR, which allows the backup nexthops to steer traffic around the failures (using as many SIDs as necessary). In addition to that, the repair paths always follow the post-convergence SPF tree, which prevents transient congestions and suboptimal routing from happening.
Deploying TI-LFA is very simple as it only requires a single configuration command for each interface that needs to be protected (both link protection and node protection are available). In addition to IPv4 and IPv6 routes, SR Prefix-SIDs and Adj-SIDs are also protected by the backup nexthops computed by the TI-LFA algorithms.
Link to the TI-LFA draft: https://tools.ietf.org/html/draft-ietf-rtgwg-segment-routing-ti-lfa-03
Testing done:
Last but not least, big thanks to Mark (@mjstapp) for implementing support for backup nexthops in zebra, without which this work wouldn't be possible.