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
service: Update for L7 LB while locked #31744
Conversation
Labeled for author backport due to recent churn in the affected code. |
/test |
Keep service manager locked while reupserting services after L7 LB redirection updates. This removes the race where a service would be re-upserted while having been (concurrently) removed. Fixes: cilium#18894 Reported-by: Jussi Maki <joamaki@isovalent.com> Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
af14436
to
5c01103
Compare
/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.
Nice fix and cleanup - thanks Jarno!
This PR is linked to #18894, but I can't find the report for the problem this PR fixes. Could you give me a pointer to the report? |
#18894 introduced the racy code that is fixed here. Original code gets copy of a service and then adds it again to account for the changes in L7 LB service registrations. The problem is that this was done without holding a lock, making it possible that service that has been removed after the copy was created is erroneously added back. |
I see. Thanks for the explanation! |
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.
LGTM!
Keep service manager locked while updating services after L7 LB redirection updates. This removes the race where a service would be re-upserted while having been (concurrently) removed.
Fixes: #18894
Reported-by: Jussi Maki joamaki@isovalent.com