Skip to content
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

Merged

Conversation

klueska
Copy link
Contributor

@klueska klueska commented May 6, 2021

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 its cset on every container from every pod every 10 seconds (or whatever the reconcilePeriod is set to, which is 10s by default). This patch updates this logic to only call Update() if the container's cset has changed since (1) it was first started, or (2) the last time Update() 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 the cset cgroup):
opencontainers/runc#2366 (comment)

Which issue(s) this PR fixes:

Fixes #100906

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 6, 2021
@k8s-ci-robot
Copy link
Contributor

@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 triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 6, 2021
@k8s-ci-robot k8s-ci-robot requested review from mrunalp and vishh May 6, 2021 21:34
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2021
Signed-off-by: Kevin Klues <kklues@nvidia.com>
@klueska klueska force-pushed the upstream-only-uppdate-if-needed branch from 09e93e9 to 6646039 Compare May 6, 2021 21:38
@klueska
Copy link
Contributor Author

klueska commented May 9, 2021

/cc @nolancon

Copy link

@nolancon nolancon left a 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! 😄)

@klueska
Copy link
Contributor Author

klueska commented May 10, 2021

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.

@nolancon
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1604256 into kubernetes:master May 10, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The container’s CpusetCpus information needs to be added to the CRI Container definition
3 participants