Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Nhg reuse intf down up #16800

Merged
merged 5 commits into from
Oct 4, 2024
Merged

Nhg reuse intf down up #16800

merged 5 commits into from
Oct 4, 2024

Conversation

donaldsharp
Copy link
Member

Effective allow reuse of nhg's when an interface goes down/up

    Routing entry for 9.9.9.9/32
      Known via "static", distance 1, metric 0, best
      Last update 00:00:08 ago
      * 192.168.99.33, via dummy1, weight 1
      * 192.168.100.33, via dummy2, weight 1
      * 192.168.101.33, via dummy3, weight 1
      * 192.168.102.33, via dummy4, weight 1
    
    eva(config)# do show ip route nexthop-group 9.9.9.9/32
    % Unknown command: do show ip route nexthop-group 9.9.9.9/32
    eva(config)# do show ip route 9.9.9.9/32 nexthop-group
    Routing entry for 9.9.9.9/32
      Known via "static", distance 1, metric 0, best
      Last update 00:00:54 ago
      Nexthop Group ID: 57
      * 192.168.99.33, via dummy1, weight 1
      * 192.168.100.33, via dummy2, weight 1
      * 192.168.101.33, via dummy3, weight 1
      * 192.168.102.33, via dummy4, weight 1
    
    eva(config)# exit
    eva# conf
    eva(config)# int dummy3
    eva(config-if)# shut
    eva(config-if)# no shut
    eva(config-if)# do show ip route 9.9.9.9/32 nexthop-group
    Routing entry for 9.9.9.9/32
      Known via "static", distance 1, metric 0, best
      Last update 00:00:08 ago
      Nexthop Group ID: 57
      * 192.168.99.33, via dummy1, weight 1
      * 192.168.100.33, via dummy2, weight 1
      * 192.168.101.33, via dummy3, weight 1
      * 192.168.102.33, via dummy4, weight 1
    
    eva(config-if)# exit
    eva(config)# exit
    eva# exit
    sharpd@eva ~/frr1 (master) [255]> ip nexthop show id 57
    id 57 group 37/43/50/58 proto zebra
    sharpd@eva ~/frr1 (master)> ip route show 9.9.9.9/32
    9.9.9.9 nhid 57 proto 196 metric 20
            nexthop via 192.168.99.33 dev dummy1 weight 1
            nexthop via 192.168.100.33 dev dummy2 weight 1
            nexthop via 192.168.101.33 dev dummy3 weight 1
            nexthop via 192.168.102.33 dev dummy4 weight 1```

@frrbot frrbot bot added tests Topotests, make check, etc zebra labels Sep 11, 2024
ton31337
ton31337 reviewed Sep 11, 2024
View reviewed changes
zebra/zebra_rib.c Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

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

@donaldsharp donaldsharp force-pushed the nhg_reuse_intf_down_up branch from 6c97024 to cbf519b Compare September 12, 2024 12:14
@donaldsharp donaldsharp force-pushed the nhg_reuse_intf_down_up branch from cbf519b to b968262 Compare September 12, 2024 12:26
@ton31337
Copy link
Member

frrbot?

@donaldsharp
tests: When finding nexthops ensure that they are active
5a58921 
Do not accept a nexthop as valid unless it is marked as being
active.

Signed-off-by: Donald Sharp <[email protected]>
@donaldsharp
zebra: Properly note that a nhg's nexthop has gone down
1bbbcf0 
Current code when a link is set down is to just mark the
nexthop group as not properly setup.  Leaving situations
where when an interface goes down and show output is
entered we see incorrect state.  This is true for anything
that would be checking those flags at that point in time.

Modify the interface down nexthop group code to notice the
nexthops appropriately ( and I mean set the appropriate flags )
and to allow a `show ip route` command to actually display
what is going on with the nexthops.

eva# show ip route 1.0.0.0
Routing entry for 1.0.0.0/32
  Known via "sharp", distance 150, metric 0, best
  Last update 00:00:06 ago
  * 192.168.44.33, via dummy1, weight 1
  * 192.168.45.33, via dummy2, weight 1

sharpd@eva:~/frr1$ sudo ip link set dummy2 down

eva# show ip route 1.0.0.0
Routing entry for 1.0.0.0/32
  Known via "sharp", distance 150, metric 0, best
  Last update 00:00:12 ago
  * 192.168.44.33, via dummy1, weight 1
    192.168.45.33, via dummy2 inactive, weight 1

Notice now that the 1.0.0.0/32 route now correctly
displays the route for the nexthop group entry.

Signed-off-by: Donald Sharp <[email protected]>
@donaldsharp
zebra: Expose _route_entry_dump_nh so it can be used.
ce166ca 
Expose this helper function so it can be used in zebra_nhg.c

Signed-off-by: Donald Sharp <[email protected]>
@donaldsharp
zebra: Reinstall nexthop when interface comes back up
3be8b48 
If a interface down event caused a nexthop group to remove
one of the entries in the kernel, have it be reinstalled
when the interface comes back up.  Mark the nexthop as
usable.

new behavior:
eva# show nexthop-group rib 181818168
ID: 181818168 (sharp)
     RefCnt: 1
     Uptime: 00:00:23
     VRF: default(bad-value)
     Valid, Installed
     Depends: (35) (38) (44) (51)
           via 192.168.99.33, dummy1 (vrf default), weight 1
           via 192.168.100.33, dummy2 (vrf default), weight 1
           via 192.168.101.33, dummy3 (vrf default), weight 1
           via 192.168.102.33, dummy4 (vrf default), weight 1
eva# conf
eva(config)# int dummy3
eva(config-if)# shut
eva(config-if)# do show nexthop-group rib 181818168
ID: 181818168 (sharp)
     RefCnt: 1
     Uptime: 00:00:44
     VRF: default(bad-value)
     Depends: (35) (38) (44) (51)
           via 192.168.99.33, dummy1 (vrf default), weight 1
           via 192.168.100.33, dummy2 (vrf default), weight 1
           via 192.168.101.33, dummy3 (vrf default) inactive, weight 1
           via 192.168.102.33, dummy4 (vrf default), weight 1
eva(config-if)# no shut
eva(config-if)# do show nexthop-group rib 181818168
ID: 181818168 (sharp)
     RefCnt: 1
     Uptime: 00:00:53
     VRF: default(bad-value)
     Valid, Installed
     Depends: (35) (38) (44) (51)
           via 192.168.99.33, dummy1 (vrf default), weight 1
           via 192.168.100.33, dummy2 (vrf default), weight 1
           via 192.168.101.33, dummy3 (vrf default), weight 1
           via 192.168.102.33, dummy4 (vrf default), weight 1
eva(config-if)# exit
eva(config)# exit
eva# exit
sharpd@eva ~/frr1 (master) [255]> ip nexthop show id 181818168
id 181818168 group 35/38/44/51 proto 194
sharpd@eva ~/frr1 (master)>

Signed-off-by: Donald Sharp <[email protected]>
@donaldsharp
zebra: Attempt to reuse NHG after interface up and route reinstall
f02d76f 
The previous commit modified zebra to reinstall the singleton
nexthops for a nexthop group when a interface event comes up.
Now let's modify zebra to attempt to reuse the nexthop group
when this happens and the upper level protocol resends the
route down with that.  Only match if the protocol is the same
as well as the instance and the nexthop groups would match.

Here is the new behavior:
eva(config)# do show ip route 9.9.9.9/32
Routing entry for 9.9.9.9/32
  Known via "static", distance 1, metric 0, best
  Last update 00:00:08 ago
  * 192.168.99.33, via dummy1, weight 1
  * 192.168.100.33, via dummy2, weight 1
  * 192.168.101.33, via dummy3, weight 1
  * 192.168.102.33, via dummy4, weight 1

eva(config)# do show ip route nexthop-group 9.9.9.9/32
% Unknown command: do show ip route nexthop-group 9.9.9.9/32
eva(config)# do show ip route 9.9.9.9/32 nexthop-group
Routing entry for 9.9.9.9/32
  Known via "static", distance 1, metric 0, best
  Last update 00:00:54 ago
  Nexthop Group ID: 57
  * 192.168.99.33, via dummy1, weight 1
  * 192.168.100.33, via dummy2, weight 1
  * 192.168.101.33, via dummy3, weight 1
  * 192.168.102.33, via dummy4, weight 1

eva(config)# exit
eva# conf
eva(config)# int dummy3
eva(config-if)# shut
eva(config-if)# no shut
eva(config-if)# do show ip route 9.9.9.9/32 nexthop-group
Routing entry for 9.9.9.9/32
  Known via "static", distance 1, metric 0, best
  Last update 00:00:08 ago
  Nexthop Group ID: 57
  * 192.168.99.33, via dummy1, weight 1
  * 192.168.100.33, via dummy2, weight 1
  * 192.168.101.33, via dummy3, weight 1
  * 192.168.102.33, via dummy4, weight 1

eva(config-if)# exit
eva(config)# exit
eva# exit
sharpd@eva ~/frr1 (master) [255]> ip nexthop show id 57
id 57 group 37/43/50/58 proto zebra
sharpd@eva ~/frr1 (master)> ip route show 9.9.9.9/32
9.9.9.9 nhid 57 proto 196 metric 20
	nexthop via 192.168.99.33 dev dummy1 weight 1
	nexthop via 192.168.100.33 dev dummy2 weight 1
	nexthop via 192.168.101.33 dev dummy3 weight 1
	nexthop via 192.168.102.33 dev dummy4 weight 1
sharpd@eva ~/frr1 (master)>

Notice that we now no longer are creating a bunch of new
nexthop groups.

Signed-off-by: Donald Sharp <[email protected]>
@donaldsharp donaldsharp force-pushed the nhg_reuse_intf_down_up branch from b968262 to f02d76f Compare September 16, 2024 13:34
@donaldsharp
Copy link
Member Author

yeah fixed

ton31337
ton31337 approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mjstapp mjstapp self-requested a review September 17, 2024 15:27
riw777
riw777 approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good ... I vote to merge this, but I'll wait to see if @mjstapp wants to review it before hitting the button

@donaldsharp
Copy link
Member Author

I believe Mark is out of town for a week, which will be after the branch pull

@riw777 riw777 merged commit 15991e1 into FRRouting:master Oct 4, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jafaral Jafaral Jafaral approved these changes

@ton31337 ton31337 ton31337 approved these changes

@riw777 riw777 riw777 approved these changes

@mjstapp mjstapp Awaiting requested review from mjstapp

Assignees
No one assigned
Labels
master size/L tests Topotests, make check, etc zebra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@donaldsharp @ton31337 @riw777 @Jafaral