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 candidate BSR/RP support #16438

Merged
merged 8 commits into from
Sep 13, 2024
Merged

Conversation

Jafaral
Copy link
Member

@Jafaral Jafaral commented Jul 23, 2024

This Brings full BSR and RP candidate support to FRR (rfc 5059)

Replaces #9180 , in coordination with @eqvinox

@frrbot frrbot bot added documentation pim tests Topotests, make check, etc labels Jul 23, 2024
@Jafaral Jafaral force-pushed the pim-10.1-cand-rp branch 6 times, most recently from 1392c5a to 511528d Compare July 26, 2024 18:04
pimd/pim_bsm.c Outdated Show resolved Hide resolved
pimd/pim_nb_config.c Outdated Show resolved Hide resolved
@donaldsharp
Copy link
Member

Ran the test and got this new memory leaks:

  ## showing active allocations in memory group pimd
pimd:     PIM BSR C-RP                  :      2 *         96
pimd:     PIM BSR range                 :      3 *         80
pimd:     PIM BSR C-RP range item       :      4 *         88

My expectation is that we should not be adding new code that leaks

pimd/pim6_cmd.c Outdated Show resolved Hide resolved
pimd/pim6_cmd.c Outdated Show resolved Hide resolved
pimd/pim_cmd.c Outdated Show resolved Hide resolved
pimd/pim_cmd.c Outdated Show resolved Hide resolved
@Jafaral
Copy link
Member Author

Jafaral commented Aug 1, 2024

Ran the test and got this new memory leaks:

  ## showing active allocations in memory group pimd
pimd:     PIM BSR C-RP                  :      2 *         96
pimd:     PIM BSR range                 :      3 *         80
pimd:     PIM BSR C-RP range item       :      4 *         88

My expectation is that we should not be adding new code that leaks

will fix.

@Jafaral Jafaral force-pushed the pim-10.1-cand-rp branch 2 times, most recently from d63c2e9 to bbcfe38 Compare August 22, 2024 20:57
Copy link
Contributor

@nabahr nabahr left a comment

Choose a reason for hiding this comment

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

Just a few minor comments

doc/user/pim.rst Outdated Show resolved Hide resolved
pimd/pim6_cmd.c Outdated Show resolved Hide resolved
pimd/pim6_cmd.c Outdated Show resolved Hide resolved
lib/command.h Outdated Show resolved Hide resolved
pimd/pim_cmd.c Outdated Show resolved Hide resolved
doc/user/pim.rst Outdated Show resolved Hide resolved
pimd/pim_cmd.c Outdated Show resolved Hide resolved
pimd/pim_cmd.c Outdated Show resolved Hide resolved
pimd/pim_cmd.c Outdated Show resolved Hide resolved
pimd/pim6_cmd.c Outdated
event_timer_remain_msec(
scope->cand_rp_adv_timer));

vty_out(vty, "%s\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use "vty_json" instead of vty_out and json_object_free()

Copy link
Member

Choose a reason for hiding this comment

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

Why was this resolved? I started to write a comment about this as well, I see that vty_json is not being used in several spots

pimd/pim_bsm.c Outdated Show resolved Hide resolved
pimd/pim_msg.h Show resolved Hide resolved
@Jafaral
Copy link
Member Author

Jafaral commented Aug 26, 2024

Working on addressing comments from @nabahr and @mobash-rasool

@Jafaral Jafaral force-pushed the pim-10.1-cand-rp branch 4 times, most recently from 5d3d1b7 to 0158267 Compare August 29, 2024 05:35
pimd/pim6_cmd.c Outdated
vty_out(vty, "{}\n");
else
vty_out(vty,
"This router is not currently operating as Candidate RP\n");
Copy link
Member

Choose a reason for hiding this comment

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

I suggest adding this also to JSON output, e.g.:

{"warning": "..."}

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't a warning really, just informational.

Copy link
Member Author

Choose a reason for hiding this comment

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

I.e, the correct behavior it return an empty object.

Copy link
Member

Choose a reason for hiding this comment

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

Neither agree, nor disagree, e.g. we have in BGP such patterns:

	if (!peer) {
		if (use_json) {
			json_object *json_no = NULL;
			json_no = json_object_new_object();
			json_object_string_add(json_no, "warning",
					       "No such neighbor in this view/vrf");
			vty_json(vty, json_no);
		} else
			vty_out(vty, "No such neighbor in this view/vrf\n");
		return NULL;
	}

or

	if (is_synchronized(rpki_vrf)) {
		if (strmatch(prefixkind, "prefix-count"))
			print_prefix_table(vty, rpki_vrf, json, true);
		else
			print_prefix_table(vty, rpki_vrf, json, false);
	} else {
		if (json) {
			json_object_string_add(json, "error", "No Connection to RPKI cache server.");
			vty_json(vty, json);
		} else
			vty_out(vty, "No connection to RPKI cache server.\n");
		return CMD_WARNING;
	}

Anyway, it's not a blocker. Just an improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both of these are a bit different. The first , if you query a neighbor that doesn't exist, a warning is a good idea about it. The second case is an error, so that does need an error return. So I see why those have them. The case I have is purely informational.

pimd/pim6_cmd.c Outdated Show resolved Hide resolved
pimd/pim_bsm.c Outdated Show resolved Hide resolved
pimd/pim_bsm.c Outdated Show resolved Hide resolved
@Jafaral
Copy link
Member Author

Jafaral commented Aug 29, 2024

I addressed all comments on this PR.

@Jafaral Jafaral force-pushed the pim-10.1-cand-rp branch 4 times, most recently from 322cda5 to 073c6d5 Compare September 9, 2024 14:20
@github-actions github-actions bot added the rebase PR needs rebase label Sep 9, 2024
Signed-off-by: David Lamparter <[email protected]>
Signed-off-by: Jafar Al-Gharaibeh <[email protected]>
Signed-off-by: Jafar Al-Gharaibeh <[email protected]>
This change re-adds an additional check bsr rpf that was removed
in 2c6a32f.

Without that check, bsr messages is resent (causing a loop) if we
have more than one pim neighbor on a link.

Signed-off-by: Jafar Al-Gharaibeh <[email protected]>
@donaldsharp donaldsharp merged commit f3fc33e into FRRouting:master Sep 13, 2024
11 checks passed
Jafaral added a commit that referenced this pull request Nov 12, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation master pim rebase PR needs rebase size/XXL tests Topotests, make check, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants