PIM: Implement AutoRP functionality #16634
Conversation
nabahr
force-pushed
the
autorp
branch
4 times, most recently
from
1902f6d to
e4a3784
Compare
tests/topotests/multicast_pim_autorp/test_multicast_pim_autorp.py
Outdated
Show resolved
Hide resolved
tests/topotests/multicast_pim_autorp/test_multicast_pim_autorp.py
Outdated
Show resolved
Hide resolved
nabahr
force-pushed
the
autorp
branch
3 times, most recently
from
f6bd605 to
ff67de5
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
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. |
nabahr
force-pushed
the
autorp
branch
3 times, most recently
from
b4b9158 to
8a54168
Compare
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. |
There was a problem hiding this comment.
I'm not a fan of negative numbers for success and 0 for failure. Especially when all that is ever done is did this function succeed fail. In addition by not testing for negative numbers for success the if statements just looking at them in a vacumn really looks like the function failed. Please go through and use bool to indicate success/failure and have the functions test appropriately where used.
|
No argument on the failure checking. I've refactored it to make more sense. |
tests/topotests/multicast_pim_sm_topo3/igmp_single_if_group_all_brief.json
Outdated
Show resolved
Hide resolved
nabahr
force-pushed
the
autorp
branch
2 times, most recently
from
656d150 to
5ac93a9
Compare
passed CI actually. |
Perform AutoRP discovery and candidate RP announcements using the AutoRP protocol. Mapping agent is not yet implemented, but this feature is not necessary for FRR to support AutoRP as we only need one AutoRP mapping agent in the network. Signed-off-by: Nathan Bahr <[email protected]>
New CLI commands added:
router pim [vrf NAME]
autorp discovery
autorp announce RP-ADDR [GROUP | group-list PREFIX-LIST]
autorp announce {scope (1-255) | interval (1-65535) | holdtime (0-65535)}
autorp discovery
Enables Auto RP discovery for learning dynamic RP information using the
AutoRP protocol.
autorp announce RP-ADDR [GROUP | group-list PREFIX-LIST]
Enable announcements of a candidate RP with the given group range, or
prefix list of group ranges, to an AutoRP mapping agent.
autorp announce {scope (1-255) | interval (1-65535) | holdtime (0-65535)}
Configure the parameters of the AutoRP announcement messages.
The scope sets the packet TTL.
The interval sets the time between TX of announcements.
The holdtime sets the hold time in the message, the time the mapping
agent should wait before invalidating the candidate RP information.
debug pim autorp
Enable debug logging of the AutoRP protocol
show ip pim [vrf NAME] autorp [json]
Show details of the AutoRP protocol.
To view learned RP info, use the existing command 'show ip pim rp-info'
Extend pim yang for new configuration:
augment /frr-rt:routing/frr-rt:control-plane-protocols/frr-rt:control-plane-protocol/frr-pim:pim/frr-pim:address-family:
+--rw rp
+--rw auto-rp
+--rw discovery-enabled? boolean
+--rw announce-scope? uint8
+--rw announce-interval? uint16
+--rw announce-holdtime? uint16
+--rw candidate-rp-list* [rp-address]
+--rw rp-address inet:ip-address
+--rw (group-or-prefix-list)?
+--:(group)
| +--rw group? frr-route-types:ip-multicast-group-prefix
+--:(prefix-list)
+--rw prefix-list? plist-ref
Signed-off-by: Nathan Bahr <[email protected]>
Document the CLI commands to configure AutoRP discovery and candidate RP announcements. Signed-off-by: Nathan Bahr <[email protected]>
Uses hardcoded sample AutoRP packets injected in to test message parsing and proper application of AutoRP learned RP info. Tests mix of AutoRP and static RP's. Signed-off-by: Nathan Bahr <[email protected]>
With AutoRP discovery running by default, that adds a new IGMP group that needs to be accounted for in IGMP output. For multicast_pim_sm_topo3: Ignore the total group number as it is unnecessary for the test. Signed-off-by: Nathan Bahr <[email protected]>
With AutoRP discovery running by default, that adds a new IGMP group that needs to be accounted for in IGMP output. For pim.py The clear IGMP interfaces function is in a broken state. It was already ignoring any errors and returned true always, but with the addition of the AutoRP discovery group, you could end up with a different group order in the json which would cause a key error making the test fail. For now I just added a check to avoid the key error. Signed-off-by: Nathan Bahr <[email protected]>
|
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. |
New Features Highlight: - PIM candidate BSR/RP [#16438] - Static IGMP join without an IGMP report [1#6450] - PIM AutoRP discovery/announcements [#16634] - IGMP proxy [#16861] - SRv6 SID Manager [#15604] - Add `bgp ipv6-auto-ra` command [#16354] - Implement `neighbor x remote-as auto` for BGP [#16345] - Implement `bgp dual-as` for BGP [#16816] - Implement BGP-wide configuration for graceful restart [#16099] - Handle kernel routes appropriately (should fix recent NOPREFIXROUTE issue) [#16300] - Add `cisco-authentication` password support for NHRP [#16172] Signed-off-by: Jafar Al-Gharaibeh <[email protected]>
This adds most of the AutoRP functionality to PIMD for dynamic RP support using the AutoRP protocol.
RP Discovery is supported and is enabled by default. To disable AutoRP discovery:
When discovery is enabled, we will listen for the AutoRP Discovery message sent by the AutoRP mapping agent and we will use the discovered RP information.
AutoRP announcement support is also implemented. To configure PIM to advertise a candidate RP via AutoRP:
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-infocommand 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