isisd: add support for Topology Independent LFA (TI-LFA) #7011
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
Aug 27, 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/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 warningsWarnings 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 ( |
rwestphal
force-pushed
the
isis-ti-lfa
branch
from
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 |
rwestphal
force-pushed
the
isis-ti-lfa
branch
from
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 warningsWarnings 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 |
rwestphal
force-pushed
the
isis-ti-lfa
branch
from
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 warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
riw777
reviewed
Sep 18, 2020
rwestphal
force-pushed
the
isis-ti-lfa
branch
from
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 warningsWarnings 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: |
odd22
requested changes
Oct 6, 2020
There was a problem hiding this comment.
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
Comment on lines
+198
to
+248
| struct tilfa_find_qnode_adj_sid_args { | ||
| const uint8_t *qnode_sysid; | ||
| mpls_label_t label; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Even if it uses only in this file, it is better to move structure declaration in the corresponding .h file
Comment on lines
+82
to
+100
| 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.
Please add doxygen doc for each function prototype
isisd/isis_sr.c
Outdated
Comment on lines
1580
to
1594
| 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.
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.
Done in preparation for the upcoming backup Adj-SID changes.
isisd/isis_sr.c
Outdated
Comment on lines
1705
to
1711
| 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.
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.
Done in preparation for the upcoming backup Adj-SID changes.
isisd/isis_zebra.c
Outdated
Comment on lines
289
to
292
| #if 0 | ||
| SET_FLAG(api.message, ZAPI_MESSAGE_DISTANCE); | ||
| api.distance = route_info->depth; | ||
| #endif |
There was a problem hiding this comment.
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 :)
riw777
approved these changes
Oct 6, 2020
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>
rwestphal
force-pushed
the
isis-ti-lfa
branch
from
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 warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
odd22
approved these changes
Oct 16, 2020
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.
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.