Skip to content

improvements for bgp router ID ip pool #40340

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
Merged
merged 3 commits into from
Aug 14, 2025
Merged

improvements for bgp router ID ip pool #40340

merged 3 commits into from
Aug 14, 2025

Conversation

liyihuang
Copy link
Contributor

@liyihuang liyihuang commented Jul 3, 2025 •
edited
Loading

This is the enhacenment PR of #38300

  • Enhances the router ID override logic for RouterIDIPPool mode so it can update the exsiting allocation accordingly.
  • Fix my previous unit test of using incorrect return with EventuallyWithT and other errors in previous test codes in bgp router ID ip pool implementation #38300
  • Update the BGP docs to reflect the new RouterIDIPPool mode.
Fix a BGP bug where the routerID specified in a CiliumBGPNodeConfigOverride was not correctly updated in RouterIDIPPool mode.

@liyihuang liyihuang added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. area/bgp Impacts the Border Gateway Protocol feature. labels Jul 3, 2025
@liyihuang liyihuang force-pushed the pr/liyihuang/bgp_rotuer_id_ip_pool_doc branch from 5d25e62 to b766126 Compare July 3, 2025 03:14
@liyihuang liyihuang mentioned this pull request Jul 3, 2025
Merged
@liyihuang
Copy link
Contributor Author

/test

@liyihuang liyihuang marked this pull request as ready for review July 3, 2025 17:43
@liyihuang liyihuang requested review from a team as code owners July 3, 2025 17:43
@joestringer
Copy link
Member

@yushoyamaguchi would you be interested to review the content here? I saw you raised this discussion topic on #38300.

joestringer
joestringer requested changes Jul 3, 2025
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Couple of comments from @cilium/docs-structure perspective to ensure consistency with docs style.

@joestringer joestringer added the needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch label Jul 3, 2025
@yushoyamaguchi
Copy link
Contributor

@joestringer
Thank you for your mentioning.
The content of this commit looks good to me!

@liyihuang
Copy link
Contributor Author

I have found some issues with my orginal PR in the test while adding the test in this PR. I will put this back in draft.

@liyihuang liyihuang marked this pull request as draft July 7, 2025 20:36
@liyihuang liyihuang changed the title docs: add bgp IP pool router-id allocation mode improvements for bgp router ID ip pool Jul 7, 2025
@liyihuang liyihuang force-pushed the pr/liyihuang/bgp_rotuer_id_ip_pool_doc branch from 41db7c5 to 476a1a0 Compare July 8, 2025 03:08
@liyihuang
Copy link
Contributor Author

/test

@liyihuang liyihuang force-pushed the pr/liyihuang/bgp_rotuer_id_ip_pool_doc branch from 476a1a0 to 4f81484 Compare July 8, 2025 03:18
@liyihuang
Copy link
Contributor Author

/test

@liyihuang liyihuang force-pushed the pr/liyihuang/bgp_rotuer_id_ip_pool_doc branch 2 times, most recently from f19b22c to 6229d78 Compare July 8, 2025 13:07
@liyihuang
Copy link
Contributor Author

/test

@joestringer
Copy link
Member

👋

I see the release note is currently:

Enhances the router ID override logic for RouterIDIPPool mode and update the unit test and docs around RouterIDIPPool mode

However, this seems a bit vague for users and includes unnecessary details. What is changing about the router ID override logic? I would also not mention unit tests or docs if this is making a behavior change (all behavioral changes should be accompanied by unit tests and docs, so the user doesn't need to read this release note to know about those changes).

@joestringer joestringer added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Jul 8, 2025
@joestringer
Copy link
Member

joestringer commented Jul 8, 2025 •
edited
Loading

One more comment, I think it may be worth while to split the PR into two PRs - one for docs that we backport to v1.18 and document the change in behavior, and one separate PR for the functional changes which we will merge into main for v1.19 early next year. It's getting late in the cycle to make functional changes to existing functionality in v1.18 unless there's some severe repercussion or bug that is being fixed.

YutaroHayakawa
YutaroHayakawa approved these changes Aug 7, 2025
View reviewed changes
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

I think now we're in a good shape. Thanks for your patience. We still have a code clarity issue I raised in this comment, but the logic looks correct to me now, so I won't block this PR further.

@YutaroHayakawa
Copy link
Member

/test

@liyihuang
Copy link
Contributor Author

I think now we're in a good shape. Thanks for your patience. We still have a code clarity issue I raised in this comment, but the logic looks correct to me now, so I won't block this PR further.

I totally understand it. I will push another commit to move the restore the logic. the previous comment was trying to see if there is anything that I need to change for my overall logcis besides moving restore the logic to another place

@liyihuang liyihuang force-pushed the pr/liyihuang/bgp_rotuer_id_ip_pool_doc branch 2 times, most recently from e78cef6 to a7ef800 Compare August 7, 2025 17:14
@liyihuang
Copy link
Contributor Author

@YutaroHayakawa

I have the following changes in my latest commits

  • Moved the restore logic to its own function and put it in initializeJobs() .
  • Added a small example to the doc

Please let me know if you have any more questions.

Thanks for review

@liyihuang
Copy link
Contributor Author

/test

@liyihuang
docs: add bgp IP pool router-id allocation mode
325876d 
This is the follow up PR of cilium#38300
where I didn't upadte the docs after implementing the feature

Signed-off-by: Liyi Huang <liyi.huang@isovalent.com>
@liyihuang
fix(tests): Update BGP cluster config tests
242e028 
This commit is to fix my previous unit test of using incorrect return with EventuallyWithT and other errors in previous test codes.

Signed-off-by: Liyi Huang <liyi.huang@isovalent.com>
@liyihuang
bgp: Enhance router ID allocation logic and update tests
572c6bd 
Enhance the router ID override logic for RouterIDIPPool mode so it can update the exsiting allocation accordingly.
Move the restore logic to initializeJobs so the overall logic is clear

Update the unit test for override to cover new function `handleRouterIDOverride`

Signed-off-by: Liyi Huang <liyi.huang@isovalent.com>
@liyihuang liyihuang force-pushed the pr/liyihuang/bgp_rotuer_id_ip_pool_doc branch from a7ef800 to 572c6bd Compare August 7, 2025 19:24
@liyihuang
Copy link
Contributor Author

/test

@YutaroHayakawa YutaroHayakawa added this pull request to the merge queue Aug 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2025
@joestringer joestringer added this pull request to the merge queue Aug 14, 2025
Merged via the queue into cilium:main with commit 41a303a Aug 14, 2025
68 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Aug 14, 2025
@liyihuang liyihuang deleted the pr/liyihuang/bgp_rotuer_id_ip_pool_doc branch August 16, 2025 02:11
@joamaki joamaki mentioned this pull request Aug 19, 2025
Merged
19 tasks
@joamaki joamaki added backport-pending/1.18 The backport for Cilium 1.18.x for this PR is in progress. and removed needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch labels Aug 19, 2025
@github-actions github-actions bot added backport-done/1.18 The backport for Cilium 1.18.x for this PR is done. and removed backport-pending/1.18 The backport for Cilium 1.18.x for this PR is in progress. labels Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

+1 more reviewer

@yushoyamaguchi yushoyamaguchi yushoyamaguchi approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area/bgp Impacts the Border Gateway Protocol feature. area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. backport-done/1.18 The backport for Cilium 1.18.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@liyihuang @joestringer @yushoyamaguchi @YutaroHayakawa @joamaki