-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Remove deadlock from AuthMap Endpoint GC #29082
Conversation
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.
couple of suggestions; but yep I believe this fixes the deadlock 👍
For the release note, typically we'd want to describe the impact of the bug on the user, rather than a description of what the bug is. For example, we'd want to say something like "Fix potential deadlock that results in stale authentication entries in Cilium". |
a325c56
to
d60d688
Compare
b44c74a
to
55a2d96
Compare
1837248
to
1e26bbf
Compare
/test |
/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. Glad we ended up no longer having a dependency on the endpointrepo from the authmap cache at all. 🙏
Only some non-blocking suggestions that i like to discuss.
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.
Also solves the deadlock as far as I can tell 👍
1e26bbf
to
81b0144
Compare
This change keeps a copy of the endpoints map inside the Autentication GC code. It will be updated on subscribe events, This is then used in the AuthMap GC code instead of doing a call that caused a deadlock. Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
81b0144
to
19dda85
Compare
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.
Good to go from my side! Thanks @meyskens
/test |
@@ -10,11 +10,15 @@ type Subscriber interface { | |||
// EndpointCreated is called at the end of endpoint creation. | |||
// Implementations must not attempt write operations on the | |||
// EndpointManager from this callback. | |||
// This function is being called inside a RLock, so it must not attempt |
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.
just an fyi: We typically follow the convention - EndpointCreatedLocked
.
This changes that a create and delete event from the EndpointManager will also give a "state of the world" update of all current endpoints it has.
This allows the subscribers to check against other endpoints without calling a function that issues a lock when the event was send inside an RLock.
This is then used in the AuthMap GC code instead of doing a call that caused a deadlock.
Fixes: #29078