-
Notifications
You must be signed in to change notification settings - Fork 40.1k
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
Add logic to only call CPUManager Update() if state different than last Update() #101771
Add logic to only call CPUManager Update() if state different than last Update() #101771
Conversation
@klueska: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: klueska 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 |
Signed-off-by: Kevin Klues <kklues@nvidia.com>
09e93e9
to
6646039
Compare
/cc @nolancon |
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.
Makes sense & logic looks good.
One QQ on the unit testing - it looks like lastUpdateState
is always empty, so then when comparing cset
v lcset
, we always perform the update and existing test cases are exercised - just wondering should there be some nuance added to the lastUpdateState
? But that might be overkill - I'm happy to do a follow-up PR with some additional cases if you think it's worthwhile (I haven't contributed in a while! 😄)
Yeah, I didn't add any new unit tests (just made the old ones work). It'd be great if you did a follow up with some better testing around this, but I don't think it's strictly necessary for this one. |
/lgtm |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Currently, the CPUManager will call
Update()
into the container runtime to update itscset
on every container from every pod every 10 seconds (or whatever thereconcilePeriod
is set to, which is 10s by default). This patch updates this logic to only callUpdate()
if the container'scset
has changed since (1) it was first started, or (2) the last timeUpdate()
was called on it.This PR also has the side effect of mitigating issues caused by the following since calls to
Update()
trigger all cgroups to be updated (and not just thecset
cgroup):opencontainers/runc#2366 (comment)
Which issue(s) this PR fixes:
Fixes #100906
Does this PR introduce a user-facing change?