Add per-service mutex to prevent concurrent reconciliation - #57
Conversation
12a02c9 to
98b6400
Compare
5f32ead to
7be51a0
Compare
98b6400 to
137ebee
Compare
| func (l *loadbalancer) lockForService(uid types.UID) func() { | ||
| rawMu, _ := l.muMap.LoadOrStore(string(uid), new(sync.Mutex)) | ||
| mu := rawMu.(*sync.Mutex) | ||
| klog.V(4).InfoS("acquiring service lock", "uid", uid) | ||
| mu.Lock() | ||
|
|
||
| return func() { | ||
| klog.V(4).InfoS("releasing service lock", "uid", uid) | ||
| mu.Unlock() | ||
| } |
There was a problem hiding this comment.
I compared it with: https://github.com/cloudscale-ch/terraform-provider-cloudscale/blob/master/cloudscale/mutex_kv.go
- any reason why you did not use the channel pattern you suggested in that repo?
- I just now realized that both implementations do never actually delete map entries. In Terraform this should not be a problem, as the whole process is short-lived, in the CCM the process could live for weeks/months. Do you have an idea how we can address it? I don't, at least not a trivial one :)
There was a problem hiding this comment.
any reason why you did not use the channel pattern you suggested in that repo?
the mutex_kv.go is a slightly different case: it used a loop with timer for waiting until the lock is there. It also supports cancellation with context to support terraform-native timeouts (and ctrl+c). It used TryLock for this and we changed it to a channel-based approach to get rid of the loop and timer and instead use "native" Go polling.
This case here is different and much simpler: we just acquire the lock and nobody else waits on it. We don't really need context cancellation here since it's running as a service. While it may happen that the parent context could be cancelled for some reason, the lock/unlock should not be the point where this gets respected (and hasn't been so far).
I just now realized that both implementations do never actually delete map entries. In Terraform this should not be a problem, as the whole process is short-lived, in the CCM the process could live for weeks/months. Do you have an idea how we can address it? I don't, at least not a trivial one :)
That's true and I tried to address this in the commit message:
Locks are not cleaned up on service deletion to avoid issues with late-arriving goroutines.
How often this happens is questionable but it avoids potential issues without much cost: Even large clusters won't have more than a couple loadbalancers. Keeping them around would be a couple 100 bytes (one entry is ~150B). Of course if somebody creates and deletes services 1000s of times it eventually could crash the CCM, but that would rather speak for a misuse of the system than anything else TBH.
Still, We could remove the entry for a service in two ways:
- once
EnsureLoadBalancerDeletedis successfully done - Add a goroutine which periodically removes unused mutexes. This would need to track which services still exist.
point 1 is rather trivial, I just wonder if there are any edge cases. I don't think so, but I didn't add this change because the benefit is not 100% clear to me.
Let me know what you think - happy to add the deletion logic if you feel it's valuable.
There was a problem hiding this comment.
As an aside: we could also use a plain map with an accompanying single mutex in this case. I think either approach would work well.
There was a problem hiding this comment.
FTR we discussed this offline and also considered using a simple sync.Mutex instead.
sync.Mutex can't be used because of the way the reconciliation works at this point: a single loadbalancer reconcile can take up to two minutes until it returns, fully blocking the mutex during that time.
Then we tried to reduce the scope of the Mutex, but because refetch is done not at the right places, the issue then can still happen.
We settled on the current approach with the intention to refactor reconcililation using api.RetryError instead of sleeps instead.
137ebee to
66acf8a
Compare
8fdb2f8 to
bbb8874
Compare
Prevents duplicate LB creation when multiple goroutines process the same service concurrently (e.g. EnsureLoadBalancer + UpdateLoadBalancer triggered by node sync). Uses sync.Map with service UID keys. Includes a failing unit test that reproduces the concurrent creation race.
no zone found results in an invalid create loadbalancer call because the zone is required. Until the CCM annotated the server, the zone of a server is not known and therefore the create lb call can't be made.
bbb8874 to
92f8e55
Compare
2 participants
Prevents duplicate LB creation when multiple goroutines process the same service concurrently (e.g. EnsureLoadBalancer + UpdateLoadBalancer triggered by node sync).
Uses sync.Map with service.UID keys.
Includes a failing unit test that reproduces the concurrent creation race.