cacher avoid double locking #117410
cacher avoid double locking #117410
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
area/apiserver
sig/api-machinery
p0lyn0mial
force-pushed
the
upstream-watcher-bookmark-rm
branch
from
2da8995
to
e4ee8fd
Compare
k8s-ci-robot
added
size/M
k8s-ci-robot
added
release-note-none
|
/retest |
|
/assign @wojtek-t |
k8s-ci-robot
added
triage/accepted
| @@ -205,7 +204,7 @@ func newTimeBucketWatchers(clock clock.Clock, bookmarkFrequency time.Duration) * | |||
|
|
|||
| // adds a watcher to the bucket, if the deadline is before the start, it will be | |||
| // added to the first one. | |||
| func (t *watcherBookmarkTimeBuckets) addWatcher(w *cacheWatcher) bool { | |||
| func (t *watcherBookmarkTimeBuckets) addWatcherThreadUnsafe(w *cacheWatcher) bool { | |||
There was a problem hiding this comment.
Can you also in this line:
https://github.com/kubernetes/kubernetes/blob/e4ee8fded488ee400f03862f83b3d46a3ec8a4c5/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go#L326
(where bookmarkWatchers field is defined)
add an additional comment, that this field when accessed should be protected by the Cacher.lock itself.
| defer cacher.bookmarkWatchers.lock.Unlock() | ||
| cacher.Lock() | ||
| cacher.bookmarkWatchers.popExpiredWatchersThreadUnsafe() | ||
| cacher.Unlock() |
There was a problem hiding this comment.
Why did you change this line?
Below we're also accessing bookmarkWatchers, so we should (as previously) do that under lock.
There was a problem hiding this comment.
If we don't acquire a lock here then we race with https://github.com/kubernetes/kubernetes/pull/117410/files#diff-14c4fb9f290753f50e0af4856d871ff41e2b520d747158cbff7bafc577fbb29eR929 which tics every second.
There was a problem hiding this comment.
You didn't understand my comment - we need to aquire a lock.
I'm just saying you shouldn't unlock here, just revert back to "defer unlock"
There was a problem hiding this comment.
ah, now I see it, popExpiredWatchersThreadUnsafe also accesses the bookmarkWatchers, good catch, thx!
it turns out that the watcherBookmarkTimeBuckets is called from only three places/methods: startDispatching, finishDispatching and Watch. All these methods acquire c.Lock() before touching watcherBookmarkTimeBuckets. Thus we could remove explicit locking in watcherBookmarkTimeBuckets since the access is already synced.
p0lyn0mial
force-pushed
the
upstream-watcher-bookmark-rm
branch
from
e4ee8fd
to
bb89d17
Compare
…oper synchronisation must be used
p0lyn0mial
force-pushed
the
upstream-watcher-bookmark-rm
branch
from
bb89d17
to
3a54489
Compare
wojtek-t
added
the
tide/merge-method-squash
|
/label tide/merge-method-squash /lgtm Thanks! |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: b4d90edec930edc7cbf5b83c665086c1d10b5591
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/kind cleanup |
k8s-ci-robot
added
kind/cleanup
* cacher: remove locking from watcherBookmarkTimeBuckets it turns out that the watcherBookmarkTimeBuckets is called from only three places/methods: startDispatching, finishDispatching and Watch. All these methods acquire c.Lock() before touching watcherBookmarkTimeBuckets. Thus we could remove explicit locking in watcherBookmarkTimeBuckets since the access is already synced. * cacher: rename watcherBookmarkTimeBuckets methods to indicate that proper synchronisation must be used
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
it turns out that the watcherBookmarkTimeBuckets
is called from only three places/methods: startDispatching, finishDispatching and Watch.
All these methods acquire c.Lock() before touching watcherBookmarkTimeBuckets.
Thus we could remove explicit locking in
watcherBookmarkTimeBuckets since the access is already synced.
xref: kubernetes/enhancements#3157
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: