-
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
Fix Unlock handling for kvstore locks #9973
Conversation
test-me-please |
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.
Only a minor nit but I think it's important to track context TODOs correctly.
pkg/allocator/allocator.go
Outdated
@@ -443,7 +443,7 @@ func (a *Allocator) lockedAllocate(ctx context.Context, key AllocatorKey) (idpoo | |||
return 0, false, err | |||
} | |||
|
|||
defer lock.Unlock(ctx) | |||
defer lock.Unlock(context.TODO()) |
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.
I think we want Background()
as this is not a context that has yet to be defined but it's a context that should never expire.
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.
ah that's a good point. Fixed
ee53547
to
886a28c
Compare
test-me-please |
test-me-please |
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.
Yikes.
Using the same context used to lock a key is not the right functionality that we want as it can cause distributed locking issues. For example, in case a client gets a distributed key and the context is canceled while that lock is being held, the Unlock() functions will not execute since that same context was canceled leaving the distributed lock locked until the client disconnects from the kvstore. Fixes: d3ad5c7 ("kvstore: plumb context") Fixes: 157089d ("allocator: add context to `RunGC`") Signed-off-by: André Martins <andre@cilium.io>
Earlier returning from the function stating the lock was acquired it might make the caller of waitForInitLock to cancel the context to earlier which makes the Unlock operation to fail. Althought this context bug was fixed by "pkg/kvstore: use different context for lock and unlock" the right behavior of this function should be to return successfully after the Unlock was performed. Signed-off-by: André Martins <andre@cilium.io>
886a28c
to
1c2a453
Compare
test-me-please |
This PR fixes a couple bugs encountered in master with regarding the handling of unlock function. Please read per commit for more info.
Does not f i x: #9855
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)