Refine locking in API Priority and Fairness config controller #104833
Conversation
Instead of a plain `Mutex`, use an `RWMutex` so that the common operations can proceed in parallel.
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
sig/api-machinery
|
/priority important-soon |
k8s-ci-robot
added
priority/important-soon
|
Thanks Mike - let's see how much this will help and if we need to improve it further. /lgtm @mborsz - FYI |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MikeSpreitzer, 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 |
|
/triage accepted |
k8s-ci-robot
added
the
triage/accepted
k8s-ci-robot
removed
the
needs-triage
|
@MikeSpreitzer @wojtek-t What do you think about backporting this fix to all currently supported versions (>=1.20)? |
|
I think back-porting would be a good idea. |
|
Thanks, I've opened above cherrypick PRs. PTAL if that sgty. |
| // call and will be locked upon return. For a configController, the | ||
| // suffix "ReadLocked" stipulates a read lock while just "Locked" | ||
| // stipulates a full lock. Absence of either suffix means that either | ||
| // (a) the lock must NOT be held at call time and will not be held |
There was a problem hiding this comment.
It is not good for that to mean either of these two options, since they are contradictory.
There was a problem hiding this comment.
I look at it this way: using "Locked" gives some information. Not using "Locked" is the complement of that. Yes, it might be nice to distinguish cases in that complement. But do you really want to see more stuff added to more func names?
There was a problem hiding this comment.
Also, this runs into trouble when implementing an interface. You do not want an implementation distinction (between "doesn't care about the implementation's lock" and "must not hold the implementation's lock") to appear in the func names in the interface.
There was a problem hiding this comment.
I think it would be better to describe the situation like this: "For functions without either suffix, look at the function comment for any locking requirements."
There was a problem hiding this comment.
I think that's the obvious conclusion from the existing text, but would be fine with adding that explicit statement.
There was a problem hiding this comment.
How would you recommend proceeding, from the current state of the codebase and PRs?
There was a problem hiding this comment.
I don't think it's worth changing this since it already merged, I was just (attempting to) note that the comment isn't very helpful, but is written in language that makes it seem like it should be helpful.
…4833-upstream-release-1.21 Automated cherry pick of #104833 (1.21): Refine locking in API Priority and Fairness config controller
…4833-upstream-release-1.22 Automated cherry pick of #104833 (1.22): Refine locking in API Priority and Fairness config controller
…4833-upstream-release-1.20 Automated cherry pick of #104833 (1.20): Refine locking in API Priority and Fairness config controller
| 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.
is it correct that we're calling UpdateObservations() under a read lock?
There was a problem hiding this comment.
Yes. The locking is carefully documented. See https://github.com/kubernetes/apiserver/blob/3df972bf11afa6715f44c5a457392ba3ff7b597d/pkg/util/flowcontrol/apf_controller.go#L146-L154
…!770) cherrypick PR104833: Refine locking in API Priority and Fairness config controller cherrypick PR104833: Refine locking in API Priority and Fairness config controller kubernetes#104833
What type of PR is this?
/kind flake
What this PR does / why we need it:
Instead of a plain
Mutex, use anRWMutexso that the common operations can proceed in parallel.Which issue(s) this PR fixes:
Fixes #104811
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: