zebra: add LSP entry to nexthop via recursive (part 2) #15259

dmytroshytyi-6WIND · 2024-01-30T14:58:20Z

This is a second part of this PR: #12469

The topology of topotest presented in this pull request is described by this figure:

(BGP)  ------- (BGP)  -------  (BGP)  -------  (BGP)
r1 ---- r2 ---- r3 ---- r4 ----- r5 ---- r6 ---- r7
<--- ospf ----> <---- isis -----> <--- ospf ---->

Command example:

mpls fec nexthop-resolution

Nexthop via recursive disabled state:

and r3 has the next mpls table:

r3# show mpls table
 Inbound Label  Type        Nexthop     Outbound Label  
 ------------------------------------------------------- 
 1011           SR (OSPF)   10.126.0.2  1011             
 1022           SR (OSPF)   10.126.0.2  implicit-null    
 11044          SR (IS-IS)  10.127.0.4  implicit-null    
 11055          SR (IS-IS)  10.127.0.4  11055            
 30000          SR (OSPF)   10.126.0.2  implicit-null    
 30001          SR (OSPF)   10.126.0.2  implicit-null    
 36000          SR (IS-IS)  10.127.0.4  implicit-null

Nexthop via recursive enabled state:

and r3 has the next mpls table:

r3# show mpls table
 Inbound Label  Type        Nexthop     Outbound Label  
 -------------------------------------------------------
 17             SR (IS-IS)  10.127.0.4  11055           
 18             SR (OSPF)   10.126.0.2  1011            
 19             BGP         10.127.0.4  11055/19        
 1011           SR (OSPF)   10.126.0.2  1011            
 1022           SR (OSPF)   10.126.0.2  implicit-null   
 11044          SR (IS-IS)  10.127.0.4  implicit-null   
 11055          SR (IS-IS)  10.127.0.4  11055           
 30000          SR (OSPF)   10.126.0.2  implicit-null   
 30001          SR (OSPF)   10.126.0.2  implicit-null   
 36000          SR (IS-IS)  10.127.0.4  implicit-null

Signed-off-by: Dmytro Shytyi [email protected]

dmytroshytyi-6WIND · 2024-01-30T16:53:42Z

ci:rerun

github-actions · 2024-02-05T12:35:32Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

louis-6wind · 2024-02-07T15:10:14Z

ci:rerun

louis-6wind · 2024-02-16T14:59:48Z

zebra/zebra_vrf.c

@@ -541,11 +576,22 @@ int zebra_vrf_netns_handler_create(struct vty *vty, struct vrf *vrf,
 	return CMD_SUCCESS;
 }

+static struct cmd_node vrf_node = {


no need to redefine that. Please remove

I used this because without I have zebra down during init with this message:
mpls_fec_nexthop_resolution_vrf_cmd[[no$no] mpls fec nexthop-resolution]: node 21 does not exist. please call install_node() before install_element()
If you want to remove it you might have a comment how to overturn the consequence?

I don't think redefining the structure here is the right answer though ... maybe @mjstapp might be able to help sort this?

Hi @riw777, In this way we have it working. I agree that this not the best solution possible. If @mjstapp has a better way to suggest, I'll be happy to include it into the PR.

try doing what bgpd does with vrf_cmd_init() ?

Hello @mjstapp.

Thank you for pointing this out.
I removed static struct cmd_node vrf_node = {.

zebra/zebra_vrf.c

louis-6wind · 2024-02-16T15:01:30Z

zebra/zebra_vrf.c

@@ -450,6 +450,41 @@ struct route_table *zebra_vrf_table(afi_t afi, safi_t safi, vrf_id_t vrf_id)
 	return zvrf->table[afi][safi];
 }

+DEFPY(mpls_fec_nexthop_resolution, mpls_fec_nexthop_resolution_cmd,
+      "[no$no] mpls fec nexthop-resolution",


mpls fec nexthop-resolution not seen in show run.
Please fix

Where is the fix?

Starting from 3900813 , it should be done in mgmtd

Hello,

Due to the internal priorities the push was largely delayed. Nevertheless, the fix was uploaded recently. Waiting for CI to pass.

riw777

looks good ... waiting on other reviewer comments, etc.

louis-6wind · 2024-04-24T09:44:08Z

zebra/zebra_vrf.c

@@ -450,6 +450,41 @@ struct route_table *zebra_vrf_table(afi_t afi, safi_t safi, vrf_id_t vrf_id)
 	return zvrf->table[afi][safi];
 }

+DEFPY(mpls_fec_nexthop_resolution, mpls_fec_nexthop_resolution_cmd,
+      "[no$no] mpls fec nexthop-resolution",


Where is the fix?

Starting from 3900813 , it should be done in mgmtd

dmytroshytyi-6WIND · 2024-05-02T13:12:09Z

Interesting, frrbot spotted an issue? and proposes a fix.
The fix it proposes is not that great actually...

dmytroshytyi-6WIND · 2024-05-15T13:01:02Z

@louis-6wind, here is a rebase with small fix.

riw777 · 2024-05-28T14:03:44Z

are we waiting on something else for this one? it looks like all the comments have been addressed?

dmytroshytyi-6WIND · 2024-05-28T14:25:10Z

are we waiting on something else for this one? it looks like all the comments have been addressed?

Thank you for review, no more changes on my side is planned, unless new comments arrive.

louis-6wind · 2024-06-05T12:19:52Z

zebra/zebra_mpls_vty.c

@@ -271,6 +271,7 @@ static int zebra_mpls_config(struct vty *vty)

 	write += zebra_mpls_write_lsp_config(vty, zvrf);
 	write += zebra_mpls_write_fec_config(vty, zvrf);
+	write += zebra_mpls_write_nexthop_resolution_config(vty, zvrf);


This does not write anything in show run when doing

vrf NAME mpls fec nexthop-resolution

You have to migrate that to mgmtd

New changes were introduced that outdates this question.
Now the show running-config output is like this:

! vrf vrf10 mpls fec nexthop-resolution exit-vrf !

dmytroshytyi-6WIND · 2024-06-07T09:34:42Z

Changes introduced that cover @louis-6wind question + rebase.
Thank you @riw777 @mjstapp @louis-6wind very much for preformed reviews and provided comments.

louis-6wind

LGTM

Commands added: r3# configure r3(config)# mpls fec MPLS FEC table label Label configuration ldp Label Distribution Protocol lsp Establish label switched path r3(config)# mpls fec mpls fec nexthop-resolution Authorise nexthop resolution over all labeled routes. r3(config)# mpls fec mpls fec nexthop-resolution r3# configure r3(config)# vrf default r3(config-vrf)# mpls fec MPLS FEC table r3(config-vrf)# mpls fec mpls fec nexthop-resolution Authorise nexthop resolution over all labeled routes. r3(config-vrf)# mpls fec nexthop-resolution east-vm# show running-config Building configuration... ... ! mpls fec nexthop-resolution ! ... Signed-off-by: Dmytro Shytyi <[email protected]>

Upon reconfiguring nexthop-resolution updates, update the lsp entries accordingly. If fec nexthop-resolution becomes true, then call again fec_change_update_lsp() for each fec entry available. If fec nexthop-resolution becomes false, then call again fec_change_update_lsp() for each fec entry available, and if the update fails, uninstall any lsp related with the fec entry. In the case lsp_install() and no lsp entry could be created or updated, then consider this call as a failure, and return -1. Co-developed-by: Dmytro Shytyi <[email protected]> Signed-off-by: Philippe Guibert <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]>

This commit addresses an issue that happens when using bgp labeled unicast peering with a rr client, with a received prefix which is the local ip address of the bgp session. When using bgp ipv4 labeled session, the local prefix is received by a peer, and finds out that the proposed prefix and its next-hop are the same. To avoid a route loop locally, no nexthop entry is referenced for that prefix, and the route will not be selected. As it has been done for ipv4-unicast, apply the following fix for labeled address families: when the received peer is a route reflector, the prefix has to be selected, even if the route can not be installed locally. Fixes: f874552 ("bgpd: authorise to select bgp self peer prefix on rr case") Signed-off-by: Philippe Guibert <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]>

There are two ways of iterating over nexthops of a given route entry. - Either only the main nexthop are taken into account (which is the case today when attempting to install an LSP entry on a BGP connected labeled route. - Or by taking into account nexthops that are resolved and linked in nexthop->resolved of the previous nexthop which has RECURSIVE flag set. This second case has to be taken into account in the case where recursive routes may be used to install an LSP entry. Introduce a new API in nexthop that will parse over the appropriate nexthop, if the nexthop-resolution flag is turned on or not on the given VRF. Use that API in the lsp_install() function so as to walk over the appropriate nexthops. Co-developed-by: Dmytro Shytyi <[email protected]> Signed-off-by: Philippe Guibert <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]>

When an LSP entry is created from a FEC entry, multiple labels may now be appended to the LSP entry, instead of one single. Upon lsp creation, the LSP trace will display all the labels appended. Signed-off-by: Dmytro Shytyi <[email protected]>

Until now, when a FEC entry is added in zebra, driven by the reception of a BGP labeled unicast update, an LSP entry is created. That LSP entry is resolved by using the route entry which is also installed by BGP labeled unicast. This route entry is not available when we face with i-bgp peering session. I-BGP labeled sessions are used to establish IP connectivity across separate IGPs. The below dumps illustrate a 3 IGP topology. An attempt to create connectivity between the north and the south machines is done. The 3 separate IGPs are configured in Segment routing: - north-east - east-west - west-south We create BGP peerings between each endpoint of each IGP: - iBGP between (north) and (east) - iBGP between (east) and (west) - iBGP between (west) and (south) Before that patch, the FEC entries could not be resolved on the east machine: Before: east-vm# show mpls fec 192.0.2.1/32 Label: 18 Client list: bgp(fd 48) 192.0.2.5/32 Label: 17 Client list: bgp(fd 48) 192.0.2.7/32 Label: 19 Client list: bgp(fd 48) east-vm# show mpls table Inbound Label Type Nexthop Outbound Label -------------------------------------------------------- 1011 SR (OSPF) 192.168.2.2 1011 1022 SR (OSPF) 192.168.2.2 implicit-null 11044 SR (IS-IS) 192.168.3.4 implicit-null 11055 SR (IS-IS) 192.168.3.4 11055 30000 SR (OSPF) 192.168.2.2 implicit-null 30001 SR (OSPF) 192.168.2.2 implicit-null 36000 SR (IS-IS) 192.168.3.4 implicit-null east-vm# show ip route [..] B 192.0.2.1/32 [200/0] via 192.0.2.1 inactive, label implicit-null, weight 1, 00:17:45 O>* 192.0.2.1/32 [110/20] via 192.168.2.2, r3-eth0, label 1011, weight 1, 00:17:47 O>* 192.0.2.2/32 [110/10] via 192.168.2.2, r3-eth0, label implicit-null, weight 1, 00:17:47 O 192.0.2.3/32 [110/0] is directly connected, lo, weight 1, 00:17:57 C>* 192.0.2.3/32 is directly connected, lo, 00:18:03 I>* 192.0.2.4/32 [115/20] via 192.168.3.4, r3-eth1, label implicit-null, weight 1, 00:17:59 B 192.0.2.5/32 [200/0] via 192.0.2.5 inactive, label implicit-null, weight 1, 00:17:56 I>* 192.0.2.5/32 [115/30] via 192.168.3.4, r3-eth1, label 11055, weight 1, 00:17:58 B> 192.0.2.7/32 [200/0] via 192.0.2.5 (recursive), label 19, weight 1, 00:17:45 * via 192.168.3.4, r3-eth1, label 11055/19, weight 1, 00:17:45 [..] After command "mpls fec nexthop-resolution" was applied, the FEC entries will resolve over any non BGP route that has a labeled path selected. east-vm# show mpls table Inbound Label Type Nexthop Outbound Label -------------------------------------------------------- 17 SR (IS-IS) 192.168.3.4 11055 18 SR (OSPF) 192.168.2.2 1011 19 BGP 192.168.3.4 11055/19 1011 SR (OSPF) 192.168.2.2 1011 1022 SR (OSPF) 192.168.2.2 implicit-null 11044 SR (IS-IS) 192.168.3.4 implicit-null 11055 SR (IS-IS) 192.168.3.4 11055 30000 SR (OSPF) 192.168.2.2 implicit-null 30001 SR (OSPF) 192.168.2.2 implicit-null 36000 SR (IS-IS) 192.168.3.4 implicit-null Co-developed-by: Dmytro Shytyi <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]> Signed-off-by: Philippe Guibert <[email protected]>

Change the comment in the code that refers to ZEBRA_FLAG_XXX defines. Signed-off-by: Philippe Guibert <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]>

Because the nhlfe label stack may contain more than one label, ensure to copy all labels. Co-developed-by: Dmytro Shytyi <[email protected]> Signed-off-by: Philippe Guibert <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]>

zebra_mpls_lsp_install() returned integer is never checked. Return void instead. Signed-off-by: Dmytro Shytyi <[email protected]>

There are 3 tests with OSPF, IS-IS, BGP and MPLS configured: 1. Check the status of BGP session between North and South == Established 2. Check the connectivity with "ping South -I North" 3. Check the label on the West. Signed-off-by: Philippe Guibert <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]>

FEC nexthop entry resolution over MPLS networks Signed-off-by: Philippe Guibert <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]>

dmytroshytyi-6WIND · 2024-06-07T10:35:41Z

rebase on upstream latest state was done

dmytroshytyi-6WIND · 2024-06-07T17:31:06Z

ci:rerun

riw777 · 2024-06-11T13:54:33Z

failure looks related --

   assert Generated JSON diff error report:
     
     > $->totalPrefixCounter: output has element with value '1' but in expected it has value '2'```

riw777 · 2024-07-09T02:57:24Z

@mjstapp want to look at this before merging?

riw777 · 2024-08-14T22:29:44Z

@mjstapp ping ... :-)

riw777 · 2024-08-27T12:04:00Z

@mjstapp ping :-)

…xthop_resolution" This reverts commit add56c6, reversing changes made to b774fc6.

frrbot bot added bgp bugfix documentation libfrr zebra labels Jan 30, 2024

github-actions bot added size/XXL master labels Jan 30, 2024

riw777 self-requested a review January 30, 2024 15:13

github-actions bot added the conflicts label Feb 5, 2024

louis-6wind mentioned this pull request Feb 8, 2024

draft zebra: add LSP entry to nexthop via recursive (part 2) #15331

Closed

dmytroshytyi-6WIND force-pushed the nexthop_resolution branch from 817070d to 2685f19 Compare February 13, 2024 09:46

github-actions bot removed the conflicts label Feb 13, 2024

dmytroshytyi-6WIND force-pushed the nexthop_resolution branch from 2685f19 to 2070438 Compare February 13, 2024 10:04

louis-6wind suggested changes Feb 16, 2024

View reviewed changes

riw777 approved these changes Feb 27, 2024

View reviewed changes

mjstapp self-requested a review February 27, 2024 16:25

louis-6wind suggested changes Apr 24, 2024

View reviewed changes

dmytroshytyi-6WIND force-pushed the nexthop_resolution branch from 2070438 to 516a757 Compare May 2, 2024 13:00

dmytroshytyi-6WIND force-pushed the nexthop_resolution branch from 516a757 to d532b60 Compare May 15, 2024 12:58

dmytroshytyi-6WIND force-pushed the nexthop_resolution branch 2 times, most recently from 710441c to 04d2b0d Compare May 23, 2024 15:46

riw777 requested a review from louis-6wind June 4, 2024 11:34

louis-6wind suggested changes Jun 5, 2024

View reviewed changes

dmytroshytyi-6WIND force-pushed the nexthop_resolution branch from 04d2b0d to e452cfa Compare June 7, 2024 09:29

dmytroshytyi-6WIND force-pushed the nexthop_resolution branch 2 times, most recently from ac4bedb to 72b16e5 Compare June 7, 2024 10:26

github-actions bot added the rebase PR needs rebase label Jun 7, 2024

louis-6wind approved these changes Jun 7, 2024

View reviewed changes

dmytroshytyi-6WIND force-pushed the nexthop_resolution branch from 72b16e5 to 2936551 Compare June 7, 2024 10:29

dmytroshytyi-6WIND and others added 11 commits June 7, 2024 12:33

zebra: the header containing the re->flags is in zclient.h

e9c7b5d

Change the comment in the code that refers to ZEBRA_FLAG_XXX defines. Signed-off-by: Philippe Guibert <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]>

zebra: return void zebra_mpls_lsp_install

f62ee81

zebra_mpls_lsp_install() returned integer is never checked. Return void instead. Signed-off-by: Dmytro Shytyi <[email protected]>

doc: fec nexthop resolution documentation

b2c2113

FEC nexthop entry resolution over MPLS networks Signed-off-by: Philippe Guibert <[email protected]> Signed-off-by: Dmytro Shytyi <[email protected]>

dmytroshytyi-6WIND force-pushed the nexthop_resolution branch from 2936551 to b2c2113 Compare June 7, 2024 10:34

pguibert6WIND removed the rebase PR needs rebase label Jun 7, 2024

riw777 merged commit add56c6 into FRRouting:master Sep 10, 2024
12 checks passed

piotrsuchy added a commit to piotrsuchy/frr that referenced this pull request Oct 2, 2024

Revert "Merge pull request FRRouting#15259 from dmytroshytyi-6WIND/ne…

20e9fd7

…xthop_resolution" This reverts commit add56c6, reversing changes made to b774fc6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zebra: add LSP entry to nexthop via recursive (part 2) #15259

zebra: add LSP entry to nexthop via recursive (part 2) #15259

dmytroshytyi-6WIND commented Jan 30, 2024

dmytroshytyi-6WIND commented Jan 30, 2024

github-actions bot commented Feb 5, 2024

louis-6wind commented Feb 7, 2024

louis-6wind Feb 16, 2024

dmytroshytyi-6WIND Feb 27, 2024

riw777 Apr 9, 2024

dmytroshytyi-6WIND May 2, 2024

mjstapp May 15, 2024

dmytroshytyi-6WIND May 23, 2024

louis-6wind Feb 16, 2024

dmytroshytyi-6WIND Feb 27, 2024

louis-6wind Apr 24, 2024 •

edited

Loading

dmytroshytyi-6WIND May 2, 2024

riw777 left a comment

louis-6wind Apr 24, 2024 •

edited

Loading

dmytroshytyi-6WIND commented May 2, 2024

dmytroshytyi-6WIND commented May 15, 2024

riw777 commented May 28, 2024

dmytroshytyi-6WIND commented May 28, 2024

louis-6wind Jun 5, 2024

dmytroshytyi-6WIND Jun 7, 2024

louis-6wind Jun 7, 2024

dmytroshytyi-6WIND commented Jun 7, 2024

louis-6wind left a comment

dmytroshytyi-6WIND commented Jun 7, 2024

dmytroshytyi-6WIND commented Jun 7, 2024

riw777 commented Jun 11, 2024

riw777 commented Jul 9, 2024

riw777 commented Aug 14, 2024

riw777 commented Aug 27, 2024

zebra: add LSP entry to nexthop via recursive (part 2) #15259

zebra: add LSP entry to nexthop via recursive (part 2) #15259

Conversation

dmytroshytyi-6WIND commented Jan 30, 2024

dmytroshytyi-6WIND commented Jan 30, 2024

github-actions bot commented Feb 5, 2024

louis-6wind commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louis-6wind Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riw777 left a comment

Choose a reason for hiding this comment

louis-6wind Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

dmytroshytyi-6WIND commented May 2, 2024

dmytroshytyi-6WIND commented May 15, 2024

riw777 commented May 28, 2024

dmytroshytyi-6WIND commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytroshytyi-6WIND commented Jun 7, 2024

louis-6wind left a comment

Choose a reason for hiding this comment

dmytroshytyi-6WIND commented Jun 7, 2024

dmytroshytyi-6WIND commented Jun 7, 2024

riw777 commented Jun 11, 2024

riw777 commented Jul 9, 2024

riw777 commented Aug 14, 2024

riw777 commented Aug 27, 2024

louis-6wind Apr 24, 2024 •

edited

Loading

louis-6wind Apr 24, 2024 •

edited

Loading