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

PIM: Implement static IGMP joins without an IGMP report #16450

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

nabahr
Copy link
Contributor

@nabahr nabahr commented Jul 23, 2024

Implement a new ip igmp static-group ... command separate from the ip igmp join ... command. The latter command joins the group via a local socket and generates an IGMP report. The new static-group command will join the group but bypasses the IGMP reports. This is analogous to the Cisco static-group command.
Hide the previous ip igmp join ... command and added a new alias for ip igmp join-group ....
Updated documentation for the new command and alias.
Added show command for the static-groups separate from the join-groups.

@nabahr
Copy link
Contributor Author

nabahr commented Jul 24, 2024

ci:rerun

pimd/pim_cmd.c Outdated Show resolved Hide resolved
pimd/pim6_cmd.c Outdated Show resolved Hide resolved
pimd/pim6_cmd.c Outdated Show resolved Hide resolved
pimd/pim_iface.c Outdated Show resolved Hide resolved
pimd/pim_iface.c Outdated Show resolved Hide resolved
Copy link
Member

@donaldsharp donaldsharp left a comment

Choose a reason for hiding this comment

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

topotest needed as well as my question around yang validation answered

@nabahr
Copy link
Contributor Author

nabahr commented Aug 15, 2024

@donaldsharp I agree with the comments here. I would say most of them are as a result of mirroring the static join code which was old/wrong to begin with. Should I go ahead and update the static join parts as well or just leave it alone for now?

@donaldsharp
Copy link
Member

I would say update where we can, unfortunately

@donaldsharp
Copy link
Member

for the yang stuff let's double check my thoughts with the more yang minded people in our group. I've already asked them to do so.

@nabahr
Copy link
Contributor Author

nabahr commented Aug 15, 2024

Looks like the group address in the nb validation does check for reserved group addresses but does not verify it's a multicast address, but the group address type in the yang is ietf-routing-types:ip-multicast-group-address.
So I assume trying to set an address that is not a multicast group will result in an error already but might not result in a friendly error message.

nabahr added 3 commits August 15, 2024 16:20
This will add a static IGMP group that does not rely on an underlying
socket join which sends traffic to the cpu unneccesarily. Instead, the
groups are joined directly without any IGMP interactions.
New command is under interfaces, 'ip igmp static-group ...'.
Added an alias for 'ip igmp join ...' to 'ip igmp join-group'.
Moved IGMP join groups to new yang list "join-group" and reused
the "static-group" list for the IGMP static groups.

Signed-off-by: Nathan Bahr <[email protected]>
Copied the existing "join-group" test and modified to test
static groups instead. Functionally the same but without IGMP
reports.

Signed-off-by: Nathan Bahr <[email protected]>
@frrbot frrbot bot added the tests Topotests, make check, etc label Aug 15, 2024
@github-actions github-actions bot added size/XXL rebase PR needs rebase and removed size/XL labels Aug 15, 2024
@nabahr nabahr requested a review from donaldsharp August 16, 2024 19:58
Copy link
Member

@donaldsharp donaldsharp left a comment

Choose a reason for hiding this comment

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

lgtm now

@donaldsharp donaldsharp merged commit 05c17ef into FRRouting:master Aug 22, 2024
13 checks passed
@nabahr nabahr deleted the static_joins branch August 22, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants