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
Automated cherry pick of #104833 (1.22): Refine locking in API Priority and Fairness config controller #105049
Automated cherry pick of #104833 (1.22): Refine locking in API Priority and Fairness config controller #105049
Conversation
Instead of a plain `Mutex`, use an `RWMutex` so that the common operations can proceed in parallel.
/kind bug |
// watchTracker implements the necessary WatchTracker interface. | ||
WatchTracker |
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.
This was wrongly added with the rebase, lemme fix it.
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.
Sorry, ignore above comment. I was looking at the cherrypick for older versions 1.21 and 1.20.
/test pull-kubernetes-integration |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MikeSpreitzer, shyamjvs 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 |
@shyamjvs : thanks for doing this |
Release managers note: we are proposing to cherry-pick this because it significantly reduces the lock contention in one of the request filters in the apiserver, thus making a performance improvement for all requests. |
@savitharaghunathan Given you were the release manager for 1.22, could you help approve this cherrypick? |
/triage accepted |
Thank you for the cherry picks @shyamjvs ! |
Why is tide not responding? Let's prod it. |
/unhold |
cfgCtlr.lock.Lock() | ||
defer cfgCtlr.lock.Unlock() | ||
cfgCtlr.lock.RLock() | ||
defer cfgCtlr.lock.RUnlock() | ||
for _, plc := range cfgCtlr.priorityLevelStates { | ||
if plc.queues != nil { | ||
plc.queues.UpdateObservations() |
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.
is it correct that we're calling UpdateObservations() under a read lock?
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.
Yes. See the writes into the observers are not what's covered by the write locking here.
Cherry pick of #104833 on release-1.22.
#104833: Refine locking in API Priority and Fairness config controller
For details on the cherry pick process, see the cherry pick requests page.