Skip to content

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented Aug 6, 2025

Please review commit by commit, and refer to the individual messages for additional details.

To mimic the same method provided by [netip.Addr]. Additionally, let's
reimplement the [AddrCluster.Less] on top of it.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Let's make use of the newly introduced [AddrCluster.Compare] function,
to ensure consistent ordering also when there are multiple backends
associated with the same IP, but different ClusterID.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94 giorio94 added area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/misc This PR makes changes that have no direct user impact. area/loadbalancing Impacts load-balancing and Kubernetes service implementations labels Aug 6, 2025
@giorio94
Copy link
Member Author

giorio94 commented Aug 6, 2025

/test

@giorio94 giorio94 added the needs-backport/1.18 This PR / issue needs backporting to the v1.18 branch label Aug 6, 2025
@giorio94 giorio94 requested a review from joamaki August 6, 2025 12:22
@giorio94 giorio94 marked this pull request as ready for review August 6, 2025 12:22
@giorio94 giorio94 requested review from a team as code owners August 6, 2025 12:22
@giorio94 giorio94 requested a review from MrFreezeex August 6, 2025 12:22
Copy link
Member

@MrFreezeex MrFreezeex left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

I left some small non blocking question/suggestion inline.

Also guessing this should be tagged as a bug release note rather than a misc one maybe?

Currently, the loadbalancer reconciler is affected by a bug causing the
incorrect pruning of backends if associated with a non-zero ClusterID,
because the ClusterID read from the map is ignored. Let's get this
fixed by unifying the [BackendValue.{GetAddress,GetIPCluster]] methods,
to ensure that all callers correctly account for the ClusterID.

Additionally, let's also extend the unit tests to cover this use-case
and prevent possible regressions.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94 giorio94 force-pushed the mio/lb-fix-clusterid branch from e4cba20 to 4415a62 Compare August 6, 2025 15:27
@giorio94
Copy link
Member Author

giorio94 commented Aug 6, 2025

Also guessing this should be tagged as a bug release note rather than a misc one maybe?

I've marked it as misc because the ClusterID is never set to a value different from zero in this context today, AFAIK, so it doesn't really affect users. The main reason I've marked it for backport is to avoid having the two implementations diverge for the moment, given that the fix is trivial. But I can definitely remove the backport label is people feel against it.

@giorio94
Copy link
Member Author

giorio94 commented Aug 6, 2025

/test

@giorio94 giorio94 enabled auto-merge August 6, 2025 15:35
@MrFreezeex
Copy link
Member

Also guessing this should be tagged as a bug release note rather than a misc one maybe?

I've marked it as misc because the ClusterID is never set to a value different from zero in this context today, AFAIK, so it doesn't really affect users. The main reason I've marked it for backport is to avoid having the two implementations diverge for the moment, given that the fix is trivial. But I can definitely remove the backport label is people feel against it.

Looks reasonable to me thanks for the explanation!

@giorio94 giorio94 added this pull request to the merge queue Aug 7, 2025
@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 Aug 7, 2025
Merged via the queue into cilium:main with commit f28cb90 Aug 7, 2025
67 of 68 checks passed
@giorio94 giorio94 deleted the mio/lb-fix-clusterid branch August 7, 2025 15:55
@YutaroHayakawa YutaroHayakawa mentioned this pull request Aug 11, 2025
10 tasks
@YutaroHayakawa YutaroHayakawa 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 11, 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 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/loadbalancing Impacts load-balancing and Kubernetes service implementations 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/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants