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.

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

Fix PIMD RPF lookup mode and nexthop tracking #17252

Merged
merged 11 commits into from
Dec 16, 2024

Conversation

nabahr
Copy link
Contributor

@nabahr nabahr commented Oct 25, 2024

Generalized the zapi ZEBRA_NEXTHOP_LOOKUP_MRIB to just ZEBRA_NEXTHOP_LOOKUP and added the safi into the request message.

Moved the RPF lookup mode configuration from zebra into pimd.
Now the command show ip rpf x.x.x.x command will just do a lookup against the MRIB, not according to the lookup mode.
To do a nexthop lookup according to the lookup mode, use the command show ip pim nexthop-lookup x.x.x.x y.y.y.y where x.x.x.x is the source address, and y.y.y.y is the group address, which may affect the lookup.

Refactored the PIM zlookup (the synchronous nexthop lookup using ZEBRA_NEXTHOP_LOOKUP) to take the RPF lookup mode into account, which means we may lookup via the URIB, MRIB, or both.

Refactored the PIM next hop tracking and isolated the nexthop lookup and tracking code into pim_nht.h/c.
Now we track in both the MRIB and URIB tables so that decisions about the nexthop based on the RPF lookup mode can be taken into account, and changes to the lookup mode can be reflected immediately without performing additional lookups.

Updated show commands, documentation, and tests. Including a new PIM mrib test to exercise the RPF lookup mode command and verify that NHT is working correctly.

pimd/pim_zlookup.c Outdated Show resolved Hide resolved
pimd/pim_zlookup.c Outdated Show resolved Hide resolved
@nabahr nabahr force-pushed the mcast-mode branch 2 times, most recently from 34f41ee to 74f6819 Compare October 27, 2024 00:22
@nabahr
Copy link
Contributor Author

nabahr commented Oct 27, 2024

New mrib tests pass when built with changes from PR #17254.

@Jafaral Jafaral changed the title WIP: Refactor PIMD RPF lookup mode and nexthop tracking WIP: Fix PIMD RPF lookup mode and nexthop tracking Oct 27, 2024
Copy link

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

@nabahr nabahr force-pushed the mcast-mode branch 3 times, most recently from 01f34c8 to 953d829 Compare October 29, 2024 03:17
@nabahr nabahr changed the title WIP: Fix PIMD RPF lookup mode and nexthop tracking Fix PIMD RPF lookup mode and nexthop tracking Oct 29, 2024
@eqvinox eqvinox self-requested a review October 29, 2024 15:31
@eqvinox
Copy link
Contributor

eqvinox commented Oct 29, 2024

Now the command show ip rpf x.x.x.x command will just do a lookup against the MRIB, not according to the lookup mode.

Hm. I think this is a little problematic UX-wise because it's a "silent" change users can trip over without noticing. Can we just add a multicast or safi multicast option to the show ip route commands? That'd also have the benefit of making the other show ip route options available for looking at the MRIB

@nabahr
Copy link
Contributor Author

nabahr commented Oct 29, 2024

Now the command show ip rpf x.x.x.x command will just do a lookup against the MRIB, not according to the lookup mode.

Hm. I think this is a little problematic UX-wise because it's a "silent" change users can trip over without noticing. Can we just add a multicast or safi multicast option to the show ip route commands? That'd also have the benefit of making the other show ip route options available for looking at the MRIB

Since the rpf lookup mode was moved out of zebra, the command cannot work as it did before these changes. So should this command show ip rpf [A.B.C.D] be removed completely? Deprecated? Add a warning in the output?

@eqvinox
Copy link
Contributor

eqvinox commented Oct 29, 2024

So should this command show ip rpf [A.B.C.D] be removed completely?

I think that's the only reasonable option, and/or replace it with a stub that prints "this command doesn't exist anymore because it's in pimd now"

Now that I think about it… the command can be implemented in pimd and vtysh can dispatch it there instead

@donaldsharp
Copy link
Member

can you please fix this documentation issue, that is now showing up:

make[1]: *** [Makefile:8699: pimd/pim6d] Error 1
make[1]: *** Waiting for unfinished jobs....
/home/sharpd/frr4/doc/user/pim.rst:789: WARNING: undefined label: multicast-rib-commands
make[1]: Leaving directory '/home/sharpd/frr4'

@donaldsharp
Copy link
Member

More issues:

pimd/pim_rp.c: In function ‘pim_embedded_rp_new’:
pimd/pim_rp.c:1499:9: warning: implicit declaration of function ‘pim_find_or_track_nexthop’ [-Wimplicit-function-declaration]
 1499 |         pim_find_or_track_nexthop(pim, rp_info->rp.rpf_addr, NULL, rp_info, NULL);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~
  CC       yang/pimd_pim6d-frr-pim-candidate.yang.o
pimd/pim_rp.c:1500:14: warning: implicit declaration of function ‘pim_ecmp_nexthop_lookup’ [-Wimplicit-function-declaration]
 1500 |         if (!pim_ecmp_nexthop_lookup(pim, &rp_info->rp.source_nexthop, rp_info->rp.rpf_addr,
      |              ^~~~~~~~~~~~~~~~~~~~~~~
  CC       yang/pimd_pim6d-frr-gmp.yang.o
  CC       pbrd/pbr_main.o
pimd/pim_rp.c: In function ‘pim_embedded_rp_free’:
pimd/pim_rp.c:1541:9: warning: implicit declaration of function ‘pim_delete_tracked_nexthop’ [-Wimplicit-function-declaration]
 1541 |         pim_delete_tracked_nexthop(pim, rp_info->rp.rpf_addr, NULL, rp_info);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~

@nabahr nabahr force-pushed the mcast-mode branch 2 times, most recently from 5a00aac to a6119f9 Compare November 26, 2024 20:43
@nabahr
Copy link
Contributor Author

nabahr commented Nov 26, 2024

I rebased and fixed the errors from a bad merge with embedded RP. Docs link issue also fixed.

Copy link

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

Modified ZEBRA_NEXTHOP_LOOKUP_MRIB to include the SAFI from which to do the lookup.
This generalizes the API away from MRIB specifically and allows the user to decide how it should do lookups.
Rename ZEBRA_NEXTHOP_LOOKUP_MRIB to ZEBRA_NEXTHOP_LOOKUP now that it is more generalized.
This change is in preperation to remove multicast lookup mode completely from zebra.

Signed-off-by: Nathan Bahr <[email protected]>
Multicast mode belongs in PIM, so removing it completely from zebra.
Modified `show (ip|ipv6) rpf ADDRESS` to always lookup from SAFI_MULTICAST.
This means this command is now specific to the multicast table and does
not necessarily reflect the PIM RPF lookup, but that should be implemented
in PIM instead.

Signed-off-by: Nathan Bahr <[email protected]>
Add `mrib` flag to existing "show ip route" commands which then use
the multicast safi rather than the unicast safi. Updated the vty output
to include the AFI and SAFI string when printing the table.
Deprecate `show ip rpf` command, aliased to `show ip route mrib`.
Removed `show ip rpf A.B.C.D`.

Signed-off-by: Nathan Bahr <[email protected]>
Remove use of `ip multicast rpf-lookup-mode` from unrelated tests.
Looks like this test was just unlucky enough to pick that command as an
example for use here. Just changed it to something less likely to be
removed in the future.
Update route table output to include AFI SAFI output.

Signed-off-by: Nathan Bahr <[email protected]>
Copy link
Member

@rzalamena rzalamena left a comment

Choose a reason for hiding this comment

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

Overall PR looks good, I've made just a few observations.

pimd/pim_nb_config.c Outdated Show resolved Hide resolved
pimd/pim_nht.c Outdated Show resolved Hide resolved
tests/topotests/pim_mrib/test_pim_mrib.py Outdated Show resolved Hide resolved
Add rpf-lookup-mode MODE vty command under router pim block.
Including NB piping and config write. Using the mode still pending.

Signed-off-by: Nathan Bahr <[email protected]>
Add prefix length in nexthop response.
Apply lookup mode to the sychronous lookups, where we may lookup
the MRIB, URIB, or both and make a decision based on the nexthop.

Signed-off-by: Nathan Bahr <[email protected]>
Refactor the next hop tracking in PIM to fully support the configured RPF lookup mode.
Moved many NHT related functions to pim_nht.h/c
NHT now tracks both MRIB and URIB tables and makes nexthop decisions based on the configured lookup mode.

Signed-off-by: Nathan Bahr <[email protected]>
Link up the RPF lookup mode changing to a force update to RP's and
upstreams registered for nexthop lookup cache updates.

Signed-off-by: Nathan Bahr <[email protected]>
Moved `show ip rpf A.B.C.D` command here from zebra, deprecated and aliased
to `show ip pim nexthop-lookup`.
Allow group to be optional in the lookup command. Only validate group if
source is ANY. Documented setting source via RP if not provided.
Added new output if ANY source + group lookup is performed and no
RP is found for the group. Updated output to include souce and
group for lookup.

Signed-off-by: Nathan Bahr <[email protected]>
Test mrib overrides and rpf lookup mode changes.

Signed-off-by: Nathan Bahr <[email protected]>
Moved it all to PIM section and updated docs for recent changes.

Signed-off-by: Nathan Bahr <[email protected]>
@nabahr nabahr requested a review from rzalamena December 16, 2024 00:50
@rzalamena rzalamena merged commit 3bebb7b into FRRouting:master Dec 16, 2024
11 checks passed
@nabahr nabahr deleted the mcast-mode branch December 16, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants