frr icon indicating copy to clipboard operation
frr copied to clipboard

PIM: Implement AutoRP functionality

Open nabahr opened this issue 1 year ago • 1 comments

This adds most of the AutoRP functionality to PIMD for dynamic RP support using the AutoRP protocol. RP Discovery is supported and is disabled by default. To enable AutoRP discovery:

router pim [vrf NAME]
  autorp discovery

Once discovery is enabled, we will listen for the AutoRP Discovery message sent by the AutoRP mapping agent and will use the discovered RP information.

AutoRP announcement support is also implemented. To configure PIM to advertise itself as a candidate RP via AutoRP:

router pim [vrf NAME]
  autorp announce RP-ADDR [GROUP | group-list PREFIX-LIST]
  autorp announce {scope (1-255) | interval (1-65535) | holdtime (0-65535)}

The RP advertisement can default to all multicast (224.0.0.0/4), a specific range (225.0.0.0/24), or specified in a prefix list. The specifics of the announcement messages sent can be configured with the second command. The scope sets the TTL of the packet, the interval sets how often the packet is sent, and the hold time sets the hold time used in the packet, i.e. how long the information in the packet is considered valid (0=disabled).

The only part of AutoRP not yet implemented is the AutoRP mapping agent.

Show information available with: show ip pim [vrf NAME] autorp [json] This will display state of AutoRP discovery (enabled/disabled) and the configured candidate RP's for AutoRP. The existing show ip rp-info command will display RP's learned via AutoRP.

Resources used for reference during development: https://github.com/troglobit/pimd/blob/master/doc/pim-autorp-spec01.txt https://www.cisco.com/c/en/us/td/docs/ios-xml/ios/ipmulti_pim/configuration/imc-pim-xe-3e/imc_autorp.html https://www.cisco.com/c/en/us/support/docs/ip/multicast/118405-config-rp-00.html

nabahr avatar Aug 22 '24 23:08 nabahr

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

github-actions[bot] avatar Sep 14 '24 00:09 github-actions[bot]

I'm a bit grumpy about the fact that we have basically 3k lines of diff in one big patch. What can be done to break this up into smaller logical commits? As it stands it is almost impossible to review given the way it is written.

donaldsharp avatar Sep 16 '24 14:09 donaldsharp

I'm a bit grumpy about the fact that we have basically 3k lines of diff in one big patch. What can be done to break this up into smaller logical commits? As it stands it is almost impossible to review given the way it is written.

The original PR split commits between the implementation of discovery and announcements, but the addition of announcements ended up rewriting parts of the discovery implementation making a review of each commit difficult so I've combined those separate commits and then split that up between the CLI/NB implementation and the actual feature implementation. This splits the size of the change roughly in half. I could try to further decompose the feature implementation but keeping each commit building error free and not adding potential runtime issues to the daemon more difficult.

nabahr avatar Sep 17 '24 03:09 nabahr

No argument on the failure checking. I've refactored it to make more sense. I also fixed the PIM version checking TODO's that got overlooked. @Jafaral also asked me to enable AutoRP discovery by default, which matches the behavior of Cisco. This caused a few problems with existing PIM tests so I fixed those in an extra commit.

nabahr avatar Sep 17 '24 21:09 nabahr

changes look good ... waiting on blockers to be cleared and ci to pass

passed CI actually.

Jafaral avatar Sep 24 '24 16:09 Jafaral

Why no testing of sending announcements? Basically in pim_autorp.c lines 531 - 890 are not even called. Pim code coverage actually goes down from 54.0% line coverage -> 53.3 % and function coverage is from 63.5% -> 63.0

Large swaths of cli that are added are not exercised at all as well.

donaldsharp avatar Sep 24 '24 17:09 donaldsharp

Why no testing of sending announcements? Basically in pim_autorp.c lines 531 - 890 are not even called. Pim code coverage actually goes down from 54.0% line coverage -> 53.3 % and function coverage is from 63.5% -> 63.0

Large swaths of cli that are added are not exercised at all as well.

Manual testing of the announcement messages was done using packet dumps during topotest, but without an AutoRP mapping agent running in the test, the only other way to automate it in topotest would require raw captures to validate the packet data. I couldn't find any examples of other tests that do this or any documentation on how to do this in topotest.

Completing the AutoRP mapping agent is on the agenda and will be added soon but it is a lower priority at this moment. Once that implementation is done, these tests will be fully flushed out.

nabahr avatar Sep 24 '24 17:09 nabahr