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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

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

@eqvinox
Copy link
Contributor

eqvinox commented Oct 29, 2024

Actually — "duh" — nothing says the command needs to stay in zebra, pimd can just handle it?

(Edit: the variant with the IP address parameter — showing the table is probably not doable/sensible from pimd)

@nabahr
Copy link
Contributor Author

nabahr commented Oct 29, 2024

Ok, so I believe the best path forward to clean up the MRIB UX:

  • Move show ip rpf x.x.x.x from zebra to pim, aliased (and probably deprecated) to the existing show ip pim nexthop-lookup ... command.
  • Expand existing show ip route ... to include a mrib flag for showing the multicast safi table show ip route mrib ...
  • Keep show ip rpf in zebra, but deprecated/aliased to show ip route mrib.

@nabahr nabahr force-pushed the mcast-mode branch 4 times, most recently from 5122396 to 98a5ff6 Compare October 29, 2024 21:49
Copy link

github-actions bot commented Nov 5, 2024

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

@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);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~

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]>
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
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.

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.

3 participants