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

Duplicate fib proposal #16342

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Conversation

pguibert6WIND
Copy link
Member

For route updates:

  • add duplicate flag wherever needed
  • remove the fib flag when duplicate is present

@pguibert6WIND
Copy link
Member Author

ci:rerun

1 similar comment
@pguibert6WIND
Copy link
Member Author

ci:rerun

During the bgp_peer_type_multipath_relax_test, the test does not
check the 'duplicate' flag value of the duplicate nexthop.

Fix this by adding the duplicate value in the expected json files.

Fixes: ee88563 ("bgpd: Add 'bgp bestpath peer-type multipath-relax'")

Signed-off-by: Philippe Guibert <[email protected]>
@pguibert6WIND pguibert6WIND force-pushed the duplicate_fib_proposal branch from 9f71889 to f328bbb Compare July 8, 2024 13:30
The bgp_duplicate_nexthop test installs routes with nexthop's
flags set to both DUPLICATE and FIB: this should not happen.

The DUPLICATE flag of a nexthop indicates this nexthop is already
used in the same nexthop-group, and there is no need to install it
twice in the system; having the FIB flag set indicates that the
nexthop is installed in the system. This is why both flags should
not be set on the same nexthop.

This case happens at installation time, but can also happen
at update time.
- Fix this by not setting the FIB flag value when the DUPLICATE
flag is present.
- Modify the bgp_duplicate_test to check that the FIB flag is not
present on duplicated nexthops.
- Modify the bgp_peer_type_multipath_relax test.

Signed-off-by: Philippe Guibert <[email protected]>
@pguibert6WIND pguibert6WIND force-pushed the duplicate_fib_proposal branch from f328bbb to 731f74e Compare July 8, 2024 13:43
Copy link

mergify bot commented Jul 8, 2024

⚠️ The sha of the head commit of this PR conflicts with #16332. Mergify cannot evaluate rules on this PR. ⚠️

pguibert6WIND added a commit to pguibert6WIND/frr that referenced this pull request Jul 8, 2024
The bgp_duplicate_nexthop test installs routes with nexthop's
flags set to both DUPLICATE and FIB: this should not happen.

The DUPLICATE flag of a nexthop indicates this nexthop is already
used in the same nexthop-group, and there is no need to install it
twice in the system; having the FIB flag set indicates that the
nexthop is installed in the system. This is why both flags should
not be set on the same nexthop.

This case happens at installation time, but can also happen
at update time.
- Fix this by not setting the FIB flag value when the DUPLICATE
flag is present.
- Modify the bgp_duplicate_test to check that the FIB flag is not
present on duplicated nexthops.
- Modify the bgp_peer_type_multipath_relax test.

Link: FRRouting#16342
Signed-off-by: Philippe Guibert <[email protected]>
Copy link

mergify bot commented Jul 8, 2024

⚠️ The sha of the head commit of this PR conflicts with #16332. Mergify cannot evaluate rules on this PR. ⚠️

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.

this looks correct to me, but it would be good if @mjs looked at it

@Jafaral Jafaral requested a review from mjstapp July 9, 2024 15:30
Copy link
Contributor

@mjstapp mjstapp 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, thanks

@mjstapp mjstapp merged commit 7d08b29 into FRRouting:master Jul 9, 2024
17 checks passed
@mjs
Copy link

mjs commented Jul 10, 2024

@riw777 You probably didn't mean to mention me. I have nothing to do with this project :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants