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.

Sign up for GitHub

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

Jump to bottom

Router pim config #16269

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Router pim config #16269

merged 3 commits into from
Jul 18, 2024

Conversation

nabahr
Copy link
Contributor

@nabahr nabahr commented Jun 21, 2024 •
edited by Jafaral
Loading

Added new 'router pim[6] [vrf NAME]' config node.

Moved all existing global/vrf PIM configs to the new subnode and the existing configuration was set to be hidden and deprecated.
Both versions of the configuration still work together for the time being until they can be safely removed.

PIM now always writes out configuration in the "router pim" syntax so ,in order to support frr-reload, the JSON based configuration in the PIM topotests also needs to generate configuration in the same syntax.

Config file style tests are left in the global pim configuration syntax for now as a test of backwards compatibility.

Fixes #16196

@eqvinox
Copy link
Contributor

eqvinox commented Jun 24, 2024

oh god that diff size T_T …

@eqvinox eqvinox self-requested a review June 24, 2024 14:38
eqvinox
eqvinox reviewed Jun 24, 2024
View reviewed changes
pimd/pim_cmd_common.h Outdated
* that VTY_CURR_XPATH is correct and/or uses relative paths, and at
* the end vty is moved back to the original node and xpath popped
*/
#define START_PIM_DEPRECATED(clinode) \
Copy link
Contributor

Choose a reason for hiding this comment

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

When and how are we going to get rid of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it could go away now, but I'd say once it's been long enough that no body remembers the previous PIM syntax then this stuff can be removed.

eqvinox
eqvinox reviewed Jun 24, 2024
View reviewed changes
pimd/pim_vty.c

if (pim->vrf->vrf_id == VRF_DEFAULT)
Copy link
Contributor

Choose a reason for hiding this comment

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

is the indentation off now, for router pim blocks inside VRFs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VRF-aware PIM configuration is also at the top level.

router pim
  ...
router pim vrf blue
  ...

@nabahr
Copy link
Contributor Author

nabahr commented Jun 24, 2024

oh god that diff size T_T …

Yeah. I tried different ways to ALIAS the commands but couldn't get it to work so I essentially had to duplicate all of the global PIM commands. Also all of the pim global xpath's have been touched, since now the current XPath is set and commands use relative paths.
I am happy to hear any other ideas.

@Jafaral
Copy link
Member

Jafaral commented Jun 24, 2024

Documentation needs to be updated to reflect the new syntax.

@nabahr nabahr force-pushed the router_pim_config branch 3 times, most recently from fb6fb8e to f6ab303 Compare June 25, 2024 13:33
@riw777 riw777 self-requested a review June 25, 2024 15:24
@Jafaral Jafaral added the release-notes should be added to release notes label Jun 25, 2024
@nabahr nabahr force-pushed the router_pim_config branch 4 times, most recently from cce7ba2 to 91c3882 Compare June 27, 2024 14:58
@nabahr
Copy link
Contributor Author

nabahr commented Jun 28, 2024 •
edited
Loading

Verify source still fails for these 2 issues that won't be fixed

> < WARNING: Comparisons should place the constant on the right side of the test
< #5618: FILE: /tmp/f1-589488/pim_cmd_common.c:5618:

This is a simple comparison of two defines, either order produces this warning.

> < WARNING: Macros with flow control statements should be avoided
< #207: FILE: /tmp/f1-589488/pim_cmd_common.h:207:

This macro is meant to set up a few function variables and set the correct XPath for the deprecated CLI handlers. I can remove the macro if required but that'll require copying this duplicate code to all of the deprecated CLI handlers. As it is now I can't fix this warning.

Fixed these warnings now.

@nabahr nabahr force-pushed the router_pim_config branch from 91c3882 to 702700e Compare July 1, 2024 16:00
Jafaral
Jafaral reviewed Jul 8, 2024
View reviewed changes
pimd/pim_cmd_common.c Outdated
continue;

snprintfrr(framestr, sizeof(framestr), "router pim%s",
(PIM_AF == AF_INET6 ? "6" : ""));
Copy link
Member

Choose a reason for hiding this comment

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

Common code between pim and pim6. PIM_AF == AF_INET6 triggers

WARNING: Comparisons should place the constant on the right side of the test

The warning is trigged regardless of the order, a rare use case.

@riw777 riw777 removed their request for review July 9, 2024 02:53
@nabahr nabahr force-pushed the router_pim_config branch 3 times, most recently from 848eb56 to 6776a28 Compare July 15, 2024 14:25
@riw777 riw777 self-requested a review July 16, 2024 15:23
@nabahr
pimd, lib, vtysh: Added new 'router pim[6] [vrf NAME]' config node
fd8edc3 
Moved all existing global/vrf PIM config to the new subnode.
Existing configuration updated to be hidden and deprecated.
Both versions of configuration still work together.

Signed-off-by: Nathan Bahr <[email protected]>
@nabahr nabahr force-pushed the router_pim_config branch 2 times, most recently from 41409c2 to ffbc081 Compare July 16, 2024 18:39
nabahr added 2 commits July 16, 2024 13:41
@nabahr
tools: Fix frr-reload to support legacy pim configuration from file
98d47f4 
Fix load from file in frr-reload to detect and convert legacy pim
configuration so that the tool can continue to be used with legacy
configurations.

Signed-off-by: Nathan Bahr <[email protected]>
@nabahr
doc: Update PIM[6] configure docs
b58592d 
Document 'router pim[6] [vrf NAME]' configuration.
All the commands are basically the same, just dropped the
'ip pim[6]' prefix and document them under the router pim block.

Signed-off-by: Nathan Bahr <[email protected]>
@nabahr nabahr force-pushed the router_pim_config branch from ffbc081 to b58592d Compare July 16, 2024 18:41
@nabahr
Copy link
Contributor Author

nabahr commented Jul 16, 2024

Added a fix to frr-reload to detect and convert legacy pim configuration so that the tool works as expected. I reverted the changes to topotests so they are all back to using the legacy pim configuration.

riw777
riw777 approved these changes Jul 18, 2024
View reviewed changes
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

looks good

@riw777 riw777 merged commit 8dae5cf into FRRouting:master Jul 18, 2024
11 checks passed
@nabahr nabahr deleted the router_pim_config branch July 19, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eqvinox eqvinox eqvinox left review comments

@Jafaral Jafaral Jafaral approved these changes

@riw777 riw777 riw777 approved these changes

Assignees
No one assigned
Labels
documentation libfrr master pim release-notes should be added to release notes size/XXL tests Topotests, make check, etc tools vtysh
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nabahr @eqvinox @Jafaral @riw777