-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix loadbalancer handling of backends with ClusterID set #40968
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
Conversation
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>
/test |
There was a problem hiding this 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>
e4cba20
to
4415a62
Compare
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. |
/test |
Looks reasonable to me thanks for the explanation! |
Please review commit by commit, and refer to the individual messages for additional details.