Skip to content

Conversation

rastislavs
Copy link
Contributor

@rastislavs rastislavs commented Oct 2, 2024

As multiple services can share the same VIP (e.g. using lbipam.cilium.io/sharing-key), the service reconciliation logic needs to be adapted to not withdraw a route to VIP that is still in use by some other service.

To address that, this change introduces resource reference counting by reconciling service advertisements.

This fix is specific to BGPv2 service reconciliation only.

Part of #35018 (BGPv2 only)

@rastislavs rastislavs added release-note/bug This PR fixes an issue in a previous release of Cilium. area/bgp Impacts the Border Gateway Protocol feature. needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Oct 2, 2024
@rastislavs
Copy link
Contributor Author

/test

@rastislavs rastislavs marked this pull request as ready for review October 2, 2024 13:15
@rastislavs rastislavs requested a review from a team as a code owner October 2, 2024 13:15
Copy link
Contributor

@harsimran-pabla harsimran-pabla left a comment

Choose a reason for hiding this comment

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

Thanks, very nice fix. Minor change required, otherwise lgtm 🚀

@rastislavs rastislavs force-pushed the shared-svc-bgpv2 branch 2 times, most recently from 0506739 to f7473bf Compare October 3, 2024 13:26
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.

Now it looks simple enough. Thanks!

@rastislavs
Copy link
Contributor Author

/test

@rastislavs
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 4, 2024
@rastislavs rastislavs added dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 4, 2024
As multiple services can share the same VIP (e.g. using
lbipam.cilium.io/sharing-key), the service reconciliation logic
needs to be adapted to not withdraw a route to VIP that is
still in use by some other service.
To address that, this change introduces a new ReconcileResourceAFPaths
method for reconciling BGP advertisements per resource and address family
with integrated resource reference conting, that is used in
the Service and PodIPPool reconcilers.
This fix is specific to BGPv2 resource reconciliation only.

Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
@rastislavs rastislavs removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Oct 4, 2024
@rastislavs
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 6, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue Oct 7, 2024
Merged via the queue into cilium:main with commit 4cceb0f Oct 7, 2024
65 checks passed
@giorio94 giorio94 mentioned this pull request Oct 7, 2024
15 tasks
@giorio94 giorio94 added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Oct 7, 2024
@github-actions github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bgp Impacts the Border Gateway Protocol feature. backport-done/1.16 The backport for Cilium 1.16.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
Development

Successfully merging this pull request may close these issues.

5 participants